[PATCH 0/7] drm/msm/dpu: cleanup dpu encoder code

[PATCH 0/7] drm/msm/dpu: cleanup dpu encoder code

[Dmitry Baryshkov]

This patchset targets DPU encoder code, removing unused artifacts (empty
callbacks, MSM bus client id, etc).

Dmitry Baryshkov (7):
  drm/msm: move struct msm_display_info to dpu driver
  drm/msm/dpu: simplify intf allocation code
  drm/msm/dpu: remove msm_dp cached in dpu_encoder_virt
  drm/msm/dpu: drop bus_scaling_client field
  drm/msm/dpu: encoder: drop unused callbacks
  drm/msm/dpu: switch dpu_encoder to use atomic_mode_set
  drm/msm/dpu: pull connector from dpu_encoder_phys to dpu_encoder_virt

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 118 +++++-------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   |  18 +++
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  18 +--
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  28 +----
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  28 +----
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |   4 +-
 drivers/gpu/drm/msm/msm_drv.h                 |  18 ---
 7 files changed, 63 insertions(+), 169 deletions(-)

-- 
2.34.1

[PATCH 0/7] drm/msm/dpu: cleanup dpu encoder code

[Dmitry Baryshkov]

This patchset targets DPU encoder code, removing unused artifacts (empty
callbacks, MSM bus client id, etc).

Dmitry Baryshkov (7):
  drm/msm: move struct msm_display_info to dpu driver
  drm/msm/dpu: simplify intf allocation code
  drm/msm/dpu: remove msm_dp cached in dpu_encoder_virt
  drm/msm/dpu: drop bus_scaling_client field
  drm/msm/dpu: encoder: drop unused callbacks
  drm/msm/dpu: switch dpu_encoder to use atomic_mode_set
  drm/msm/dpu: pull connector from dpu_encoder_phys to dpu_encoder_virt

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 118 +++++-------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   |  18 +++
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  18 +--
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  |  28 +----
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  28 +----
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |   4 +-
 drivers/gpu/drm/msm/msm_drv.h                 |  18 ---
 7 files changed, 63 insertions(+), 169 deletions(-)

-- 
2.34.1

[PATCH 1/7] drm/msm: move struct msm_display_info to dpu driver

[Dmitry Baryshkov]

The msm_display_info structure is not used by the rest of msm driver, so
move it into the dpu1 (dpu_encoder.h to be precise).

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 18 ++++++++++++++++++
 drivers/gpu/drm/msm/msm_drv.h               | 18 ------------------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index e241914a9677..ebe3944355bb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -34,6 +34,24 @@ struct dpu_encoder_hw_resources {
 void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
 				  struct dpu_encoder_hw_resources *hw_res);
 
+/**
+ * struct msm_display_info - defines display properties
+ * @intf_type:          DRM_MODE_ENCODER_ type
+ * @capabilities:       Bitmask of display flags
+ * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
+ * @h_tile_instance:    Controller instance used per tile. Number of elements is
+ *                      based on num_of_h_tiles
+ * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
+ *				 used instead of panel TE in cmd mode panels
+ */
+struct msm_display_info {
+	int intf_type;
+	uint32_t capabilities;
+	uint32_t num_of_h_tiles;
+	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
+	bool is_te_using_watchdog_timer;
+};
+
 /**
  * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
  * @encoder:	encoder pointer
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index d7574e6bd4e4..16f9e25ee19e 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -109,24 +109,6 @@ struct msm_display_topology {
 	u32 num_dspp;
 };
 
-/**
- * struct msm_display_info - defines display properties
- * @intf_type:          DRM_MODE_ENCODER_ type
- * @capabilities:       Bitmask of display flags
- * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
- * @h_tile_instance:    Controller instance used per tile. Number of elements is
- *                      based on num_of_h_tiles
- * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
- *				 used instead of panel TE in cmd mode panels
- */
-struct msm_display_info {
-	int intf_type;
-	uint32_t capabilities;
-	uint32_t num_of_h_tiles;
-	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
-	bool is_te_using_watchdog_timer;
-};
-
 /* Commit/Event thread specific structure */
 struct msm_drm_thread {
 	struct drm_device *dev;
-- 
2.34.1

[PATCH 1/7] drm/msm: move struct msm_display_info to dpu driver

[Dmitry Baryshkov]

The msm_display_info structure is not used by the rest of msm driver, so
move it into the dpu1 (dpu_encoder.h to be precise).

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 18 ++++++++++++++++++
 drivers/gpu/drm/msm/msm_drv.h               | 18 ------------------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index e241914a9677..ebe3944355bb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -34,6 +34,24 @@ struct dpu_encoder_hw_resources {
 void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
 				  struct dpu_encoder_hw_resources *hw_res);
 
+/**
+ * struct msm_display_info - defines display properties
+ * @intf_type:          DRM_MODE_ENCODER_ type
+ * @capabilities:       Bitmask of display flags
+ * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
+ * @h_tile_instance:    Controller instance used per tile. Number of elements is
+ *                      based on num_of_h_tiles
+ * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
+ *				 used instead of panel TE in cmd mode panels
+ */
+struct msm_display_info {
+	int intf_type;
+	uint32_t capabilities;
+	uint32_t num_of_h_tiles;
+	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
+	bool is_te_using_watchdog_timer;
+};
+
 /**
  * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
  * @encoder:	encoder pointer
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index d7574e6bd4e4..16f9e25ee19e 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -109,24 +109,6 @@ struct msm_display_topology {
 	u32 num_dspp;
 };
 
-/**
- * struct msm_display_info - defines display properties
- * @intf_type:          DRM_MODE_ENCODER_ type
- * @capabilities:       Bitmask of display flags
- * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
- * @h_tile_instance:    Controller instance used per tile. Number of elements is
- *                      based on num_of_h_tiles
- * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
- *				 used instead of panel TE in cmd mode panels
- */
-struct msm_display_info {
-	int intf_type;
-	uint32_t capabilities;
-	uint32_t num_of_h_tiles;
-	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
-	bool is_te_using_watchdog_timer;
-};
-
 /* Commit/Event thread specific structure */
 struct msm_drm_thread {
 	struct drm_device *dev;
-- 
2.34.1

Re: [Freedreno] [PATCH 1/7] drm/msm: move struct msm_display_info to dpu driver

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> The msm_display_info structure is not used by the rest of msm driver, so
> move it into the dpu1 (dpu_encoder.h to be precise).
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Yes, this is true. Its not used by rest of msm driver.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 18 ++++++++++++++++++
>   drivers/gpu/drm/msm/msm_drv.h               | 18 ------------------
>   2 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index e241914a9677..ebe3944355bb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -34,6 +34,24 @@ struct dpu_encoder_hw_resources {
>   void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
>   				  struct dpu_encoder_hw_resources *hw_res);
>   
> +/**
> + * struct msm_display_info - defines display properties
> + * @intf_type:          DRM_MODE_ENCODER_ type
> + * @capabilities:       Bitmask of display flags
> + * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
> + * @h_tile_instance:    Controller instance used per tile. Number of elements is
> + *                      based on num_of_h_tiles
> + * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> + *				 used instead of panel TE in cmd mode panels
> + */
> +struct msm_display_info {
> +	int intf_type;
> +	uint32_t capabilities;
> +	uint32_t num_of_h_tiles;
> +	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> +	bool is_te_using_watchdog_timer;
> +};
> +
>   /**
>    * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
>    * @encoder:	encoder pointer
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index d7574e6bd4e4..16f9e25ee19e 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -109,24 +109,6 @@ struct msm_display_topology {
>   	u32 num_dspp;
>   };
>   
> -/**
> - * struct msm_display_info - defines display properties
> - * @intf_type:          DRM_MODE_ENCODER_ type
> - * @capabilities:       Bitmask of display flags
> - * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
> - * @h_tile_instance:    Controller instance used per tile. Number of elements is
> - *                      based on num_of_h_tiles
> - * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> - *				 used instead of panel TE in cmd mode panels
> - */
> -struct msm_display_info {
> -	int intf_type;
> -	uint32_t capabilities;
> -	uint32_t num_of_h_tiles;
> -	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> -	bool is_te_using_watchdog_timer;
> -};
> -
>   /* Commit/Event thread specific structure */
>   struct msm_drm_thread {
>   	struct drm_device *dev;

Re: [Freedreno] [PATCH 1/7] drm/msm: move struct msm_display_info to dpu driver

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> The msm_display_info structure is not used by the rest of msm driver, so
> move it into the dpu1 (dpu_encoder.h to be precise).
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Yes, this is true. Its not used by rest of msm driver.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 18 ++++++++++++++++++
>   drivers/gpu/drm/msm/msm_drv.h               | 18 ------------------
>   2 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index e241914a9677..ebe3944355bb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -34,6 +34,24 @@ struct dpu_encoder_hw_resources {
>   void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
>   				  struct dpu_encoder_hw_resources *hw_res);
>   
> +/**
> + * struct msm_display_info - defines display properties
> + * @intf_type:          DRM_MODE_ENCODER_ type
> + * @capabilities:       Bitmask of display flags
> + * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
> + * @h_tile_instance:    Controller instance used per tile. Number of elements is
> + *                      based on num_of_h_tiles
> + * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> + *				 used instead of panel TE in cmd mode panels
> + */
> +struct msm_display_info {
> +	int intf_type;
> +	uint32_t capabilities;
> +	uint32_t num_of_h_tiles;
> +	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> +	bool is_te_using_watchdog_timer;
> +};
> +
>   /**
>    * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
>    * @encoder:	encoder pointer
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index d7574e6bd4e4..16f9e25ee19e 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -109,24 +109,6 @@ struct msm_display_topology {
>   	u32 num_dspp;
>   };
>   
> -/**
> - * struct msm_display_info - defines display properties
> - * @intf_type:          DRM_MODE_ENCODER_ type
> - * @capabilities:       Bitmask of display flags
> - * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
> - * @h_tile_instance:    Controller instance used per tile. Number of elements is
> - *                      based on num_of_h_tiles
> - * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
> - *				 used instead of panel TE in cmd mode panels
> - */
> -struct msm_display_info {
> -	int intf_type;
> -	uint32_t capabilities;
> -	uint32_t num_of_h_tiles;
> -	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> -	bool is_te_using_watchdog_timer;
> -};
> -
>   /* Commit/Event thread specific structure */
>   struct msm_drm_thread {
>   	struct drm_device *dev;

[PATCH 2/7] drm/msm/dpu: simplify intf allocation code

[Dmitry Baryshkov]

Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to guess,
which intf type we mean, pass INTF_DSI/INTF_DP directly.

While we are at it, fix the DP audio enablement code which was comparing
intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
DRM_MODE_CONNECTOR_DisplayPort (= 10).
Which would never succeed.

Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port on MSM")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 +++++++--------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  4 +--
 3 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1e648db439f9..e8fc029ad607 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
 	hw_mdptop = phys_enc->hw_mdptop;
 	disp_info = &dpu_enc->disp_info;
 
-	if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
+	if (disp_info->intf_type != INTF_DSI)
 		return;
 
 	/**
@@ -555,7 +555,7 @@ static struct msm_display_topology dpu_encoder_get_topology(
 	else
 		topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
 
-	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
+	if (dpu_enc->disp_info.intf_type == INTF_DSI) {
 		if (dpu_kms->catalog->dspp &&
 			(dpu_kms->catalog->dspp_count >= topology.num_lm))
 			topology.num_dspp = topology.num_lm;
@@ -1099,7 +1099,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
 	}
 
 
-	if (dpu_enc->disp_info.intf_type == DRM_MODE_CONNECTOR_DisplayPort &&
+	if (dpu_enc->disp_info.intf_type == INTF_DP &&
 		dpu_enc->cur_master->hw_mdptop &&
 		dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select)
 		dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
@@ -1107,7 +1107,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
 
 	_dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
 
-	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
+	if (dpu_enc->disp_info.intf_type == INTF_DSI &&
 			!WARN_ON(dpu_enc->num_phys_encs == 0)) {
 		unsigned bpc = dpu_enc->phys_encs[0]->connector->display_info.bpc;
 		for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
@@ -1981,7 +1981,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 {
 	int ret = 0;
 	int i = 0;
-	enum dpu_intf_type intf_type = INTF_NONE;
 	struct dpu_enc_phys_init_params phys_params;
 
 	if (!dpu_enc) {
@@ -1997,15 +1996,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 	phys_params.parent_ops = &dpu_encoder_parent_ops;
 	phys_params.enc_spinlock = &dpu_enc->enc_spinlock;
 
-	switch (disp_info->intf_type) {
-	case DRM_MODE_ENCODER_DSI:
-		intf_type = INTF_DSI;
-		break;
-	case DRM_MODE_ENCODER_TMDS:
-		intf_type = INTF_DP;
-		break;
-	}
-
 	WARN_ON(disp_info->num_of_h_tiles < 1);
 
 	DPU_DEBUG("dsi_info->num_of_h_tiles %d\n", disp_info->num_of_h_tiles);
@@ -2037,11 +2027,11 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 				i, controller_id, phys_params.split_role);
 
 		phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
-													intf_type,
-													controller_id);
+				disp_info->intf_type,
+				controller_id);
 		if (phys_params.intf_idx == INTF_MAX) {
 			DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id %d\n",
-						  intf_type, controller_id);
+						  disp_info->intf_type, controller_id);
 			ret = -EINVAL;
 		}
 
@@ -2124,11 +2114,11 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
 	timer_setup(&dpu_enc->frame_done_timer,
 			dpu_encoder_frame_done_timeout, 0);
 
-	if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
+	if (disp_info->intf_type == INTF_DSI)
 		timer_setup(&dpu_enc->vsync_event_timer,
 				dpu_encoder_vsync_event_handler,
 				0);
-	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
+	else if (disp_info->intf_type == INTF_DP || disp_info->intf_type == INTF_EDP)
 		dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
 
 	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index ebe3944355bb..3891bcbbe5a4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -36,7 +36,7 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
 
 /**
  * struct msm_display_info - defines display properties
- * @intf_type:          DRM_MODE_ENCODER_ type
+ * @intf_type:          INTF_ type
  * @capabilities:       Bitmask of display flags
  * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
  * @h_tile_instance:    Controller instance used per tile. Number of elements is
@@ -45,7 +45,7 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
  *				 used instead of panel TE in cmd mode panels
  */
 struct msm_display_info {
-	int intf_type;
+	enum dpu_intf_type intf_type;
 	uint32_t capabilities;
 	uint32_t num_of_h_tiles;
 	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 47fe11a84a77..f4028be9e2e2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -564,7 +564,7 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 		priv->encoders[priv->num_encoders++] = encoder;
 
 		memset(&info, 0, sizeof(info));
-		info.intf_type = encoder->encoder_type;
+		info.intf_type = INTF_DSI;
 
 		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
 		if (rc) {
@@ -630,7 +630,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 		info.num_of_h_tiles = 1;
 		info.h_tile_instance[0] = i;
 		info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
-		info.intf_type = encoder->encoder_type;
+		info.intf_type = INTF_DP; /* FIXME: support eDP too */
 		rc = dpu_encoder_setup(dev, encoder, &info);
 		if (rc) {
 			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
-- 
2.34.1

[PATCH 2/7] drm/msm/dpu: simplify intf allocation code

[Dmitry Baryshkov]

Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to guess,
which intf type we mean, pass INTF_DSI/INTF_DP directly.

While we are at it, fix the DP audio enablement code which was comparing
intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
DRM_MODE_CONNECTOR_DisplayPort (= 10).
Which would never succeed.

Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port on MSM")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 +++++++--------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  4 +--
 3 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1e648db439f9..e8fc029ad607 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
 	hw_mdptop = phys_enc->hw_mdptop;
 	disp_info = &dpu_enc->disp_info;
 
-	if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
+	if (disp_info->intf_type != INTF_DSI)
 		return;
 
 	/**
@@ -555,7 +555,7 @@ static struct msm_display_topology dpu_encoder_get_topology(
 	else
 		topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
 
-	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
+	if (dpu_enc->disp_info.intf_type == INTF_DSI) {
 		if (dpu_kms->catalog->dspp &&
 			(dpu_kms->catalog->dspp_count >= topology.num_lm))
 			topology.num_dspp = topology.num_lm;
@@ -1099,7 +1099,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
 	}
 
 
-	if (dpu_enc->disp_info.intf_type == DRM_MODE_CONNECTOR_DisplayPort &&
+	if (dpu_enc->disp_info.intf_type == INTF_DP &&
 		dpu_enc->cur_master->hw_mdptop &&
 		dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select)
 		dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
@@ -1107,7 +1107,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
 
 	_dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
 
-	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
+	if (dpu_enc->disp_info.intf_type == INTF_DSI &&
 			!WARN_ON(dpu_enc->num_phys_encs == 0)) {
 		unsigned bpc = dpu_enc->phys_encs[0]->connector->display_info.bpc;
 		for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
@@ -1981,7 +1981,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 {
 	int ret = 0;
 	int i = 0;
-	enum dpu_intf_type intf_type = INTF_NONE;
 	struct dpu_enc_phys_init_params phys_params;
 
 	if (!dpu_enc) {
@@ -1997,15 +1996,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 	phys_params.parent_ops = &dpu_encoder_parent_ops;
 	phys_params.enc_spinlock = &dpu_enc->enc_spinlock;
 
-	switch (disp_info->intf_type) {
-	case DRM_MODE_ENCODER_DSI:
-		intf_type = INTF_DSI;
-		break;
-	case DRM_MODE_ENCODER_TMDS:
-		intf_type = INTF_DP;
-		break;
-	}
-
 	WARN_ON(disp_info->num_of_h_tiles < 1);
 
 	DPU_DEBUG("dsi_info->num_of_h_tiles %d\n", disp_info->num_of_h_tiles);
@@ -2037,11 +2027,11 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 				i, controller_id, phys_params.split_role);
 
 		phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
-													intf_type,
-													controller_id);
+				disp_info->intf_type,
+				controller_id);
 		if (phys_params.intf_idx == INTF_MAX) {
 			DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id %d\n",
-						  intf_type, controller_id);
+						  disp_info->intf_type, controller_id);
 			ret = -EINVAL;
 		}
 
@@ -2124,11 +2114,11 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
 	timer_setup(&dpu_enc->frame_done_timer,
 			dpu_encoder_frame_done_timeout, 0);
 
-	if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
+	if (disp_info->intf_type == INTF_DSI)
 		timer_setup(&dpu_enc->vsync_event_timer,
 				dpu_encoder_vsync_event_handler,
 				0);
-	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
+	else if (disp_info->intf_type == INTF_DP || disp_info->intf_type == INTF_EDP)
 		dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
 
 	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index ebe3944355bb..3891bcbbe5a4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -36,7 +36,7 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
 
 /**
  * struct msm_display_info - defines display properties
- * @intf_type:          DRM_MODE_ENCODER_ type
+ * @intf_type:          INTF_ type
  * @capabilities:       Bitmask of display flags
  * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
  * @h_tile_instance:    Controller instance used per tile. Number of elements is
@@ -45,7 +45,7 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
  *				 used instead of panel TE in cmd mode panels
  */
 struct msm_display_info {
-	int intf_type;
+	enum dpu_intf_type intf_type;
 	uint32_t capabilities;
 	uint32_t num_of_h_tiles;
 	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 47fe11a84a77..f4028be9e2e2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -564,7 +564,7 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 		priv->encoders[priv->num_encoders++] = encoder;
 
 		memset(&info, 0, sizeof(info));
-		info.intf_type = encoder->encoder_type;
+		info.intf_type = INTF_DSI;
 
 		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
 		if (rc) {
@@ -630,7 +630,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
 		info.num_of_h_tiles = 1;
 		info.h_tile_instance[0] = i;
 		info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
-		info.intf_type = encoder->encoder_type;
+		info.intf_type = INTF_DP; /* FIXME: support eDP too */
 		rc = dpu_encoder_setup(dev, encoder, &info);
 		if (rc) {
 			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
-- 
2.34.1

Re: [PATCH 2/7] drm/msm/dpu: simplify intf allocation code

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to guess,
> which intf type we mean, pass INTF_DSI/INTF_DP directly.
>

Typically, I am only seeing a 1:1 mapping like

DRM_MODE_ENCODER_DSI means DSI
DRM_MODE_ENCODER_VIRTUAL means WB

So I am not seeing any guessing for the encoder.

The only conflict I am seeing is between DP and EDP as both use
DRM_MODE_ENCODER_TMDS and hence this approach will be useful there.

But that has been marked as a "FIXME" below.

I am suggesting an approach to handle that as well below.
Let me know if you agree with that.


> While we are at it, fix the DP audio enablement code which was comparing
> intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
> DRM_MODE_CONNECTOR_DisplayPort (= 10).
> Which would never succeed.

This is a surprising catch for me and left me thinking for a while about 
how DP audio is working with this bug because that piece of code was 
done to program a register which is needed for DP audio.

This bug happened due to difference in the meaning of intf_type between
upstream and downstream.

After checking more, we found that the register in question has been 
deprecated on newer chipsets so I have asked Kuogee to selectively 
program it. Here is the change for that:

https://patchwork.freedesktop.org/patch/473869/

> 

> Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port on MSM")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 +++++++--------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 +--
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  4 +--
>   3 files changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1e648db439f9..e8fc029ad607 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
>   	hw_mdptop = phys_enc->hw_mdptop;
>   	disp_info = &dpu_enc->disp_info;
>   
> -	if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
> +	if (disp_info->intf_type != INTF_DSI)
>   		return;
>   
>   	/**
> @@ -555,7 +555,7 @@ static struct msm_display_topology dpu_encoder_get_topology(
>   	else
>   		topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
>   
> -	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
> +	if (dpu_enc->disp_info.intf_type == INTF_DSI) {
>   		if (dpu_kms->catalog->dspp &&
>   			(dpu_kms->catalog->dspp_count >= topology.num_lm))
>   			topology.num_dspp = topology.num_lm;
> @@ -1099,7 +1099,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>   	}
>   
>   
> -	if (dpu_enc->disp_info.intf_type == DRM_MODE_CONNECTOR_DisplayPort &&
> +	if (dpu_enc->disp_info.intf_type == INTF_DP &&
>   		dpu_enc->cur_master->hw_mdptop &&
>   		dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select)
>   		dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
> @@ -1107,7 +1107,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>   
>   	_dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
>   
> -	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
> +	if (dpu_enc->disp_info.intf_type == INTF_DSI &&
>   			!WARN_ON(dpu_enc->num_phys_encs == 0)) {
>   		unsigned bpc = dpu_enc->phys_encs[0]->connector->display_info.bpc;
>   		for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
> @@ -1981,7 +1981,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   {
>   	int ret = 0;
>   	int i = 0;
> -	enum dpu_intf_type intf_type = INTF_NONE;
>   	struct dpu_enc_phys_init_params phys_params;
>   
>   	if (!dpu_enc) {
> @@ -1997,15 +1996,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   	phys_params.parent_ops = &dpu_encoder_parent_ops;
>   	phys_params.enc_spinlock = &dpu_enc->enc_spinlock;
>   
> -	switch (disp_info->intf_type) {
> -	case DRM_MODE_ENCODER_DSI:
> -		intf_type = INTF_DSI;
> -		break;
> -	case DRM_MODE_ENCODER_TMDS:
> -		intf_type = INTF_DP;
> -		break;
> -	}
> -
>   	WARN_ON(disp_info->num_of_h_tiles < 1);
>   
>   	DPU_DEBUG("dsi_info->num_of_h_tiles %d\n", disp_info->num_of_h_tiles);
> @@ -2037,11 +2027,11 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   				i, controller_id, phys_params.split_role);
>   
>   		phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
> -													intf_type,
> -													controller_id);
> +				disp_info->intf_type,
> +				controller_id);
>   		if (phys_params.intf_idx == INTF_MAX) {
>   			DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id %d\n",
> -						  intf_type, controller_id);
> +						  disp_info->intf_type, controller_id);
>   			ret = -EINVAL;
>   		}
>   
> @@ -2124,11 +2114,11 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   	timer_setup(&dpu_enc->frame_done_timer,
>   			dpu_encoder_frame_done_timeout, 0);
>   
> -	if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
> +	if (disp_info->intf_type == INTF_DSI)
>   		timer_setup(&dpu_enc->vsync_event_timer,
>   				dpu_encoder_vsync_event_handler,
>   				0);
> -	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
> +	else if (disp_info->intf_type == INTF_DP || disp_info->intf_type == INTF_EDP)
>   		dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>   
>   	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index ebe3944355bb..3891bcbbe5a4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -36,7 +36,7 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
>   
>   /**
>    * struct msm_display_info - defines display properties
> - * @intf_type:          DRM_MODE_ENCODER_ type
> + * @intf_type:          INTF_ type
>    * @capabilities:       Bitmask of display flags
>    * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
>    * @h_tile_instance:    Controller instance used per tile. Number of elements is
> @@ -45,7 +45,7 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
>    *				 used instead of panel TE in cmd mode panels
>    */
>   struct msm_display_info {
> -	int intf_type;
> +	enum dpu_intf_type intf_type;
>   	uint32_t capabilities;
>   	uint32_t num_of_h_tiles;
>   	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 47fe11a84a77..f4028be9e2e2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -564,7 +564,7 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>   		priv->encoders[priv->num_encoders++] = encoder;
>   
>   		memset(&info, 0, sizeof(info));
> -		info.intf_type = encoder->encoder_type;
> +		info.intf_type = INTF_DSI;
>   
>   		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>   		if (rc) {
> @@ -630,7 +630,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>   		info.num_of_h_tiles = 1;
>   		info.h_tile_instance[0] = i;
>   		info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
> -		info.intf_type = encoder->encoder_type;

You can query the connector type from the DP driver like this:

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 7cc4d21..0fae0fc 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1457,7 +1457,7 @@ void msm_dp_debugfs_init(struct msm_dp 
*dp_display, struct drm_minor *minor)
  }

  int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
-                       struct drm_encoder *encoder)
+                       struct drm_encoder *encoder, int **connector_type)
  {
         struct msm_drm_private *priv;
         int ret;
@@ -1498,6 +1498,8 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, 
struct drm_device *dev,

         priv->bridges[priv->num_bridges++] = dp_display->bridge;

+       *connector_type = dp_display->connector_type;
+
         return 0;
  }

Then you can do something like:

if (connector_type == eDP)
     	info.intf_type = INTF_eDP;
else if (connector_type == DP)
         info.intf_type = INTF_DP;
> +		info.intf_type = INTF_DP; /* FIXME: support eDP too */
>   		rc = dpu_encoder_setup(dev, encoder, &info);
>   		if (rc) {
>   			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",

Re: [PATCH 2/7] drm/msm/dpu: simplify intf allocation code

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to guess,
> which intf type we mean, pass INTF_DSI/INTF_DP directly.
>

Typically, I am only seeing a 1:1 mapping like

DRM_MODE_ENCODER_DSI means DSI
DRM_MODE_ENCODER_VIRTUAL means WB

So I am not seeing any guessing for the encoder.

The only conflict I am seeing is between DP and EDP as both use
DRM_MODE_ENCODER_TMDS and hence this approach will be useful there.

But that has been marked as a "FIXME" below.

I am suggesting an approach to handle that as well below.
Let me know if you agree with that.


> While we are at it, fix the DP audio enablement code which was comparing
> intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
> DRM_MODE_CONNECTOR_DisplayPort (= 10).
> Which would never succeed.

This is a surprising catch for me and left me thinking for a while about 
how DP audio is working with this bug because that piece of code was 
done to program a register which is needed for DP audio.

This bug happened due to difference in the meaning of intf_type between
upstream and downstream.

After checking more, we found that the register in question has been 
deprecated on newer chipsets so I have asked Kuogee to selectively 
program it. Here is the change for that:

https://patchwork.freedesktop.org/patch/473869/

> 

> Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port on MSM")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 +++++++--------------
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 +--
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  4 +--
>   3 files changed, 13 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1e648db439f9..e8fc029ad607 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
>   	hw_mdptop = phys_enc->hw_mdptop;
>   	disp_info = &dpu_enc->disp_info;
>   
> -	if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
> +	if (disp_info->intf_type != INTF_DSI)
>   		return;
>   
>   	/**
> @@ -555,7 +555,7 @@ static struct msm_display_topology dpu_encoder_get_topology(
>   	else
>   		topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
>   
> -	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
> +	if (dpu_enc->disp_info.intf_type == INTF_DSI) {
>   		if (dpu_kms->catalog->dspp &&
>   			(dpu_kms->catalog->dspp_count >= topology.num_lm))
>   			topology.num_dspp = topology.num_lm;
> @@ -1099,7 +1099,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>   	}
>   
>   
> -	if (dpu_enc->disp_info.intf_type == DRM_MODE_CONNECTOR_DisplayPort &&
> +	if (dpu_enc->disp_info.intf_type == INTF_DP &&
>   		dpu_enc->cur_master->hw_mdptop &&
>   		dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select)
>   		dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
> @@ -1107,7 +1107,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>   
>   	_dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
>   
> -	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
> +	if (dpu_enc->disp_info.intf_type == INTF_DSI &&
>   			!WARN_ON(dpu_enc->num_phys_encs == 0)) {
>   		unsigned bpc = dpu_enc->phys_encs[0]->connector->display_info.bpc;
>   		for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
> @@ -1981,7 +1981,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   {
>   	int ret = 0;
>   	int i = 0;
> -	enum dpu_intf_type intf_type = INTF_NONE;
>   	struct dpu_enc_phys_init_params phys_params;
>   
>   	if (!dpu_enc) {
> @@ -1997,15 +1996,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   	phys_params.parent_ops = &dpu_encoder_parent_ops;
>   	phys_params.enc_spinlock = &dpu_enc->enc_spinlock;
>   
> -	switch (disp_info->intf_type) {
> -	case DRM_MODE_ENCODER_DSI:
> -		intf_type = INTF_DSI;
> -		break;
> -	case DRM_MODE_ENCODER_TMDS:
> -		intf_type = INTF_DP;
> -		break;
> -	}
> -
>   	WARN_ON(disp_info->num_of_h_tiles < 1);
>   
>   	DPU_DEBUG("dsi_info->num_of_h_tiles %d\n", disp_info->num_of_h_tiles);
> @@ -2037,11 +2027,11 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
>   				i, controller_id, phys_params.split_role);
>   
>   		phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
> -													intf_type,
> -													controller_id);
> +				disp_info->intf_type,
> +				controller_id);
>   		if (phys_params.intf_idx == INTF_MAX) {
>   			DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id %d\n",
> -						  intf_type, controller_id);
> +						  disp_info->intf_type, controller_id);
>   			ret = -EINVAL;
>   		}
>   
> @@ -2124,11 +2114,11 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   	timer_setup(&dpu_enc->frame_done_timer,
>   			dpu_encoder_frame_done_timeout, 0);
>   
> -	if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
> +	if (disp_info->intf_type == INTF_DSI)
>   		timer_setup(&dpu_enc->vsync_event_timer,
>   				dpu_encoder_vsync_event_handler,
>   				0);
> -	else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
> +	else if (disp_info->intf_type == INTF_DP || disp_info->intf_type == INTF_EDP)
>   		dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>   
>   	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index ebe3944355bb..3891bcbbe5a4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -36,7 +36,7 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
>   
>   /**
>    * struct msm_display_info - defines display properties
> - * @intf_type:          DRM_MODE_ENCODER_ type
> + * @intf_type:          INTF_ type
>    * @capabilities:       Bitmask of display flags
>    * @num_of_h_tiles:     Number of horizontal tiles in case of split interface
>    * @h_tile_instance:    Controller instance used per tile. Number of elements is
> @@ -45,7 +45,7 @@ void dpu_encoder_get_hw_resources(struct drm_encoder *encoder,
>    *				 used instead of panel TE in cmd mode panels
>    */
>   struct msm_display_info {
> -	int intf_type;
> +	enum dpu_intf_type intf_type;
>   	uint32_t capabilities;
>   	uint32_t num_of_h_tiles;
>   	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 47fe11a84a77..f4028be9e2e2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -564,7 +564,7 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>   		priv->encoders[priv->num_encoders++] = encoder;
>   
>   		memset(&info, 0, sizeof(info));
> -		info.intf_type = encoder->encoder_type;
> +		info.intf_type = INTF_DSI;
>   
>   		rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>   		if (rc) {
> @@ -630,7 +630,7 @@ static int _dpu_kms_initialize_displayport(struct drm_device *dev,
>   		info.num_of_h_tiles = 1;
>   		info.h_tile_instance[0] = i;
>   		info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
> -		info.intf_type = encoder->encoder_type;

You can query the connector type from the DP driver like this:

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
b/drivers/gpu/drm/msm/dp/dp_display.c
index 7cc4d21..0fae0fc 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1457,7 +1457,7 @@ void msm_dp_debugfs_init(struct msm_dp 
*dp_display, struct drm_minor *minor)
  }

  int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
-                       struct drm_encoder *encoder)
+                       struct drm_encoder *encoder, int **connector_type)
  {
         struct msm_drm_private *priv;
         int ret;
@@ -1498,6 +1498,8 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, 
struct drm_device *dev,

         priv->bridges[priv->num_bridges++] = dp_display->bridge;

+       *connector_type = dp_display->connector_type;
+
         return 0;
  }

Then you can do something like:

if (connector_type == eDP)
     	info.intf_type = INTF_eDP;
else if (connector_type == DP)
         info.intf_type = INTF_DP;
> +		info.intf_type = INTF_DP; /* FIXME: support eDP too */
>   		rc = dpu_encoder_setup(dev, encoder, &info);
>   		if (rc) {
>   			DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",

Re: [PATCH 2/7] drm/msm/dpu: simplify intf allocation code

[Dmitry Baryshkov]

On 12/02/2022 02:38, Abhinav Kumar wrote:
> 
> 
> On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
>> Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to guess,
>> which intf type we mean, pass INTF_DSI/INTF_DP directly.
>>
> 
> Typically, I am only seeing a 1:1 mapping like
> 
> DRM_MODE_ENCODER_DSI means DSI
> DRM_MODE_ENCODER_VIRTUAL means WB
> 
> So I am not seeing any guessing for the encoder.

s/guessing/deriving/

Initially I spotted the DRM_MODE_CONNECTOR_DisplayPort comparison, then 
I noticed that there is a misnaming, we were talking about the intf_type 
(which clearly belongs to INTF_foo namespace), while passing 
DRM_ENCODER_bar instead. Thus comes the proposal to make intf_type 
actually contain INTF_type and let DRM_ENCODER live in encoder's code.


> 
> The only conflict I am seeing is between DP and EDP as both use
> DRM_MODE_ENCODER_TMDS and hence this approach will be useful there.
> 
> But that has been marked as a "FIXME" below.
> 
> I am suggesting an approach to handle that as well below.
> Let me know if you agree with that.

Actually this brings a question to me. Do we need to distinguish between 
INTF_DP and INTF_EDP from the DPU driver point of view? Are there any 
differences? Or we'd better always use INTF_DP here and make INTF_EDP a 
memorial of old eDP IP found in 8x74/8x84?

So far I see that sc7280 uses INTF_5 for DRM_MODE_CONNECTOR_eDP, but 
declares it as INTF_DP. Most probably we should stick to this idea and 
drop INTF_EDP from dpu1?

> 
> 
>> While we are at it, fix the DP audio enablement code which was comparing
>> intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
>> DRM_MODE_CONNECTOR_DisplayPort (= 10).
>> Which would never succeed.
> 
> This is a surprising catch for me and left me thinking for a while about 
> how DP audio is working with this bug because that piece of code was 
> done to program a register which is needed for DP audio.

So did I!

> 
> This bug happened due to difference in the meaning of intf_type between
> upstream and downstream.
> 
> After checking more, we found that the register in question has been 
> deprecated on newer chipsets so I have asked Kuogee to selectively 
> program it. Here is the change for that:
> 
> https://patchwork.freedesktop.org/patch/473869/

I'll further comment on it in the respective thread.

> 
>>
> 
>> Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port 
>> on MSM")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 +++++++--------------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 +--
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  4 +--
>>   3 files changed, 13 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 1e648db439f9..e8fc029ad607 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
>>       hw_mdptop = phys_enc->hw_mdptop;
>>       disp_info = &dpu_enc->disp_info;
>> -    if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
>> +    if (disp_info->intf_type != INTF_DSI)
>>           return;
>>       /**
>> @@ -555,7 +555,7 @@ static struct msm_display_topology 
>> dpu_encoder_get_topology(
>>       else
>>           topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 
>> : 1;
>> -    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
>> +    if (dpu_enc->disp_info.intf_type == INTF_DSI) {
>>           if (dpu_kms->catalog->dspp &&
>>               (dpu_kms->catalog->dspp_count >= topology.num_lm))
>>               topology.num_dspp = topology.num_lm;
>> @@ -1099,7 +1099,7 @@ static void 
>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>       }
>> -    if (dpu_enc->disp_info.intf_type == 
>> DRM_MODE_CONNECTOR_DisplayPort &&
>> +    if (dpu_enc->disp_info.intf_type == INTF_DP &&
>>           dpu_enc->cur_master->hw_mdptop &&
>>           dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select)
>>           dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
>> @@ -1107,7 +1107,7 @@ static void 
>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>       _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
>> -    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
>> +    if (dpu_enc->disp_info.intf_type == INTF_DSI &&
>>               !WARN_ON(dpu_enc->num_phys_encs == 0)) {
>>           unsigned bpc = 
>> dpu_enc->phys_encs[0]->connector->display_info.bpc;
>>           for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>> @@ -1981,7 +1981,6 @@ static int dpu_encoder_setup_display(struct 
>> dpu_encoder_virt *dpu_enc,
>>   {
>>       int ret = 0;
>>       int i = 0;
>> -    enum dpu_intf_type intf_type = INTF_NONE;
>>       struct dpu_enc_phys_init_params phys_params;
>>       if (!dpu_enc) {
>> @@ -1997,15 +1996,6 @@ static int dpu_encoder_setup_display(struct 
>> dpu_encoder_virt *dpu_enc,
>>       phys_params.parent_ops = &dpu_encoder_parent_ops;
>>       phys_params.enc_spinlock = &dpu_enc->enc_spinlock;
>> -    switch (disp_info->intf_type) {
>> -    case DRM_MODE_ENCODER_DSI:
>> -        intf_type = INTF_DSI;
>> -        break;
>> -    case DRM_MODE_ENCODER_TMDS:
>> -        intf_type = INTF_DP;
>> -        break;
>> -    }
>> -
>>       WARN_ON(disp_info->num_of_h_tiles < 1);
>>       DPU_DEBUG("dsi_info->num_of_h_tiles %d\n", 
>> disp_info->num_of_h_tiles);
>> @@ -2037,11 +2027,11 @@ static int dpu_encoder_setup_display(struct 
>> dpu_encoder_virt *dpu_enc,
>>                   i, controller_id, phys_params.split_role);
>>           phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>> -                                                    intf_type,
>> -                                                    controller_id);
>> +                disp_info->intf_type,
>> +                controller_id);
>>           if (phys_params.intf_idx == INTF_MAX) {
>>               DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id 
>> %d\n",
>> -                          intf_type, controller_id);
>> +                          disp_info->intf_type, controller_id);
>>               ret = -EINVAL;
>>           }
>> @@ -2124,11 +2114,11 @@ int dpu_encoder_setup(struct drm_device *dev, 
>> struct drm_encoder *enc,
>>       timer_setup(&dpu_enc->frame_done_timer,
>>               dpu_encoder_frame_done_timeout, 0);
>> -    if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
>> +    if (disp_info->intf_type == INTF_DSI)
>>           timer_setup(&dpu_enc->vsync_event_timer,
>>                   dpu_encoder_vsync_event_handler,
>>                   0);
>> -    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
>> +    else if (disp_info->intf_type == INTF_DP || disp_info->intf_type 
>> == INTF_EDP)
>>           dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>>       INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index ebe3944355bb..3891bcbbe5a4 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -36,7 +36,7 @@ void dpu_encoder_get_hw_resources(struct drm_encoder 
>> *encoder,
>>   /**
>>    * struct msm_display_info - defines display properties
>> - * @intf_type:          DRM_MODE_ENCODER_ type
>> + * @intf_type:          INTF_ type
>>    * @capabilities:       Bitmask of display flags
>>    * @num_of_h_tiles:     Number of horizontal tiles in case of split 
>> interface
>>    * @h_tile_instance:    Controller instance used per tile. Number of 
>> elements is
>> @@ -45,7 +45,7 @@ void dpu_encoder_get_hw_resources(struct drm_encoder 
>> *encoder,
>>    *                 used instead of panel TE in cmd mode panels
>>    */
>>   struct msm_display_info {
>> -    int intf_type;
>> +    enum dpu_intf_type intf_type;
>>       uint32_t capabilities;
>>       uint32_t num_of_h_tiles;
>>       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 47fe11a84a77..f4028be9e2e2 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -564,7 +564,7 @@ static int _dpu_kms_initialize_dsi(struct 
>> drm_device *dev,
>>           priv->encoders[priv->num_encoders++] = encoder;
>>           memset(&info, 0, sizeof(info));
>> -        info.intf_type = encoder->encoder_type;
>> +        info.intf_type = INTF_DSI;
>>           rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>           if (rc) {
>> @@ -630,7 +630,7 @@ static int _dpu_kms_initialize_displayport(struct 
>> drm_device *dev,
>>           info.num_of_h_tiles = 1;
>>           info.h_tile_instance[0] = i;
>>           info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
>> -        info.intf_type = encoder->encoder_type;
> 
> You can query the connector type from the DP driver like this:
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 7cc4d21..0fae0fc 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1457,7 +1457,7 @@ void msm_dp_debugfs_init(struct msm_dp 
> *dp_display, struct drm_minor *minor)
>   }
> 
>   int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device 
> *dev,
> -                       struct drm_encoder *encoder)
> +                       struct drm_encoder *encoder, int **connector_type)
>   {
>          struct msm_drm_private *priv;
>          int ret;
> @@ -1498,6 +1498,8 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, 
> struct drm_device *dev,
> 
>          priv->bridges[priv->num_bridges++] = dp_display->bridge;
> 
> +       *connector_type = dp_display->connector_type;
> +
>          return 0;
>   }
> 
> Then you can do something like:
> 
> if (connector_type == eDP)
>          info.intf_type = INTF_eDP;
> else if (connector_type == DP)
>          info.intf_type = INTF_DP;
>> +        info.intf_type = INTF_DP; /* FIXME: support eDP too */
>>           rc = dpu_encoder_setup(dev, encoder, &info);
>>           if (rc) {
>>               DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",


-- 
With best wishes
Dmitry

Re: [PATCH 2/7] drm/msm/dpu: simplify intf allocation code

[Dmitry Baryshkov]

On 12/02/2022 02:38, Abhinav Kumar wrote:
> 
> 
> On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
>> Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to guess,
>> which intf type we mean, pass INTF_DSI/INTF_DP directly.
>>
> 
> Typically, I am only seeing a 1:1 mapping like
> 
> DRM_MODE_ENCODER_DSI means DSI
> DRM_MODE_ENCODER_VIRTUAL means WB
> 
> So I am not seeing any guessing for the encoder.

s/guessing/deriving/

Initially I spotted the DRM_MODE_CONNECTOR_DisplayPort comparison, then 
I noticed that there is a misnaming, we were talking about the intf_type 
(which clearly belongs to INTF_foo namespace), while passing 
DRM_ENCODER_bar instead. Thus comes the proposal to make intf_type 
actually contain INTF_type and let DRM_ENCODER live in encoder's code.


> 
> The only conflict I am seeing is between DP and EDP as both use
> DRM_MODE_ENCODER_TMDS and hence this approach will be useful there.
> 
> But that has been marked as a "FIXME" below.
> 
> I am suggesting an approach to handle that as well below.
> Let me know if you agree with that.

Actually this brings a question to me. Do we need to distinguish between 
INTF_DP and INTF_EDP from the DPU driver point of view? Are there any 
differences? Or we'd better always use INTF_DP here and make INTF_EDP a 
memorial of old eDP IP found in 8x74/8x84?

So far I see that sc7280 uses INTF_5 for DRM_MODE_CONNECTOR_eDP, but 
declares it as INTF_DP. Most probably we should stick to this idea and 
drop INTF_EDP from dpu1?

> 
> 
>> While we are at it, fix the DP audio enablement code which was comparing
>> intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
>> DRM_MODE_CONNECTOR_DisplayPort (= 10).
>> Which would never succeed.
> 
> This is a surprising catch for me and left me thinking for a while about 
> how DP audio is working with this bug because that piece of code was 
> done to program a register which is needed for DP audio.

So did I!

> 
> This bug happened due to difference in the meaning of intf_type between
> upstream and downstream.
> 
> After checking more, we found that the register in question has been 
> deprecated on newer chipsets so I have asked Kuogee to selectively 
> program it. Here is the change for that:
> 
> https://patchwork.freedesktop.org/patch/473869/

I'll further comment on it in the respective thread.

> 
>>
> 
>> Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port 
>> on MSM")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 +++++++--------------
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 +--
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  4 +--
>>   3 files changed, 13 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 1e648db439f9..e8fc029ad607 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
>>       hw_mdptop = phys_enc->hw_mdptop;
>>       disp_info = &dpu_enc->disp_info;
>> -    if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
>> +    if (disp_info->intf_type != INTF_DSI)
>>           return;
>>       /**
>> @@ -555,7 +555,7 @@ static struct msm_display_topology 
>> dpu_encoder_get_topology(
>>       else
>>           topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 
>> : 1;
>> -    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
>> +    if (dpu_enc->disp_info.intf_type == INTF_DSI) {
>>           if (dpu_kms->catalog->dspp &&
>>               (dpu_kms->catalog->dspp_count >= topology.num_lm))
>>               topology.num_dspp = topology.num_lm;
>> @@ -1099,7 +1099,7 @@ static void 
>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>       }
>> -    if (dpu_enc->disp_info.intf_type == 
>> DRM_MODE_CONNECTOR_DisplayPort &&
>> +    if (dpu_enc->disp_info.intf_type == INTF_DP &&
>>           dpu_enc->cur_master->hw_mdptop &&
>>           dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select)
>>           dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
>> @@ -1107,7 +1107,7 @@ static void 
>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>       _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
>> -    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
>> +    if (dpu_enc->disp_info.intf_type == INTF_DSI &&
>>               !WARN_ON(dpu_enc->num_phys_encs == 0)) {
>>           unsigned bpc = 
>> dpu_enc->phys_encs[0]->connector->display_info.bpc;
>>           for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>> @@ -1981,7 +1981,6 @@ static int dpu_encoder_setup_display(struct 
>> dpu_encoder_virt *dpu_enc,
>>   {
>>       int ret = 0;
>>       int i = 0;
>> -    enum dpu_intf_type intf_type = INTF_NONE;
>>       struct dpu_enc_phys_init_params phys_params;
>>       if (!dpu_enc) {
>> @@ -1997,15 +1996,6 @@ static int dpu_encoder_setup_display(struct 
>> dpu_encoder_virt *dpu_enc,
>>       phys_params.parent_ops = &dpu_encoder_parent_ops;
>>       phys_params.enc_spinlock = &dpu_enc->enc_spinlock;
>> -    switch (disp_info->intf_type) {
>> -    case DRM_MODE_ENCODER_DSI:
>> -        intf_type = INTF_DSI;
>> -        break;
>> -    case DRM_MODE_ENCODER_TMDS:
>> -        intf_type = INTF_DP;
>> -        break;
>> -    }
>> -
>>       WARN_ON(disp_info->num_of_h_tiles < 1);
>>       DPU_DEBUG("dsi_info->num_of_h_tiles %d\n", 
>> disp_info->num_of_h_tiles);
>> @@ -2037,11 +2027,11 @@ static int dpu_encoder_setup_display(struct 
>> dpu_encoder_virt *dpu_enc,
>>                   i, controller_id, phys_params.split_role);
>>           phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>> -                                                    intf_type,
>> -                                                    controller_id);
>> +                disp_info->intf_type,
>> +                controller_id);
>>           if (phys_params.intf_idx == INTF_MAX) {
>>               DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id 
>> %d\n",
>> -                          intf_type, controller_id);
>> +                          disp_info->intf_type, controller_id);
>>               ret = -EINVAL;
>>           }
>> @@ -2124,11 +2114,11 @@ int dpu_encoder_setup(struct drm_device *dev, 
>> struct drm_encoder *enc,
>>       timer_setup(&dpu_enc->frame_done_timer,
>>               dpu_encoder_frame_done_timeout, 0);
>> -    if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
>> +    if (disp_info->intf_type == INTF_DSI)
>>           timer_setup(&dpu_enc->vsync_event_timer,
>>                   dpu_encoder_vsync_event_handler,
>>                   0);
>> -    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
>> +    else if (disp_info->intf_type == INTF_DP || disp_info->intf_type 
>> == INTF_EDP)
>>           dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>>       INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index ebe3944355bb..3891bcbbe5a4 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -36,7 +36,7 @@ void dpu_encoder_get_hw_resources(struct drm_encoder 
>> *encoder,
>>   /**
>>    * struct msm_display_info - defines display properties
>> - * @intf_type:          DRM_MODE_ENCODER_ type
>> + * @intf_type:          INTF_ type
>>    * @capabilities:       Bitmask of display flags
>>    * @num_of_h_tiles:     Number of horizontal tiles in case of split 
>> interface
>>    * @h_tile_instance:    Controller instance used per tile. Number of 
>> elements is
>> @@ -45,7 +45,7 @@ void dpu_encoder_get_hw_resources(struct drm_encoder 
>> *encoder,
>>    *                 used instead of panel TE in cmd mode panels
>>    */
>>   struct msm_display_info {
>> -    int intf_type;
>> +    enum dpu_intf_type intf_type;
>>       uint32_t capabilities;
>>       uint32_t num_of_h_tiles;
>>       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 47fe11a84a77..f4028be9e2e2 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -564,7 +564,7 @@ static int _dpu_kms_initialize_dsi(struct 
>> drm_device *dev,
>>           priv->encoders[priv->num_encoders++] = encoder;
>>           memset(&info, 0, sizeof(info));
>> -        info.intf_type = encoder->encoder_type;
>> +        info.intf_type = INTF_DSI;
>>           rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>           if (rc) {
>> @@ -630,7 +630,7 @@ static int _dpu_kms_initialize_displayport(struct 
>> drm_device *dev,
>>           info.num_of_h_tiles = 1;
>>           info.h_tile_instance[0] = i;
>>           info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
>> -        info.intf_type = encoder->encoder_type;
> 
> You can query the connector type from the DP driver like this:
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 7cc4d21..0fae0fc 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1457,7 +1457,7 @@ void msm_dp_debugfs_init(struct msm_dp 
> *dp_display, struct drm_minor *minor)
>   }
> 
>   int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device 
> *dev,
> -                       struct drm_encoder *encoder)
> +                       struct drm_encoder *encoder, int **connector_type)
>   {
>          struct msm_drm_private *priv;
>          int ret;
> @@ -1498,6 +1498,8 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, 
> struct drm_device *dev,
> 
>          priv->bridges[priv->num_bridges++] = dp_display->bridge;
> 
> +       *connector_type = dp_display->connector_type;
> +
>          return 0;
>   }
> 
> Then you can do something like:
> 
> if (connector_type == eDP)
>          info.intf_type = INTF_eDP;
> else if (connector_type == DP)
>          info.intf_type = INTF_DP;
>> +        info.intf_type = INTF_DP; /* FIXME: support eDP too */
>>           rc = dpu_encoder_setup(dev, encoder, &info);
>>           if (rc) {
>>               DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",


-- 
With best wishes
Dmitry

Re: [PATCH 2/7] drm/msm/dpu: simplify intf allocation code

[Abhinav Kumar]

On 2/11/2022 4:05 PM, Dmitry Baryshkov wrote:
> On 12/02/2022 02:38, Abhinav Kumar wrote:
>>
>>
>> On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
>>> Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to guess,
>>> which intf type we mean, pass INTF_DSI/INTF_DP directly.
>>>
>>
>> Typically, I am only seeing a 1:1 mapping like
>>
>> DRM_MODE_ENCODER_DSI means DSI
>> DRM_MODE_ENCODER_VIRTUAL means WB
>>
>> So I am not seeing any guessing for the encoder.
> 
> s/guessing/deriving/
> 
> Initially I spotted the DRM_MODE_CONNECTOR_DisplayPort comparison, then 
> I noticed that there is a misnaming, we were talking about the intf_type 
> (which clearly belongs to INTF_foo namespace), while passing 
> DRM_ENCODER_bar instead. Thus comes the proposal to make intf_type 
> actually contain INTF_type and let DRM_ENCODER live in encoder's code.
> 
> 
>>
>> The only conflict I am seeing is between DP and EDP as both use
>> DRM_MODE_ENCODER_TMDS and hence this approach will be useful there.
>>
>> But that has been marked as a "FIXME" below.
>>
>> I am suggesting an approach to handle that as well below.
>> Let me know if you agree with that.
> 
> Actually this brings a question to me. Do we need to distinguish between 
> INTF_DP and INTF_EDP from the DPU driver point of view? Are there any 
> differences? Or we'd better always use INTF_DP here and make INTF_EDP a 
> memorial of old eDP IP found in 8x74/8x84?
> 
> So far I see that sc7280 uses INTF_5 for DRM_MODE_CONNECTOR_eDP, but 
> declares it as INTF_DP. Most probably we should stick to this idea and 
> drop INTF_EDP from dpu1?

I believe you are referring to this part:

static const struct dpu_intf_cfg sc7280_intf[] = {
     INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, MSM_DP_CONTROLLER_0, 
24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
     INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24, 
INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
     INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, MSM_DP_CONTROLLER_1, 
24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
};

This is interesting ... this should have been marked as INTF_eDP unless 
I am missing some reason why.

My take on this is that, since are re-using DP driver now for DP and 
eDP, less confusion the better in terms of naming of DP/eDP.

I think we should fix that in the catalog to INTF_eDP. And in case some 
place is missed out due to this change fix that too.

Meanwhile I can check with sankeerth if there was any specific reason 
for not using INTF_eDP in the original commit:

commit ef7837ff091c8805cfa18d2ad06c2e5f4d820a7e
Author: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
Date:   Tue Nov 2 13:18:42 2021 +0530

     drm/msm/dp: Add DP controllers for sc7280

     The eDP controller on SC7280 is similar to the eDP/DP controllers
     supported by the current driver implementation.

     SC7280 supports one EDP and one DP controller which can operate
     concurrently.

     This change adds the support for eDP and DP controller on sc7280.

     Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>

> 
>>
>>
>>> While we are at it, fix the DP audio enablement code which was comparing
>>> intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
>>> DRM_MODE_CONNECTOR_DisplayPort (= 10).
>>> Which would never succeed.
>>
>> This is a surprising catch for me and left me thinking for a while 
>> about how DP audio is working with this bug because that piece of code 
>> was done to program a register which is needed for DP audio.
> 
> So did I!
> 
>>
>> This bug happened due to difference in the meaning of intf_type between
>> upstream and downstream.
>>
>> After checking more, we found that the register in question has been 
>> deprecated on newer chipsets so I have asked Kuogee to selectively 
>> program it. Here is the change for that:
>>
>> https://patchwork.freedesktop.org/patch/473869/
> 
> I'll further comment on it in the respective thread.
> 
>>
>>>
>>
>>> Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port 
>>> on MSM")
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 +++++++--------------
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 +--
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  4 +--
>>>   3 files changed, 13 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 1e648db439f9..e8fc029ad607 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
>>>       hw_mdptop = phys_enc->hw_mdptop;
>>>       disp_info = &dpu_enc->disp_info;
>>> -    if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
>>> +    if (disp_info->intf_type != INTF_DSI)
>>>           return;
>>>       /**
>>> @@ -555,7 +555,7 @@ static struct msm_display_topology 
>>> dpu_encoder_get_topology(
>>>       else
>>>           topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 
>>> : 1;
>>> -    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
>>> +    if (dpu_enc->disp_info.intf_type == INTF_DSI) {
>>>           if (dpu_kms->catalog->dspp &&
>>>               (dpu_kms->catalog->dspp_count >= topology.num_lm))
>>>               topology.num_dspp = topology.num_lm;
>>> @@ -1099,7 +1099,7 @@ static void 
>>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>>       }
>>> -    if (dpu_enc->disp_info.intf_type == 
>>> DRM_MODE_CONNECTOR_DisplayPort &&
>>> +    if (dpu_enc->disp_info.intf_type == INTF_DP &&
>>>           dpu_enc->cur_master->hw_mdptop &&
>>>           dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select)
>>>           dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
>>> @@ -1107,7 +1107,7 @@ static void 
>>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>>       _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
>>> -    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
>>> +    if (dpu_enc->disp_info.intf_type == INTF_DSI &&
>>>               !WARN_ON(dpu_enc->num_phys_encs == 0)) {
>>>           unsigned bpc = 
>>> dpu_enc->phys_encs[0]->connector->display_info.bpc;
>>>           for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>>> @@ -1981,7 +1981,6 @@ static int dpu_encoder_setup_display(struct 
>>> dpu_encoder_virt *dpu_enc,
>>>   {
>>>       int ret = 0;
>>>       int i = 0;
>>> -    enum dpu_intf_type intf_type = INTF_NONE;
>>>       struct dpu_enc_phys_init_params phys_params;
>>>       if (!dpu_enc) {
>>> @@ -1997,15 +1996,6 @@ static int dpu_encoder_setup_display(struct 
>>> dpu_encoder_virt *dpu_enc,
>>>       phys_params.parent_ops = &dpu_encoder_parent_ops;
>>>       phys_params.enc_spinlock = &dpu_enc->enc_spinlock;
>>> -    switch (disp_info->intf_type) {
>>> -    case DRM_MODE_ENCODER_DSI:
>>> -        intf_type = INTF_DSI;
>>> -        break;
>>> -    case DRM_MODE_ENCODER_TMDS:
>>> -        intf_type = INTF_DP;
>>> -        break;
>>> -    }
>>> -
>>>       WARN_ON(disp_info->num_of_h_tiles < 1);
>>>       DPU_DEBUG("dsi_info->num_of_h_tiles %d\n", 
>>> disp_info->num_of_h_tiles);
>>> @@ -2037,11 +2027,11 @@ static int dpu_encoder_setup_display(struct 
>>> dpu_encoder_virt *dpu_enc,
>>>                   i, controller_id, phys_params.split_role);
>>>           phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>>> -                                                    intf_type,
>>> -                                                    controller_id);
>>> +                disp_info->intf_type,
>>> +                controller_id);
>>>           if (phys_params.intf_idx == INTF_MAX) {
>>>               DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id 
>>> %d\n",
>>> -                          intf_type, controller_id);
>>> +                          disp_info->intf_type, controller_id);
>>>               ret = -EINVAL;
>>>           }
>>> @@ -2124,11 +2114,11 @@ int dpu_encoder_setup(struct drm_device *dev, 
>>> struct drm_encoder *enc,
>>>       timer_setup(&dpu_enc->frame_done_timer,
>>>               dpu_encoder_frame_done_timeout, 0);
>>> -    if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
>>> +    if (disp_info->intf_type == INTF_DSI)
>>>           timer_setup(&dpu_enc->vsync_event_timer,
>>>                   dpu_encoder_vsync_event_handler,
>>>                   0);
>>> -    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
>>> +    else if (disp_info->intf_type == INTF_DP || disp_info->intf_type 
>>> == INTF_EDP)
>>>           dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>>>       INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> index ebe3944355bb..3891bcbbe5a4 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> @@ -36,7 +36,7 @@ void dpu_encoder_get_hw_resources(struct 
>>> drm_encoder *encoder,
>>>   /**
>>>    * struct msm_display_info - defines display properties
>>> - * @intf_type:          DRM_MODE_ENCODER_ type
>>> + * @intf_type:          INTF_ type
>>>    * @capabilities:       Bitmask of display flags
>>>    * @num_of_h_tiles:     Number of horizontal tiles in case of split 
>>> interface
>>>    * @h_tile_instance:    Controller instance used per tile. Number 
>>> of elements is
>>> @@ -45,7 +45,7 @@ void dpu_encoder_get_hw_resources(struct 
>>> drm_encoder *encoder,
>>>    *                 used instead of panel TE in cmd mode panels
>>>    */
>>>   struct msm_display_info {
>>> -    int intf_type;
>>> +    enum dpu_intf_type intf_type;
>>>       uint32_t capabilities;
>>>       uint32_t num_of_h_tiles;
>>>       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 47fe11a84a77..f4028be9e2e2 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -564,7 +564,7 @@ static int _dpu_kms_initialize_dsi(struct 
>>> drm_device *dev,
>>>           priv->encoders[priv->num_encoders++] = encoder;
>>>           memset(&info, 0, sizeof(info));
>>> -        info.intf_type = encoder->encoder_type;
>>> +        info.intf_type = INTF_DSI;
>>>           rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>>           if (rc) {
>>> @@ -630,7 +630,7 @@ static int _dpu_kms_initialize_displayport(struct 
>>> drm_device *dev,
>>>           info.num_of_h_tiles = 1;
>>>           info.h_tile_instance[0] = i;
>>>           info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
>>> -        info.intf_type = encoder->encoder_type;
>>
>> You can query the connector type from the DP driver like this:
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 7cc4d21..0fae0fc 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1457,7 +1457,7 @@ void msm_dp_debugfs_init(struct msm_dp 
>> *dp_display, struct drm_minor *minor)
>>   }
>>
>>   int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device 
>> *dev,
>> -                       struct drm_encoder *encoder)
>> +                       struct drm_encoder *encoder, int 
>> **connector_type)
>>   {
>>          struct msm_drm_private *priv;
>>          int ret;
>> @@ -1498,6 +1498,8 @@ int msm_dp_modeset_init(struct msm_dp 
>> *dp_display, struct drm_device *dev,
>>
>>          priv->bridges[priv->num_bridges++] = dp_display->bridge;
>>
>> +       *connector_type = dp_display->connector_type;
>> +
>>          return 0;
>>   }
>>
>> Then you can do something like:
>>
>> if (connector_type == eDP)
>>          info.intf_type = INTF_eDP;
>> else if (connector_type == DP)
>>          info.intf_type = INTF_DP;
>>> +        info.intf_type = INTF_DP; /* FIXME: support eDP too */
>>>           rc = dpu_encoder_setup(dev, encoder, &info);
>>>           if (rc) {
>>>               DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> 
> 

Re: [PATCH 2/7] drm/msm/dpu: simplify intf allocation code

[Abhinav Kumar]

On 2/11/2022 4:05 PM, Dmitry Baryshkov wrote:
> On 12/02/2022 02:38, Abhinav Kumar wrote:
>>
>>
>> On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
>>> Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to guess,
>>> which intf type we mean, pass INTF_DSI/INTF_DP directly.
>>>
>>
>> Typically, I am only seeing a 1:1 mapping like
>>
>> DRM_MODE_ENCODER_DSI means DSI
>> DRM_MODE_ENCODER_VIRTUAL means WB
>>
>> So I am not seeing any guessing for the encoder.
> 
> s/guessing/deriving/
> 
> Initially I spotted the DRM_MODE_CONNECTOR_DisplayPort comparison, then 
> I noticed that there is a misnaming, we were talking about the intf_type 
> (which clearly belongs to INTF_foo namespace), while passing 
> DRM_ENCODER_bar instead. Thus comes the proposal to make intf_type 
> actually contain INTF_type and let DRM_ENCODER live in encoder's code.
> 
> 
>>
>> The only conflict I am seeing is between DP and EDP as both use
>> DRM_MODE_ENCODER_TMDS and hence this approach will be useful there.
>>
>> But that has been marked as a "FIXME" below.
>>
>> I am suggesting an approach to handle that as well below.
>> Let me know if you agree with that.
> 
> Actually this brings a question to me. Do we need to distinguish between 
> INTF_DP and INTF_EDP from the DPU driver point of view? Are there any 
> differences? Or we'd better always use INTF_DP here and make INTF_EDP a 
> memorial of old eDP IP found in 8x74/8x84?
> 
> So far I see that sc7280 uses INTF_5 for DRM_MODE_CONNECTOR_eDP, but 
> declares it as INTF_DP. Most probably we should stick to this idea and 
> drop INTF_EDP from dpu1?

I believe you are referring to this part:

static const struct dpu_intf_cfg sc7280_intf[] = {
     INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, MSM_DP_CONTROLLER_0, 
24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
     INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24, 
INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
     INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, MSM_DP_CONTROLLER_1, 
24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
};

This is interesting ... this should have been marked as INTF_eDP unless 
I am missing some reason why.

My take on this is that, since are re-using DP driver now for DP and 
eDP, less confusion the better in terms of naming of DP/eDP.

I think we should fix that in the catalog to INTF_eDP. And in case some 
place is missed out due to this change fix that too.

Meanwhile I can check with sankeerth if there was any specific reason 
for not using INTF_eDP in the original commit:

commit ef7837ff091c8805cfa18d2ad06c2e5f4d820a7e
Author: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
Date:   Tue Nov 2 13:18:42 2021 +0530

     drm/msm/dp: Add DP controllers for sc7280

     The eDP controller on SC7280 is similar to the eDP/DP controllers
     supported by the current driver implementation.

     SC7280 supports one EDP and one DP controller which can operate
     concurrently.

     This change adds the support for eDP and DP controller on sc7280.

     Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>

> 
>>
>>
>>> While we are at it, fix the DP audio enablement code which was comparing
>>> intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
>>> DRM_MODE_CONNECTOR_DisplayPort (= 10).
>>> Which would never succeed.
>>
>> This is a surprising catch for me and left me thinking for a while 
>> about how DP audio is working with this bug because that piece of code 
>> was done to program a register which is needed for DP audio.
> 
> So did I!
> 
>>
>> This bug happened due to difference in the meaning of intf_type between
>> upstream and downstream.
>>
>> After checking more, we found that the register in question has been 
>> deprecated on newer chipsets so I have asked Kuogee to selectively 
>> program it. Here is the change for that:
>>
>> https://patchwork.freedesktop.org/patch/473869/
> 
> I'll further comment on it in the respective thread.
> 
>>
>>>
>>
>>> Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port 
>>> on MSM")
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 +++++++--------------
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 +--
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  4 +--
>>>   3 files changed, 13 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 1e648db439f9..e8fc029ad607 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
>>>       hw_mdptop = phys_enc->hw_mdptop;
>>>       disp_info = &dpu_enc->disp_info;
>>> -    if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
>>> +    if (disp_info->intf_type != INTF_DSI)
>>>           return;
>>>       /**
>>> @@ -555,7 +555,7 @@ static struct msm_display_topology 
>>> dpu_encoder_get_topology(
>>>       else
>>>           topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 
>>> : 1;
>>> -    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
>>> +    if (dpu_enc->disp_info.intf_type == INTF_DSI) {
>>>           if (dpu_kms->catalog->dspp &&
>>>               (dpu_kms->catalog->dspp_count >= topology.num_lm))
>>>               topology.num_dspp = topology.num_lm;
>>> @@ -1099,7 +1099,7 @@ static void 
>>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>>       }
>>> -    if (dpu_enc->disp_info.intf_type == 
>>> DRM_MODE_CONNECTOR_DisplayPort &&
>>> +    if (dpu_enc->disp_info.intf_type == INTF_DP &&
>>>           dpu_enc->cur_master->hw_mdptop &&
>>>           dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select)
>>>           dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
>>> @@ -1107,7 +1107,7 @@ static void 
>>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>>       _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
>>> -    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
>>> +    if (dpu_enc->disp_info.intf_type == INTF_DSI &&
>>>               !WARN_ON(dpu_enc->num_phys_encs == 0)) {
>>>           unsigned bpc = 
>>> dpu_enc->phys_encs[0]->connector->display_info.bpc;
>>>           for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>>> @@ -1981,7 +1981,6 @@ static int dpu_encoder_setup_display(struct 
>>> dpu_encoder_virt *dpu_enc,
>>>   {
>>>       int ret = 0;
>>>       int i = 0;
>>> -    enum dpu_intf_type intf_type = INTF_NONE;
>>>       struct dpu_enc_phys_init_params phys_params;
>>>       if (!dpu_enc) {
>>> @@ -1997,15 +1996,6 @@ static int dpu_encoder_setup_display(struct 
>>> dpu_encoder_virt *dpu_enc,
>>>       phys_params.parent_ops = &dpu_encoder_parent_ops;
>>>       phys_params.enc_spinlock = &dpu_enc->enc_spinlock;
>>> -    switch (disp_info->intf_type) {
>>> -    case DRM_MODE_ENCODER_DSI:
>>> -        intf_type = INTF_DSI;
>>> -        break;
>>> -    case DRM_MODE_ENCODER_TMDS:
>>> -        intf_type = INTF_DP;
>>> -        break;
>>> -    }
>>> -
>>>       WARN_ON(disp_info->num_of_h_tiles < 1);
>>>       DPU_DEBUG("dsi_info->num_of_h_tiles %d\n", 
>>> disp_info->num_of_h_tiles);
>>> @@ -2037,11 +2027,11 @@ static int dpu_encoder_setup_display(struct 
>>> dpu_encoder_virt *dpu_enc,
>>>                   i, controller_id, phys_params.split_role);
>>>           phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>>> -                                                    intf_type,
>>> -                                                    controller_id);
>>> +                disp_info->intf_type,
>>> +                controller_id);
>>>           if (phys_params.intf_idx == INTF_MAX) {
>>>               DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, id 
>>> %d\n",
>>> -                          intf_type, controller_id);
>>> +                          disp_info->intf_type, controller_id);
>>>               ret = -EINVAL;
>>>           }
>>> @@ -2124,11 +2114,11 @@ int dpu_encoder_setup(struct drm_device *dev, 
>>> struct drm_encoder *enc,
>>>       timer_setup(&dpu_enc->frame_done_timer,
>>>               dpu_encoder_frame_done_timeout, 0);
>>> -    if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
>>> +    if (disp_info->intf_type == INTF_DSI)
>>>           timer_setup(&dpu_enc->vsync_event_timer,
>>>                   dpu_encoder_vsync_event_handler,
>>>                   0);
>>> -    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
>>> +    else if (disp_info->intf_type == INTF_DP || disp_info->intf_type 
>>> == INTF_EDP)
>>>           dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>>>       INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> index ebe3944355bb..3891bcbbe5a4 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>> @@ -36,7 +36,7 @@ void dpu_encoder_get_hw_resources(struct 
>>> drm_encoder *encoder,
>>>   /**
>>>    * struct msm_display_info - defines display properties
>>> - * @intf_type:          DRM_MODE_ENCODER_ type
>>> + * @intf_type:          INTF_ type
>>>    * @capabilities:       Bitmask of display flags
>>>    * @num_of_h_tiles:     Number of horizontal tiles in case of split 
>>> interface
>>>    * @h_tile_instance:    Controller instance used per tile. Number 
>>> of elements is
>>> @@ -45,7 +45,7 @@ void dpu_encoder_get_hw_resources(struct 
>>> drm_encoder *encoder,
>>>    *                 used instead of panel TE in cmd mode panels
>>>    */
>>>   struct msm_display_info {
>>> -    int intf_type;
>>> +    enum dpu_intf_type intf_type;
>>>       uint32_t capabilities;
>>>       uint32_t num_of_h_tiles;
>>>       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index 47fe11a84a77..f4028be9e2e2 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -564,7 +564,7 @@ static int _dpu_kms_initialize_dsi(struct 
>>> drm_device *dev,
>>>           priv->encoders[priv->num_encoders++] = encoder;
>>>           memset(&info, 0, sizeof(info));
>>> -        info.intf_type = encoder->encoder_type;
>>> +        info.intf_type = INTF_DSI;
>>>           rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>>           if (rc) {
>>> @@ -630,7 +630,7 @@ static int _dpu_kms_initialize_displayport(struct 
>>> drm_device *dev,
>>>           info.num_of_h_tiles = 1;
>>>           info.h_tile_instance[0] = i;
>>>           info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
>>> -        info.intf_type = encoder->encoder_type;
>>
>> You can query the connector type from the DP driver like this:
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 7cc4d21..0fae0fc 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1457,7 +1457,7 @@ void msm_dp_debugfs_init(struct msm_dp 
>> *dp_display, struct drm_minor *minor)
>>   }
>>
>>   int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device 
>> *dev,
>> -                       struct drm_encoder *encoder)
>> +                       struct drm_encoder *encoder, int 
>> **connector_type)
>>   {
>>          struct msm_drm_private *priv;
>>          int ret;
>> @@ -1498,6 +1498,8 @@ int msm_dp_modeset_init(struct msm_dp 
>> *dp_display, struct drm_device *dev,
>>
>>          priv->bridges[priv->num_bridges++] = dp_display->bridge;
>>
>> +       *connector_type = dp_display->connector_type;
>> +
>>          return 0;
>>   }
>>
>> Then you can do something like:
>>
>> if (connector_type == eDP)
>>          info.intf_type = INTF_eDP;
>> else if (connector_type == DP)
>>          info.intf_type = INTF_DP;
>>> +        info.intf_type = INTF_DP; /* FIXME: support eDP too */
>>>           rc = dpu_encoder_setup(dev, encoder, &info);
>>>           if (rc) {
>>>               DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
> 
> 

Re: [PATCH 2/7] drm/msm/dpu: simplify intf allocation code

[Dmitry Baryshkov]

On 12/02/2022 03:17, Abhinav Kumar wrote:
> 
> 
> On 2/11/2022 4:05 PM, Dmitry Baryshkov wrote:
>> On 12/02/2022 02:38, Abhinav Kumar wrote:
>>>
>>>
>>> On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
>>>> Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to 
>>>> guess,
>>>> which intf type we mean, pass INTF_DSI/INTF_DP directly.
>>>>
>>>
>>> Typically, I am only seeing a 1:1 mapping like
>>>
>>> DRM_MODE_ENCODER_DSI means DSI
>>> DRM_MODE_ENCODER_VIRTUAL means WB
>>>
>>> So I am not seeing any guessing for the encoder.
>>
>> s/guessing/deriving/
>>
>> Initially I spotted the DRM_MODE_CONNECTOR_DisplayPort comparison, 
>> then I noticed that there is a misnaming, we were talking about the 
>> intf_type (which clearly belongs to INTF_foo namespace), while passing 
>> DRM_ENCODER_bar instead. Thus comes the proposal to make intf_type 
>> actually contain INTF_type and let DRM_ENCODER live in encoder's code.
>>
>>
>>>
>>> The only conflict I am seeing is between DP and EDP as both use
>>> DRM_MODE_ENCODER_TMDS and hence this approach will be useful there.
>>>
>>> But that has been marked as a "FIXME" below.
>>>
>>> I am suggesting an approach to handle that as well below.
>>> Let me know if you agree with that.
>>
>> Actually this brings a question to me. Do we need to distinguish 
>> between INTF_DP and INTF_EDP from the DPU driver point of view? Are 
>> there any differences? Or we'd better always use INTF_DP here and make 
>> INTF_EDP a memorial of old eDP IP found in 8x74/8x84?
>>
>> So far I see that sc7280 uses INTF_5 for DRM_MODE_CONNECTOR_eDP, but 
>> declares it as INTF_DP. Most probably we should stick to this idea and 
>> drop INTF_EDP from dpu1?
> 
> I believe you are referring to this part:
> 
> static const struct dpu_intf_cfg sc7280_intf[] = {
>      INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, MSM_DP_CONTROLLER_0, 
> 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>      INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24, 
> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
>      INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, MSM_DP_CONTROLLER_1, 
> 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> };

Yep, this one.

> 
> This is interesting ... this should have been marked as INTF_eDP unless 
> I am missing some reason why.
> 
> My take on this is that, since are re-using DP driver now for DP and 
> eDP, less confusion the better in terms of naming of DP/eDP.
> 
> I think we should fix that in the catalog to INTF_eDP. And in case some 
> place is missed out due to this change fix that too.

Well, we need to fix that if there is a difference from the DPU point of 
view (not from the DP IP block). If there is no difference between 
INTF_0 and INTF_5, it might be easier to just use INTF_DP for both of them.

In case we change it, your suggestion seems fine to me, I'll include it 
in v2.

> 
> Meanwhile I can check with sankeerth if there was any specific reason 
> for not using INTF_eDP in the original commit:
> 
> commit ef7837ff091c8805cfa18d2ad06c2e5f4d820a7e
> Author: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> Date:   Tue Nov 2 13:18:42 2021 +0530
> 
>      drm/msm/dp: Add DP controllers for sc7280
> 
>      The eDP controller on SC7280 is similar to the eDP/DP controllers
>      supported by the current driver implementation.
> 
>      SC7280 supports one EDP and one DP controller which can operate
>      concurrently.
> 
>      This change adds the support for eDP and DP controller on sc7280.
> 
>      Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> 
>>
>>>
>>>
>>>> While we are at it, fix the DP audio enablement code which was 
>>>> comparing
>>>> intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
>>>> DRM_MODE_CONNECTOR_DisplayPort (= 10).
>>>> Which would never succeed.
>>>
>>> This is a surprising catch for me and left me thinking for a while 
>>> about how DP audio is working with this bug because that piece of 
>>> code was done to program a register which is needed for DP audio.
>>
>> So did I!
>>
>>>
>>> This bug happened due to difference in the meaning of intf_type between
>>> upstream and downstream.
>>>
>>> After checking more, we found that the register in question has been 
>>> deprecated on newer chipsets so I have asked Kuogee to selectively 
>>> program it. Here is the change for that:
>>>
>>> https://patchwork.freedesktop.org/patch/473869/
>>
>> I'll further comment on it in the respective thread.
>>
>>>
>>>>
>>>
>>>> Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port 
>>>> on MSM")
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 
>>>> +++++++--------------
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 +--
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  4 +--
>>>>   3 files changed, 13 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 1e648db439f9..e8fc029ad607 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
>>>>       hw_mdptop = phys_enc->hw_mdptop;
>>>>       disp_info = &dpu_enc->disp_info;
>>>> -    if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
>>>> +    if (disp_info->intf_type != INTF_DSI)
>>>>           return;
>>>>       /**
>>>> @@ -555,7 +555,7 @@ static struct msm_display_topology 
>>>> dpu_encoder_get_topology(
>>>>       else
>>>>           topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 
>>>> 2 : 1;
>>>> -    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
>>>> +    if (dpu_enc->disp_info.intf_type == INTF_DSI) {
>>>>           if (dpu_kms->catalog->dspp &&
>>>>               (dpu_kms->catalog->dspp_count >= topology.num_lm))
>>>>               topology.num_dspp = topology.num_lm;
>>>> @@ -1099,7 +1099,7 @@ static void 
>>>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>>>       }
>>>> -    if (dpu_enc->disp_info.intf_type == 
>>>> DRM_MODE_CONNECTOR_DisplayPort &&
>>>> +    if (dpu_enc->disp_info.intf_type == INTF_DP &&
>>>>           dpu_enc->cur_master->hw_mdptop &&
>>>>           dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select)
>>>>           dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
>>>> @@ -1107,7 +1107,7 @@ static void 
>>>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>>>       _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
>>>> -    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
>>>> +    if (dpu_enc->disp_info.intf_type == INTF_DSI &&
>>>>               !WARN_ON(dpu_enc->num_phys_encs == 0)) {
>>>>           unsigned bpc = 
>>>> dpu_enc->phys_encs[0]->connector->display_info.bpc;
>>>>           for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>>>> @@ -1981,7 +1981,6 @@ static int dpu_encoder_setup_display(struct 
>>>> dpu_encoder_virt *dpu_enc,
>>>>   {
>>>>       int ret = 0;
>>>>       int i = 0;
>>>> -    enum dpu_intf_type intf_type = INTF_NONE;
>>>>       struct dpu_enc_phys_init_params phys_params;
>>>>       if (!dpu_enc) {
>>>> @@ -1997,15 +1996,6 @@ static int dpu_encoder_setup_display(struct 
>>>> dpu_encoder_virt *dpu_enc,
>>>>       phys_params.parent_ops = &dpu_encoder_parent_ops;
>>>>       phys_params.enc_spinlock = &dpu_enc->enc_spinlock;
>>>> -    switch (disp_info->intf_type) {
>>>> -    case DRM_MODE_ENCODER_DSI:
>>>> -        intf_type = INTF_DSI;
>>>> -        break;
>>>> -    case DRM_MODE_ENCODER_TMDS:
>>>> -        intf_type = INTF_DP;
>>>> -        break;
>>>> -    }
>>>> -
>>>>       WARN_ON(disp_info->num_of_h_tiles < 1);
>>>>       DPU_DEBUG("dsi_info->num_of_h_tiles %d\n", 
>>>> disp_info->num_of_h_tiles);
>>>> @@ -2037,11 +2027,11 @@ static int dpu_encoder_setup_display(struct 
>>>> dpu_encoder_virt *dpu_enc,
>>>>                   i, controller_id, phys_params.split_role);
>>>>           phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>>>> -                                                    intf_type,
>>>> -                                                    controller_id);
>>>> +                disp_info->intf_type,
>>>> +                controller_id);
>>>>           if (phys_params.intf_idx == INTF_MAX) {
>>>>               DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, 
>>>> id %d\n",
>>>> -                          intf_type, controller_id);
>>>> +                          disp_info->intf_type, controller_id);
>>>>               ret = -EINVAL;
>>>>           }
>>>> @@ -2124,11 +2114,11 @@ int dpu_encoder_setup(struct drm_device 
>>>> *dev, struct drm_encoder *enc,
>>>>       timer_setup(&dpu_enc->frame_done_timer,
>>>>               dpu_encoder_frame_done_timeout, 0);
>>>> -    if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
>>>> +    if (disp_info->intf_type == INTF_DSI)
>>>>           timer_setup(&dpu_enc->vsync_event_timer,
>>>>                   dpu_encoder_vsync_event_handler,
>>>>                   0);
>>>> -    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
>>>> +    else if (disp_info->intf_type == INTF_DP || 
>>>> disp_info->intf_type == INTF_EDP)
>>>>           dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>>>>       INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> index ebe3944355bb..3891bcbbe5a4 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> @@ -36,7 +36,7 @@ void dpu_encoder_get_hw_resources(struct 
>>>> drm_encoder *encoder,
>>>>   /**
>>>>    * struct msm_display_info - defines display properties
>>>> - * @intf_type:          DRM_MODE_ENCODER_ type
>>>> + * @intf_type:          INTF_ type
>>>>    * @capabilities:       Bitmask of display flags
>>>>    * @num_of_h_tiles:     Number of horizontal tiles in case of 
>>>> split interface
>>>>    * @h_tile_instance:    Controller instance used per tile. Number 
>>>> of elements is
>>>> @@ -45,7 +45,7 @@ void dpu_encoder_get_hw_resources(struct 
>>>> drm_encoder *encoder,
>>>>    *                 used instead of panel TE in cmd mode panels
>>>>    */
>>>>   struct msm_display_info {
>>>> -    int intf_type;
>>>> +    enum dpu_intf_type intf_type;
>>>>       uint32_t capabilities;
>>>>       uint32_t num_of_h_tiles;
>>>>       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index 47fe11a84a77..f4028be9e2e2 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -564,7 +564,7 @@ static int _dpu_kms_initialize_dsi(struct 
>>>> drm_device *dev,
>>>>           priv->encoders[priv->num_encoders++] = encoder;
>>>>           memset(&info, 0, sizeof(info));
>>>> -        info.intf_type = encoder->encoder_type;
>>>> +        info.intf_type = INTF_DSI;
>>>>           rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>>>           if (rc) {
>>>> @@ -630,7 +630,7 @@ static int 
>>>> _dpu_kms_initialize_displayport(struct drm_device *dev,
>>>>           info.num_of_h_tiles = 1;
>>>>           info.h_tile_instance[0] = i;
>>>>           info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
>>>> -        info.intf_type = encoder->encoder_type;
>>>
>>> You can query the connector type from the DP driver like this:
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index 7cc4d21..0fae0fc 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -1457,7 +1457,7 @@ void msm_dp_debugfs_init(struct msm_dp 
>>> *dp_display, struct drm_minor *minor)
>>>   }
>>>
>>>   int msm_dp_modeset_init(struct msm_dp *dp_display, struct 
>>> drm_device *dev,
>>> -                       struct drm_encoder *encoder)
>>> +                       struct drm_encoder *encoder, int 
>>> **connector_type)
>>>   {
>>>          struct msm_drm_private *priv;
>>>          int ret;
>>> @@ -1498,6 +1498,8 @@ int msm_dp_modeset_init(struct msm_dp 
>>> *dp_display, struct drm_device *dev,
>>>
>>>          priv->bridges[priv->num_bridges++] = dp_display->bridge;
>>>
>>> +       *connector_type = dp_display->connector_type;
>>> +
>>>          return 0;
>>>   }
>>>
>>> Then you can do something like:
>>>
>>> if (connector_type == eDP)
>>>          info.intf_type = INTF_eDP;
>>> else if (connector_type == DP)
>>>          info.intf_type = INTF_DP;
>>>> +        info.intf_type = INTF_DP; /* FIXME: support eDP too */
>>>>           rc = dpu_encoder_setup(dev, encoder, &info);
>>>>           if (rc) {
>>>>               DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>
>>


-- 
With best wishes
Dmitry

Re: [PATCH 2/7] drm/msm/dpu: simplify intf allocation code

[Dmitry Baryshkov]

On 12/02/2022 03:17, Abhinav Kumar wrote:
> 
> 
> On 2/11/2022 4:05 PM, Dmitry Baryshkov wrote:
>> On 12/02/2022 02:38, Abhinav Kumar wrote:
>>>
>>>
>>> On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
>>>> Rather than passing DRM_MODE_ENCODER_* and letting dpu_encoder to 
>>>> guess,
>>>> which intf type we mean, pass INTF_DSI/INTF_DP directly.
>>>>
>>>
>>> Typically, I am only seeing a 1:1 mapping like
>>>
>>> DRM_MODE_ENCODER_DSI means DSI
>>> DRM_MODE_ENCODER_VIRTUAL means WB
>>>
>>> So I am not seeing any guessing for the encoder.
>>
>> s/guessing/deriving/
>>
>> Initially I spotted the DRM_MODE_CONNECTOR_DisplayPort comparison, 
>> then I noticed that there is a misnaming, we were talking about the 
>> intf_type (which clearly belongs to INTF_foo namespace), while passing 
>> DRM_ENCODER_bar instead. Thus comes the proposal to make intf_type 
>> actually contain INTF_type and let DRM_ENCODER live in encoder's code.
>>
>>
>>>
>>> The only conflict I am seeing is between DP and EDP as both use
>>> DRM_MODE_ENCODER_TMDS and hence this approach will be useful there.
>>>
>>> But that has been marked as a "FIXME" below.
>>>
>>> I am suggesting an approach to handle that as well below.
>>> Let me know if you agree with that.
>>
>> Actually this brings a question to me. Do we need to distinguish 
>> between INTF_DP and INTF_EDP from the DPU driver point of view? Are 
>> there any differences? Or we'd better always use INTF_DP here and make 
>> INTF_EDP a memorial of old eDP IP found in 8x74/8x84?
>>
>> So far I see that sc7280 uses INTF_5 for DRM_MODE_CONNECTOR_eDP, but 
>> declares it as INTF_DP. Most probably we should stick to this idea and 
>> drop INTF_EDP from dpu1?
> 
> I believe you are referring to this part:
> 
> static const struct dpu_intf_cfg sc7280_intf[] = {
>      INTF_BLK("intf_0", INTF_0, 0x34000, INTF_DP, MSM_DP_CONTROLLER_0, 
> 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 24, 25),
>      INTF_BLK("intf_1", INTF_1, 0x35000, INTF_DSI, 0, 24, 
> INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 26, 27),
>      INTF_BLK("intf_5", INTF_5, 0x39000, INTF_DP, MSM_DP_CONTROLLER_1, 
> 24, INTF_SC7280_MASK, MDP_SSPP_TOP0_INTR, 22, 23),
> };

Yep, this one.

> 
> This is interesting ... this should have been marked as INTF_eDP unless 
> I am missing some reason why.
> 
> My take on this is that, since are re-using DP driver now for DP and 
> eDP, less confusion the better in terms of naming of DP/eDP.
> 
> I think we should fix that in the catalog to INTF_eDP. And in case some 
> place is missed out due to this change fix that too.

Well, we need to fix that if there is a difference from the DPU point of 
view (not from the DP IP block). If there is no difference between 
INTF_0 and INTF_5, it might be easier to just use INTF_DP for both of them.

In case we change it, your suggestion seems fine to me, I'll include it 
in v2.

> 
> Meanwhile I can check with sankeerth if there was any specific reason 
> for not using INTF_eDP in the original commit:
> 
> commit ef7837ff091c8805cfa18d2ad06c2e5f4d820a7e
> Author: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> Date:   Tue Nov 2 13:18:42 2021 +0530
> 
>      drm/msm/dp: Add DP controllers for sc7280
> 
>      The eDP controller on SC7280 is similar to the eDP/DP controllers
>      supported by the current driver implementation.
> 
>      SC7280 supports one EDP and one DP controller which can operate
>      concurrently.
> 
>      This change adds the support for eDP and DP controller on sc7280.
> 
>      Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> 
>>
>>>
>>>
>>>> While we are at it, fix the DP audio enablement code which was 
>>>> comparing
>>>> intf_type, DRM_MODE_ENCODER_TMDS (= 2) with
>>>> DRM_MODE_CONNECTOR_DisplayPort (= 10).
>>>> Which would never succeed.
>>>
>>> This is a surprising catch for me and left me thinking for a while 
>>> about how DP audio is working with this bug because that piece of 
>>> code was done to program a register which is needed for DP audio.
>>
>> So did I!
>>
>>>
>>> This bug happened due to difference in the meaning of intf_type between
>>> upstream and downstream.
>>>
>>> After checking more, we found that the register in question has been 
>>> deprecated on newer chipsets so I have asked Kuogee to selectively 
>>> program it. Here is the change for that:
>>>
>>> https://patchwork.freedesktop.org/patch/473869/
>>
>> I'll further comment on it in the respective thread.
>>
>>>
>>>>
>>>
>>>> Fixes: d13e36d7d222 ("drm/msm/dp: add audio support for Display Port 
>>>> on MSM")
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 28 
>>>> +++++++--------------
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 +--
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  4 +--
>>>>   3 files changed, 13 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 1e648db439f9..e8fc029ad607 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -493,7 +493,7 @@ void dpu_encoder_helper_split_config(
>>>>       hw_mdptop = phys_enc->hw_mdptop;
>>>>       disp_info = &dpu_enc->disp_info;
>>>> -    if (disp_info->intf_type != DRM_MODE_ENCODER_DSI)
>>>> +    if (disp_info->intf_type != INTF_DSI)
>>>>           return;
>>>>       /**
>>>> @@ -555,7 +555,7 @@ static struct msm_display_topology 
>>>> dpu_encoder_get_topology(
>>>>       else
>>>>           topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 
>>>> 2 : 1;
>>>> -    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI) {
>>>> +    if (dpu_enc->disp_info.intf_type == INTF_DSI) {
>>>>           if (dpu_kms->catalog->dspp &&
>>>>               (dpu_kms->catalog->dspp_count >= topology.num_lm))
>>>>               topology.num_dspp = topology.num_lm;
>>>> @@ -1099,7 +1099,7 @@ static void 
>>>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>>>       }
>>>> -    if (dpu_enc->disp_info.intf_type == 
>>>> DRM_MODE_CONNECTOR_DisplayPort &&
>>>> +    if (dpu_enc->disp_info.intf_type == INTF_DP &&
>>>>           dpu_enc->cur_master->hw_mdptop &&
>>>>           dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select)
>>>>           dpu_enc->cur_master->hw_mdptop->ops.intf_audio_select(
>>>> @@ -1107,7 +1107,7 @@ static void 
>>>> _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>>>       _dpu_encoder_update_vsync_source(dpu_enc, &dpu_enc->disp_info);
>>>> -    if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
>>>> +    if (dpu_enc->disp_info.intf_type == INTF_DSI &&
>>>>               !WARN_ON(dpu_enc->num_phys_encs == 0)) {
>>>>           unsigned bpc = 
>>>> dpu_enc->phys_encs[0]->connector->display_info.bpc;
>>>>           for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>>>> @@ -1981,7 +1981,6 @@ static int dpu_encoder_setup_display(struct 
>>>> dpu_encoder_virt *dpu_enc,
>>>>   {
>>>>       int ret = 0;
>>>>       int i = 0;
>>>> -    enum dpu_intf_type intf_type = INTF_NONE;
>>>>       struct dpu_enc_phys_init_params phys_params;
>>>>       if (!dpu_enc) {
>>>> @@ -1997,15 +1996,6 @@ static int dpu_encoder_setup_display(struct 
>>>> dpu_encoder_virt *dpu_enc,
>>>>       phys_params.parent_ops = &dpu_encoder_parent_ops;
>>>>       phys_params.enc_spinlock = &dpu_enc->enc_spinlock;
>>>> -    switch (disp_info->intf_type) {
>>>> -    case DRM_MODE_ENCODER_DSI:
>>>> -        intf_type = INTF_DSI;
>>>> -        break;
>>>> -    case DRM_MODE_ENCODER_TMDS:
>>>> -        intf_type = INTF_DP;
>>>> -        break;
>>>> -    }
>>>> -
>>>>       WARN_ON(disp_info->num_of_h_tiles < 1);
>>>>       DPU_DEBUG("dsi_info->num_of_h_tiles %d\n", 
>>>> disp_info->num_of_h_tiles);
>>>> @@ -2037,11 +2027,11 @@ static int dpu_encoder_setup_display(struct 
>>>> dpu_encoder_virt *dpu_enc,
>>>>                   i, controller_id, phys_params.split_role);
>>>>           phys_params.intf_idx = dpu_encoder_get_intf(dpu_kms->catalog,
>>>> -                                                    intf_type,
>>>> -                                                    controller_id);
>>>> +                disp_info->intf_type,
>>>> +                controller_id);
>>>>           if (phys_params.intf_idx == INTF_MAX) {
>>>>               DPU_ERROR_ENC(dpu_enc, "could not get intf: type %d, 
>>>> id %d\n",
>>>> -                          intf_type, controller_id);
>>>> +                          disp_info->intf_type, controller_id);
>>>>               ret = -EINVAL;
>>>>           }
>>>> @@ -2124,11 +2114,11 @@ int dpu_encoder_setup(struct drm_device 
>>>> *dev, struct drm_encoder *enc,
>>>>       timer_setup(&dpu_enc->frame_done_timer,
>>>>               dpu_encoder_frame_done_timeout, 0);
>>>> -    if (disp_info->intf_type == DRM_MODE_ENCODER_DSI)
>>>> +    if (disp_info->intf_type == INTF_DSI)
>>>>           timer_setup(&dpu_enc->vsync_event_timer,
>>>>                   dpu_encoder_vsync_event_handler,
>>>>                   0);
>>>> -    else if (disp_info->intf_type == DRM_MODE_ENCODER_TMDS)
>>>> +    else if (disp_info->intf_type == INTF_DP || 
>>>> disp_info->intf_type == INTF_EDP)
>>>>           dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>>>>       INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> index ebe3944355bb..3891bcbbe5a4 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> @@ -36,7 +36,7 @@ void dpu_encoder_get_hw_resources(struct 
>>>> drm_encoder *encoder,
>>>>   /**
>>>>    * struct msm_display_info - defines display properties
>>>> - * @intf_type:          DRM_MODE_ENCODER_ type
>>>> + * @intf_type:          INTF_ type
>>>>    * @capabilities:       Bitmask of display flags
>>>>    * @num_of_h_tiles:     Number of horizontal tiles in case of 
>>>> split interface
>>>>    * @h_tile_instance:    Controller instance used per tile. Number 
>>>> of elements is
>>>> @@ -45,7 +45,7 @@ void dpu_encoder_get_hw_resources(struct 
>>>> drm_encoder *encoder,
>>>>    *                 used instead of panel TE in cmd mode panels
>>>>    */
>>>>   struct msm_display_info {
>>>> -    int intf_type;
>>>> +    enum dpu_intf_type intf_type;
>>>>       uint32_t capabilities;
>>>>       uint32_t num_of_h_tiles;
>>>>       uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index 47fe11a84a77..f4028be9e2e2 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -564,7 +564,7 @@ static int _dpu_kms_initialize_dsi(struct 
>>>> drm_device *dev,
>>>>           priv->encoders[priv->num_encoders++] = encoder;
>>>>           memset(&info, 0, sizeof(info));
>>>> -        info.intf_type = encoder->encoder_type;
>>>> +        info.intf_type = INTF_DSI;
>>>>           rc = msm_dsi_modeset_init(priv->dsi[i], dev, encoder);
>>>>           if (rc) {
>>>> @@ -630,7 +630,7 @@ static int 
>>>> _dpu_kms_initialize_displayport(struct drm_device *dev,
>>>>           info.num_of_h_tiles = 1;
>>>>           info.h_tile_instance[0] = i;
>>>>           info.capabilities = MSM_DISPLAY_CAP_VID_MODE;
>>>> -        info.intf_type = encoder->encoder_type;
>>>
>>> You can query the connector type from the DP driver like this:
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>> index 7cc4d21..0fae0fc 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>> @@ -1457,7 +1457,7 @@ void msm_dp_debugfs_init(struct msm_dp 
>>> *dp_display, struct drm_minor *minor)
>>>   }
>>>
>>>   int msm_dp_modeset_init(struct msm_dp *dp_display, struct 
>>> drm_device *dev,
>>> -                       struct drm_encoder *encoder)
>>> +                       struct drm_encoder *encoder, int 
>>> **connector_type)
>>>   {
>>>          struct msm_drm_private *priv;
>>>          int ret;
>>> @@ -1498,6 +1498,8 @@ int msm_dp_modeset_init(struct msm_dp 
>>> *dp_display, struct drm_device *dev,
>>>
>>>          priv->bridges[priv->num_bridges++] = dp_display->bridge;
>>>
>>> +       *connector_type = dp_display->connector_type;
>>> +
>>>          return 0;
>>>   }
>>>
>>> Then you can do something like:
>>>
>>> if (connector_type == eDP)
>>>          info.intf_type = INTF_eDP;
>>> else if (connector_type == DP)
>>>          info.intf_type = INTF_DP;
>>>> +        info.intf_type = INTF_DP; /* FIXME: support eDP too */
>>>>           rc = dpu_encoder_setup(dev, encoder, &info);
>>>>           if (rc) {
>>>>               DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
>>
>>


-- 
With best wishes
Dmitry

[PATCH 3/7] drm/msm/dpu: remove msm_dp cached in dpu_encoder_virt

[Dmitry Baryshkov]

Stop caching msm_dp instance in dpu_encoder_virt since it's not used
now.

Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index e8fc029ad607..6c1a19ffae38 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -168,7 +168,6 @@ enum dpu_enc_rc_states {
  * @vsync_event_work:		worker to handle vsync event for autorefresh
  * @topology:                   topology of the display
  * @idle_timeout:		idle timeout duration in milliseconds
- * @dp:				msm_dp pointer, for DP encoders
  */
 struct dpu_encoder_virt {
 	struct drm_encoder base;
@@ -207,8 +206,6 @@ struct dpu_encoder_virt {
 	struct msm_display_topology topology;
 
 	u32 idle_timeout;
-
-	struct msm_dp *dp;
 };
 
 #define to_dpu_encoder_virt(x) container_of(x, struct dpu_encoder_virt, base)
@@ -2118,8 +2115,6 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
 		timer_setup(&dpu_enc->vsync_event_timer,
 				dpu_encoder_vsync_event_handler,
 				0);
-	else if (disp_info->intf_type == INTF_DP || disp_info->intf_type == INTF_EDP)
-		dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
 
 	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
 			dpu_encoder_off_work);
-- 
2.34.1

[PATCH 3/7] drm/msm/dpu: remove msm_dp cached in dpu_encoder_virt

[Dmitry Baryshkov]

Stop caching msm_dp instance in dpu_encoder_virt since it's not used
now.

Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index e8fc029ad607..6c1a19ffae38 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -168,7 +168,6 @@ enum dpu_enc_rc_states {
  * @vsync_event_work:		worker to handle vsync event for autorefresh
  * @topology:                   topology of the display
  * @idle_timeout:		idle timeout duration in milliseconds
- * @dp:				msm_dp pointer, for DP encoders
  */
 struct dpu_encoder_virt {
 	struct drm_encoder base;
@@ -207,8 +206,6 @@ struct dpu_encoder_virt {
 	struct msm_display_topology topology;
 
 	u32 idle_timeout;
-
-	struct msm_dp *dp;
 };
 
 #define to_dpu_encoder_virt(x) container_of(x, struct dpu_encoder_virt, base)
@@ -2118,8 +2115,6 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
 		timer_setup(&dpu_enc->vsync_event_timer,
 				dpu_encoder_vsync_event_handler,
 				0);
-	else if (disp_info->intf_type == INTF_DP || disp_info->intf_type == INTF_EDP)
-		dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
 
 	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
 			dpu_encoder_off_work);
-- 
2.34.1

Re: [PATCH 3/7] drm/msm/dpu: remove msm_dp cached in dpu_encoder_virt

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> Stop caching msm_dp instance in dpu_encoder_virt since it's not used
> now.
> 
> Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index e8fc029ad607..6c1a19ffae38 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -168,7 +168,6 @@ enum dpu_enc_rc_states {
>    * @vsync_event_work:		worker to handle vsync event for autorefresh
>    * @topology:                   topology of the display
>    * @idle_timeout:		idle timeout duration in milliseconds
> - * @dp:				msm_dp pointer, for DP encoders
>    */
>   struct dpu_encoder_virt {
>   	struct drm_encoder base;
> @@ -207,8 +206,6 @@ struct dpu_encoder_virt {
>   	struct msm_display_topology topology;
>   
>   	u32 idle_timeout;
> -
> -	struct msm_dp *dp;
>   };
>   
>   #define to_dpu_encoder_virt(x) container_of(x, struct dpu_encoder_virt, base)
> @@ -2118,8 +2115,6 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   		timer_setup(&dpu_enc->vsync_event_timer,
>   				dpu_encoder_vsync_event_handler,
>   				0);
> -	else if (disp_info->intf_type == INTF_DP || disp_info->intf_type == INTF_EDP)
> -		dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>   
>   	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>   			dpu_encoder_off_work);

Re: [PATCH 3/7] drm/msm/dpu: remove msm_dp cached in dpu_encoder_virt

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> Stop caching msm_dp instance in dpu_encoder_virt since it's not used
> now.
> 
> Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable and disable")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 5 -----
>   1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index e8fc029ad607..6c1a19ffae38 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -168,7 +168,6 @@ enum dpu_enc_rc_states {
>    * @vsync_event_work:		worker to handle vsync event for autorefresh
>    * @topology:                   topology of the display
>    * @idle_timeout:		idle timeout duration in milliseconds
> - * @dp:				msm_dp pointer, for DP encoders
>    */
>   struct dpu_encoder_virt {
>   	struct drm_encoder base;
> @@ -207,8 +206,6 @@ struct dpu_encoder_virt {
>   	struct msm_display_topology topology;
>   
>   	u32 idle_timeout;
> -
> -	struct msm_dp *dp;
>   };
>   
>   #define to_dpu_encoder_virt(x) container_of(x, struct dpu_encoder_virt, base)
> @@ -2118,8 +2115,6 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,
>   		timer_setup(&dpu_enc->vsync_event_timer,
>   				dpu_encoder_vsync_event_handler,
>   				0);
> -	else if (disp_info->intf_type == INTF_DP || disp_info->intf_type == INTF_EDP)
> -		dpu_enc->dp = priv->dp[disp_info->h_tile_instance[0]];
>   
>   	INIT_DELAYED_WORK(&dpu_enc->delayed_off_work,
>   			dpu_encoder_off_work);

[PATCH 4/7] drm/msm/dpu: drop bus_scaling_client field

[Dmitry Baryshkov]

We do not use MSM bus client, so drop bus_scaling_client field from
dpu_encoder_virt.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 6c1a19ffae38..4530549850f0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -127,7 +127,6 @@ enum dpu_enc_rc_states {
  *	Virtual encoder registers itself with the DRM Framework as the encoder.
  * @base:		drm_encoder base class for registration with DRM
  * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
- * @bus_scaling_client:	Client handle to the bus scaling interface
  * @enabled:		True if the encoder is active, protected by enc_lock
  * @num_phys_encs:	Actual number of physical encoders contained.
  * @phys_encs:		Container of physical encoders managed.
@@ -172,7 +171,6 @@ enum dpu_enc_rc_states {
 struct dpu_encoder_virt {
 	struct drm_encoder base;
 	spinlock_t enc_spinlock;
-	uint32_t bus_scaling_client;
 
 	bool enabled;
 
-- 
2.34.1

[PATCH 4/7] drm/msm/dpu: drop bus_scaling_client field

[Dmitry Baryshkov]

We do not use MSM bus client, so drop bus_scaling_client field from
dpu_encoder_virt.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 6c1a19ffae38..4530549850f0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -127,7 +127,6 @@ enum dpu_enc_rc_states {
  *	Virtual encoder registers itself with the DRM Framework as the encoder.
  * @base:		drm_encoder base class for registration with DRM
  * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
- * @bus_scaling_client:	Client handle to the bus scaling interface
  * @enabled:		True if the encoder is active, protected by enc_lock
  * @num_phys_encs:	Actual number of physical encoders contained.
  * @phys_encs:		Container of physical encoders managed.
@@ -172,7 +171,6 @@ enum dpu_enc_rc_states {
 struct dpu_encoder_virt {
 	struct drm_encoder base;
 	spinlock_t enc_spinlock;
-	uint32_t bus_scaling_client;
 
 	bool enabled;
 
-- 
2.34.1

Re: [PATCH 4/7] drm/msm/dpu: drop bus_scaling_client field

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> We do not use MSM bus client, so drop bus_scaling_client field from
> dpu_encoder_virt.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 6c1a19ffae38..4530549850f0 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -127,7 +127,6 @@ enum dpu_enc_rc_states {
>    *	Virtual encoder registers itself with the DRM Framework as the encoder.
>    * @base:		drm_encoder base class for registration with DRM
>    * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
> - * @bus_scaling_client:	Client handle to the bus scaling interface
>    * @enabled:		True if the encoder is active, protected by enc_lock
>    * @num_phys_encs:	Actual number of physical encoders contained.
>    * @phys_encs:		Container of physical encoders managed.
> @@ -172,7 +171,6 @@ enum dpu_enc_rc_states {
>   struct dpu_encoder_virt {
>   	struct drm_encoder base;
>   	spinlock_t enc_spinlock;
> -	uint32_t bus_scaling_client;
>   
>   	bool enabled;
>   

Re: [PATCH 4/7] drm/msm/dpu: drop bus_scaling_client field

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> We do not use MSM bus client, so drop bus_scaling_client field from
> dpu_encoder_virt.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 6c1a19ffae38..4530549850f0 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -127,7 +127,6 @@ enum dpu_enc_rc_states {
>    *	Virtual encoder registers itself with the DRM Framework as the encoder.
>    * @base:		drm_encoder base class for registration with DRM
>    * @enc_spinlock:	Virtual-Encoder-Wide Spin Lock for IRQ purposes
> - * @bus_scaling_client:	Client handle to the bus scaling interface
>    * @enabled:		True if the encoder is active, protected by enc_lock
>    * @num_phys_encs:	Actual number of physical encoders contained.
>    * @phys_encs:		Container of physical encoders managed.
> @@ -172,7 +171,6 @@ enum dpu_enc_rc_states {
>   struct dpu_encoder_virt {
>   	struct drm_encoder base;
>   	spinlock_t enc_spinlock;
> -	uint32_t bus_scaling_client;
>   
>   	bool enabled;
>   

[PATCH 5/7] drm/msm/dpu: encoder: drop unused callbacks

[Dmitry Baryshkov]

Both cmd and vid backends provide no atomic_check() callbacks and
useless mode_fixup() callbacks. Drop support for both of them.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 39 +++++--------------
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  8 ----
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 10 -----
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 14 -------
 4 files changed, 9 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 4530549850f0..e1c43a0c0643 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -573,7 +573,6 @@ static int dpu_encoder_virt_atomic_check(
 	struct drm_display_mode *adj_mode;
 	struct msm_display_topology topology;
 	struct dpu_global_state *global_state;
-	int i = 0;
 	int ret = 0;
 
 	if (!drm_enc || !crtc_state || !conn_state) {
@@ -595,39 +594,19 @@ static int dpu_encoder_virt_atomic_check(
 
 	trace_dpu_enc_atomic_check(DRMID(drm_enc));
 
-	/* perform atomic check on the first physical encoder (master) */
-	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-
-		if (phys->ops.atomic_check)
-			ret = phys->ops.atomic_check(phys, crtc_state,
-					conn_state);
-		else if (phys->ops.mode_fixup)
-			if (!phys->ops.mode_fixup(phys, mode, adj_mode))
-				ret = -EINVAL;
-
-		if (ret) {
-			DPU_ERROR_ENC(dpu_enc,
-					"mode unsupported, phys idx %d\n", i);
-			break;
-		}
-	}
-
 	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
 
 	/* Reserve dynamic resources now. */
-	if (!ret) {
-		/*
-		 * Release and Allocate resources on every modeset
-		 * Dont allocate when active is false.
-		 */
-		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
-			dpu_rm_release(global_state, drm_enc);
+	/*
+	 * Release and Allocate resources on every modeset
+	 * Dont allocate when active is false.
+	 */
+	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
+		dpu_rm_release(global_state, drm_enc);
 
-			if (!crtc_state->active_changed || crtc_state->active)
-				ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
-						drm_enc, crtc_state, topology);
-		}
+		if (!crtc_state->active_changed || crtc_state->active)
+			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
+					drm_enc, crtc_state, topology);
 	}
 
 	trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index e7270eb6b84b..159deb8ed7fb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -84,12 +84,10 @@ struct dpu_encoder_virt_ops {
  * @is_master:			Whether this phys_enc is the current master
  *				encoder. Can be switched at enable time. Based
  *				on split_role and current mode (CMD/VID).
- * @mode_fixup:			DRM Call. Fixup a DRM mode.
  * @mode_set:			DRM Call. Set a DRM mode.
  *				This likely caches the mode, for use at enable.
  * @enable:			DRM Call. Enable a DRM mode.
  * @disable:			DRM Call. Disable mode.
- * @atomic_check:		DRM Call. Atomic check new DRM state.
  * @destroy:			DRM Call. Destroy and release resources.
  * @get_hw_resources:		Populate the structure with the hardware
  *				resources that this phys_enc is using.
@@ -117,17 +115,11 @@ struct dpu_encoder_phys_ops {
 			struct dentry *debugfs_root);
 	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
 	bool (*is_master)(struct dpu_encoder_phys *encoder);
-	bool (*mode_fixup)(struct dpu_encoder_phys *encoder,
-			const struct drm_display_mode *mode,
-			struct drm_display_mode *adjusted_mode);
 	void (*mode_set)(struct dpu_encoder_phys *encoder,
 			struct drm_display_mode *mode,
 			struct drm_display_mode *adjusted_mode);
 	void (*enable)(struct dpu_encoder_phys *encoder);
 	void (*disable)(struct dpu_encoder_phys *encoder);
-	int (*atomic_check)(struct dpu_encoder_phys *encoder,
-			    struct drm_crtc_state *crtc_state,
-			    struct drm_connector_state *conn_state);
 	void (*destroy)(struct dpu_encoder_phys *encoder);
 	void (*get_hw_resources)(struct dpu_encoder_phys *encoder,
 				 struct dpu_encoder_hw_resources *hw_res);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 34a6940d12c5..45fe97fb612d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -45,15 +45,6 @@ static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc)
 	return (phys_enc->split_role != ENC_ROLE_SLAVE);
 }
 
-static bool dpu_encoder_phys_cmd_mode_fixup(
-		struct dpu_encoder_phys *phys_enc,
-		const struct drm_display_mode *mode,
-		struct drm_display_mode *adj_mode)
-{
-	DPU_DEBUG_CMDENC(to_dpu_encoder_phys_cmd(phys_enc), "\n");
-	return true;
-}
-
 static void _dpu_encoder_phys_cmd_update_intf_cfg(
 		struct dpu_encoder_phys *phys_enc)
 {
@@ -732,7 +723,6 @@ static void dpu_encoder_phys_cmd_init_ops(
 	ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
 	ops->is_master = dpu_encoder_phys_cmd_is_master;
 	ops->mode_set = dpu_encoder_phys_cmd_mode_set;
-	ops->mode_fixup = dpu_encoder_phys_cmd_mode_fixup;
 	ops->enable = dpu_encoder_phys_cmd_enable;
 	ops->disable = dpu_encoder_phys_cmd_disable;
 	ops->destroy = dpu_encoder_phys_cmd_destroy;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index ddd9d89cd456..1831fe37c88c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -225,19 +225,6 @@ static void programmable_fetch_config(struct dpu_encoder_phys *phys_enc,
 	spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
 }
 
-static bool dpu_encoder_phys_vid_mode_fixup(
-		struct dpu_encoder_phys *phys_enc,
-		const struct drm_display_mode *mode,
-		struct drm_display_mode *adj_mode)
-{
-	DPU_DEBUG_VIDENC(phys_enc, "\n");
-
-	/*
-	 * Modifying mode has consequences when the mode comes back to us
-	 */
-	return true;
-}
-
 static void dpu_encoder_phys_vid_setup_timing_engine(
 		struct dpu_encoder_phys *phys_enc)
 {
@@ -676,7 +663,6 @@ static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
 {
 	ops->is_master = dpu_encoder_phys_vid_is_master;
 	ops->mode_set = dpu_encoder_phys_vid_mode_set;
-	ops->mode_fixup = dpu_encoder_phys_vid_mode_fixup;
 	ops->enable = dpu_encoder_phys_vid_enable;
 	ops->disable = dpu_encoder_phys_vid_disable;
 	ops->destroy = dpu_encoder_phys_vid_destroy;
-- 
2.34.1

[PATCH 5/7] drm/msm/dpu: encoder: drop unused callbacks

[Dmitry Baryshkov]

Both cmd and vid backends provide no atomic_check() callbacks and
useless mode_fixup() callbacks. Drop support for both of them.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 39 +++++--------------
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  8 ----
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 10 -----
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 14 -------
 4 files changed, 9 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 4530549850f0..e1c43a0c0643 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -573,7 +573,6 @@ static int dpu_encoder_virt_atomic_check(
 	struct drm_display_mode *adj_mode;
 	struct msm_display_topology topology;
 	struct dpu_global_state *global_state;
-	int i = 0;
 	int ret = 0;
 
 	if (!drm_enc || !crtc_state || !conn_state) {
@@ -595,39 +594,19 @@ static int dpu_encoder_virt_atomic_check(
 
 	trace_dpu_enc_atomic_check(DRMID(drm_enc));
 
-	/* perform atomic check on the first physical encoder (master) */
-	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
-
-		if (phys->ops.atomic_check)
-			ret = phys->ops.atomic_check(phys, crtc_state,
-					conn_state);
-		else if (phys->ops.mode_fixup)
-			if (!phys->ops.mode_fixup(phys, mode, adj_mode))
-				ret = -EINVAL;
-
-		if (ret) {
-			DPU_ERROR_ENC(dpu_enc,
-					"mode unsupported, phys idx %d\n", i);
-			break;
-		}
-	}
-
 	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
 
 	/* Reserve dynamic resources now. */
-	if (!ret) {
-		/*
-		 * Release and Allocate resources on every modeset
-		 * Dont allocate when active is false.
-		 */
-		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
-			dpu_rm_release(global_state, drm_enc);
+	/*
+	 * Release and Allocate resources on every modeset
+	 * Dont allocate when active is false.
+	 */
+	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
+		dpu_rm_release(global_state, drm_enc);
 
-			if (!crtc_state->active_changed || crtc_state->active)
-				ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
-						drm_enc, crtc_state, topology);
-		}
+		if (!crtc_state->active_changed || crtc_state->active)
+			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
+					drm_enc, crtc_state, topology);
 	}
 
 	trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index e7270eb6b84b..159deb8ed7fb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -84,12 +84,10 @@ struct dpu_encoder_virt_ops {
  * @is_master:			Whether this phys_enc is the current master
  *				encoder. Can be switched at enable time. Based
  *				on split_role and current mode (CMD/VID).
- * @mode_fixup:			DRM Call. Fixup a DRM mode.
  * @mode_set:			DRM Call. Set a DRM mode.
  *				This likely caches the mode, for use at enable.
  * @enable:			DRM Call. Enable a DRM mode.
  * @disable:			DRM Call. Disable mode.
- * @atomic_check:		DRM Call. Atomic check new DRM state.
  * @destroy:			DRM Call. Destroy and release resources.
  * @get_hw_resources:		Populate the structure with the hardware
  *				resources that this phys_enc is using.
@@ -117,17 +115,11 @@ struct dpu_encoder_phys_ops {
 			struct dentry *debugfs_root);
 	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
 	bool (*is_master)(struct dpu_encoder_phys *encoder);
-	bool (*mode_fixup)(struct dpu_encoder_phys *encoder,
-			const struct drm_display_mode *mode,
-			struct drm_display_mode *adjusted_mode);
 	void (*mode_set)(struct dpu_encoder_phys *encoder,
 			struct drm_display_mode *mode,
 			struct drm_display_mode *adjusted_mode);
 	void (*enable)(struct dpu_encoder_phys *encoder);
 	void (*disable)(struct dpu_encoder_phys *encoder);
-	int (*atomic_check)(struct dpu_encoder_phys *encoder,
-			    struct drm_crtc_state *crtc_state,
-			    struct drm_connector_state *conn_state);
 	void (*destroy)(struct dpu_encoder_phys *encoder);
 	void (*get_hw_resources)(struct dpu_encoder_phys *encoder,
 				 struct dpu_encoder_hw_resources *hw_res);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 34a6940d12c5..45fe97fb612d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -45,15 +45,6 @@ static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc)
 	return (phys_enc->split_role != ENC_ROLE_SLAVE);
 }
 
-static bool dpu_encoder_phys_cmd_mode_fixup(
-		struct dpu_encoder_phys *phys_enc,
-		const struct drm_display_mode *mode,
-		struct drm_display_mode *adj_mode)
-{
-	DPU_DEBUG_CMDENC(to_dpu_encoder_phys_cmd(phys_enc), "\n");
-	return true;
-}
-
 static void _dpu_encoder_phys_cmd_update_intf_cfg(
 		struct dpu_encoder_phys *phys_enc)
 {
@@ -732,7 +723,6 @@ static void dpu_encoder_phys_cmd_init_ops(
 	ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
 	ops->is_master = dpu_encoder_phys_cmd_is_master;
 	ops->mode_set = dpu_encoder_phys_cmd_mode_set;
-	ops->mode_fixup = dpu_encoder_phys_cmd_mode_fixup;
 	ops->enable = dpu_encoder_phys_cmd_enable;
 	ops->disable = dpu_encoder_phys_cmd_disable;
 	ops->destroy = dpu_encoder_phys_cmd_destroy;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index ddd9d89cd456..1831fe37c88c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -225,19 +225,6 @@ static void programmable_fetch_config(struct dpu_encoder_phys *phys_enc,
 	spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
 }
 
-static bool dpu_encoder_phys_vid_mode_fixup(
-		struct dpu_encoder_phys *phys_enc,
-		const struct drm_display_mode *mode,
-		struct drm_display_mode *adj_mode)
-{
-	DPU_DEBUG_VIDENC(phys_enc, "\n");
-
-	/*
-	 * Modifying mode has consequences when the mode comes back to us
-	 */
-	return true;
-}
-
 static void dpu_encoder_phys_vid_setup_timing_engine(
 		struct dpu_encoder_phys *phys_enc)
 {
@@ -676,7 +663,6 @@ static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
 {
 	ops->is_master = dpu_encoder_phys_vid_is_master;
 	ops->mode_set = dpu_encoder_phys_vid_mode_set;
-	ops->mode_fixup = dpu_encoder_phys_vid_mode_fixup;
 	ops->enable = dpu_encoder_phys_vid_enable;
 	ops->disable = dpu_encoder_phys_vid_disable;
 	ops->destroy = dpu_encoder_phys_vid_destroy;
-- 
2.34.1

Re: [PATCH 5/7] drm/msm/dpu: encoder: drop unused callbacks

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 10244 bytes --]

Hi Dmitry,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm/drm-next]
[also build test WARNING on v5.17-rc2 next-20220203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Dmitry-Baryshkov/drm-msm-dpu-cleanup-dpu-encoder-code/20220203-162806
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: arc-randconfig-r043-20220130 (https://download.01.org/0day-ci/archive/20220203/202202031835.R3GWD0oa-lkp(a)intel.com/config)
compiler: arc-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4661d4d24ba160d3be73c1760f78cddcdc113004
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Dmitry-Baryshkov/drm-msm-dpu-cleanup-dpu-encoder-code/20220203-162806
        git checkout 4661d4d24ba160d3be73c1760f78cddcdc113004
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash drivers/gpu/drm/msm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c: In function 'dpu_encoder_virt_atomic_check':
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:572:40: warning: variable 'mode' set but not used [-Wunused-but-set-variable]
     572 |         const struct drm_display_mode *mode;
         |                                        ^~~~
   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c: In function 'dpu_encoder_virt_enable':
   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:1119:33: warning: variable 'priv' set but not used [-Wunused-but-set-variable]
    1119 |         struct msm_drm_private *priv;
         |                                 ^~~~
   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c: In function 'dpu_encoder_virt_disable':
   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:1156:33: warning: variable 'priv' set but not used [-Wunused-but-set-variable]
    1156 |         struct msm_drm_private *priv;
         |                                 ^~~~


vim +/mode +572 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  521  
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  522  static struct msm_display_topology dpu_encoder_get_topology(
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  523  			struct dpu_encoder_virt *dpu_enc,
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  524  			struct dpu_kms *dpu_kms,
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  525  			struct drm_display_mode *mode)
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  526  {
5fddd4f5db8730 Kalyan Thota      2020-06-11  527  	struct msm_display_topology topology = {0};
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  528  	int i, intf_count = 0;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  529  
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  530  	for (i = 0; i < MAX_PHYS_ENCODERS_PER_VIRTUAL; i++)
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  531  		if (dpu_enc->phys_encs[i])
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  532  			intf_count++;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  533  
42a558b71c9ead Kalyan Thota      2019-11-25  534  	/* Datapath topology selection
42a558b71c9ead Kalyan Thota      2019-11-25  535  	 *
42a558b71c9ead Kalyan Thota      2019-11-25  536  	 * Dual display
42a558b71c9ead Kalyan Thota      2019-11-25  537  	 * 2 LM, 2 INTF ( Split display using 2 interfaces)
42a558b71c9ead Kalyan Thota      2019-11-25  538  	 *
42a558b71c9ead Kalyan Thota      2019-11-25  539  	 * Single display
42a558b71c9ead Kalyan Thota      2019-11-25  540  	 * 1 LM, 1 INTF
42a558b71c9ead Kalyan Thota      2019-11-25  541  	 * 2 LM, 1 INTF (stream merge to support high resolution interfaces)
42a558b71c9ead Kalyan Thota      2019-11-25  542  	 *
5fddd4f5db8730 Kalyan Thota      2020-06-11  543  	 * Adding color blocks only to primary interface if available in
5fddd4f5db8730 Kalyan Thota      2020-06-11  544  	 * sufficient number
42a558b71c9ead Kalyan Thota      2019-11-25  545  	 */
42a558b71c9ead Kalyan Thota      2019-11-25  546  	if (intf_count == 2)
42a558b71c9ead Kalyan Thota      2019-11-25  547  		topology.num_lm = 2;
42a558b71c9ead Kalyan Thota      2019-11-25  548  	else if (!dpu_kms->catalog->caps->has_3d_merge)
42a558b71c9ead Kalyan Thota      2019-11-25  549  		topology.num_lm = 1;
42a558b71c9ead Kalyan Thota      2019-11-25  550  	else
42a558b71c9ead Kalyan Thota      2019-11-25  551  		topology.num_lm = (mode->hdisplay > MAX_HDISPLAY_SPLIT) ? 2 : 1;
42a558b71c9ead Kalyan Thota      2019-11-25  552  
f6393abee4827d Dmitry Baryshkov  2022-02-03  553  	if (dpu_enc->disp_info.intf_type == INTF_DSI) {
5fddd4f5db8730 Kalyan Thota      2020-06-11  554  		if (dpu_kms->catalog->dspp &&
5fddd4f5db8730 Kalyan Thota      2020-06-11  555  			(dpu_kms->catalog->dspp_count >= topology.num_lm))
e47616df008b10 Kalyan Thota      2020-03-24  556  			topology.num_dspp = topology.num_lm;
5fddd4f5db8730 Kalyan Thota      2020-06-11  557  	}
e47616df008b10 Kalyan Thota      2020-03-24  558  
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  559  	topology.num_enc = 0;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  560  	topology.num_intf = intf_count;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  561  
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  562  	return topology;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  563  }
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  564  static int dpu_encoder_virt_atomic_check(
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  565  		struct drm_encoder *drm_enc,
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  566  		struct drm_crtc_state *crtc_state,
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  567  		struct drm_connector_state *conn_state)
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  568  {
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  569  	struct dpu_encoder_virt *dpu_enc;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  570  	struct msm_drm_private *priv;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  571  	struct dpu_kms *dpu_kms;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 @572  	const struct drm_display_mode *mode;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  573  	struct drm_display_mode *adj_mode;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  574  	struct msm_display_topology topology;
de3916c70a24e3 Drew Davenport    2020-02-19  575  	struct dpu_global_state *global_state;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  576  	int ret = 0;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  577  
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  578  	if (!drm_enc || !crtc_state || !conn_state) {
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  579  		DPU_ERROR("invalid arg(s), drm_enc %d, crtc/conn state %d/%d\n",
e6790f7210062e Zheng Bin         2020-01-23  580  				drm_enc != NULL, crtc_state != NULL, conn_state != NULL);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  581  		return -EINVAL;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  582  	}
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  583  
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  584  	dpu_enc = to_dpu_encoder_virt(drm_enc);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  585  	DPU_DEBUG_ENC(dpu_enc, "\n");
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  586  
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  587  	priv = drm_enc->dev->dev_private;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  588  	dpu_kms = to_dpu_kms(priv->kms);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  589  	mode = &crtc_state->mode;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  590  	adj_mode = &crtc_state->adjusted_mode;
ccc862b957c641 Kalyan Thota      2020-08-10  591  	global_state = dpu_kms_get_global_state(crtc_state->state);
ccc862b957c641 Kalyan Thota      2020-08-10  592  	if (IS_ERR(global_state))
ccc862b957c641 Kalyan Thota      2020-08-10  593  		return PTR_ERR(global_state);
ccc862b957c641 Kalyan Thota      2020-08-10  594  
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  595  	trace_dpu_enc_atomic_check(DRMID(drm_enc));
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  596  
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  597  	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  598  
de3916c70a24e3 Drew Davenport    2020-02-19  599  	/* Reserve dynamic resources now. */
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  600  	/*
ccc862b957c641 Kalyan Thota      2020-08-10  601  	 * Release and Allocate resources on every modeset
ccc862b957c641 Kalyan Thota      2020-08-10  602  	 * Dont allocate when active is false.
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  603  	 */
de3916c70a24e3 Drew Davenport    2020-02-19  604  	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
ccc862b957c641 Kalyan Thota      2020-08-10  605  		dpu_rm_release(global_state, drm_enc);
ccc862b957c641 Kalyan Thota      2020-08-10  606  
ccc862b957c641 Kalyan Thota      2020-08-10  607  		if (!crtc_state->active_changed || crtc_state->active)
de3916c70a24e3 Drew Davenport    2020-02-19  608  			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
de3916c70a24e3 Drew Davenport    2020-02-19  609  					drm_enc, crtc_state, topology);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  610  	}
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  611  
5dce0c0b92d426 Ville Syrjälä     2020-04-28  612  	trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  613  
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  614  	return ret;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  615  }
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27  616  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH 5/7] drm/msm/dpu: encoder: drop unused callbacks

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> Both cmd and vid backends provide no atomic_check() callbacks and
> useless mode_fixup() callbacks. Drop support for both of them.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

We can drop mode_fixup() but I am using atomic_check() for WB to 
validate whether the programmed FB exceeds WB limits OR
the programmed mode. Hence without an alternative for that we cannot 
drop that

Please refer to dpu_encoder_phys_wb_atomic_check in 
https://patchwork.freedesktop.org/patch/472324/

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 39 +++++--------------
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  8 ----
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 10 -----
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 14 -------
>   4 files changed, 9 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 4530549850f0..e1c43a0c0643 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -573,7 +573,6 @@ static int dpu_encoder_virt_atomic_check(
>   	struct drm_display_mode *adj_mode;
>   	struct msm_display_topology topology;
>   	struct dpu_global_state *global_state;
> -	int i = 0;
>   	int ret = 0;
>   
>   	if (!drm_enc || !crtc_state || !conn_state) {
> @@ -595,39 +594,19 @@ static int dpu_encoder_virt_atomic_check(
>   
>   	trace_dpu_enc_atomic_check(DRMID(drm_enc));
>   
> -	/* perform atomic check on the first physical encoder (master) */
> -	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> -		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> -
> -		if (phys->ops.atomic_check)
> -			ret = phys->ops.atomic_check(phys, crtc_state,
> -					conn_state);
> -		else if (phys->ops.mode_fixup)
> -			if (!phys->ops.mode_fixup(phys, mode, adj_mode))
> -				ret = -EINVAL;
> -
> -		if (ret) {
> -			DPU_ERROR_ENC(dpu_enc,
> -					"mode unsupported, phys idx %d\n", i);
> -			break;
> -		}
> -	}
> -
>   	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
>   
>   	/* Reserve dynamic resources now. */
> -	if (!ret) {
> -		/*
> -		 * Release and Allocate resources on every modeset
> -		 * Dont allocate when active is false.
> -		 */
> -		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> -			dpu_rm_release(global_state, drm_enc);
> +	/*
> +	 * Release and Allocate resources on every modeset
> +	 * Dont allocate when active is false.
> +	 */
> +	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> +		dpu_rm_release(global_state, drm_enc);
>   
> -			if (!crtc_state->active_changed || crtc_state->active)
> -				ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> -						drm_enc, crtc_state, topology);
> -		}
> +		if (!crtc_state->active_changed || crtc_state->active)
> +			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> +					drm_enc, crtc_state, topology);
>   	}
>   
>   	trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index e7270eb6b84b..159deb8ed7fb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -84,12 +84,10 @@ struct dpu_encoder_virt_ops {
>    * @is_master:			Whether this phys_enc is the current master
>    *				encoder. Can be switched at enable time. Based
>    *				on split_role and current mode (CMD/VID).
> - * @mode_fixup:			DRM Call. Fixup a DRM mode.
>    * @mode_set:			DRM Call. Set a DRM mode.
>    *				This likely caches the mode, for use at enable.
>    * @enable:			DRM Call. Enable a DRM mode.
>    * @disable:			DRM Call. Disable mode.
> - * @atomic_check:		DRM Call. Atomic check new DRM state.
>    * @destroy:			DRM Call. Destroy and release resources.
>    * @get_hw_resources:		Populate the structure with the hardware
>    *				resources that this phys_enc is using.
> @@ -117,17 +115,11 @@ struct dpu_encoder_phys_ops {
>   			struct dentry *debugfs_root);
>   	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
>   	bool (*is_master)(struct dpu_encoder_phys *encoder);
> -	bool (*mode_fixup)(struct dpu_encoder_phys *encoder,
> -			const struct drm_display_mode *mode,
> -			struct drm_display_mode *adjusted_mode);
>   	void (*mode_set)(struct dpu_encoder_phys *encoder,
>   			struct drm_display_mode *mode,
>   			struct drm_display_mode *adjusted_mode);
>   	void (*enable)(struct dpu_encoder_phys *encoder);
>   	void (*disable)(struct dpu_encoder_phys *encoder);
> -	int (*atomic_check)(struct dpu_encoder_phys *encoder,
> -			    struct drm_crtc_state *crtc_state,
> -			    struct drm_connector_state *conn_state);
>   	void (*destroy)(struct dpu_encoder_phys *encoder);
>   	void (*get_hw_resources)(struct dpu_encoder_phys *encoder,
>   				 struct dpu_encoder_hw_resources *hw_res);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index 34a6940d12c5..45fe97fb612d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -45,15 +45,6 @@ static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc)
>   	return (phys_enc->split_role != ENC_ROLE_SLAVE);
>   }
>   
> -static bool dpu_encoder_phys_cmd_mode_fixup(
> -		struct dpu_encoder_phys *phys_enc,
> -		const struct drm_display_mode *mode,
> -		struct drm_display_mode *adj_mode)
> -{
> -	DPU_DEBUG_CMDENC(to_dpu_encoder_phys_cmd(phys_enc), "\n");
> -	return true;
> -}
> -
>   static void _dpu_encoder_phys_cmd_update_intf_cfg(
>   		struct dpu_encoder_phys *phys_enc)
>   {
> @@ -732,7 +723,6 @@ static void dpu_encoder_phys_cmd_init_ops(
>   	ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
>   	ops->is_master = dpu_encoder_phys_cmd_is_master;
>   	ops->mode_set = dpu_encoder_phys_cmd_mode_set;
> -	ops->mode_fixup = dpu_encoder_phys_cmd_mode_fixup;
>   	ops->enable = dpu_encoder_phys_cmd_enable;
>   	ops->disable = dpu_encoder_phys_cmd_disable;
>   	ops->destroy = dpu_encoder_phys_cmd_destroy;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index ddd9d89cd456..1831fe37c88c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -225,19 +225,6 @@ static void programmable_fetch_config(struct dpu_encoder_phys *phys_enc,
>   	spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
>   }
>   
> -static bool dpu_encoder_phys_vid_mode_fixup(
> -		struct dpu_encoder_phys *phys_enc,
> -		const struct drm_display_mode *mode,
> -		struct drm_display_mode *adj_mode)
> -{
> -	DPU_DEBUG_VIDENC(phys_enc, "\n");
> -
> -	/*
> -	 * Modifying mode has consequences when the mode comes back to us
> -	 */
> -	return true;
> -}
> -
>   static void dpu_encoder_phys_vid_setup_timing_engine(
>   		struct dpu_encoder_phys *phys_enc)
>   {
> @@ -676,7 +663,6 @@ static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
>   {
>   	ops->is_master = dpu_encoder_phys_vid_is_master;
>   	ops->mode_set = dpu_encoder_phys_vid_mode_set;
> -	ops->mode_fixup = dpu_encoder_phys_vid_mode_fixup;
>   	ops->enable = dpu_encoder_phys_vid_enable;
>   	ops->disable = dpu_encoder_phys_vid_disable;
>   	ops->destroy = dpu_encoder_phys_vid_destroy;

Re: [PATCH 5/7] drm/msm/dpu: encoder: drop unused callbacks

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> Both cmd and vid backends provide no atomic_check() callbacks and
> useless mode_fixup() callbacks. Drop support for both of them.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

We can drop mode_fixup() but I am using atomic_check() for WB to 
validate whether the programmed FB exceeds WB limits OR
the programmed mode. Hence without an alternative for that we cannot 
drop that

Please refer to dpu_encoder_phys_wb_atomic_check in 
https://patchwork.freedesktop.org/patch/472324/

> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 39 +++++--------------
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  8 ----
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 10 -----
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 14 -------
>   4 files changed, 9 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 4530549850f0..e1c43a0c0643 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -573,7 +573,6 @@ static int dpu_encoder_virt_atomic_check(
>   	struct drm_display_mode *adj_mode;
>   	struct msm_display_topology topology;
>   	struct dpu_global_state *global_state;
> -	int i = 0;
>   	int ret = 0;
>   
>   	if (!drm_enc || !crtc_state || !conn_state) {
> @@ -595,39 +594,19 @@ static int dpu_encoder_virt_atomic_check(
>   
>   	trace_dpu_enc_atomic_check(DRMID(drm_enc));
>   
> -	/* perform atomic check on the first physical encoder (master) */
> -	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> -		struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> -
> -		if (phys->ops.atomic_check)
> -			ret = phys->ops.atomic_check(phys, crtc_state,
> -					conn_state);
> -		else if (phys->ops.mode_fixup)
> -			if (!phys->ops.mode_fixup(phys, mode, adj_mode))
> -				ret = -EINVAL;
> -
> -		if (ret) {
> -			DPU_ERROR_ENC(dpu_enc,
> -					"mode unsupported, phys idx %d\n", i);
> -			break;
> -		}
> -	}
> -
>   	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
>   
>   	/* Reserve dynamic resources now. */
> -	if (!ret) {
> -		/*
> -		 * Release and Allocate resources on every modeset
> -		 * Dont allocate when active is false.
> -		 */
> -		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> -			dpu_rm_release(global_state, drm_enc);
> +	/*
> +	 * Release and Allocate resources on every modeset
> +	 * Dont allocate when active is false.
> +	 */
> +	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> +		dpu_rm_release(global_state, drm_enc);
>   
> -			if (!crtc_state->active_changed || crtc_state->active)
> -				ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> -						drm_enc, crtc_state, topology);
> -		}
> +		if (!crtc_state->active_changed || crtc_state->active)
> +			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> +					drm_enc, crtc_state, topology);
>   	}
>   
>   	trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index e7270eb6b84b..159deb8ed7fb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -84,12 +84,10 @@ struct dpu_encoder_virt_ops {
>    * @is_master:			Whether this phys_enc is the current master
>    *				encoder. Can be switched at enable time. Based
>    *				on split_role and current mode (CMD/VID).
> - * @mode_fixup:			DRM Call. Fixup a DRM mode.
>    * @mode_set:			DRM Call. Set a DRM mode.
>    *				This likely caches the mode, for use at enable.
>    * @enable:			DRM Call. Enable a DRM mode.
>    * @disable:			DRM Call. Disable mode.
> - * @atomic_check:		DRM Call. Atomic check new DRM state.
>    * @destroy:			DRM Call. Destroy and release resources.
>    * @get_hw_resources:		Populate the structure with the hardware
>    *				resources that this phys_enc is using.
> @@ -117,17 +115,11 @@ struct dpu_encoder_phys_ops {
>   			struct dentry *debugfs_root);
>   	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
>   	bool (*is_master)(struct dpu_encoder_phys *encoder);
> -	bool (*mode_fixup)(struct dpu_encoder_phys *encoder,
> -			const struct drm_display_mode *mode,
> -			struct drm_display_mode *adjusted_mode);
>   	void (*mode_set)(struct dpu_encoder_phys *encoder,
>   			struct drm_display_mode *mode,
>   			struct drm_display_mode *adjusted_mode);
>   	void (*enable)(struct dpu_encoder_phys *encoder);
>   	void (*disable)(struct dpu_encoder_phys *encoder);
> -	int (*atomic_check)(struct dpu_encoder_phys *encoder,
> -			    struct drm_crtc_state *crtc_state,
> -			    struct drm_connector_state *conn_state);
>   	void (*destroy)(struct dpu_encoder_phys *encoder);
>   	void (*get_hw_resources)(struct dpu_encoder_phys *encoder,
>   				 struct dpu_encoder_hw_resources *hw_res);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index 34a6940d12c5..45fe97fb612d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -45,15 +45,6 @@ static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc)
>   	return (phys_enc->split_role != ENC_ROLE_SLAVE);
>   }
>   
> -static bool dpu_encoder_phys_cmd_mode_fixup(
> -		struct dpu_encoder_phys *phys_enc,
> -		const struct drm_display_mode *mode,
> -		struct drm_display_mode *adj_mode)
> -{
> -	DPU_DEBUG_CMDENC(to_dpu_encoder_phys_cmd(phys_enc), "\n");
> -	return true;
> -}
> -
>   static void _dpu_encoder_phys_cmd_update_intf_cfg(
>   		struct dpu_encoder_phys *phys_enc)
>   {
> @@ -732,7 +723,6 @@ static void dpu_encoder_phys_cmd_init_ops(
>   	ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
>   	ops->is_master = dpu_encoder_phys_cmd_is_master;
>   	ops->mode_set = dpu_encoder_phys_cmd_mode_set;
> -	ops->mode_fixup = dpu_encoder_phys_cmd_mode_fixup;
>   	ops->enable = dpu_encoder_phys_cmd_enable;
>   	ops->disable = dpu_encoder_phys_cmd_disable;
>   	ops->destroy = dpu_encoder_phys_cmd_destroy;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index ddd9d89cd456..1831fe37c88c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -225,19 +225,6 @@ static void programmable_fetch_config(struct dpu_encoder_phys *phys_enc,
>   	spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
>   }
>   
> -static bool dpu_encoder_phys_vid_mode_fixup(
> -		struct dpu_encoder_phys *phys_enc,
> -		const struct drm_display_mode *mode,
> -		struct drm_display_mode *adj_mode)
> -{
> -	DPU_DEBUG_VIDENC(phys_enc, "\n");
> -
> -	/*
> -	 * Modifying mode has consequences when the mode comes back to us
> -	 */
> -	return true;
> -}
> -
>   static void dpu_encoder_phys_vid_setup_timing_engine(
>   		struct dpu_encoder_phys *phys_enc)
>   {
> @@ -676,7 +663,6 @@ static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
>   {
>   	ops->is_master = dpu_encoder_phys_vid_is_master;
>   	ops->mode_set = dpu_encoder_phys_vid_mode_set;
> -	ops->mode_fixup = dpu_encoder_phys_vid_mode_fixup;
>   	ops->enable = dpu_encoder_phys_vid_enable;
>   	ops->disable = dpu_encoder_phys_vid_disable;
>   	ops->destroy = dpu_encoder_phys_vid_destroy;

Re: [PATCH 5/7] drm/msm/dpu: encoder: drop unused callbacks

[Dmitry Baryshkov]

On 12/02/2022 02:53, Abhinav Kumar wrote:
> 
> 
> On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
>> Both cmd and vid backends provide no atomic_check() callbacks and
>> useless mode_fixup() callbacks. Drop support for both of them.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> We can drop mode_fixup() but I am using atomic_check() for WB to 
> validate whether the programmed FB exceeds WB limits OR
> the programmed mode. Hence without an alternative for that we cannot 
> drop that

Ack, will change in v2, thanks for catching this!

Just as a remark: as I think more and more about the WB/CMD/VID, I have 
an impression that at some point we should turn drm_encoder inside out. 
Let drm_encoder_phys/vid/wb be a first class citizens and call helpers 
from dpu_encoder.c rather than making dpu_encoder.c calling out 
functions from respective backend. For now it's quick rough idea, but 
your thoughts are welcomed.

> 
> Please refer to dpu_encoder_phys_wb_atomic_check in 
> https://patchwork.freedesktop.org/patch/472324/
> 
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 39 +++++--------------
>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  8 ----
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 10 -----
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 14 -------
>>   4 files changed, 9 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 4530549850f0..e1c43a0c0643 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -573,7 +573,6 @@ static int dpu_encoder_virt_atomic_check(
>>       struct drm_display_mode *adj_mode;
>>       struct msm_display_topology topology;
>>       struct dpu_global_state *global_state;
>> -    int i = 0;
>>       int ret = 0;
>>       if (!drm_enc || !crtc_state || !conn_state) {
>> @@ -595,39 +594,19 @@ static int dpu_encoder_virt_atomic_check(
>>       trace_dpu_enc_atomic_check(DRMID(drm_enc));
>> -    /* perform atomic check on the first physical encoder (master) */
>> -    for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> -        struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> -
>> -        if (phys->ops.atomic_check)
>> -            ret = phys->ops.atomic_check(phys, crtc_state,
>> -                    conn_state);
>> -        else if (phys->ops.mode_fixup)
>> -            if (!phys->ops.mode_fixup(phys, mode, adj_mode))
>> -                ret = -EINVAL;
>> -
>> -        if (ret) {
>> -            DPU_ERROR_ENC(dpu_enc,
>> -                    "mode unsupported, phys idx %d\n", i);
>> -            break;
>> -        }
>> -    }
>> -
>>       topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
>>       /* Reserve dynamic resources now. */
>> -    if (!ret) {
>> -        /*
>> -         * Release and Allocate resources on every modeset
>> -         * Dont allocate when active is false.
>> -         */
>> -        if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>> -            dpu_rm_release(global_state, drm_enc);
>> +    /*
>> +     * Release and Allocate resources on every modeset
>> +     * Dont allocate when active is false.
>> +     */
>> +    if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>> +        dpu_rm_release(global_state, drm_enc);
>> -            if (!crtc_state->active_changed || crtc_state->active)
>> -                ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
>> -                        drm_enc, crtc_state, topology);
>> -        }
>> +        if (!crtc_state->active_changed || crtc_state->active)
>> +            ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
>> +                    drm_enc, crtc_state, topology);
>>       }
>>       trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> index e7270eb6b84b..159deb8ed7fb 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> @@ -84,12 +84,10 @@ struct dpu_encoder_virt_ops {
>>    * @is_master:            Whether this phys_enc is the current master
>>    *                encoder. Can be switched at enable time. Based
>>    *                on split_role and current mode (CMD/VID).
>> - * @mode_fixup:            DRM Call. Fixup a DRM mode.
>>    * @mode_set:            DRM Call. Set a DRM mode.
>>    *                This likely caches the mode, for use at enable.
>>    * @enable:            DRM Call. Enable a DRM mode.
>>    * @disable:            DRM Call. Disable mode.
>> - * @atomic_check:        DRM Call. Atomic check new DRM state.
>>    * @destroy:            DRM Call. Destroy and release resources.
>>    * @get_hw_resources:        Populate the structure with the hardware
>>    *                resources that this phys_enc is using.
>> @@ -117,17 +115,11 @@ struct dpu_encoder_phys_ops {
>>               struct dentry *debugfs_root);
>>       void (*prepare_commit)(struct dpu_encoder_phys *encoder);
>>       bool (*is_master)(struct dpu_encoder_phys *encoder);
>> -    bool (*mode_fixup)(struct dpu_encoder_phys *encoder,
>> -            const struct drm_display_mode *mode,
>> -            struct drm_display_mode *adjusted_mode);
>>       void (*mode_set)(struct dpu_encoder_phys *encoder,
>>               struct drm_display_mode *mode,
>>               struct drm_display_mode *adjusted_mode);
>>       void (*enable)(struct dpu_encoder_phys *encoder);
>>       void (*disable)(struct dpu_encoder_phys *encoder);
>> -    int (*atomic_check)(struct dpu_encoder_phys *encoder,
>> -                struct drm_crtc_state *crtc_state,
>> -                struct drm_connector_state *conn_state);
>>       void (*destroy)(struct dpu_encoder_phys *encoder);
>>       void (*get_hw_resources)(struct dpu_encoder_phys *encoder,
>>                    struct dpu_encoder_hw_resources *hw_res);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> index 34a6940d12c5..45fe97fb612d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> @@ -45,15 +45,6 @@ static bool dpu_encoder_phys_cmd_is_master(struct 
>> dpu_encoder_phys *phys_enc)
>>       return (phys_enc->split_role != ENC_ROLE_SLAVE);
>>   }
>> -static bool dpu_encoder_phys_cmd_mode_fixup(
>> -        struct dpu_encoder_phys *phys_enc,
>> -        const struct drm_display_mode *mode,
>> -        struct drm_display_mode *adj_mode)
>> -{
>> -    DPU_DEBUG_CMDENC(to_dpu_encoder_phys_cmd(phys_enc), "\n");
>> -    return true;
>> -}
>> -
>>   static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>           struct dpu_encoder_phys *phys_enc)
>>   {
>> @@ -732,7 +723,6 @@ static void dpu_encoder_phys_cmd_init_ops(
>>       ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
>>       ops->is_master = dpu_encoder_phys_cmd_is_master;
>>       ops->mode_set = dpu_encoder_phys_cmd_mode_set;
>> -    ops->mode_fixup = dpu_encoder_phys_cmd_mode_fixup;
>>       ops->enable = dpu_encoder_phys_cmd_enable;
>>       ops->disable = dpu_encoder_phys_cmd_disable;
>>       ops->destroy = dpu_encoder_phys_cmd_destroy;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> index ddd9d89cd456..1831fe37c88c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> @@ -225,19 +225,6 @@ static void programmable_fetch_config(struct 
>> dpu_encoder_phys *phys_enc,
>>       spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
>>   }
>> -static bool dpu_encoder_phys_vid_mode_fixup(
>> -        struct dpu_encoder_phys *phys_enc,
>> -        const struct drm_display_mode *mode,
>> -        struct drm_display_mode *adj_mode)
>> -{
>> -    DPU_DEBUG_VIDENC(phys_enc, "\n");
>> -
>> -    /*
>> -     * Modifying mode has consequences when the mode comes back to us
>> -     */
>> -    return true;
>> -}
>> -
>>   static void dpu_encoder_phys_vid_setup_timing_engine(
>>           struct dpu_encoder_phys *phys_enc)
>>   {
>> @@ -676,7 +663,6 @@ static void dpu_encoder_phys_vid_init_ops(struct 
>> dpu_encoder_phys_ops *ops)
>>   {
>>       ops->is_master = dpu_encoder_phys_vid_is_master;
>>       ops->mode_set = dpu_encoder_phys_vid_mode_set;
>> -    ops->mode_fixup = dpu_encoder_phys_vid_mode_fixup;
>>       ops->enable = dpu_encoder_phys_vid_enable;
>>       ops->disable = dpu_encoder_phys_vid_disable;
>>       ops->destroy = dpu_encoder_phys_vid_destroy;


-- 
With best wishes
Dmitry

Re: [PATCH 5/7] drm/msm/dpu: encoder: drop unused callbacks

[Dmitry Baryshkov]

On 12/02/2022 02:53, Abhinav Kumar wrote:
> 
> 
> On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
>> Both cmd and vid backends provide no atomic_check() callbacks and
>> useless mode_fixup() callbacks. Drop support for both of them.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> We can drop mode_fixup() but I am using atomic_check() for WB to 
> validate whether the programmed FB exceeds WB limits OR
> the programmed mode. Hence without an alternative for that we cannot 
> drop that

Ack, will change in v2, thanks for catching this!

Just as a remark: as I think more and more about the WB/CMD/VID, I have 
an impression that at some point we should turn drm_encoder inside out. 
Let drm_encoder_phys/vid/wb be a first class citizens and call helpers 
from dpu_encoder.c rather than making dpu_encoder.c calling out 
functions from respective backend. For now it's quick rough idea, but 
your thoughts are welcomed.

> 
> Please refer to dpu_encoder_phys_wb_atomic_check in 
> https://patchwork.freedesktop.org/patch/472324/
> 
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 39 +++++--------------
>>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  8 ----
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 10 -----
>>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 14 -------
>>   4 files changed, 9 insertions(+), 62 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 4530549850f0..e1c43a0c0643 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -573,7 +573,6 @@ static int dpu_encoder_virt_atomic_check(
>>       struct drm_display_mode *adj_mode;
>>       struct msm_display_topology topology;
>>       struct dpu_global_state *global_state;
>> -    int i = 0;
>>       int ret = 0;
>>       if (!drm_enc || !crtc_state || !conn_state) {
>> @@ -595,39 +594,19 @@ static int dpu_encoder_virt_atomic_check(
>>       trace_dpu_enc_atomic_check(DRMID(drm_enc));
>> -    /* perform atomic check on the first physical encoder (master) */
>> -    for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>> -        struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
>> -
>> -        if (phys->ops.atomic_check)
>> -            ret = phys->ops.atomic_check(phys, crtc_state,
>> -                    conn_state);
>> -        else if (phys->ops.mode_fixup)
>> -            if (!phys->ops.mode_fixup(phys, mode, adj_mode))
>> -                ret = -EINVAL;
>> -
>> -        if (ret) {
>> -            DPU_ERROR_ENC(dpu_enc,
>> -                    "mode unsupported, phys idx %d\n", i);
>> -            break;
>> -        }
>> -    }
>> -
>>       topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
>>       /* Reserve dynamic resources now. */
>> -    if (!ret) {
>> -        /*
>> -         * Release and Allocate resources on every modeset
>> -         * Dont allocate when active is false.
>> -         */
>> -        if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>> -            dpu_rm_release(global_state, drm_enc);
>> +    /*
>> +     * Release and Allocate resources on every modeset
>> +     * Dont allocate when active is false.
>> +     */
>> +    if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>> +        dpu_rm_release(global_state, drm_enc);
>> -            if (!crtc_state->active_changed || crtc_state->active)
>> -                ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
>> -                        drm_enc, crtc_state, topology);
>> -        }
>> +        if (!crtc_state->active_changed || crtc_state->active)
>> +            ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
>> +                    drm_enc, crtc_state, topology);
>>       }
>>       trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> index e7270eb6b84b..159deb8ed7fb 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
>> @@ -84,12 +84,10 @@ struct dpu_encoder_virt_ops {
>>    * @is_master:            Whether this phys_enc is the current master
>>    *                encoder. Can be switched at enable time. Based
>>    *                on split_role and current mode (CMD/VID).
>> - * @mode_fixup:            DRM Call. Fixup a DRM mode.
>>    * @mode_set:            DRM Call. Set a DRM mode.
>>    *                This likely caches the mode, for use at enable.
>>    * @enable:            DRM Call. Enable a DRM mode.
>>    * @disable:            DRM Call. Disable mode.
>> - * @atomic_check:        DRM Call. Atomic check new DRM state.
>>    * @destroy:            DRM Call. Destroy and release resources.
>>    * @get_hw_resources:        Populate the structure with the hardware
>>    *                resources that this phys_enc is using.
>> @@ -117,17 +115,11 @@ struct dpu_encoder_phys_ops {
>>               struct dentry *debugfs_root);
>>       void (*prepare_commit)(struct dpu_encoder_phys *encoder);
>>       bool (*is_master)(struct dpu_encoder_phys *encoder);
>> -    bool (*mode_fixup)(struct dpu_encoder_phys *encoder,
>> -            const struct drm_display_mode *mode,
>> -            struct drm_display_mode *adjusted_mode);
>>       void (*mode_set)(struct dpu_encoder_phys *encoder,
>>               struct drm_display_mode *mode,
>>               struct drm_display_mode *adjusted_mode);
>>       void (*enable)(struct dpu_encoder_phys *encoder);
>>       void (*disable)(struct dpu_encoder_phys *encoder);
>> -    int (*atomic_check)(struct dpu_encoder_phys *encoder,
>> -                struct drm_crtc_state *crtc_state,
>> -                struct drm_connector_state *conn_state);
>>       void (*destroy)(struct dpu_encoder_phys *encoder);
>>       void (*get_hw_resources)(struct dpu_encoder_phys *encoder,
>>                    struct dpu_encoder_hw_resources *hw_res);
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> index 34a6940d12c5..45fe97fb612d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
>> @@ -45,15 +45,6 @@ static bool dpu_encoder_phys_cmd_is_master(struct 
>> dpu_encoder_phys *phys_enc)
>>       return (phys_enc->split_role != ENC_ROLE_SLAVE);
>>   }
>> -static bool dpu_encoder_phys_cmd_mode_fixup(
>> -        struct dpu_encoder_phys *phys_enc,
>> -        const struct drm_display_mode *mode,
>> -        struct drm_display_mode *adj_mode)
>> -{
>> -    DPU_DEBUG_CMDENC(to_dpu_encoder_phys_cmd(phys_enc), "\n");
>> -    return true;
>> -}
>> -
>>   static void _dpu_encoder_phys_cmd_update_intf_cfg(
>>           struct dpu_encoder_phys *phys_enc)
>>   {
>> @@ -732,7 +723,6 @@ static void dpu_encoder_phys_cmd_init_ops(
>>       ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
>>       ops->is_master = dpu_encoder_phys_cmd_is_master;
>>       ops->mode_set = dpu_encoder_phys_cmd_mode_set;
>> -    ops->mode_fixup = dpu_encoder_phys_cmd_mode_fixup;
>>       ops->enable = dpu_encoder_phys_cmd_enable;
>>       ops->disable = dpu_encoder_phys_cmd_disable;
>>       ops->destroy = dpu_encoder_phys_cmd_destroy;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> index ddd9d89cd456..1831fe37c88c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>> @@ -225,19 +225,6 @@ static void programmable_fetch_config(struct 
>> dpu_encoder_phys *phys_enc,
>>       spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
>>   }
>> -static bool dpu_encoder_phys_vid_mode_fixup(
>> -        struct dpu_encoder_phys *phys_enc,
>> -        const struct drm_display_mode *mode,
>> -        struct drm_display_mode *adj_mode)
>> -{
>> -    DPU_DEBUG_VIDENC(phys_enc, "\n");
>> -
>> -    /*
>> -     * Modifying mode has consequences when the mode comes back to us
>> -     */
>> -    return true;
>> -}
>> -
>>   static void dpu_encoder_phys_vid_setup_timing_engine(
>>           struct dpu_encoder_phys *phys_enc)
>>   {
>> @@ -676,7 +663,6 @@ static void dpu_encoder_phys_vid_init_ops(struct 
>> dpu_encoder_phys_ops *ops)
>>   {
>>       ops->is_master = dpu_encoder_phys_vid_is_master;
>>       ops->mode_set = dpu_encoder_phys_vid_mode_set;
>> -    ops->mode_fixup = dpu_encoder_phys_vid_mode_fixup;
>>       ops->enable = dpu_encoder_phys_vid_enable;
>>       ops->disable = dpu_encoder_phys_vid_disable;
>>       ops->destroy = dpu_encoder_phys_vid_destroy;


-- 
With best wishes
Dmitry

[PATCH 6/7] drm/msm/dpu: switch dpu_encoder to use atomic_mode_set

[Dmitry Baryshkov]

Make dpu_encoder use atomic_mode_set to receive connector and CRTC
states as arguments rather than finding connector and CRTC by manually
looping through the respective lists.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 37 +++++--------------
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  8 ++--
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 18 ++-------
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 14 ++-----
 4 files changed, 21 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index e1c43a0c0643..1462c426c14c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -930,16 +930,13 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
 	return 0;
 }
 
-static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
-				      struct drm_display_mode *mode,
-				      struct drm_display_mode *adj_mode)
+static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
+					     struct drm_crtc_state *crtc_state,
+					     struct drm_connector_state *conn_state)
 {
 	struct dpu_encoder_virt *dpu_enc;
 	struct msm_drm_private *priv;
 	struct dpu_kms *dpu_kms;
-	struct list_head *connector_list;
-	struct drm_connector *conn = NULL, *conn_iter;
-	struct drm_crtc *drm_crtc;
 	struct dpu_crtc_state *cstate;
 	struct dpu_global_state *global_state;
 	struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC];
@@ -959,7 +956,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 
 	priv = drm_enc->dev->dev_private;
 	dpu_kms = to_dpu_kms(priv->kms);
-	connector_list = &dpu_kms->dev->mode_config.connector_list;
 
 	global_state = dpu_kms_get_existing_global_state(dpu_kms);
 	if (IS_ERR_OR_NULL(global_state)) {
@@ -969,22 +965,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 
 	trace_dpu_enc_mode_set(DRMID(drm_enc));
 
-	list_for_each_entry(conn_iter, connector_list, head)
-		if (conn_iter->encoder == drm_enc)
-			conn = conn_iter;
-
-	if (!conn) {
-		DPU_ERROR_ENC(dpu_enc, "failed to find attached connector\n");
-		return;
-	} else if (!conn->state) {
-		DPU_ERROR_ENC(dpu_enc, "invalid connector state\n");
-		return;
-	}
-
-	drm_for_each_crtc(drm_crtc, drm_enc->dev)
-		if (drm_crtc->state->encoder_mask & drm_encoder_mask(drm_enc))
-			break;
-
 	/* Query resource that have been reserved in atomic check step. */
 	num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
 		drm_enc->base.id, DPU_HW_BLK_PINGPONG, hw_pp,
@@ -1001,7 +981,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 		dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i])
 						: NULL;
 
-	cstate = to_dpu_crtc_state(drm_crtc->state);
+	cstate = to_dpu_crtc_state(crtc_state);
 
 	for (i = 0; i < num_lm; i++) {
 		int ctl_idx = (i < num_ctl) ? i : (num_ctl-1);
@@ -1050,9 +1030,10 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 			return;
 		}
 
-		phys->connector = conn->state->connector;
-		if (phys->ops.mode_set)
-			phys->ops.mode_set(phys, mode, adj_mode);
+		phys->connector = conn_state->connector;
+		phys->cached_mode = crtc_state->adjusted_mode;
+		if (phys->ops.atomic_mode_set)
+			phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
 	}
 }
 
@@ -2057,7 +2038,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
 }
 
 static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
-	.mode_set = dpu_encoder_virt_mode_set,
+	.atomic_mode_set = dpu_encoder_virt_atomic_mode_set,
 	.disable = dpu_encoder_virt_disable,
 	.enable = dpu_encoder_virt_enable,
 	.atomic_check = dpu_encoder_virt_atomic_check,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 159deb8ed7fb..6e80321b13c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -84,7 +84,7 @@ struct dpu_encoder_virt_ops {
  * @is_master:			Whether this phys_enc is the current master
  *				encoder. Can be switched at enable time. Based
  *				on split_role and current mode (CMD/VID).
- * @mode_set:			DRM Call. Set a DRM mode.
+ * @atomic_mode_set:		DRM Call. Set a DRM mode.
  *				This likely caches the mode, for use at enable.
  * @enable:			DRM Call. Enable a DRM mode.
  * @disable:			DRM Call. Disable mode.
@@ -115,9 +115,9 @@ struct dpu_encoder_phys_ops {
 			struct dentry *debugfs_root);
 	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
 	bool (*is_master)(struct dpu_encoder_phys *encoder);
-	void (*mode_set)(struct dpu_encoder_phys *encoder,
-			struct drm_display_mode *mode,
-			struct drm_display_mode *adjusted_mode);
+	void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
+			struct drm_crtc_state *crtc_state,
+			struct drm_connector_state *conn_state);
 	void (*enable)(struct dpu_encoder_phys *encoder);
 	void (*disable)(struct dpu_encoder_phys *encoder);
 	void (*destroy)(struct dpu_encoder_phys *encoder);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 45fe97fb612d..6de298d521ce 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -135,23 +135,13 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg, int irq_idx)
 			phys_enc);
 }
 
-static void dpu_encoder_phys_cmd_mode_set(
+static void dpu_encoder_phys_cmd_atomic_mode_set(
 		struct dpu_encoder_phys *phys_enc,
-		struct drm_display_mode *mode,
-		struct drm_display_mode *adj_mode)
+		struct drm_crtc_state *crtc_state,
+		struct drm_connector_state *conn_state)
 {
-	struct dpu_encoder_phys_cmd *cmd_enc =
-		to_dpu_encoder_phys_cmd(phys_enc);
 	struct dpu_encoder_irq *irq;
 
-	if (!mode || !adj_mode) {
-		DPU_ERROR("invalid args\n");
-		return;
-	}
-	phys_enc->cached_mode = *adj_mode;
-	DPU_DEBUG_CMDENC(cmd_enc, "caching mode:\n");
-	drm_mode_debug_printmodeline(adj_mode);
-
 	irq = &phys_enc->irq[INTR_IDX_CTL_START];
 	irq->irq_idx = phys_enc->hw_ctl->caps->intr_start;
 
@@ -722,7 +712,7 @@ static void dpu_encoder_phys_cmd_init_ops(
 {
 	ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
 	ops->is_master = dpu_encoder_phys_cmd_is_master;
-	ops->mode_set = dpu_encoder_phys_cmd_mode_set;
+	ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
 	ops->enable = dpu_encoder_phys_cmd_enable;
 	ops->disable = dpu_encoder_phys_cmd_disable;
 	ops->destroy = dpu_encoder_phys_cmd_destroy;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 1831fe37c88c..0c07db5021eb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -348,19 +348,13 @@ static bool dpu_encoder_phys_vid_needs_single_flush(
 	return phys_enc->split_role != ENC_ROLE_SOLO;
 }
 
-static void dpu_encoder_phys_vid_mode_set(
+static void dpu_encoder_phys_vid_atomic_mode_set(
 		struct dpu_encoder_phys *phys_enc,
-		struct drm_display_mode *mode,
-		struct drm_display_mode *adj_mode)
+		struct drm_crtc_state *crtc_state,
+		struct drm_connector_state *conn_state)
 {
 	struct dpu_encoder_irq *irq;
 
-	if (adj_mode) {
-		phys_enc->cached_mode = *adj_mode;
-		drm_mode_debug_printmodeline(adj_mode);
-		DPU_DEBUG_VIDENC(phys_enc, "caching mode:\n");
-	}
-
 	irq = &phys_enc->irq[INTR_IDX_VSYNC];
 	irq->irq_idx = phys_enc->hw_intf->cap->intr_vsync;
 
@@ -662,7 +656,7 @@ static int dpu_encoder_phys_vid_get_frame_count(
 static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
 {
 	ops->is_master = dpu_encoder_phys_vid_is_master;
-	ops->mode_set = dpu_encoder_phys_vid_mode_set;
+	ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set;
 	ops->enable = dpu_encoder_phys_vid_enable;
 	ops->disable = dpu_encoder_phys_vid_disable;
 	ops->destroy = dpu_encoder_phys_vid_destroy;
-- 
2.34.1

[PATCH 6/7] drm/msm/dpu: switch dpu_encoder to use atomic_mode_set

[Dmitry Baryshkov]

Make dpu_encoder use atomic_mode_set to receive connector and CRTC
states as arguments rather than finding connector and CRTC by manually
looping through the respective lists.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 37 +++++--------------
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  8 ++--
 .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 18 ++-------
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 14 ++-----
 4 files changed, 21 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index e1c43a0c0643..1462c426c14c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -930,16 +930,13 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
 	return 0;
 }
 
-static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
-				      struct drm_display_mode *mode,
-				      struct drm_display_mode *adj_mode)
+static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
+					     struct drm_crtc_state *crtc_state,
+					     struct drm_connector_state *conn_state)
 {
 	struct dpu_encoder_virt *dpu_enc;
 	struct msm_drm_private *priv;
 	struct dpu_kms *dpu_kms;
-	struct list_head *connector_list;
-	struct drm_connector *conn = NULL, *conn_iter;
-	struct drm_crtc *drm_crtc;
 	struct dpu_crtc_state *cstate;
 	struct dpu_global_state *global_state;
 	struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC];
@@ -959,7 +956,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 
 	priv = drm_enc->dev->dev_private;
 	dpu_kms = to_dpu_kms(priv->kms);
-	connector_list = &dpu_kms->dev->mode_config.connector_list;
 
 	global_state = dpu_kms_get_existing_global_state(dpu_kms);
 	if (IS_ERR_OR_NULL(global_state)) {
@@ -969,22 +965,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 
 	trace_dpu_enc_mode_set(DRMID(drm_enc));
 
-	list_for_each_entry(conn_iter, connector_list, head)
-		if (conn_iter->encoder == drm_enc)
-			conn = conn_iter;
-
-	if (!conn) {
-		DPU_ERROR_ENC(dpu_enc, "failed to find attached connector\n");
-		return;
-	} else if (!conn->state) {
-		DPU_ERROR_ENC(dpu_enc, "invalid connector state\n");
-		return;
-	}
-
-	drm_for_each_crtc(drm_crtc, drm_enc->dev)
-		if (drm_crtc->state->encoder_mask & drm_encoder_mask(drm_enc))
-			break;
-
 	/* Query resource that have been reserved in atomic check step. */
 	num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
 		drm_enc->base.id, DPU_HW_BLK_PINGPONG, hw_pp,
@@ -1001,7 +981,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 		dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i])
 						: NULL;
 
-	cstate = to_dpu_crtc_state(drm_crtc->state);
+	cstate = to_dpu_crtc_state(crtc_state);
 
 	for (i = 0; i < num_lm; i++) {
 		int ctl_idx = (i < num_ctl) ? i : (num_ctl-1);
@@ -1050,9 +1030,10 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
 			return;
 		}
 
-		phys->connector = conn->state->connector;
-		if (phys->ops.mode_set)
-			phys->ops.mode_set(phys, mode, adj_mode);
+		phys->connector = conn_state->connector;
+		phys->cached_mode = crtc_state->adjusted_mode;
+		if (phys->ops.atomic_mode_set)
+			phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
 	}
 }
 
@@ -2057,7 +2038,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
 }
 
 static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
-	.mode_set = dpu_encoder_virt_mode_set,
+	.atomic_mode_set = dpu_encoder_virt_atomic_mode_set,
 	.disable = dpu_encoder_virt_disable,
 	.enable = dpu_encoder_virt_enable,
 	.atomic_check = dpu_encoder_virt_atomic_check,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 159deb8ed7fb..6e80321b13c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -84,7 +84,7 @@ struct dpu_encoder_virt_ops {
  * @is_master:			Whether this phys_enc is the current master
  *				encoder. Can be switched at enable time. Based
  *				on split_role and current mode (CMD/VID).
- * @mode_set:			DRM Call. Set a DRM mode.
+ * @atomic_mode_set:		DRM Call. Set a DRM mode.
  *				This likely caches the mode, for use at enable.
  * @enable:			DRM Call. Enable a DRM mode.
  * @disable:			DRM Call. Disable mode.
@@ -115,9 +115,9 @@ struct dpu_encoder_phys_ops {
 			struct dentry *debugfs_root);
 	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
 	bool (*is_master)(struct dpu_encoder_phys *encoder);
-	void (*mode_set)(struct dpu_encoder_phys *encoder,
-			struct drm_display_mode *mode,
-			struct drm_display_mode *adjusted_mode);
+	void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
+			struct drm_crtc_state *crtc_state,
+			struct drm_connector_state *conn_state);
 	void (*enable)(struct dpu_encoder_phys *encoder);
 	void (*disable)(struct dpu_encoder_phys *encoder);
 	void (*destroy)(struct dpu_encoder_phys *encoder);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 45fe97fb612d..6de298d521ce 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -135,23 +135,13 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg, int irq_idx)
 			phys_enc);
 }
 
-static void dpu_encoder_phys_cmd_mode_set(
+static void dpu_encoder_phys_cmd_atomic_mode_set(
 		struct dpu_encoder_phys *phys_enc,
-		struct drm_display_mode *mode,
-		struct drm_display_mode *adj_mode)
+		struct drm_crtc_state *crtc_state,
+		struct drm_connector_state *conn_state)
 {
-	struct dpu_encoder_phys_cmd *cmd_enc =
-		to_dpu_encoder_phys_cmd(phys_enc);
 	struct dpu_encoder_irq *irq;
 
-	if (!mode || !adj_mode) {
-		DPU_ERROR("invalid args\n");
-		return;
-	}
-	phys_enc->cached_mode = *adj_mode;
-	DPU_DEBUG_CMDENC(cmd_enc, "caching mode:\n");
-	drm_mode_debug_printmodeline(adj_mode);
-
 	irq = &phys_enc->irq[INTR_IDX_CTL_START];
 	irq->irq_idx = phys_enc->hw_ctl->caps->intr_start;
 
@@ -722,7 +712,7 @@ static void dpu_encoder_phys_cmd_init_ops(
 {
 	ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
 	ops->is_master = dpu_encoder_phys_cmd_is_master;
-	ops->mode_set = dpu_encoder_phys_cmd_mode_set;
+	ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
 	ops->enable = dpu_encoder_phys_cmd_enable;
 	ops->disable = dpu_encoder_phys_cmd_disable;
 	ops->destroy = dpu_encoder_phys_cmd_destroy;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 1831fe37c88c..0c07db5021eb 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -348,19 +348,13 @@ static bool dpu_encoder_phys_vid_needs_single_flush(
 	return phys_enc->split_role != ENC_ROLE_SOLO;
 }
 
-static void dpu_encoder_phys_vid_mode_set(
+static void dpu_encoder_phys_vid_atomic_mode_set(
 		struct dpu_encoder_phys *phys_enc,
-		struct drm_display_mode *mode,
-		struct drm_display_mode *adj_mode)
+		struct drm_crtc_state *crtc_state,
+		struct drm_connector_state *conn_state)
 {
 	struct dpu_encoder_irq *irq;
 
-	if (adj_mode) {
-		phys_enc->cached_mode = *adj_mode;
-		drm_mode_debug_printmodeline(adj_mode);
-		DPU_DEBUG_VIDENC(phys_enc, "caching mode:\n");
-	}
-
 	irq = &phys_enc->irq[INTR_IDX_VSYNC];
 	irq->irq_idx = phys_enc->hw_intf->cap->intr_vsync;
 
@@ -662,7 +656,7 @@ static int dpu_encoder_phys_vid_get_frame_count(
 static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
 {
 	ops->is_master = dpu_encoder_phys_vid_is_master;
-	ops->mode_set = dpu_encoder_phys_vid_mode_set;
+	ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set;
 	ops->enable = dpu_encoder_phys_vid_enable;
 	ops->disable = dpu_encoder_phys_vid_disable;
 	ops->destroy = dpu_encoder_phys_vid_destroy;
-- 
2.34.1

Re: [PATCH 6/7] drm/msm/dpu: switch dpu_encoder to use atomic_mode_set

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> Make dpu_encoder use atomic_mode_set to receive connector and CRTC
> states as arguments rather than finding connector and CRTC by manually
> looping through the respective lists.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Nice cleanup !!

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 37 +++++--------------
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  8 ++--
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 18 ++-------
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 14 ++-----
>   4 files changed, 21 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index e1c43a0c0643..1462c426c14c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -930,16 +930,13 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
>   	return 0;
>   }
>   
> -static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
> -				      struct drm_display_mode *mode,
> -				      struct drm_display_mode *adj_mode)
> +static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> +					     struct drm_crtc_state *crtc_state,
> +					     struct drm_connector_state *conn_state)
>   {
>   	struct dpu_encoder_virt *dpu_enc;
>   	struct msm_drm_private *priv;
>   	struct dpu_kms *dpu_kms;
> -	struct list_head *connector_list;
> -	struct drm_connector *conn = NULL, *conn_iter;
> -	struct drm_crtc *drm_crtc;
>   	struct dpu_crtc_state *cstate;
>   	struct dpu_global_state *global_state;
>   	struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC];
> @@ -959,7 +956,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>   
>   	priv = drm_enc->dev->dev_private;
>   	dpu_kms = to_dpu_kms(priv->kms);
> -	connector_list = &dpu_kms->dev->mode_config.connector_list;
>   
>   	global_state = dpu_kms_get_existing_global_state(dpu_kms);
>   	if (IS_ERR_OR_NULL(global_state)) {
> @@ -969,22 +965,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>   
>   	trace_dpu_enc_mode_set(DRMID(drm_enc));
>   
> -	list_for_each_entry(conn_iter, connector_list, head)
> -		if (conn_iter->encoder == drm_enc)
> -			conn = conn_iter;
> -
> -	if (!conn) {
> -		DPU_ERROR_ENC(dpu_enc, "failed to find attached connector\n");
> -		return;
> -	} else if (!conn->state) {
> -		DPU_ERROR_ENC(dpu_enc, "invalid connector state\n");
> -		return;
> -	}
> -
> -	drm_for_each_crtc(drm_crtc, drm_enc->dev)
> -		if (drm_crtc->state->encoder_mask & drm_encoder_mask(drm_enc))
> -			break;
> -
>   	/* Query resource that have been reserved in atomic check step. */
>   	num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
>   		drm_enc->base.id, DPU_HW_BLK_PINGPONG, hw_pp,
> @@ -1001,7 +981,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>   		dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i])
>   						: NULL;
>   
> -	cstate = to_dpu_crtc_state(drm_crtc->state);
> +	cstate = to_dpu_crtc_state(crtc_state);
>   
>   	for (i = 0; i < num_lm; i++) {
>   		int ctl_idx = (i < num_ctl) ? i : (num_ctl-1);
> @@ -1050,9 +1030,10 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>   			return;
>   		}
>   
> -		phys->connector = conn->state->connector;
> -		if (phys->ops.mode_set)
> -			phys->ops.mode_set(phys, mode, adj_mode);
> +		phys->connector = conn_state->connector;
> +		phys->cached_mode = crtc_state->adjusted_mode;
> +		if (phys->ops.atomic_mode_set)
> +			phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
>   	}
>   }
>   
> @@ -2057,7 +2038,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
>   }
>   
>   static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
> -	.mode_set = dpu_encoder_virt_mode_set,
> +	.atomic_mode_set = dpu_encoder_virt_atomic_mode_set,
>   	.disable = dpu_encoder_virt_disable,
>   	.enable = dpu_encoder_virt_enable,
>   	.atomic_check = dpu_encoder_virt_atomic_check,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index 159deb8ed7fb..6e80321b13c5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -84,7 +84,7 @@ struct dpu_encoder_virt_ops {
>    * @is_master:			Whether this phys_enc is the current master
>    *				encoder. Can be switched at enable time. Based
>    *				on split_role and current mode (CMD/VID).
> - * @mode_set:			DRM Call. Set a DRM mode.
> + * @atomic_mode_set:		DRM Call. Set a DRM mode.
>    *				This likely caches the mode, for use at enable.
>    * @enable:			DRM Call. Enable a DRM mode.
>    * @disable:			DRM Call. Disable mode.
> @@ -115,9 +115,9 @@ struct dpu_encoder_phys_ops {
>   			struct dentry *debugfs_root);
>   	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
>   	bool (*is_master)(struct dpu_encoder_phys *encoder);
> -	void (*mode_set)(struct dpu_encoder_phys *encoder,
> -			struct drm_display_mode *mode,
> -			struct drm_display_mode *adjusted_mode);
> +	void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
> +			struct drm_crtc_state *crtc_state,
> +			struct drm_connector_state *conn_state);
>   	void (*enable)(struct dpu_encoder_phys *encoder);
>   	void (*disable)(struct dpu_encoder_phys *encoder);
>   	void (*destroy)(struct dpu_encoder_phys *encoder);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index 45fe97fb612d..6de298d521ce 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -135,23 +135,13 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg, int irq_idx)
>   			phys_enc);
>   }
>   
> -static void dpu_encoder_phys_cmd_mode_set(
> +static void dpu_encoder_phys_cmd_atomic_mode_set(
>   		struct dpu_encoder_phys *phys_enc,
> -		struct drm_display_mode *mode,
> -		struct drm_display_mode *adj_mode)
> +		struct drm_crtc_state *crtc_state,
> +		struct drm_connector_state *conn_state)
>   {
> -	struct dpu_encoder_phys_cmd *cmd_enc =
> -		to_dpu_encoder_phys_cmd(phys_enc);
>   	struct dpu_encoder_irq *irq;
>   
> -	if (!mode || !adj_mode) {
> -		DPU_ERROR("invalid args\n");
> -		return;
> -	}
> -	phys_enc->cached_mode = *adj_mode;
> -	DPU_DEBUG_CMDENC(cmd_enc, "caching mode:\n");
> -	drm_mode_debug_printmodeline(adj_mode);
> -
>   	irq = &phys_enc->irq[INTR_IDX_CTL_START];
>   	irq->irq_idx = phys_enc->hw_ctl->caps->intr_start;
>   
> @@ -722,7 +712,7 @@ static void dpu_encoder_phys_cmd_init_ops(
>   {
>   	ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
>   	ops->is_master = dpu_encoder_phys_cmd_is_master;
> -	ops->mode_set = dpu_encoder_phys_cmd_mode_set;
> +	ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
>   	ops->enable = dpu_encoder_phys_cmd_enable;
>   	ops->disable = dpu_encoder_phys_cmd_disable;
>   	ops->destroy = dpu_encoder_phys_cmd_destroy;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index 1831fe37c88c..0c07db5021eb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -348,19 +348,13 @@ static bool dpu_encoder_phys_vid_needs_single_flush(
>   	return phys_enc->split_role != ENC_ROLE_SOLO;
>   }
>   
> -static void dpu_encoder_phys_vid_mode_set(
> +static void dpu_encoder_phys_vid_atomic_mode_set(
>   		struct dpu_encoder_phys *phys_enc,
> -		struct drm_display_mode *mode,
> -		struct drm_display_mode *adj_mode)
> +		struct drm_crtc_state *crtc_state,
> +		struct drm_connector_state *conn_state)
>   {
>   	struct dpu_encoder_irq *irq;
>   
> -	if (adj_mode) {
> -		phys_enc->cached_mode = *adj_mode;
> -		drm_mode_debug_printmodeline(adj_mode);
> -		DPU_DEBUG_VIDENC(phys_enc, "caching mode:\n");
> -	}
> -
>   	irq = &phys_enc->irq[INTR_IDX_VSYNC];
>   	irq->irq_idx = phys_enc->hw_intf->cap->intr_vsync;
>   
> @@ -662,7 +656,7 @@ static int dpu_encoder_phys_vid_get_frame_count(
>   static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
>   {
>   	ops->is_master = dpu_encoder_phys_vid_is_master;
> -	ops->mode_set = dpu_encoder_phys_vid_mode_set;
> +	ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set;
>   	ops->enable = dpu_encoder_phys_vid_enable;
>   	ops->disable = dpu_encoder_phys_vid_disable;
>   	ops->destroy = dpu_encoder_phys_vid_destroy;

Re: [PATCH 6/7] drm/msm/dpu: switch dpu_encoder to use atomic_mode_set

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> Make dpu_encoder use atomic_mode_set to receive connector and CRTC
> states as arguments rather than finding connector and CRTC by manually
> looping through the respective lists.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Nice cleanup !!

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   | 37 +++++--------------
>   .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h  |  8 ++--
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c  | 18 ++-------
>   .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 14 ++-----
>   4 files changed, 21 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index e1c43a0c0643..1462c426c14c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -930,16 +930,13 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,
>   	return 0;
>   }
>   
> -static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
> -				      struct drm_display_mode *mode,
> -				      struct drm_display_mode *adj_mode)
> +static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
> +					     struct drm_crtc_state *crtc_state,
> +					     struct drm_connector_state *conn_state)
>   {
>   	struct dpu_encoder_virt *dpu_enc;
>   	struct msm_drm_private *priv;
>   	struct dpu_kms *dpu_kms;
> -	struct list_head *connector_list;
> -	struct drm_connector *conn = NULL, *conn_iter;
> -	struct drm_crtc *drm_crtc;
>   	struct dpu_crtc_state *cstate;
>   	struct dpu_global_state *global_state;
>   	struct dpu_hw_blk *hw_pp[MAX_CHANNELS_PER_ENC];
> @@ -959,7 +956,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>   
>   	priv = drm_enc->dev->dev_private;
>   	dpu_kms = to_dpu_kms(priv->kms);
> -	connector_list = &dpu_kms->dev->mode_config.connector_list;
>   
>   	global_state = dpu_kms_get_existing_global_state(dpu_kms);
>   	if (IS_ERR_OR_NULL(global_state)) {
> @@ -969,22 +965,6 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>   
>   	trace_dpu_enc_mode_set(DRMID(drm_enc));
>   
> -	list_for_each_entry(conn_iter, connector_list, head)
> -		if (conn_iter->encoder == drm_enc)
> -			conn = conn_iter;
> -
> -	if (!conn) {
> -		DPU_ERROR_ENC(dpu_enc, "failed to find attached connector\n");
> -		return;
> -	} else if (!conn->state) {
> -		DPU_ERROR_ENC(dpu_enc, "invalid connector state\n");
> -		return;
> -	}
> -
> -	drm_for_each_crtc(drm_crtc, drm_enc->dev)
> -		if (drm_crtc->state->encoder_mask & drm_encoder_mask(drm_enc))
> -			break;
> -
>   	/* Query resource that have been reserved in atomic check step. */
>   	num_pp = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
>   		drm_enc->base.id, DPU_HW_BLK_PINGPONG, hw_pp,
> @@ -1001,7 +981,7 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>   		dpu_enc->hw_pp[i] = i < num_pp ? to_dpu_hw_pingpong(hw_pp[i])
>   						: NULL;
>   
> -	cstate = to_dpu_crtc_state(drm_crtc->state);
> +	cstate = to_dpu_crtc_state(crtc_state);
>   
>   	for (i = 0; i < num_lm; i++) {
>   		int ctl_idx = (i < num_ctl) ? i : (num_ctl-1);
> @@ -1050,9 +1030,10 @@ static void dpu_encoder_virt_mode_set(struct drm_encoder *drm_enc,
>   			return;
>   		}
>   
> -		phys->connector = conn->state->connector;
> -		if (phys->ops.mode_set)
> -			phys->ops.mode_set(phys, mode, adj_mode);
> +		phys->connector = conn_state->connector;
> +		phys->cached_mode = crtc_state->adjusted_mode;
> +		if (phys->ops.atomic_mode_set)
> +			phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
>   	}
>   }
>   
> @@ -2057,7 +2038,7 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
>   }
>   
>   static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
> -	.mode_set = dpu_encoder_virt_mode_set,
> +	.atomic_mode_set = dpu_encoder_virt_atomic_mode_set,
>   	.disable = dpu_encoder_virt_disable,
>   	.enable = dpu_encoder_virt_enable,
>   	.atomic_check = dpu_encoder_virt_atomic_check,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index 159deb8ed7fb..6e80321b13c5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -84,7 +84,7 @@ struct dpu_encoder_virt_ops {
>    * @is_master:			Whether this phys_enc is the current master
>    *				encoder. Can be switched at enable time. Based
>    *				on split_role and current mode (CMD/VID).
> - * @mode_set:			DRM Call. Set a DRM mode.
> + * @atomic_mode_set:		DRM Call. Set a DRM mode.
>    *				This likely caches the mode, for use at enable.
>    * @enable:			DRM Call. Enable a DRM mode.
>    * @disable:			DRM Call. Disable mode.
> @@ -115,9 +115,9 @@ struct dpu_encoder_phys_ops {
>   			struct dentry *debugfs_root);
>   	void (*prepare_commit)(struct dpu_encoder_phys *encoder);
>   	bool (*is_master)(struct dpu_encoder_phys *encoder);
> -	void (*mode_set)(struct dpu_encoder_phys *encoder,
> -			struct drm_display_mode *mode,
> -			struct drm_display_mode *adjusted_mode);
> +	void (*atomic_mode_set)(struct dpu_encoder_phys *encoder,
> +			struct drm_crtc_state *crtc_state,
> +			struct drm_connector_state *conn_state);
>   	void (*enable)(struct dpu_encoder_phys *encoder);
>   	void (*disable)(struct dpu_encoder_phys *encoder);
>   	void (*destroy)(struct dpu_encoder_phys *encoder);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index 45fe97fb612d..6de298d521ce 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -135,23 +135,13 @@ static void dpu_encoder_phys_cmd_underrun_irq(void *arg, int irq_idx)
>   			phys_enc);
>   }
>   
> -static void dpu_encoder_phys_cmd_mode_set(
> +static void dpu_encoder_phys_cmd_atomic_mode_set(
>   		struct dpu_encoder_phys *phys_enc,
> -		struct drm_display_mode *mode,
> -		struct drm_display_mode *adj_mode)
> +		struct drm_crtc_state *crtc_state,
> +		struct drm_connector_state *conn_state)
>   {
> -	struct dpu_encoder_phys_cmd *cmd_enc =
> -		to_dpu_encoder_phys_cmd(phys_enc);
>   	struct dpu_encoder_irq *irq;
>   
> -	if (!mode || !adj_mode) {
> -		DPU_ERROR("invalid args\n");
> -		return;
> -	}
> -	phys_enc->cached_mode = *adj_mode;
> -	DPU_DEBUG_CMDENC(cmd_enc, "caching mode:\n");
> -	drm_mode_debug_printmodeline(adj_mode);
> -
>   	irq = &phys_enc->irq[INTR_IDX_CTL_START];
>   	irq->irq_idx = phys_enc->hw_ctl->caps->intr_start;
>   
> @@ -722,7 +712,7 @@ static void dpu_encoder_phys_cmd_init_ops(
>   {
>   	ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit;
>   	ops->is_master = dpu_encoder_phys_cmd_is_master;
> -	ops->mode_set = dpu_encoder_phys_cmd_mode_set;
> +	ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set;
>   	ops->enable = dpu_encoder_phys_cmd_enable;
>   	ops->disable = dpu_encoder_phys_cmd_disable;
>   	ops->destroy = dpu_encoder_phys_cmd_destroy;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> index 1831fe37c88c..0c07db5021eb 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
> @@ -348,19 +348,13 @@ static bool dpu_encoder_phys_vid_needs_single_flush(
>   	return phys_enc->split_role != ENC_ROLE_SOLO;
>   }
>   
> -static void dpu_encoder_phys_vid_mode_set(
> +static void dpu_encoder_phys_vid_atomic_mode_set(
>   		struct dpu_encoder_phys *phys_enc,
> -		struct drm_display_mode *mode,
> -		struct drm_display_mode *adj_mode)
> +		struct drm_crtc_state *crtc_state,
> +		struct drm_connector_state *conn_state)
>   {
>   	struct dpu_encoder_irq *irq;
>   
> -	if (adj_mode) {
> -		phys_enc->cached_mode = *adj_mode;
> -		drm_mode_debug_printmodeline(adj_mode);
> -		DPU_DEBUG_VIDENC(phys_enc, "caching mode:\n");
> -	}
> -
>   	irq = &phys_enc->irq[INTR_IDX_VSYNC];
>   	irq->irq_idx = phys_enc->hw_intf->cap->intr_vsync;
>   
> @@ -662,7 +656,7 @@ static int dpu_encoder_phys_vid_get_frame_count(
>   static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
>   {
>   	ops->is_master = dpu_encoder_phys_vid_is_master;
> -	ops->mode_set = dpu_encoder_phys_vid_mode_set;
> +	ops->atomic_mode_set = dpu_encoder_phys_vid_atomic_mode_set;
>   	ops->enable = dpu_encoder_phys_vid_enable;
>   	ops->disable = dpu_encoder_phys_vid_disable;
>   	ops->destroy = dpu_encoder_phys_vid_destroy;

[PATCH 7/7] drm/msm/dpu: pull connector from dpu_encoder_phys to dpu_encoder_virt

[Dmitry Baryshkov]

All physical encoders used by virtual encoder share the same connector,
so pull the connector field from dpu_encoder_phys into dpu_encoder_virt
structure.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 11 ++++++-----
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  2 --
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1462c426c14c..afafdaf48aea 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -143,6 +143,7 @@ enum dpu_enc_rc_states {
  *			link between encoder/crtc. However in this case we need
  *			to track crtc in the disable() hook which is called
  *			_after_ encoder_mask is cleared.
+ * @connector:		If a mode is set, cached pointer to the active connector
  * @crtc_kickoff_cb:		Callback into CRTC that will flush & start
  *				all CTL paths
  * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
@@ -183,6 +184,7 @@ struct dpu_encoder_virt {
 	bool intfs_swapped;
 
 	struct drm_crtc *crtc;
+	struct drm_connector *connector;
 
 	struct dentry *debugfs_root;
 	struct mutex enc_lock;
@@ -993,6 +995,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 
 	cstate->num_mixers = num_lm;
 
+	dpu_enc->connector = conn_state->connector;
+
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
 		int num_blk;
 		struct dpu_hw_blk *hw_blk[MAX_CHANNELS_PER_ENC];
@@ -1030,7 +1034,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 			return;
 		}
 
-		phys->connector = conn_state->connector;
 		phys->cached_mode = crtc_state->adjusted_mode;
 		if (phys->ops.atomic_mode_set)
 			phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
@@ -1064,7 +1067,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
 
 	if (dpu_enc->disp_info.intf_type == INTF_DSI &&
 			!WARN_ON(dpu_enc->num_phys_encs == 0)) {
-		unsigned bpc = dpu_enc->phys_encs[0]->connector->display_info.bpc;
+		unsigned bpc = dpu_enc->connector->display_info.bpc;
 		for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
 			if (!dpu_enc->hw_pp[i])
 				continue;
@@ -1168,9 +1171,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
 
 	dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_STOP);
 
-	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-		dpu_enc->phys_encs[i]->connector = NULL;
-	}
+	dpu_enc->connector = NULL;
 
 	DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 6e80321b13c5..5093810f6663 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -174,7 +174,6 @@ struct dpu_encoder_irq {
  *	tied to a specific panel / sub-panel. Abstract type, sub-classed by
  *	phys_vid or phys_cmd for video mode or command mode encs respectively.
  * @parent:		Pointer to the containing virtual encoder
- * @connector:		If a mode is set, cached pointer to the active connector
  * @ops:		Operations exposed to the virtual encoder
  * @parent_ops:		Callbacks exposed by the parent to the phys_enc
  * @hw_mdptop:		Hardware interface to the top registers
@@ -203,7 +202,6 @@ struct dpu_encoder_irq {
  */
 struct dpu_encoder_phys {
 	struct drm_encoder *parent;
-	struct drm_connector *connector;
 	struct dpu_encoder_phys_ops ops;
 	const struct dpu_encoder_virt_ops *parent_ops;
 	struct dpu_hw_mdp *hw_mdptop;
-- 
2.34.1

[PATCH 7/7] drm/msm/dpu: pull connector from dpu_encoder_phys to dpu_encoder_virt

[Dmitry Baryshkov]

All physical encoders used by virtual encoder share the same connector,
so pull the connector field from dpu_encoder_phys into dpu_encoder_virt
structure.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 11 ++++++-----
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  2 --
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 1462c426c14c..afafdaf48aea 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -143,6 +143,7 @@ enum dpu_enc_rc_states {
  *			link between encoder/crtc. However in this case we need
  *			to track crtc in the disable() hook which is called
  *			_after_ encoder_mask is cleared.
+ * @connector:		If a mode is set, cached pointer to the active connector
  * @crtc_kickoff_cb:		Callback into CRTC that will flush & start
  *				all CTL paths
  * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
@@ -183,6 +184,7 @@ struct dpu_encoder_virt {
 	bool intfs_swapped;
 
 	struct drm_crtc *crtc;
+	struct drm_connector *connector;
 
 	struct dentry *debugfs_root;
 	struct mutex enc_lock;
@@ -993,6 +995,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 
 	cstate->num_mixers = num_lm;
 
+	dpu_enc->connector = conn_state->connector;
+
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
 		int num_blk;
 		struct dpu_hw_blk *hw_blk[MAX_CHANNELS_PER_ENC];
@@ -1030,7 +1034,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 			return;
 		}
 
-		phys->connector = conn_state->connector;
 		phys->cached_mode = crtc_state->adjusted_mode;
 		if (phys->ops.atomic_mode_set)
 			phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
@@ -1064,7 +1067,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
 
 	if (dpu_enc->disp_info.intf_type == INTF_DSI &&
 			!WARN_ON(dpu_enc->num_phys_encs == 0)) {
-		unsigned bpc = dpu_enc->phys_encs[0]->connector->display_info.bpc;
+		unsigned bpc = dpu_enc->connector->display_info.bpc;
 		for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
 			if (!dpu_enc->hw_pp[i])
 				continue;
@@ -1168,9 +1171,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
 
 	dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_STOP);
 
-	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
-		dpu_enc->phys_encs[i]->connector = NULL;
-	}
+	dpu_enc->connector = NULL;
 
 	DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
index 6e80321b13c5..5093810f6663 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
@@ -174,7 +174,6 @@ struct dpu_encoder_irq {
  *	tied to a specific panel / sub-panel. Abstract type, sub-classed by
  *	phys_vid or phys_cmd for video mode or command mode encs respectively.
  * @parent:		Pointer to the containing virtual encoder
- * @connector:		If a mode is set, cached pointer to the active connector
  * @ops:		Operations exposed to the virtual encoder
  * @parent_ops:		Callbacks exposed by the parent to the phys_enc
  * @hw_mdptop:		Hardware interface to the top registers
@@ -203,7 +202,6 @@ struct dpu_encoder_irq {
  */
 struct dpu_encoder_phys {
 	struct drm_encoder *parent;
-	struct drm_connector *connector;
 	struct dpu_encoder_phys_ops ops;
 	const struct dpu_encoder_virt_ops *parent_ops;
 	struct dpu_hw_mdp *hw_mdptop;
-- 
2.34.1

Re: [PATCH 7/7] drm/msm/dpu: pull connector from dpu_encoder_phys to dpu_encoder_virt

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> All physical encoders used by virtual encoder share the same connector,
> so pull the connector field from dpu_encoder_phys into dpu_encoder_virt
> structure.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 11 ++++++-----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  2 --
>   2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1462c426c14c..afafdaf48aea 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -143,6 +143,7 @@ enum dpu_enc_rc_states {
>    *			link between encoder/crtc. However in this case we need
>    *			to track crtc in the disable() hook which is called
>    *			_after_ encoder_mask is cleared.
> + * @connector:		If a mode is set, cached pointer to the active connector
>    * @crtc_kickoff_cb:		Callback into CRTC that will flush & start
>    *				all CTL paths
>    * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
> @@ -183,6 +184,7 @@ struct dpu_encoder_virt {
>   	bool intfs_swapped;
>   
>   	struct drm_crtc *crtc;
> +	struct drm_connector *connector;
>   
>   	struct dentry *debugfs_root;
>   	struct mutex enc_lock;
> @@ -993,6 +995,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>   
>   	cstate->num_mixers = num_lm;
>   
> +	dpu_enc->connector = conn_state->connector;
> +
>   	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>   		int num_blk;
>   		struct dpu_hw_blk *hw_blk[MAX_CHANNELS_PER_ENC];
> @@ -1030,7 +1034,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>   			return;
>   		}
>   
> -		phys->connector = conn_state->connector;
>   		phys->cached_mode = crtc_state->adjusted_mode;
>   		if (phys->ops.atomic_mode_set)
>   			phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
> @@ -1064,7 +1067,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>   
>   	if (dpu_enc->disp_info.intf_type == INTF_DSI &&
>   			!WARN_ON(dpu_enc->num_phys_encs == 0)) {
> -		unsigned bpc = dpu_enc->phys_encs[0]->connector->display_info.bpc;
> +		unsigned bpc = dpu_enc->connector->display_info.bpc;
>   		for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>   			if (!dpu_enc->hw_pp[i])
>   				continue;
> @@ -1168,9 +1171,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>   
>   	dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_STOP);
>   
> -	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> -		dpu_enc->phys_encs[i]->connector = NULL;
> -	}
> +	dpu_enc->connector = NULL;
>   
>   	DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index 6e80321b13c5..5093810f6663 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -174,7 +174,6 @@ struct dpu_encoder_irq {
>    *	tied to a specific panel / sub-panel. Abstract type, sub-classed by
>    *	phys_vid or phys_cmd for video mode or command mode encs respectively.
>    * @parent:		Pointer to the containing virtual encoder
> - * @connector:		If a mode is set, cached pointer to the active connector
>    * @ops:		Operations exposed to the virtual encoder
>    * @parent_ops:		Callbacks exposed by the parent to the phys_enc
>    * @hw_mdptop:		Hardware interface to the top registers
> @@ -203,7 +202,6 @@ struct dpu_encoder_irq {
>    */
>   struct dpu_encoder_phys {
>   	struct drm_encoder *parent;
> -	struct drm_connector *connector;
>   	struct dpu_encoder_phys_ops ops;
>   	const struct dpu_encoder_virt_ops *parent_ops;
>   	struct dpu_hw_mdp *hw_mdptop;

Re: [PATCH 7/7] drm/msm/dpu: pull connector from dpu_encoder_phys to dpu_encoder_virt

[Abhinav Kumar]

On 2/3/2022 12:26 AM, Dmitry Baryshkov wrote:
> All physical encoders used by virtual encoder share the same connector,
> so pull the connector field from dpu_encoder_phys into dpu_encoder_virt
> structure.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c      | 11 ++++++-----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h |  2 --
>   2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1462c426c14c..afafdaf48aea 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -143,6 +143,7 @@ enum dpu_enc_rc_states {
>    *			link between encoder/crtc. However in this case we need
>    *			to track crtc in the disable() hook which is called
>    *			_after_ encoder_mask is cleared.
> + * @connector:		If a mode is set, cached pointer to the active connector
>    * @crtc_kickoff_cb:		Callback into CRTC that will flush & start
>    *				all CTL paths
>    * @crtc_kickoff_cb_data:	Opaque user data given to crtc_kickoff_cb
> @@ -183,6 +184,7 @@ struct dpu_encoder_virt {
>   	bool intfs_swapped;
>   
>   	struct drm_crtc *crtc;
> +	struct drm_connector *connector;
>   
>   	struct dentry *debugfs_root;
>   	struct mutex enc_lock;
> @@ -993,6 +995,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>   
>   	cstate->num_mixers = num_lm;
>   
> +	dpu_enc->connector = conn_state->connector;
> +
>   	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
>   		int num_blk;
>   		struct dpu_hw_blk *hw_blk[MAX_CHANNELS_PER_ENC];
> @@ -1030,7 +1034,6 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>   			return;
>   		}
>   
> -		phys->connector = conn_state->connector;
>   		phys->cached_mode = crtc_state->adjusted_mode;
>   		if (phys->ops.atomic_mode_set)
>   			phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
> @@ -1064,7 +1067,7 @@ static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>   
>   	if (dpu_enc->disp_info.intf_type == INTF_DSI &&
>   			!WARN_ON(dpu_enc->num_phys_encs == 0)) {
> -		unsigned bpc = dpu_enc->phys_encs[0]->connector->display_info.bpc;
> +		unsigned bpc = dpu_enc->connector->display_info.bpc;
>   		for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
>   			if (!dpu_enc->hw_pp[i])
>   				continue;
> @@ -1168,9 +1171,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>   
>   	dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_STOP);
>   
> -	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> -		dpu_enc->phys_encs[i]->connector = NULL;
> -	}
> +	dpu_enc->connector = NULL;
>   
>   	DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
>   
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> index 6e80321b13c5..5093810f6663 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys.h
> @@ -174,7 +174,6 @@ struct dpu_encoder_irq {
>    *	tied to a specific panel / sub-panel. Abstract type, sub-classed by
>    *	phys_vid or phys_cmd for video mode or command mode encs respectively.
>    * @parent:		Pointer to the containing virtual encoder
> - * @connector:		If a mode is set, cached pointer to the active connector
>    * @ops:		Operations exposed to the virtual encoder
>    * @parent_ops:		Callbacks exposed by the parent to the phys_enc
>    * @hw_mdptop:		Hardware interface to the top registers
> @@ -203,7 +202,6 @@ struct dpu_encoder_irq {
>    */
>   struct dpu_encoder_phys {
>   	struct drm_encoder *parent;
> -	struct drm_connector *connector;
>   	struct dpu_encoder_phys_ops ops;
>   	const struct dpu_encoder_virt_ops *parent_ops;
>   	struct dpu_hw_mdp *hw_mdptop;