Re: [PATCH 2/5] drm/msm/dp: Modify prototype of encoder based API

On 25/07/2021 07:24, Bjorn Andersson wrote:

Functions in the DisplayPort code that relates to individual instances (encoders) are passed both the struct msm_dp and the struct drm_encoder. But in a situation where multiple DP instances would exist this means that the caller need to resolve which struct msm_dp relates to the struct drm_encoder at hand.

The information for doing this lookup is available inside the DP driver, so update the API to take the struct msm_drm_private and the struct drm_encoder and have the DP code figure out which struct msm_dp the operation relates to.

Initially I thought to propose moving encoder->dp lookup into dpu code by adding msm_dp_display_get_encoder() function. However as I was writing that, I remembered that at some point I had to refactor my own patchset in the way to get rid of calling msm_FOO_get_encoder().

I'd propose simpler solution. In dpu_encoder_setup() you have the DP index and the encoder. So you can store valid msm_dp pointer in the dpu_encoder_virt and remove all the lookups. Then you can replace all priv->dp with bare dpu_enc->dp accesses. Will this work for you?

Signed-off-by: Bjorn Andersson bjorn.andersson@linaro.org

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +++++---- drivers/gpu/drm/msm/dp/dp_display.c | 38 +++++++++++++++++---- drivers/gpu/drm/msm/msm_drv.h | 31 +++++++++-------- 3 files changed, 56 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 1c04b7cce43e..0d64ef0819af 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1002,8 +1002,8 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,

trace_dpu_enc_mode_set(DRMID(drm_enc));

• if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp)

• msm_dp_display_mode_set(priv->dp, drm_enc, mode, adj_mode);
  

• if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS)

• msm_dp_display_mode_set(priv, drm_enc, mode, adj_mode);
  
  

  list_for_each_entry(conn_iter, connector_list, head) if (conn_iter->encoder == drm_enc)

@@ -1184,9 +1184,8 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)

_dpu_encoder_virt_enable_helper(drm_enc);

• if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {

• ret = msm_dp_display_enable(priv->dp,
  

• 				drm_enc);
  

• if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {

• ret = msm_dp_display_enable(priv, drm_enc);
  

  if (ret) { DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n", ret);

@@ -1226,8 +1225,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) /* wait for idle */ dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);

• if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {

• if (msm_dp_display_pre_disable(priv->dp, drm_enc))
  

• if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {

• if (msm_dp_display_pre_disable(priv, drm_enc))
  DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
  

  }

@@ -1255,8 +1254,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)

DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");

• if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && priv->dp) {

• if (msm_dp_display_disable(priv->dp, drm_enc))
  

• if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS) {

• if (msm_dp_display_disable(priv, drm_enc))
  DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
  

  }

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 8696b36d30e4..59ffd6c8f41f 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1432,12 +1432,25 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, return 0; }

-int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) +static struct msm_dp *msm_dp_from_drm_encoder(struct msm_drm_private *priv,

• 			      struct drm_encoder *encoder)
  

+{

• if (priv->dp && priv->dp->encoder == encoder)

• return priv->dp;
  

• return NULL;

+}

+int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder) { int rc = 0; struct dp_display_private *dp_display;

• struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder); u32 state;

• if (!dp)

• return -EINVAL;
  

• dp_display = container_of(dp, struct dp_display_private, dp_display); if (!dp_display->dp_mode.drm_mode.clock) { DRM_ERROR("invalid params\n");

@@ -1489,9 +1502,13 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) return rc; }

-int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder) +int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder) { struct dp_display_private *dp_display;

• struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);

• if (!dp)

• return 0;
  
  

  dp_display = container_of(dp, struct dp_display_private, dp_display);

@@ -1500,11 +1517,15 @@ int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder) return 0; }

-int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder) +int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder) { int rc = 0; u32 state; struct dp_display_private *dp_display;

• struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);

• if (!dp)

• return 0;
  
  

  dp_display = container_of(dp, struct dp_display_private, dp_display);

@@ -1531,11 +1552,16 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder) return rc; }

-void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,

• 		struct drm_display_mode *mode,
  

• 		struct drm_display_mode *adjusted_mode)
  

+void msm_dp_display_mode_set(struct msm_drm_private *priv,

• 	     struct drm_encoder *encoder,
  

• 	     struct drm_display_mode *mode,
  

• 	     struct drm_display_mode *adjusted_mode)
  

  { struct dp_display_private *dp_display;

• struct msm_dp *dp = msm_dp_from_drm_encoder(priv, encoder);

• if (!dp)

• return;
  
  

  dp_display = container_of(dp, struct dp_display_private, dp_display);

diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 9bfd37855969..e9232032b266 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -388,12 +388,13 @@ int __init msm_dp_register(void); void __exit msm_dp_unregister(void); int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, struct drm_encoder *encoder); -int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder); -int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder); -int msm_dp_display_pre_disable(struct msm_dp *dp, struct drm_encoder *encoder); -void msm_dp_display_mode_set(struct msm_dp *dp, struct drm_encoder *encoder,

• 		struct drm_display_mode *mode,
  

• 		struct drm_display_mode *adjusted_mode);
  

+int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder); +int msm_dp_display_disable(struct msm_drm_private *priv, struct drm_encoder *encoder); +int msm_dp_display_pre_disable(struct msm_drm_private *priv, struct drm_encoder *encoder); +void msm_dp_display_mode_set(struct msm_drm_private *priv,

• 	     struct drm_encoder *encoder,
  

• 	     struct drm_display_mode *mode,
  

• 	     struct drm_display_mode *adjusted_mode);
  

  void msm_dp_irq_postinstall(struct msm_dp *dp_display); void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_display);

@@ -413,25 +414,25 @@ static inline int msm_dp_modeset_init(struct msm_dp *dp_display, { return -EINVAL; } -static inline int msm_dp_display_enable(struct msm_dp *dp, +static inline int msm_dp_display_enable(struct msm_drm_private *priv, struct drm_encoder *encoder) { return -EINVAL; } -static inline int msm_dp_display_disable(struct msm_dp *dp,

• 			struct drm_encoder *encoder)
  

+static inline int msm_dp_display_disable(struct msm_drm_private *priv,

• 			 struct drm_encoder *encoder)
  

  { return -EINVAL; }

-static inline int msm_dp_display_pre_disable(struct msm_dp *dp,

• 			struct drm_encoder *encoder)
  

+static inline int msm_dp_display_pre_disable(struct msm_drm_private *priv,

• 			     struct drm_encoder *encoder)
  

  { return -EINVAL; }

-static inline void msm_dp_display_mode_set(struct msm_dp *dp,

• 		struct drm_encoder *encoder,
  

• 		struct drm_display_mode *mode,
  

• 		struct drm_display_mode *adjusted_mode)
  

+static inline void msm_dp_display_mode_set(struct msm_drm_private *priv,

• 			   struct drm_encoder *encoder,
  

• 			   struct drm_display_mode *mode,
  

• 			   struct drm_display_mode *adjusted_mode)
  

  { }