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

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