diff mbox series

[v2,1/2] drm/msm/dpu: retrieve DSI DSC struct at atomic_check()

Message ID 1685657331-23280-2-git-send-email-quic_khsieh@quicinc.com (mailing list archive)
State New, archived
Headers show
Series retrieve DSI DSC through DRM bridge | expand

Commit Message

Kuogee Hsieh June 1, 2023, 10:08 p.m. UTC
At current implementation, DSI DSC struct is populated at display setup
during system bootup. This mechanism works fine with embedded display.
But will run into problem with plugin/unplug oriented external display,
such as DP, due to DSC struct will become stale once external display
unplugged. New DSC struct has to be re populated to reflect newer external
display which just plugged in. Move retrieving of DSI DSC struct to
atomic_check() so that same mechanism will work for both embedded display
and external plugin/unplug oriented display.

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Comments

Dmitry Baryshkov June 1, 2023, 10:19 p.m. UTC | #1
On 02/06/2023 01:08, Kuogee Hsieh wrote:
> At current implementation, DSI DSC struct is populated at display setup
> during system bootup. This mechanism works fine with embedded display.
> But will run into problem with plugin/unplug oriented external display,
> such as DP, due to DSC struct will become stale once external display
> unplugged. New DSC struct has to be re populated to reflect newer external
> display which just plugged in. Move retrieving of DSI DSC struct to
> atomic_check() so that same mechanism will work for both embedded display
> and external plugin/unplug oriented display.
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 3b416e1..5c440a0 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -604,7 +604,7 @@ 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 index, i = 0;
>   	int ret = 0;
>   
>   	if (!drm_enc || !crtc_state || !conn_state) {
> @@ -639,6 +639,10 @@ static int dpu_encoder_virt_atomic_check(
>   		}
>   	}
>   
> +	index = dpu_enc->disp_info.h_tile_instance[0];
> +        if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI)
> +		dpu_enc->dsc = msm_dsi_get_dsc_config(priv->dsi[index]);

As discussed previously, one should not write to non-state objects from 
atomic_check. This chunk does.

Not to mention that this will start exploding once you try adding DP 
next to it.

Please abstain from posting next revisions until the discussions on the 
previous one are more or less finished. For now this is NAK.

Not to mention that this patch doesn't pass checkpatch.pl.

> +
>   	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
>   
>   	/*
> @@ -1034,7 +1038,7 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>   	struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
>   	int num_lm, num_ctl, num_pp, num_dsc;
>   	unsigned int dsc_mask = 0;
> -	int i;
> +	int index, i;
>   
>   	if (!drm_enc) {
>   		DPU_ERROR("invalid encoder\n");
> @@ -1055,6 +1059,10 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>   
>   	trace_dpu_enc_mode_set(DRMID(drm_enc));
>   
> +	index = dpu_enc->disp_info.h_tile_instance[0];
> +        if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI)
> +		dpu_enc->dsc = msm_dsi_get_dsc_config(priv->dsi[index]);

Doesn't this seem 100% same as the previous chunk? Doesn't it plead to 
be extracted to a helper function?

> +
>   	/* 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,
> @@ -2121,8 +2129,10 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
>   					phys_enc->hw_pp->merge_3d->idx);
>   	}
>   
> -	if (dpu_enc->dsc)
> +	if (dpu_enc->dsc) {
>   		dpu_encoder_unprep_dsc(dpu_enc);
> +		dpu_enc->dsc = NULL;
> +	}
>   
>   	intf_cfg.stream_sel = 0; /* Don't care value for video mode */
>   	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 3b416e1..5c440a0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -604,7 +604,7 @@  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 index, i = 0;
 	int ret = 0;
 
 	if (!drm_enc || !crtc_state || !conn_state) {
@@ -639,6 +639,10 @@  static int dpu_encoder_virt_atomic_check(
 		}
 	}
 
+	index = dpu_enc->disp_info.h_tile_instance[0];
+        if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI)
+		dpu_enc->dsc = msm_dsi_get_dsc_config(priv->dsi[index]);
+
 	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
 
 	/*
@@ -1034,7 +1038,7 @@  static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 	struct dpu_hw_blk *hw_dsc[MAX_CHANNELS_PER_ENC];
 	int num_lm, num_ctl, num_pp, num_dsc;
 	unsigned int dsc_mask = 0;
-	int i;
+	int index, i;
 
 	if (!drm_enc) {
 		DPU_ERROR("invalid encoder\n");
@@ -1055,6 +1059,10 @@  static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 
 	trace_dpu_enc_mode_set(DRMID(drm_enc));
 
+	index = dpu_enc->disp_info.h_tile_instance[0];
+        if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI)
+		dpu_enc->dsc = msm_dsi_get_dsc_config(priv->dsi[index]);
+
 	/* 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,
@@ -2121,8 +2129,10 @@  void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
 					phys_enc->hw_pp->merge_3d->idx);
 	}
 
-	if (dpu_enc->dsc)
+	if (dpu_enc->dsc) {
 		dpu_encoder_unprep_dsc(dpu_enc);
+		dpu_enc->dsc = NULL;
+	}
 
 	intf_cfg.stream_sel = 0; /* Don't care value for video mode */
 	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);