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

[Kandpal, Suraj]

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