[PATCH 01/11] drm/i915/dp: Fix DSC line buffer depth programming

[PATCH 01/11] drm/i915/dp: Fix DSC line buffer depth programming

[Imre Deak]

Fix the calculation of the DSC line buffer depth. This is limited both
by the source's and sink's maximum line buffer depth, but the former one
was not taken into account. On all Intel platform's the source's maximum
buffer depth is 13, so the overall limit is simply the minimum of the
source/sink's limit, regardless of the DSC version.

This leaves the DSI DSC line buffer depth calculation as-is, trusting
VBT.

On DSC version 1.2 for sinks reporting a maximum line buffer depth of 16
the line buffer depth was incorrectly programmed as 0, leading to a
corruption in color gradients / lines on the decompressed screen image.

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 16 ++++++----------
 include/drm/display/drm_dsc.h           |  3 ---
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index af7ca00e9bc0a..dbe65651bf277 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -89,6 +89,9 @@
 #define DP_DSC_MAX_ENC_THROUGHPUT_0		340000
 #define DP_DSC_MAX_ENC_THROUGHPUT_1		400000
 
+/* Max DSC line buffer depth supported by HW. */
+#define INTEL_DP_DSC_MAX_LINE_BUF_DEPTH		13
+
 /* DP DSC FEC Overhead factor in ppm = 1/(0.972261) = 1.028530 */
 #define DP_DSC_FEC_OVERHEAD_FACTOR		1028530
 
@@ -1703,7 +1706,6 @@ static int intel_dp_dsc_compute_params(const struct intel_connector *connector,
 {
 	struct drm_i915_private *i915 = to_i915(connector->base.dev);
 	struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
-	u8 line_buf_depth;
 	int ret;
 
 	/*
@@ -1732,20 +1734,14 @@ static int intel_dp_dsc_compute_params(const struct intel_connector *connector,
 			connector->dp.dsc_dpcd[DP_DSC_DEC_COLOR_FORMAT_CAP - DP_DSC_SUPPORT] &
 			DP_DSC_RGB;
 
-	line_buf_depth = drm_dp_dsc_sink_line_buf_depth(connector->dp.dsc_dpcd);
-	if (!line_buf_depth) {
+	vdsc_cfg->line_buf_depth = min(INTEL_DP_DSC_MAX_LINE_BUF_DEPTH,
+				       drm_dp_dsc_sink_line_buf_depth(connector->dp.dsc_dpcd));
+	if (!vdsc_cfg->line_buf_depth) {
 		drm_dbg_kms(&i915->drm,
 			    "DSC Sink Line Buffer Depth invalid\n");
 		return -EINVAL;
 	}
 
-	if (vdsc_cfg->dsc_version_minor == 2)
-		vdsc_cfg->line_buf_depth = (line_buf_depth == DSC_1_2_MAX_LINEBUF_DEPTH_BITS) ?
-			DSC_1_2_MAX_LINEBUF_DEPTH_VAL : line_buf_depth;
-	else
-		vdsc_cfg->line_buf_depth = (line_buf_depth > DSC_1_1_MAX_LINEBUF_DEPTH_BITS) ?
-			DSC_1_1_MAX_LINEBUF_DEPTH_BITS : line_buf_depth;
-
 	vdsc_cfg->block_pred_enable =
 		connector->dp.dsc_dpcd[DP_DSC_BLK_PREDICTION_SUPPORT - DP_DSC_SUPPORT] &
 		DP_DSC_BLK_PREDICTION_IS_SUPPORTED;
diff --git a/include/drm/display/drm_dsc.h b/include/drm/display/drm_dsc.h
index bc90273d06a62..bbbe7438473d3 100644
--- a/include/drm/display/drm_dsc.h
+++ b/include/drm/display/drm_dsc.h
@@ -40,9 +40,6 @@
 #define DSC_PPS_RC_RANGE_MINQP_SHIFT		11
 #define DSC_PPS_RC_RANGE_MAXQP_SHIFT		6
 #define DSC_PPS_NATIVE_420_SHIFT		1
-#define DSC_1_2_MAX_LINEBUF_DEPTH_BITS		16
-#define DSC_1_2_MAX_LINEBUF_DEPTH_VAL		0
-#define DSC_1_1_MAX_LINEBUF_DEPTH_BITS		13
 
 /**
  * struct drm_dsc_rc_range_parameters - DSC Rate Control range parameters
-- 
2.43.3

Re: [PATCH 01/11] drm/i915/dp: Fix DSC line buffer depth programming

[Nautiyal, Ankit K]

On 3/21/2024 1:41 AM, Imre Deak wrote:
> Fix the calculation of the DSC line buffer depth. This is limited both
> by the source's and sink's maximum line buffer depth, but the former one
> was not taken into account. On all Intel platform's the source's maximum
> buffer depth is 13, so the overall limit is simply the minimum of the
> source/sink's limit, regardless of the DSC version.
>
> This leaves the DSI DSC line buffer depth calculation as-is, trusting
> VBT.
>
> On DSC version 1.2 for sinks reporting a maximum line buffer depth of 16
> the line buffer depth was incorrectly programmed as 0, leading to a
> corruption in color gradients / lines on the decompressed screen image.
>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>

LGTM.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

> ---
>   drivers/gpu/drm/i915/display/intel_dp.c | 16 ++++++----------
>   include/drm/display/drm_dsc.h           |  3 ---
>   2 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index af7ca00e9bc0a..dbe65651bf277 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -89,6 +89,9 @@
>   #define DP_DSC_MAX_ENC_THROUGHPUT_0		340000
>   #define DP_DSC_MAX_ENC_THROUGHPUT_1		400000
>   
> +/* Max DSC line buffer depth supported by HW. */
> +#define INTEL_DP_DSC_MAX_LINE_BUF_DEPTH		13
> +
>   /* DP DSC FEC Overhead factor in ppm = 1/(0.972261) = 1.028530 */
>   #define DP_DSC_FEC_OVERHEAD_FACTOR		1028530
>   
> @@ -1703,7 +1706,6 @@ static int intel_dp_dsc_compute_params(const struct intel_connector *connector,
>   {
>   	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>   	struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
> -	u8 line_buf_depth;
>   	int ret;
>   
>   	/*
> @@ -1732,20 +1734,14 @@ static int intel_dp_dsc_compute_params(const struct intel_connector *connector,
>   			connector->dp.dsc_dpcd[DP_DSC_DEC_COLOR_FORMAT_CAP - DP_DSC_SUPPORT] &
>   			DP_DSC_RGB;
>   
> -	line_buf_depth = drm_dp_dsc_sink_line_buf_depth(connector->dp.dsc_dpcd);
> -	if (!line_buf_depth) {
> +	vdsc_cfg->line_buf_depth = min(INTEL_DP_DSC_MAX_LINE_BUF_DEPTH,
> +				       drm_dp_dsc_sink_line_buf_depth(connector->dp.dsc_dpcd));
> +	if (!vdsc_cfg->line_buf_depth) {
>   		drm_dbg_kms(&i915->drm,
>   			    "DSC Sink Line Buffer Depth invalid\n");
>   		return -EINVAL;
>   	}
>   
> -	if (vdsc_cfg->dsc_version_minor == 2)
> -		vdsc_cfg->line_buf_depth = (line_buf_depth == DSC_1_2_MAX_LINEBUF_DEPTH_BITS) ?
> -			DSC_1_2_MAX_LINEBUF_DEPTH_VAL : line_buf_depth;
> -	else
> -		vdsc_cfg->line_buf_depth = (line_buf_depth > DSC_1_1_MAX_LINEBUF_DEPTH_BITS) ?
> -			DSC_1_1_MAX_LINEBUF_DEPTH_BITS : line_buf_depth;
> -
>   	vdsc_cfg->block_pred_enable =
>   		connector->dp.dsc_dpcd[DP_DSC_BLK_PREDICTION_SUPPORT - DP_DSC_SUPPORT] &
>   		DP_DSC_BLK_PREDICTION_IS_SUPPORTED;
> diff --git a/include/drm/display/drm_dsc.h b/include/drm/display/drm_dsc.h
> index bc90273d06a62..bbbe7438473d3 100644
> --- a/include/drm/display/drm_dsc.h
> +++ b/include/drm/display/drm_dsc.h
> @@ -40,9 +40,6 @@
>   #define DSC_PPS_RC_RANGE_MINQP_SHIFT		11
>   #define DSC_PPS_RC_RANGE_MAXQP_SHIFT		6
>   #define DSC_PPS_NATIVE_420_SHIFT		1
> -#define DSC_1_2_MAX_LINEBUF_DEPTH_BITS		16
> -#define DSC_1_2_MAX_LINEBUF_DEPTH_VAL		0
> -#define DSC_1_1_MAX_LINEBUF_DEPTH_BITS		13
>   
>   /**
>    * struct drm_dsc_rc_range_parameters - DSC Rate Control range parameters

Re: [PATCH 01/11] drm/i915/dp: Fix DSC line buffer depth programming

[Manasi Navare]

Hi Imre,

Thanks for the DSC fixes.
Would the line buf depth calculation that was getting set to 0 impact
DSC on all platforms
or was this issue only specific to MTL and was getting set correctly
with older platforms?
We didnt notice any DSC issues/corruptions with ADL based systems.

The actual change makes sense, just want to confirm if this applies to
all platforms or any particular?
With that clarification:

Reviewed-by: Manasi Navare <navaremanasi@chromium.org>

Regards
Manasi

On Tue, Mar 26, 2024 at 3:01 AM Nautiyal, Ankit K
<ankit.k.nautiyal@intel.com> wrote:
>
>
> On 3/21/2024 1:41 AM, Imre Deak wrote:
> > Fix the calculation of the DSC line buffer depth. This is limited both
> > by the source's and sink's maximum line buffer depth, but the former one
> > was not taken into account. On all Intel platform's the source's maximum
> > buffer depth is 13, so the overall limit is simply the minimum of the
> > source/sink's limit, regardless of the DSC version.
> >
> > This leaves the DSI DSC line buffer depth calculation as-is, trusting
> > VBT.
> >
> > On DSC version 1.2 for sinks reporting a maximum line buffer depth of 16
> > the line buffer depth was incorrectly programmed as 0, leading to a
> > corruption in color gradients / lines on the decompressed screen image.
> >
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>
> LGTM.
>
> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>
> > ---
> >   drivers/gpu/drm/i915/display/intel_dp.c | 16 ++++++----------
> >   include/drm/display/drm_dsc.h           |  3 ---
> >   2 files changed, 6 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index af7ca00e9bc0a..dbe65651bf277 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -89,6 +89,9 @@
> >   #define DP_DSC_MAX_ENC_THROUGHPUT_0         340000
> >   #define DP_DSC_MAX_ENC_THROUGHPUT_1         400000
> >
> > +/* Max DSC line buffer depth supported by HW. */
> > +#define INTEL_DP_DSC_MAX_LINE_BUF_DEPTH              13
> > +
> >   /* DP DSC FEC Overhead factor in ppm = 1/(0.972261) = 1.028530 */
> >   #define DP_DSC_FEC_OVERHEAD_FACTOR          1028530
> >
> > @@ -1703,7 +1706,6 @@ static int intel_dp_dsc_compute_params(const struct intel_connector *connector,
> >   {
> >       struct drm_i915_private *i915 = to_i915(connector->base.dev);
> >       struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
> > -     u8 line_buf_depth;
> >       int ret;
> >
> >       /*
> > @@ -1732,20 +1734,14 @@ static int intel_dp_dsc_compute_params(const struct intel_connector *connector,
> >                       connector->dp.dsc_dpcd[DP_DSC_DEC_COLOR_FORMAT_CAP - DP_DSC_SUPPORT] &
> >                       DP_DSC_RGB;
> >
> > -     line_buf_depth = drm_dp_dsc_sink_line_buf_depth(connector->dp.dsc_dpcd);
> > -     if (!line_buf_depth) {
> > +     vdsc_cfg->line_buf_depth = min(INTEL_DP_DSC_MAX_LINE_BUF_DEPTH,
> > +                                    drm_dp_dsc_sink_line_buf_depth(connector->dp.dsc_dpcd));
> > +     if (!vdsc_cfg->line_buf_depth) {
> >               drm_dbg_kms(&i915->drm,
> >                           "DSC Sink Line Buffer Depth invalid\n");
> >               return -EINVAL;
> >       }
> >
> > -     if (vdsc_cfg->dsc_version_minor == 2)
> > -             vdsc_cfg->line_buf_depth = (line_buf_depth == DSC_1_2_MAX_LINEBUF_DEPTH_BITS) ?
> > -                     DSC_1_2_MAX_LINEBUF_DEPTH_VAL : line_buf_depth;
> > -     else
> > -             vdsc_cfg->line_buf_depth = (line_buf_depth > DSC_1_1_MAX_LINEBUF_DEPTH_BITS) ?
> > -                     DSC_1_1_MAX_LINEBUF_DEPTH_BITS : line_buf_depth;
> > -
> >       vdsc_cfg->block_pred_enable =
> >               connector->dp.dsc_dpcd[DP_DSC_BLK_PREDICTION_SUPPORT - DP_DSC_SUPPORT] &
> >               DP_DSC_BLK_PREDICTION_IS_SUPPORTED;
> > diff --git a/include/drm/display/drm_dsc.h b/include/drm/display/drm_dsc.h
> > index bc90273d06a62..bbbe7438473d3 100644
> > --- a/include/drm/display/drm_dsc.h
> > +++ b/include/drm/display/drm_dsc.h
> > @@ -40,9 +40,6 @@
> >   #define DSC_PPS_RC_RANGE_MINQP_SHIFT                11
> >   #define DSC_PPS_RC_RANGE_MAXQP_SHIFT                6
> >   #define DSC_PPS_NATIVE_420_SHIFT            1
> > -#define DSC_1_2_MAX_LINEBUF_DEPTH_BITS               16
> > -#define DSC_1_2_MAX_LINEBUF_DEPTH_VAL                0
> > -#define DSC_1_1_MAX_LINEBUF_DEPTH_BITS               13
> >
> >   /**
> >    * struct drm_dsc_rc_range_parameters - DSC Rate Control range parameters

Re: [PATCH 01/11] drm/i915/dp: Fix DSC line buffer depth programming

[Imre Deak]

On Tue, Mar 26, 2024 at 12:50:17PM -0700, Manasi Navare wrote:
Hi,

> Hi Imre,
> 
> Thanks for the DSC fixes.
>
> Would the line buf depth calculation that was getting set to 0 impact
> DSC on all platforms or was this issue only specific to MTL and was
> getting set correctly with older platforms?

Only those configs are affected where both the source and the sink supports
DSC1.2, so ADL is not affected.

> We didnt notice any DSC issues/corruptions with ADL based systems.

Yes, that makes sense.

> The actual change makes sense, just want to confirm if this applies to
> all platforms or any particular?

The change will make a difference only on MTL+.

> With that clarification:
> 
> Reviewed-by: Manasi Navare <navaremanasi@chromium.org>

Thanks.

> Regards
> Manasi
> 
> On Tue, Mar 26, 2024 at 3:01 AM Nautiyal, Ankit K
> <ankit.k.nautiyal@intel.com> wrote:
> >
> >
> > On 3/21/2024 1:41 AM, Imre Deak wrote:
> > > Fix the calculation of the DSC line buffer depth. This is limited both
> > > by the source's and sink's maximum line buffer depth, but the former one
> > > was not taken into account. On all Intel platform's the source's maximum
> > > buffer depth is 13, so the overall limit is simply the minimum of the
> > > source/sink's limit, regardless of the DSC version.
> > >
> > > This leaves the DSI DSC line buffer depth calculation as-is, trusting
> > > VBT.
> > >
> > > On DSC version 1.2 for sinks reporting a maximum line buffer depth of 16
> > > the line buffer depth was incorrectly programmed as 0, leading to a
> > > corruption in color gradients / lines on the decompressed screen image.
> > >
> > > Cc: dri-devel@lists.freedesktop.org
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> >
> > LGTM.
> >
> > Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >
> > > ---
> > >   drivers/gpu/drm/i915/display/intel_dp.c | 16 ++++++----------
> > >   include/drm/display/drm_dsc.h           |  3 ---
> > >   2 files changed, 6 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index af7ca00e9bc0a..dbe65651bf277 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -89,6 +89,9 @@
> > >   #define DP_DSC_MAX_ENC_THROUGHPUT_0         340000
> > >   #define DP_DSC_MAX_ENC_THROUGHPUT_1         400000
> > >
> > > +/* Max DSC line buffer depth supported by HW. */
> > > +#define INTEL_DP_DSC_MAX_LINE_BUF_DEPTH              13
> > > +
> > >   /* DP DSC FEC Overhead factor in ppm = 1/(0.972261) = 1.028530 */
> > >   #define DP_DSC_FEC_OVERHEAD_FACTOR          1028530
> > >
> > > @@ -1703,7 +1706,6 @@ static int intel_dp_dsc_compute_params(const struct intel_connector *connector,
> > >   {
> > >       struct drm_i915_private *i915 = to_i915(connector->base.dev);
> > >       struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
> > > -     u8 line_buf_depth;
> > >       int ret;
> > >
> > >       /*
> > > @@ -1732,20 +1734,14 @@ static int intel_dp_dsc_compute_params(const struct intel_connector *connector,
> > >                       connector->dp.dsc_dpcd[DP_DSC_DEC_COLOR_FORMAT_CAP - DP_DSC_SUPPORT] &
> > >                       DP_DSC_RGB;
> > >
> > > -     line_buf_depth = drm_dp_dsc_sink_line_buf_depth(connector->dp.dsc_dpcd);
> > > -     if (!line_buf_depth) {
> > > +     vdsc_cfg->line_buf_depth = min(INTEL_DP_DSC_MAX_LINE_BUF_DEPTH,
> > > +                                    drm_dp_dsc_sink_line_buf_depth(connector->dp.dsc_dpcd));
> > > +     if (!vdsc_cfg->line_buf_depth) {
> > >               drm_dbg_kms(&i915->drm,
> > >                           "DSC Sink Line Buffer Depth invalid\n");
> > >               return -EINVAL;
> > >       }
> > >
> > > -     if (vdsc_cfg->dsc_version_minor == 2)
> > > -             vdsc_cfg->line_buf_depth = (line_buf_depth == DSC_1_2_MAX_LINEBUF_DEPTH_BITS) ?
> > > -                     DSC_1_2_MAX_LINEBUF_DEPTH_VAL : line_buf_depth;
> > > -     else
> > > -             vdsc_cfg->line_buf_depth = (line_buf_depth > DSC_1_1_MAX_LINEBUF_DEPTH_BITS) ?
> > > -                     DSC_1_1_MAX_LINEBUF_DEPTH_BITS : line_buf_depth;
> > > -
> > >       vdsc_cfg->block_pred_enable =
> > >               connector->dp.dsc_dpcd[DP_DSC_BLK_PREDICTION_SUPPORT - DP_DSC_SUPPORT] &
> > >               DP_DSC_BLK_PREDICTION_IS_SUPPORTED;
> > > diff --git a/include/drm/display/drm_dsc.h b/include/drm/display/drm_dsc.h
> > > index bc90273d06a62..bbbe7438473d3 100644
> > > --- a/include/drm/display/drm_dsc.h
> > > +++ b/include/drm/display/drm_dsc.h
> > > @@ -40,9 +40,6 @@
> > >   #define DSC_PPS_RC_RANGE_MINQP_SHIFT                11
> > >   #define DSC_PPS_RC_RANGE_MAXQP_SHIFT                6
> > >   #define DSC_PPS_NATIVE_420_SHIFT            1
> > > -#define DSC_1_2_MAX_LINEBUF_DEPTH_BITS               16
> > > -#define DSC_1_2_MAX_LINEBUF_DEPTH_VAL                0
> > > -#define DSC_1_1_MAX_LINEBUF_DEPTH_BITS               13
> > >
> > >   /**
> > >    * struct drm_dsc_rc_range_parameters - DSC Rate Control range parameters

Re: [PATCH 01/11] drm/i915/dp: Fix DSC line buffer depth programming

[Imre Deak]

On Wed, Mar 20, 2024 at 10:11:41PM +0200, Imre Deak wrote:
> Fix the calculation of the DSC line buffer depth. This is limited both
> by the source's and sink's maximum line buffer depth, but the former one
> was not taken into account. On all Intel platform's the source's maximum
> buffer depth is 13, so the overall limit is simply the minimum of the
> source/sink's limit, regardless of the DSC version.
> 
> This leaves the DSI DSC line buffer depth calculation as-is, trusting
> VBT.
> 
> On DSC version 1.2 for sinks reporting a maximum line buffer depth of 16
> the line buffer depth was incorrectly programmed as 0, leading to a
> corruption in color gradients / lines on the decompressed screen image.
> 
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Hi Maarten, Thomas, Maxime,

are you ok to merge the DRM DP-DSC/MST changes in patches 1, 7-9, 11 via
drm-intel-next?

--Imre

> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 16 ++++++----------
>  include/drm/display/drm_dsc.h           |  3 ---
>  2 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index af7ca00e9bc0a..dbe65651bf277 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -89,6 +89,9 @@
>  #define DP_DSC_MAX_ENC_THROUGHPUT_0		340000
>  #define DP_DSC_MAX_ENC_THROUGHPUT_1		400000
>  
> +/* Max DSC line buffer depth supported by HW. */
> +#define INTEL_DP_DSC_MAX_LINE_BUF_DEPTH		13
> +
>  /* DP DSC FEC Overhead factor in ppm = 1/(0.972261) = 1.028530 */
>  #define DP_DSC_FEC_OVERHEAD_FACTOR		1028530
>  
> @@ -1703,7 +1706,6 @@ static int intel_dp_dsc_compute_params(const struct intel_connector *connector,
>  {
>  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>  	struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
> -	u8 line_buf_depth;
>  	int ret;
>  
>  	/*
> @@ -1732,20 +1734,14 @@ static int intel_dp_dsc_compute_params(const struct intel_connector *connector,
>  			connector->dp.dsc_dpcd[DP_DSC_DEC_COLOR_FORMAT_CAP - DP_DSC_SUPPORT] &
>  			DP_DSC_RGB;
>  
> -	line_buf_depth = drm_dp_dsc_sink_line_buf_depth(connector->dp.dsc_dpcd);
> -	if (!line_buf_depth) {
> +	vdsc_cfg->line_buf_depth = min(INTEL_DP_DSC_MAX_LINE_BUF_DEPTH,
> +				       drm_dp_dsc_sink_line_buf_depth(connector->dp.dsc_dpcd));
> +	if (!vdsc_cfg->line_buf_depth) {
>  		drm_dbg_kms(&i915->drm,
>  			    "DSC Sink Line Buffer Depth invalid\n");
>  		return -EINVAL;
>  	}
>  
> -	if (vdsc_cfg->dsc_version_minor == 2)
> -		vdsc_cfg->line_buf_depth = (line_buf_depth == DSC_1_2_MAX_LINEBUF_DEPTH_BITS) ?
> -			DSC_1_2_MAX_LINEBUF_DEPTH_VAL : line_buf_depth;
> -	else
> -		vdsc_cfg->line_buf_depth = (line_buf_depth > DSC_1_1_MAX_LINEBUF_DEPTH_BITS) ?
> -			DSC_1_1_MAX_LINEBUF_DEPTH_BITS : line_buf_depth;
> -
>  	vdsc_cfg->block_pred_enable =
>  		connector->dp.dsc_dpcd[DP_DSC_BLK_PREDICTION_SUPPORT - DP_DSC_SUPPORT] &
>  		DP_DSC_BLK_PREDICTION_IS_SUPPORTED;
> diff --git a/include/drm/display/drm_dsc.h b/include/drm/display/drm_dsc.h
> index bc90273d06a62..bbbe7438473d3 100644
> --- a/include/drm/display/drm_dsc.h
> +++ b/include/drm/display/drm_dsc.h
> @@ -40,9 +40,6 @@
>  #define DSC_PPS_RC_RANGE_MINQP_SHIFT		11
>  #define DSC_PPS_RC_RANGE_MAXQP_SHIFT		6
>  #define DSC_PPS_NATIVE_420_SHIFT		1
> -#define DSC_1_2_MAX_LINEBUF_DEPTH_BITS		16
> -#define DSC_1_2_MAX_LINEBUF_DEPTH_VAL		0
> -#define DSC_1_1_MAX_LINEBUF_DEPTH_BITS		13
>  
>  /**
>   * struct drm_dsc_rc_range_parameters - DSC Rate Control range parameters
> -- 
> 2.43.3
> 

Re: [PATCH 01/11] drm/i915/dp: Fix DSC line buffer depth programming

[Jani Nikula]

On Thu, 28 Mar 2024, Imre Deak <imre.deak@intel.com> wrote:
> On Wed, Mar 20, 2024 at 10:11:41PM +0200, Imre Deak wrote:
>> Fix the calculation of the DSC line buffer depth. This is limited both
>> by the source's and sink's maximum line buffer depth, but the former one
>> was not taken into account. On all Intel platform's the source's maximum
>> buffer depth is 13, so the overall limit is simply the minimum of the
>> source/sink's limit, regardless of the DSC version.
>> 
>> This leaves the DSI DSC line buffer depth calculation as-is, trusting
>> VBT.
>> 
>> On DSC version 1.2 for sinks reporting a maximum line buffer depth of 16
>> the line buffer depth was incorrectly programmed as 0, leading to a
>> corruption in color gradients / lines on the decompressed screen image.
>> 
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>
> Hi Maarten, Thomas, Maxime,
>
> are you ok to merge the DRM DP-DSC/MST changes in patches 1, 7-9, 11 via
> drm-intel-next?

Ping? Ack for merging via drm-intel-next, please?

BR,
Jani.


>
> --Imre
>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dp.c | 16 ++++++----------
>>  include/drm/display/drm_dsc.h           |  3 ---
>>  2 files changed, 6 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index af7ca00e9bc0a..dbe65651bf277 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -89,6 +89,9 @@
>>  #define DP_DSC_MAX_ENC_THROUGHPUT_0		340000
>>  #define DP_DSC_MAX_ENC_THROUGHPUT_1		400000
>>  
>> +/* Max DSC line buffer depth supported by HW. */
>> +#define INTEL_DP_DSC_MAX_LINE_BUF_DEPTH		13
>> +
>>  /* DP DSC FEC Overhead factor in ppm = 1/(0.972261) = 1.028530 */
>>  #define DP_DSC_FEC_OVERHEAD_FACTOR		1028530
>>  
>> @@ -1703,7 +1706,6 @@ static int intel_dp_dsc_compute_params(const struct intel_connector *connector,
>>  {
>>  	struct drm_i915_private *i915 = to_i915(connector->base.dev);
>>  	struct drm_dsc_config *vdsc_cfg = &crtc_state->dsc.config;
>> -	u8 line_buf_depth;
>>  	int ret;
>>  
>>  	/*
>> @@ -1732,20 +1734,14 @@ static int intel_dp_dsc_compute_params(const struct intel_connector *connector,
>>  			connector->dp.dsc_dpcd[DP_DSC_DEC_COLOR_FORMAT_CAP - DP_DSC_SUPPORT] &
>>  			DP_DSC_RGB;
>>  
>> -	line_buf_depth = drm_dp_dsc_sink_line_buf_depth(connector->dp.dsc_dpcd);
>> -	if (!line_buf_depth) {
>> +	vdsc_cfg->line_buf_depth = min(INTEL_DP_DSC_MAX_LINE_BUF_DEPTH,
>> +				       drm_dp_dsc_sink_line_buf_depth(connector->dp.dsc_dpcd));
>> +	if (!vdsc_cfg->line_buf_depth) {
>>  		drm_dbg_kms(&i915->drm,
>>  			    "DSC Sink Line Buffer Depth invalid\n");
>>  		return -EINVAL;
>>  	}
>>  
>> -	if (vdsc_cfg->dsc_version_minor == 2)
>> -		vdsc_cfg->line_buf_depth = (line_buf_depth == DSC_1_2_MAX_LINEBUF_DEPTH_BITS) ?
>> -			DSC_1_2_MAX_LINEBUF_DEPTH_VAL : line_buf_depth;
>> -	else
>> -		vdsc_cfg->line_buf_depth = (line_buf_depth > DSC_1_1_MAX_LINEBUF_DEPTH_BITS) ?
>> -			DSC_1_1_MAX_LINEBUF_DEPTH_BITS : line_buf_depth;
>> -
>>  	vdsc_cfg->block_pred_enable =
>>  		connector->dp.dsc_dpcd[DP_DSC_BLK_PREDICTION_SUPPORT - DP_DSC_SUPPORT] &
>>  		DP_DSC_BLK_PREDICTION_IS_SUPPORTED;
>> diff --git a/include/drm/display/drm_dsc.h b/include/drm/display/drm_dsc.h
>> index bc90273d06a62..bbbe7438473d3 100644
>> --- a/include/drm/display/drm_dsc.h
>> +++ b/include/drm/display/drm_dsc.h
>> @@ -40,9 +40,6 @@
>>  #define DSC_PPS_RC_RANGE_MINQP_SHIFT		11
>>  #define DSC_PPS_RC_RANGE_MAXQP_SHIFT		6
>>  #define DSC_PPS_NATIVE_420_SHIFT		1
>> -#define DSC_1_2_MAX_LINEBUF_DEPTH_BITS		16
>> -#define DSC_1_2_MAX_LINEBUF_DEPTH_VAL		0
>> -#define DSC_1_1_MAX_LINEBUF_DEPTH_BITS		13
>>  
>>  /**
>>   * struct drm_dsc_rc_range_parameters - DSC Rate Control range parameters
>> -- 
>> 2.43.3
>> 

-- 
Jani Nikula, Intel

[PATCH 07/11] drm/dp: Add drm_dp_uhbr_channel_coding_supported()

[Imre Deak]

Factor out a function to check for UHBR channel coding support used by a
follow-up patch in the patchset.

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
 include/drm/display/drm_dp_helper.h     | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index dbe65651bf277..1d13a1ba2b97d 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -217,7 +217,7 @@ static void intel_dp_set_dpcd_sink_rates(struct intel_dp *intel_dp)
 	 * Sink rates for 128b/132b. If set, sink should support all 8b/10b
 	 * rates and 10 Gbps.
 	 */
-	if (intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_128B132B) {
+	if (drm_dp_uhbr_channel_coding_supported(intel_dp->dpcd)) {
 		u8 uhbr_rates = 0;
 
 		BUILD_BUG_ON(ARRAY_SIZE(intel_dp->sink_rates) < ARRAY_SIZE(dp_rates) + 3);
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index a62fcd051d4d4..150c37a99a16f 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -221,6 +221,12 @@ drm_dp_channel_coding_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 	return dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_8B10B;
 }
 
+static inline bool
+drm_dp_uhbr_channel_coding_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
+{
+	return dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_128B132B;
+}
+
 static inline bool
 drm_dp_alternate_scrambler_reset_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 {
-- 
2.43.3

Re: [PATCH 07/11] drm/dp: Add drm_dp_uhbr_channel_coding_supported()

[Nautiyal, Ankit K]

On 3/21/2024 1:41 AM, Imre Deak wrote:
> Factor out a function to check for UHBR channel coding support used by a
> follow-up patch in the patchset.
>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>

LGTM.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

> ---
>   drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
>   include/drm/display/drm_dp_helper.h     | 6 ++++++
>   2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index dbe65651bf277..1d13a1ba2b97d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -217,7 +217,7 @@ static void intel_dp_set_dpcd_sink_rates(struct intel_dp *intel_dp)
>   	 * Sink rates for 128b/132b. If set, sink should support all 8b/10b
>   	 * rates and 10 Gbps.
>   	 */
> -	if (intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_128B132B) {
> +	if (drm_dp_uhbr_channel_coding_supported(intel_dp->dpcd)) {
>   		u8 uhbr_rates = 0;
>   
>   		BUILD_BUG_ON(ARRAY_SIZE(intel_dp->sink_rates) < ARRAY_SIZE(dp_rates) + 3);
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index a62fcd051d4d4..150c37a99a16f 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -221,6 +221,12 @@ drm_dp_channel_coding_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
>   	return dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_8B10B;
>   }
>   
> +static inline bool
> +drm_dp_uhbr_channel_coding_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +{
> +	return dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_128B132B;
> +}
> +
>   static inline bool
>   drm_dp_alternate_scrambler_reset_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
>   {

Re: [PATCH 07/11] drm/dp: Add drm_dp_uhbr_channel_coding_supported()

[Manasi Navare]

Reviewed-by: Manasi Navare <navaremanasi@chromium.org>

Manasi

On Tue, Mar 26, 2024 at 5:54 AM Nautiyal, Ankit K
<ankit.k.nautiyal@intel.com> wrote:
>
>
> On 3/21/2024 1:41 AM, Imre Deak wrote:
> > Factor out a function to check for UHBR channel coding support used by a
> > follow-up patch in the patchset.
> >
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
>
> LGTM.
>
> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>
> > ---
> >   drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> >   include/drm/display/drm_dp_helper.h     | 6 ++++++
> >   2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index dbe65651bf277..1d13a1ba2b97d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -217,7 +217,7 @@ static void intel_dp_set_dpcd_sink_rates(struct intel_dp *intel_dp)
> >        * Sink rates for 128b/132b. If set, sink should support all 8b/10b
> >        * rates and 10 Gbps.
> >        */
> > -     if (intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_128B132B) {
> > +     if (drm_dp_uhbr_channel_coding_supported(intel_dp->dpcd)) {
> >               u8 uhbr_rates = 0;
> >
> >               BUILD_BUG_ON(ARRAY_SIZE(intel_dp->sink_rates) < ARRAY_SIZE(dp_rates) + 3);
> > diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> > index a62fcd051d4d4..150c37a99a16f 100644
> > --- a/include/drm/display/drm_dp_helper.h
> > +++ b/include/drm/display/drm_dp_helper.h
> > @@ -221,6 +221,12 @@ drm_dp_channel_coding_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> >       return dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_8B10B;
> >   }
> >
> > +static inline bool
> > +drm_dp_uhbr_channel_coding_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> > +{
> > +     return dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_128B132B;
> > +}
> > +
> >   static inline bool
> >   drm_dp_alternate_scrambler_reset_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> >   {

[PATCH 08/11] drm/dp_mst: Factor out drm_dp_mst_port_is_logical()

[Imre Deak]

Factor out a function to check if an MST port is logical, used by a
follow-up i915 patch in the patchset.

Cc: Lyude Paul <lyude@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 6 +++---
 include/drm/display/drm_dp_mst_helper.h       | 7 +++++++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 03d5282094262..6bd471a2266ce 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -2274,7 +2274,7 @@ drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb,
 
 	if (port->pdt != DP_PEER_DEVICE_NONE &&
 	    drm_dp_mst_is_end_device(port->pdt, port->mcs) &&
-	    port->port_num >= DP_MST_LOGICAL_PORT_0)
+	    drm_dp_mst_port_is_logical(port))
 		port->cached_edid = drm_edid_read_ddc(port->connector,
 						      &port->aux.ddc);
 
@@ -4213,7 +4213,7 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
 	case DP_PEER_DEVICE_SST_SINK:
 		ret = connector_status_connected;
 		/* for logical ports - cache the EDID */
-		if (port->port_num >= DP_MST_LOGICAL_PORT_0 && !port->cached_edid)
+		if (drm_dp_mst_port_is_logical(port) && !port->cached_edid)
 			port->cached_edid = drm_edid_read_ddc(connector, &port->aux.ddc);
 		break;
 	case DP_PEER_DEVICE_DP_LEGACY_CONV:
@@ -5977,7 +5977,7 @@ static bool drm_dp_mst_is_virtual_dpcd(struct drm_dp_mst_port *port)
 		return false;
 
 	/* Virtual DP Sink (Internal Display Panel) */
-	if (port->port_num >= 8)
+	if (drm_dp_mst_port_is_logical(port))
 		return true;
 
 	/* DP-to-HDMI Protocol Converter */
diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
index 3ae88a383a41f..c12f18b744d01 100644
--- a/include/drm/display/drm_dp_mst_helper.h
+++ b/include/drm/display/drm_dp_mst_helper.h
@@ -927,6 +927,13 @@ int __must_check drm_dp_mst_root_conn_atomic_check(struct drm_connector_state *n
 void drm_dp_mst_get_port_malloc(struct drm_dp_mst_port *port);
 void drm_dp_mst_put_port_malloc(struct drm_dp_mst_port *port);
 
+static inline
+bool drm_dp_mst_port_is_logical(struct drm_dp_mst_port *port)
+{
+	return port->port_num >= DP_MST_LOGICAL_PORT_0;
+}
+
+struct drm_dp_aux *drm_dp_mst_aux_for_parent(struct drm_dp_mst_port *port);
 struct drm_dp_aux *drm_dp_mst_dsc_aux_for_port(struct drm_dp_mst_port *port);
 
 static inline struct drm_dp_mst_topology_state *
-- 
2.43.3

Re: [PATCH 08/11] drm/dp_mst: Factor out drm_dp_mst_port_is_logical()

[Nautiyal, Ankit K]

On 3/21/2024 1:41 AM, Imre Deak wrote:
> Factor out a function to check if an MST port is logical, used by a
> follow-up i915 patch in the patchset.
>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/display/drm_dp_mst_topology.c | 6 +++---
>   include/drm/display/drm_dp_mst_helper.h       | 7 +++++++
>   2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 03d5282094262..6bd471a2266ce 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -2274,7 +2274,7 @@ drm_dp_mst_port_add_connector(struct drm_dp_mst_branch *mstb,
>   
>   	if (port->pdt != DP_PEER_DEVICE_NONE &&
>   	    drm_dp_mst_is_end_device(port->pdt, port->mcs) &&
> -	    port->port_num >= DP_MST_LOGICAL_PORT_0)
> +	    drm_dp_mst_port_is_logical(port))
>   		port->cached_edid = drm_edid_read_ddc(port->connector,
>   						      &port->aux.ddc);
>   
> @@ -4213,7 +4213,7 @@ drm_dp_mst_detect_port(struct drm_connector *connector,
>   	case DP_PEER_DEVICE_SST_SINK:
>   		ret = connector_status_connected;
>   		/* for logical ports - cache the EDID */
> -		if (port->port_num >= DP_MST_LOGICAL_PORT_0 && !port->cached_edid)
> +		if (drm_dp_mst_port_is_logical(port) && !port->cached_edid)
>   			port->cached_edid = drm_edid_read_ddc(connector, &port->aux.ddc);
>   		break;
>   	case DP_PEER_DEVICE_DP_LEGACY_CONV:
> @@ -5977,7 +5977,7 @@ static bool drm_dp_mst_is_virtual_dpcd(struct drm_dp_mst_port *port)
>   		return false;
>   
>   	/* Virtual DP Sink (Internal Display Panel) */
> -	if (port->port_num >= 8)
> +	if (drm_dp_mst_port_is_logical(port))
>   		return true;
>   
>   	/* DP-to-HDMI Protocol Converter */
> diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> index 3ae88a383a41f..c12f18b744d01 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -927,6 +927,13 @@ int __must_check drm_dp_mst_root_conn_atomic_check(struct drm_connector_state *n
>   void drm_dp_mst_get_port_malloc(struct drm_dp_mst_port *port);
>   void drm_dp_mst_put_port_malloc(struct drm_dp_mst_port *port);
>   
> +static inline
> +bool drm_dp_mst_port_is_logical(struct drm_dp_mst_port *port)
> +{
> +	return port->port_num >= DP_MST_LOGICAL_PORT_0;
> +}
> +
> +struct drm_dp_aux *drm_dp_mst_aux_for_parent(struct drm_dp_mst_port *port);

This line should be part of next patch, where this helper is defined.

Otherwise LGTM.

With the above line removed, this is:

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>


Regards,

Ankit

>   struct drm_dp_aux *drm_dp_mst_dsc_aux_for_port(struct drm_dp_mst_port *port);
>   
>   static inline struct drm_dp_mst_topology_state *

[PATCH 09/11] drm/dp_mst: Add drm_dp_mst_aux_for_parent()

[Imre Deak]

Add a function to get the AUX device of the parent of an MST port, used
by a follow-up i915 patch in the patchset.

Cc: Lyude Paul <lyude@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 6bd471a2266ce..d70f7de644371 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -6004,6 +6004,22 @@ static bool drm_dp_mst_is_virtual_dpcd(struct drm_dp_mst_port *port)
 	return false;
 }
 
+/**
+ * drm_dp_mst_aux_for_parent() - Get the AUX device for an MST port's parent
+ * @port: MST port whose parent's AUX device is returned
+ *
+ * Return the AUX device for @port's parent or NULL if port's parent is the
+ * root port.
+ */
+struct drm_dp_aux *drm_dp_mst_aux_for_parent(struct drm_dp_mst_port *port)
+{
+	if (!port->parent || !port->parent->port_parent)
+		return NULL;
+
+	return &port->parent->port_parent->aux;
+}
+EXPORT_SYMBOL(drm_dp_mst_aux_for_parent);
+
 /**
  * drm_dp_mst_dsc_aux_for_port() - Find the correct aux for DSC
  * @port: The port to check. A leaf of the MST tree with an attached display.
-- 
2.43.3

Re: [PATCH 09/11] drm/dp_mst: Add drm_dp_mst_aux_for_parent()

[Nautiyal, Ankit K]

On 3/21/2024 1:41 AM, Imre Deak wrote:
> Add a function to get the AUX device of the parent of an MST port, used
> by a follow-up i915 patch in the patchset.
>
> Cc: Lyude Paul <lyude@redhat.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/display/drm_dp_mst_topology.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 6bd471a2266ce..d70f7de644371 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -6004,6 +6004,22 @@ static bool drm_dp_mst_is_virtual_dpcd(struct drm_dp_mst_port *port)
>   	return false;
>   }
>   
> +/**
> + * drm_dp_mst_aux_for_parent() - Get the AUX device for an MST port's parent
> + * @port: MST port whose parent's AUX device is returned
> + *
> + * Return the AUX device for @port's parent or NULL if port's parent is the
> + * root port.
> + */
> +struct drm_dp_aux *drm_dp_mst_aux_for_parent(struct drm_dp_mst_port *port)
> +{
> +	if (!port->parent || !port->parent->port_parent)
> +		return NULL;
> +
> +	return &port->parent->port_parent->aux;
> +}
> +EXPORT_SYMBOL(drm_dp_mst_aux_for_parent);

As mentioned in previous patch, the declaration of this in the header, 
got included in previous patch.

Regards,

Ankit

> +
>   /**
>    * drm_dp_mst_dsc_aux_for_port() - Find the correct aux for DSC
>    * @port: The port to check. A leaf of the MST tree with an attached display.

Re: [PATCH 09/11] drm/dp_mst: Add drm_dp_mst_aux_for_parent()

[Imre Deak]

On Wed, Mar 27, 2024 at 02:30:53PM +0530, Nautiyal, Ankit K wrote:
> 
> On 3/21/2024 1:41 AM, Imre Deak wrote:
> > Add a function to get the AUX device of the parent of an MST port, used
> > by a follow-up i915 patch in the patchset.
> > 
> > Cc: Lyude Paul <lyude@redhat.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/display/drm_dp_mst_topology.c | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > index 6bd471a2266ce..d70f7de644371 100644
> > --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> > @@ -6004,6 +6004,22 @@ static bool drm_dp_mst_is_virtual_dpcd(struct drm_dp_mst_port *port)
> >   	return false;
> >   }
> > +/**
> > + * drm_dp_mst_aux_for_parent() - Get the AUX device for an MST port's parent
> > + * @port: MST port whose parent's AUX device is returned
> > + *
> > + * Return the AUX device for @port's parent or NULL if port's parent is the
> > + * root port.
> > + */
> > +struct drm_dp_aux *drm_dp_mst_aux_for_parent(struct drm_dp_mst_port *port)
> > +{
> > +	if (!port->parent || !port->parent->port_parent)
> > +		return NULL;
> > +
> > +	return &port->parent->port_parent->aux;
> > +}
> > +EXPORT_SYMBOL(drm_dp_mst_aux_for_parent);
> 
> As mentioned in previous patch, the declaration of this in the header,
> got included in previous patch.

Yes thanks, the header change should've been in this patch, will move it here
(while applying the patches if nothing else comes up).

> Regards,
> 
> Ankit
> 
> > +
> >   /**
> >    * drm_dp_mst_dsc_aux_for_port() - Find the correct aux for DSC
> >    * @port: The port to check. A leaf of the MST tree with an attached display.

Re: [PATCH 09/11] drm/dp_mst: Add drm_dp_mst_aux_for_parent()

[Nautiyal, Ankit K]

On 3/27/2024 7:55 PM, Imre Deak wrote:
> On Wed, Mar 27, 2024 at 02:30:53PM +0530, Nautiyal, Ankit K wrote:
>> On 3/21/2024 1:41 AM, Imre Deak wrote:
>>> Add a function to get the AUX device of the parent of an MST port, used
>>> by a follow-up i915 patch in the patchset.
>>>
>>> Cc: Lyude Paul <lyude@redhat.com>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>>> ---
>>>    drivers/gpu/drm/display/drm_dp_mst_topology.c | 16 ++++++++++++++++
>>>    1 file changed, 16 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>>> index 6bd471a2266ce..d70f7de644371 100644
>>> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
>>> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
>>> @@ -6004,6 +6004,22 @@ static bool drm_dp_mst_is_virtual_dpcd(struct drm_dp_mst_port *port)
>>>    	return false;
>>>    }
>>> +/**
>>> + * drm_dp_mst_aux_for_parent() - Get the AUX device for an MST port's parent
>>> + * @port: MST port whose parent's AUX device is returned
>>> + *
>>> + * Return the AUX device for @port's parent or NULL if port's parent is the
>>> + * root port.
>>> + */
>>> +struct drm_dp_aux *drm_dp_mst_aux_for_parent(struct drm_dp_mst_port *port)
>>> +{
>>> +	if (!port->parent || !port->parent->port_parent)
>>> +		return NULL;
>>> +
>>> +	return &port->parent->port_parent->aux;
>>> +}
>>> +EXPORT_SYMBOL(drm_dp_mst_aux_for_parent);
>> As mentioned in previous patch, the declaration of this in the header,
>> got included in previous patch.
> Yes thanks, the header change should've been in this patch, will move it here
> (while applying the patches if nothing else comes up).

With the above fixed, this is:

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

>> Regards,
>>
>> Ankit
>>
>>> +
>>>    /**
>>>     * drm_dp_mst_dsc_aux_for_port() - Find the correct aux for DSC
>>>     * @port: The port to check. A leaf of the MST tree with an attached display.

[PATCH 11/11] drm/i915/dp_mst: Enable HBLANK expansion quirk for UHBR rates

[Imre Deak]

Enabling the 5k@60Hz uncompressed mode on the MediaTek/Dell U3224KBA
monitor results in a blank screen, at least on MTL platforms on UHBR
link rates with some (<30) uncompressed bpp values. Enabling compression
fixes the problem, so do that for now. Windows enables DSC always if the
sink supports it and forcing it to enable the mode without compression
leads to the same problem above (which suggests a panel issue with
uncompressed mode).

The same 5k mode on non-UHBR link rates is not affected and lower
resolution modes are not affected either. The problem is similar to the
one fixed by the HBLANK expansion quirk on Synaptics hubs, with the
difference that the problematic mode has a longer HBLANK duration. Also
the monitor doesn't report supporting HBLANK expansion; either its
internal MST hub does the expansion internally - similarly to the
Synaptics hub - or the issue has another root cause, but still related
to the mode's short HBLANK duration. Enable the quirk for the monitor
adjusting the detection for the above differences.

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_helper.c     |  2 ++
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 22 +++++++++++++++++----
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index f5d4be8978660..3e8e1bb59dea3 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -2281,6 +2281,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
 	{ OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD) },
 	/* Synaptics DP1.4 MST hubs require DSC for some modes on which it applies HBLANK expansion. */
 	{ OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC) },
+	/* MediaTek panels (at least in U3224KBA) require DSC for modes with a short HBLANK on UHBR links. */
+	{ OUI(0x00, 0x0C, 0xE7), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC) },
 	/* Apple MacBookPro 2017 15 inch eDP Retina panel reports too low DP_MAX_LINK_RATE */
 	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID(101, 68, 21, 101, 98, 97), false, BIT(DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS) },
 };
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 76a8fb21b8e52..b5224fe6cc16b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -407,15 +407,22 @@ static int mode_hblank_period_ns(const struct drm_display_mode *mode)
 
 static bool
 hblank_expansion_quirk_needs_dsc(const struct intel_connector *connector,
-				 const struct intel_crtc_state *crtc_state)
+				 const struct intel_crtc_state *crtc_state,
+				 const struct link_config_limits *limits)
 {
 	const struct drm_display_mode *adjusted_mode =
 		&crtc_state->hw.adjusted_mode;
+	bool is_uhbr_sink = connector->mst_port &&
+			    drm_dp_uhbr_channel_coding_supported(connector->mst_port->dpcd);
+	int hblank_limit = is_uhbr_sink ? 500 : 300;
 
 	if (!connector->dp.dsc_hblank_expansion_quirk)
 		return false;
 
-	if (mode_hblank_period_ns(adjusted_mode) > 300)
+	if (is_uhbr_sink && !drm_dp_is_uhbr_rate(limits->max_rate))
+		return false;
+
+	if (mode_hblank_period_ns(adjusted_mode) > hblank_limit)
 		return false;
 
 	return true;
@@ -431,7 +438,7 @@ adjust_limits_for_dsc_hblank_expansion_quirk(const struct intel_connector *conne
 	const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	int min_bpp_x16 = limits->link.min_bpp_x16;
 
-	if (!hblank_expansion_quirk_needs_dsc(connector, crtc_state))
+	if (!hblank_expansion_quirk_needs_dsc(connector, crtc_state, limits))
 		return true;
 
 	if (!dsc) {
@@ -1539,7 +1546,14 @@ static bool detect_dsc_hblank_expansion_quirk(const struct intel_connector *conn
 			      DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC))
 		return false;
 
-	if (!(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
+	/*
+	 * UHBR (MST sink) devices requiring this quirk doesn't advertise the
+	 * HBLANK expansion support. Presuming that they perform HBLANK
+	 * expansion internally, or are affected by this issue on modes with a
+	 * short HBLANK for other reasons.
+	 */
+	if (!drm_dp_uhbr_channel_coding_supported(dpcd) &&
+	    !(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
 		return false;
 
 	drm_dbg_kms(&i915->drm,
-- 
2.43.3

Re: [PATCH 11/11] drm/i915/dp_mst: Enable HBLANK expansion quirk for UHBR rates

[Nautiyal, Ankit K]

On 3/21/2024 1:41 AM, Imre Deak wrote:
> Enabling the 5k@60Hz uncompressed mode on the MediaTek/Dell U3224KBA
> monitor results in a blank screen, at least on MTL platforms on UHBR
> link rates with some (<30) uncompressed bpp values. Enabling compression
> fixes the problem, so do that for now. Windows enables DSC always if the
> sink supports it and forcing it to enable the mode without compression
> leads to the same problem above (which suggests a panel issue with
> uncompressed mode).
>
> The same 5k mode on non-UHBR link rates is not affected and lower
> resolution modes are not affected either. The problem is similar to the
> one fixed by the HBLANK expansion quirk on Synaptics hubs, with the
> difference that the problematic mode has a longer HBLANK duration. Also
> the monitor doesn't report supporting HBLANK expansion; either its
> internal MST hub does the expansion internally - similarly to the
> Synaptics hub - or the issue has another root cause, but still related
> to the mode's short HBLANK duration. Enable the quirk for the monitor
> adjusting the detection for the above differences.
>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Imre Deak <imre.deak@intel.com>

LGTM.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>


> ---
>   drivers/gpu/drm/display/drm_dp_helper.c     |  2 ++
>   drivers/gpu/drm/i915/display/intel_dp_mst.c | 22 +++++++++++++++++----
>   2 files changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index f5d4be8978660..3e8e1bb59dea3 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -2281,6 +2281,8 @@ static const struct dpcd_quirk dpcd_quirk_list[] = {
>   	{ OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_DSC_WITHOUT_VIRTUAL_DPCD) },
>   	/* Synaptics DP1.4 MST hubs require DSC for some modes on which it applies HBLANK expansion. */
>   	{ OUI(0x90, 0xCC, 0x24), DEVICE_ID_ANY, true, BIT(DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC) },
> +	/* MediaTek panels (at least in U3224KBA) require DSC for modes with a short HBLANK on UHBR links. */
> +	{ OUI(0x00, 0x0C, 0xE7), DEVICE_ID_ANY, false, BIT(DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC) },
>   	/* Apple MacBookPro 2017 15 inch eDP Retina panel reports too low DP_MAX_LINK_RATE */
>   	{ OUI(0x00, 0x10, 0xfa), DEVICE_ID(101, 68, 21, 101, 98, 97), false, BIT(DP_DPCD_QUIRK_CAN_DO_MAX_LINK_RATE_3_24_GBPS) },
>   };
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 76a8fb21b8e52..b5224fe6cc16b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -407,15 +407,22 @@ static int mode_hblank_period_ns(const struct drm_display_mode *mode)
>   
>   static bool
>   hblank_expansion_quirk_needs_dsc(const struct intel_connector *connector,
> -				 const struct intel_crtc_state *crtc_state)
> +				 const struct intel_crtc_state *crtc_state,
> +				 const struct link_config_limits *limits)
>   {
>   	const struct drm_display_mode *adjusted_mode =
>   		&crtc_state->hw.adjusted_mode;
> +	bool is_uhbr_sink = connector->mst_port &&
> +			    drm_dp_uhbr_channel_coding_supported(connector->mst_port->dpcd);
> +	int hblank_limit = is_uhbr_sink ? 500 : 300;
>   
>   	if (!connector->dp.dsc_hblank_expansion_quirk)
>   		return false;
>   
> -	if (mode_hblank_period_ns(adjusted_mode) > 300)
> +	if (is_uhbr_sink && !drm_dp_is_uhbr_rate(limits->max_rate))
> +		return false;
> +
> +	if (mode_hblank_period_ns(adjusted_mode) > hblank_limit)
>   		return false;
>   
>   	return true;
> @@ -431,7 +438,7 @@ adjust_limits_for_dsc_hblank_expansion_quirk(const struct intel_connector *conne
>   	const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>   	int min_bpp_x16 = limits->link.min_bpp_x16;
>   
> -	if (!hblank_expansion_quirk_needs_dsc(connector, crtc_state))
> +	if (!hblank_expansion_quirk_needs_dsc(connector, crtc_state, limits))
>   		return true;
>   
>   	if (!dsc) {
> @@ -1539,7 +1546,14 @@ static bool detect_dsc_hblank_expansion_quirk(const struct intel_connector *conn
>   			      DP_DPCD_QUIRK_HBLANK_EXPANSION_REQUIRES_DSC))
>   		return false;
>   
> -	if (!(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
> +	/*
> +	 * UHBR (MST sink) devices requiring this quirk doesn't advertise the
> +	 * HBLANK expansion support. Presuming that they perform HBLANK
> +	 * expansion internally, or are affected by this issue on modes with a
> +	 * short HBLANK for other reasons.
> +	 */
> +	if (!drm_dp_uhbr_channel_coding_supported(dpcd) &&
> +	    !(dpcd[DP_RECEIVE_PORT_0_CAP_0] & DP_HBLANK_EXPANSION_CAPABLE))
>   		return false;
>   
>   	drm_dbg_kms(&i915->drm,