[PATCH v4 0/2] retrieve DSI DSC through priv-dsi[0]

[PATCH v4 0/2] retrieve DSI DSC through priv-dsi[0]

[Kuogee Hsieh]

moving retrieving struct drm_dsc_cofnig from setup_display to
atomic_enable() and delete struct drm_dsc_config from
struct msm_display_info.

Kuogee Hsieh (2):
  drm/msm/dpu: retrieve DSI DSC struct through priv->dsi[0]
  drm/msm/dpu: remove struct drm_dsc_config from struct msm_display_info

 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32 +++++++++++++++++++++++------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  2 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 --
 3 files changed, 26 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH v4 1/2] drm/msm/dpu: retrieve DSI DSC struct through priv->dsi[0]

[Kuogee Hsieh]

Currently DSI DSC struct is populated at display setup during
system bootup. This mechanism works fine with embedded display
but not for pluggable displays as the DSC struct will become
stale once external display unplugged.

Move storing of DSI DSC struct to both atomic_mode_set() and
atomic_enable() so that same mechanism will work for both
embedded display and pluggable displays.

Changes in v4:
-- fix checkpatch.pl warning

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 2e1873d..367d374 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -543,11 +543,24 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
 	return (num_dsc > 0) && (num_dsc > intf_count);
 }
 
+static struct drm_dsc_config *dpu_encoder_get_dsc_config(struct drm_encoder *drm_enc)
+{
+	struct msm_drm_private *priv = drm_enc->dev->dev_private;
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+	int index = dpu_enc->disp_info.h_tile_instance[0];
+
+	if (dpu_enc->disp_info.intf_type == INTF_DSI)
+		return msm_dsi_get_dsc_config(priv->dsi[index]);
+
+	return NULL;
+}
+
 static struct msm_display_topology dpu_encoder_get_topology(
 			struct dpu_encoder_virt *dpu_enc,
 			struct dpu_kms *dpu_kms,
 			struct drm_display_mode *mode,
-			struct drm_crtc_state *crtc_state)
+			struct drm_crtc_state *crtc_state,
+			struct drm_dsc_config *dsc)
 {
 	struct msm_display_topology topology = {0};
 	int i, intf_count = 0;
@@ -579,7 +592,7 @@ static struct msm_display_topology dpu_encoder_get_topology(
 
 	topology.num_intf = intf_count;
 
-	if (dpu_enc->dsc) {
+	if (dsc) {
 		/*
 		 * In case of Display Stream Compression (DSC), we would use
 		 * 2 DSC encoders, 2 layer mixers and 1 interface
@@ -605,6 +618,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;
+	struct drm_dsc_config *dsc;
 	int i = 0;
 	int ret = 0;
 
@@ -640,7 +654,9 @@ static int dpu_encoder_virt_atomic_check(
 		}
 	}
 
-	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
+	dsc = dpu_encoder_get_dsc_config(drm_enc);
+
+	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, dsc);
 
 	/*
 	 * Release and Allocate resources on every modeset
@@ -1056,6 +1072,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
 
 	trace_dpu_enc_mode_set(DRMID(drm_enc));
 
+	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
+
 	/* 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,
@@ -1187,6 +1205,8 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
 
 	dpu_enc = to_dpu_encoder_virt(drm_enc);
 
+	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
+
 	mutex_lock(&dpu_enc->enc_lock);
 	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
 
@@ -2109,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);
-- 
2.7.4

[PATCH v4 2/2] drm/msm/dpu: remove struct drm_dsc_config from struct msm_display_info

[Kuogee Hsieh]

Since struct drm_dsc_config is stored at atomic_mode_set() instead
of display setup time during bootup, saving struct drm_dsc_config at
struct msm_display_info is not necessary. Lets drop the dsc member
from struct msm_display_info.

Changes in v4:
-- fix "Since" at commit text

Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 2 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 2 --
 3 files changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 367d374..b68610e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2313,8 +2313,6 @@ static int dpu_encoder_setup_display(struct dpu_encoder_virt *dpu_enc,
 		dpu_enc->idle_pc_supported =
 				dpu_kms->catalog->caps->has_idle_pc;
 
-	dpu_enc->dsc = disp_info->dsc;
-
 	mutex_lock(&dpu_enc->enc_lock);
 	for (i = 0; i < disp_info->num_of_h_tiles && !ret; i++) {
 		/*
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 90e1925..4c05fd5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -28,7 +28,6 @@
  * @is_cmd_mode		Boolean to indicate if the CMD mode is requested
  * @is_te_using_watchdog_timer:  Boolean to indicate watchdog TE is
  *				 used instead of panel TE in cmd mode panels
- * @dsc:		DSC configuration data for DSC-enabled displays
  */
 struct msm_display_info {
 	enum dpu_intf_type intf_type;
@@ -36,7 +35,6 @@ struct msm_display_info {
 	uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY];
 	bool is_cmd_mode;
 	bool is_te_using_watchdog_timer;
-	struct drm_dsc_config *dsc;
 };
 
 /**
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 613384b..5e77e09 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -544,8 +544,6 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 
 		info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]);
 
-		info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]);
-
 		encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_DSI, &info);
 		if (IS_ERR(encoder)) {
 			DPU_ERROR("encoder init failed for dsi display\n");
-- 
2.7.4

Re: [PATCH v4 0/2] retrieve DSI DSC through priv-dsi[0]

[Marijn Suijten]

On 2023-06-21 09:18:16, Kuogee Hsieh wrote:
> moving retrieving struct drm_dsc_cofnig from setup_display to
> atomic_enable() and delete struct drm_dsc_config from
> struct msm_display_info.

Abhinav suggested to reword this for clarity in v3, but none of that
seems to have made it through?

- Marijn

> Kuogee Hsieh (2):
>   drm/msm/dpu: retrieve DSI DSC struct through priv->dsi[0]
>   drm/msm/dpu: remove struct drm_dsc_config from struct msm_display_info
> 
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32 +++++++++++++++++++++++------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  2 --
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  2 --
>  3 files changed, 26 insertions(+), 10 deletions(-)
> 
> -- 
> 2.7.4
> 

Re: [PATCH v4 1/2] drm/msm/dpu: retrieve DSI DSC struct through priv->dsi[0]

[Dmitry Baryshkov]

On 21/06/2023 19:18, Kuogee Hsieh wrote:
> Currently DSI DSC struct is populated at display setup during
> system bootup. This mechanism works fine with embedded display
> but not for pluggable displays as the DSC struct will become
> stale once external display unplugged.
> 
> Move storing of DSI DSC struct to both atomic_mode_set() and
> atomic_enable() so that same mechanism will work for both
> embedded display and pluggable displays.
> 
> Changes in v4:
> -- fix checkpatch.pl warning
> 
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 30 +++++++++++++++++++++++++----
>   1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 2e1873d..367d374 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -543,11 +543,24 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
>   	return (num_dsc > 0) && (num_dsc > intf_count);
>   }
>   
> +static struct drm_dsc_config *dpu_encoder_get_dsc_config(struct drm_encoder *drm_enc)
> +{
> +	struct msm_drm_private *priv = drm_enc->dev->dev_private;
> +	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> +	int index = dpu_enc->disp_info.h_tile_instance[0];
> +
> +	if (dpu_enc->disp_info.intf_type == INTF_DSI)
> +		return msm_dsi_get_dsc_config(priv->dsi[index]);
> +
> +	return NULL;
> +}
> +
>   static struct msm_display_topology dpu_encoder_get_topology(
>   			struct dpu_encoder_virt *dpu_enc,
>   			struct dpu_kms *dpu_kms,
>   			struct drm_display_mode *mode,
> -			struct drm_crtc_state *crtc_state)
> +			struct drm_crtc_state *crtc_state,
> +			struct drm_dsc_config *dsc)
>   {
>   	struct msm_display_topology topology = {0};
>   	int i, intf_count = 0;
> @@ -579,7 +592,7 @@ static struct msm_display_topology dpu_encoder_get_topology(
>   
>   	topology.num_intf = intf_count;
>   
> -	if (dpu_enc->dsc) {
> +	if (dsc) {
>   		/*
>   		 * In case of Display Stream Compression (DSC), we would use
>   		 * 2 DSC encoders, 2 layer mixers and 1 interface
> @@ -605,6 +618,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;
> +	struct drm_dsc_config *dsc;
>   	int i = 0;
>   	int ret = 0;
>   
> @@ -640,7 +654,9 @@ static int dpu_encoder_virt_atomic_check(
>   		}
>   	}
>   
> -	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
> +	dsc = dpu_encoder_get_dsc_config(drm_enc);
> +
> +	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, dsc);
>   
>   	/*
>   	 * Release and Allocate resources on every modeset
> @@ -1056,6 +1072,8 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>   
>   	trace_dpu_enc_mode_set(DRMID(drm_enc));
>   
> +	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
> +
>   	/* 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,
> @@ -1187,6 +1205,8 @@ static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>   
>   	dpu_enc = to_dpu_encoder_virt(drm_enc);
>   
> +	dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);

Let's have a single place where dpu_enc->dsc is set. I have slight 
preference for atomic_enable, but atomic_mode_set is fine too.

> +
>   	mutex_lock(&dpu_enc->enc_lock);
>   	cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
>   
> @@ -2109,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);

-- 
With best wishes
Dmitry

Re: [PATCH v4 1/2] drm/msm/dpu: retrieve DSI DSC struct through priv->dsi[0]

[Abhinav Kumar]

On 6/22/2023 7:00 AM, Dmitry Baryshkov wrote:
> On 21/06/2023 19:18, Kuogee Hsieh wrote:
>> Currently DSI DSC struct is populated at display setup during
>> system bootup. This mechanism works fine with embedded display
>> but not for pluggable displays as the DSC struct will become
>> stale once external display unplugged.
>>
>> Move storing of DSI DSC struct to both atomic_mode_set() and
>> atomic_enable() so that same mechanism will work for both
>> embedded display and pluggable displays.
>>
>> Changes in v4:
>> -- fix checkpatch.pl warning
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 30 
>> +++++++++++++++++++++++++----
>>   1 file changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 2e1873d..367d374 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -543,11 +543,24 @@ bool dpu_encoder_use_dsc_merge(struct 
>> drm_encoder *drm_enc)
>>       return (num_dsc > 0) && (num_dsc > intf_count);
>>   }
>> +static struct drm_dsc_config *dpu_encoder_get_dsc_config(struct 
>> drm_encoder *drm_enc)
>> +{
>> +    struct msm_drm_private *priv = drm_enc->dev->dev_private;
>> +    struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
>> +    int index = dpu_enc->disp_info.h_tile_instance[0];
>> +
>> +    if (dpu_enc->disp_info.intf_type == INTF_DSI)
>> +        return msm_dsi_get_dsc_config(priv->dsi[index]);
>> +
>> +    return NULL;
>> +}
>> +
>>   static struct msm_display_topology dpu_encoder_get_topology(
>>               struct dpu_encoder_virt *dpu_enc,
>>               struct dpu_kms *dpu_kms,
>>               struct drm_display_mode *mode,
>> -            struct drm_crtc_state *crtc_state)
>> +            struct drm_crtc_state *crtc_state,
>> +            struct drm_dsc_config *dsc)
>>   {
>>       struct msm_display_topology topology = {0};
>>       int i, intf_count = 0;
>> @@ -579,7 +592,7 @@ static struct msm_display_topology 
>> dpu_encoder_get_topology(
>>       topology.num_intf = intf_count;
>> -    if (dpu_enc->dsc) {
>> +    if (dsc) {
>>           /*
>>            * In case of Display Stream Compression (DSC), we would use
>>            * 2 DSC encoders, 2 layer mixers and 1 interface
>> @@ -605,6 +618,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;
>> +    struct drm_dsc_config *dsc;
>>       int i = 0;
>>       int ret = 0;
>> @@ -640,7 +654,9 @@ static int dpu_encoder_virt_atomic_check(
>>           }
>>       }
>> -    topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, 
>> crtc_state);
>> +    dsc = dpu_encoder_get_dsc_config(drm_enc);
>> +
>> +    topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, 
>> crtc_state, dsc);
>>       /*
>>        * Release and Allocate resources on every modeset
>> @@ -1056,6 +1072,8 @@ static void 
>> dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>       trace_dpu_enc_mode_set(DRMID(drm_enc));
>> +    dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
>> +
>>       /* 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,
>> @@ -1187,6 +1205,8 @@ static void 
>> dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
>>       dpu_enc = to_dpu_encoder_virt(drm_enc);
>> +    dpu_enc->dsc = dpu_encoder_get_dsc_config(drm_enc);
> 
> Let's have a single place where dpu_enc->dsc is set. I have slight 
> preference for atomic_enable, but atomic_mode_set is fine too.
> 

Yes, it was in a single place in v3. Then some mistake happened and it 
got dropped in v4. Now, it has been added back for v5.

Sorry for the confusion.