[Intel-gfx] [PATCH] drm/i915/dsi: Explicit first_line_bpg_offset assignment for DSI

[Kandpal, Suraj]

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 at linux.intel.com> wrote:
> > > On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal at intel.com>
> wrote:
> > >>> On Mon, 07 Aug 2023, Suraj Kandpal <suraj.kandpal at 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 at 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