Re: [Intel-gfx] [PATCH 2/4] drm/i915/cdclk: un-inline intel_cdclk_state functions

[Jani Nikula <jani.nikula@intel.com>]

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

-- 
Jani Nikula, Intel Open Source Graphics Center