[RFC PATCH 00/13] drm/msm: Add Display Stream Compression Support

[RFC PATCH 00/13] drm/msm: Add Display Stream Compression Support

[Vinod Koul]

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

[RFC PATCH 02/13] dt-bindings: msm/dsi: Document Display Stream Compression (DSC) parameters

[Vinod Koul]

DSC enables streams to be compressed before we send to panel. This
requires DSC enabled encoder and a panel to be present. So we add this
information in board DTS and find if DSC can be enabled and the
parameters required to configure DSC are added to binding document along
with example

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 .../devicetree/bindings/display/msm/dsi.txt       | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
index b9a64d3ff184..83d2fb92267e 100644
--- a/Documentation/devicetree/bindings/display/msm/dsi.txt
+++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
@@ -48,6 +48,13 @@ Optional properties:
 - pinctrl-n: the "sleep" pinctrl state
 - ports: contains DSI controller input and output ports as children, each
   containing one endpoint subnode.
+- qcom,mdss-dsc-enabled: Display Stream Compression (DSC) is enabled
+- qcom,mdss-slice-height: DSC slice height in pixels
+- qcom,mdss-slice-width: DSC slice width in pixels
+- qcom,mdss-slice-per-pkt: DSC slices per packet
+- qcom,mdss-bit-per-component: DSC bits per component
+- qcom,mdss-bit-per-pixel: DSC bits per pixel
+- qcom,mdss-block-prediction-enable: Block prediction mode of DSC enabled
 
   DSI Endpoint properties:
   - remote-endpoint: For port@0, set to phandle of the connected panel/bridge's
@@ -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;
+
 		qcom,mdss-mdp-transfer-time-us = <12000>;
 
 		pinctrl-names = "default", "sleep";
-- 
2.26.3

Re: [RFC PATCH 02/13] dt-bindings: msm/dsi: Document Display Stream Compression (DSC) parameters

[Rob Herring]

On Fri, May 21, 2021 at 7:50 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> DSC enables streams to be compressed before we send to panel. This
> requires DSC enabled encoder and a panel to be present. So we add this
> information in board DTS and find if DSC can be enabled and the
> parameters required to configure DSC are added to binding document along
> with example
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  .../devicetree/bindings/display/msm/dsi.txt       | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)

This is getting converted to schema. Hopefully, v17 will be it. Sigh.

Rob

Re: [RFC PATCH 02/13] dt-bindings: msm/dsi: Document Display Stream Compression (DSC) parameters

[Vinod Koul]

On 21-05-21, 08:18, Rob Herring wrote:
> On Fri, May 21, 2021 at 7:50 AM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > DSC enables streams to be compressed before we send to panel. This
> > requires DSC enabled encoder and a panel to be present. So we add this
> > information in board DTS and find if DSC can be enabled and the
> > parameters required to configure DSC are added to binding document along
> > with example
> >
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  .../devicetree/bindings/display/msm/dsi.txt       | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> 
> This is getting converted to schema. Hopefully, v17 will be it. Sigh.

I will update these on top, whenever that one gets merged... Any comments
on the parameters added here?

-- 
~Vinod

Re: [Freedreno] [RFC PATCH 00/13] drm/msm: Add Display Stream Compression Support

[Jeffrey Hugo]

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.

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.

Additionally, we define panel properties (resolution, BPP, etc) at the
panel, and have the display drivers pull it from the panel.  However,
for DSC, you do the reverse (define it in the display driver, and push
it to the panel).  If the argument is that DSC properties can be
dynamic, well, so can resolution.  Every panel for MSM MTPs supports
multiple resolutions, yet we define that with the panel in Linux.

Finally, I haven't seen the DT bits, but I'm concerned about using DT
for this.  It inherently excludes ACPI systems.  You appear to have
sdm845 support in this series, but what about ACPI boot on the Lenovo
C630 for example?  Or any of the 8cx laptops?  We don't read the panel
resolution, etc from DT, so why the DSC?

I'm glad that work is being done to add DSC to Linux, it's something I
struggled with when working on the 8998 mtp, and I realize this is a
bit of a drive-by review.  However, it seems like there should be a
better way.

>
> 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
>
> _______________________________________________
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [RFC PATCH 02/13] dt-bindings: msm/dsi: Document Display Stream Compression (DSC) parameters

[Bjorn Andersson]

On Fri 21 May 07:49 CDT 2021, Vinod Koul wrote:

> DSC enables streams to be compressed before we send to panel. This
> requires DSC enabled encoder and a panel to be present. So we add this
> information in board DTS and find if DSC can be enabled and the
> parameters required to configure DSC are added to binding document along
> with example
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  .../devicetree/bindings/display/msm/dsi.txt       | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> index b9a64d3ff184..83d2fb92267e 100644
> --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> @@ -48,6 +48,13 @@ Optional properties:
>  - pinctrl-n: the "sleep" pinctrl state
>  - ports: contains DSI controller input and output ports as children, each
>    containing one endpoint subnode.
> +- qcom,mdss-dsc-enabled: Display Stream Compression (DSC) is enabled
> +- qcom,mdss-slice-height: DSC slice height in pixels
> +- qcom,mdss-slice-width: DSC slice width in pixels
> +- qcom,mdss-slice-per-pkt: DSC slices per packet
> +- qcom,mdss-bit-per-component: DSC bits per component
> +- qcom,mdss-bit-per-pixel: DSC bits per pixel
> +- qcom,mdss-block-prediction-enable: Block prediction mode of DSC enabled
>  
>    DSI Endpoint properties:
>    - remote-endpoint: For port@0, set to phandle of the connected panel/bridge's
> @@ -188,6 +195,14 @@ Example:
>  		qcom,master-dsi;
>  		qcom,sync-dual-dsi;
>  
> +		qcom,mdss-dsc-enabled;

To me the activation of DSC seems to be a property of the panel.

> +		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;

Which of these properties relates to the DSC encoder and what needs to
be agreed with the sink? Can't we derive e.g. bpp from the information
we have from the attached panel already?

Regards,
Bjorn

> +
>  		qcom,mdss-mdp-transfer-time-us = <12000>;
>  
>  		pinctrl-names = "default", "sleep";
> -- 
> 2.26.3
> 

Re: [RFC PATCH 02/13] dt-bindings: msm/dsi: Document Display Stream Compression (DSC) parameters

[Vinod Koul]

On 21-05-21, 09:42, Bjorn Andersson wrote:
> On Fri 21 May 07:49 CDT 2021, Vinod Koul wrote:
> 
> > DSC enables streams to be compressed before we send to panel. This
> > requires DSC enabled encoder and a panel to be present. So we add this
> > information in board DTS and find if DSC can be enabled and the
> > parameters required to configure DSC are added to binding document along
> > with example
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  .../devicetree/bindings/display/msm/dsi.txt       | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > index b9a64d3ff184..83d2fb92267e 100644
> > --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> > +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > @@ -48,6 +48,13 @@ Optional properties:
> >  - pinctrl-n: the "sleep" pinctrl state
> >  - ports: contains DSI controller input and output ports as children, each
> >    containing one endpoint subnode.
> > +- qcom,mdss-dsc-enabled: Display Stream Compression (DSC) is enabled
> > +- qcom,mdss-slice-height: DSC slice height in pixels
> > +- qcom,mdss-slice-width: DSC slice width in pixels
> > +- qcom,mdss-slice-per-pkt: DSC slices per packet
> > +- qcom,mdss-bit-per-component: DSC bits per component
> > +- qcom,mdss-bit-per-pixel: DSC bits per pixel
> > +- qcom,mdss-block-prediction-enable: Block prediction mode of DSC enabled
> >  
> >    DSI Endpoint properties:
> >    - remote-endpoint: For port@0, set to phandle of the connected panel/bridge's
> > @@ -188,6 +195,14 @@ Example:
> >  		qcom,master-dsi;
> >  		qcom,sync-dual-dsi;
> >  
> > +		qcom,mdss-dsc-enabled;
> 
> To me the activation of DSC seems to be a property of the panel.

I think there are three parts to the problem
1. Panel needs to support it
2. Host needs to support it
3. Someone needs to decide to use when both the above conditions are
met.

There are cases where above 1, 2 will be satisfied, but we might be okay
without DSC too.. so how to decide when to do DSC :)

I feel it is more of a system property. And I also think that these
parameters here are host configuration and not really for panel...

> 
> > +		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;
> 
> Which of these properties relates to the DSC encoder and what needs to
> be agreed with the sink? Can't we derive e.g. bpp from the information
> we have from the attached panel already?

Let me go back and check on this a bit more

Thanks
-- 
~Vinod

Re: [RFC PATCH 02/13] dt-bindings: msm/dsi: Document Display Stream Compression (DSC) parameters

[Bjorn Andersson]

On Mon 24 May 02:30 CDT 2021, Vinod Koul wrote:

> On 21-05-21, 09:42, Bjorn Andersson wrote:
> > On Fri 21 May 07:49 CDT 2021, Vinod Koul wrote:
> > 
> > > DSC enables streams to be compressed before we send to panel. This
> > > requires DSC enabled encoder and a panel to be present. So we add this
> > > information in board DTS and find if DSC can be enabled and the
> > > parameters required to configure DSC are added to binding document along
> > > with example
> > > 
> > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > > ---
> > >  .../devicetree/bindings/display/msm/dsi.txt       | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > > index b9a64d3ff184..83d2fb92267e 100644
> > > --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> > > +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > > @@ -48,6 +48,13 @@ Optional properties:
> > >  - pinctrl-n: the "sleep" pinctrl state
> > >  - ports: contains DSI controller input and output ports as children, each
> > >    containing one endpoint subnode.
> > > +- qcom,mdss-dsc-enabled: Display Stream Compression (DSC) is enabled
> > > +- qcom,mdss-slice-height: DSC slice height in pixels
> > > +- qcom,mdss-slice-width: DSC slice width in pixels
> > > +- qcom,mdss-slice-per-pkt: DSC slices per packet
> > > +- qcom,mdss-bit-per-component: DSC bits per component
> > > +- qcom,mdss-bit-per-pixel: DSC bits per pixel
> > > +- qcom,mdss-block-prediction-enable: Block prediction mode of DSC enabled
> > >  
> > >    DSI Endpoint properties:
> > >    - remote-endpoint: For port@0, set to phandle of the connected panel/bridge's
> > > @@ -188,6 +195,14 @@ Example:
> > >  		qcom,master-dsi;
> > >  		qcom,sync-dual-dsi;
> > >  
> > > +		qcom,mdss-dsc-enabled;
> > 
> > To me the activation of DSC seems to be a property of the panel.
> 
> I think there are three parts to the problem
> 1. Panel needs to support it

In the case of DP there's bits to be read in the panel to figure this
out, for DSI panels this seems like a property that the panel (driver)
should know about.

> 2. Host needs to support it

Right, so this needs to be known by the driver. My suggestion is that we
derive it from the compatible or from the HW version.

> 3. Someone needs to decide to use when both the above conditions are
> met.
> 
> There are cases where above 1, 2 will be satisfied, but we might be okay
> without DSC too.. so how to decide when to do DSC :)
> 

Can we describe those cases? E.g. is it because enabling DSC would not
cause a reduction in clock speed that's worth the effort? Or do we only
use DSC for DSI when it allows us to squeeze everything into a single
link?

Regards,
Bjorn

> I feel it is more of a system property. And I also think that these
> parameters here are host configuration and not really for panel...
> 
> > 
> > > +		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;
> > 
> > Which of these properties relates to the DSC encoder and what needs to
> > be agreed with the sink? Can't we derive e.g. bpp from the information
> > we have from the attached panel already?
> 
> Let me go back and check on this a bit more
> 
> Thanks
> -- 
> ~Vinod

Re: [RFC PATCH 02/13] dt-bindings: msm/dsi: Document Display Stream Compression (DSC) parameters

[Vinod Koul]

On 24-05-21, 10:08, Bjorn Andersson wrote:
> On Mon 24 May 02:30 CDT 2021, Vinod Koul wrote:
> 
> > On 21-05-21, 09:42, Bjorn Andersson wrote:
> > > On Fri 21 May 07:49 CDT 2021, Vinod Koul wrote:
> > > 
> > > > DSC enables streams to be compressed before we send to panel. This
> > > > requires DSC enabled encoder and a panel to be present. So we add this
> > > > information in board DTS and find if DSC can be enabled and the
> > > > parameters required to configure DSC are added to binding document along
> > > > with example
> > > > 
> > > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > > > ---
> > > >  .../devicetree/bindings/display/msm/dsi.txt       | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > > > index b9a64d3ff184..83d2fb92267e 100644
> > > > --- a/Documentation/devicetree/bindings/display/msm/dsi.txt
> > > > +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt
> > > > @@ -48,6 +48,13 @@ Optional properties:
> > > >  - pinctrl-n: the "sleep" pinctrl state
> > > >  - ports: contains DSI controller input and output ports as children, each
> > > >    containing one endpoint subnode.
> > > > +- qcom,mdss-dsc-enabled: Display Stream Compression (DSC) is enabled
> > > > +- qcom,mdss-slice-height: DSC slice height in pixels
> > > > +- qcom,mdss-slice-width: DSC slice width in pixels
> > > > +- qcom,mdss-slice-per-pkt: DSC slices per packet
> > > > +- qcom,mdss-bit-per-component: DSC bits per component
> > > > +- qcom,mdss-bit-per-pixel: DSC bits per pixel
> > > > +- qcom,mdss-block-prediction-enable: Block prediction mode of DSC enabled
> > > >  
> > > >    DSI Endpoint properties:
> > > >    - remote-endpoint: For port@0, set to phandle of the connected panel/bridge's
> > > > @@ -188,6 +195,14 @@ Example:
> > > >  		qcom,master-dsi;
> > > >  		qcom,sync-dual-dsi;
> > > >  
> > > > +		qcom,mdss-dsc-enabled;
> > > 
> > > To me the activation of DSC seems to be a property of the panel.
> > 
> > I think there are three parts to the problem
> > 1. Panel needs to support it
> 
> In the case of DP there's bits to be read in the panel to figure this
> out, for DSI panels this seems like a property that the panel (driver)
> should know about.

Yes panel should know..

> 
> > 2. Host needs to support it
> 
> Right, so this needs to be known by the driver. My suggestion is that we
> derive it from the compatible or from the HW version.

Okay

> 
> > 3. Someone needs to decide to use when both the above conditions are
> > met.
> > 
> > There are cases where above 1, 2 will be satisfied, but we might be okay
> > without DSC too.. so how to decide when to do DSC :)
> > 
> 
> Can we describe those cases? E.g. is it because enabling DSC would not
> cause a reduction in clock speed that's worth the effort? Or do we only
> use DSC for DSI when it allows us to squeeze everything into a single
> link?

I really dont know how and when we should decide that :-|
One thing we can do is that if both support then always enable and get
benefit of getting power and clock speed reduction.

With this, who should have slice and bpp settings, panel or host?

Thanks
-- 
~Vinod

Re: [Freedreno] [RFC PATCH 00/13] drm/msm: Add Display Stream Compression Support

[Vinod Koul]

Hello Jeff,

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 didn't envision DSC to be a specific thing, most of
the patches here are hardware enabling ones for DSC bits for MSM
hardware.

> Additionally, we define panel properties (resolution, BPP, etc) at the
> panel, and have the display drivers pull it from the panel.  However,
> for DSC, you do the reverse (define it in the display driver, and push
> it to the panel).  If the argument is that DSC properties can be
> dynamic, well, so can resolution.  Every panel for MSM MTPs supports
> multiple resolutions, yet we define that with the panel in Linux.

I dont have an answer for that right now, to start with yes the
properties are in host but I am okay to discuss this and put wherever we
feel is most correct thing.  I somehow dont like that we should pull
from panel DT and program host with that. Here using struct
drm_dsc_config allows me to configure panel based on resolution passed

> Finally, I haven't seen the DT bits, but I'm concerned about using DT
> for this.  It inherently excludes ACPI systems.  You appear to have
> sdm845 support in this series, but what about ACPI boot on the Lenovo
> C630 for example?  Or any of the 8cx laptops?  We don't read the panel
> resolution, etc from DT, so why the DSC?

But you must read from somewhere like ACPI tables. I think ACPI systems
would have some ACPI table info out there which would help on this.
Yes that is another task which we need to start with once we enable OF
systems.

Thanks
-- 
~Vinod

Re: [Freedreno] [RFC PATCH 00/13] drm/msm: Add Display Stream Compression Support

[Jeffrey Hugo]

On Tue, May 25, 2021 at 11:46 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> Hello Jeff,
>
> 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'm surprised Rob Herring didn't have more to say on this.

> I didn't envision DSC to be a specific thing, most of
> the patches here are hardware enabling ones for DSC bits for MSM
> hardware.
>
> > Additionally, we define panel properties (resolution, BPP, etc) at the
> > panel, and have the display drivers pull it from the panel.  However,
> > for DSC, you do the reverse (define it in the display driver, and push
> > it to the panel).  If the argument is that DSC properties can be
> > dynamic, well, so can resolution.  Every panel for MSM MTPs supports
> > multiple resolutions, yet we define that with the panel in Linux.
>
> I dont have an answer for that right now, to start with yes the
> properties are in host but I am okay to discuss this and put wherever we
> feel is most correct thing.  I somehow dont like that we should pull
> from panel DT and program host with that. Here using struct
> drm_dsc_config allows me to configure panel based on resolution passed

I somewhat agree that pulling from the panel and programing the host
based on that is an odd solution, but we have it currently.  Have a
look at Documentation/devicetree/bindings/display/panel in particular
panel-timing.  All of that ends up informing the mdss programing
anyways (particularly the dsi and its phy).  So my problem is that we
currently have a solution that seems to just need to be extended, and
instead you have proposed a completely different solution which is
arguably contradictory.

However, I'd like to see thoughts from Rob Clark, David, and any
others that typically handle this stuff (maybe Sam Ravenborg from the
panel side?).  I consider them to be the experts, and if they think
your solution is the way to go, I'll shut up.  I consider myself to be
a novice that has dabbled in this area, and while this currently
doesn't make sense to me, maybe I need some education here to see the
light.

> > Finally, I haven't seen the DT bits, but I'm concerned about using DT
> > for this.  It inherently excludes ACPI systems.  You appear to have
> > sdm845 support in this series, but what about ACPI boot on the Lenovo
> > C630 for example?  Or any of the 8cx laptops?  We don't read the panel
> > resolution, etc from DT, so why the DSC?
>
> But you must read from somewhere like ACPI tables. I think ACPI systems
> would have some ACPI table info out there which would help on this.
> Yes that is another task which we need to start with once we enable OF
> systems.

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.

Generally we've defined simple-panel entities for these, with the
timings in code (you can see what Bjorn and I have upstreamed), and
just match on the compatible.

In summary, I don't mean to be difficult, I just think this solution
needs more "baking".

Re: [Freedreno] [RFC PATCH 00/13] drm/msm: Add Display Stream Compression Support

[Rob Clark]

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:
> >
> > Hello Jeff,
> >
> > 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'm surprised Rob Herring didn't have more to say on this.
>
> > I didn't envision DSC to be a specific thing, most of
> > the patches here are hardware enabling ones for DSC bits for MSM
> > hardware.
> >
> > > Additionally, we define panel properties (resolution, BPP, etc) at the
> > > panel, and have the display drivers pull it from the panel.  However,
> > > for DSC, you do the reverse (define it in the display driver, and push
> > > it to the panel).  If the argument is that DSC properties can be
> > > dynamic, well, so can resolution.  Every panel for MSM MTPs supports
> > > multiple resolutions, yet we define that with the panel in Linux.
> >
> > I dont have an answer for that right now, to start with yes the
> > properties are in host but I am okay to discuss this and put wherever we
> > feel is most correct thing.  I somehow dont like that we should pull
> > from panel DT and program host with that. Here using struct
> > drm_dsc_config allows me to configure panel based on resolution passed
>
> I somewhat agree that pulling from the panel and programing the host
> based on that is an odd solution, but we have it currently.  Have a
> look at Documentation/devicetree/bindings/display/panel in particular
> panel-timing.  All of that ends up informing the mdss programing
> anyways (particularly the dsi and its phy).  So my problem is that we
> currently have a solution that seems to just need to be extended, and
> instead you have proposed a completely different solution which is
> arguably contradictory.
>
> However, I'd like to see thoughts from Rob Clark, David, and any
> others that typically handle this stuff (maybe Sam Ravenborg from the
> panel side?).  I consider them to be the experts, and if they think
> your solution is the way to go, I'll shut up.  I consider myself to be
> a novice that has dabbled in this area, and while this currently
> doesn't make sense to me, maybe I need some education here to see the
> light.
>
> > > Finally, I haven't seen the DT bits, but I'm concerned about using DT
> > > for this.  It inherently excludes ACPI systems.  You appear to have
> > > sdm845 support in this series, but what about ACPI boot on the Lenovo
> > > C630 for example?  Or any of the 8cx laptops?  We don't read the panel
> > > resolution, etc from DT, so why the DSC?
> >
> > But you must read from somewhere like ACPI tables. I think ACPI systems
> > would have some ACPI table info out there which would help on this.
> > Yes that is another task which we need to start with once we enable OF
> > systems.
>
> 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.

I'd worry more about what makes sense in a DT world, when it comes to
DT bindings.

BR,
-R

> Generally we've defined simple-panel entities for these, with the
> timings in code (you can see what Bjorn and I have upstreamed), and
> just match on the compatible.
>
> In summary, I don't mean to be difficult, I just think this solution
> needs more "baking".

Re: [Freedreno] [RFC PATCH 00/13] drm/msm: Add Display Stream Compression Support

[Vinod Koul]

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

Re: [Freedreno] [RFC PATCH 00/13] drm/msm: Add Display Stream Compression Support

[Vinod Koul]

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..?

-- 
~Vinod

Re: [Freedreno] [RFC PATCH 00/13] drm/msm: Add Display Stream Compression Support

[abhinavk]

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

Re: [Freedreno] [RFC PATCH 00/13] drm/msm: Add Display Stream Compression Support

[Rob Clark]

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

Re: [Freedreno] [RFC PATCH 00/13] drm/msm: Add Display Stream Compression Support

[Vinod Koul]

On 03-06-21, 16:40, abhinavk@codeaurora.org wrote:
> On 2021-06-02 04:01, Vinod Koul wrote:
> > On 27-05-21, 16:30, Rob Clark wrote:
> > 
> > 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:

Okay so we have discussed this w/ Bjorn and Abhinav and here are the
conclusions and recommendations for binding

1. the properties are generic and not msm specific
2. The host supports multiple formats but the one we choose depends
mostly upon panel. Notably host runs the config which the panel supports.

So the recommendations is to add a table of dsc properties in the panel
driver. No DT binding here.

I should also note that for DP we should be able to calculate these
values from EDID like the i915 driver seems to do

With this I will drop the binding patch and move dsc properties to panel
driver

Thanks

-- 
~Vinod