[PATCH v2 1/4] drm/atomic: add drm_atomic_set_fence_for_plane()

[PATCH v2 1/4] drm/atomic: add drm_atomic_set_fence_for_plane()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

This new function should be used by drivers when setting a implicit
fence for the plane. It abstracts the fact that the user might have
chosen explicit fencing instead.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_atomic.c | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_atomic.h     |  2 ++
 include/drm/drm_plane.h      |  2 +-
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index c32fb3c..5e73954 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1147,6 +1147,36 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
 EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
 
 /**
+ * drm_atomic_set_fence_for_plane - set fence for plane
+ * @plane_state: atomic state object for the plane
+ * @fence: dma_fence to use for the plane
+ *
+ * Helper to setup the plane_state fence in case it is not set yet.
+ * By using this drivers doesn't need to worry if the user choose
+ * implicit or explicit fencing.
+ *
+ * This function will not set the fence to the state if it was set
+ * via explicit fencing interfaces on the atomic ioctl. It will
+ * all drope the reference to the fence as we not storing it
+ * anywhere.
+ *
+ * Otherwise, if plane_state->fence is not set this function we
+ * just set it with the received implict fence.
+ */
+void
+drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
+			       struct dma_fence *fence)
+{
+	if (plane_state->fence) {
+		dma_fence_put(fence);
+		return;
+	}
+
+	plane_state->fence = fence;
+}
+EXPORT_SYMBOL(drm_atomic_set_fence_for_plane);
+
+/**
  * drm_atomic_set_crtc_for_connector - set crtc for connector
  * @conn_state: atomic state object for the connector
  * @crtc: crtc to use for the connector
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index fc8af53..2d1e9c9 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -345,6 +345,8 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
 			      struct drm_crtc *crtc);
 void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
 				 struct drm_framebuffer *fb);
+void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state,
+				    struct dma_fence *fence);
 int __must_check
 drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
 				  struct drm_crtc *crtc);
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index c5e8a0d..68f6d22 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -59,7 +59,7 @@ struct drm_plane_state {
 
 	struct drm_crtc *crtc;   /* do not write directly, use drm_atomic_set_crtc_for_plane() */
 	struct drm_framebuffer *fb;  /* do not write directly, use drm_atomic_set_fb_for_plane() */
-	struct dma_fence *fence;
+	struct dma_fence *fence; /* do not write directly, use drm_atomic_set_fence_for_plane() */
 
 	/* Signed dest location allows it to be partially off screen */
 	int32_t crtc_x, crtc_y;
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/4] drm/imx: use drm_atomic_set_fence_for_plane() to set the fence

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

drm_atomic_set_fence_for_plane() is smart and won't overwrite
plane_state->fence if the user already set an explicit fence there.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index 98df09c..07fe955 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -158,6 +158,7 @@ static int imx_drm_atomic_commit(struct drm_device *dev,
 	struct drm_plane_state *plane_state;
 	struct drm_plane *plane;
 	struct dma_buf *dma_buf;
+	struct dma_fence *fence;
 	int i;
 
 	/*
@@ -170,8 +171,9 @@ static int imx_drm_atomic_commit(struct drm_device *dev,
 							 0)->base.dma_buf;
 			if (!dma_buf)
 				continue;
-			plane_state->fence =
-				reservation_object_get_excl_rcu(dma_buf->resv);
+			fence = reservation_object_get_excl_rcu(dma_buf->resv);
+
+			drm_atomic_set_fence_for_plane(plane_state, fence);
 		}
 	}
 
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/4] drm/imx: use drm_atomic_set_fence_for_plane() to set the fence

[Philipp Zabel]

Am Montag, den 07.11.2016, 19:03 +0900 schrieb Gustavo Padovan:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> drm_atomic_set_fence_for_plane() is smart and won't overwrite
> plane_state->fence if the user already set an explicit fence there.
> 
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

> ---
>  drivers/gpu/drm/imx/imx-drm-core.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
> index 98df09c..07fe955 100644
> --- a/drivers/gpu/drm/imx/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/imx-drm-core.c
> @@ -158,6 +158,7 @@ static int imx_drm_atomic_commit(struct drm_device *dev,
>  	struct drm_plane_state *plane_state;
>  	struct drm_plane *plane;
>  	struct dma_buf *dma_buf;
> +	struct dma_fence *fence;
>  	int i;
>  
>  	/*
> @@ -170,8 +171,9 @@ static int imx_drm_atomic_commit(struct drm_device *dev,
>  							 0)->base.dma_buf;
>  			if (!dma_buf)
>  				continue;
> -			plane_state->fence =
> -				reservation_object_get_excl_rcu(dma_buf->resv);
> +			fence = reservation_object_get_excl_rcu(dma_buf->resv);
> +
> +			drm_atomic_set_fence_for_plane(plane_state, fence);
>  		}
>  	}

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/4] drm/msm: use drm_atomic_set_fence_for_plane() to set the fence

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

drm_atomic_set_fence_for_plane() is smart and won't overwrite
plane_state->fence if the user already set an explicit fence there.

Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/msm/msm_atomic.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index db193f8..4e21e1d 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -217,8 +217,9 @@ int msm_atomic_commit(struct drm_device *dev,
 		if ((plane->state->fb != plane_state->fb) && plane_state->fb) {
 			struct drm_gem_object *obj = msm_framebuffer_bo(plane_state->fb, 0);
 			struct msm_gem_object *msm_obj = to_msm_bo(obj);
+			struct dma_fence *fence = reservation_object_get_excl_rcu(msm_obj->resv);
 
-			plane_state->fence = reservation_object_get_excl_rcu(msm_obj->resv);
+			drm_atomic_set_fence_for_plane(plane_state, fence);
 		}
 	}
 
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/4] drm/msm: use drm_atomic_set_fence_for_plane() to set the fence

[Rob Clark]

On Mon, Nov 7, 2016 at 5:03 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
>
> drm_atomic_set_fence_for_plane() is smart and won't overwrite
> plane_state->fence if the user already set an explicit fence there.
>
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

LGTM

Acked-by: Rob Clark <robdclark@gmail.com>

let me know if I should pull this in via msm-next for 4.10.. otherwise
taking it via drm-misc is fine by me

BR,
-R


> ---
>  drivers/gpu/drm/msm/msm_atomic.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index db193f8..4e21e1d 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -217,8 +217,9 @@ int msm_atomic_commit(struct drm_device *dev,
>                 if ((plane->state->fb != plane_state->fb) && plane_state->fb) {
>                         struct drm_gem_object *obj = msm_framebuffer_bo(plane_state->fb, 0);
>                         struct msm_gem_object *msm_obj = to_msm_bo(obj);
> +                       struct dma_fence *fence = reservation_object_get_excl_rcu(msm_obj->resv);
>
> -                       plane_state->fence = reservation_object_get_excl_rcu(msm_obj->resv);
> +                       drm_atomic_set_fence_for_plane(plane_state, fence);
>                 }
>         }
>
> --
> 2.5.5
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/4] drm/plane: add inline doc for struct drm_plane

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Some of the members of struct drm_plane had extra comments so for these
add inline kernel comment to consolidate all documentation in one place.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
---
 include/drm/drm_plane.h | 61 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 12 deletions(-)

diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 68f6d22..683b170 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -32,11 +32,6 @@ struct drm_crtc;
 /**
  * struct drm_plane_state - mutable plane state
  * @plane: backpointer to the plane
- * @crtc: currently bound CRTC, NULL if disabled
- * @fb: currently bound framebuffer
- * @fence: optional fence to wait for before scanning out @fb
- * @crtc_x: left position of visible portion of plane on crtc
- * @crtc_y: upper position of visible portion of plane on crtc
  * @crtc_w: width of visible portion of plane on crtc
  * @crtc_h: height of visible portion of plane on crtc
  * @src_x: left position of visible portion of plane within
@@ -51,18 +46,56 @@ struct drm_crtc;
  *	where N is the number of active planes for given crtc
  * @src: clipped source coordinates of the plane (in 16.16)
  * @dst: clipped destination coordinates of the plane
- * @visible: visibility of the plane
  * @state: backpointer to global drm_atomic_state
  */
 struct drm_plane_state {
 	struct drm_plane *plane;
 
-	struct drm_crtc *crtc;   /* do not write directly, use drm_atomic_set_crtc_for_plane() */
-	struct drm_framebuffer *fb;  /* do not write directly, use drm_atomic_set_fb_for_plane() */
-	struct dma_fence *fence; /* do not write directly, use drm_atomic_set_fence_for_plane() */
+	/**
+	 * @crtc:
+	 *
+	 * currently bound CRTC, NULL if disabled
+	 *
+	 * do not write directly, use drm_atomic_set_crtc_for_plane()
+	 */
+	struct drm_crtc *crtc;
+
+	/**
+	 * @fb:
+	 *
+	 * currently bound framebuffer
+	 *
+	 * do not write directly, use drm_atomic_set_fb_for_plane()
+	 */
+	struct drm_framebuffer *fb;
+
+	/**
+	 * @fence:
+	 *
+	 * optional fence to wait for before scanning out @fb
+	 *
+	 * do not write directly, use drm_atomic_set_fence_for_plane()
+	 */
+	struct dma_fence *fence;
+
+	/**
+	 * @crtc_x:
+	 *
+	 * left position of visible portion of plane on crtc
+	 *
+	 * Signed dest location allows it to be partially off screen.
+	 */
+
+	int32_t crtc_x;
+	/**
+	 * @crtc_y:
+	 *
+	 * upper position of visible portion of plane on crtc
+	 *
+	 * Signed dest location allows it to be partially off screen.
+	 */
+	int32_t crtc_y;
 
-	/* Signed dest location allows it to be partially off screen */
-	int32_t crtc_x, crtc_y;
 	uint32_t crtc_w, crtc_h;
 
 	/* Source values are 16.16 fixed point */
@@ -79,7 +112,11 @@ struct drm_plane_state {
 	/* Clipped coordinates */
 	struct drm_rect src, dst;
 
-	/*
+	/**
+	 * @visible:
+	 *
+	 * visibility of the plane
+	 *
 	 * Is the plane actually visible? Can be false even
 	 * if fb!=NULL and crtc!=NULL, due to clipping.
 	 */
-- 
2.5.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/4] drm/plane: add inline doc for struct drm_plane

[Daniel Vetter]

On Mon, Nov 07, 2016 at 07:03:33PM +0900, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> Some of the members of struct drm_plane had extra comments so for these
> add inline kernel comment to consolidate all documentation in one place.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

For in-line kerneldoc I think we want real paragraphs with real sentences,
just looks better. I bikesheded your patch to suit and applied the entire
series to drm-misc.

Thanks, Daniel

> ---
>  include/drm/drm_plane.h | 61 +++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 49 insertions(+), 12 deletions(-)
> 
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 68f6d22..683b170 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -32,11 +32,6 @@ struct drm_crtc;
>  /**
>   * struct drm_plane_state - mutable plane state
>   * @plane: backpointer to the plane
> - * @crtc: currently bound CRTC, NULL if disabled
> - * @fb: currently bound framebuffer
> - * @fence: optional fence to wait for before scanning out @fb
> - * @crtc_x: left position of visible portion of plane on crtc
> - * @crtc_y: upper position of visible portion of plane on crtc
>   * @crtc_w: width of visible portion of plane on crtc
>   * @crtc_h: height of visible portion of plane on crtc
>   * @src_x: left position of visible portion of plane within
> @@ -51,18 +46,56 @@ struct drm_crtc;
>   *	where N is the number of active planes for given crtc
>   * @src: clipped source coordinates of the plane (in 16.16)
>   * @dst: clipped destination coordinates of the plane
> - * @visible: visibility of the plane
>   * @state: backpointer to global drm_atomic_state
>   */
>  struct drm_plane_state {
>  	struct drm_plane *plane;
>  
> -	struct drm_crtc *crtc;   /* do not write directly, use drm_atomic_set_crtc_for_plane() */
> -	struct drm_framebuffer *fb;  /* do not write directly, use drm_atomic_set_fb_for_plane() */
> -	struct dma_fence *fence; /* do not write directly, use drm_atomic_set_fence_for_plane() */
> +	/**
> +	 * @crtc:
> +	 *
> +	 * currently bound CRTC, NULL if disabled
> +	 *
> +	 * do not write directly, use drm_atomic_set_crtc_for_plane()
> +	 */
> +	struct drm_crtc *crtc;
> +
> +	/**
> +	 * @fb:
> +	 *
> +	 * currently bound framebuffer
> +	 *
> +	 * do not write directly, use drm_atomic_set_fb_for_plane()
> +	 */
> +	struct drm_framebuffer *fb;
> +
> +	/**
> +	 * @fence:
> +	 *
> +	 * optional fence to wait for before scanning out @fb
> +	 *
> +	 * do not write directly, use drm_atomic_set_fence_for_plane()
> +	 */
> +	struct dma_fence *fence;
> +
> +	/**
> +	 * @crtc_x:
> +	 *
> +	 * left position of visible portion of plane on crtc
> +	 *
> +	 * Signed dest location allows it to be partially off screen.
> +	 */
> +
> +	int32_t crtc_x;
> +	/**
> +	 * @crtc_y:
> +	 *
> +	 * upper position of visible portion of plane on crtc
> +	 *
> +	 * Signed dest location allows it to be partially off screen.
> +	 */
> +	int32_t crtc_y;
>  
> -	/* Signed dest location allows it to be partially off screen */
> -	int32_t crtc_x, crtc_y;
>  	uint32_t crtc_w, crtc_h;
>  
>  	/* Source values are 16.16 fixed point */
> @@ -79,7 +112,11 @@ struct drm_plane_state {
>  	/* Clipped coordinates */
>  	struct drm_rect src, dst;
>  
> -	/*
> +	/**
> +	 * @visible:
> +	 *
> +	 * visibility of the plane
> +	 *
>  	 * Is the plane actually visible? Can be false even
>  	 * if fb!=NULL and crtc!=NULL, due to clipping.
>  	 */
> -- 
> 2.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel