| Message ID | 20241219214909.104869-3-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/display: Reduce global state funcs boilerplate | expand |
-----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. > + 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 > >
On Thu, 19 Dec 2024, Gustavo Sousa <gustavo.sousa@intel.com> 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; > +} > + > +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
On Thu, 19 Dec 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote: > 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> What do you need this for? > +#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
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. 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. > +} > + > +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
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 >> >>
Quoting Jani Nikula (2024-12-20 05:50:05-03:00) >On Thu, 19 Dec 2024, Gustavo Sousa <gustavo.sousa@intel.com> 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; >> +} >> + >> +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". Yeah, I kept wondering about this and even considered a possible rename of the struct type, but thought that would be probably too much just because of two fields that should be considered private. > >They would have to be passed to intel_atomic_global_obj_init() and >initialized runtime. That's a downside. That sounds okay to me, although I would prefer something that would not require explicit calls to sizeof() and offsetof(). > 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. Yeah, passing NULL seems like a good idea as it allows one to even skip defining the funcs structure, but I think dropping INTEL_GLOBAL_STATE_DEFAULTS() would make this an "all or nothing" type of stuff: if you just need to customize one behavior, you will need to define the whole structure explicitly. -- Gustavo Sousa > >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
Quoting Jani Nikula (2024-12-20 05:51:51-03:00) >On Thu, 19 Dec 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote: >> 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> > >What do you need this for? Because of offsetof(). -- Gustavo Sousa > >> +#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 > >-- >Jani Nikula, Intel
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
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; + 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
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(-)