On Tue, Apr 3, 2018 at 8:53 PM, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > 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 ? > >> /* 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 ? Implicit fencing is very much still a thing on most X and wayland setups. There's a push to eventually make everything explicit, but it'll take a while I think. -Daniel > >> + } >> +} > > This looks very similar to drm_gem_fb_prepare_fb(), couldn't you use that > function instead ? > >> 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 > > > > _______________________________________________ > 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