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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel