On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal@xxxxxxxxx> 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@xxxxxxxxxxxxxxx> wrote: >> > > On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal@xxxxxxxxx> >> wrote: >> > >>> On Mon, 07 Aug 2023, Suraj Kandpal <suraj.kandpal@xxxxxxxxx> >> 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@xxxxxxxxx> >> > >>> > --- >> > >>> > 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
