Re: [PATCH v2] drm/amd/display: fix PSR-SU/DSC interoperability support

[Leo Li]

On 1/5/23 15:07, Hamza Mahfooz wrote:
> On 1/5/23 13:29, Harry Wentland wrote:
> > On 1/5/23 12:38, Hamza Mahfooz wrote:
> > > Currently, there are issues with enabling PSR-SU + DSC. This stems from
> > > the fact that DSC imposes a slice height on transmitted video data and
> > > we are not conforming to that slice height in PSR-SU regions. So, pass
> > > slice_height into su_y_granularity to feed the DSC slice height into
> > > PSR-SU code.
> > >
> > > Signed-off-by: Hamza Mahfooz <hamza.mahfooz@xxxxxxx>
> > > ---
> > > v2: move code to modules/power.
> > > ---
> > >   .../drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c |  3 ++
> > >   .../amd/display/modules/power/power_helpers.c | 35 +++++++++++++++++++
> > >   .../amd/display/modules/power/power_helpers.h |  3 ++
> > >   3 files changed, 41 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c 
> > > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c
> > > index 26291db0a3cf..872d06fe1436 100644
> > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c
> > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_psr.c
> > > @@ -122,6 +122,9 @@ bool amdgpu_dm_link_setup_psr(struct 
> > > dc_stream_state *stream)
> > >           psr_config.allow_multi_disp_optimizations =
> > >               (amdgpu_dc_feature_mask & DC_PSR_ALLOW_MULTI_DISP_OPT);
> > > +        if (!psr_su_set_y_granularity(dc, link, stream, &psr_config))
> > > +            return false;
> > > +
> > >           ret = dc_link_setup_psr(link, stream, &psr_config, 
> > > &psr_context);
> > >       }
> > > diff --git 
> > > a/drivers/gpu/drm/amd/display/modules/power/power_helpers.c 
> > > b/drivers/gpu/drm/amd/display/modules/power/power_helpers.c
> > > index 9b5d9b2c9a6a..4d27ad9f7370 100644
> > > --- a/drivers/gpu/drm/amd/display/modules/power/power_helpers.c
> > > +++ b/drivers/gpu/drm/amd/display/modules/power/power_helpers.c
> > > @@ -916,3 +916,38 @@ bool mod_power_only_edp(const struct dc_state 
> > > *context, const struct dc_stream_s
> > >   {
> > >       return context && context->stream_count == 1 && 
> > > dc_is_embedded_signal(stream->signal);
> > >   }
> > > +
> > > +bool psr_su_set_y_granularity(struct dc *dc, struct dc_link *link,
> > > +                  struct dc_stream_state *stream,
> > > +                  struct psr_config *config)
> > > +{
> > > +    uint16_t pic_height;
> > > +    uint8_t slice_height;
> > > +
> > > +    if (!dc->caps.edp_dsc_support ||
> > > +        link->panel_config.dsc.disable_dsc_edp ||
> > > +        
> > > !link->dpcd_caps.dsc_caps.dsc_basic_caps.fields.dsc_support.DSC_SUPPORT ||
> > > +        !stream->timing.dsc_cfg.num_slices_v)
> >
> > I'm not sure this condition is correct. We can have DSC but not eDP DSC
> > support.
>
> AFAIK PSR-SU displays use eDP exclusively, so we shouldn't have to worry 
> about this case.

Right, the dc_link here should only be eDP. I suppose that isn't quite
clear.

Maybe add this as part of the condition?

if (!(link->connector_signal & SIGNAL_TYPE_EDP))
    return true;

Thanks,
Leo

> > > +        return true;
> > > +
> > > +    pic_height = stream->timing.v_addressable +
> > > +        stream->timing.v_border_top + stream->timing.v_border_bottom;
> > > +    slice_height = pic_height / stream->timing.dsc_cfg.num_slices_v;
> > > +
> > > +    if (slice_height) {
> > > +        if (config->su_y_granularity &&
> > > +            (slice_height % config->su_y_granularity)) {
> > > +            WARN(1,
> >
> > We don't use WARN in display/dc or display/modules. DC_LOG_WARNING
> > might be better, or log it in the caller.
> >
> > Harry
> >
> > > +                 "%s: dsc: %d, slice_height: %d, num_slices_v: %d\n",
> > > +                 __func__,
> > > +                 stream->sink->dsc_caps.dsc_dec_caps.is_dsc_supported,
> > > +                 slice_height,
> > > +                 stream->timing.dsc_cfg.num_slices_v);
> > > +            return false;
> > > +        }
> > > +
> > > +        config->su_y_granularity = slice_height;
> > > +    }
> > > +
> > > +    return true;
> > > +}
> > > diff --git 
> > > a/drivers/gpu/drm/amd/display/modules/power/power_helpers.h 
> > > b/drivers/gpu/drm/amd/display/modules/power/power_helpers.h
> > > index 316452e9dbc9..bb16b37b83da 100644
> > > --- a/drivers/gpu/drm/amd/display/modules/power/power_helpers.h
> > > +++ b/drivers/gpu/drm/amd/display/modules/power/power_helpers.h
> > > @@ -59,4 +59,7 @@ void mod_power_calc_psr_configs(struct psr_config 
> > > *psr_config,
> > >           const struct dc_stream_state *stream);
> > >   bool mod_power_only_edp(const struct dc_state *context,
> > >           const struct dc_stream_state *stream);
> > > +bool psr_su_set_y_granularity(struct dc *dc, struct dc_link *link,
> > > +                  struct dc_stream_state *stream,
> > > +                  struct psr_config *config);
> > >   #endif /* MODULES_POWER_POWER_HELPERS_H_ */