Re: [PATCH 5/7] drm/i915/hwmon: Expose card reactive critical power

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

On 9/22/2022 8:47 AM, Dixit, Ashutosh wrote:
> On Wed, 21 Sep 2022 08:07:15 -0700, Gupta, Anshuman wrote:
>>
> 
> Hi Anshuman,
> 
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 55c35903adca..956e5298ef1e 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -6644,6 +6644,12 @@
>>>    #define   DG1_PCODE_STATUS			0x7E
>>>    #define     DG1_UNCORE_GET_INIT_STATUS		0x0
>>>    #define     DG1_UNCORE_INIT_STATUS_COMPLETE	0x1
>>> +#define   PCODE_POWER_SETUP			0x7C
>>> +#define     POWER_SETUP_SUBCOMMAND_READ_I1	0x4
>>> +#define     POWER_SETUP_SUBCOMMAND_WRITE_I1	0x5
>>> +#define	    POWER_SETUP_I1_WATTS		REG_BIT(31)
>>> +#define	    POWER_SETUP_I1_SHIFT		6	/* 10.6 fixed point format */
>> Could please add some comment to explain, why POWER_SETUP_I1_SHIFT  = 6,
>> what is excatly 10.6 fixed point format ?
>> With that.
>> Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> 
> 10.6 fixed point format means a 16 bit number is represented as x.y where x
> are the top 10 bits and y are the bottom 6 bits. The float value of this 16
> bit number is (x + (y / 2^6)), so (x + (y / 64)). For example the number
> 0x8008 will have the value (1 * 2^9 + 8 / 2^6) == 512.125. Note that the
> hexadecimal number 0x8008 == 32776 and 512.125 == 32776 / 64 which is why
> POWER_SETUP_I1_SHIFT is 6 (2^6 == 64).
> 
> Similarly, the 8.8 fixed point format is explained in
> gt/intel_gt_sysfs_pm.c. Do you think this needs a comment? I thought "10.6
> fixed point format" is a sufficient hint (fixed point numbers are fairly
> well known).
> 
> An even trickier data format is in the patch "drm/i915/hwmon: Expose
> power1_max_interval" in hwm_power1_max_interval_show() but I think I have a
> long comment there.
Thanks for explaining this, i was unaware of fixed point representation.
My RB can can be used without any change.
Br,
Anshuman.

> 
> Thanks.
> --
> Ashutosh