On Tue, 31 Dec 2024, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> 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@xxxxxxxxx>
>>Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>
> This looks good to me, so
>
> Reviewed-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
>
> , 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
