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