> On Mon, 10 Jul 2023, Suraj Kandpal <suraj.kandpal@xxxxxxxxx> wrote:
> > With the dsc config being readout and filled in crtc_state add macros
> > and use them to compare current and previous PPS param in DSC.
> >
> > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 55
> > ++++++++++++++++++++
> > 1 file changed, 55 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index eed01957bdb9..5c1596d7cd92 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -5007,6 +5007,8 @@ intel_pipe_config_compare(const struct
> > intel_crtc_state *current_config, {
> > struct drm_i915_private *dev_priv = to_i915(current_config-
> >uapi.crtc->dev);
> > struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc);
> > + const struct drm_dsc_config *dsc_current_config = ¤t_config-
> >dsc.config;
> > + const struct drm_dsc_config *dsc_pipe_config =
> > +&pipe_config->dsc.config;
> > bool ret = true;
> > bool fixup_inherited = fastset &&
> > current_config->inherited && !pipe_config->inherited; @@ -
> 5202,6
> > +5204,26 @@ intel_pipe_config_compare(const struct intel_crtc_state
> > *current_config, #define PIPE_CONF_QUIRK(quirk) \
> > ((current_config->quirks | pipe_config->quirks) & (quirk))
> >
> > +#define PIPE_DSC_CONF_CHECK_I(name) do { \
> > + if (dsc_current_config->name != dsc_pipe_config->name) { \
> > + pipe_config_mismatch(fastset, crtc, __stringify(name), \
> > + "(expected %i, found %i)", \
> > + dsc_current_config->name, \
> > + dsc_pipe_config->name); \
> > + ret = false; \
> > + } \
> > +} while (0)
> > +
> > +#define PIPE_DSC_CONF_CHECK_BOOL(name) do { \
> > + if (dsc_current_config->name != dsc_pipe_config->name) { \
> > + pipe_config_mismatch(fastset, crtc, __stringify(name), \
> > + "(expected %s, found %s)", \
> > + str_yes_no(dsc_current_config->name), \
> > + str_yes_no(dsc_pipe_config->name)); \
> > + ret = false; \
> > + } \
> > +} while (0)
> > +
> > PIPE_CONF_CHECK_I(hw.enable);
> > PIPE_CONF_CHECK_I(hw.active);
> >
> > @@ -5378,6 +5400,39 @@ intel_pipe_config_compare(const struct
> intel_crtc_state *current_config,
> > PIPE_CONF_CHECK_I(master_transcoder);
> > PIPE_CONF_CHECK_X(bigjoiner_pipes);
> >
> > + PIPE_DSC_CONF_CHECK_BOOL(block_pred_enable);
>
> You should be able to pass the dsc substruct as name, no need to define
> dupe macros for DSC. See e.g. PIPE_CONF_CHECK_I(hw.enable); above in
> the patch context above.
>
Hi
Thanks for the review will get this fixed in the next revision.
> > + PIPE_DSC_CONF_CHECK_BOOL(convert_rgb);
> > + PIPE_DSC_CONF_CHECK_BOOL(simple_422);
> > + PIPE_DSC_CONF_CHECK_BOOL(native_422);
> > + PIPE_DSC_CONF_CHECK_BOOL(native_420);
> > + PIPE_DSC_CONF_CHECK_BOOL(vbr_enable);
> > + PIPE_DSC_CONF_CHECK_I(line_buf_depth);
> > + PIPE_DSC_CONF_CHECK_I(bits_per_component);
> > + PIPE_DSC_CONF_CHECK_I(pic_width);
> > + PIPE_DSC_CONF_CHECK_I(pic_height);
> > + PIPE_DSC_CONF_CHECK_I(slice_width);
> > + PIPE_DSC_CONF_CHECK_I(slice_height);
> > + PIPE_DSC_CONF_CHECK_I(initial_dec_delay);
> > + PIPE_DSC_CONF_CHECK_I(initial_xmit_delay);
> > + PIPE_DSC_CONF_CHECK_I(scale_decrement_interval);
> > + PIPE_DSC_CONF_CHECK_I(scale_increment_interval);
> > + PIPE_DSC_CONF_CHECK_I(initial_scale_value);
> > + PIPE_DSC_CONF_CHECK_I(first_line_bpg_offset);
> > + PIPE_DSC_CONF_CHECK_I(flatness_min_qp);
> > + PIPE_DSC_CONF_CHECK_I(flatness_max_qp);
> > + PIPE_DSC_CONF_CHECK_I(slice_bpg_offset);
> > + PIPE_DSC_CONF_CHECK_I(nfl_bpg_offset);
> > + PIPE_DSC_CONF_CHECK_I(initial_offset);
> > + PIPE_DSC_CONF_CHECK_I(final_offset);
> > + PIPE_DSC_CONF_CHECK_I(rc_model_size);
> > + PIPE_DSC_CONF_CHECK_I(rc_quant_incr_limit0);
> > + PIPE_DSC_CONF_CHECK_I(rc_quant_incr_limit1);
> > + PIPE_DSC_CONF_CHECK_I(slice_chunk_size);
> > + if (DISPLAY_VER(dev_priv) >= 14) {
> > + PIPE_DSC_CONF_CHECK_I(second_line_bpg_offset);
> > + PIPE_DSC_CONF_CHECK_I(nsl_bpg_offset);
> > + }
>
> I'd prefer it if we didn't have version checks here. Just check the values
> anyway, it should be zeros in both hw and sw states for display < 14, and if it's
> not, the state checker caught a bug.
>
Oh got it will remove the version check from here.
Regards,
Suraj Kandpal
>
> > +
> > PIPE_CONF_CHECK_I(dsc.compression_enable);
> > PIPE_CONF_CHECK_I(dsc.dsc_split);
> > PIPE_CONF_CHECK_I(dsc.compressed_bpp);
>
> --
> Jani Nikula, Intel Open Source Graphics Center
