| Message ID | 20210521124946.3617862-1-vkoul@kernel.org |
|---|---|
| Headers | show |
| Series | drm/msm: Add Display Stream Compression Support | expand |
On 26-05-21, 09:00, Jeffrey Hugo wrote: > On Tue, May 25, 2021 at 11:46 PM Vinod Koul <vkoul@kernel.org> wrote: > > On 21-05-21, 08:09, Jeffrey Hugo wrote: > > > On Fri, May 21, 2021 at 6:50 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > > > > > Display Stream Compression (DSC) compresses the display stream in host which > > > > is later decoded by panel. This series enables this for Qualcomm msm driver. > > > > This was tested on Google Pixel3 phone which use LGE SW43408 panel. > > > > > > > > The changes include adding DT properties for DSC then hardware blocks support > > > > required in DPU1 driver and support in encoder. We also add support in DSI > > > > and introduce required topology changes. > > > > > > > > In order for panel to set the DSC parameters we add dsc in drm_panel and set > > > > it from the msm driver. > > > > > > > > Complete changes which enable this for Pixel3 along with panel driver (not > > > > part of this series) and DT changes can be found at: > > > > git.linaro.org/people/vinod.koul/kernel.git pixel/dsc_rfc > > > > > > > > Comments welcome! > > > > > > This feels backwards to me. I've only skimmed this series, and the DT > > > changes didn't come through for me, so perhaps I have an incomplete > > > view. > > > > Not sure why, I see it on lore: > > https://lore.kernel.org/dri-devel/20210521124946.3617862-3-vkoul@kernel.org/ > > > > > DSC is not MSM specific. There is a standard for it. Yet it looks > > > like everything is implemented in a MSM specific way, and then pushed > > > to the panel. So, every vendor needs to implement their vendor > > > specific way to get the DSC info, and then push it to the panel? > > > Seems wrong, given there is an actual standard for this feature. > > > > I have added slice and bpp info in the DT here under the host and then > > pass the generic struct drm_dsc_config to panel which allows panel to > > write the pps cmd > > > > Nothing above is MSM specific.. It can very well work with non MSM > > controllers too. > > I disagree. > > The DT bindings you defined (thanks for the direct link) are MSM > specific. I'm not talking (yet) about the properties you defined, but > purely from the stand point that you defined the binding within the > scope of the MSM dsi binding. No other vendor can use those bindings. > Of course, if we look at the properties themselves, they are prefixed > with "qcom", which is vendor specific. > > So, purely on the face of it, this is MSM specific. > > Assuming we want a DT solution for DSC, I think it should be something > like Documentation/devicetree/bindings/clock/clock-bindings.txt (the > first example that comes to mind), which is a non-vendor specific > generic set of properties that each vendor/device specific binding can > inherit. Panel has similar things. > > Specific to the properties, I don't much like that you duplicate BPP, > which is already associated with the panel (although perhaps not in > the scope of DT). What if the panel and your DSC bindings disagree? > Also, I guess I need to ask, have you read the DSC spec? Last I > looked, there were something like 3 dozen properties that could be > configured. You have five in your proposed binding. To me, this is > not a generic DSC solution, this is MSM specific (and frankly I don't > think this supports all the configuration the MSM hardware can do, > either). I would concede the point that DT is msm specific. I dont disagree on making them a common dsc biding which anyone can use. I think your idea does have merits... I am still not sure who should include these properties, would it be the panel or host. Either would work and rest of the system can use these properties... -- ~Vinod
On 2021-06-02 04:01, Vinod Koul wrote: > On 27-05-21, 16:30, Rob Clark wrote: >> On Wed, May 26, 2021 at 8:00 AM Jeffrey Hugo >> <jeffrey.l.hugo@gmail.com> wrote: >> > On Tue, May 25, 2021 at 11:46 PM Vinod Koul <vkoul@kernel.org> wrote: > >> > Frankly, I don't like the MSM ACPI solution that I've seen on the laptops. >> > The ACPI assumes the entire MDSS (including DSI parts) and GPU is one >> > device, and ultimately handled by one driver. That driver needs to >> > get a value from UEFI (set by the bootloader) that is the "panel id". >> > Then the driver calls into ACPI (I think its _ROM, but I might be >> > mistaken, doing this from memory) with that id. It gets back a binary >> > blob which is mostly an xml file (format is publicly documented) that >> > contains the panel timings and such. >> >> tbh, I kinda suspect that having a single "gpu" device (which also >> includes venus, in addition to display, IIRC) in the ACPI tables is a >> windowsism, trying to make things look to userspace like a single "GPU >> card" in the x86 world.. but either way, I think the ACPI tables on >> the windows arm laptops which use dsi->bridge->edp is too much of a >> lost cause to even consider here. Possibly ACPI boot on these devices >> would be more feasible on newer devices which have direct eDP out of >> the SoC without requiring external bridge/panel glue. > > yeah that is always a very different world. although it might make > sense > to use information in tables and try to deduce information about the > system can be helpful... > >> I'd worry more about what makes sense in a DT world, when it comes to >> DT bindings. > > And do you have thoughts on that..? At the moment, I will comment on the bindings first and my idea on how to proceed. The bindings mentioned here: https://lore.kernel.org/dri-devel/20210521124946.3617862-3-vkoul@kernel.org/ seem to be just taken directly from downstream which was not the plan. I think all of these should be part of the generic panel bindings as none of these are QC specific: @@ -188,6 +195,14 @@ Example: qcom,master-dsi; qcom,sync-dual-dsi; + qcom,mdss-dsc-enabled; + qcom,mdss-slice-height = <16>; + qcom,mdss-slice-width = <540>; + qcom,mdss-slice-per-pkt = <1>; + qcom,mdss-bit-per-component = <8>; + qcom,mdss-bit-per-pixel = <8>; + qcom,mdss-block-prediction-enable; + How about having a panel-dsc.yaml which will have these properties and have a panel-dsc node to have this information? I would like to hear the feedback on this proposal then the series can be reworked. Thanks Abhinav
On Wed, Jun 2, 2021 at 4:01 AM Vinod Koul <vkoul@kernel.org> wrote: > > On 27-05-21, 16:30, Rob Clark wrote: > > On Wed, May 26, 2021 at 8:00 AM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote: > > > On Tue, May 25, 2021 at 11:46 PM Vinod Koul <vkoul@kernel.org> wrote: > > > > Frankly, I don't like the MSM ACPI solution that I've seen on the laptops. > > > The ACPI assumes the entire MDSS (including DSI parts) and GPU is one > > > device, and ultimately handled by one driver. That driver needs to > > > get a value from UEFI (set by the bootloader) that is the "panel id". > > > Then the driver calls into ACPI (I think its _ROM, but I might be > > > mistaken, doing this from memory) with that id. It gets back a binary > > > blob which is mostly an xml file (format is publicly documented) that > > > contains the panel timings and such. > > > > tbh, I kinda suspect that having a single "gpu" device (which also > > includes venus, in addition to display, IIRC) in the ACPI tables is a > > windowsism, trying to make things look to userspace like a single "GPU > > card" in the x86 world.. but either way, I think the ACPI tables on > > the windows arm laptops which use dsi->bridge->edp is too much of a > > lost cause to even consider here. Possibly ACPI boot on these devices > > would be more feasible on newer devices which have direct eDP out of > > the SoC without requiring external bridge/panel glue. > > yeah that is always a very different world. although it might make sense > to use information in tables and try to deduce information about the > system can be helpful... > > > I'd worry more about what makes sense in a DT world, when it comes to > > DT bindings. > > And do you have thoughts on that..? Only that I wouldn't get too hung up on existing snapdragon ACPI tables.. not sure if there is prior art as far as ACPI tables for this on x86 systems, if so that *might* be a thing to consider, but otherwise it does sound a bit like we want less qcom specific bindings here. But other than that I'll leave it to folks who spend more time thinking about bindings.. left to my own devices I'd come up with a point solution and go back to working on mesa, so that probably isn't the opinion you want to follow ;-) BR, -R
Display Stream Compression (DSC) compresses the display stream in host which is later decoded by panel. This series enables this for Qualcomm msm driver. This was tested on Google Pixel3 phone which use LGE SW43408 panel. The changes include adding DT properties for DSC then hardware blocks support required in DPU1 driver and support in encoder. We also add support in DSI and introduce required topology changes. In order for panel to set the DSC parameters we add dsc in drm_panel and set it from the msm driver. Complete changes which enable this for Pixel3 along with panel driver (not part of this series) and DT changes can be found at: git.linaro.org/people/vinod.koul/kernel.git pixel/dsc_rfc Comments welcome! Vinod Koul (13): drm/dsc: Add dsc pps header init function dt-bindings: msm/dsi: Document Display Stream Compression (DSC) parameters drm/msm/dsi: add support for dsc data drm/msm/disp/dpu1: Add support for DSC drm/msm/disp/dpu1: Add support for DSC in pingpong block drm/msm/disp/dpu1: Add DSC support in RM drm/msm/disp/dpu1: Add DSC for SDM845 to hw_catalog drm/msm/disp/dpu1: Add DSC support in hw_ctl drm/msm/disp/dpu1: Don't use DSC with mode_3d drm/msm/disp/dpu1: Add support for DSC in encoder drm/msm/disp/dpu1: Add support for DSC in topology drm/msm/dsi: Add support for DSC configuration drm/msm/dsi: Pass DSC params to drm_panel .../devicetree/bindings/display/msm/dsi.txt | 15 + drivers/gpu/drm/drm_dsc.c | 11 + drivers/gpu/drm/msm/Makefile | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 204 +++++++++++- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h | 11 + .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 2 + .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 22 ++ .../gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 26 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 12 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.h | 2 + drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 221 +++++++++++++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 79 +++++ drivers/gpu/drm/msm/disp/dpu1/dpu_hw_mdss.h | 13 + .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 32 ++ .../gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.h | 14 + drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 + drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 32 ++ drivers/gpu/drm/msm/disp/dpu1/dpu_rm.h | 1 + drivers/gpu/drm/msm/dsi/dsi.xml.h | 10 + drivers/gpu/drm/msm/dsi/dsi_host.c | 293 +++++++++++++++++- drivers/gpu/drm/msm/msm_drv.h | 32 ++ include/drm/drm_dsc.h | 16 + include/drm/drm_panel.h | 7 + 23 files changed, 1043 insertions(+), 14 deletions(-) create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c create mode 100644 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h -- 2.26.3