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