[v2,14/24] drm/msm: dpu: Grab the modeset locks in frame_event

Submitted by Sean Paul on Nov. 16, 2018, 6:42 p.m.

Details

Message ID 20181116184238.170034-15-sean@poorly.run
State New
Headers show
Series "drm/msm: Various dpu locking and legacy cleanups" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Sean Paul Nov. 16, 2018, 6:42 p.m.
From: Sean Paul <seanpaul@chromium.org>

This patch wraps dpu_core_perf_crtc_release_bw() with modeset locks
since it digs into the state objects.

Changes in v2:
- None

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 ++
 1 file changed, 2 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 80de5289ada3..156f4c77ca44 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -335,7 +335,9 @@  static void dpu_crtc_frame_event_work(struct kthread_work *work)
 			/* release bandwidth and other resources */
 			trace_dpu_crtc_frame_event_done(DRMID(crtc),
 							fevent->event);
+			drm_modeset_lock_all(crtc->dev);
 			dpu_core_perf_crtc_release_bw(crtc);
+			drm_modeset_unlock_all(crtc->dev);
 		} else {
 			trace_dpu_crtc_frame_event_more_pending(DRMID(crtc),
 								fevent->event);

Comments

On 2018-11-16 10:42, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> This patch wraps dpu_core_perf_crtc_release_bw() with modeset locks
> since it digs into the state objects.
> 
> Changes in v2:
> - None
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>

>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 80de5289ada3..156f4c77ca44 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -335,7 +335,9 @@ static void dpu_crtc_frame_event_work(struct
> kthread_work *work)
>  			/* release bandwidth and other resources */
>  			trace_dpu_crtc_frame_event_done(DRMID(crtc),
>  							fevent->event);
> +			drm_modeset_lock_all(crtc->dev);
>  			dpu_core_perf_crtc_release_bw(crtc);
> +			drm_modeset_unlock_all(crtc->dev);
We might need to revisit this locking when we measure for performance as 
it
could block the incoming frame locking.

>  		} else {
> 
> trace_dpu_crtc_frame_event_more_pending(DRMID(crtc),
> 
> fevent->event);
On Fri, Nov 16, 2018 at 12:02:54PM -0800, Jeykumar Sankaran wrote:
> On 2018-11-16 10:42, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > This patch wraps dpu_core_perf_crtc_release_bw() with modeset locks
> > since it digs into the state objects.
> > 
> > Changes in v2:
> > - None
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>
> 
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 80de5289ada3..156f4c77ca44 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -335,7 +335,9 @@ static void dpu_crtc_frame_event_work(struct
> > kthread_work *work)
> >  			/* release bandwidth and other resources */
> >  			trace_dpu_crtc_frame_event_done(DRMID(crtc),
> >  							fevent->event);
> > +			drm_modeset_lock_all(crtc->dev);
> >  			dpu_core_perf_crtc_release_bw(crtc);
> > +			drm_modeset_unlock_all(crtc->dev);
> We might need to revisit this locking when we measure for performance as it
> could block the incoming frame locking.
> 

Definitely something to keep an eye on.

That said, we really do want it to block the incoming frame since we're reducing
bw. It would be really unfortunate if this happened concurrently with a commit.
If this _does_ cause a performance problem, we should really investigate the
criteria for reducing bw.

Sean

> >  		} else {
> > 
> > trace_dpu_crtc_frame_event_more_pending(DRMID(crtc),
> > 
> > fevent->event);
> 
> -- 
> Jeykumar S