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

[Kandpal, Suraj]

] [PATCH] drm/i915/dsi: Explicit first_line_bpg_offset
> assignment for DSI
> 
> On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal at intel.com> wrote:
> >>
> >> 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.
> 
> 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 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
> 
> --
> Jani Nikula, Intel Open Source Graphics Center