[PATCH v3 7/9] drm/msm/dpu: drop _dpu_crtc_check_and_setup_lm_bounds from atomic_begin

[Dmitry Baryshkov]

On Wed, 19 Jun 2024 at 20:10, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>
>
>
> On 6/18/2024 8:26 PM, Dmitry Baryshkov wrote:
> > On Wed, 19 Jun 2024 at 01:56, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
> >> On 6/13/2024 4:20 PM, Abhinav Kumar wrote:
> >>> On 6/13/2024 3:36 PM, Dmitry Baryshkov wrote:
> >>>> The dpu_crtc_atomic_check() already calls the function
> >>>> _dpu_crtc_check_and_setup_lm_bounds().  There is no need to call it
> >>>> again from dpu_crtc_atomic_begin().
> >>>>
> >>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> >>>> ---
> >>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 --
> >>>>    1 file changed, 2 deletions(-)
> >>>>
> >>>
> >>> Reviewed-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
> >>
> >>
> >> This change is causing a small regression on sc7280 chromebook.
> >>
> >> I have tested and concluded that this is causing the chrome boot
> >> animation to disappear.
> >>
> >> I have tested a couple of times and without this change it works fine.
> >>
> >> If this change was meant as an optimization, can we drop this one and
> >> investigate later why this is causing one? I have not spent time
> >> investigating why it happened. Rest of the series works well and I dont
> >> see any dependency as such. Let me know if that works for you. Otherwise
> >> I will have to spend a little more time on this patch and why chrome
> >> compositor does not like this for the animation screen.
> >
> > Oh, my. Thank you for the test!
> > I think I know what's happening. The cstate->num_mixers gets set only
> > in dpu_encoder_virt_atomic_mode_set(). So during
> > dpu_crtc_atomic_check() we don't have cstate->num_mixers is stale (and
> > if it is 0, the check is skipped).
> >
>
> Yes, it is a possible explanation for this.
>
> > I guess I'll have to move cstate->mixers[] and cstate->num_mixers
> > assignment to the dpu_encoder_virt_atomic_check(). And maybe we should
> > start thinking about my old idea of moving resource allocation to the
> > CRTC code.
> >
>
> I wonder if thats the right fix though because it seems correct to me
> that num_mixers is set in mode_set after the atomic_check phase.

The state should be consistent after the atomic_check(). Currently it
is not. cstate->num_mixers is not correct until mode_set().

>
> Perhaps the right way would be to breakup check_and_set() to check() and
> set() respectively and call only the check() part in atomic_check() and
> keep the set() part in atomic_begin to avoid duplication.
>
> Either way, I think we should re-visit this as this patch by itself is
> an optimization and I am totally fine if you want to merge the rest of
> this series just dropping this one for now.

The patch itself might be an optimization, but it pointed out the
actual issue with cstate->num_mixers.

-- 
With best wishes
Dmitry