-----Original Message-----
From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Gustavo Sousa
Sent: Thursday, December 19, 2024 1:49 PM
To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx
Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>; Nikula, Jani <jani.nikula@xxxxxxxxx>
Subject: [PATCH 2/3] drm/i915/display: Add infra to reduce global state funcs boilerplate
>
> If we look at how the members of struct intel_global_state_funcs, we see
> a common pattern repeating itself. Let's add the necessary
> infra-structure to allow reducing the boilerplate. We do that by
> adding common generic implementations for each member and adding a macro
> INTEL_GLOBAL_STATE_DEFAULTS() to be used when initializing an instance
> of struct intel_global_state_funcs.
>
> That way, a global state that does not need custom behavior can have
> its funcs structure be initialized as in the following example,
>
> static const struct intel_global_state_funcs <prefix>_funcs = {
> INTEL_GLOBAL_STATE_DEFAULTS(struct <prefix>_state, <base_member_name>),
> };
>
> , without the need to implementing the functions.
>
> That doesn't come without cost - we will need to store two size_t
> members -, but that cost is arguably justified by the simplification
> gained.
>
> In an upcoming change we will put that infra into action on existing
> users.
>
> Signed-off-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
> ---
> .../gpu/drm/i915/display/intel_global_state.c | 41 ++++++++++++++++++-
> .../gpu/drm/i915/display/intel_global_state.h | 15 +++++++
> 2 files changed, 54 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_global_state.c b/drivers/gpu/drm/i915/display/intel_global_state.c
> index cbcd1e91b7be..4b4c33fa99fb 100644
> --- a/drivers/gpu/drm/i915/display/intel_global_state.c
> +++ b/drivers/gpu/drm/i915/display/intel_global_state.c
> @@ -59,7 +59,10 @@ static void __intel_atomic_global_state_free(struct kref *kref)
>
> commit_put(obj_state->commit);
>
> - obj->funcs->atomic_destroy_state(obj, obj_state);
> + if (obj->funcs->atomic_destroy_state)
This is good, though I think it's standard practice as a part of
these kinds of sanity checks to assert that the functions pointer
also exists before attempting to dereference into a function itself.
Or, in simpler terms, I think we want to check obj->funcs here,
in addition to the atomic_destroy_state:
"""
If (obj->funcs && obj->funcs->atomic_destroy_state)
"""
Though maybe obj->funcs has some guarantee associated with
it that ensures it always exists, making this sanity check unnecessary?
Much like the guarantee obj exists? I'm not familiar enough with the
display driver one way or the other to make that declaration, so I
won't block on this.
> + obj->funcs->atomic_destroy_state(obj, obj_state);
> + else
> + intel_atomic_global_destroy_state_common(obj, obj_state);
> }
>
> static void intel_atomic_global_state_put(struct intel_global_state *obj_state)
> @@ -164,7 +167,11 @@ intel_atomic_get_global_obj_state(struct intel_atomic_state *state,
> index = state->num_global_objs;
> memset(&state->global_objs[index], 0, sizeof(*state->global_objs));
>
> - obj_state = obj->funcs->atomic_duplicate_state(obj);
> + if (obj->funcs->atomic_duplicate_state)
Same comment as above, except with atomic_duplicate_state
instead of atomic_destroy_state.
> + obj_state = obj->funcs->atomic_duplicate_state(obj);
> + else
> + obj_state = intel_atomic_global_duplicate_state_common(obj);
> +
> if (!obj_state)
> return ERR_PTR(-ENOMEM);
>
> @@ -382,3 +389,33 @@ intel_atomic_global_state_commit_done(struct intel_atomic_state *state)
> complete_all(&commit->done);
> }
> }
> +
> +struct intel_global_state *
> +intel_atomic_global_duplicate_state_common(struct intel_global_obj *obj)
I personally prefer these kinds of functions to be defined before their
first usage when possible, as it mirrors how we need to define static
functions before their first uses. However, I recognize that because
this function is defined in intel_global_state.h, it's not necessary to
maintain that kind of function ordering and, in fact, it's more
important to maintain function ordering parity with the header
file. So I'll leave that kind of change to your discretion.
> +{
> + void *state_wrapper;
> +
> + if (WARN_ON(obj->funcs->state_size == 0))
> + return NULL;
> +
> + state_wrapper = (void *)obj->state - obj->funcs->base_offset;
> +
> + state_wrapper = kmemdup(state_wrapper, obj->funcs->state_size, GFP_KERNEL);
> + if (!state_wrapper)
> + return NULL;
> +
> + return state_wrapper + obj->funcs->base_offset;
> +}
> +
> +void intel_atomic_global_destroy_state_common(struct intel_global_obj *obj,
> + struct intel_global_state *state)
> +{
> + void *state_wrapper;
> +
> + if (WARN_ON(obj->funcs->state_size == 0))
> + return;
> +
> + state_wrapper = (void *)state - obj->funcs->base_offset;
> +
> + kfree(state_wrapper);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_global_state.h b/drivers/gpu/drm/i915/display/intel_global_state.h
> index 6506a8e32972..e47e007225cc 100644
> --- a/drivers/gpu/drm/i915/display/intel_global_state.h
> +++ b/drivers/gpu/drm/i915/display/intel_global_state.h
> @@ -8,6 +8,8 @@
>
> #include <linux/kref.h>
> #include <linux/list.h>
> +#include <linux/stddef.h>
> +#include <linux/types.h>
>
> struct drm_i915_private;
> struct intel_atomic_state;
> @@ -15,6 +17,10 @@ struct intel_global_obj;
> struct intel_global_state;
>
> struct intel_global_state_funcs {
> + /* state_size and base_offset are initialized by INTEL_GLOBAL_STATE_DEFAULTS() */
> + size_t state_size;
> + size_t base_offset;
> +
> struct intel_global_state *(*atomic_duplicate_state)(struct intel_global_obj *obj);
> void (*atomic_destroy_state)(struct intel_global_obj *obj,
> struct intel_global_state *state);
> @@ -26,6 +32,10 @@ struct intel_global_obj {
> const struct intel_global_state_funcs *funcs;
> };
>
> +#define INTEL_GLOBAL_STATE_DEFAULTS(type, base_member) \
> + .state_size = sizeof(type), \
> + .base_offset = offsetof(type, base_member)
> +
> #define intel_for_each_global_obj(obj, dev_priv) \
> list_for_each_entry(obj, &(dev_priv)->display.global.obj_list, head)
>
> @@ -96,4 +106,9 @@ int intel_atomic_global_state_wait_for_dependencies(struct intel_atomic_state *s
>
> bool intel_atomic_global_state_is_serialized(struct intel_atomic_state *state);
>
> +struct intel_global_state *
> +intel_atomic_global_duplicate_state_common(struct intel_global_obj *obj);
> +void intel_atomic_global_destroy_state_common(struct intel_global_obj *obj,
> + struct intel_global_state *state);
> +
I have no major complaints. Just some notes above.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx>
-Jonathan Cavitt
> #endif
> --
> 2.47.1
>
>
