| Message ID | 20230807142754.3891787-1-suraj.kandpal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dsi: Explicit first_line_bpg_offset assignment for DSI | expand |
The patch is tested and the issue is resolved. Thank you. Tested-by: William Tseng <william.tseng@intel.com> -----Original Message----- From: Kandpal, Suraj <suraj.kandpal@intel.com> Sent: Monday, August 7, 2023 10:28 PM To: intel-gfx@lists.freedesktop.org Cc: Nautiyal, Ankit K <ankit.k.nautiyal@intel.com>; Tseng, William <william.tseng@intel.com>; Heikkila, Juha-pekka <juha-pekka.heikkila@intel.com>; Kandpal, Suraj <suraj.kandpal@intel.com> Subject: [PATCH] drm/i915/dsi: Explicit first_line_bpg_offset assignment for DSI Assign explicit value of 12 at 8bpp as per Table E2 of DSC 1.1 for DSI panels even though we already use calculations from CModel for first_line_bpg_offset. The reason being some DSI monitors may have not have added the change in errata for the calculation of first_line_bpg_offset. Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- drivers/gpu/drm/i915/display/icl_dsi.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c index f7ebc146f96d..2376d5000d78 100644 --- a/drivers/gpu/drm/i915/display/icl_dsi.c +++ b/drivers/gpu/drm/i915/display/icl_dsi.c @@ -1585,6 +1585,11 @@ static int gen11_dsi_dsc_compute_config(struct intel_encoder *encoder, if (ret) return ret; + /* From Table E-2 in DSC 1.1*/ + if (vdsc_cfg->dsc_version_minor == 1 && + vdsc_cfg->bits_per_pixel == 128) + vdsc_cfg->first_line_bpg_offset = 12; + /* DSI specific sanity checks on the common code */ drm_WARN_ON(&dev_priv->drm, vdsc_cfg->vbr_enable); drm_WARN_ON(&dev_priv->drm, vdsc_cfg->simple_422); -- 2.25.1
On Mon, 07 Aug 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote: > Assign explicit value of 12 at 8bpp as per Table E2 of DSC 1.1 for DSI > panels even though we already use calculations from CModel for > first_line_bpg_offset. > The reason being some DSI monitors may have not have added the > change in errata for the calculation of first_line_bpg_offset. > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > --- > drivers/gpu/drm/i915/display/icl_dsi.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c > index f7ebc146f96d..2376d5000d78 100644 > --- a/drivers/gpu/drm/i915/display/icl_dsi.c > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c > @@ -1585,6 +1585,11 @@ static int gen11_dsi_dsc_compute_config(struct intel_encoder *encoder, > if (ret) > return ret; > > + /* From Table E-2 in DSC 1.1*/ > + if (vdsc_cfg->dsc_version_minor == 1 && > + vdsc_cfg->bits_per_pixel == 128) So, vdsc_cfg->bits_per_pixel has 4 fractional bits, and that's 8 bpp compressed? Better describe it that way, instead of as 128. But... looking around, in intel_vdsc.c we set: pps_val |= DSC_BPP(vdsc_cfg->bits_per_pixel); and we have: #define DSC_BPP(bpp) ((bpp) << 4) however, when reading it back in intel_dsc_get_config(), it's just directly: vdsc_cfg->bits_per_pixel = pps1; Are we always sending x16 bpp in PPS? BR, Jani. > + vdsc_cfg->first_line_bpg_offset = 12; > + > /* DSI specific sanity checks on the common code */ > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->vbr_enable); > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->simple_422);
> On Mon, 07 Aug 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote: > > Assign explicit value of 12 at 8bpp as per Table E2 of DSC 1.1 for DSI > > panels even though we already use calculations from CModel for > > first_line_bpg_offset. > > The reason being some DSI monitors may have not have added the change > > in errata for the calculation of first_line_bpg_offset. > > > > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > --- > > drivers/gpu/drm/i915/display/icl_dsi.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c > > b/drivers/gpu/drm/i915/display/icl_dsi.c > > index f7ebc146f96d..2376d5000d78 100644 > > --- a/drivers/gpu/drm/i915/display/icl_dsi.c > > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c > > @@ -1585,6 +1585,11 @@ static int gen11_dsi_dsc_compute_config(struct > intel_encoder *encoder, > > if (ret) > > return ret; > > > > + /* From Table E-2 in DSC 1.1*/ > > + if (vdsc_cfg->dsc_version_minor == 1 && > > + vdsc_cfg->bits_per_pixel == 128) > Hi Jani, Thanks for the review > So, vdsc_cfg->bits_per_pixel has 4 fractional bits, and that's 8 bpp > compressed? > > Better describe it that way, instead of as 128. > Yes would be better to right shift (vdsc_cfg->bits_per_pixel) by 4 then compare with 8 to avoid confusion. > But... looking around, in intel_vdsc.c we set: > > pps_val |= DSC_BPP(vdsc_cfg->bits_per_pixel); > > and we have: > > #define DSC_BPP(bpp) ((bpp) << 4) > > however, when reading it back in intel_dsc_get_config(), it's just > directly: > > vdsc_cfg->bits_per_pixel = pps1; > > Are we always sending x16 bpp in PPS? Yes we are always sending bpp x16 considering the fractional bits also in intel_vdsc_regs.h We have #define DSC_BPP(bpp) ((bpp) << 0) Which in hindsight can be renamed as it has the same name as the one in drm_dsc_helper.c But then again the DSC_BPP macro there is more local to that file. Moreover vdsc_cfg->bits_per_pixel is being set in intel_dsc_compute_params(among other places but is still being set x16 the value). vdsc_cfg->bits_per_pixel = compressed_bpp << 4; Regards, Suraj Kandpal > > > BR, > Jani. > > > > > + vdsc_cfg->first_line_bpg_offset = 12; > > + > > /* DSI specific sanity checks on the common code */ > > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->vbr_enable); > > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->simple_422); > > -- > Jani Nikula, Intel Open Source Graphics Center
On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote: >> On Mon, 07 Aug 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote: >> > Assign explicit value of 12 at 8bpp as per Table E2 of DSC 1.1 for DSI >> > panels even though we already use calculations from CModel for >> > first_line_bpg_offset. >> > The reason being some DSI monitors may have not have added the change >> > in errata for the calculation of first_line_bpg_offset. We should be using DRM_DSC_1_1_PRE_SCR parameters for this, right? Why does the array have post-SCR values for first_line_bpg_offset? >> > >> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/icl_dsi.c | 5 +++++ >> > 1 file changed, 5 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c >> > b/drivers/gpu/drm/i915/display/icl_dsi.c >> > index f7ebc146f96d..2376d5000d78 100644 >> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c >> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c >> > @@ -1585,6 +1585,11 @@ static int gen11_dsi_dsc_compute_config(struct >> intel_encoder *encoder, >> > if (ret) >> > return ret; >> > >> > + /* From Table E-2 in DSC 1.1*/ >> > + if (vdsc_cfg->dsc_version_minor == 1 && >> > + vdsc_cfg->bits_per_pixel == 128) >> > Hi Jani, > Thanks for the review > >> So, vdsc_cfg->bits_per_pixel has 4 fractional bits, and that's 8 bpp >> compressed? >> >> Better describe it that way, instead of as 128. >> > > Yes would be better to right shift (vdsc_cfg->bits_per_pixel) by 4 then compare with 8 > to avoid confusion. > >> But... looking around, in intel_vdsc.c we set: >> >> pps_val |= DSC_BPP(vdsc_cfg->bits_per_pixel); >> >> and we have: >> >> #define DSC_BPP(bpp) ((bpp) << 4) >> >> however, when reading it back in intel_dsc_get_config(), it's just >> directly: >> >> vdsc_cfg->bits_per_pixel = pps1; >> >> Are we always sending x16 bpp in PPS? > > Yes we are always sending bpp x16 considering the fractional bits also in intel_vdsc_regs.h > We have > #define DSC_BPP(bpp) ((bpp) << 0) This is the part that confused me. BR, Jani. > Which in hindsight can be renamed as it has the same name as the one in drm_dsc_helper.c > But then again the DSC_BPP macro there is more local to that file. > > Moreover vdsc_cfg->bits_per_pixel is being set in intel_dsc_compute_params(among other places > but is still being set x16 the value). > > vdsc_cfg->bits_per_pixel = compressed_bpp << 4; > > Regards, > Suraj Kandpal >> >> >> BR, >> Jani. >> >> >> >> > + vdsc_cfg->first_line_bpg_offset = 12; >> > + >> > /* DSI specific sanity checks on the common code */ >> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->vbr_enable); >> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->simple_422); >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
On Wed, 16 Aug 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote: >>> On Mon, 07 Aug 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote: >>> > Assign explicit value of 12 at 8bpp as per Table E2 of DSC 1.1 for DSI >>> > panels even though we already use calculations from CModel for >>> > first_line_bpg_offset. >>> > The reason being some DSI monitors may have not have added the change >>> > in errata for the calculation of first_line_bpg_offset. > > We should be using DRM_DSC_1_1_PRE_SCR parameters for this, right? Why > does the array have post-SCR values for first_line_bpg_offset? I'm asking for logs on the gitlab issue, and trying to get at the root of this, because so many times in the past adding a specific fix like this for a specific panel (albeit using generic conditions), it has caused more troule for other panels in the future. What if other panels don't work with 12? BR, Jani. > > >>> > >>> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> >>> > --- >>> > drivers/gpu/drm/i915/display/icl_dsi.c | 5 +++++ >>> > 1 file changed, 5 insertions(+) >>> > >>> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c >>> > b/drivers/gpu/drm/i915/display/icl_dsi.c >>> > index f7ebc146f96d..2376d5000d78 100644 >>> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c >>> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c >>> > @@ -1585,6 +1585,11 @@ static int gen11_dsi_dsc_compute_config(struct >>> intel_encoder *encoder, >>> > if (ret) >>> > return ret; >>> > >>> > + /* From Table E-2 in DSC 1.1*/ >>> > + if (vdsc_cfg->dsc_version_minor == 1 && >>> > + vdsc_cfg->bits_per_pixel == 128) >>> >> Hi Jani, >> Thanks for the review >> >>> So, vdsc_cfg->bits_per_pixel has 4 fractional bits, and that's 8 bpp >>> compressed? >>> >>> Better describe it that way, instead of as 128. >>> >> >> Yes would be better to right shift (vdsc_cfg->bits_per_pixel) by 4 then compare with 8 >> to avoid confusion. >> >>> But... looking around, in intel_vdsc.c we set: >>> >>> pps_val |= DSC_BPP(vdsc_cfg->bits_per_pixel); >>> >>> and we have: >>> >>> #define DSC_BPP(bpp) ((bpp) << 4) >>> >>> however, when reading it back in intel_dsc_get_config(), it's just >>> directly: >>> >>> vdsc_cfg->bits_per_pixel = pps1; >>> >>> Are we always sending x16 bpp in PPS? >> >> Yes we are always sending bpp x16 considering the fractional bits also in intel_vdsc_regs.h >> We have >> #define DSC_BPP(bpp) ((bpp) << 0) > > This is the part that confused me. > > BR, > Jani. > >> Which in hindsight can be renamed as it has the same name as the one in drm_dsc_helper.c >> But then again the DSC_BPP macro there is more local to that file. >> >> Moreover vdsc_cfg->bits_per_pixel is being set in intel_dsc_compute_params(among other places >> but is still being set x16 the value). >> >> vdsc_cfg->bits_per_pixel = compressed_bpp << 4; >> >> Regards, >> Suraj Kandpal >>> >>> >>> BR, >>> Jani. >>> >>> >>> >>> > + vdsc_cfg->first_line_bpg_offset = 12; >>> > + >>> > /* DSI specific sanity checks on the common code */ >>> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->vbr_enable); >>> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->simple_422); >>> >>> -- >>> Jani Nikula, Intel Open Source Graphics Center
> > On Wed, 16 Aug 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote: > >>> On Mon, 07 Aug 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote: > >>> > Assign explicit value of 12 at 8bpp as per Table E2 of DSC 1.1 for > >>> > DSI panels even though we already use calculations from CModel for > >>> > first_line_bpg_offset. > >>> > The reason being some DSI monitors may have not have added the > >>> > change in errata for the calculation of first_line_bpg_offset. > > > > We should be using DRM_DSC_1_1_PRE_SCR parameters for this, right? > Why Sorry I seemed to have missed this comment in my previous reply but from how the code is written if display_ver >= 13 we call on calculate_rc_params which uses formulas to calculate the params and we don't rely on the dsc tables in drm_dsc_helpers so the DRM_DSC_1_1_PRE_SCR scenario does not come in picture. > > does the array have post-SCR values for first_line_bpg_offset? > > I'm asking for logs on the gitlab issue, and trying to get at the root of this, > because so many times in the past adding a specific fix like this for a specific > panel (albeit using generic conditions), it has caused more troule for other > panels in the future. What if other panels don't work with 12? > That is true which is why I too was unsure on the fix. Maybe Tseng can provide the logs them on the gitlab issue. Regards, Suraj Kandpal > BR, > Jani. > > > > > > > >>> > > >>> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > >>> > --- > >>> > drivers/gpu/drm/i915/display/icl_dsi.c | 5 +++++ > >>> > 1 file changed, 5 insertions(+) > >>> > > >>> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c > >>> > b/drivers/gpu/drm/i915/display/icl_dsi.c > >>> > index f7ebc146f96d..2376d5000d78 100644 > >>> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c > >>> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c > >>> > @@ -1585,6 +1585,11 @@ static int > >>> > gen11_dsi_dsc_compute_config(struct > >>> intel_encoder *encoder, > >>> > if (ret) > >>> > return ret; > >>> > > >>> > + /* From Table E-2 in DSC 1.1*/ > >>> > + if (vdsc_cfg->dsc_version_minor == 1 && > >>> > + vdsc_cfg->bits_per_pixel == 128) > >>> > >> Hi Jani, > >> Thanks for the review > >> > >>> So, vdsc_cfg->bits_per_pixel has 4 fractional bits, and that's 8 bpp > >>> compressed? > >>> > >>> Better describe it that way, instead of as 128. > >>> > >> > >> Yes would be better to right shift (vdsc_cfg->bits_per_pixel) by 4 > >> then compare with 8 to avoid confusion. > >> > >>> But... looking around, in intel_vdsc.c we set: > >>> > >>> pps_val |= DSC_BPP(vdsc_cfg->bits_per_pixel); > >>> > >>> and we have: > >>> > >>> #define DSC_BPP(bpp) ((bpp) << 4) > >>> > >>> however, when reading it back in intel_dsc_get_config(), it's just > >>> directly: > >>> > >>> vdsc_cfg->bits_per_pixel = pps1; > >>> > >>> Are we always sending x16 bpp in PPS? > >> > >> Yes we are always sending bpp x16 considering the fractional bits > >> also in intel_vdsc_regs.h We have > >> #define DSC_BPP(bpp) ((bpp) << 0) > > > > This is the part that confused me. > > > > BR, > > Jani. > > > >> Which in hindsight can be renamed as it has the same name as the one > >> in drm_dsc_helper.c But then again the DSC_BPP macro there is more > local to that file. > >> > >> Moreover vdsc_cfg->bits_per_pixel is being set in > >> intel_dsc_compute_params(among other places but is still being set x16 > the value). > >> > >> vdsc_cfg->bits_per_pixel = compressed_bpp << 4; > >> > >> Regards, > >> Suraj Kandpal > >>> > >>> > >>> BR, > >>> Jani. > >>> > >>> > >>> > >>> > + vdsc_cfg->first_line_bpg_offset = 12; > >>> > + > >>> > /* DSI specific sanity checks on the common code */ > >>> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->vbr_enable); > >>> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->simple_422); > >>> > >>> -- > >>> Jani Nikula, Intel Open Source Graphics Center > > -- > Jani Nikula, Intel Open Source Graphics Center
Also a small thing I needed to add is we found this patch series https://patchwork.freedesktop.org/patch/549863/?series=121487&rev=2 where panel sets the slice height to 40 if a value around 30 is given it should work not sure how relevant this is here but just an FYI Regards, Suraj Kandpal > > > > On Wed, 16 Aug 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote: > > > On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> > wrote: > > >>> On Mon, 07 Aug 2023, Suraj Kandpal <suraj.kandpal@intel.com> > wrote: > > >>> > Assign explicit value of 12 at 8bpp as per Table E2 of DSC 1.1 > > >>> > for DSI panels even though we already use calculations from > > >>> > CModel for first_line_bpg_offset. > > >>> > The reason being some DSI monitors may have not have added the > > >>> > change in errata for the calculation of first_line_bpg_offset. > > > > > > We should be using DRM_DSC_1_1_PRE_SCR parameters for this, right? > > Why > > Sorry I seemed to have missed this comment in my previous reply but from > how the code is written if display_ver >= 13 we call on calculate_rc_params > which uses formulas to calculate the params and we don't rely on the dsc > tables in drm_dsc_helpers so the DRM_DSC_1_1_PRE_SCR scenario does not > come in picture. > > > > does the array have post-SCR values for first_line_bpg_offset? > > > > I'm asking for logs on the gitlab issue, and trying to get at the root > > of this, because so many times in the past adding a specific fix like > > this for a specific panel (albeit using generic conditions), it has > > caused more troule for other panels in the future. What if other panels > don't work with 12? > > > > That is true which is why I too was unsure on the fix. > > Maybe Tseng can provide the logs them on the gitlab issue. > > Regards, > Suraj Kandpal > > > BR, > > Jani. > > > > > > > > > > > > >>> > > > >>> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > > >>> > --- > > >>> > drivers/gpu/drm/i915/display/icl_dsi.c | 5 +++++ > > >>> > 1 file changed, 5 insertions(+) > > >>> > > > >>> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c > > >>> > b/drivers/gpu/drm/i915/display/icl_dsi.c > > >>> > index f7ebc146f96d..2376d5000d78 100644 > > >>> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c > > >>> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c > > >>> > @@ -1585,6 +1585,11 @@ static int > > >>> > gen11_dsi_dsc_compute_config(struct > > >>> intel_encoder *encoder, > > >>> > if (ret) > > >>> > return ret; > > >>> > > > >>> > + /* From Table E-2 in DSC 1.1*/ > > >>> > + if (vdsc_cfg->dsc_version_minor == 1 && > > >>> > + vdsc_cfg->bits_per_pixel == 128) > > >>> > > >> Hi Jani, > > >> Thanks for the review > > >> > > >>> So, vdsc_cfg->bits_per_pixel has 4 fractional bits, and that's 8 > > >>> bpp compressed? > > >>> > > >>> Better describe it that way, instead of as 128. > > >>> > > >> > > >> Yes would be better to right shift (vdsc_cfg->bits_per_pixel) by 4 > > >> then compare with 8 to avoid confusion. > > >> > > >>> But... looking around, in intel_vdsc.c we set: > > >>> > > >>> pps_val |= DSC_BPP(vdsc_cfg->bits_per_pixel); > > >>> > > >>> and we have: > > >>> > > >>> #define DSC_BPP(bpp) ((bpp) << 4) > > >>> > > >>> however, when reading it back in intel_dsc_get_config(), it's just > > >>> directly: > > >>> > > >>> vdsc_cfg->bits_per_pixel = pps1; > > >>> > > >>> Are we always sending x16 bpp in PPS? > > >> > > >> Yes we are always sending bpp x16 considering the fractional bits > > >> also in intel_vdsc_regs.h We have > > >> #define DSC_BPP(bpp) ((bpp) << 0) > > > > > > This is the part that confused me. > > > > > > BR, > > > Jani. > > > > > >> Which in hindsight can be renamed as it has the same name as the > > >> one in drm_dsc_helper.c But then again the DSC_BPP macro there is > > >> more > > local to that file. > > >> > > >> Moreover vdsc_cfg->bits_per_pixel is being set in > > >> intel_dsc_compute_params(among other places but is still being set > > >> x16 > > the value). > > >> > > >> vdsc_cfg->bits_per_pixel = compressed_bpp << 4; > > >> > > >> Regards, > > >> Suraj Kandpal > > >>> > > >>> > > >>> BR, > > >>> Jani. > > >>> > > >>> > > >>> > > >>> > + vdsc_cfg->first_line_bpg_offset = 12; > > >>> > + > > >>> > /* DSI specific sanity checks on the common code */ > > >>> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->vbr_enable); > > >>> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->simple_422); > > >>> > > >>> -- > > >>> Jani Nikula, Intel Open Source Graphics Center > > > > -- > > Jani Nikula, Intel Open Source Graphics Center
On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote: >> >> On Wed, 16 Aug 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote: >> > On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote: >> >>> On Mon, 07 Aug 2023, Suraj Kandpal <suraj.kandpal@intel.com> wrote: >> >>> > Assign explicit value of 12 at 8bpp as per Table E2 of DSC 1.1 for >> >>> > DSI panels even though we already use calculations from CModel for >> >>> > first_line_bpg_offset. >> >>> > The reason being some DSI monitors may have not have added the >> >>> > change in errata for the calculation of first_line_bpg_offset. >> > >> > We should be using DRM_DSC_1_1_PRE_SCR parameters for this, right? >> Why > > Sorry I seemed to have missed this comment in my previous reply but > from how the code is written if display_ver >= 13 we call on calculate_rc_params > which uses formulas to calculate the params and we don't rely on the dsc tables > in drm_dsc_helpers so the DRM_DSC_1_1_PRE_SCR scenario does not come in picture. Ah, right. Thought it was display 12 in question. But why do we make that decision purely on source capability, also for DSC 1.1 sinks? BR, Jani. > >> > does the array have post-SCR values for first_line_bpg_offset? >> >> I'm asking for logs on the gitlab issue, and trying to get at the root of this, >> because so many times in the past adding a specific fix like this for a specific >> panel (albeit using generic conditions), it has caused more troule for other >> panels in the future. What if other panels don't work with 12? >> > > That is true which is why I too was unsure on the fix. > > Maybe Tseng can provide the logs them on the gitlab issue. > > Regards, > Suraj Kandpal > >> BR, >> Jani. >> >> >> > >> > >> >>> > >> >>> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> >> >>> > --- >> >>> > drivers/gpu/drm/i915/display/icl_dsi.c | 5 +++++ >> >>> > 1 file changed, 5 insertions(+) >> >>> > >> >>> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c >> >>> > b/drivers/gpu/drm/i915/display/icl_dsi.c >> >>> > index f7ebc146f96d..2376d5000d78 100644 >> >>> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c >> >>> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c >> >>> > @@ -1585,6 +1585,11 @@ static int >> >>> > gen11_dsi_dsc_compute_config(struct >> >>> intel_encoder *encoder, >> >>> > if (ret) >> >>> > return ret; >> >>> > >> >>> > + /* From Table E-2 in DSC 1.1*/ >> >>> > + if (vdsc_cfg->dsc_version_minor == 1 && >> >>> > + vdsc_cfg->bits_per_pixel == 128) >> >>> >> >> Hi Jani, >> >> Thanks for the review >> >> >> >>> So, vdsc_cfg->bits_per_pixel has 4 fractional bits, and that's 8 bpp >> >>> compressed? >> >>> >> >>> Better describe it that way, instead of as 128. >> >>> >> >> >> >> Yes would be better to right shift (vdsc_cfg->bits_per_pixel) by 4 >> >> then compare with 8 to avoid confusion. >> >> >> >>> But... looking around, in intel_vdsc.c we set: >> >>> >> >>> pps_val |= DSC_BPP(vdsc_cfg->bits_per_pixel); >> >>> >> >>> and we have: >> >>> >> >>> #define DSC_BPP(bpp) ((bpp) << 4) >> >>> >> >>> however, when reading it back in intel_dsc_get_config(), it's just >> >>> directly: >> >>> >> >>> vdsc_cfg->bits_per_pixel = pps1; >> >>> >> >>> Are we always sending x16 bpp in PPS? >> >> >> >> Yes we are always sending bpp x16 considering the fractional bits >> >> also in intel_vdsc_regs.h We have >> >> #define DSC_BPP(bpp) ((bpp) << 0) >> > >> > This is the part that confused me. >> > >> > BR, >> > Jani. >> > >> >> Which in hindsight can be renamed as it has the same name as the one >> >> in drm_dsc_helper.c But then again the DSC_BPP macro there is more >> local to that file. >> >> >> >> Moreover vdsc_cfg->bits_per_pixel is being set in >> >> intel_dsc_compute_params(among other places but is still being set x16 >> the value). >> >> >> >> vdsc_cfg->bits_per_pixel = compressed_bpp << 4; >> >> >> >> Regards, >> >> Suraj Kandpal >> >>> >> >>> >> >>> BR, >> >>> Jani. >> >>> >> >>> >> >>> >> >>> > + vdsc_cfg->first_line_bpg_offset = 12; >> >>> > + >> >>> > /* DSI specific sanity checks on the common code */ >> >>> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->vbr_enable); >> >>> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->simple_422); >> >>> >> >>> -- >> >>> Jani Nikula, Intel Open Source Graphics Center >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote: > Also a small thing I needed to add is we found this patch series > https://patchwork.freedesktop.org/patch/549863/?series=121487&rev=2 > > where panel sets the slice height to 40 if a value around 30 is given > it should work not sure how relevant this is here but just an FYI Parse error. :p BR, Jani. > > Regards, > Suraj Kandpal >> > >> > On Wed, 16 Aug 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote: >> > > On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> >> wrote: >> > >>> On Mon, 07 Aug 2023, Suraj Kandpal <suraj.kandpal@intel.com> >> wrote: >> > >>> > Assign explicit value of 12 at 8bpp as per Table E2 of DSC 1.1 >> > >>> > for DSI panels even though we already use calculations from >> > >>> > CModel for first_line_bpg_offset. >> > >>> > The reason being some DSI monitors may have not have added the >> > >>> > change in errata for the calculation of first_line_bpg_offset. >> > > >> > > We should be using DRM_DSC_1_1_PRE_SCR parameters for this, right? >> > Why >> >> Sorry I seemed to have missed this comment in my previous reply but from >> how the code is written if display_ver >= 13 we call on calculate_rc_params >> which uses formulas to calculate the params and we don't rely on the dsc >> tables in drm_dsc_helpers so the DRM_DSC_1_1_PRE_SCR scenario does not >> come in picture. >> >> > > does the array have post-SCR values for first_line_bpg_offset? >> > >> > I'm asking for logs on the gitlab issue, and trying to get at the root >> > of this, because so many times in the past adding a specific fix like >> > this for a specific panel (albeit using generic conditions), it has >> > caused more troule for other panels in the future. What if other panels >> don't work with 12? >> > >> >> That is true which is why I too was unsure on the fix. >> >> Maybe Tseng can provide the logs them on the gitlab issue. >> >> Regards, >> Suraj Kandpal >> >> > BR, >> > Jani. >> > >> > >> > > >> > > >> > >>> > >> > >>> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> >> > >>> > --- >> > >>> > drivers/gpu/drm/i915/display/icl_dsi.c | 5 +++++ >> > >>> > 1 file changed, 5 insertions(+) >> > >>> > >> > >>> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c >> > >>> > b/drivers/gpu/drm/i915/display/icl_dsi.c >> > >>> > index f7ebc146f96d..2376d5000d78 100644 >> > >>> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c >> > >>> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c >> > >>> > @@ -1585,6 +1585,11 @@ static int >> > >>> > gen11_dsi_dsc_compute_config(struct >> > >>> intel_encoder *encoder, >> > >>> > if (ret) >> > >>> > return ret; >> > >>> > >> > >>> > + /* From Table E-2 in DSC 1.1*/ >> > >>> > + if (vdsc_cfg->dsc_version_minor == 1 && >> > >>> > + vdsc_cfg->bits_per_pixel == 128) >> > >>> >> > >> Hi Jani, >> > >> Thanks for the review >> > >> >> > >>> So, vdsc_cfg->bits_per_pixel has 4 fractional bits, and that's 8 >> > >>> bpp compressed? >> > >>> >> > >>> Better describe it that way, instead of as 128. >> > >>> >> > >> >> > >> Yes would be better to right shift (vdsc_cfg->bits_per_pixel) by 4 >> > >> then compare with 8 to avoid confusion. >> > >> >> > >>> But... looking around, in intel_vdsc.c we set: >> > >>> >> > >>> pps_val |= DSC_BPP(vdsc_cfg->bits_per_pixel); >> > >>> >> > >>> and we have: >> > >>> >> > >>> #define DSC_BPP(bpp) ((bpp) << 4) >> > >>> >> > >>> however, when reading it back in intel_dsc_get_config(), it's just >> > >>> directly: >> > >>> >> > >>> vdsc_cfg->bits_per_pixel = pps1; >> > >>> >> > >>> Are we always sending x16 bpp in PPS? >> > >> >> > >> Yes we are always sending bpp x16 considering the fractional bits >> > >> also in intel_vdsc_regs.h We have >> > >> #define DSC_BPP(bpp) ((bpp) << 0) >> > > >> > > This is the part that confused me. >> > > >> > > BR, >> > > Jani. >> > > >> > >> Which in hindsight can be renamed as it has the same name as the >> > >> one in drm_dsc_helper.c But then again the DSC_BPP macro there is >> > >> more >> > local to that file. >> > >> >> > >> Moreover vdsc_cfg->bits_per_pixel is being set in >> > >> intel_dsc_compute_params(among other places but is still being set >> > >> x16 >> > the value). >> > >> >> > >> vdsc_cfg->bits_per_pixel = compressed_bpp << 4; >> > >> >> > >> Regards, >> > >> Suraj Kandpal >> > >>> >> > >>> >> > >>> BR, >> > >>> Jani. >> > >>> >> > >>> >> > >>> >> > >>> > + vdsc_cfg->first_line_bpg_offset = 12; >> > >>> > + >> > >>> > /* DSI specific sanity checks on the common code */ >> > >>> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->vbr_enable); >> > >>> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->simple_422); >> > >>> >> > >>> -- >> > >>> Jani Nikula, Intel Open Source Graphics Center >> > >> > -- >> > Jani Nikula, Intel Open Source Graphics Center
] [PATCH] drm/i915/dsi: Explicit first_line_bpg_offset > assignment for DSI > > On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote: > >> > >> On Wed, 16 Aug 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote: > >> > On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> > wrote: > >> >>> On Mon, 07 Aug 2023, Suraj Kandpal <suraj.kandpal@intel.com> > wrote: > >> >>> > Assign explicit value of 12 at 8bpp as per Table E2 of DSC 1.1 > >> >>> > for DSI panels even though we already use calculations from > >> >>> > CModel for first_line_bpg_offset. > >> >>> > The reason being some DSI monitors may have not have added the > >> >>> > change in errata for the calculation of first_line_bpg_offset. > >> > > >> > We should be using DRM_DSC_1_1_PRE_SCR parameters for this, right? > >> Why > > > > Sorry I seemed to have missed this comment in my previous reply but > > from how the code is written if display_ver >= 13 we call on > > calculate_rc_params which uses formulas to calculate the params and we > > don't rely on the dsc tables in drm_dsc_helpers so the > DRM_DSC_1_1_PRE_SCR scenario does not come in picture. > > Ah, right. Thought it was display 12 in question. But why do we make that > decision purely on source capability, also for DSC 1.1 sinks So the decision was made base on our capability to send or use bpp other than the standard 6,8,10,12 bpp and here the normal tables that gives the best static value don't work requiring us To use the formulas To answer you second question yes the change was made with DSC 1.1 sinks in mind and it show support it Have a look at the commit below gives a better picture db514cac08fd4861a3b221bed5f21b441a1242c3 drm/i915/xelpd: Calculate VDSC RC parameters Regards, Suraj Kandpal > > BR, > Jani. > > > > >> > does the array have post-SCR values for first_line_bpg_offset? > >> > >> I'm asking for logs on the gitlab issue, and trying to get at the > >> root of this, because so many times in the past adding a specific fix > >> like this for a specific panel (albeit using generic conditions), it > >> has caused more troule for other panels in the future. What if other > panels don't work with 12? > >> > > > > That is true which is why I too was unsure on the fix. > > > > Maybe Tseng can provide the logs them on the gitlab issue. > > > > Regards, > > Suraj Kandpal > > > >> BR, > >> Jani. > >> > >> > >> > > >> > > >> >>> > > >> >>> > Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> > >> >>> > --- > >> >>> > drivers/gpu/drm/i915/display/icl_dsi.c | 5 +++++ > >> >>> > 1 file changed, 5 insertions(+) > >> >>> > > >> >>> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c > >> >>> > b/drivers/gpu/drm/i915/display/icl_dsi.c > >> >>> > index f7ebc146f96d..2376d5000d78 100644 > >> >>> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c > >> >>> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c > >> >>> > @@ -1585,6 +1585,11 @@ static int > >> >>> > gen11_dsi_dsc_compute_config(struct > >> >>> intel_encoder *encoder, > >> >>> > if (ret) > >> >>> > return ret; > >> >>> > > >> >>> > + /* From Table E-2 in DSC 1.1*/ > >> >>> > + if (vdsc_cfg->dsc_version_minor == 1 && > >> >>> > + vdsc_cfg->bits_per_pixel == 128) > >> >>> > >> >> Hi Jani, > >> >> Thanks for the review > >> >> > >> >>> So, vdsc_cfg->bits_per_pixel has 4 fractional bits, and that's 8 > >> >>> bpp compressed? > >> >>> > >> >>> Better describe it that way, instead of as 128. > >> >>> > >> >> > >> >> Yes would be better to right shift (vdsc_cfg->bits_per_pixel) by > >> >> 4 then compare with 8 to avoid confusion. > >> >> > >> >>> But... looking around, in intel_vdsc.c we set: > >> >>> > >> >>> pps_val |= DSC_BPP(vdsc_cfg->bits_per_pixel); > >> >>> > >> >>> and we have: > >> >>> > >> >>> #define DSC_BPP(bpp) ((bpp) << 4) > >> >>> > >> >>> however, when reading it back in intel_dsc_get_config(), it's > >> >>> just > >> >>> directly: > >> >>> > >> >>> vdsc_cfg->bits_per_pixel = pps1; > >> >>> > >> >>> Are we always sending x16 bpp in PPS? > >> >> > >> >> Yes we are always sending bpp x16 considering the fractional bits > >> >> also in intel_vdsc_regs.h We have > >> >> #define DSC_BPP(bpp) ((bpp) << 0) > >> > > >> > This is the part that confused me. > >> > > >> > BR, > >> > Jani. > >> > > >> >> Which in hindsight can be renamed as it has the same name as the > >> >> one in drm_dsc_helper.c But then again the DSC_BPP macro there is > >> >> more > >> local to that file. > >> >> > >> >> Moreover vdsc_cfg->bits_per_pixel is being set in > >> >> intel_dsc_compute_params(among other places but is still being set > >> >> x16 > >> the value). > >> >> > >> >> vdsc_cfg->bits_per_pixel = compressed_bpp << 4; > >> >> > >> >> Regards, > >> >> Suraj Kandpal > >> >>> > >> >>> > >> >>> BR, > >> >>> Jani. > >> >>> > >> >>> > >> >>> > >> >>> > + vdsc_cfg->first_line_bpg_offset = 12; > >> >>> > + > >> >>> > /* DSI specific sanity checks on the common code */ > >> >>> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->vbr_enable); > >> >>> > drm_WARN_ON(&dev_priv->drm, vdsc_cfg->simple_422); > >> >>> > >> >>> -- > >> >>> Jani Nikula, Intel Open Source Graphics Center > >> > >> -- > >> Jani Nikula, Intel Open Source Graphics Center > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c index f7ebc146f96d..2376d5000d78 100644 --- a/drivers/gpu/drm/i915/display/icl_dsi.c +++ b/drivers/gpu/drm/i915/display/icl_dsi.c @@ -1585,6 +1585,11 @@ static int gen11_dsi_dsc_compute_config(struct intel_encoder *encoder, if (ret) return ret; + /* From Table E-2 in DSC 1.1*/ + if (vdsc_cfg->dsc_version_minor == 1 && + vdsc_cfg->bits_per_pixel == 128) + vdsc_cfg->first_line_bpg_offset = 12; + /* DSI specific sanity checks on the common code */ drm_WARN_ON(&dev_priv->drm, vdsc_cfg->vbr_enable); drm_WARN_ON(&dev_priv->drm, vdsc_cfg->simple_422);
Assign explicit value of 12 at 8bpp as per Table E2 of DSC 1.1 for DSI panels even though we already use calculations from CModel for first_line_bpg_offset. The reason being some DSI monitors may have not have added the change in errata for the calculation of first_line_bpg_offset. Signed-off-by: Suraj Kandpal <suraj.kandpal@intel.com> --- drivers/gpu/drm/i915/display/icl_dsi.c | 5 +++++ 1 file changed, 5 insertions(+)