On Wed, Apr 4, 2018 at 12:59 PM, Ucan, Emre (ADITG/ESB) <eucan@xxxxxxxxxxxxxx> wrote: > Hello Laurent > > Thank you for your review. > >> -----Original Message----- >> From: Laurent Pinchart [mailto:laurent.pinchart@xxxxxxxxxxxxxxxx] >> Sent: Dienstag, 3. April 2018 20:53 >> To: Ucan, Emre (ADITG/ESB) >> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx >> 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@xxxxxxxxxxxxxx> >> > --- >> > 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. You can use it in your own hook. Patch to update the kerneldoc to clarify that would be great. -Daniel > >> >> > /* 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 > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
