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