diff mbox series

[v1,2/3] drm/msm/disp/dpu1: add dspps into reservation if there is a ctm request

Message ID 1675092092-26412-3-git-send-email-quic_kalyant@quicinc.com (mailing list archive)
State New, archived
Headers show
Series Reserve dspps based on user request | expand

Commit Message

Kalyan Thota Jan. 30, 2023, 3:21 p.m. UTC 
Add dspp blocks into the topology for reservation, if there is a ctm
request for that composition.

Changes in v1:
- Minor nits (Dmitry)

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

Dmitry Baryshkov Jan. 30, 2023, 6:36 p.m. UTC | #1 
On 30/01/2023 17:21, Kalyan Thota wrote:
> Add dspp blocks into the topology for reservation, if there is a ctm
> request for that composition.
> 
> Changes in v1:
> - Minor nits (Dmitry)
> 
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++-------
>   1 file 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 9c6817b..3bd46b4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
>   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_display_mode *mode,
> +			struct drm_crtc_state *crtc_state)
>   {
>   	struct msm_display_topology topology = {0};
>   	int i, intf_count = 0;
> @@ -573,11 +574,9 @@ 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_kms->catalog->dspp &&
> -			(dpu_kms->catalog->dspp_count >= topology.num_lm))
> -			topology.num_dspp = topology.num_lm;
> -	}
> +	if (dpu_kms->catalog->dspp &&
> +	    crtc_state->ctm && (dpu_kms->catalog->dspp_count >= topology.num_lm))

This condition doesn't look correct anymore for the following reasons:
- if there are no DSPPs we will completely ignore the ctm property
- if there are not enough DSPPs, the CTM property will be ignore

I think, this should be just:

if (crtc_state->ctm)
     topology.num_dspp = topology.num_lm;




> +		topology.num_dspp = topology.num_lm;
>   
>   	topology.num_enc = 0;
>   	topology.num_intf = intf_count;
> @@ -643,7 +642,7 @@ static int dpu_encoder_virt_atomic_check(
>   		}
>   	}
>   
> -	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
> +	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
>   
>   	/* Reserve dynamic resources now. */
>   	if (!ret) {
Marijn Suijten Feb. 1, 2023, 11:16 a.m. UTC | #2 
On 2023-01-30 07:21:31, Kalyan Thota wrote:
> Add dspp blocks into the topology for reservation, if there is a ctm
> request for that composition.

DSPP

> Changes in v1:
> - Minor nits (Dmitry)

This should go below the triple dashes, so that it /does not/ become
part of the patch/commit that is applied to the tree (where review
history is irrelevant as it can be searched for separately).

> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++-------
>  1 file 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 9c6817b..3bd46b4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
>  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_display_mode *mode,
> +			struct drm_crtc_state *crtc_state)
>  {
>  	struct msm_display_topology topology = {0};
>  	int i, intf_count = 0;
> @@ -573,11 +574,9 @@ 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_kms->catalog->dspp &&
> -			(dpu_kms->catalog->dspp_count >= topology.num_lm))
> -			topology.num_dspp = topology.num_lm;
> -	}
> +	if (dpu_kms->catalog->dspp &&
> +	    crtc_state->ctm && (dpu_kms->catalog->dspp_count >= topology.num_lm))

Multiline-if-clause is typically indented with two tabs, not a half tab
(4 spaces).

Nit: swap the && here?  dspp and dspp_count are related, so check ctm
first or last but not in the middle - makes reading easier.

> +		topology.num_dspp = topology.num_lm;
>  
>  	topology.num_enc = 0;
>  	topology.num_intf = intf_count;
> @@ -643,7 +642,7 @@ static int dpu_encoder_virt_atomic_check(
>  		}
>  	}
>  
> -	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
> +	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
>  
>  	/* Reserve dynamic resources now. */
>  	if (!ret) {
> -- 
> 2.7.4
>
Marijn Suijten Feb. 1, 2023, 11:26 a.m. UTC | #3 
On 2023-02-01 12:16:05, Marijn Suijten wrote:
<snip>
> > +	if (dpu_kms->catalog->dspp &&
> > +	    crtc_state->ctm && (dpu_kms->catalog->dspp_count >= topology.num_lm))
> 
> Multiline-if-clause is typically indented with two tabs, not a half tab
> (4 spaces).

Hmm, Dmitry requested indent-to-opening-parenthesis in v1 instead; and
the majority of dpu1 uses the worst version of all: indent with a single
tab so that the contents line up with the code block below.  Dmitry,
I'll leave final say to you (and fix it up in my own DPU series
accordingly too).

- Marijn
Dmitry Baryshkov Feb. 1, 2023, 1:48 p.m. UTC | #4 
On 01/02/2023 13:16, Marijn Suijten wrote:
> On 2023-01-30 07:21:31, Kalyan Thota wrote:
>> Add dspp blocks into the topology for reservation, if there is a ctm
>> request for that composition.
> 
> DSPP
> 
>> Changes in v1:
>> - Minor nits (Dmitry)
> 
> This should go below the triple dashes, so that it /does not/ become
> part of the patch/commit that is applied to the tree (where review
> history is irrelevant as it can be searched for separately).

This is one of DRM peculiarities which we have to live with.

> 
>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++-------
>>   1 file 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 9c6817b..3bd46b4 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
>>   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_display_mode *mode,
>> +			struct drm_crtc_state *crtc_state)
>>   {
>>   	struct msm_display_topology topology = {0};
>>   	int i, intf_count = 0;
>> @@ -573,11 +574,9 @@ 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_kms->catalog->dspp &&
>> -			(dpu_kms->catalog->dspp_count >= topology.num_lm))
>> -			topology.num_dspp = topology.num_lm;
>> -	}
>> +	if (dpu_kms->catalog->dspp &&
>> +	    crtc_state->ctm && (dpu_kms->catalog->dspp_count >= topology.num_lm))
> 
> Multiline-if-clause is typically indented with two tabs, not a half tab
> (4 spaces).

I tend to disagree here. Lately I have mostly seen it being indented to 
the opening parenthesis, so that nested statements also indent nicely.

> Nit: swap the && here?  dspp and dspp_count are related, so check ctm
> first or last but not in the middle - makes reading easier.

I think we can ignore dpu_kms->catalog->dspp completely. checking 
dspp_count should be enough for the purpose of the check (and note, the 
check for dspp/dspp_count is misleading and should be omitted).

> 
>> +		topology.num_dspp = topology.num_lm;
>>   
>>   	topology.num_enc = 0;
>>   	topology.num_intf = intf_count;
>> @@ -643,7 +642,7 @@ static int dpu_encoder_virt_atomic_check(
>>   		}
>>   	}
>>   
>> -	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
>> +	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
>>   
>>   	/* Reserve dynamic resources now. */
>>   	if (!ret) {
>> -- 
>> 2.7.4
>>
Dmitry Baryshkov Feb. 1, 2023, 1:49 p.m. UTC | #5 
On 01/02/2023 13:26, Marijn Suijten wrote:
> On 2023-02-01 12:16:05, Marijn Suijten wrote:
> <snip>
>>> +	if (dpu_kms->catalog->dspp &&
>>> +	    crtc_state->ctm && (dpu_kms->catalog->dspp_count >= topology.num_lm))
>>
>> Multiline-if-clause is typically indented with two tabs, not a half tab
>> (4 spaces).
> 
> Hmm, Dmitry requested indent-to-opening-parenthesis in v1 instead; and
> the majority of dpu1 uses the worst version of all: indent with a single
> tab so that the contents line up with the code block below.  Dmitry,
> I'll leave final say to you (and fix it up in my own DPU series
> accordingly too).

Well,

:set cino=(0

> 
> - Marijn
Marijn Suijten Feb. 1, 2023, 3:10 p.m. UTC | #6 
On 2023-02-01 15:48:02, Dmitry Baryshkov wrote:
> On 01/02/2023 13:16, Marijn Suijten wrote:
> > On 2023-01-30 07:21:31, Kalyan Thota wrote:
> >> Add dspp blocks into the topology for reservation, if there is a ctm
> >> request for that composition.
> > 
> > DSPP
> > 
> >> Changes in v1:
> >> - Minor nits (Dmitry)
> > 
> > This should go below the triple dashes, so that it /does not/ become
> > part of the patch/commit that is applied to the tree (where review
> > history is irrelevant as it can be searched for separately).
> 
> This is one of DRM peculiarities which we have to live with.

Not sure I follow.  Keeping "changes since vXX" out of commit messages
seems to be a kernel-wide convention, after all the title doesn't
include which revision of the patch ended up being applied to the tree
either.  Having the changelog checked in to the tree has no relevance.

> >> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> >> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 ++++++-------
> >>   1 file 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 9c6817b..3bd46b4 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> @@ -545,7 +545,8 @@ bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
> >>   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_display_mode *mode,
> >> +			struct drm_crtc_state *crtc_state)
> >>   {
> >>   	struct msm_display_topology topology = {0};
> >>   	int i, intf_count = 0;
> >> @@ -573,11 +574,9 @@ 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_kms->catalog->dspp &&
> >> -			(dpu_kms->catalog->dspp_count >= topology.num_lm))
> >> -			topology.num_dspp = topology.num_lm;
> >> -	}
> >> +	if (dpu_kms->catalog->dspp &&
> >> +	    crtc_state->ctm && (dpu_kms->catalog->dspp_count >= topology.num_lm))
> > 
> > Multiline-if-clause is typically indented with two tabs, not a half tab
> > (4 spaces).
> 
> I tend to disagree here. Lately I have mostly seen it being indented to 
> the opening parenthesis, so that nested statements also indent nicely.

Ack, hence double-checked in a followup message; there's no concistency
in dpu1 now but I agree that for ts=8 a 4-space-indented wraparound
neatly aligns with the expression on the first line /and/ prevents
inadvertently aligning with the conditional body on the next line.

Will fix up in my own series too, thanks!

> > Nit: swap the && here?  dspp and dspp_count are related, so check ctm
> > first or last but not in the middle - makes reading easier.
> 
> I think we can ignore dpu_kms->catalog->dspp completely. checking 
> dspp_count should be enough for the purpose of the check (and note, the 
> check for dspp/dspp_count is misleading and should be omitted).

Ack, thanks!

- Marijn
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 9c6817b..3bd46b4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -545,7 +545,8 @@  bool dpu_encoder_use_dsc_merge(struct drm_encoder *drm_enc)
 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_display_mode *mode,
+			struct drm_crtc_state *crtc_state)
 {
 	struct msm_display_topology topology = {0};
 	int i, intf_count = 0;
@@ -573,11 +574,9 @@  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_kms->catalog->dspp &&
-			(dpu_kms->catalog->dspp_count >= topology.num_lm))
-			topology.num_dspp = topology.num_lm;
-	}
+	if (dpu_kms->catalog->dspp &&
+	    crtc_state->ctm && (dpu_kms->catalog->dspp_count >= topology.num_lm))
+		topology.num_dspp = topology.num_lm;
 
 	topology.num_enc = 0;
 	topology.num_intf = intf_count;
@@ -643,7 +642,7 @@  static int dpu_encoder_virt_atomic_check(
 		}
 	}
 
-	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode);
+	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state);
 
 	/* Reserve dynamic resources now. */
 	if (!ret) {