Re: [PATCH v7 3/4] drm/xe/hwmon: Update xe_hwmon_process_reg

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 18-04-2024 18:44, Lucas De Marchi wrote:
> On Tue, Apr 09, 2024 at 02:52:07PM GMT, Rodrigo Vivi wrote:
>> On Tue, Apr 09, 2024 at 12:52:34PM -0500, Lucas De Marchi wrote:
>>> On Fri, Apr 05, 2024 at 06:31:26PM +0530, Karthik Poosa wrote:
>>> > Return u64 from xe_hwmon_process_reg, instead of a void return.
>>> > u64* input pointer not needed with this change.
>>> >
>>> > With this caller can directly assign return value to a variable 
>>> without
>>> > need of explicit initialization and pass by reference.
>>> >
>>> > v2:
>>> > - Fix checkpatch warnings.
>>> >
>>> > v3:
>>> > - Rebase
>>> > - Removed unncessary break statements.
>>> >
>>> > Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
>>> > Suggested-by: Lucas De Marchi <lucas.demarchi@intel.com>
>>> > Cc: Badal Nilawar <badal.nilawar@intel.com>
>>>
>>> Applied the other patches.  This one I'm putting on hold to think about.
>>>
>>> I'm not sure the approach in that hwmon in general is good with the
>>> xe_hwmon_get_reg() + xe_hwmon_process_reg(). It seems it's even taking
>>> some pm refs when it doesn't need (to decide if attribute is visible).
>>
>> I believe this approach is fine.
>> We do need to earlier get pm refs if we believe that there will be mmio
>> operations underneath. Better more then less in this case.
> 
> see this example:
> 
> static umode_t xe_hwmon_attributes_visible(struct kobject *kobj, 
>                                             struct attribute *attr, int 
> index) {         struct device *dev = kobj_to_dev(kobj);         struct 
> xe_hwmon *hwmon = dev_get_drvdata(dev);         int ret = 0;         
> xe_pm_runtime_get(gt_to_xe(hwmon->gt));         ret = 
> xe_reg_is_valid(xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, index)) ? 
> attr->mode : 0;         xe_pm_runtime_put(gt_to_xe(hwmon->gt));         
> return ret; } that xe_pm_runtime_get() is totally bogus. We are 
> basically doing a
> SW-only check, calling xe_hwmon_get_reg() and xe_reg_is_valid().
> It's only xe_hwmon_process_reg() that actually read/write/rmw anything.
> I think we are following the approach "upon any entry from sysfs,
> xe_pm_runtime_get(), lock hwmon, etc", which I don't like.
hwmon lock are taken only for write and rmw functions and not for read 
function. Suggestion was to avoid taking lock before rpm get.

RPM call may not be needed for above case but not for attributes 
curr_crit, power_crit as these are pcode mailbox based attributes. For 
now rpm calls can be moved to one level down but going further for 
certain attributes we might need to perform read operation. So it make 
sense to keep rpm calls in top level visible function.

Moreover visible functions are called once only during driver load.
> 
> Besides that...  xe_hwmon_process_reg() is an umbrella function for no
> good reason. The caller hardcodes the OP. It could very well have called
> the right function and just didn't because xe_hwmon_process_reg() also
> bundles getting the reg.
> 
> taking xe_hwmon_power_max_read as example, why can't we
> split the get_reg() / read_reg() like below?
> 
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 453e601ddd5e..8ce8d9a66df9 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -160,10 +160,22 @@ static void xe_hwmon_process_reg(struct xe_hwmon 
> *hwmon, enum xe_hwmon_reg hwmon
>   static void xe_hwmon_power_max_read(struct xe_hwmon *hwmon, int 
> channel, long *value)
>   {
>          u64 reg_val, min, max;
> +       struct xe_reg reg_power_sku, reg_rapl_limit;
> +
> +       reg_rapl_limit = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, 
> channel);
> +       reg_power_sku = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, 
> channel);
> +
> +       /* XXX could probably be xe_gt_assert() or add a warning */
> +       if (!xe_reg_is_valid(reg_power_sku) || 
> !xe_reg_is_valid(reg_rapl_limit))
> +               return;
> 
>          mutex_lock(&hwmon->hwmon_lock);
> 
> -       xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ32, 
> &reg_val, 0, 0, channel);
> +       reg_val = xe_mmio_read32(hwmon->gt, reg_rapl_limit);
>          /* Check if PL1 limit is disabled */
>          if (!(reg_val & PKG_PWR_LIM_1_EN)) {
>                  *value = PL1_DISABLE;
> @@ -173,7 +185,7 @@ static void xe_hwmon_power_max_read(struct xe_hwmon 
> *hwmon, int channel, long *v
>          reg_val = REG_FIELD_GET(PKG_PWR_LIM_1, reg_val);
>          *value = mul_u64_u32_shr(reg_val, SF_POWER, 
> hwmon->scl_shift_power);
> 
> -       xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU, REG_READ64, 
> &reg_val, 0, 0, channel);
> +       reg_val = xe_mmio_read64_2x32(hwmon->gt, reg_power_sku);
>          min = REG_FIELD_GET(PKG_MIN_PWR, reg_val);
>          min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
>          max = REG_FIELD_GET(PKG_MAX_PWR, reg_val);
> 
> 
> Yes, it's longer, but it also uncovers one issue that could have been a
> silent error: if there's a programming mistake and
> xe_hwmon_process_reg() returns a valid register for REG_PKG_RAPL_LIMIT
> but not REG_PKG_POWER_SKU, we'd basically use the value from the first
> register in the calculation, which is wrong and with no
> warning/error/whatever. Btw, should we change the visible()
> method to cover that?
yes visible method for power_max attribute if we add additional check 
for REG_PKG_POWER_SKU above case will not arrive.
> 
> I's my personal feeling. I don't like the pattern xe_hwmon.c is using
> since it's very easy to introduce buggy code. If someone wants to give a
> r-b and merge this particular patch, fine. But I do think this pattern
> should be changed.
Idea of xe_hwmon_process_reg to abstract the register accesses. In i915 
we maintain register addresses inside the hwmon structure. During 
reviews that idea was not much likeable. So we came up with 
xe_hwmon_get_reg, xe_hwmon_process_reg.

Regards,
Badal
> 
> Lucas De Marchi
> 
>>
>>>
>>> Lucas De Marchi