[v1] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder

[v1] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder

[Kalyan Thota]

Pin each crtc with one encoder. This arrangement will
disallow crtc switching between encoders and also will
facilitate to advertise certain features on crtc based
on encoder type.

Changes in v1:
- use drm_for_each_encoder macro while iterating through
  encoder list (Dmitry)

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 7a5fabc..0d94eec0d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -798,19 +798,20 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
 	max_crtc_count = min(max_crtc_count, primary_planes_idx);
 
 	/* Create one CRTC per encoder */
-	for (i = 0; i < max_crtc_count; i++) {
-		crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
-		if (IS_ERR(crtc)) {
-			ret = PTR_ERR(crtc);
-			return ret;
+	i = 0;
+	drm_for_each_encoder(encoder, dev) {
+		if (i < max_crtc_count) {
+			crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
+			if (IS_ERR(crtc)) {
+				ret = PTR_ERR(crtc);
+				return ret;
+			}
+			priv->crtcs[priv->num_crtcs++] = crtc;
+			encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
 		}
-		priv->crtcs[priv->num_crtcs++] = crtc;
+		i++;
 	}
 
-	/* All CRTCs are compatible with all encoders */
-	drm_for_each_encoder(encoder, dev)
-		encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
-
 	return 0;
 }
 
-- 
2.7.4

Re: [v1] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder

[Dmitry Baryshkov]

On 11/11/2022 16:56, Kalyan Thota wrote:
> Pin each crtc with one encoder. This arrangement will
> disallow crtc switching between encoders and also will
> facilitate to advertise certain features on crtc based
> on encoder type.
> 
> Changes in v1:
> - use drm_for_each_encoder macro while iterating through
>    encoder list (Dmitry)

BTW: if these patches form a series, please send them so.

> 
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 7a5fabc..0d94eec0d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -798,19 +798,20 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms *dpu_kms)
>   	max_crtc_count = min(max_crtc_count, primary_planes_idx);
>   
>   	/* Create one CRTC per encoder */
> -	for (i = 0; i < max_crtc_count; i++) {
> -		crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
> -		if (IS_ERR(crtc)) {
> -			ret = PTR_ERR(crtc);
> -			return ret;
> +	i = 0;
> +	drm_for_each_encoder(encoder, dev) {
> +		if (i < max_crtc_count) {

What if max_crtc_counter < num_encoders? I think we should disallow such 
configuration. Can it happen on any of relevant platforms?

> +			crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
> +			if (IS_ERR(crtc)) {
> +				ret = PTR_ERR(crtc);
> +				return ret;
> +			}
> +			priv->crtcs[priv->num_crtcs++] = crtc;
> +			encoder->possible_crtcs = 1 << drm_crtc_index(crtc);
>   		}
> -		priv->crtcs[priv->num_crtcs++] = crtc;
> +		i++;
>   	}
>   
> -	/* All CRTCs are compatible with all encoders */
> -	drm_for_each_encoder(encoder, dev)
> -		encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
> -
>   	return 0;
>   }
>   

-- 
With best wishes
Dmitry

RE: [v1] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder

[Kalyan Thota]

>-----Original Message-----
>From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>Sent: Saturday, November 12, 2022 3:51 AM
>To: Kalyan Thota (QUIC) <quic_kalyant@quicinc.com>; dri-
>devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org;
>freedreno@lists.freedesktop.org; devicetree@vger.kernel.org
>Cc: linux-kernel@vger.kernel.org; robdclark@chromium.org;
>dianders@chromium.org; swboyd@chromium.org; Vinod Polimera (QUIC)
><quic_vpolimer@quicinc.com>; Abhinav Kumar (QUIC)
><quic_abhinavk@quicinc.com>
>Subject: Re: [v1] drm/msm/disp/dpu1: pin 1 crtc to 1 encoder
>
>WARNING: This email originated from outside of Qualcomm. Please be wary of
>any links or attachments, and do not enable macros.
>
>On 11/11/2022 16:56, Kalyan Thota wrote:
>> Pin each crtc with one encoder. This arrangement will disallow crtc
>> switching between encoders and also will facilitate to advertise
>> certain features on crtc based on encoder type.
>>
>> Changes in v1:
>> - use drm_for_each_encoder macro while iterating through
>>    encoder list (Dmitry)
>
>BTW: if these patches form a series, please send them so.
>
>>
>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 +++++++++++----------
>>   1 file changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 7a5fabc..0d94eec0d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -798,19 +798,20 @@ static int _dpu_kms_drm_obj_init(struct dpu_kms
>*dpu_kms)
>>       max_crtc_count = min(max_crtc_count, primary_planes_idx);
>>
>>       /* Create one CRTC per encoder */
>> -     for (i = 0; i < max_crtc_count; i++) {
>> -             crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
>> -             if (IS_ERR(crtc)) {
>> -                     ret = PTR_ERR(crtc);
>> -                     return ret;
>> +     i = 0;
>> +     drm_for_each_encoder(encoder, dev) {
>> +             if (i < max_crtc_count) {
>
>What if max_crtc_counter < num_encoders? I think we should disallow such
>configuration. Can it happen on any of relevant platforms?
>
Yes, we don't need the below checks accounting for crtc as all the platforms in the catalog has sufficient resources.

max_crtc_count = min(catalog->mixer_count, num_encoders); 
	This check is not needed, as mixer resource allocation will happen at later time, even though you have less mixers than encoders, one can turn off
	the crtc and get the mixers back to free pool.
	
max_crtc_count = min(max_crtc_count, primary_planes_idx);
	A safety check, but mostly, all the platforms are ensured that at least 1 primary plane is available per interface.
	will add WARN ON additionally

>> +                     crtc = dpu_crtc_init(dev, primary_planes[i], cursor_planes[i]);
>> +                     if (IS_ERR(crtc)) {
>> +                             ret = PTR_ERR(crtc);
>> +                             return ret;
>> +                     }
>> +                     priv->crtcs[priv->num_crtcs++] = crtc;
>> +                     encoder->possible_crtcs = 1 <<
>> + drm_crtc_index(crtc);
>>               }
>> -             priv->crtcs[priv->num_crtcs++] = crtc;
>> +             i++;
>>       }
>>
>> -     /* All CRTCs are compatible with all encoders */
>> -     drm_for_each_encoder(encoder, dev)
>> -             encoder->possible_crtcs = (1 << priv->num_crtcs) - 1;
>> -
>>       return 0;
>>   }
>>
>
>--
>With best wishes
>Dmitry