| Message ID | 66f687db7321846049a6aa524dfafd45cf0cb77f.1639068649.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/cdclk: improve abstractions | expand |
On Thu, Dec 09, 2021 at 06:51:23PM +0200, Jani Nikula wrote: > Hide the details better. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/display/intel_cdclk.c | 18 ++++++++++++++++++ > drivers/gpu/drm/i915/display/intel_cdclk.h | 13 ++++++++----- > 2 files changed, 26 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c > index a216a350006d..84674a4f7226 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c > @@ -2630,6 +2630,24 @@ intel_atomic_get_cdclk_state(struct intel_atomic_state *state) > return to_intel_cdclk_state(cdclk_state); > } > > +struct intel_cdclk_state * > +to_intel_cdclk_state(struct intel_global_state *cdclk_state) > +{ > + return container_of(cdclk_state, struct intel_cdclk_state, base); > +} > + > +struct intel_cdclk_state * > +intel_atomic_get_old_cdclk_state(struct intel_atomic_state *state) > +{ > + return to_intel_cdclk_state(intel_atomic_get_old_global_obj_state(state, &to_i915(state->base.dev)->cdclk.obj)); > +} > + > +struct intel_cdclk_state * > +intel_atomic_get_new_cdclk_state(struct intel_atomic_state *state) > +{ > + return to_intel_cdclk_state(intel_atomic_get_new_global_obj_state(state, &to_i915(state->base.dev)->cdclk.obj)); > +} > + Not really sure about this one. We don't do this for any other similar cases, and I think the macro versions are needed if we have any kind of const vs. non-const funny business going on. I guess in this particular case we don't, but pretty sure that was a real thing for some other atomic states when I was pondering about using functions rather than macros for those. So I'm tempted to say we should stick to a common pattern across the board if possible. > int intel_cdclk_atomic_check(struct intel_atomic_state *state, > bool *need_cdclk_calc) > { > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h > index bb3a778c506b..77e8c8e1708f 100644 > --- a/drivers/gpu/drm/i915/display/intel_cdclk.h > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h > @@ -76,11 +76,14 @@ int intel_cdclk_atomic_check(struct intel_atomic_state *state, > struct intel_cdclk_state * > intel_atomic_get_cdclk_state(struct intel_atomic_state *state); > > -#define to_intel_cdclk_state(x) container_of((x), struct intel_cdclk_state, base) > -#define intel_atomic_get_old_cdclk_state(state) \ > - to_intel_cdclk_state(intel_atomic_get_old_global_obj_state(state, &to_i915(state->base.dev)->cdclk.obj)) > -#define intel_atomic_get_new_cdclk_state(state) \ > - to_intel_cdclk_state(intel_atomic_get_new_global_obj_state(state, &to_i915(state->base.dev)->cdclk.obj)) > +struct intel_cdclk_state * > +to_intel_cdclk_state(struct intel_global_state *cdclk_state); > + > +struct intel_cdclk_state * > +intel_atomic_get_old_cdclk_state(struct intel_atomic_state *state); > + > +struct intel_cdclk_state * > +intel_atomic_get_new_cdclk_state(struct intel_atomic_state *state); > > int intel_cdclk_init(struct drm_i915_private *dev_priv); > > -- > 2.30.2
On Thu, 09 Dec 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Dec 09, 2021 at 06:51:23PM +0200, Jani Nikula wrote: >> Hide the details better. >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_cdclk.c | 18 ++++++++++++++++++ >> drivers/gpu/drm/i915/display/intel_cdclk.h | 13 ++++++++----- >> 2 files changed, 26 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c >> index a216a350006d..84674a4f7226 100644 >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c >> @@ -2630,6 +2630,24 @@ intel_atomic_get_cdclk_state(struct intel_atomic_state *state) >> return to_intel_cdclk_state(cdclk_state); >> } >> >> +struct intel_cdclk_state * >> +to_intel_cdclk_state(struct intel_global_state *cdclk_state) >> +{ >> + return container_of(cdclk_state, struct intel_cdclk_state, base); >> +} >> + >> +struct intel_cdclk_state * >> +intel_atomic_get_old_cdclk_state(struct intel_atomic_state *state) >> +{ >> + return to_intel_cdclk_state(intel_atomic_get_old_global_obj_state(state, &to_i915(state->base.dev)->cdclk.obj)); >> +} >> + >> +struct intel_cdclk_state * >> +intel_atomic_get_new_cdclk_state(struct intel_atomic_state *state) >> +{ >> + return to_intel_cdclk_state(intel_atomic_get_new_global_obj_state(state, &to_i915(state->base.dev)->cdclk.obj)); >> +} >> + > > Not really sure about this one. We don't do this for any other similar > cases, and I think the macro versions are needed if we have any kind of > const vs. non-const funny business going on. I guess in this particular > case we don't, but pretty sure that was a real thing for some other > atomic states when I was pondering about using functions rather than > macros for those. > > So I'm tempted to say we should stick to a common pattern across the > board if possible. The main pattern I'm aiming for is "don't look into the guts of struct drm_i915_private in headers all over the place". That's the main headache for pretty much all the include un-tangling. I guess having them as macros makes it kind of work if the users include all the dependencies, but that's not pretty either. As an alternative, I was in fact looking for ways to make all of the above internal to intel_cdclk.c. Make struct intel_cdclk_state an opaque type, and add interfaces to peek and poke it instead of letting callers mess with it directly. Ditto with intel_dbuf_state and intel_bw_state. I think this is just one example of our tendency to add complicated object hierarchies, and then let everyone look at and modify their internals. I've sort of been on this crusade of adding interfaces and hiding stuff behind those interfaces, and I think long term that's the only way to keep the driver manageable. If this seems to bring about too many functions in an interface, I think it's also an indication the "poke at the objects directly" wasn't really properly thought out either. BR, Jani. > >> int intel_cdclk_atomic_check(struct intel_atomic_state *state, >> bool *need_cdclk_calc) >> { >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h >> index bb3a778c506b..77e8c8e1708f 100644 >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.h >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h >> @@ -76,11 +76,14 @@ int intel_cdclk_atomic_check(struct intel_atomic_state *state, >> struct intel_cdclk_state * >> intel_atomic_get_cdclk_state(struct intel_atomic_state *state); >> >> -#define to_intel_cdclk_state(x) container_of((x), struct intel_cdclk_state, base) >> -#define intel_atomic_get_old_cdclk_state(state) \ >> - to_intel_cdclk_state(intel_atomic_get_old_global_obj_state(state, &to_i915(state->base.dev)->cdclk.obj)) >> -#define intel_atomic_get_new_cdclk_state(state) \ >> - to_intel_cdclk_state(intel_atomic_get_new_global_obj_state(state, &to_i915(state->base.dev)->cdclk.obj)) >> +struct intel_cdclk_state * >> +to_intel_cdclk_state(struct intel_global_state *cdclk_state); >> + >> +struct intel_cdclk_state * >> +intel_atomic_get_old_cdclk_state(struct intel_atomic_state *state); >> + >> +struct intel_cdclk_state * >> +intel_atomic_get_new_cdclk_state(struct intel_atomic_state *state); >> >> int intel_cdclk_init(struct drm_i915_private *dev_priv); >> >> -- >> 2.30.2
diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c index a216a350006d..84674a4f7226 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.c +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c @@ -2630,6 +2630,24 @@ intel_atomic_get_cdclk_state(struct intel_atomic_state *state) return to_intel_cdclk_state(cdclk_state); } +struct intel_cdclk_state * +to_intel_cdclk_state(struct intel_global_state *cdclk_state) +{ + return container_of(cdclk_state, struct intel_cdclk_state, base); +} + +struct intel_cdclk_state * +intel_atomic_get_old_cdclk_state(struct intel_atomic_state *state) +{ + return to_intel_cdclk_state(intel_atomic_get_old_global_obj_state(state, &to_i915(state->base.dev)->cdclk.obj)); +} + +struct intel_cdclk_state * +intel_atomic_get_new_cdclk_state(struct intel_atomic_state *state) +{ + return to_intel_cdclk_state(intel_atomic_get_new_global_obj_state(state, &to_i915(state->base.dev)->cdclk.obj)); +} + int intel_cdclk_atomic_check(struct intel_atomic_state *state, bool *need_cdclk_calc) { diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.h b/drivers/gpu/drm/i915/display/intel_cdclk.h index bb3a778c506b..77e8c8e1708f 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.h +++ b/drivers/gpu/drm/i915/display/intel_cdclk.h @@ -76,11 +76,14 @@ int intel_cdclk_atomic_check(struct intel_atomic_state *state, struct intel_cdclk_state * intel_atomic_get_cdclk_state(struct intel_atomic_state *state); -#define to_intel_cdclk_state(x) container_of((x), struct intel_cdclk_state, base) -#define intel_atomic_get_old_cdclk_state(state) \ - to_intel_cdclk_state(intel_atomic_get_old_global_obj_state(state, &to_i915(state->base.dev)->cdclk.obj)) -#define intel_atomic_get_new_cdclk_state(state) \ - to_intel_cdclk_state(intel_atomic_get_new_global_obj_state(state, &to_i915(state->base.dev)->cdclk.obj)) +struct intel_cdclk_state * +to_intel_cdclk_state(struct intel_global_state *cdclk_state); + +struct intel_cdclk_state * +intel_atomic_get_old_cdclk_state(struct intel_atomic_state *state); + +struct intel_cdclk_state * +intel_atomic_get_new_cdclk_state(struct intel_atomic_state *state); int intel_cdclk_init(struct drm_i915_private *dev_priv);
Hide the details better. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/display/intel_cdclk.c | 18 ++++++++++++++++++ drivers/gpu/drm/i915/display/intel_cdclk.h | 13 ++++++++----- 2 files changed, 26 insertions(+), 5 deletions(-)