| Message ID | 20240614-dpu-mode-config-width-v3-7-29ec4069c99b@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dpu: be more friendly to X.org | expand |
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>
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.
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). 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.
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.
On Wed, 19 Jun 2024 at 20:10, Abhinav Kumar <quic_abhinavk@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@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. 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.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index b0d81e8ea765..5dbf5254d310 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -809,8 +809,6 @@ static void dpu_crtc_atomic_begin(struct drm_crtc *crtc, DRM_DEBUG_ATOMIC("crtc%d\n", crtc->base.id); - _dpu_crtc_check_and_setup_lm_bounds(crtc, crtc->state); - /* encoder will trigger pending mask now */ drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) dpu_encoder_trigger_kickoff_pending(encoder);
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(-)