| Message ID | 1680271114-1534-2-git-send-email-quic_vpolimer@quicinc.com |
|---|---|
| State | Accepted |
| Commit | 501bd8dea55d641422d2a06b94f7276f21f7be21 |
| Headers | show |
| Series | Fixes for PSR | expand |
> On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <quic_vpolimer@quicinc.com> > wrote: > > > > While in virtual terminal mode with PSR enabled, there will be > > no atomic commits triggered without dirty_fb being set. This > > will create a notion of no screen update. Allow atomic commit > > when dirty_fb ioctl is issued, so that it can trigger a PSR exit > > and shows update on the screen. > > Will this impact non-VT workloads? If I remember correctly, we added > dirty_fb handling to prevent the framework from limiting the page > flips to vblank events (in DSI video mode). > From the use case referred in the commit text ( 9e4dde28e drm/msm: Avoid dirtyfb stalls on video mode displays (v2)). There can be an impact on the workload. I can think of two solutions: 1) Add modparam to configure PSR wait time (SELF_REFRESH_AVG_SEED_MS 200) and application can set it to "-1" if they are operating on dirty_fb 2) In msm drivers, disable psr if dirty_fb_ioctl is requested from the framework and re-enable it during regular commit. This will involve copying dirtyflags from drm fb ->msm_fb->dpu_plane and add atomic_check failure if ( dirty_flags && self_refresh_active). Please let me know your thoughts on the above two. Thanks, Vinod. > > > > Reported-by: Bjorn Andersson <andersson@kernel.org> > > Link: > https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/ > > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > index ab636da..96f645e 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > > @@ -1158,6 +1158,9 @@ static bool dpu_crtc_needs_dirtyfb(struct > drm_crtc_state *cstate) > > struct drm_crtc *crtc = cstate->crtc; > > struct drm_encoder *encoder; > > > > + if (cstate->self_refresh_active) > > + return true; > > + > > drm_for_each_encoder_mask (encoder, crtc->dev, cstate- > >encoder_mask) { > > if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_CMD) { > > return true; > > -- > > 2.7.4 > > > > > -- > With best wishes > Dmitry
On 31/03/2023 17:45, Dmitry Baryshkov wrote: > On Fri, 31 Mar 2023 at 16:59, Vinod Polimera <quic_vpolimer@quicinc.com> wrote: >> >> While in virtual terminal mode with PSR enabled, there will be >> no atomic commits triggered without dirty_fb being set. This >> will create a notion of no screen update. Allow atomic commit >> when dirty_fb ioctl is issued, so that it can trigger a PSR exit >> and shows update on the screen. > > Will this impact non-VT workloads? If I remember correctly, we added > dirty_fb handling to prevent the framework from limiting the page > flips to vblank events (in DSI video mode). Actually, this is kind of stupid. If we care about the workload of this pipe, then it is being updated, which means it is not in SR mode, self_refresh_active = false. Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >> >> Reported-by: Bjorn Andersson <andersson@kernel.org> >> Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/ >> Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> index ab636da..96f645e 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c >> @@ -1158,6 +1158,9 @@ static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state *cstate) >> struct drm_crtc *crtc = cstate->crtc; >> struct drm_encoder *encoder; >> >> + if (cstate->self_refresh_active) >> + return true; >> + >> drm_for_each_encoder_mask (encoder, crtc->dev, cstate->encoder_mask) { >> if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_CMD) { >> return true; >> -- >> 2.7.4 >> > >
Hi, On Fri, Mar 31, 2023 at 6:59 AM Vinod Polimera <quic_vpolimer@quicinc.com> wrote: > > While in virtual terminal mode with PSR enabled, there will be > no atomic commits triggered without dirty_fb being set. This > will create a notion of no screen update. Allow atomic commit > when dirty_fb ioctl is issued, so that it can trigger a PSR exit > and shows update on the screen. > > Reported-by: Bjorn Andersson <andersson@kernel.org> > Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/ > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 +++ > 1 file changed, 3 insertions(+) I can confirm that this patch plus patch #2 fixes the typing problems seen on VT2 on a Chromebook using PSR. Tested-by: Douglas Anderson <dianders@chromium.org> -Doug
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index ab636da..96f645e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -1158,6 +1158,9 @@ static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state *cstate) struct drm_crtc *crtc = cstate->crtc; struct drm_encoder *encoder; + if (cstate->self_refresh_active) + return true; + drm_for_each_encoder_mask (encoder, crtc->dev, cstate->encoder_mask) { if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_CMD) { return true;
While in virtual terminal mode with PSR enabled, there will be no atomic commits triggered without dirty_fb being set. This will create a notion of no screen update. Allow atomic commit when dirty_fb ioctl is issued, so that it can trigger a PSR exit and shows update on the screen. Reported-by: Bjorn Andersson <andersson@kernel.org> Link: https://lore.kernel.org/all/20230326162723.3lo6pnsfdwzsvbhj@ripper/ Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com> --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 3 +++ 1 file changed, 3 insertions(+)