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@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
