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, "Kandpal, Suraj" <suraj.kandpal@intel.com> wrote:
> 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

Parse error. :p

BR,
Jani.

>
> Regards,
> Suraj Kandpal
>> >
>> > 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
>> 
>> 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@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