[PATCH] drm/msm/dpu: Force disabling commits to take non-async path

[Abhinav Kumar]

On 1/8/2025 6:14 PM, Dmitry Baryshkov wrote:
> On Thu, 9 Jan 2025 at 03:45, Rob Clark <robdclark at gmail.com> wrote:
>>
>> On Wed, Jan 8, 2025 at 2:58 PM Jessica Zhang <quic_jesszhan at quicinc.com> wrote:
>>>
>>> Force commit that are disabling a plane in the async_crtc to take the
>>> non-async commit tail path.
>>>
>>> In cases where there are two consecutive async cursor updates (one
>>> regular non-NULL update followed by a disabling NULL FB update), it is
>>> possible for the second NULL update to not be queued (due to the
>>> pending_crtc_mask check) or otherwise not be run before the cursor FB is
>>> deallocated by drm_atomic_helper_cleanup_planes(). This would cause a
>>> context fault since the hardware would try to fetch the old plane state
>>> with the stale FB address.
>>>
>>> Avoid this issue by forcing cursor updates that will disable the cursor
>>> plane to be blocking commits. This will ensure that hardware clears and
>>> stops fetching the FB source address before the driver deallocates the FB
>>>
>>> Fixes: 2d99ced787e3 ("drm/msm: async commit support")
>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/msm_atomic.c | 13 +++++++++++++
>>>   1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
>>> index 9c45d641b5212c11078ab38c13a519663d85e10a..ddc74c68148c643d34ca631dd28d4cdc2b8c7dc0 100644
>>> --- a/drivers/gpu/drm/msm/msm_atomic.c
>>> +++ b/drivers/gpu/drm/msm/msm_atomic.c
>>> @@ -142,6 +142,7 @@ static bool can_do_async(struct drm_atomic_state *state,
>>>          struct drm_connector_state *connector_state;
>>>          struct drm_connector *connector;
>>>          struct drm_crtc_state *crtc_state;
>>> +       struct drm_plane_state *plane_state;
>>>          struct drm_crtc *crtc;
>>>          int i, num_crtcs = 0;
>>>
>>> @@ -162,6 +163,18 @@ static bool can_do_async(struct drm_atomic_state *state,
>>>                  *async_crtc = crtc;
>>>          }
>>>
>>> +       /*
>>> +        * Force a blocking commit if the cursor is being disabled. This is to
>>> +        * ensure that the registers are cleared and hardware doesn't try to
>>> +        * fetch from a stale address.
>>> +        */
>>> +       if (*async_crtc) {
>>> +               plane_state = drm_atomic_get_new_plane_state(state,
>>> +                                                            (*async_crtc)->cursor);
>>> +               if (plane_state && !plane_state->fb)
>>> +                       return false;
>>
>> hmm, I suppose we want the same even if the fb changes?  Or
>> alternatively somewhere hold an extra ref to the backing obj until hw
>> has finished scanout?
>

Hi Rob

Do you mean we need to also check if old_plane_state->fb != 
new_plane_state->fb, then use blocking commit?

We can try that out.

holding extra ref gets tricky IMO. In this way, the calls are balanced 
in places we know.

> I think a more correct approach would be to run a worker, waiting for
> the commit to happen and then freeing the FBs.
> 

Hi Dmitry

This option was tried . It gets very messy to handle it this way. Then 
we realized that, the worker is going to try to do the same thing a 
blocking commit does which is to wait for hw to finish scanout and then 
cleanup planes. Hence this was preferred and is better IMO.

>>
>> BR,
>> -R
>>
>>> +       }
>>> +
>>>          return true;
>>>   }
>>>
>>>
>>> ---
>>> base-commit: 866e43b945bf98f8e807dfa45eca92f931f3a032
>>> change-id: 20250108-async-disable-fix-cc1b9a1d5b19
>>>
>>> Best regards,
>>> --
>>> Jessica Zhang <quic_jesszhan at quicinc.com>
>>>
> 
> 
>