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

[Kalyan Thota]

>-----Original Message-----
>From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>Sent: Tuesday, January 17, 2023 10:26 PM
>To: Kalyan Thota (QUIC) <quic_kalyant@xxxxxxxxxxx>; dri-
>devel@xxxxxxxxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx;
>freedreno@xxxxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
>Cc: linux-kernel@xxxxxxxxxxxxxxx; robdclark@xxxxxxxxxxxx;
>dianders@xxxxxxxxxxxx; swboyd@xxxxxxxxxxxx; Vinod Polimera (QUIC)
><quic_vpolimer@xxxxxxxxxxx>; Abhinav Kumar (QUIC)
><quic_abhinavk@xxxxxxxxxxx>
>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@xxxxxxxxxxx>
>> ---
>>   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