drm: Add helper macro for duplicating custom drm_*_state
diff mbox

Message ID 1486654902-2558-2-git-send-email-mihail.atanassov@arm.com
State New
Headers show

Commit Message

Mihail Atanassov Feb. 9, 2017, 3:41 p.m. UTC
Assuming a derived struct of the form:

struct foo_bar_state
{
	struct drm_bar_state bar_state;
	struct foo_private priv;
	struct foo_private2 *priv2;
};

memcpy priv and priv2 to the new instance of foo_bar_state. The
intention is to use this macro in ->atomic_duplicate_state in conjunction with
__drm_atomic_helper_*_duplicate_state, which already copies the relevant
drm_*_state struct.

There's an equality check for new_state and old_state to ensure that
they are distinct instances of the same type, and a BUILD_BUG if the
base struct (bar_state in the above example) is not first in the derived
struct, to avoid missing any data before it and corrupting the base's data.

Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
---
 include/drm/drm_atomic_helper.h | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

Comments

Daniel Vetter Feb. 9, 2017, 5:35 p.m. UTC | #1
On Thu, Feb 09, 2017 at 03:41:42PM +0000, Mihail Atanassov wrote:
> Assuming a derived struct of the form:
> 
> struct foo_bar_state
> {
> 	struct drm_bar_state bar_state;
> 	struct foo_private priv;
> 	struct foo_private2 *priv2;
> };
> 
> memcpy priv and priv2 to the new instance of foo_bar_state. The
> intention is to use this macro in ->atomic_duplicate_state in conjunction with
> __drm_atomic_helper_*_duplicate_state, which already copies the relevant
> drm_*_state struct.
> 
> There's an equality check for new_state and old_state to ensure that
> they are distinct instances of the same type, and a BUILD_BUG if the
> base struct (bar_state in the above example) is not first in the derived
> struct, to avoid missing any data before it and corrupting the base's data.
> 
> Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>

Seems reasonable, but I don't have any strong opinions about it's
usefulness. Converting a few drivers (to really establish it as a
pattern), or at least a few acks from maintainers, to prove that it's a
good idea, and I'll merge this.
-Daniel

> ---
>  include/drm/drm_atomic_helper.h | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index d066e94..ecc6a82 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -171,6 +171,39 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>  				       uint32_t size);
>  
>  /**
> + * drm_atomic_duplicate_custom_state - helper macro for duplicating
> + * driver-private additions to drm_*_state.
> + * @new_state: pointer to destination state struct
> + * @old_state: pointer to source state struct
> + * @basename: name of the drm_*_state member of the new_state/old_state struct
> + *
> + * Copies the data after the base struct until the end of the custom struct,
> + * e.g. given a structure
> + *
> + * struct foo_bar_state {
> + * 	struct drm_bar_state base;
> + * 	struct foo_private priv;
> + * 	struct foo_private2 *priv2;
> + * };
> + *
> + * this copies priv and priv2. NB: the base struct *must* be the first element
> + * of the derived struct, and new_state and old_state have to be two distinct
> + * instances.
> + */
> +#define drm_atomic_helper_duplicate_custom_state(new_state, old_state, basename) \
> +	do { \
> +		size_t base_size = sizeof(new_state->basename); \
> +		size_t base_offset = offsetof(typeof(*new_state), basename); \
> +		\
> +		BUILD_BUG_ON(base_offset != 0); \
> +		if (new_state == old_state) /* Type-check */ \
> +			break; \
> +		memcpy((char *)new_state + base_size, \
> +		       (char *)old_state + base_size, \
> +		       sizeof(*new_state) - base_size); \
> +	} while(0)
> +
> +/**
>   * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
>   * @plane: the loop cursor
>   * @crtc:  the crtc whose planes are iterated
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Feb. 9, 2017, 5:38 p.m. UTC | #2
On Thu, Feb 09, 2017 at 03:41:42PM +0000, Mihail Atanassov wrote:
> Assuming a derived struct of the form:
> 
> struct foo_bar_state
> {
> 	struct drm_bar_state bar_state;
> 	struct foo_private priv;
> 	struct foo_private2 *priv2;
> };
> 
> memcpy priv and priv2 to the new instance of foo_bar_state. The
> intention is to use this macro in ->atomic_duplicate_state in conjunction with
> __drm_atomic_helper_*_duplicate_state, which already copies the relevant
> drm_*_state struct.
> 
> There's an equality check for new_state and old_state to ensure that
> they are distinct instances of the same type, and a BUILD_BUG if the
> base struct (bar_state in the above example) is not first in the derived
> struct, to avoid missing any data before it and corrupting the base's data.
> 
> Signed-off-by: Mihail Atanassov <mihail.atanassov@arm.com>
> ---
>  include/drm/drm_atomic_helper.h | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index d066e94..ecc6a82 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -171,6 +171,39 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
>  				       uint32_t size);
>  
>  /**
> + * drm_atomic_duplicate_custom_state - helper macro for duplicating
> + * driver-private additions to drm_*_state.
> + * @new_state: pointer to destination state struct
> + * @old_state: pointer to source state struct
> + * @basename: name of the drm_*_state member of the new_state/old_state struct
> + *
> + * Copies the data after the base struct until the end of the custom struct,
> + * e.g. given a structure
> + *
> + * struct foo_bar_state {
> + * 	struct drm_bar_state base;
> + * 	struct foo_private priv;
> + * 	struct foo_private2 *priv2;
> + * };

Forgot the kernel-doc bikeshed: Please reformat this that it's a proper
rst quoted block for prettier output. See

http://blog.ffwll.ch/2016/12/howto-docs.html

> + *
> + * this copies priv and priv2. NB: the base struct *must* be the first element

Iirc rst has a different opinion how bold higlighting works, pls double
check this is the right one.

> + * of the derived struct, and new_state and old_state have to be two distinct

@new_state and @old_state

> + * instances.

btw for even more fanciness you could do an example of what a driver's
foo_bar_duplicate_state() function would look like, using this. That would
be even better I think. And maybe s/foo/drv/ for clarity ...

Just ideas, feel free to prettify as you see fit.

Cheers, Daniel

> + */
> +#define drm_atomic_helper_duplicate_custom_state(new_state, old_state, basename) \
> +	do { \
> +		size_t base_size = sizeof(new_state->basename); \
> +		size_t base_offset = offsetof(typeof(*new_state), basename); \
> +		\
> +		BUILD_BUG_ON(base_offset != 0); \
> +		if (new_state == old_state) /* Type-check */ \
> +			break; \
> +		memcpy((char *)new_state + base_size, \
> +		       (char *)old_state + base_size, \
> +		       sizeof(*new_state) - base_size); \
> +	} while(0)
> +
> +/**
>   * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
>   * @plane: the loop cursor
>   * @crtc:  the crtc whose planes are iterated
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch
diff mbox

diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index d066e94..ecc6a82 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -171,6 +171,39 @@  int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
 				       uint32_t size);
 
 /**
+ * drm_atomic_duplicate_custom_state - helper macro for duplicating
+ * driver-private additions to drm_*_state.
+ * @new_state: pointer to destination state struct
+ * @old_state: pointer to source state struct
+ * @basename: name of the drm_*_state member of the new_state/old_state struct
+ *
+ * Copies the data after the base struct until the end of the custom struct,
+ * e.g. given a structure
+ *
+ * struct foo_bar_state {
+ * 	struct drm_bar_state base;
+ * 	struct foo_private priv;
+ * 	struct foo_private2 *priv2;
+ * };
+ *
+ * this copies priv and priv2. NB: the base struct *must* be the first element
+ * of the derived struct, and new_state and old_state have to be two distinct
+ * instances.
+ */
+#define drm_atomic_helper_duplicate_custom_state(new_state, old_state, basename) \
+	do { \
+		size_t base_size = sizeof(new_state->basename); \
+		size_t base_offset = offsetof(typeof(*new_state), basename); \
+		\
+		BUILD_BUG_ON(base_offset != 0); \
+		if (new_state == old_state) /* Type-check */ \
+			break; \
+		memcpy((char *)new_state + base_size, \
+		       (char *)old_state + base_size, \
+		       sizeof(*new_state) - base_size); \
+	} while(0)
+
+/**
  * drm_atomic_crtc_for_each_plane - iterate over planes currently attached to CRTC
  * @plane: the loop cursor
  * @crtc:  the crtc whose planes are iterated