diff mbox series

[v2] drm/i915/dsc: Change rc parameters calculation for DSC 1.1

Message ID 20250221015637.507741-1-william.tseng@intel.com (mailing list archive)
State New
Headers show
Series [v2] drm/i915/dsc: Change rc parameters calculation for DSC 1.1 | expand

Commit Message

Tseng, William Feb. 21, 2025, 1:56 a.m. UTC 
When calculating dsc parameters, the rc parameters calculated by
calculate_rc_params() is incorrect in the case of DSC 1.1 and
DISPLAY_VER(dev_priv) >= 13. It turns out to be some noise-like
lines are displayed on a MIPI DSI panel supporting DSC 1.1.

The function calculate_rc_params() is implemented by following
the Table E-2 in DSC 1.2a and creates incorrect rc parameters
for DSC 1.1. As a result, add the additional condition
(vdsc_cfg->dsc_version_minor >= 2) to get the right function to
calculate rc parameters for DSC 1.1.

v1: initial version.
v2: modify the commit comment and change the condition
    regarding DSC version checking.

Cc: Suraj Kandpal <suraj.kandpal@intel.com>
Cc: Juha-Pekka Heikkil <juha-pekka.heikkila@intel.com>
Signed-off-by: William Tseng <william.tseng@intel.com>
---
 drivers/gpu/drm/i915/display/intel_vdsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Kandpal, Suraj Feb. 21, 2025, 3:43 a.m. UTC | #1 
> -----Original Message-----
> From: Tseng, William <william.tseng@intel.com>
> Sent: Friday, February 21, 2025 7:27 AM
> To: intel-gfx@lists.freedesktop.org
> Cc: Tseng, William <william.tseng@intel.com>; Kandpal, Suraj
> <suraj.kandpal@intel.com>; Heikkila, Juha-pekka <juha-
> pekka.heikkila@intel.com>
> Subject: [PATCH v2] drm/i915/dsc: Change rc parameters calculation for DSC 1.1

I think you missed one of my previous comments in which I asked you to send it to the intel-xe mailing list
So that the CI can test more thoroughly.
Also this patch is causing a regression on DG2 + DSC 1.1 config my guess is it would cause a issue on any
Machine 13 and above with DSC 1.1 when input bpp is 24 and compressed bpp is 18

> 
> When calculating dsc parameters, the rc parameters calculated by
> calculate_rc_params() is incorrect in the case of DSC 1.1 and
> DISPLAY_VER(dev_priv) >= 13. It turns out to be some noise-like lines are
> displayed on a MIPI DSI panel supporting DSC 1.1.
> 
> The function calculate_rc_params() is implemented by following the Table E-2
> in DSC 1.2a and creates incorrect rc parameters for DSC 1.1. As a result, add the
> additional condition (vdsc_cfg->dsc_version_minor >= 2) to get the right
> function to calculate rc parameters for DSC 1.1.
> 
> v1: initial version.
> v2: modify the commit comment and change the condition
>     regarding DSC version checking.
> 
> Cc: Suraj Kandpal <suraj.kandpal@intel.com>
> Cc: Juha-Pekka Heikkil <juha-pekka.heikkila@intel.com>
> Signed-off-by: William Tseng <william.tseng@intel.com>

Needs a closes-by tag

Regards,
Suraj Kandpal

> ---
>  drivers/gpu/drm/i915/display/intel_vdsc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> b/drivers/gpu/drm/i915/display/intel_vdsc.c
> index b355c479eda3..b0a7a2179844 100644
> --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> @@ -320,7 +320,7 @@ int intel_dsc_compute_params(struct intel_crtc_state
> *pipe_config)
>  	 * upto uncompressed bpp-1, hence add calculations for all the rc
>  	 * parameters
>  	 */
> -	if (DISPLAY_VER(dev_priv) >= 13) {
> +	if (DISPLAY_VER(dev_priv) >= 13 && vdsc_cfg->dsc_version_minor >=
> 2) {
>  		calculate_rc_params(vdsc_cfg);
>  	} else {
>  		if ((compressed_bpp == 8 ||
> --
> 2.34.1
Tseng, William Feb. 21, 2025, 7:23 a.m. UTC | #2 
> -----Original Message-----
> From: Kandpal, Suraj <suraj.kandpal@intel.com>
> Sent: Friday, February 21, 2025 11:43 AM
> To: Tseng, William <william.tseng@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: Heikkila, Juha-pekka <juha-pekka.heikkila@intel.com>
> Subject: RE: [PATCH v2] drm/i915/dsc: Change rc parameters calculation for
> DSC 1.1
> 
> 
> 
> > -----Original Message-----
> > From: Tseng, William <william.tseng@intel.com>
> > Sent: Friday, February 21, 2025 7:27 AM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: Tseng, William <william.tseng@intel.com>; Kandpal, Suraj
> > <suraj.kandpal@intel.com>; Heikkila, Juha-pekka <juha-
> > pekka.heikkila@intel.com>
> > Subject: [PATCH v2] drm/i915/dsc: Change rc parameters calculation for
> > DSC 1.1
> 
> I think you missed one of my previous comments in which I asked you to send
> it to the intel-xe mailing list So that the CI can test more thoroughly.
> Also this patch is causing a regression on DG2 + DSC 1.1 config my guess is it
> would cause a issue on any Machine 13 and above with DSC 1.1 when input
> bpp is 24 and compressed bpp is 18
> 

Sorry for missing the comment about the mailing list intel-xe.
I will send the revised patch to it next time.

As to the regression concern, I think it won't happen if DSC 1.1
does not support the case of input bpp is 24 and compressed bpp is 18.
What do you think of it?

> >
> > When calculating dsc parameters, the rc parameters calculated by
> > calculate_rc_params() is incorrect in the case of DSC 1.1 and
> > DISPLAY_VER(dev_priv) >= 13. It turns out to be some noise-like lines
> > are displayed on a MIPI DSI panel supporting DSC 1.1.
> >
> > The function calculate_rc_params() is implemented by following the
> > Table E-2 in DSC 1.2a and creates incorrect rc parameters for DSC 1.1.
> > As a result, add the additional condition (vdsc_cfg->dsc_version_minor
> > >= 2) to get the right function to calculate rc parameters for DSC 1.1.
> >
> > v1: initial version.
> > v2: modify the commit comment and change the condition
> >     regarding DSC version checking.
> >
> > Cc: Suraj Kandpal <suraj.kandpal@intel.com>
> > Cc: Juha-Pekka Heikkil <juha-pekka.heikkila@intel.com>
> > Signed-off-by: William Tseng <william.tseng@intel.com>
> 
> Needs a closes-by tag
> 

Will add closes-by as the line. Is it correct?
closes-by : https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13719

Regards
William

> Regards,
> Suraj Kandpal
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_vdsc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > index b355c479eda3..b0a7a2179844 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > @@ -320,7 +320,7 @@ int intel_dsc_compute_params(struct
> > intel_crtc_state
> > *pipe_config)
> >  	 * upto uncompressed bpp-1, hence add calculations for all the rc
> >  	 * parameters
> >  	 */
> > -	if (DISPLAY_VER(dev_priv) >= 13) {
> > +	if (DISPLAY_VER(dev_priv) >= 13 && vdsc_cfg->dsc_version_minor >=
> > 2) {
> >  		calculate_rc_params(vdsc_cfg);
> >  	} else {
> >  		if ((compressed_bpp == 8 ||
> > --
> > 2.34.1
Kandpal, Suraj Feb. 21, 2025, 8:15 a.m. UTC | #3 
> -----Original Message-----
> From: Tseng, William <william.tseng@intel.com>
> Sent: Friday, February 21, 2025 12:53 PM
> To: Kandpal, Suraj <suraj.kandpal@intel.com>; intel-gfx@lists.freedesktop.org;
> intel-xe@lists.freedesktop.org
> Cc: Heikkila, Juha-pekka <juha-pekka.heikkila@intel.com>
> Subject: RE: [PATCH v2] drm/i915/dsc: Change rc parameters calculation for
> DSC 1.1
> 
> 
> 
> > -----Original Message-----
> > From: Kandpal, Suraj <suraj.kandpal@intel.com>
> > Sent: Friday, February 21, 2025 11:43 AM
> > To: Tseng, William <william.tseng@intel.com>;
> > intel-gfx@lists.freedesktop.org
> > Cc: Heikkila, Juha-pekka <juha-pekka.heikkila@intel.com>
> > Subject: RE: [PATCH v2] drm/i915/dsc: Change rc parameters calculation
> > for DSC 1.1
> >
> >
> >
> > > -----Original Message-----
> > > From: Tseng, William <william.tseng@intel.com>
> > > Sent: Friday, February 21, 2025 7:27 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Cc: Tseng, William <william.tseng@intel.com>; Kandpal, Suraj
> > > <suraj.kandpal@intel.com>; Heikkila, Juha-pekka <juha-
> > > pekka.heikkila@intel.com>
> > > Subject: [PATCH v2] drm/i915/dsc: Change rc parameters calculation
> > > for DSC 1.1
> >
> > I think you missed one of my previous comments in which I asked you to
> > send it to the intel-xe mailing list So that the CI can test more thoroughly.
> > Also this patch is causing a regression on DG2 + DSC 1.1 config my
> > guess is it would cause a issue on any Machine 13 and above with DSC
> > 1.1 when input bpp is 24 and compressed bpp is 18
> >
> 
> Sorry for missing the comment about the mailing list intel-xe.
> I will send the revised patch to it next time.
> 
> As to the regression concern, I think it won't happen if DSC 1.1 does not support
> the case of input bpp is 24 and compressed bpp is 18.
> What do you think of it?

DSC 1.1 does support input bpc uptil 12 so it should support input bpp of 24 and compressed bpp of 18
I don't see anywhere in the spec where it says otherwise.

> 
> > >
> > > When calculating dsc parameters, the rc parameters calculated by
> > > calculate_rc_params() is incorrect in the case of DSC 1.1 and
> > > DISPLAY_VER(dev_priv) >= 13. It turns out to be some noise-like
> > > lines are displayed on a MIPI DSI panel supporting DSC 1.1.
> > >
> > > The function calculate_rc_params() is implemented by following the
> > > Table E-2 in DSC 1.2a and creates incorrect rc parameters for DSC 1.1.
> > > As a result, add the additional condition
> > > (vdsc_cfg->dsc_version_minor
> > > >= 2) to get the right function to calculate rc parameters for DSC 1.1.
> > >
> > > v1: initial version.
> > > v2: modify the commit comment and change the condition
> > >     regarding DSC version checking.
> > >
> > > Cc: Suraj Kandpal <suraj.kandpal@intel.com>
> > > Cc: Juha-Pekka Heikkil <juha-pekka.heikkila@intel.com>
> > > Signed-off-by: William Tseng <william.tseng@intel.com>
> >
> > Needs a closes-by tag
> >
> 
> Will add closes-by as the line. Is it correct?
> closes-by : https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13719

It should be Closes: 
My bad
Also lets refrain from sending new patches/ revisions till we root cause this.

Regards,
Suraj Kandpal

> 
> Regards
> William
> 
> > Regards,
> > Suraj Kandpal
> >
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_vdsc.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > index b355c479eda3..b0a7a2179844 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
> > > @@ -320,7 +320,7 @@ int intel_dsc_compute_params(struct
> > > intel_crtc_state
> > > *pipe_config)
> > >  	 * upto uncompressed bpp-1, hence add calculations for all the rc
> > >  	 * parameters
> > >  	 */
> > > -	if (DISPLAY_VER(dev_priv) >= 13) {
> > > +	if (DISPLAY_VER(dev_priv) >= 13 && vdsc_cfg->dsc_version_minor >=
> > > 2) {
> > >  		calculate_rc_params(vdsc_cfg);
> > >  	} else {
> > >  		if ((compressed_bpp == 8 ||
> > > --
> > > 2.34.1
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c
index b355c479eda3..b0a7a2179844 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
@@ -320,7 +320,7 @@  int intel_dsc_compute_params(struct intel_crtc_state *pipe_config)
 	 * upto uncompressed bpp-1, hence add calculations for all the rc
 	 * parameters
 	 */
-	if (DISPLAY_VER(dev_priv) >= 13) {
+	if (DISPLAY_VER(dev_priv) >= 13 && vdsc_cfg->dsc_version_minor >= 2) {
 		calculate_rc_params(vdsc_cfg);
 	} else {
 		if ((compressed_bpp == 8 ||