Re: [PATCH 2/3] drm/i915/display: Add infra to reduce global state funcs boilerplate

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Ville Syrjälä (2024-12-20 06:23:24-03:00)
>On Thu, Dec 19, 2024 at 06:48:37PM -0300, Gustavo Sousa 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@intel.com>
>> ---
>>  .../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;
>
>I'm not really a fan. What was obvious code before now looks 
>complicated.

Well it is 1 somewhat complication here to simplify N cases in the
subclasses. Maybe that's worth it?

>
>Also this no longer matches how any of the standard kms object
>types work, which I don't think is a good idea. IMO if we
>want to do something like this then it should probably try to
>cover all kms object types.

Do you mean to try to implement this on the DRM layer? In that case, we
would also endup having to implement this here right? Because we have
our own thing. Unless I interpreted your comment above wrong...

--
Gustavo Sousa
>
>> +}
>> +
>> +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);
>> +
>>  #endif
>> -- 
>> 2.47.1
>
>-- 
>Ville Syrjälä
>Intel