[PATCH v5 2/4] drm/xe/hwmon: Update xe_hwmon_get_reg to return struct xe_reg

[Lucas De Marchi]

On Thu, Apr 04, 2024 at 03:24:53PM +0530, Poosa, Karthik wrote:
>
>On 04-04-2024 14:16, Jani Nikula wrote:
>>On Thu, 04 Apr 2024, Karthik Poosa <karthik.poosa at intel.com> wrote:
>>>Return register address as struct xe_reg from xe_hwmon_get_reg
>>>instead of u32. (Lucas)
>>>Use XE_REG_IS_VALID() from caller to check returned xe_reg. (Badal)
>>>These changes are done to have abstracted usage of struct xe_reg.
>>>
>>>Signed-off-by: Karthik Poosa <karthik.poosa at intel.com>
>>>---
>>>  drivers/gpu/drm/xe/xe_hwmon.c | 47 +++++++++++++++++++----------------
>>>  1 file changed, 26 insertions(+), 21 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
>>>index 7e8caac838e0..2385f05d9504 100644
>>>--- a/drivers/gpu/drm/xe/xe_hwmon.c
>>>+++ b/drivers/gpu/drm/xe/xe_hwmon.c
>>>@@ -79,46 +79,46 @@ struct xe_hwmon {
>>>  	struct xe_hwmon_energy_info ei[CHANNEL_MAX];
>>>  };
>>>-static u32 xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg, int channel)
>>>+static struct xe_reg xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
>>>+				      int channel)
>>>  {
>>>  	struct xe_device *xe = gt_to_xe(hwmon->gt);
>>>-	struct xe_reg reg = XE_REG(0);
>>>  	switch (hwmon_reg) {
>>>  	case REG_PKG_RAPL_LIMIT:
>>>  		if (xe->info.platform == XE_PVC && channel == CHANNEL_PKG)
>>>-			reg = PVC_GT0_PACKAGE_RAPL_LIMIT;
>>>+			return PVC_GT0_PACKAGE_RAPL_LIMIT;
>>>  		else if ((xe->info.platform == XE_DG2) && (channel == CHANNEL_PKG))
>>>-			reg = PCU_CR_PACKAGE_RAPL_LIMIT;
>>>+			return PCU_CR_PACKAGE_RAPL_LIMIT;
>>>  		break;
>>>  	case REG_PKG_POWER_SKU:
>>>  		if (xe->info.platform == XE_PVC && channel == CHANNEL_PKG)
>>>-			reg = PVC_GT0_PACKAGE_POWER_SKU;
>>>+			return PVC_GT0_PACKAGE_POWER_SKU;
>>>  		else if ((xe->info.platform == XE_DG2) && (channel == CHANNEL_PKG))
>>>-			reg = PCU_CR_PACKAGE_POWER_SKU;
>>>+			return PCU_CR_PACKAGE_POWER_SKU;
>>>  		break;
>>>  	case REG_PKG_POWER_SKU_UNIT:
>>>  		if (xe->info.platform == XE_PVC)
>>>-			reg = PVC_GT0_PACKAGE_POWER_SKU_UNIT;
>>>+			return PVC_GT0_PACKAGE_POWER_SKU_UNIT;
>>>  		else if (xe->info.platform == XE_DG2)
>>>-			reg = PCU_CR_PACKAGE_POWER_SKU_UNIT;
>>>+			return PCU_CR_PACKAGE_POWER_SKU_UNIT;
>>>  		break;
>>>  	case REG_GT_PERF_STATUS:
>>>  		if (xe->info.platform == XE_DG2 && channel == CHANNEL_PKG)
>>>-			reg = GT_PERF_STATUS;
>>>+			return GT_PERF_STATUS;
>>>  		break;
>>>  	case REG_PKG_ENERGY_STATUS:
>>>  		if (xe->info.platform == XE_PVC && channel == CHANNEL_PKG)
>>>-			reg = PVC_GT0_PLATFORM_ENERGY_STATUS;
>>>+			return PVC_GT0_PLATFORM_ENERGY_STATUS;
>>>  		else if ((xe->info.platform == XE_DG2) && (channel == CHANNEL_PKG))
>>>-			reg = PCU_CR_PACKAGE_ENERGY_STATUS;
>>>+			return PCU_CR_PACKAGE_ENERGY_STATUS;
>>>  		break;
>>>  	default:
>>>  		drm_warn(&xe->drm, "Unknown xe hwmon reg id: %d\n", hwmon_reg);
>>>  		break;
>>>  	}
>>>-	return reg.raw;
>>>+	return XE_REG(0);
>>You might want to look at i915_reg_defs.h, which abstracts away the
>>magic 0 as the invalid address, and provides type safety for the reg
>>validity checks via _Generic:
>>
>>#define INVALID_MMIO_REG _MMIO(0)
>>
>>#define i915_mmio_reg_offset(r) \
>>	_Generic((r), i915_reg_t: (r).reg, i915_mcr_reg_t: (r).reg)
>>#define i915_mmio_reg_equal(a, b) (i915_mmio_reg_offset(a) == i915_mmio_reg_offset(b))
>>#define i915_mmio_reg_valid(r) (!i915_mmio_reg_equal(r, INVALID_MMIO_REG))
>>
>>BR,
>>Jani.
>Lucas, do we need similar abstraction here ?

we can add that later, but we will may need to be more careful in xe as it's
the _addr_ that shows if a register is valid or not. In i915
i915_reg_t is just a struct wrapping the address. In xe it's wrapping a
union that contains the address.

XE_REG(0) is _a_ invalid register, not _the_ invalid register.
XE_REG(0, XE_REG_OPTION_MASKED) is invalid as well. Defining INVALID_XE_REG
could be misleading.

Yeah, having XE_REG(0) in the middle of the code may not be pretty,
but I think we can live with that for now.

Ditto for the equality check. In same places we want a equality with the
.raw, not the .addr to make sure we are not mixing variants of the
register for different platforms. We check this e.g. in the reg_sr
infra.

Lucas De Marchi