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

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 6/18/2024 8:26 PM, Dmitry Baryshkov wrote:
> On Wed, 19 Jun 2024 at 01:56, Abhinav Kumar <quic_abhinavk@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@linaro.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 --
>>>>    1 file changed, 2 deletions(-)
>>>>
>>>
>>> Reviewed-by: Abhinav Kumar <quic_abhinavk@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.

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.