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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7f7177c3-c6e1-81b8-2599-0d24245b416d@amd.com>
Date:   Thu, 5 Jan 2023 15:07:59 -0500
From:   Hamza Mahfooz <hamza.mahfooz@....com>
To:     Harry Wentland <harry.wentland@....com>,
        amd-gfx@...ts.freedesktop.org
Cc:     Leo Li <sunpeng.li@....com>,
        Rodrigo Siqueira <Rodrigo.Siqueira@....com>,
        Alex Deucher <alexander.deucher@....com>,
        Christian König <christian.koenig@....com>,
        "Pan, Xinhui" <Xinhui.Pan@....com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Zhang <dingchen.zhang@....com>,
        Nicholas Kazlauskas <nicholas.kazlauskas@....com>,
        Alex Hung <alex.hung@....com>,
        Po Ting Chen <robin.chen@....com>,
        Shirish S <shirish.s@....com>, Robin Chen <po-tchen@....com>,
        Brian Chang <Brian.Chang@....com>,
        Camille Cho <Camille.Cho@....com>,
        Tom Chung <chiahsuan.chung@....com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] drm/amd/display: fix PSR-SU/DSC interoperability
 support

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

>> +		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_ */
> 

-- 
Hamza