[PATCH 0/6] i915 atomic properties

[PATCH 0/6] i915 atomic properties

[Matt Roper]

I had hoped to make this a full "nuclear pageflip" enablement series, but I
didn't have as much time to work on this today as I hoped, so I still need to
finish up the final integration with the DRM core atomic code.  Several of my
early patches are working and relatively straightforward, so I figured I'd post
these as a preliminary patchset; hopefully I'll have the rest of the patches
necessary to expose "nuclear pageflip" (i.e., single-crtc, plane-only updates)
ready in the next day or so.

This series does add (and then use) a transitional helper for planes'
.set_property() handler, similar to what we have for .update_plane() and
.disable_plane().  The transitional helper can be used by drivers that are
starting to implement the atomic driver entrypoints, but aren't ready to switch
over to full atomic functionality yet.

Note that the i915-specific patches here depend on Ander's recent series which
hasn't been merged yet:
        [PATCH 0/7] Make drm_crtc->state match pipe_config

Cc: dri-devel@lists.freedesktop.org

Matt Roper (6):
  drm/i915: Move rotation from intel_plane to intel_plane_state
  drm/i915: Consolidate plane handler vtables
  drm/plane-helper: Add transitional helper for .set_plane().
  drm/plane-helper: Fix transitional helper kerneldocs
  drm/i915: Add .atomic_{get,set}_property() entrypoints to planes
  drm/i915: Replace intel_set_property() with transitional helper

 drivers/gpu/drm/drm_plane_helper.c        | 109 +++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_atomic_plane.c |  84 +++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_display.c      |  63 ++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h          |  22 ++++--
 drivers/gpu/drm/i915/intel_fbc.c          |   4 +-
 drivers/gpu/drm/i915/intel_sprite.c       |  65 ++++++------------
 include/drm/drm_plane_helper.h            |   3 +
 7 files changed, 268 insertions(+), 82 deletions(-)

-- 
1.8.5.1

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

[PATCH 3/6] drm/plane-helper: Add transitional helper for .set_plane().

[Matt Roper]

Add a transitional helper for planes' .set_property() entrypoint,
similar to what we already have for .update_plane() and
.disable_plane().  This allows drivers that are still in the process of
transitioning to implement and exercise the .atomic_set_property()
driver entrypoint that will eventually be used by atomic.

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 105 +++++++++++++++++++++++++++++++++++++
 include/drm/drm_plane_helper.h     |   3 ++
 2 files changed, 108 insertions(+)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index f24c4cf..1b1e927 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -578,3 +578,108 @@ int drm_plane_helper_disable(struct drm_plane *plane)
 	return drm_plane_helper_commit(plane, plane_state, plane->fb);
 }
 EXPORT_SYMBOL(drm_plane_helper_disable);
+
+/**
+ * drm_plane_helper_set_property() - Transitional helper for property updates
+ * @plane: plane object to update
+ * @prop: property to update
+ * @val: value to set property to
+ *
+ * Provides a default plane property handler using the atomic plane update
+ * functions.  Drivers in the process of transitioning to atomic should
+ * replace their plane.set_property() entrypoint with this function and
+ * then implement the plane.atomic_set_property() entrypoint.
+ *
+ * This is useful for piecewise transitioning of a driver to the atomic helpers.
+ *
+ * Note that this helper assumes that no hardware programming should be
+ * performed if the plane is disabled.  When the plane is not attached to a
+ * crtc, we just swap in the validated plane state and return; there's no
+ * call to atomic_begin()/atomic_update()/atomic_flush().
+ *
+ * RETURNS:
+ * Zero on success, error code on failure
+ */
+int drm_plane_helper_set_property(struct drm_plane *plane,
+				  struct drm_property *prop,
+				  uint64_t val)
+{
+	struct drm_plane_state *plane_state;
+	struct drm_plane_helper_funcs *plane_funcs = plane->helper_private;
+	struct drm_crtc_helper_funcs *crtc_funcs;
+	int ret;
+
+	/*
+	 * Drivers may not have an .atomic_set_property() handler if they have
+	 * no driver-specific properties, but they generally wouldn't setup a
+	 * .set_property() handler (pointing to this function) for the plane in
+	 * that case either; throw a warning since this is probably a mistake.
+	 */
+	if (WARN_ON(!plane->funcs->atomic_set_property))
+		return -EINVAL;
+
+	if (plane->funcs->atomic_duplicate_state)
+		plane_state = plane->funcs->atomic_duplicate_state(plane);
+	else if (plane->state)
+		plane_state = drm_atomic_helper_plane_duplicate_state(plane);
+	else
+		plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
+	if (!plane_state)
+		return -ENOMEM;
+	plane_state->plane = plane;
+
+	/*
+	 * Update the state object with the new property value we want to
+	 * set.  If the property is not recognized as a driver-specific property,
+	 * -EINVAL will be returned here.
+	 */
+	ret = plane->funcs->atomic_set_property(plane, plane_state, prop, val);
+	if (ret)
+		goto out;
+
+	/*
+	 * Check that the updated plane state is actually programmable (e.g.,
+	 * doesn't violate hardware constraints).
+	 */
+	if (plane_funcs->atomic_check) {
+		ret = plane_funcs->atomic_check(plane, plane_state);
+		if (ret)
+			goto out;
+	}
+
+	/* Point of no return, commit sw state. */
+	swap(plane->state, plane_state);
+
+	/*
+	 * If the plane is disabled, we're done.  No hardware programming is
+	 * attempted when the plane is disabled.
+	 */
+	if (!plane->crtc)
+		goto out;
+
+	/* Start hardware transaction to commit new state to hardware */
+	crtc_funcs = plane->crtc->helper_private;
+	if (crtc_funcs && crtc_funcs->atomic_begin)
+		crtc_funcs->atomic_begin(plane->crtc);
+	plane_funcs->atomic_update(plane, plane_state);
+	if (crtc_funcs && crtc_funcs->atomic_flush)
+		crtc_funcs->atomic_flush(plane->crtc);
+
+	/*
+	 * Since we're not using full atomic yet, we still need to update the shadow
+	 * property value that's managed by the DRM core since that's where the
+	 * property values will be read back from.
+	 */
+	drm_object_property_set_value(&plane->base, prop, val);
+
+out:
+	if (plane_state) {
+		if (plane->funcs->atomic_destroy_state)
+			plane->funcs->atomic_destroy_state(plane, plane_state);
+		else
+			drm_atomic_helper_plane_destroy_state(plane, plane_state);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_plane_helper_set_property);
diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
index a185392..4a56961 100644
--- a/include/drm/drm_plane_helper.h
+++ b/include/drm/drm_plane_helper.h
@@ -107,6 +107,9 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 			    uint32_t src_x, uint32_t src_y,
 			    uint32_t src_w, uint32_t src_h);
 int drm_plane_helper_disable(struct drm_plane *plane);
+int drm_plane_helper_set_property(struct drm_plane *plane,
+				  struct drm_property *prop,
+				  uint64_t val);
 
 /* For use by drm_crtc_helper.c */
 int drm_plane_helper_commit(struct drm_plane *plane,
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/6] drm/plane-helper: Fix transitional helper kerneldocs

[Matt Roper]

drm_plane_helper_{update,disable} are not specific to primary planes;
fix some copy/paste summaries to avoid confusion.

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_plane_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
index 1b1e927..3e7e26f 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -484,7 +484,7 @@ out:
 }
 
 /**
- * drm_plane_helper_update() - Helper for primary plane update
+ * drm_plane_helper_update() - Transitional helper for plane update
  * @plane: plane object to update
  * @crtc: owning CRTC of owning plane
  * @fb: framebuffer to flip onto plane
@@ -541,7 +541,7 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
 EXPORT_SYMBOL(drm_plane_helper_update);
 
 /**
- * drm_plane_helper_disable() - Helper for primary plane disable
+ * drm_plane_helper_disable() - Transitional helper for plane disable
  * @plane: plane to disable
  *
  * Provides a default plane disable handler using the atomic plane update
-- 
1.8.5.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/plane-helper: Add transitional helper for .set_plane().

[Daniel Vetter]

On Thu, Jan 15, 2015 at 06:34:21PM -0800, Matt Roper wrote:
> Add a transitional helper for planes' .set_property() entrypoint,
> similar to what we already have for .update_plane() and
> .disable_plane().  This allows drivers that are still in the process of
> transitioning to implement and exercise the .atomic_set_property()
> driver entrypoint that will eventually be used by atomic.
> 
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Since i915 is the only driver with an interest in this (I presume at
least) maybe we should make that much fuzz about this here and just move
this function into i915 private code?

At least the problem I see with too fancy transitional helpers is that
they'll grow stale fast. Which will happen with this one here as soon as
we move rotation into drm_plane_state, and will happen every time we add
something there. And I expect a lot of common core properties, e.g. for Z
order or blending modes.
-Daniel

> ---
>  drivers/gpu/drm/drm_plane_helper.c | 105 +++++++++++++++++++++++++++++++++++++
>  include/drm/drm_plane_helper.h     |   3 ++
>  2 files changed, 108 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index f24c4cf..1b1e927 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -578,3 +578,108 @@ int drm_plane_helper_disable(struct drm_plane *plane)
>  	return drm_plane_helper_commit(plane, plane_state, plane->fb);
>  }
>  EXPORT_SYMBOL(drm_plane_helper_disable);
> +
> +/**
> + * drm_plane_helper_set_property() - Transitional helper for property updates
> + * @plane: plane object to update
> + * @prop: property to update
> + * @val: value to set property to
> + *
> + * Provides a default plane property handler using the atomic plane update
> + * functions.  Drivers in the process of transitioning to atomic should
> + * replace their plane.set_property() entrypoint with this function and
> + * then implement the plane.atomic_set_property() entrypoint.
> + *
> + * This is useful for piecewise transitioning of a driver to the atomic helpers.
> + *
> + * Note that this helper assumes that no hardware programming should be
> + * performed if the plane is disabled.  When the plane is not attached to a
> + * crtc, we just swap in the validated plane state and return; there's no
> + * call to atomic_begin()/atomic_update()/atomic_flush().
> + *
> + * RETURNS:
> + * Zero on success, error code on failure
> + */
> +int drm_plane_helper_set_property(struct drm_plane *plane,
> +				  struct drm_property *prop,
> +				  uint64_t val)
> +{
> +	struct drm_plane_state *plane_state;
> +	struct drm_plane_helper_funcs *plane_funcs = plane->helper_private;
> +	struct drm_crtc_helper_funcs *crtc_funcs;
> +	int ret;
> +
> +	/*
> +	 * Drivers may not have an .atomic_set_property() handler if they have
> +	 * no driver-specific properties, but they generally wouldn't setup a
> +	 * .set_property() handler (pointing to this function) for the plane in
> +	 * that case either; throw a warning since this is probably a mistake.
> +	 */
> +	if (WARN_ON(!plane->funcs->atomic_set_property))
> +		return -EINVAL;
> +
> +	if (plane->funcs->atomic_duplicate_state)
> +		plane_state = plane->funcs->atomic_duplicate_state(plane);
> +	else if (plane->state)
> +		plane_state = drm_atomic_helper_plane_duplicate_state(plane);
> +	else
> +		plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL);
> +	if (!plane_state)
> +		return -ENOMEM;
> +	plane_state->plane = plane;
> +
> +	/*
> +	 * Update the state object with the new property value we want to
> +	 * set.  If the property is not recognized as a driver-specific property,
> +	 * -EINVAL will be returned here.
> +	 */
> +	ret = plane->funcs->atomic_set_property(plane, plane_state, prop, val);
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * Check that the updated plane state is actually programmable (e.g.,
> +	 * doesn't violate hardware constraints).
> +	 */
> +	if (plane_funcs->atomic_check) {
> +		ret = plane_funcs->atomic_check(plane, plane_state);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	/* Point of no return, commit sw state. */
> +	swap(plane->state, plane_state);
> +
> +	/*
> +	 * If the plane is disabled, we're done.  No hardware programming is
> +	 * attempted when the plane is disabled.
> +	 */
> +	if (!plane->crtc)
> +		goto out;
> +
> +	/* Start hardware transaction to commit new state to hardware */
> +	crtc_funcs = plane->crtc->helper_private;
> +	if (crtc_funcs && crtc_funcs->atomic_begin)
> +		crtc_funcs->atomic_begin(plane->crtc);
> +	plane_funcs->atomic_update(plane, plane_state);
> +	if (crtc_funcs && crtc_funcs->atomic_flush)
> +		crtc_funcs->atomic_flush(plane->crtc);
> +
> +	/*
> +	 * Since we're not using full atomic yet, we still need to update the shadow
> +	 * property value that's managed by the DRM core since that's where the
> +	 * property values will be read back from.
> +	 */
> +	drm_object_property_set_value(&plane->base, prop, val);
> +
> +out:
> +	if (plane_state) {
> +		if (plane->funcs->atomic_destroy_state)
> +			plane->funcs->atomic_destroy_state(plane, plane_state);
> +		else
> +			drm_atomic_helper_plane_destroy_state(plane, plane_state);
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_plane_helper_set_property);
> diff --git a/include/drm/drm_plane_helper.h b/include/drm/drm_plane_helper.h
> index a185392..4a56961 100644
> --- a/include/drm/drm_plane_helper.h
> +++ b/include/drm/drm_plane_helper.h
> @@ -107,6 +107,9 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>  			    uint32_t src_x, uint32_t src_y,
>  			    uint32_t src_w, uint32_t src_h);
>  int drm_plane_helper_disable(struct drm_plane *plane);
> +int drm_plane_helper_set_property(struct drm_plane *plane,
> +				  struct drm_property *prop,
> +				  uint64_t val);
>  
>  /* For use by drm_crtc_helper.c */
>  int drm_plane_helper_commit(struct drm_plane *plane,
> -- 
> 1.8.5.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/plane-helper: Fix transitional helper kerneldocs

[Daniel Vetter]

On Thu, Jan 15, 2015 at 06:34:22PM -0800, Matt Roper wrote:
> drm_plane_helper_{update,disable} are not specific to primary planes;
> fix some copy/paste summaries to avoid confusion.
> 
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Applied to my topic/atomic-core branch.

Thanks, Daniel
> ---
>  drivers/gpu/drm/drm_plane_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c
> index 1b1e927..3e7e26f 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -484,7 +484,7 @@ out:
>  }
>  
>  /**
> - * drm_plane_helper_update() - Helper for primary plane update
> + * drm_plane_helper_update() - Transitional helper for plane update
>   * @plane: plane object to update
>   * @crtc: owning CRTC of owning plane
>   * @fb: framebuffer to flip onto plane
> @@ -541,7 +541,7 @@ int drm_plane_helper_update(struct drm_plane *plane, struct drm_crtc *crtc,
>  EXPORT_SYMBOL(drm_plane_helper_update);
>  
>  /**
> - * drm_plane_helper_disable() - Helper for primary plane disable
> + * drm_plane_helper_disable() - Transitional helper for plane disable
>   * @plane: plane to disable
>   *
>   * Provides a default plane disable handler using the atomic plane update
> -- 
> 1.8.5.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx