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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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@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 ?

>  	/* 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 ?

> +	}
> +}

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel