Message-ID: <a0c48f70-2a0f-45b0-b179-91dd544b5b59@quicinc.com>
Date: Wed, 8 Jan 2025 21:22:56 -0800
From: Abhinav Kumar <quic_abhinavk@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard
	<mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie
	<airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
        Rob Clark
	<robdclark@...il.com>, Sean Paul <sean@...rly.run>,
        Marijn Suijten
	<marijn.suijten@...ainline.org>,
        Chandan Uddaraju <chandanu@...eaurora.org>,
        Jeykumar Sankaran <jsanka@...eaurora.org>,
        Jordan Crouse
	<jordan@...micpenguin.net>,
        Sravanthi Kollukuduru <skolluku@...eaurora.org>,
        <dri-devel@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        Archit
 Taneja <architt@...eaurora.org>,
        Rajesh Yadav <ryadav@...eaurora.org>, <linux-arm-msm@...r.kernel.org>,
        <freedreno@...ts.freedesktop.org>,
        Simona
 Vetter <simona.vetter@...ll.ch>
Subject: Re: [PATCH 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from
 atomic_check()



On 1/8/2025 8:26 PM, Dmitry Baryshkov wrote:
> On Wed, Jan 08, 2025 at 08:11:27PM -0800, Abhinav Kumar wrote:
>>
>>
>> On 1/8/2025 6:27 PM, Abhinav Kumar wrote:
>>>
>>>
>>> On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
>>>> The MSM driver uses drm_atomic_helper_check() which mandates that none
>>>> of the atomic_check() callbacks toggles crtc_state->mode_changed.
>>>> Perform corresponding check before calling the drm_atomic_helper_check()
>>>> function.
>>>>
>>>> Fixes: 8b45a26f2ba9 ("drm/msm/dpu: reserve cdm blocks for writeback
>>>> in case of YUV output")
>>>> Reported-by: Simona Vetter <simona.vetter@...ll.ch>
>>>> Closes:
>>>> https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32
>>>> +++++++++++++++++++++++++----
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ++++
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 26
>>>> +++++++++++++++++++++++
>>>>    drivers/gpu/drm/msm/msm_atomic.c            | 13 +++++++++++-
>>>>    drivers/gpu/drm/msm/msm_kms.h               |  7 +++++++
>>>>    5 files changed, 77 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 209e6fb605b2d8724935b62001032e7d39540366..b7c3aa8d0e2ca58091deacdeaccb0819d2bf045c
>>>> 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -753,6 +753,34 @@ static void
>>>> dpu_encoder_assign_crtc_resources(struct dpu_kms *dpu_kms,
>>>>        cstate->num_mixers = num_lm;
>>>>    }
>>>> +/**
>>>> + * dpu_encoder_virt_check_mode_changed: check if full modeset is
>>>> required
>>>> + * @drm_enc:    Pointer to drm encoder structure
>>>> + * @crtc_state:    Corresponding CRTC state to be checked
>>>> + * @conn_state: Corresponding Connector's state to be checked
>>>> + *
>>>> + * Check if the changes in the object properties demand full mode set.
>>>> + */
>>>> +int dpu_encoder_virt_check_mode_changed(struct drm_encoder *drm_enc,
>>>> +                    struct drm_crtc_state *crtc_state,
>>>> +                    struct drm_connector_state *conn_state)
>>>> +{
>>>> +    struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
>>>> +    struct msm_display_topology topology;
>>>> +
>>>> +    DPU_DEBUG_ENC(dpu_enc, "\n");
>>>> +
>>>> +    /* Using mode instead of adjusted_mode as it wasn't computed yet */
>>>> +    topology = dpu_encoder_get_topology(dpu_enc, &crtc_state->mode,
>>>> crtc_state, conn_state);
>>>> +
>>>> +    if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
>>>> +        crtc_state->mode_changed = true;
>>>> +    else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
>>>> +        crtc_state->mode_changed = true;
>>>> +
>>>> +    return 0;
>>>> +}
>>>
>>> How will this work exactly?
>>>
>>> needs_cdm is set in the encoder's atomic_check which is called inside
>>> drm_atomic_helper_check(). But this function is called before that.
>>>
>>> So needs_cdm will never hit.
>>>
>>
>> Sorry, my bad. after change (4) of this series needs_cdm is also populated
>> within  dpu_encoder_get_topology().
>>
>> To follow up on https://patchwork.freedesktop.org/patch/629231/?series=137975&rev=4#comment_1148651
>>
>> So is the plan for CWB to add a dpu_crtc_check_mode_changed() like
>> dpu_encoder's and call it?
> 
> I think dpu_encoder_virt_check_mode_changed() would transform into the
> dpu_crtc_check_mode_changed() together with one of the patches that
> moves resource allocation and refactors topology handling.
>

hmm we need the cur_master for cdm. That will not be accessible in 
dpu_crtc.c so we will end up with a separate 
dpu_crtc_check_mode_changed() for CWB from what I see. We will discuss 
it further when we re-post CWB.

But overall, I think we can make CWB work on top of this.

Hence,

Reviewed-by: Abhinav Kumar <quic_abhinavk@...cinc.com>

I do not know how important patch 2 is for this series and I would 
prefer not delaying CWB even more than what it already has been.

If we cannot reach a conclusion on patch 2, can you break that one out 
of this series so that the rest of it is ready to land?

>>
>>
>>>
>>>> +
>>>>    static int dpu_encoder_virt_atomic_check(
>>>>            struct drm_encoder *drm_enc,
>>>>            struct drm_crtc_state *crtc_state,
> 

Powered by blists - more mailing lists