[12/25] drm/rcar-du: Annotate dma-fence critical section in commit path
diff mbox series

Message ID 20200707201229.472834-13-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • dma-fence annotations, round 3
Related show

Commit Message

Daniel Vetter July 7, 2020, 8:12 p.m. UTC
Ends right after drm_atomic_helper_commit_hw_done(), absolutely
nothing fancy going on here.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Laurent Pinchart July 7, 2020, 11:32 p.m. UTC | #1
Hi Daniel,

Thank you for the patch.

On Tue, Jul 07, 2020 at 10:12:16PM +0200, Daniel Vetter wrote:
> Ends right after drm_atomic_helper_commit_hw_done(), absolutely
> nothing fancy going on here.

Just looking at this patch and the commit message, I have no idea what
this does, and why. It would be nice to expand the commit message to
give some more context, and especially explain why ending signalling
right after drm_atomic_helper_commit_hw_done() is the right option.

I suppose I'll have to check the whole series in the meantime :-)

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 482329102f19..42c5dc588435 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -391,6 +391,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_crtc *crtc;
>  	unsigned int i;
> +	bool fence_cookie = dma_fence_begin_signalling();

Can this be moved right before the
drm_atomic_helper_commit_modeset_disables() call ?

>  
>  	/*
>  	 * Store RGB routing to DPAD0 and DPAD1, the hardware will be configured
> @@ -417,6 +418,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
>  
>  	drm_atomic_helper_commit_hw_done(old_state);
> +	dma_fence_end_signalling(fence_cookie);
>  	drm_atomic_helper_wait_for_flip_done(dev, old_state);
>  
>  	drm_atomic_helper_cleanup_planes(dev, old_state);
Daniel Vetter July 14, 2020, 8:39 a.m. UTC | #2
On Wed, Jul 08, 2020 at 02:32:40AM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Tue, Jul 07, 2020 at 10:12:16PM +0200, Daniel Vetter wrote:
> > Ends right after drm_atomic_helper_commit_hw_done(), absolutely
> > nothing fancy going on here.
> 
> Just looking at this patch and the commit message, I have no idea what
> this does, and why. It would be nice to expand the commit message to
> give some more context, and especially explain why ending signalling
> right after drm_atomic_helper_commit_hw_done() is the right option.
> 
> I suppose I'll have to check the whole series in the meantime :-)

Yes first three patches. They should land in the next few days. The
explanation is a few pages long, not sure that makes much sense to
copypaste into every driver patch here.

Also patch 16 has some more explanation specific for display.

> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > Cc: linux-renesas-soc@vger.kernel.org
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > index 482329102f19..42c5dc588435 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -391,6 +391,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> >  	struct drm_crtc_state *crtc_state;
> >  	struct drm_crtc *crtc;
> >  	unsigned int i;
> > +	bool fence_cookie = dma_fence_begin_signalling();
> 
> Can this be moved right before the
> drm_atomic_helper_commit_modeset_disables() call ?

The critical section starts even before this function starts, but for
composability each part is individually annotated. That's why I've put it
as the very first thing in every patch. Currently there's nothing between
the funciton start and drm_atomic_helper_commit_modeset_disables which
could break dma-fence rules, but the entire point of annotations is to not
have to manually prove stuff like this. Wrapping it all is the point here.

Does that make sense?

Also, what I'm realling looking for is testing with lockdep enabled.
Neither me nor you is going to catch issues with review here :-)
-Daniel

> 
> >  
> >  	/*
> >  	 * Store RGB routing to DPAD0 and DPAD1, the hardware will be configured
> > @@ -417,6 +418,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> >  	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> >  
> >  	drm_atomic_helper_commit_hw_done(old_state);
> > +	dma_fence_end_signalling(fence_cookie);
> >  	drm_atomic_helper_wait_for_flip_done(dev, old_state);
> >  
> >  	drm_atomic_helper_cleanup_planes(dev, old_state);
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 482329102f19..42c5dc588435 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -391,6 +391,7 @@  static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
 	unsigned int i;
+	bool fence_cookie = dma_fence_begin_signalling();
 
 	/*
 	 * Store RGB routing to DPAD0 and DPAD1, the hardware will be configured
@@ -417,6 +418,7 @@  static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
 	drm_atomic_helper_commit_hw_done(old_state);
+	dma_fence_end_signalling(fence_cookie);
 	drm_atomic_helper_wait_for_flip_done(dev, old_state);
 
 	drm_atomic_helper_cleanup_planes(dev, old_state);