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

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

On Tue, 31 Dec 2024, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> 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@intel.com>
>>Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> This looks good to me, so
>
> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
>
> , but I'm also taking this opportunity to reply to your comment below.

Thanks. I opted to merge this series as-is, with the idea that we can
make further improvements on top.

BR,
Jani.

>
>>
>>---
>>
>>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
>>

-- 
Jani Nikula, Intel