[PATCH 0/3] Allow composer fallbacks for color features

[PATCH 0/3] Allow composer fallbacks for color features

[Kalyan Thota]

This series will enable color features on sc7280 target which has primary panel as eDP

The series removes dspp allocation based on encoder type and allows the datapath reservation
even if dspps are not available.

The series also adds a check to fail the composition during atomic check , if color management is requested 
and no dspps are allocated in the datapath.

This can allow composer fallbacks for color features if no relevant HW blocks are available.

Kalyan Thota (3):
  drm/msm/disp/dpu1: allow reservation even if dspps are not available.
  drm/msm/disp/dpu1: allow dspp selection for all the interfaces
  drm/msm/disp/dpu1: fail atomic check if color feature is requested
    with no dspp

 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 11 +++++++++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +++++++++---------
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c      |  8 +++++++-
 3 files changed, 27 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH 1/3] drm/msm/disp/dpu1: allow reservation even if dspps are not available.

[Kalyan Thota]

if any topology requests for dspps and catalogue doesn't have the
allocation, avoid failing the reservation.

This can pave way to build logic allowing composer fallbacks
for all the color features that are handled in dspp.

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
index 73b3442..c8899ae 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
@@ -343,7 +343,13 @@ static bool _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
 		return true;
 
 	idx = lm_cfg->dspp - DSPP_0;
-	if (idx < 0 || idx >= ARRAY_SIZE(rm->dspp_blks)) {
+
+	if (idx < 0) {
+		DPU_DEBUG("lm doesn't have dspp, ignoring the request %d\n", lm_cfg->dspp);
+		return true;
+	}
+
+	if (idx >= ARRAY_SIZE(rm->dspp_blks)) {
 		DPU_ERROR("failed to get dspp on lm %d\n", lm_cfg->dspp);
 		return false;
 	}
-- 
2.7.4

Re: [PATCH 1/3] drm/msm/disp/dpu1: allow reservation even if dspps are not available.

[Dmitry Baryshkov]

On 17/01/2023 18:21, Kalyan Thota wrote:
> if any topology requests for dspps and catalogue doesn't have the
> allocation, avoid failing the reservation.
> 
> This can pave way to build logic allowing composer fallbacks
> for all the color features that are handled in dspp.
> 
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> index 73b3442..c8899ae 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
> @@ -343,7 +343,13 @@ static bool _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
>   		return true;
>   
>   	idx = lm_cfg->dspp - DSPP_0;
> -	if (idx < 0 || idx >= ARRAY_SIZE(rm->dspp_blks)) {
> +
> +	if (idx < 0) {

The change doesn't correspond to commit message.

> +		DPU_DEBUG("lm doesn't have dspp, ignoring the request %d\n", lm_cfg->dspp);
> +		return true;
> +	}
> +
> +	if (idx >= ARRAY_SIZE(rm->dspp_blks)) {
>   		DPU_ERROR("failed to get dspp on lm %d\n", lm_cfg->dspp);
>   		return false;
>   	}

If you'd like to remove duplicate for the (idx >= ARRAY_SIZE) check, I'd 
suggest dropping the second one


-- 
With best wishes
Dmitry

Re: [PATCH 1/3] drm/msm/disp/dpu1: allow reservation even if dspps are not available.

[Dmitry Baryshkov]

On 17/01/2023 18:35, Dmitry Baryshkov wrote:
> On 17/01/2023 18:21, Kalyan Thota wrote:
>> if any topology requests for dspps and catalogue doesn't have the
>> allocation, avoid failing the reservation.
>>
>> This can pave way to build logic allowing composer fallbacks
>> for all the color features that are handled in dspp.
>>
>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> index 73b3442..c8899ae 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>> @@ -343,7 +343,13 @@ static bool 
>> _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
>>           return true;
>>       idx = lm_cfg->dspp - DSPP_0;
>> -    if (idx < 0 || idx >= ARRAY_SIZE(rm->dspp_blks)) {
>> +
>> +    if (idx < 0) {
> 
> The change doesn't correspond to commit message.
> 
>> +        DPU_DEBUG("lm doesn't have dspp, ignoring the request %d\n", 
>> lm_cfg->dspp);
>> +        return true;
>> +    }
>> +
>> +    if (idx >= ARRAY_SIZE(rm->dspp_blks)) {
>>           DPU_ERROR("failed to get dspp on lm %d\n", lm_cfg->dspp);
>>           return false;
>>       }
> 
> If you'd like to remove duplicate for the (idx >= ARRAY_SIZE) check, I'd 
> suggest dropping the second one
> 

I've misread the patch. However I don't see, why would one request 
DSPP_NONE while specifying topology->num_dspp. I think that you are 
trying to put additional logic into a function that should just check 
for the available resources.

-- 
With best wishes
Dmitry

RE: [PATCH 1/3] drm/msm/disp/dpu1: allow reservation even if dspps are not available.

[Kalyan Thota]

>-----Original Message-----
>From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>Sent: Tuesday, January 17, 2023 10:10 PM
>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: [PATCH 1/3] drm/msm/disp/dpu1: allow reservation even if dspps are
>not available.
>
>WARNING: This email originated from outside of Qualcomm. Please be wary of
>any links or attachments, and do not enable macros.
>
>On 17/01/2023 18:35, Dmitry Baryshkov wrote:
>> On 17/01/2023 18:21, Kalyan Thota wrote:
>>> if any topology requests for dspps and catalogue doesn't have the
>>> allocation, avoid failing the reservation.
>>>
>>> This can pave way to build logic allowing composer fallbacks for all
>>> the color features that are handled in dspp.
>>>
>>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> index 73b3442..c8899ae 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>> @@ -343,7 +343,13 @@ static bool
>>> _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
>>>           return true;
>>>       idx = lm_cfg->dspp - DSPP_0;
>>> -    if (idx < 0 || idx >= ARRAY_SIZE(rm->dspp_blks)) {
>>> +
>>> +    if (idx < 0) {
>>
>> The change doesn't correspond to commit message.
>>
>>> +        DPU_DEBUG("lm doesn't have dspp, ignoring the request %d\n",
>>> lm_cfg->dspp);
>>> +        return true;
>>> +    }
>>> +
>>> +    if (idx >= ARRAY_SIZE(rm->dspp_blks)) {
>>>           DPU_ERROR("failed to get dspp on lm %d\n", lm_cfg->dspp);
>>>           return false;
>>>       }
>>
>> If you'd like to remove duplicate for the (idx >= ARRAY_SIZE) check,
>> I'd suggest dropping the second one
>>
>
>I've misread the patch. However I don't see, why would one request DSPP_NONE
>while specifying topology->num_dspp. I think that you are trying to put additional
>logic into a function that should just check for the available resources.
>

The link is specified in the catalogue. 
For example:

	LM_BLK("lm_0", LM_0, 0x44000, MIXER_SC7180_MASK,
		&sc7180_lm_sblk, PINGPONG_0, 0, DSPP_0),     --> This LM has DSPP attached 
	LM_BLK("lm_2", LM_2, 0x46000, MIXER_SC7180_MASK,
		&sc7180_lm_sblk, PINGPONG_2, LM_3, 0),  --> no DSPP 
	LM_BLK("lm_3", LM_3, 0x47000, MIXER_SC7180_MASK,
		&sc7180_lm_sblk, PINGPONG_3, LM_2, 0), --> no DSPP 

For the above example, num_dspps will be 1 which is nonzero. But if a request comes on second interface and if there are no dspps then we are not failing the reservation of data path as color features can be offloaded to GPU.
Idx for LM_2 and LM_3 will be -1 for above case hence the check not to fail reservation.

topology->num_dspp previously was filled based on encoder type, since we want to move away from encoder checks, we are now passing it same as LM number. If there are dspps available we will allocate, 
in case of non-availability then we are not failing the datapath reservation so that composer fallbacks can be implemented.

>--
>With best wishes
>Dmitry

Re: [PATCH 1/3] drm/msm/disp/dpu1: allow reservation even if dspps are not available.

[Dmitry Baryshkov]

On 18/01/2023 05:24, Kalyan Thota wrote:
> 
> 
>> -----Original Message-----
>> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Sent: Tuesday, January 17, 2023 10:10 PM
>> 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: [PATCH 1/3] drm/msm/disp/dpu1: allow reservation even if dspps are
>> not available.
>>
>> WARNING: This email originated from outside of Qualcomm. Please be wary of
>> any links or attachments, and do not enable macros.
>>
>> On 17/01/2023 18:35, Dmitry Baryshkov wrote:
>>> On 17/01/2023 18:21, Kalyan Thota wrote:
>>>> if any topology requests for dspps and catalogue doesn't have the
>>>> allocation, avoid failing the reservation.
>>>>
>>>> This can pave way to build logic allowing composer fallbacks for all
>>>> the color features that are handled in dspp.
>>>>
>>>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> index 73b3442..c8899ae 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_rm.c
>>>> @@ -343,7 +343,13 @@ static bool
>>>> _dpu_rm_check_lm_and_get_connected_blks(struct dpu_rm *rm,
>>>>            return true;
>>>>        idx = lm_cfg->dspp - DSPP_0;
>>>> -    if (idx < 0 || idx >= ARRAY_SIZE(rm->dspp_blks)) {
>>>> +
>>>> +    if (idx < 0) {
>>>
>>> The change doesn't correspond to commit message.
>>>
>>>> +        DPU_DEBUG("lm doesn't have dspp, ignoring the request %d\n",
>>>> lm_cfg->dspp);
>>>> +        return true;
>>>> +    }
>>>> +
>>>> +    if (idx >= ARRAY_SIZE(rm->dspp_blks)) {
>>>>            DPU_ERROR("failed to get dspp on lm %d\n", lm_cfg->dspp);
>>>>            return false;
>>>>        }
>>>
>>> If you'd like to remove duplicate for the (idx >= ARRAY_SIZE) check,
>>> I'd suggest dropping the second one
>>>
>>
>> I've misread the patch. However I don't see, why would one request DSPP_NONE
>> while specifying topology->num_dspp. I think that you are trying to put additional
>> logic into a function that should just check for the available resources.
>>
> 
> The link is specified in the catalogue.
> For example:
> 
> 	LM_BLK("lm_0", LM_0, 0x44000, MIXER_SC7180_MASK,
> 		&sc7180_lm_sblk, PINGPONG_0, 0, DSPP_0),     --> This LM has DSPP attached
> 	LM_BLK("lm_2", LM_2, 0x46000, MIXER_SC7180_MASK,
> 		&sc7180_lm_sblk, PINGPONG_2, LM_3, 0),  --> no DSPP
> 	LM_BLK("lm_3", LM_3, 0x47000, MIXER_SC7180_MASK,
> 		&sc7180_lm_sblk, PINGPONG_3, LM_2, 0), --> no DSPP
> 
> For the above example, num_dspps will be 1 which is nonzero. But if a request comes on second interface and if there are no dspps then we are not failing the reservation of data path as color features can be offloaded to GPU.
> Idx for LM_2 and LM_3 will be -1 for above case hence the check not to fail reservation.
> 
> topology->num_dspp previously was filled based on encoder type, since we want to move away from encoder checks, we are now passing it same as LM number. If there are dspps available we will allocate,
> in case of non-availability then we are not failing the datapath reservation so that composer fallbacks can be implemented.

As I wrote, num_dspps should be filled correctly (by the 
dpu_get_topology) to request DSPP for CTM-enabled CRTCs and to set the 
field to 0 if CRTC doesn't have CTM enabled.

Then RM code should adhere to the num_dspps passed. It must return an 
error if it can not fulfil the requirements. Also if no DSPPs are 
requested, RM should prefer non-DSPP-enabled LMs.

-- 
With best wishes
Dmitry

[PATCH 2/3] drm/msm/disp/dpu1: allow dspp selection for all the interfaces

[Kalyan Thota]

Allow dspps to be populated as a requirement for all the encoder
types it need not be just DSI. If for any encoder the dspp
allocation doesn't go through then there can be an option to
fallback for color features.

Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 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..e39b345 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;
@@ -563,8 +564,9 @@ static struct msm_display_topology dpu_encoder_get_topology(
 	 * 1 LM, 1 INTF
 	 * 2 LM, 1 INTF (stream merge to support high resolution interfaces)
 	 *
-	 * Adding color blocks only to primary interface if available in
-	 * sufficient number
+	 * dspp blocks are made optional. If RM manager cannot allocate
+	 * dspp blocks, then reservations will still go through with non dspp LM's
+	 * so as to allow color management support via composer fallbacks
 	 */
 	if (intf_count == 2)
 		topology.num_lm = 2;
@@ -573,11 +575,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 &&
+	    (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 +643,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

Re: [PATCH 2/3] drm/msm/disp/dpu1: allow dspp selection for all the interfaces

[Dmitry Baryshkov]

On 17/01/2023 18:21, Kalyan Thota wrote:
> Allow dspps to be populated as a requirement for all the encoder
> types it need not be just DSI. If for any encoder the dspp
> allocation doesn't go through then there can be an option to
> fallback for color features.
> 
> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 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..e39b345 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)

Is this new argument used at all?

>   {
>   	struct msm_display_topology topology = {0};
>   	int i, intf_count = 0;
> @@ -563,8 +564,9 @@ static struct msm_display_topology dpu_encoder_get_topology(
>   	 * 1 LM, 1 INTF
>   	 * 2 LM, 1 INTF (stream merge to support high resolution interfaces)
>   	 *
> -	 * Adding color blocks only to primary interface if available in
> -	 * sufficient number
> +	 * dspp blocks are made optional. If RM manager cannot allocate
> +	 * dspp blocks, then reservations will still go through with non dspp LM's
> +	 * so as to allow color management support via composer fallbacks
>   	 */

No, this is not the way to go.

First, RM should prefer non-DSPP-enabled LMs if DSPP blocks are not 
required.  Right now your patch makes it possible to allocate LMs, that 
have DSPP attached, for non-CTM-enabled encoder and later fail 
allocation of DSPP for the CRTC which has CTM blob attached.

Second, the decision on using DSPPs should come from 
dpu_crtc_atomic_check(). Pass 'bool need_dspp' to this function from 
dpu_atomic_check(). Fail if the need_dspp constraint can't be fulfilled.


>   	if (intf_count == 2)
>   		topology.num_lm = 2;
> @@ -573,11 +575,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 &&
> +	    (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 +643,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) {

-- 
With best wishes
Dmitry

RE: [PATCH 2/3] drm/msm/disp/dpu1: allow dspp selection for all the interfaces

[Kalyan Thota]

>-----Original Message-----
>From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>Sent: Tuesday, January 17, 2023 10:26 PM
>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: [PATCH 2/3] drm/msm/disp/dpu1: allow dspp selection for all the
>interfaces
>
>WARNING: This email originated from outside of Qualcomm. Please be wary of
>any links or attachments, and do not enable macros.
>
>On 17/01/2023 18:21, Kalyan Thota wrote:
>> Allow dspps to be populated as a requirement for all the encoder types
>> it need not be just DSI. If for any encoder the dspp allocation
>> doesn't go through then there can be an option to fallback for color
>> features.
>>
>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 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..e39b345 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)
>
>Is this new argument used at all?
>
>>   {
>>       struct msm_display_topology topology = {0};
>>       int i, intf_count = 0;
>> @@ -563,8 +564,9 @@ static struct msm_display_topology
>dpu_encoder_get_topology(
>>        * 1 LM, 1 INTF
>>        * 2 LM, 1 INTF (stream merge to support high resolution interfaces)
>>        *
>> -      * Adding color blocks only to primary interface if available in
>> -      * sufficient number
>> +      * dspp blocks are made optional. If RM manager cannot allocate
>> +      * dspp blocks, then reservations will still go through with non dspp LM's
>> +      * so as to allow color management support via composer
>> + fallbacks
>>        */
>
>No, this is not the way to go.
>
>First, RM should prefer non-DSPP-enabled LMs if DSPP blocks are not required.
>Right now your patch makes it possible to allocate LMs, that have DSPP attached,
>for non-CTM-enabled encoder and later fail allocation of DSPP for the CRTC
>which has CTM blob attached.
>
>Second, the decision on using DSPPs should come from dpu_crtc_atomic_check().
>Pass 'bool need_dspp' to this function from dpu_atomic_check(). Fail if the
>need_dspp constraint can't be fulfilled.
>
We may not get color_mgmt_changed property set during modeset commit, where as our resource allocation happens during modeset.
With this approach, dspps will get allocated on first come first serve basis

@Rob, is it possible to send color management property during modeset, in that case, we can use it for dspp allocation to the datapath. The current approach doesn't assume it.
On chrome compositor, I see that color property was being set in the subsequent commits but not in modeset.

>
>>       if (intf_count == 2)
>>               topology.num_lm = 2;
>> @@ -573,11 +575,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 &&
>> +         (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 +643,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) {
>
>--
>With best wishes
>Dmitry

Re: [PATCH 2/3] drm/msm/disp/dpu1: allow dspp selection for all the interfaces

[Dmitry Baryshkov]

On 18/01/2023 05:30, Kalyan Thota wrote:
> 
> 
>> -----Original Message-----
>> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Sent: Tuesday, January 17, 2023 10:26 PM
>> 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: [PATCH 2/3] drm/msm/disp/dpu1: allow dspp selection for all the
>> interfaces
>>
>> WARNING: This email originated from outside of Qualcomm. Please be wary of
>> any links or attachments, and do not enable macros.
>>
>> On 17/01/2023 18:21, Kalyan Thota wrote:
>>> Allow dspps to be populated as a requirement for all the encoder types
>>> it need not be just DSI. If for any encoder the dspp allocation
>>> doesn't go through then there can be an option to fallback for color
>>> features.
>>>
>>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +++++++++---------
>>>    1 file changed, 9 insertions(+), 9 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..e39b345 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)
>>
>> Is this new argument used at all?
>>
>>>    {
>>>        struct msm_display_topology topology = {0};
>>>        int i, intf_count = 0;
>>> @@ -563,8 +564,9 @@ static struct msm_display_topology
>> dpu_encoder_get_topology(
>>>         * 1 LM, 1 INTF
>>>         * 2 LM, 1 INTF (stream merge to support high resolution interfaces)
>>>         *
>>> -      * Adding color blocks only to primary interface if available in
>>> -      * sufficient number
>>> +      * dspp blocks are made optional. If RM manager cannot allocate
>>> +      * dspp blocks, then reservations will still go through with non dspp LM's
>>> +      * so as to allow color management support via composer
>>> + fallbacks
>>>         */
>>
>> No, this is not the way to go.
>>
>> First, RM should prefer non-DSPP-enabled LMs if DSPP blocks are not required.
>> Right now your patch makes it possible to allocate LMs, that have DSPP attached,
>> for non-CTM-enabled encoder and later fail allocation of DSPP for the CRTC
>> which has CTM blob attached.
>>
>> Second, the decision on using DSPPs should come from dpu_crtc_atomic_check().
>> Pass 'bool need_dspp' to this function from dpu_atomic_check(). Fail if the
>> need_dspp constraint can't be fulfilled.
>>
> We may not get color_mgmt_changed property set during modeset commit, where as our resource allocation happens during modeset.

So, you have to fix the conditions to perform LM reallocation if CTM 
usage has changed (note, color_mgmt_changed is not a correct one here).

> With this approach, dspps will get allocated on first come first serve basis

I don't think that this is what we have agreed upon.

> @Rob, is it possible to send color management property during modeset, in that case, we can use it for dspp allocation to the datapath. The current approach doesn't assume it.
> On chrome compositor, I see that color property was being set in the subsequent commits but not in modeset.
> 
>>
>>>        if (intf_count == 2)
>>>                topology.num_lm = 2;
>>> @@ -573,11 +575,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 &&
>>> +         (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 +643,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) {
>>
>> --
>> With best wishes
>> Dmitry
> 

-- 
With best wishes
Dmitry

RE: [PATCH 2/3] drm/msm/disp/dpu1: allow dspp selection for all the interfaces

[Kalyan Thota]

>-----Original Message-----
>From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>Sent: Wednesday, January 18, 2023 9:06 AM
>To: Kalyan Thota <kalyant@qti.qualcomm.com>; 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; Abhinav Kumar (QUIC)
><quic_abhinavk@quicinc.com>; Doug Anderson <dianders@google.com>
>Cc: linux-kernel@vger.kernel.org; robdclark@chromium.org;
>dianders@chromium.org; swboyd@chromium.org; Vinod Polimera (QUIC)
><quic_vpolimer@quicinc.com>
>Subject: Re: [PATCH 2/3] drm/msm/disp/dpu1: allow dspp selection for all the
>interfaces
>
>WARNING: This email originated from outside of Qualcomm. Please be wary of
>any links or attachments, and do not enable macros.
>
>On 18/01/2023 05:30, Kalyan Thota wrote:
>>
>>
>>> -----Original Message-----
>>> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> Sent: Tuesday, January 17, 2023 10:26 PM
>>> 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: [PATCH 2/3] drm/msm/disp/dpu1: allow dspp selection for
>>> all the interfaces
>>>
>>> WARNING: This email originated from outside of Qualcomm. Please be
>>> wary of any links or attachments, and do not enable macros.
>>>
>>> On 17/01/2023 18:21, Kalyan Thota wrote:
>>>> Allow dspps to be populated as a requirement for all the encoder
>>>> types it need not be just DSI. If for any encoder the dspp
>>>> allocation doesn't go through then there can be an option to
>>>> fallback for color features.
>>>>
>>>> Signed-off-by: Kalyan Thota <quic_kalyant@quicinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 18 +++++++++---------
>>>>    1 file changed, 9 insertions(+), 9 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..e39b345 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)
>>>
>>> Is this new argument used at all?
>>>
>>>>    {
>>>>        struct msm_display_topology topology = {0};
>>>>        int i, intf_count = 0;
>>>> @@ -563,8 +564,9 @@ static struct msm_display_topology
>>> dpu_encoder_get_topology(
>>>>         * 1 LM, 1 INTF
>>>>         * 2 LM, 1 INTF (stream merge to support high resolution interfaces)
>>>>         *
>>>> -      * Adding color blocks only to primary interface if available in
>>>> -      * sufficient number
>>>> +      * dspp blocks are made optional. If RM manager cannot allocate
>>>> +      * dspp blocks, then reservations will still go through with non dspp LM's
>>>> +      * so as to allow color management support via composer
>>>> + fallbacks
>>>>         */
>>>
>>> No, this is not the way to go.
>>>
>>> First, RM should prefer non-DSPP-enabled LMs if DSPP blocks are not required.
>>> Right now your patch makes it possible to allocate LMs, that have
>>> DSPP attached, for non-CTM-enabled encoder and later fail allocation
>>> of DSPP for the CRTC which has CTM blob attached.
>>>
>>> Second, the decision on using DSPPs should come from
>dpu_crtc_atomic_check().
>>> Pass 'bool need_dspp' to this function from dpu_atomic_check(). Fail
>>> if the need_dspp constraint can't be fulfilled.
>>>
>> We may not get color_mgmt_changed property set during modeset commit,
>where as our resource allocation happens during modeset.
>
>So, you have to fix the conditions to perform LM reallocation if CTM usage has
>changed (note, color_mgmt_changed is not a correct one here).
>
If I take the above approach, where should I update the new reservations as we won't be getting atomic_mode_set callback as the change is only w.r.t color management.
Sequence :
1) atomic_check on encoder
	Received a request with no ctm enabled (1LM, 0dspp, 1 intf)
	Rm will reserve 1LM and 1 intf
2) atomic_modeset on encoder
	Update the state with reservations.

3) Commit with ctm enabled ( 1 LM, 1 dspp, 1 intf)
	Here as the topology has changed, I need to re - reserve the resource freeing the earlier ones.
	But where should I update them to the state ? shall I do it as part of atomic_check for this case ?

>> With this approach, dspps will get allocated on first come first serve
>> basis
>
>I don't think that this is what we have agreed upon.
>
>> @Rob, is it possible to send color management property during modeset, in
>that case, we can use it for dspp allocation to the datapath. The current approach
>doesn't assume it.
>> On chrome compositor, I see that color property was being set in the
>subsequent commits but not in modeset.
>>
>>>
>>>>        if (intf_count == 2)
>>>>                topology.num_lm = 2;
>>>> @@ -573,11 +575,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 &&
>>>> +         (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 +643,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) {
>>>
>>> --
>>> With best wishes
>>> Dmitry
>>
>
>--
>With best wishes
>Dmitry

[PATCH 3/3] drm/msm/disp/dpu1: fail atomic check if color feature is requested with no dspp

[Kalyan Thota]

Fail atomic check if any color feature is requested with no
dspps allocated in the datapath so that composer can offload those
features.

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 4170fbe..de8d799 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1147,6 +1147,7 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
 	int left_zpos_cnt = 0, right_zpos_cnt = 0;
 	struct drm_rect crtc_rect = { 0 };
 	bool needs_dirtyfb = dpu_crtc_needs_dirtyfb(crtc_state);
+	struct dpu_crtc_mixer *mixer = cstate->mixers;
 
 	pstates = kzalloc(sizeof(*pstates) * DPU_STAGE_MAX * 4, GFP_KERNEL);
 
@@ -1173,6 +1174,16 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
 		_dpu_crtc_setup_lm_bounds(crtc, crtc_state);
 	}
 
+	if (crtc_state->color_mgmt_changed) {
+		for (i = 0; i < cstate->num_mixers; i++) {
+			if (!mixer[i].hw_dspp) {
+				DPU_DEBUG("%s: failed to get dspp for crtc%d state\n",
+						dpu_crtc->name, crtc->base.id);
+				return -EINVAL;
+			}
+		}
+	}
+
 	crtc_rect.x2 = mode->hdisplay;
 	crtc_rect.y2 = mode->vdisplay;
 
-- 
2.7.4