On Thu, 19 Dec 2024, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote:
> 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)
> + 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)
> + 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)
> +{
> + 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;
> +
Gut feeling says these should be part of struct intel_global_state
rather than struct intel_global_state_funcs. Keyword being "funcs".
They would have to be passed to intel_atomic_global_obj_init() and
initialized runtime. That's a downside. But then you could do away with
the funcs struct altogether when defaults are used, and pass NULL.
And you could also drop INTEL_GLOBAL_STATE_DEFAULTS() which I don't find
particularly pretty.
BR,
Jani.
> 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);
> +
> #endif
--
Jani Nikula, Intel
