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

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

Quoting Cavitt, Jonathan (2024-12-19 19:44:12-03:00)
>-----Original Message-----
>From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Gustavo Sousa
>Sent: Thursday, December 19, 2024 1:49 PM
>To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>; Nikula, Jani <jani.nikula@intel.com>
>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@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)
>
>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.

It is a bug obj->funcs to be NULL. The funcs member is initialized
with intel_atomic_global_obj_init(), which must be called for every
tracked global object.

Furthermore, checking (obj->funcs && obj->funcs->atomic_destroy_state)
would be problematic because
intel_atomic_global_duplicate_state_common() relies on obj->funcs being
a valid pointer (as does all of the global state logic).

--
Gustavo Sousa

>
>> +                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@intel.com>
>-Jonathan Cavitt
>
>>  #endif
>> -- 
>> 2.47.1
>> 
>>