Re: [PATCH] drm/amd/display: Fix sending VSC (+ colorimetry) packets for DP/eDP displays without PSR

[Harry Wentland <harry.wentland@amd.com>]

On 2024-01-01 13:28, Joshua Ashton wrote:
> The check for sending the vsc infopacket to the display was gated behind
> PSR (Panel Self Refresh) being enabled.
> 
> The vsc infopacket also contains the colorimetry (specifically the
> container color gamut) information for the stream on modern DP.
> 
> PSR is typically only supported on mobile phone eDP displays, thus this
> was not getting sent for typical desktop monitors or TV screens.
> 
> This functionality is needed for proper HDR10 functionality on DP as it
> wants BT2020 RGB/YCbCr for the container color space.
> 

So apparently this caused regressions on some panels. I sent a revert
and we'll need to revisit this.

https://gitlab.freedesktop.org/drm/amd/-/issues/3207
https://gitlab.freedesktop.org/drm/amd/-/issues/3151

> Signed-off-by: Joshua Ashton <joshua@froggi.es>
> 
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Xaver Hugl <xaver.hugl@gmail.com>
> Cc: Melissa Wen <mwen@igalia.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c   |  8 +++++---
>  .../amd/display/modules/info_packet/info_packet.c   | 13 ++++++++-----
>  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 2845c884398e..6dff56408bf4 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6233,8 +6233,9 @@ create_stream_for_sink(struct drm_connector *connector,
>  
>  	if (stream->signal == SIGNAL_TYPE_HDMI_TYPE_A)
>  		mod_build_hf_vsif_infopacket(stream, &stream->vsp_infopacket);
> -
> -	if (stream->link->psr_settings.psr_feature_enabled || stream->link->replay_settings.replay_feature_enabled) {
> +	else if (stream->signal == SIGNAL_TYPE_DISPLAY_PORT ||
> +			 stream->signal == SIGNAL_TYPE_DISPLAY_PORT_MST ||
> +			 stream->signal == SIGNAL_TYPE_EDP) {
>  		//
>  		// should decide stream support vsc sdp colorimetry capability
>  		// before building vsc info packet

The use_vsc_sdp_for_colorimetry is being cut off in this patch
since it's not changed. We should add a DPCD revision check to
ensure the panel's revision is >= 1.4. This is what we do on other
OSes and this is likely why we're seeing the regressions in the
freedesktop issues.

> 			if (stream->linkstream->link->dpcd_caps.dprx_feature.bits.VSC_SDP_COLORIMETRY_SUPPORTED)
> 				stream->use_vsc_sdp_for_colorimetry = true;



> @@ -6250,8 +6251,9 @@ create_stream_for_sink(struct drm_connector *connector,
>  		if (stream->out_transfer_func->tf == TRANSFER_FUNCTION_GAMMA22)
>  			tf = TRANSFER_FUNC_GAMMA_22;
>  		mod_build_vsc_infopacket(stream, &stream->vsc_infopacket, stream->output_color_space, tf);
> -		aconnector->psr_skip_count = AMDGPU_DM_PSR_ENTRY_DELAY;
>  
> +		if (stream->link->psr_settings.psr_feature_enabled)
> +			aconnector->psr_skip_count = AMDGPU_DM_PSR_ENTRY_DELAY;
>  	}
>  finish:
>  	dc_sink_release(sink);
> diff --git a/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c b/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c
> index 84f9b412a4f1..738ee763f24a 100644
> --- a/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c
> +++ b/drivers/gpu/drm/amd/display/modules/info_packet/info_packet.c
> @@ -147,12 +147,15 @@ void mod_build_vsc_infopacket(const struct dc_stream_state *stream,
>  	}
>  
>  	/* VSC packet set to 4 for PSR-SU, or 2 for PSR1 */
> -	if (stream->link->psr_settings.psr_version == DC_PSR_VERSION_SU_1)
> -		vsc_packet_revision = vsc_packet_rev4;
> -	else if (stream->link->replay_settings.config.replay_supported)
> +	if (stream->link->psr_settings.psr_feature_enabled) {
> +		if (stream->link->psr_settings.psr_version == DC_PSR_VERSION_SU_1)
> +			vsc_packet_revision = vsc_packet_rev4;
> +		else if (stream->link->psr_settings.psr_version == DC_PSR_VERSION_1)
> +			vsc_packet_revision = vsc_packet_rev2;
> +	}
> +
> +	if (stream->link->replay_settings.config.replay_supported)
>  		vsc_packet_revision = vsc_packet_rev4;
> -	else if (stream->link->psr_settings.psr_version == DC_PSR_VERSION_1)
> -		vsc_packet_revision = vsc_packet_rev2;
>  

I'm curious whether this is really needed? The original code
should already do the same, even without the additional
.psr_feature_enabled check.

I'll send a patch that is intended to fix the colorimetry while
trying to avoid the regressions.

Harry

>  	/* Update to revision 5 for extended colorimetry support */
>  	if (stream->use_vsc_sdp_for_colorimetry)