Re: [PATCH 3/7] drm/i915/display: Unify VSC SPD preparation

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Dec 14, 2023 at 01:48:34PM +0200, Jouni Högander wrote:
> There is no specific reason to prepare VSC SDP for PSR case somehow
> differently. Unify PSR and non-PSR preparation.
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c  | 43 ++++--------------------
>  drivers/gpu/drm/i915/display/intel_dp.h  |  7 ----
>  drivers/gpu/drm/i915/display/intel_psr.c |  6 ----
>  3 files changed, 6 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 08c48a58aa4d..3550cebb44f2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2616,28 +2616,17 @@ static void intel_dp_compute_vsc_sdp(struct intel_dp *intel_dp,
>  				     struct intel_crtc_state *crtc_state,
>  				     const struct drm_connector_state *conn_state)
>  {
> -	struct drm_dp_vsc_sdp *vsc = &crtc_state->infoframes.vsc;
> +	struct drm_dp_vsc_sdp *vsc;
>  
> -	/* When a crtc state has PSR, VSC SDP will be handled by PSR routine */
> -	if (crtc_state->has_psr)
> +	if ((!intel_dp->colorimetry_support ||
> +	    !intel_dp_needs_vsc_sdp(crtc_state, conn_state)) &&
> +	    !crtc_state->has_psr)
>  		return;
>  
> -	if (!intel_dp->colorimetry_support ||
> -	    !intel_dp_needs_vsc_sdp(crtc_state, conn_state))
> -		return;
> +	vsc = &crtc_state->infoframes.vsc;
>  
>  	crtc_state->infoframes.enable |= intel_hdmi_infoframe_enable(DP_SDP_VSC);
>  	vsc->sdp_type = DP_SDP_VSC;
> -	intel_dp_compute_vsc_colorimetry(crtc_state, conn_state,
> -					 &crtc_state->infoframes.vsc);
> -}
> -
> -void intel_dp_compute_psr_vsc_sdp(struct intel_dp *intel_dp,
> -				  const struct intel_crtc_state *crtc_state,
> -				  const struct drm_connector_state *conn_state,
> -				  struct drm_dp_vsc_sdp *vsc)
> -{
> -	vsc->sdp_type = DP_SDP_VSC;
>  
>  	if (crtc_state->has_psr2) {

sorry for the delay here... yesterday I started wondering if we were
sure if we could reach this point in the unified flow, but after
checking the compute config path and seeing that this is only true
if has_psr is also true, then I'm comfortable with this good unification.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

>  		if (intel_dp->colorimetry_support &&
> @@ -4289,24 +4278,6 @@ static void intel_write_dp_sdp(struct intel_encoder *encoder,
>  	dig_port->write_infoframe(encoder, crtc_state, type, &sdp, len);
>  }
>  
> -void intel_write_dp_vsc_sdp(struct intel_encoder *encoder,
> -			    const struct intel_crtc_state *crtc_state,
> -			    const struct drm_dp_vsc_sdp *vsc)
> -{
> -	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> -	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	struct dp_sdp sdp = {};
> -	ssize_t len;
> -
> -	len = intel_dp_vsc_sdp_pack(vsc, &sdp, sizeof(sdp));
> -
> -	if (drm_WARN_ON(&dev_priv->drm, len < 0))
> -		return;
> -
> -	dig_port->write_infoframe(encoder, crtc_state, DP_SDP_VSC,
> -					&sdp, len);
> -}
> -
>  void intel_dp_set_infoframes(struct intel_encoder *encoder,
>  			     bool enable,
>  			     const struct intel_crtc_state *crtc_state,
> @@ -4333,9 +4304,7 @@ void intel_dp_set_infoframes(struct intel_encoder *encoder,
>  	if (!enable)
>  		return;
>  
> -	/* When PSR is enabled, VSC SDP is handled by PSR routine */
> -	if (!crtc_state->has_psr)
> -		intel_write_dp_sdp(encoder, crtc_state, DP_SDP_VSC);
> +	intel_write_dp_sdp(encoder, crtc_state, DP_SDP_VSC);
>  
>  	intel_write_dp_sdp(encoder, crtc_state, HDMI_PACKET_TYPE_GAMUT_METADATA);
>  }
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index 05db46b111f2..b911706d2e95 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -109,13 +109,6 @@ int intel_dp_max_data_rate(int max_link_rate, int max_lanes);
>  bool intel_dp_can_bigjoiner(struct intel_dp *intel_dp);
>  bool intel_dp_needs_vsc_sdp(const struct intel_crtc_state *crtc_state,
>  			    const struct drm_connector_state *conn_state);
> -void intel_dp_compute_psr_vsc_sdp(struct intel_dp *intel_dp,
> -				  const struct intel_crtc_state *crtc_state,
> -				  const struct drm_connector_state *conn_state,
> -				  struct drm_dp_vsc_sdp *vsc);
> -void intel_write_dp_vsc_sdp(struct intel_encoder *encoder,
> -			    const struct intel_crtc_state *crtc_state,
> -			    const struct drm_dp_vsc_sdp *vsc);
>  void intel_dp_set_infoframes(struct intel_encoder *encoder, bool enable,
>  			     const struct intel_crtc_state *crtc_state,
>  			     const struct drm_connector_state *conn_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index d9fffc802335..494d08817d71 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1374,10 +1374,6 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  		return;
>  
>  	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
> -
> -	crtc_state->infoframes.enable |= intel_hdmi_infoframe_enable(DP_SDP_VSC);
> -	intel_dp_compute_psr_vsc_sdp(intel_dp, crtc_state, conn_state,
> -				     &crtc_state->infoframes.vsc);
>  }
>  
>  void intel_psr_get_config(struct intel_encoder *encoder,
> @@ -1621,7 +1617,6 @@ static void intel_psr_enable_locked(struct intel_dp *intel_dp,
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>  	enum phy phy = intel_port_to_phy(dev_priv, dig_port->base.port);
> -	struct intel_encoder *encoder = &dig_port->base;
>  	u32 val;
>  
>  	drm_WARN_ON(&dev_priv->drm, intel_dp->psr.enabled);
> @@ -1649,7 +1644,6 @@ static void intel_psr_enable_locked(struct intel_dp *intel_dp,
>  		drm_dbg_kms(&dev_priv->drm, "Enabling PSR%s\n",
>  			    intel_dp->psr.psr2_enabled ? "2" : "1");
>  
> -	intel_write_dp_vsc_sdp(encoder, crtc_state, &crtc_state->infoframes.vsc);
>  	intel_snps_phy_update_psr_power_state(dev_priv, phy, true);
>  	intel_psr_enable_sink(intel_dp);
>  	intel_psr_enable_source(intel_dp, crtc_state);
> -- 
> 2.34.1
>