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

[Jani Nikula]

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
does the array have post-SCR values for first_line_bpg_offset?


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