Hi Mitul,
Thanks for the patch please find my comments below.
> Subject: [PATCH v1] drm/i915/dp: Gate UHBR SST SDP splitting on sink
> capability
>
> SDP splitting for 128b/132b (UHBR) SST audio must only be enabled when the
> sink advertises support for it. Previously sdp_split_enable was set for every
> UHBR SST stream carrying audio, regardless of sink capability.
>
> Signed-off-by: Mitul Golani <[email protected]>
> ---
> .../drm/i915/display/intel_display_types.h | 2 ++
> drivers/gpu/drm/i915/display/intel_dp.c | 22 ++++++++++++++++++-
> 2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index c048da7d6fea..272b14289e38 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1938,6 +1938,8 @@ struct intel_dp {
>
> bool colorimetry_support;
>
> + bool sst_split_sdp_supported;
sst_split _sdp_support should be enough.
> +
> struct {
> enum transcoder transcoder;
> struct mutex lock;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0922d23b284c..77c348509a0e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3359,12 +3359,15 @@ intel_dp_audio_compute_config(struct
> intel_encoder *encoder,
> struct intel_crtc_state *pipe_config,
> struct drm_connector_state *conn_state) {
> + struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +
> pipe_config->has_audio =
> intel_dp_has_audio(encoder, conn_state) &&
> intel_audio_compute_config(encoder, pipe_config, conn_state);
>
> pipe_config->sdp_split_enable = pipe_config->has_audio &&
> - intel_dp_is_uhbr(pipe_config);
> + intel_dp_is_uhbr(pipe_config) &&
> + intel_dp->sst_split_sdp_supported;
Maybe we need to add a Fixes tag with the commit that introduced this.
Also one more question you said SST UHBR needs to be guarded does that mean MST
UHBR does not require any such guard.
> }
>
> void
> @@ -4413,6 +4416,17 @@ static bool intel_dp_get_colorimetry_status(struct
> intel_dp *intel_dp)
> return dprx & DP_VSC_SDP_EXT_FOR_COLORIMETRY_SUPPORTED;
> }
>
> +static bool intel_dp_get_sst_split_sdp_status(struct intel_dp
> +*intel_dp) {
> + u8 dprx = 0;
> +
> + if (drm_dp_dpcd_readb(&intel_dp->aux,
> DP_DPRX_FEATURE_ENUMERATION_LIST,
> + &dprx) != 1)
So from what I can see
intel_dp_get_colorimetry_status also does a DPCD read on the same register.
Which means for the same data we read the DPCD twice; Maybe pull out the
function that reads DP_DPRX_FEATURE_ENUMERATION_LIST and pass just the
Dprx value to both these static helpers.
> + return false;
> + return dprx & DP_SST_SPLIT_SDP_CAP;
> +}
> +
> +
> static int intel_dp_read_dsc_dpcd(struct drm_dp_aux *aux,
> u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
> {
> @@ -4726,6 +4740,9 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp, struct
> intel_connector *connector
> intel_dp->colorimetry_support =
> intel_dp_get_colorimetry_status(intel_dp);
>
> + intel_dp->sst_split_sdp_supported =
> + intel_dp_get_sst_split_sdp_status(intel_dp);
> +
> /*
> * Read the eDP display control registers.
> *
> @@ -4823,6 +4840,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> intel_dp->colorimetry_support =
> intel_dp_get_colorimetry_status(intel_dp);
>
> + intel_dp->sst_split_sdp_supported =
> + intel_dp_get_sst_split_sdp_status(intel_dp);
> +
Ditto
Maybe we can have a helper intel_dp_read_dprx_feature_enum() which gets called
first
static u8 intel_dp_read_dprx_feature_enum(struct intel_dp *intel_dp)
{
u8 dprx = 0;
drm_dp_dpcd_readb(&intel_dp->aux, DP_DPRX_FEATURE_ENUMERATION_LIST, &dprx);
return dprx;
}
Regards,
Suraj Kandpal
> intel_dp_update_sink_caps(intel_dp);
> }
>
> --
> 2.48.1
