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: Wednesday, January 18, 2023 9:06 AM
>To: Kalyan Thota <kalyant@xxxxxxxxxxxxxxxx>; Kalyan Thota (QUIC)
><quic_kalyant@xxxxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-arm-
>msm@xxxxxxxxxxxxxxx; freedreno@xxxxxxxxxxxxxxxxxxxxx;
>devicetree@xxxxxxxxxxxxxxx; Abhinav Kumar (QUIC)
><quic_abhinavk@xxxxxxxxxxx>; Doug Anderson <dianders@xxxxxxxxxx>
>Cc: linux-kernel@xxxxxxxxxxxxxxx; robdclark@xxxxxxxxxxxx;
>dianders@xxxxxxxxxxxx; swboyd@xxxxxxxxxxxx; Vinod Polimera (QUIC)
><quic_vpolimer@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 18/01/2023 05:30, Kalyan Thota wrote:
>>
>>
>>> -----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.
>
>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