[PATCH 1/4] drm/i915/pmdemand: convert to_intel_pmdemand_state() to a function

[Gustavo Sousa]

Quoting Jani Nikula (2024-12-31 13:27:37-03:00)
>In preparation for making struct intel_pmdemand_state an opaque type,
>convert to_intel_pmdemand_state() to a function.
>
>Cc: Gustavo Sousa <gustavo.sousa at intel.com>
>Signed-off-by: Jani Nikula <jani.nikula at intel.com>

This looks good to me, so

Reviewed-by: Gustavo Sousa <gustavo.sousa at intel.com>

, but I'm also taking this opportunity to reply to your comment below.

>
>---
>
>This is the simplest change. There could be other alternatives.
>
>Outside of intel_pmdemand.c, this is only used to convert
>display.pmdemand.obj.state to struct intel_pmdemand_state *. Maybe we
>could just pass the global object or state pointer instead? Or we could
>have a function to get the current state from, say, struct
>intel_display? What we currently have is a bit cumbersome.

I like the idea of the exposed interface receiving only pointers to the
generic types and that we make the necessary conversion internally.

We currently are only using to_intel_pmdemand_state() to be able to pass
the correct argument to other functions exposed by the pmdemand header.
Not sure there is much benefit in doing that except for some level of
compile-time type-safety?

So, I would generally say:

- For functions that can operate directly on the display.*.obj member
  (e.g. hardware state readout), we just ask for the display struct
  pointer as a parameter.

- For functions that potentially add the global state to the atomic
  state, we also ask for the pointer to the atomic state.

- For functions that operate only on the state bits and that could be
  called for a state instance that could either be the current one (or
  old) or some new state during a commit, we ask for a pointer to the
  intel_global_state struct.

--
Gustavo Sousa

>---
> drivers/gpu/drm/i915/display/intel_pmdemand.c | 5 +++++
> drivers/gpu/drm/i915/display/intel_pmdemand.h | 3 +--
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_pmdemand.c b/drivers/gpu/drm/i915/display/intel_pmdemand.c
>index cdd314956a31..69b40b3735b3 100644
>--- a/drivers/gpu/drm/i915/display/intel_pmdemand.c
>+++ b/drivers/gpu/drm/i915/display/intel_pmdemand.c
>@@ -15,6 +15,11 @@
> #include "intel_pmdemand.h"
> #include "skl_watermark.h"
> 
>+struct intel_pmdemand_state *to_intel_pmdemand_state(struct intel_global_state *obj_state)
>+{
>+        return container_of(obj_state, struct intel_pmdemand_state, base);
>+}
>+
> static struct intel_global_state *
> intel_pmdemand_duplicate_state(struct intel_global_obj *obj)
> {
>diff --git a/drivers/gpu/drm/i915/display/intel_pmdemand.h b/drivers/gpu/drm/i915/display/intel_pmdemand.h
>index a1c49efdc493..89296364ec3b 100644
>--- a/drivers/gpu/drm/i915/display/intel_pmdemand.h
>+++ b/drivers/gpu/drm/i915/display/intel_pmdemand.h
>@@ -43,8 +43,7 @@ struct intel_pmdemand_state {
>         struct pmdemand_params params;
> };
> 
>-#define to_intel_pmdemand_state(global_state) \
>-        container_of_const((global_state), struct intel_pmdemand_state, base)
>+struct intel_pmdemand_state *to_intel_pmdemand_state(struct intel_global_state *obj_state);
> 
> void intel_pmdemand_init_early(struct drm_i915_private *i915);
> int intel_pmdemand_init(struct drm_i915_private *i915);
>-- 
>2.39.5
>