Message-ID: <a098b6f9-547d-42c7-b4f5-91762dc7c631@quicinc.com>
Date: Tue, 4 Mar 2025 11:38:24 -0800
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Jessica Zhang
	<quic_jesszhan@...cinc.com>
CC: Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
        "Marijn
 Suijten" <marijn.suijten@...ainline.org>,
        David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
        <linux-arm-msm@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
        <freedreno@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] drm/msm/dpu: Force modeset if new CTLs have been
 reserved



On 3/3/2025 9:32 PM, Dmitry Baryshkov wrote:
> On Tue, 4 Mar 2025 at 03:44, Jessica Zhang <quic_jesszhan@...cinc.com> wrote:
>>
>>
>>
>> On 3/3/2025 3:49 PM, Dmitry Baryshkov wrote:
>>> On Mon, Mar 03, 2025 at 10:28:00AM -0800, Jessica Zhang wrote:
>>>> If new CTLs are reserved by CRTC but atomic_enable() is skipped, the
>>>> encoders will configure the stale CTL instead of the newly reserved one.
>>>
>>> The CTLs are propagates in .atomic_mode_set(), not in .atomic_enable().
>>
>> Hi Dmitry,
>>
>> Yes, sorry mixed up the two function ops here and in my reply in the CWB
>> thread.
>>
>>>
>>>>
>>>> Avoid this by setting mode_changed to true if new CTLs have been
>>>> reserved by CRTC.
>>>
>>> This looks very strange. First we reserve new CTLs when there is a
>>> modeset requested. Then on one of the next commits we detect that
>>> encoder has stale CTLs and try to upgrade the commit to full modeset
>>> (while the user might not have .allow_modeset set to true for whatever
>>> reason, e.g. because only ACTIVE is changed).
>>
>> Ah I see what you mean. I think this is an issue with how/when we're
>> calling dpu_rm_reserve(). Since RM reservation is tied to
>> atomic_check(), we aren't able to force a modeset based on HW block
>> reservation. The only reason we were able to avoid this issue with
>> needs_cdm is because needs_cdm didn't depend on the CDM HW block index.
>>
>> I think there's not really a good way to avoid this other than flipping
>> the order of the msm_atomic_check to drm_helper_atomic_check ->
>> dpu_kms.check_mode_changed -> drm_atomic_helper_check_modeset().
> 
> No-no-no. This would require a full drm_atomic_helper_check() call
> again, after the check_mode_changed() callback. But again, this should
> not be required at all. The whole point of .check_mode_changed() is to
> be called before performing full atomic_check() chains.
> 

Right but the documentation also allows calling 
drm_atomic_helper_check_modeset() again. We are looking at all options 
even moving forward and not just this issue.

>>
>> What do you think? It seems to be valid given the examples in the DRM
>> docs [1]
>>
>> [1]
>> https://elixir.bootlin.com/linux/v6.13.5/source/drivers/gpu/drm/drm_atomic_helper.c#L610
>>
>>>
>>> Could you please check if the following change fixes the issue: in
>>> crtc_set_mode() replace the raw !new_crtc_state->mode_changed check with
>>> the drm_atomic_crtc_needs_modeset() call?
>>
>> This also fixes the DPMS failures. IIRC Abhinav had suggested a similar
>> change to fix a different issue [2] and you gave some feedback on
>> avoiding mode_set() for enable/disable calls which don't have mode_changed.
> 
> After reading the documentation for
> drm_encoder_helper_funcs.atomic_mode_set() and looking around, I think
> the issue is in the handling of the DPMS functions. I might have a fix
> for the issue.
> 
>> Also, while this may fix the CWB CI failures, wouldn't the issue still
>> remain regarding how to force modeset for changes in HW block reservation?
> 
> I think it is the other way around: HW block reservation is only
> changed if there is a modeset. I'm currently testing my theory :-) We
> were performing HW reassignment if drm_atomic_crtc_needs_modeset() was
> true. However this function returns true in one of the cases, where
> there is no actual modeset happening (and it's even documented this
> way) - when only DPMS call has happened (in other words, when
> .active_changed = true, but two other bits are false). It is required
> not to reassign HW resources in such a case. So, I think, a correct
> fix is to change the condition in dpu_crtc_atomic_check().
> 

Yes, Jessica had also suggested this option. This will work because now 
the resource re-assignment will not happen and hence will avoid the 
issue. The documentation of DPMS was not fully clear. So it said, the 
same thing you mentioned, that when active has changed there is no need 
to reassign hardware resources but I was not sure if that would impact 
normal suspend/resume because across suspend/resume hardware resources 
need to be cleared / re-assigned. I do still think that, even if this 
also works, we will still run into issues when we will need to force a 
mode_changed based on resource assignment of other encoder based blocks 
such as DSC or PP etc.

>>
>> [2] https://gitlab.freedesktop.org/drm/msm/-/issues/59
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>
>>>>
>>>> Note: This patch only adds tracking for the CTL reservation, but eventually
>>>> all HW blocks used by encoders (i.e. DSC, PINGPONG, CWB) should have a
>>>> similar check to avoid the same issue.
>>>>
>>>> Suggested-by: Abhinav Kumar <quic_abhinavk@...cinc.com>
>>>> Closes: https://lists.freedesktop.org/archives/freedreno/2025-February/036719.html
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@...cinc.com>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 13 +++++++++++++
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 ++++++++++++
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  1 +
>>>>    3 files changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> index 4073d821158c0..a1a8be8f5ab9f 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>>> @@ -1406,19 +1406,32 @@ int dpu_crtc_check_mode_changed(struct drm_crtc_state *old_crtc_state,
>>>>       struct drm_crtc *crtc = new_crtc_state->crtc;
>>>>       bool clone_mode_enabled = drm_crtc_in_clone_mode(old_crtc_state);
>>>>       bool clone_mode_requested = drm_crtc_in_clone_mode(new_crtc_state);
>>>> +    struct dpu_crtc_state *cstate = to_dpu_crtc_state(new_crtc_state);
>>>> +    uint32_t enc_ctl_mask = 0;
>>>> +    uint32_t crtc_ctl_mask = 0;
>>>> +    struct dpu_crtc_mixer *m;
>>>>
>>>>       DRM_DEBUG_ATOMIC("%d\n", crtc->base.id);
>>>>
>>>> +    for (int i = 0; i < cstate->num_mixers; i++) {
>>>> +            m = &cstate->mixers[i];
>>>> +            crtc_ctl_mask |= BIT(m->lm_ctl->idx - CTL_0);
>>>> +    }
>>>> +
>>>>       /* there might be cases where encoder needs a modeset too */
>>>>       drm_for_each_encoder_mask(drm_enc, crtc->dev, new_crtc_state->encoder_mask) {
>>>>               if (dpu_encoder_needs_modeset(drm_enc, new_crtc_state->state))
>>>>                       new_crtc_state->mode_changed = true;
>>>> +            enc_ctl_mask |= dpu_encoder_get_ctls(drm_enc);
>>>>       }
>>>>
>>>>       if ((clone_mode_requested && !clone_mode_enabled) ||
>>>>           (!clone_mode_requested && clone_mode_enabled))
>>>>               new_crtc_state->mode_changed = true;
>>>>
>>>> +    if (crtc_ctl_mask != enc_ctl_mask)
>>>> +            new_crtc_state->mode_changed = true;
>>>> +
>>>>       return 0;
>>>>    }
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index a61598710acda..2f3101caeba91 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -188,6 +188,7 @@ struct dpu_encoder_virt {
>>>>
>>>>       unsigned int dsc_mask;
>>>>       unsigned int cwb_mask;
>>>> +    unsigned int ctl_mask;
>>>>
>>>>       bool intfs_swapped;
>>>>
>>>> @@ -707,6 +708,13 @@ void dpu_encoder_update_topology(struct drm_encoder *drm_enc,
>>>>       }
>>>>    }
>>>>
>>>> +uint32_t dpu_encoder_get_ctls(struct drm_encoder *drm_enc)
>>>> +{
>>>> +    struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
>>>> +
>>>> +    return dpu_enc->ctl_mask;
>>>> +}
>>>> +
>>>>    bool dpu_encoder_needs_modeset(struct drm_encoder *drm_enc, struct drm_atomic_state *state)
>>>>    {
>>>>       struct drm_connector *connector;
>>>> @@ -1155,6 +1163,7 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>>>       bool is_cwb_encoder;
>>>>       unsigned int dsc_mask = 0;
>>>>       unsigned int cwb_mask = 0;
>>>> +    unsigned int ctl_mask = 0;
>>>>       int i;
>>>>
>>>>       if (!drm_enc) {
>>>> @@ -1245,11 +1254,14 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>>>>                               "no ctl block assigned at idx: %d\n", i);
>>>>                       return;
>>>>               }
>>>> +            ctl_mask |= BIT(phys->hw_ctl->idx - CTL_0);
>>>>
>>>>               phys->cached_mode = crtc_state->adjusted_mode;
>>>>               if (phys->ops.atomic_mode_set)
>>>>                       phys->ops.atomic_mode_set(phys, crtc_state, conn_state);
>>>>       }
>>>> +
>>>> +    dpu_enc->ctl_mask = ctl_mask;
>>>>    }
>>>>
>>>>    static void _dpu_encoder_virt_enable_helper(struct drm_encoder *drm_enc)
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> index ca1ca2e51d7ea..70b03743dc346 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>>>> @@ -91,6 +91,7 @@ bool dpu_encoder_needs_modeset(struct drm_encoder *drm_enc, struct drm_atomic_st
>>>>
>>>>    void dpu_encoder_prepare_wb_job(struct drm_encoder *drm_enc,
>>>>               struct drm_writeback_job *job);
>>>> +uint32_t dpu_encoder_get_ctls(struct drm_encoder *drm_enc);
>>>>
>>>>    void dpu_encoder_cleanup_wb_job(struct drm_encoder *drm_enc,
>>>>               struct drm_writeback_job *job);
>>>>
>>>> ---
>>>> base-commit: 866e43b945bf98f8e807dfa45eca92f931f3a032
>>>> change-id: 20250228-force-modeset-hw-ctl-d02b80a2bb4c
>>>> prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2
>>>> prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6
>>>> prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7
>>>> prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf
>>>> prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da
>>>> prerequisite-change-id: 20250209-dpu-c3fac78fc617:v2
>>>> prerequisite-patch-id: c84d2b4b06be06384968429085d1e8ebae23a583
>>>> prerequisite-patch-id: fb8ea7b9e7c85fabd27589c6551108382a235002
>>>> prerequisite-change-id: 20250211-dither-disable-b77b1e31977f:v1
>>>> prerequisite-patch-id: 079e04296212b4b83d51394b5a9b5eea6870d98a
>>>> prerequisite-change-id: 20240618-concurrent-wb-97d62387f952:v6
>>>> prerequisite-patch-id: b52034179741dc182aea9411fd446e270fdc69d1
>>>> prerequisite-patch-id: bc472765a7d5214691f3d92696cc8b0119f3252e
>>>> prerequisite-patch-id: c959bc480e96b04297ebaf30fea3a68bbac69da6
>>>> prerequisite-patch-id: f7db8449b241a41faac357d9257f8c7cb16503ec
>>>> prerequisite-patch-id: 7beb73131d0ab100f266fcd3c1f67c818a3263f4
>>>> prerequisite-patch-id: c08cbb5cf4e67e308afd61fdad6684b89429d3b6
>>>> prerequisite-patch-id: a4e343143b8fbe98ae4aa068cc459c750105eb9d
>>>> prerequisite-patch-id: 1d09edcf12ef7e7ab43547eefacae5b604b698e9
>>>> prerequisite-patch-id: 0008f9802bfd3c5877267666cceb7608203e5830
>>>> prerequisite-patch-id: 49402eb767c97915faf2378c5f5d05ced2dcfdac
>>>> prerequisite-patch-id: 522be2a6b5fe4e3a2d609526bb1539f9bc6f828f
>>>> prerequisite-patch-id: 031da00d0fffd522f74d682a551362f3ecda0c71
>>>> prerequisite-patch-id: 9454cec22231a8f3f01c33d52a5df3e26dd88287
>>>> prerequisite-patch-id: 7edbeaace3549332e581bee3183a76b0e4d18163
>>>>
>>>> Best regards,
>>>> --
>>>> Jessica Zhang <quic_jesszhan@...cinc.com>
>>>>
>>>
>>> --
>>> With best wishes
>>> Dmitry
>>
> 
> 

Powered by blists - more mailing lists