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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 16 Aug 2023, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 16 Aug 2023, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
>>> On Mon, 07 Aug 2023, Suraj Kandpal <suraj.kandpal@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?

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?

BR,
Jani.


>
>
>>> >
>>> > Signed-off-by: Suraj Kandpal <suraj.kandpal@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