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