[PATCH] drm: rcar-du: track dma-buf fences

[Ucan, Emre (ADITG/ESB)]

Hello Laurent,

Thank you for your review.

Actually I sent this email yesterday. But  I don't see it in mailing list archives because of some reason.

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart at ideasonboard.com]
> Sent: Dienstag, 3. April 2018 20:53
> To: Ucan, Emre (ADITG/ESB)
> Cc: dri-devel at lists.freedesktop.org
> Subject: Re: [PATCH] drm: rcar-du: track dma-buf fences
> 
> Hello Emre,
> 
> Thank you for the patch.
> 
> On Tuesday, 3 April 2018 12:14:33 EEST Emre Ucan wrote:
> > We have to check dma-buf reservation objects
> > of our framebuffers before we use them.
> > Otherwise, another driver might be writing
> > on the same buffer which we are using.
> > This would cause visible tearing effects
> > on display.
> >
> > We can use existing atomic helper functions
> > to solve this problem.
> >
> > Signed-off-by: Emre Ucan <eucan at de.adit-jv.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c |  2 ++
> >  drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 20 ++++++++++++++++++++
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 0329b35..f3da3d1 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -255,6 +255,8 @@ static void rcar_du_atomic_commit_tail(struct
> > drm_atomic_state *old_state) {
> >  	struct drm_device *dev = old_state->dev;
> >
> > +	drm_atomic_helper_wait_for_fences(dev, old_state, false);
> > +
> 
> The commit_tail() function in drm_atomic_helper.c, which calls our
> atomic_commit_tail() implementation, already calls
> drm_atomic_helper_wait_for_fences(). Why is there a need to duplicate the
> call
> here ?

You are right. I will remove it in second version.

> 
> >  	/* Apply the atomic update. */
> >  	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> >  	drm_atomic_helper_commit_planes(dev, old_state,
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 2c260c3..482e23c 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > @@ -18,12 +18,16 @@
> >  #include <drm/drm_fb_cma_helper.h>
> >  #include <drm/drm_gem_cma_helper.h>
> >  #include <drm/drm_plane_helper.h>
> > +#include <drm/drm_atomic.h>
> >
> >  #include <linux/bitops.h>
> >  #include <linux/dma-mapping.h>
> > +#include <linux/dma-fence.h>
> > +#include <linux/dma-buf.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/videodev2.h>
> > +#include <linux/reservation.h>
> >
> >  #include <media/vsp1.h>
> >
> > @@ -203,6 +207,20 @@ static void rcar_du_vsp_plane_setup(struct
> > rcar_du_vsp_plane *plane) plane->index, &cfg);
> >  }
> >
> > +static void rcar_du_vsp_set_fence_for_plane(struct drm_plane_state
> *state)
> > +{
> > +	struct drm_gem_cma_object *gem;
> > +	struct dma_buf *dma_buf;
> > +	struct dma_fence *fence;
> > +
> > +	gem = drm_fb_cma_get_gem_obj(state->fb, 0);
> > +	dma_buf = gem->base.dma_buf;
> > +	if (dma_buf) {
> > +		fence = reservation_object_get_excl_rcu(dma_buf->resv);
> > +		drm_atomic_set_fence_for_plane(state, fence);
> 
> Unless I'm mistaken this is used for implicit fencing only. What is your use
> case, wouldn't it be better for userspace to use explicit fencing as that is
> the fence model that has been selected for display ?

We are using Weston on Renesas R-Car H3 SoC. I am using GPU rendered client buffers
directly as scanout buffer for the display. Weston is not using explicit fencing.

> 
> > +	}
> > +}
> 
> This looks very similar to drm_gem_fb_prepare_fb(), couldn't you use that
> function instead ?

Description of drm_gem_fb_prepare_fb() function states that it can be
used as the &drm_plane_helper_funcs.prepare_fb hook. But we have
our own hook function which is called rcar_du_vsp_plane_prepare_fb().
Therefore, I was not sure if it is correct to use drm_gem_fb_prepare_fb()
inside our hook function.

I will use it in the second version nevertheless.

> 
> >  static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> >  					struct drm_plane_state *state)
> >  {
> > @@ -237,6 +255,8 @@ static int rcar_du_vsp_plane_prepare_fb(struct
> drm_plane
> > *plane, }
> >  	}
> >
> > +	rcar_du_vsp_set_fence_for_plane(state);
> > +
> >  	return 0;
> >
> >  fail:
> 
> --
> Regards,
> 
> Laurent Pinchart
> 
> 

Best Regards,

Emre Ucan