* [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro
2023-09-21 10:25 [PATCH v5 0/6] Add HWMON support for DGFX Badal Nilawar
@ 2023-09-21 10:25 ` Badal Nilawar
2023-09-21 16:03 ` Rodrigo Vivi
2023-09-21 10:25 ` [PATCH v5 2/6] drm/xe/hwmon: Expose power attributes Badal Nilawar
` (4 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: Badal Nilawar @ 2023-09-21 10:25 UTC (permalink / raw)
To: intel-xe, linux-hwmon
Cc: anshuman.gupta, ashutosh.dixit, linux, andi.shyti, riana.tauro,
matthew.brost, rodrigo.vivi
Add XE_MISSING_CASE macro to handle missing switch case
v2: Add comment about macro usage (Himal)
Cc: Andi Shyti <andi.shyti@linux.intel.com>
Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
---
drivers/gpu/drm/xe/xe_macros.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
index daf56c846d03..6c74c69920ed 100644
--- a/drivers/gpu/drm/xe/xe_macros.h
+++ b/drivers/gpu/drm/xe/xe_macros.h
@@ -15,4 +15,8 @@
"Ioctl argument check failed at %s:%d: %s", \
__FILE__, __LINE__, #cond), 1))
+/* Parameter to macro should be a variable name */
+#define XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
+ __stringify(x), (long)(x))
+
#endif
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro
2023-09-21 10:25 ` [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro Badal Nilawar
@ 2023-09-21 16:03 ` Rodrigo Vivi
2023-09-21 16:59 ` Nilawar, Badal
2023-09-22 15:16 ` Andi Shyti
0 siblings, 2 replies; 24+ messages in thread
From: Rodrigo Vivi @ 2023-09-21 16:03 UTC (permalink / raw)
To: Badal Nilawar
Cc: intel-xe, linux-hwmon, anshuman.gupta, ashutosh.dixit, linux,
andi.shyti, riana.tauro, matthew.brost
On Thu, Sep 21, 2023 at 03:55:14PM +0530, Badal Nilawar wrote:
> Add XE_MISSING_CASE macro to handle missing switch case
>
> v2: Add comment about macro usage (Himal)
>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> ---
> drivers/gpu/drm/xe/xe_macros.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
> index daf56c846d03..6c74c69920ed 100644
> --- a/drivers/gpu/drm/xe/xe_macros.h
> +++ b/drivers/gpu/drm/xe/xe_macros.h
> @@ -15,4 +15,8 @@
> "Ioctl argument check failed at %s:%d: %s", \
> __FILE__, __LINE__, #cond), 1))
>
> +/* Parameter to macro should be a variable name */
> +#define XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
> + __stringify(x), (long)(x))
> +
No, please! Let's not add unnecessary macros.
> #endif
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro
2023-09-21 16:03 ` Rodrigo Vivi
@ 2023-09-21 16:59 ` Nilawar, Badal
2023-09-22 10:05 ` [Intel-xe] " Jani Nikula
2023-09-22 15:16 ` Andi Shyti
1 sibling, 1 reply; 24+ messages in thread
From: Nilawar, Badal @ 2023-09-21 16:59 UTC (permalink / raw)
To: Rodrigo Vivi
Cc: intel-xe, linux-hwmon, anshuman.gupta, ashutosh.dixit, linux,
andi.shyti, riana.tauro, matthew.brost
On 21-09-2023 21:33, Rodrigo Vivi wrote:
> On Thu, Sep 21, 2023 at 03:55:14PM +0530, Badal Nilawar wrote:
>> Add XE_MISSING_CASE macro to handle missing switch case
>>
>> v2: Add comment about macro usage (Himal)
>>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_macros.h | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
>> index daf56c846d03..6c74c69920ed 100644
>> --- a/drivers/gpu/drm/xe/xe_macros.h
>> +++ b/drivers/gpu/drm/xe/xe_macros.h
>> @@ -15,4 +15,8 @@
>> "Ioctl argument check failed at %s:%d: %s", \
>> __FILE__, __LINE__, #cond), 1))
>>
>> +/* Parameter to macro should be a variable name */
>> +#define XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
>> + __stringify(x), (long)(x))
>> +
>
> No, please! Let's not add unnecessary macros.
This was suggested by Andy, in fact he suggested to reuse existing
MISSING_CASE macro from i915. As I couldn't find common place to move it
I went with creating new one.
I will drop this patch and simply use drm_warn.
Regards,
Badal
>
>> #endif
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-xe] [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro
2023-09-21 16:59 ` Nilawar, Badal
@ 2023-09-22 10:05 ` Jani Nikula
0 siblings, 0 replies; 24+ messages in thread
From: Jani Nikula @ 2023-09-22 10:05 UTC (permalink / raw)
To: Nilawar, Badal, Rodrigo Vivi; +Cc: linux-hwmon, intel-xe, linux
On Thu, 21 Sep 2023, "Nilawar, Badal" <badal.nilawar@intel.com> wrote:
> On 21-09-2023 21:33, Rodrigo Vivi wrote:
>> On Thu, Sep 21, 2023 at 03:55:14PM +0530, Badal Nilawar wrote:
>>> Add XE_MISSING_CASE macro to handle missing switch case
>>>
>>> v2: Add comment about macro usage (Himal)
>>>
>>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>>> Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>> ---
>>> drivers/gpu/drm/xe/xe_macros.h | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
>>> index daf56c846d03..6c74c69920ed 100644
>>> --- a/drivers/gpu/drm/xe/xe_macros.h
>>> +++ b/drivers/gpu/drm/xe/xe_macros.h
>>> @@ -15,4 +15,8 @@
>>> "Ioctl argument check failed at %s:%d: %s", \
>>> __FILE__, __LINE__, #cond), 1))
>>>
>>> +/* Parameter to macro should be a variable name */
>>> +#define XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
>>> + __stringify(x), (long)(x))
>>> +
>>
>> No, please! Let's not add unnecessary macros.
> This was suggested by Andy, in fact he suggested to reuse existing
> MISSING_CASE macro from i915. As I couldn't find common place to move it
> I went with creating new one.
>
> I will drop this patch and simply use drm_warn.
Please use drm_WARN() or drm_WARN_ON(). With panic_on_warn=1 in CI,
it'll oops the machine, and we'll actually catch these as opposed to
just leaving them as lines in dmesg.
I guess the main purpose of MISSING_CASE() in i915 was to unify the
behaviour, though I think that was also misused.
BR,
Jani.
>
> Regards,
> Badal
>>
>>> #endif
>>> --
>>> 2.25.1
>>>
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro
2023-09-21 16:03 ` Rodrigo Vivi
2023-09-21 16:59 ` Nilawar, Badal
@ 2023-09-22 15:16 ` Andi Shyti
2023-09-22 15:19 ` Gupta, Anshuman
2023-09-22 19:03 ` Rodrigo Vivi
1 sibling, 2 replies; 24+ messages in thread
From: Andi Shyti @ 2023-09-22 15:16 UTC (permalink / raw)
To: Rodrigo Vivi
Cc: Badal Nilawar, intel-xe, linux-hwmon, anshuman.gupta,
ashutosh.dixit, linux, andi.shyti, riana.tauro, matthew.brost
Hi Rodrigo,
On Thu, Sep 21, 2023 at 12:03:26PM -0400, Rodrigo Vivi wrote:
> On Thu, Sep 21, 2023 at 03:55:14PM +0530, Badal Nilawar wrote:
> > Add XE_MISSING_CASE macro to handle missing switch case
> >
> > v2: Add comment about macro usage (Himal)
> >
> > Cc: Andi Shyti <andi.shyti@linux.intel.com>
> > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_macros.h | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
> > index daf56c846d03..6c74c69920ed 100644
> > --- a/drivers/gpu/drm/xe/xe_macros.h
> > +++ b/drivers/gpu/drm/xe/xe_macros.h
> > @@ -15,4 +15,8 @@
> > "Ioctl argument check failed at %s:%d: %s", \
> > __FILE__, __LINE__, #cond), 1))
> >
> > +/* Parameter to macro should be a variable name */
> > +#define XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
> > + __stringify(x), (long)(x))
> > +
>
> No, please! Let's not add unnecessary macros.
it's not a bad idea, actually... in i915 we have the MISSING_CASE
for switch cases with a defined number of cases and print
warnings in case none if them is the one inside the switch.
It's so widely used and actually nice to have that I thought many
times to move it to some core kernel libraries.
Andi
^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro
2023-09-22 15:16 ` Andi Shyti
@ 2023-09-22 15:19 ` Gupta, Anshuman
2023-09-25 12:08 ` [Intel-xe] " Jani Nikula
2023-09-22 19:03 ` Rodrigo Vivi
1 sibling, 1 reply; 24+ messages in thread
From: Gupta, Anshuman @ 2023-09-22 15:19 UTC (permalink / raw)
To: Andi Shyti, Vivi, Rodrigo
Cc: Nilawar, Badal, intel-xe@lists.freedesktop.org,
linux-hwmon@vger.kernel.org, Dixit, Ashutosh, linux@roeck-us.net,
Tauro, Riana, Brost, Matthew
> -----Original Message-----
> From: Andi Shyti <andi.shyti@linux.intel.com>
> Sent: Friday, September 22, 2023 8:46 PM
> To: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Cc: Nilawar, Badal <badal.nilawar@intel.com>; intel-xe@lists.freedesktop.org;
> linux-hwmon@vger.kernel.org; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Dixit, Ashutosh <ashutosh.dixit@intel.com>;
> linux@roeck-us.net; andi.shyti@linux.intel.com; Tauro, Riana
> <riana.tauro@intel.com>; Brost, Matthew <matthew.brost@intel.com>
> Subject: Re: [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro
>
> Hi Rodrigo,
>
> On Thu, Sep 21, 2023 at 12:03:26PM -0400, Rodrigo Vivi wrote:
> > On Thu, Sep 21, 2023 at 03:55:14PM +0530, Badal Nilawar wrote:
> > > Add XE_MISSING_CASE macro to handle missing switch case
> > >
> > > v2: Add comment about macro usage (Himal)
> > >
> > > Cc: Andi Shyti <andi.shyti@linux.intel.com>
> > > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > > Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > ---
> > > drivers/gpu/drm/xe/xe_macros.h | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_macros.h
> > > b/drivers/gpu/drm/xe/xe_macros.h index daf56c846d03..6c74c69920ed
> > > 100644
> > > --- a/drivers/gpu/drm/xe/xe_macros.h
> > > +++ b/drivers/gpu/drm/xe/xe_macros.h
> > > @@ -15,4 +15,8 @@
> > > "Ioctl argument check failed at %s:%d: %s", \
> > > __FILE__, __LINE__, #cond), 1))
> > >
> > > +/* Parameter to macro should be a variable name */ #define
> > > +XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
> > > + __stringify(x), (long)(x))
> > > +
> >
> > No, please! Let's not add unnecessary macros.
>
> it's not a bad idea, actually... in i915 we have the MISSING_CASE for switch
> cases with a defined number of cases and print warnings in case none if them
> is the one inside the switch.
IMHO Our CI aborts the on MISSING_CASE, which is not recoverable, drm_warn would
Be better alternative here.
Thanks,
Anshuman Gupta.
>
> It's so widely used and actually nice to have that I thought many times to
> move it to some core kernel libraries.
>
> Andi
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Intel-xe] [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro
2023-09-22 15:19 ` Gupta, Anshuman
@ 2023-09-25 12:08 ` Jani Nikula
0 siblings, 0 replies; 24+ messages in thread
From: Jani Nikula @ 2023-09-25 12:08 UTC (permalink / raw)
To: Gupta, Anshuman, Andi Shyti, Vivi, Rodrigo
Cc: linux-hwmon@vger.kernel.org, intel-xe@lists.freedesktop.org,
linux@roeck-us.net
On Fri, 22 Sep 2023, "Gupta, Anshuman" <anshuman.gupta@intel.com> wrote:
>> -----Original Message-----
>> From: Andi Shyti <andi.shyti@linux.intel.com>
>> Sent: Friday, September 22, 2023 8:46 PM
>> To: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>> Cc: Nilawar, Badal <badal.nilawar@intel.com>; intel-xe@lists.freedesktop.org;
>> linux-hwmon@vger.kernel.org; Gupta, Anshuman
>> <anshuman.gupta@intel.com>; Dixit, Ashutosh <ashutosh.dixit@intel.com>;
>> linux@roeck-us.net; andi.shyti@linux.intel.com; Tauro, Riana
>> <riana.tauro@intel.com>; Brost, Matthew <matthew.brost@intel.com>
>> Subject: Re: [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro
>>
>> Hi Rodrigo,
>>
>> On Thu, Sep 21, 2023 at 12:03:26PM -0400, Rodrigo Vivi wrote:
>> > On Thu, Sep 21, 2023 at 03:55:14PM +0530, Badal Nilawar wrote:
>> > > Add XE_MISSING_CASE macro to handle missing switch case
>> > >
>> > > v2: Add comment about macro usage (Himal)
>> > >
>> > > Cc: Andi Shyti <andi.shyti@linux.intel.com>
>> > > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> > > Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> > > ---
>> > > drivers/gpu/drm/xe/xe_macros.h | 4 ++++
>> > > 1 file changed, 4 insertions(+)
>> > >
>> > > diff --git a/drivers/gpu/drm/xe/xe_macros.h
>> > > b/drivers/gpu/drm/xe/xe_macros.h index daf56c846d03..6c74c69920ed
>> > > 100644
>> > > --- a/drivers/gpu/drm/xe/xe_macros.h
>> > > +++ b/drivers/gpu/drm/xe/xe_macros.h
>> > > @@ -15,4 +15,8 @@
>> > > "Ioctl argument check failed at %s:%d: %s", \
>> > > __FILE__, __LINE__, #cond), 1))
>> > >
>> > > +/* Parameter to macro should be a variable name */ #define
>> > > +XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
>> > > + __stringify(x), (long)(x))
>> > > +
>> >
>> > No, please! Let's not add unnecessary macros.
>>
>> it's not a bad idea, actually... in i915 we have the MISSING_CASE for switch
>> cases with a defined number of cases and print warnings in case none if them
>> is the one inside the switch.
> IMHO Our CI aborts the on MISSING_CASE, which is not recoverable, drm_warn would
> Be better alternative here.
The whole point is that it aborts, so it won't get ignored. It's only
for cases like this.
BR,
Jani.
> Thanks,
> Anshuman Gupta.
>>
>> It's so widely used and actually nice to have that I thought many times to
>> move it to some core kernel libraries.
>>
>> Andi
--
Jani Nikula, Intel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro
2023-09-22 15:16 ` Andi Shyti
2023-09-22 15:19 ` Gupta, Anshuman
@ 2023-09-22 19:03 ` Rodrigo Vivi
2023-09-25 4:07 ` Nilawar, Badal
1 sibling, 1 reply; 24+ messages in thread
From: Rodrigo Vivi @ 2023-09-22 19:03 UTC (permalink / raw)
To: Andi Shyti
Cc: Badal Nilawar, intel-xe, linux-hwmon, anshuman.gupta,
ashutosh.dixit, linux, riana.tauro, matthew.brost
On Fri, Sep 22, 2023 at 05:16:23PM +0200, Andi Shyti wrote:
> Hi Rodrigo,
>
> On Thu, Sep 21, 2023 at 12:03:26PM -0400, Rodrigo Vivi wrote:
> > On Thu, Sep 21, 2023 at 03:55:14PM +0530, Badal Nilawar wrote:
> > > Add XE_MISSING_CASE macro to handle missing switch case
> > >
> > > v2: Add comment about macro usage (Himal)
> > >
> > > Cc: Andi Shyti <andi.shyti@linux.intel.com>
> > > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > > Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > ---
> > > drivers/gpu/drm/xe/xe_macros.h | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
> > > index daf56c846d03..6c74c69920ed 100644
> > > --- a/drivers/gpu/drm/xe/xe_macros.h
> > > +++ b/drivers/gpu/drm/xe/xe_macros.h
> > > @@ -15,4 +15,8 @@
> > > "Ioctl argument check failed at %s:%d: %s", \
> > > __FILE__, __LINE__, #cond), 1))
> > >
> > > +/* Parameter to macro should be a variable name */
> > > +#define XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
> > > + __stringify(x), (long)(x))
> > > +
> >
> > No, please! Let's not add unnecessary macros.
>
> it's not a bad idea, actually... in i915 we have the MISSING_CASE
> for switch cases with a defined number of cases and print
> warnings in case none if them is the one inside the switch.
>
> It's so widely used and actually nice to have that I thought many
> times to move it to some core kernel libraries.
to be really honest, I also liked the MISSING_CASE in many occasions.
It is useful for platform enabling so once we add a new hardware we
ensure to get some splats on the first power-on and can easily
identify the missing cases.
But even in i915 I have already seen some miss-usages of that.
And i915 is full of i915isms that we believe it is good but we
never tried to move to core kernel or when we tried we got some push
backs showing that that is not very common and desired way.
I know, just looking around Xe is also starting to get Xeisms,
but I'd like to avoid that as much as we can. in this case here
it is only 2 lines that could be a standard drm_warn or similar call.
I don't believe that it is worth adding a macro for this. So, maybe
if we really want this the right approach is to move the i915's one
to core kernel and then make Xe to start using it.
>
> Andi
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro
2023-09-22 19:03 ` Rodrigo Vivi
@ 2023-09-25 4:07 ` Nilawar, Badal
0 siblings, 0 replies; 24+ messages in thread
From: Nilawar, Badal @ 2023-09-25 4:07 UTC (permalink / raw)
To: Rodrigo Vivi, Andi Shyti
Cc: intel-xe, linux-hwmon, anshuman.gupta, ashutosh.dixit, linux,
riana.tauro, matthew.brost
On 23-09-2023 00:33, Rodrigo Vivi wrote:
> On Fri, Sep 22, 2023 at 05:16:23PM +0200, Andi Shyti wrote:
>> Hi Rodrigo,
>>
>> On Thu, Sep 21, 2023 at 12:03:26PM -0400, Rodrigo Vivi wrote:
>>> On Thu, Sep 21, 2023 at 03:55:14PM +0530, Badal Nilawar wrote:
>>>> Add XE_MISSING_CASE macro to handle missing switch case
>>>>
>>>> v2: Add comment about macro usage (Himal)
>>>>
>>>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>>>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>>>> Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>>> ---
>>>> drivers/gpu/drm/xe/xe_macros.h | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_macros.h b/drivers/gpu/drm/xe/xe_macros.h
>>>> index daf56c846d03..6c74c69920ed 100644
>>>> --- a/drivers/gpu/drm/xe/xe_macros.h
>>>> +++ b/drivers/gpu/drm/xe/xe_macros.h
>>>> @@ -15,4 +15,8 @@
>>>> "Ioctl argument check failed at %s:%d: %s", \
>>>> __FILE__, __LINE__, #cond), 1))
>>>>
>>>> +/* Parameter to macro should be a variable name */
>>>> +#define XE_MISSING_CASE(x) WARN(1, "Missing case (%s == %ld)\n", \
>>>> + __stringify(x), (long)(x))
>>>> +
>>>
>>> No, please! Let's not add unnecessary macros.
>>
>> it's not a bad idea, actually... in i915 we have the MISSING_CASE
>> for switch cases with a defined number of cases and print
>> warnings in case none if them is the one inside the switch.
>>
>> It's so widely used and actually nice to have that I thought many
>> times to move it to some core kernel libraries.
>
> to be really honest, I also liked the MISSING_CASE in many occasions.
> It is useful for platform enabling so once we add a new hardware we
> ensure to get some splats on the first power-on and can easily
> identify the missing cases.
>
> But even in i915 I have already seen some miss-usages of that.
> And i915 is full of i915isms that we believe it is good but we
> never tried to move to core kernel or when we tried we got some push
> backs showing that that is not very common and desired way.
>
> I know, just looking around Xe is also starting to get Xeisms,
> but I'd like to avoid that as much as we can. in this case here
> it is only 2 lines that could be a standard drm_warn or similar call.
>
> I don't believe that it is worth adding a macro for this. So, maybe
> if we really want this the right approach is to move the i915's one
> to core kernel and then make Xe to start using it.
Agreed, for this use case I will also prefer to go with drm_warn.
Regards,
Badal
>
>>
>> Andi
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 2/6] drm/xe/hwmon: Expose power attributes
2023-09-21 10:25 [PATCH v5 0/6] Add HWMON support for DGFX Badal Nilawar
2023-09-21 10:25 ` [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro Badal Nilawar
@ 2023-09-21 10:25 ` Badal Nilawar
2023-09-21 13:22 ` Riana Tauro
` (2 more replies)
2023-09-21 10:25 ` [PATCH v5 3/6] drm/xe/hwmon: Expose card reactive critical power Badal Nilawar
` (3 subsequent siblings)
5 siblings, 3 replies; 24+ messages in thread
From: Badal Nilawar @ 2023-09-21 10:25 UTC (permalink / raw)
To: intel-xe, linux-hwmon
Cc: anshuman.gupta, ashutosh.dixit, linux, andi.shyti, riana.tauro,
matthew.brost, rodrigo.vivi
Expose Card reactive sustained (pl1) power limit as power_max and
card default power limit (tdp) as power_rated_max.
v2:
- Fix review comments (Riana)
v3:
- Use drmm_mutex_init (Matt Brost)
- Print error value (Matt Brost)
- Convert enums to uppercase (Matt Brost)
- Avoid extra reg read in hwmon_is_visible function (Riana)
- Use xe_device_assert_mem_access when applicable (Matt Brost)
- Add intel-xe@lists.freedesktop.org in Documentation (Matt Brost)
v4:
- Use prefix xe_hwmon prefix for all functions (Matt Brost/Andi)
- %s/hwmon_reg/xe_hwmon_reg (Andi)
- Fix review comments (Guenter/Andi)
v5:
- Fix review comments (Riana)
Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
.../ABI/testing/sysfs-driver-intel-xe-hwmon | 22 ++
drivers/gpu/drm/xe/Makefile | 3 +
drivers/gpu/drm/xe/regs/xe_gt_regs.h | 4 +
drivers/gpu/drm/xe/regs/xe_mchbar_regs.h | 33 ++
drivers/gpu/drm/xe/xe_device.c | 3 +
drivers/gpu/drm/xe/xe_device_types.h | 2 +
drivers/gpu/drm/xe/xe_hwmon.c | 358 ++++++++++++++++++
drivers/gpu/drm/xe/xe_hwmon.h | 20 +
8 files changed, 445 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
create mode 100644 drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
create mode 100644 drivers/gpu/drm/xe/xe_hwmon.c
create mode 100644 drivers/gpu/drm/xe/xe_hwmon.h
diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
new file mode 100644
index 000000000000..da0197a29fe4
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
@@ -0,0 +1,22 @@
+What: /sys/devices/.../hwmon/hwmon<i>/power1_max
+Date: September 2023
+KernelVersion: 6.5
+Contact: intel-xe@lists.freedesktop.org
+Description: RW. Card reactive sustained (PL1) power limit in microwatts.
+
+ The power controller will throttle the operating frequency
+ if the power averaged over a window (typically seconds)
+ exceeds this limit. A read value of 0 means that the PL1
+ power limit is disabled, writing 0 disables the
+ limit. Writing values > 0 and <= TDP will enable the power limit.
+
+ Only supported for particular Intel xe graphics platforms.
+
+What: /sys/devices/.../hwmon/hwmon<i>/power1_rated_max
+Date: September 2023
+KernelVersion: 6.5
+Contact: intel-xe@lists.freedesktop.org
+Description: RO. Card default power limit (default TDP setting).
+
+ Only supported for particular Intel xe graphics platforms.
+
diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 86c8bd4c05a3..ca77aff60d48 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -117,6 +117,9 @@ xe-y += xe_bb.o \
xe_wa.o \
xe_wopcm.o
+# graphics hardware monitoring (HWMON) support
+xe-$(CONFIG_HWMON) += xe_hwmon.o
+
# i915 Display compat #defines and #includes
subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \
-I$(srctree)/$(src)/display/ext \
diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
index e13fbbdf6929..679cdba9f383 100644
--- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
@@ -410,4 +410,8 @@
#define XEHPC_BCS5_BCS6_INTR_MASK XE_REG(0x190118)
#define XEHPC_BCS7_BCS8_INTR_MASK XE_REG(0x19011c)
+#define PVC_GT0_PACKAGE_RAPL_LIMIT XE_REG(0x281008)
+#define PVC_GT0_PACKAGE_POWER_SKU_UNIT XE_REG(0x281068)
+#define PVC_GT0_PACKAGE_POWER_SKU XE_REG(0x281080)
+
#endif
diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
new file mode 100644
index 000000000000..27f1d42baf6d
--- /dev/null
+++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#ifndef _XE_MCHBAR_REGS_H_
+#define _XE_MCHBAR_REGS_H_
+
+#include "regs/xe_reg_defs.h"
+
+/*
+ * MCHBAR mirror.
+ *
+ * This mirrors the MCHBAR MMIO space whose location is determined by
+ * device 0 function 0's pci config register 0x44 or 0x48 and matches it in
+ * every way.
+ */
+
+#define MCHBAR_MIRROR_BASE_SNB 0x140000
+
+#define PCU_CR_PACKAGE_POWER_SKU XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5930)
+#define PKG_TDP GENMASK_ULL(14, 0)
+#define PKG_MIN_PWR GENMASK_ULL(30, 16)
+#define PKG_MAX_PWR GENMASK_ULL(46, 32)
+
+#define PCU_CR_PACKAGE_POWER_SKU_UNIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
+#define PKG_PWR_UNIT REG_GENMASK(3, 0)
+
+#define PCU_CR_PACKAGE_RAPL_LIMIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
+#define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
+#define PKG_PWR_LIM_1_EN REG_BIT(15)
+
+#endif /* _XE_MCHBAR_REGS_H_ */
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index b6bcb6c3482e..2acdc22a6027 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -34,6 +34,7 @@
#include "xe_vm.h"
#include "xe_vm_madvise.h"
#include "xe_wait_user_fence.h"
+#include "xe_hwmon.h"
#ifdef CONFIG_LOCKDEP
struct lockdep_map xe_device_mem_access_lockdep_map = {
@@ -337,6 +338,8 @@ int xe_device_probe(struct xe_device *xe)
xe_pmu_register(&xe->pmu);
+ xe_hwmon_register(xe);
+
err = drmm_add_action_or_reset(&xe->drm, xe_device_sanitize, xe);
if (err)
return err;
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index a82f28c6a3a0..d1e319f305ef 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -365,6 +365,8 @@ struct xe_device {
/** @pmu: performance monitoring unit */
struct xe_pmu pmu;
+ struct xe_hwmon *hwmon;
+
/* private: */
#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
new file mode 100644
index 000000000000..7f4599d98541
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -0,0 +1,358 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#include <linux/hwmon.h>
+
+#include <drm/drm_managed.h>
+#include "regs/xe_gt_regs.h"
+#include "regs/xe_mchbar_regs.h"
+#include "xe_device.h"
+#include "xe_gt.h"
+#include "xe_hwmon.h"
+#include "xe_mmio.h"
+
+enum xe_hwmon_reg {
+ REG_PKG_RAPL_LIMIT,
+ REG_PKG_POWER_SKU,
+ REG_PKG_POWER_SKU_UNIT,
+};
+
+enum xe_hwmon_reg_operation {
+ REG_READ,
+ REG_WRITE,
+ REG_RMW,
+};
+
+/*
+ * SF_* - scale factors for particular quantities according to hwmon spec.
+ */
+#define SF_POWER 1000000 /* microwatts */
+
+struct xe_hwmon {
+ struct device *hwmon_dev;
+ struct xe_gt *gt;
+ struct mutex hwmon_lock; /* rmw operations*/
+ int scl_shift_power;
+};
+
+static u32 xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg)
+{
+ 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_DG2)
+ reg = PCU_CR_PACKAGE_RAPL_LIMIT;
+ else if (xe->info.platform == XE_PVC)
+ reg = PVC_GT0_PACKAGE_RAPL_LIMIT;
+ break;
+ case REG_PKG_POWER_SKU:
+ if (xe->info.platform == XE_DG2)
+ reg = PCU_CR_PACKAGE_POWER_SKU;
+ else if (xe->info.platform == XE_PVC)
+ reg = PVC_GT0_PACKAGE_POWER_SKU;
+ break;
+ case REG_PKG_POWER_SKU_UNIT:
+ if (xe->info.platform == XE_DG2)
+ reg = PCU_CR_PACKAGE_POWER_SKU_UNIT;
+ else if (xe->info.platform == XE_PVC)
+ reg = PVC_GT0_PACKAGE_POWER_SKU_UNIT;
+ break;
+ default:
+ XE_MISSING_CASE(hwmon_reg);
+ break;
+ }
+
+ return reg.raw;
+}
+
+static int xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
+ enum xe_hwmon_reg_operation operation, u32 *value,
+ u32 clr, u32 set)
+{
+ struct xe_reg reg;
+
+ reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
+
+ if (!reg.raw)
+ return -EOPNOTSUPP;
+
+ switch (operation) {
+ case REG_READ:
+ *value = xe_mmio_read32(hwmon->gt, reg);
+ return 0;
+ case REG_WRITE:
+ xe_mmio_write32(hwmon->gt, reg, *value);
+ return 0;
+ case REG_RMW:
+ *value = xe_mmio_rmw32(hwmon->gt, reg, clr, set);
+ return 0;
+ default:
+ XE_MISSING_CASE(operation);
+ return -EOPNOTSUPP;
+ }
+}
+
+int xe_hwmon_process_reg_read64(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg, u64 *value)
+{
+ struct xe_reg reg;
+
+ reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
+
+ if (!reg.raw)
+ return -EOPNOTSUPP;
+
+ *value = xe_mmio_read64_2x32(hwmon->gt, reg);
+
+ return 0;
+}
+
+#define PL1_DISABLE 0
+
+/*
+ * HW allows arbitrary PL1 limits to be set but silently clamps these values to
+ * "typical but not guaranteed" min/max values in REG_PKG_POWER_SKU. Follow the
+ * same pattern for sysfs, allow arbitrary PL1 limits to be set but display
+ * clamped values when read.
+ */
+static int xe_hwmon_power_max_read(struct xe_hwmon *hwmon, long *value)
+{
+ u32 reg_val;
+ u64 reg_val64, min, max;
+
+ xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, ®_val, 0, 0);
+ /* Check if PL1 limit is disabled */
+ if (!(reg_val & PKG_PWR_LIM_1_EN)) {
+ *value = PL1_DISABLE;
+ return 0;
+ }
+
+ 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_read64(hwmon, REG_PKG_POWER_SKU, ®_val64);
+ min = REG_FIELD_GET(PKG_MIN_PWR, reg_val64);
+ min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
+ max = REG_FIELD_GET(PKG_MAX_PWR, reg_val64);
+ max = mul_u64_u32_shr(max, SF_POWER, hwmon->scl_shift_power);
+
+ if (min && max)
+ *value = clamp_t(u64, *value, min, max);
+
+ return 0;
+}
+
+static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
+{
+ u32 reg_val;
+
+ /* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
+ if (value == PL1_DISABLE) {
+ xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, ®_val,
+ PKG_PWR_LIM_1_EN, 0);
+ xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, ®_val,
+ PKG_PWR_LIM_1_EN, 0);
+
+ if (reg_val & PKG_PWR_LIM_1_EN)
+ return -ENODEV;
+ }
+
+ /* Computation in 64-bits to avoid overflow. Round to nearest. */
+ reg_val = DIV_ROUND_CLOSEST_ULL((u64)value << hwmon->scl_shift_power, SF_POWER);
+ reg_val = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, reg_val);
+
+ xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, ®_val,
+ PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val);
+
+ return 0;
+}
+
+static int xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, long *value)
+{
+ u32 reg_val;
+
+ xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU, REG_READ, ®_val, 0, 0);
+ reg_val = REG_FIELD_GET(PKG_TDP, reg_val);
+ *value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
+
+ return 0;
+}
+
+static const struct hwmon_channel_info *hwmon_info[] = {
+ HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX),
+ NULL
+};
+
+static umode_t
+xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int chan)
+{
+ switch (attr) {
+ case hwmon_power_max:
+ return xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT) ? 0664 : 0;
+ case hwmon_power_rated_max:
+ return xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU) ? 0444 : 0;
+ default:
+ return 0;
+ }
+}
+
+static int
+xe_hwmon_power_read(struct xe_hwmon *hwmon, u32 attr, int chan, long *val)
+{
+ switch (attr) {
+ case hwmon_power_max:
+ return xe_hwmon_power_max_read(hwmon, val);
+ case hwmon_power_rated_max:
+ return xe_hwmon_power_rated_max_read(hwmon, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int
+xe_hwmon_power_write(struct xe_hwmon *hwmon, u32 attr, int chan, long val)
+{
+ switch (attr) {
+ case hwmon_power_max:
+ return xe_hwmon_power_max_write(hwmon, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static umode_t
+xe_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ struct xe_hwmon *hwmon = (struct xe_hwmon *)drvdata;
+ int ret;
+
+ xe_device_mem_access_get(gt_to_xe(hwmon->gt));
+
+ switch (type) {
+ case hwmon_power:
+ ret = xe_hwmon_power_is_visible(hwmon, attr, channel);
+ break;
+ default:
+ ret = 0;
+ break;
+ }
+
+ xe_device_mem_access_put(gt_to_xe(hwmon->gt));
+
+ return ret;
+}
+
+static int
+xe_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+ int channel, long *val)
+{
+ struct xe_hwmon *hwmon = dev_get_drvdata(dev);
+ int ret;
+
+ xe_device_mem_access_get(gt_to_xe(hwmon->gt));
+
+ switch (type) {
+ case hwmon_power:
+ ret = xe_hwmon_power_read(hwmon, attr, channel, val);
+ break;
+ default:
+ ret = -EOPNOTSUPP;
+ break;
+ }
+
+ xe_device_mem_access_put(gt_to_xe(hwmon->gt));
+
+ return ret;
+}
+
+static int
+xe_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
+ int channel, long val)
+{
+ struct xe_hwmon *hwmon = dev_get_drvdata(dev);
+ int ret;
+
+ xe_device_mem_access_get(gt_to_xe(hwmon->gt));
+
+ switch (type) {
+ case hwmon_power:
+ ret = xe_hwmon_power_write(hwmon, attr, channel, val);
+ break;
+ default:
+ ret = -EOPNOTSUPP;
+ break;
+ }
+
+ xe_device_mem_access_put(gt_to_xe(hwmon->gt));
+
+ return ret;
+}
+
+static const struct hwmon_ops hwmon_ops = {
+ .is_visible = xe_hwmon_is_visible,
+ .read = xe_hwmon_read,
+ .write = xe_hwmon_write,
+};
+
+static const struct hwmon_chip_info hwmon_chip_info = {
+ .ops = &hwmon_ops,
+ .info = hwmon_info,
+};
+
+static void
+xe_hwmon_get_preregistration_info(struct xe_device *xe)
+{
+ struct xe_hwmon *hwmon = xe->hwmon;
+ u32 val_sku_unit = 0;
+ int ret;
+
+ ret = xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU_UNIT, REG_READ, &val_sku_unit, 0, 0);
+ /*
+ * The contents of register PKG_POWER_SKU_UNIT do not change,
+ * so read it once and store the shift values.
+ */
+ if (!ret)
+ hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
+}
+
+void xe_hwmon_register(struct xe_device *xe)
+{
+ struct device *dev = xe->drm.dev;
+ struct xe_hwmon *hwmon;
+
+ /* hwmon is available only for dGfx */
+ if (!IS_DGFX(xe))
+ return;
+
+ hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
+ if (!hwmon)
+ return;
+
+ xe->hwmon = hwmon;
+
+ drmm_mutex_init(&xe->drm, &hwmon->hwmon_lock);
+
+ /* primary GT to access device level properties */
+ hwmon->gt = xe->tiles[0].primary_gt;
+
+ xe_hwmon_get_preregistration_info(xe);
+
+ drm_dbg(&xe->drm, "Register xe hwmon interface\n");
+
+ /* hwmon_dev points to device hwmon<i> */
+ hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev,
+ "xe",
+ hwmon,
+ &hwmon_chip_info,
+ NULL);
+ if (IS_ERR(hwmon->hwmon_dev)) {
+ drm_warn(&xe->drm, "Failed to register xe hwmon (%pe)\n", hwmon->hwmon_dev);
+ xe->hwmon = NULL;
+ return;
+ }
+}
+
diff --git a/drivers/gpu/drm/xe/xe_hwmon.h b/drivers/gpu/drm/xe/xe_hwmon.h
new file mode 100644
index 000000000000..1ec45cf1d19b
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_hwmon.h
@@ -0,0 +1,20 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#ifndef _XE_HWMON_H_
+#define _XE_HWMON_H_
+
+#include <linux/types.h>
+
+struct xe_device;
+
+#if IS_REACHABLE(CONFIG_HWMON)
+void xe_hwmon_register(struct xe_device *xe);
+#else
+static inline void xe_hwmon_register(struct xe_device *xe) { };
+#endif
+
+#endif /* _XE_HWMON_H_ */
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v5 2/6] drm/xe/hwmon: Expose power attributes
2023-09-21 10:25 ` [PATCH v5 2/6] drm/xe/hwmon: Expose power attributes Badal Nilawar
@ 2023-09-21 13:22 ` Riana Tauro
2023-09-21 16:25 ` [Intel-xe] " Rodrigo Vivi
2023-09-22 17:24 ` Andi Shyti
2 siblings, 0 replies; 24+ messages in thread
From: Riana Tauro @ 2023-09-21 13:22 UTC (permalink / raw)
To: Badal Nilawar, intel-xe, linux-hwmon
Cc: anshuman.gupta, ashutosh.dixit, linux, andi.shyti, matthew.brost,
rodrigo.vivi
On 9/21/2023 3:55 PM, Badal Nilawar wrote:
> Expose Card reactive sustained (pl1) power limit as power_max and
> card default power limit (tdp) as power_rated_max.
>
> v2:
> - Fix review comments (Riana)
> v3:
> - Use drmm_mutex_init (Matt Brost)
> - Print error value (Matt Brost)
> - Convert enums to uppercase (Matt Brost)
> - Avoid extra reg read in hwmon_is_visible function (Riana)
> - Use xe_device_assert_mem_access when applicable (Matt Brost)
> - Add intel-xe@lists.freedesktop.org in Documentation (Matt Brost)
> v4:
> - Use prefix xe_hwmon prefix for all functions (Matt Brost/Andi)
> - %s/hwmon_reg/xe_hwmon_reg (Andi)
> - Fix review comments (Guenter/Andi)
> v5:
> - Fix review comments (Riana)
>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
> .../ABI/testing/sysfs-driver-intel-xe-hwmon | 22 ++
> drivers/gpu/drm/xe/Makefile | 3 +
> drivers/gpu/drm/xe/regs/xe_gt_regs.h | 4 +
> drivers/gpu/drm/xe/regs/xe_mchbar_regs.h | 33 ++
> drivers/gpu/drm/xe/xe_device.c | 3 +
> drivers/gpu/drm/xe/xe_device_types.h | 2 +
> drivers/gpu/drm/xe/xe_hwmon.c | 358 ++++++++++++++++++
> drivers/gpu/drm/xe/xe_hwmon.h | 20 +
> 8 files changed, 445 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> create mode 100644 drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> create mode 100644 drivers/gpu/drm/xe/xe_hwmon.c
> create mode 100644 drivers/gpu/drm/xe/xe_hwmon.h
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> new file mode 100644
> index 000000000000..da0197a29fe4
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> @@ -0,0 +1,22 @@
> +What: /sys/devices/.../hwmon/hwmon<i>/power1_max
> +Date: September 2023
> +KernelVersion: 6.5
> +Contact: intel-xe@lists.freedesktop.org
> +Description: RW. Card reactive sustained (PL1) power limit in microwatts.
> +
> + The power controller will throttle the operating frequency
> + if the power averaged over a window (typically seconds)
> + exceeds this limit. A read value of 0 means that the PL1
> + power limit is disabled, writing 0 disables the
> + limit. Writing values > 0 and <= TDP will enable the power limit.
> +
> + Only supported for particular Intel xe graphics platforms.
> +
> +What: /sys/devices/.../hwmon/hwmon<i>/power1_rated_max
> +Date: September 2023
> +KernelVersion: 6.5
> +Contact: intel-xe@lists.freedesktop.org
> +Description: RO. Card default power limit (default TDP setting).
> +
> + Only supported for particular Intel xe graphics platforms.
> +
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 86c8bd4c05a3..ca77aff60d48 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -117,6 +117,9 @@ xe-y += xe_bb.o \
> xe_wa.o \
> xe_wopcm.o
>
> +# graphics hardware monitoring (HWMON) support
> +xe-$(CONFIG_HWMON) += xe_hwmon.o
> +
> # i915 Display compat #defines and #includes
> subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \
> -I$(srctree)/$(src)/display/ext \
> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> index e13fbbdf6929..679cdba9f383 100644
> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> @@ -410,4 +410,8 @@
> #define XEHPC_BCS5_BCS6_INTR_MASK XE_REG(0x190118)
> #define XEHPC_BCS7_BCS8_INTR_MASK XE_REG(0x19011c)
>
> +#define PVC_GT0_PACKAGE_RAPL_LIMIT XE_REG(0x281008)
> +#define PVC_GT0_PACKAGE_POWER_SKU_UNIT XE_REG(0x281068)
> +#define PVC_GT0_PACKAGE_POWER_SKU XE_REG(0x281080)
> +
> #endif
> diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> new file mode 100644
> index 000000000000..27f1d42baf6d
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef _XE_MCHBAR_REGS_H_
> +#define _XE_MCHBAR_REGS_H_
> +
> +#include "regs/xe_reg_defs.h"
> +
> +/*
> + * MCHBAR mirror.
> + *
> + * This mirrors the MCHBAR MMIO space whose location is determined by
> + * device 0 function 0's pci config register 0x44 or 0x48 and matches it in
> + * every way.
> + */
> +
> +#define MCHBAR_MIRROR_BASE_SNB 0x140000
> +
> +#define PCU_CR_PACKAGE_POWER_SKU XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5930)
> +#define PKG_TDP GENMASK_ULL(14, 0)
> +#define PKG_MIN_PWR GENMASK_ULL(30, 16)
> +#define PKG_MAX_PWR GENMASK_ULL(46, 32)
> +
> +#define PCU_CR_PACKAGE_POWER_SKU_UNIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
> +#define PKG_PWR_UNIT REG_GENMASK(3, 0)
> +
> +#define PCU_CR_PACKAGE_RAPL_LIMIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
> +#define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
> +#define PKG_PWR_LIM_1_EN REG_BIT(15)
> +
> +#endif /* _XE_MCHBAR_REGS_H_ */
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index b6bcb6c3482e..2acdc22a6027 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -34,6 +34,7 @@
> #include "xe_vm.h"
> #include "xe_vm_madvise.h"
> #include "xe_wait_user_fence.h"
> +#include "xe_hwmon.h"
Should be alphabetical
>
> #ifdef CONFIG_LOCKDEP
> struct lockdep_map xe_device_mem_access_lockdep_map = {
> @@ -337,6 +338,8 @@ int xe_device_probe(struct xe_device *xe)
>
> xe_pmu_register(&xe->pmu);
>
> + xe_hwmon_register(xe);
> +
> err = drmm_add_action_or_reset(&xe->drm, xe_device_sanitize, xe);
> if (err)
> return err;
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index a82f28c6a3a0..d1e319f305ef 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -365,6 +365,8 @@ struct xe_device {
> /** @pmu: performance monitoring unit */
> struct xe_pmu pmu;
>
> + struct xe_hwmon *hwmon;
> +
> /* private: */
>
> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> new file mode 100644
> index 000000000000..7f4599d98541
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -0,0 +1,358 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include <linux/hwmon.h>
> +
> +#include <drm/drm_managed.h>
> +#include "regs/xe_gt_regs.h"
> +#include "regs/xe_mchbar_regs.h"
> +#include "xe_device.h"
> +#include "xe_gt.h"
> +#include "xe_hwmon.h"
> +#include "xe_mmio.h"
> +
> +enum xe_hwmon_reg {
> + REG_PKG_RAPL_LIMIT,
> + REG_PKG_POWER_SKU,
> + REG_PKG_POWER_SKU_UNIT,
> +};
> +
> +enum xe_hwmon_reg_operation {
> + REG_READ,
> + REG_WRITE,
> + REG_RMW,
> +};
> +
> +/*
> + * SF_* - scale factors for particular quantities according to hwmon spec.
> + */
> +#define SF_POWER 1000000 /* microwatts */
> +
> +struct xe_hwmon {
> + struct device *hwmon_dev;
> + struct xe_gt *gt;
> + struct mutex hwmon_lock; /* rmw operations*/
> + int scl_shift_power;
> +};
> +
> +static u32 xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg)
> +{
> + 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_DG2)
> + reg = PCU_CR_PACKAGE_RAPL_LIMIT;
> + else if (xe->info.platform == XE_PVC)
> + reg = PVC_GT0_PACKAGE_RAPL_LIMIT;
> + break;
> + case REG_PKG_POWER_SKU:
> + if (xe->info.platform == XE_DG2)
> + reg = PCU_CR_PACKAGE_POWER_SKU;
> + else if (xe->info.platform == XE_PVC)
> + reg = PVC_GT0_PACKAGE_POWER_SKU;
> + break;
> + case REG_PKG_POWER_SKU_UNIT:
> + if (xe->info.platform == XE_DG2)
> + reg = PCU_CR_PACKAGE_POWER_SKU_UNIT;
> + else if (xe->info.platform == XE_PVC)
> + reg = PVC_GT0_PACKAGE_POWER_SKU_UNIT;
> + break;
> + default:
> + XE_MISSING_CASE(hwmon_reg);
> + break;
> + }
> +
> + return reg.raw;
> +}
> +
> +static int xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
> + enum xe_hwmon_reg_operation operation, u32 *value,
> + u32 clr, u32 set)
> +{
> + struct xe_reg reg;
> +
> + reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
> +
> + if (!reg.raw)
> + return -EOPNOTSUPP;
> +
> + switch (operation) {
> + case REG_READ:
> + *value = xe_mmio_read32(hwmon->gt, reg);
> + return 0;
> + case REG_WRITE:
> + xe_mmio_write32(hwmon->gt, reg, *value);
> + return 0;
> + case REG_RMW:
> + *value = xe_mmio_rmw32(hwmon->gt, reg, clr, set);
> + return 0;
> + default:
> + XE_MISSING_CASE(operation);
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +int xe_hwmon_process_reg_read64(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg, u64 *value)
> +{
> + struct xe_reg reg;
> +
> + reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
> +
> + if (!reg.raw)
> + return -EOPNOTSUPP;
> +
> + *value = xe_mmio_read64_2x32(hwmon->gt, reg);
> +
> + return 0;
> +}
> +
> +#define PL1_DISABLE 0
> +
> +/*
> + * HW allows arbitrary PL1 limits to be set but silently clamps these values to
> + * "typical but not guaranteed" min/max values in REG_PKG_POWER_SKU. Follow the
> + * same pattern for sysfs, allow arbitrary PL1 limits to be set but display
> + * clamped values when read.
> + */
> +static int xe_hwmon_power_max_read(struct xe_hwmon *hwmon, long *value)
> +{
> + u32 reg_val;
> + u64 reg_val64, min, max;
> +
> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, ®_val, 0, 0);
> + /* Check if PL1 limit is disabled */
> + if (!(reg_val & PKG_PWR_LIM_1_EN)) {
> + *value = PL1_DISABLE;
> + return 0;
> + }
> +
> + 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_read64(hwmon, REG_PKG_POWER_SKU, ®_val64);
> + min = REG_FIELD_GET(PKG_MIN_PWR, reg_val64);
> + min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
> + max = REG_FIELD_GET(PKG_MAX_PWR, reg_val64);
> + max = mul_u64_u32_shr(max, SF_POWER, hwmon->scl_shift_power);
> +
> + if (min && max)
> + *value = clamp_t(u64, *value, min, max);
> +
> + return 0;
> +}
> +
> +static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
> +{
> + u32 reg_val;
> +
> + /* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
> + if (value == PL1_DISABLE) {
> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, ®_val,
> + PKG_PWR_LIM_1_EN, 0);
> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, ®_val,
> + PKG_PWR_LIM_1_EN, 0);
> +
> + if (reg_val & PKG_PWR_LIM_1_EN)
> + return -ENODEV;
> + }
> +
> + /* Computation in 64-bits to avoid overflow. Round to nearest. */
> + reg_val = DIV_ROUND_CLOSEST_ULL((u64)value << hwmon->scl_shift_power, SF_POWER);
> + reg_val = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, reg_val);
> +
> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, ®_val,
> + PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val);
> +
> + return 0;
> +}
> +
> +static int xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, long *value)
> +{
> + u32 reg_val;
> +
> + xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU, REG_READ, ®_val, 0, 0);
> + reg_val = REG_FIELD_GET(PKG_TDP, reg_val);
> + *value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
> +
> + return 0;
> +}
> +
> +static const struct hwmon_channel_info *hwmon_info[] = {
> + HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX),
> + NULL
> +};
> +
> +static umode_t
> +xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int chan)
> +{
> + switch (attr) {
> + case hwmon_power_max:
> + return xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT) ? 0664 : 0;
> + case hwmon_power_rated_max:
> + return xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU) ? 0444 : 0;
> + default:
> + return 0;
> + }
> +}
> +
> +static int
> +xe_hwmon_power_read(struct xe_hwmon *hwmon, u32 attr, int chan, long *val)
> +{
> + switch (attr) {
> + case hwmon_power_max:
> + return xe_hwmon_power_max_read(hwmon, val);
> + case hwmon_power_rated_max:
> + return xe_hwmon_power_rated_max_read(hwmon, val);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int
> +xe_hwmon_power_write(struct xe_hwmon *hwmon, u32 attr, int chan, long val)
> +{
> + switch (attr) {
> + case hwmon_power_max:
> + return xe_hwmon_power_max_write(hwmon, val);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static umode_t
> +xe_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + struct xe_hwmon *hwmon = (struct xe_hwmon *)drvdata;
> + int ret;
> +
> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> +
> + switch (type) {
> + case hwmon_power:
> + ret = xe_hwmon_power_is_visible(hwmon, attr, channel);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> +
> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> +
> + return ret;
> +}
> +
> +static int
> +xe_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, long *val)
> +{
> + struct xe_hwmon *hwmon = dev_get_drvdata(dev);
> + int ret;
> +
> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> +
> + switch (type) {
> + case hwmon_power:
> + ret = xe_hwmon_power_read(hwmon, attr, channel, val);
> + break;
> + default:
> + ret = -EOPNOTSUPP;
> + break;
> + }
> +
> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> +
> + return ret;
> +}
> +
> +static int
> +xe_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, long val)
> +{
> + struct xe_hwmon *hwmon = dev_get_drvdata(dev);
> + int ret;
> +
> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> +
> + switch (type) {
> + case hwmon_power:
> + ret = xe_hwmon_power_write(hwmon, attr, channel, val);
> + break;
> + default:
> + ret = -EOPNOTSUPP;
> + break;
> + }
> +
> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> +
> + return ret;
> +}
> +
> +static const struct hwmon_ops hwmon_ops = {
> + .is_visible = xe_hwmon_is_visible,
> + .read = xe_hwmon_read,
> + .write = xe_hwmon_write,
> +};
> +
> +static const struct hwmon_chip_info hwmon_chip_info = {
> + .ops = &hwmon_ops,
> + .info = hwmon_info,
> +};
> +
> +static void
> +xe_hwmon_get_preregistration_info(struct xe_device *xe)
> +{
> + struct xe_hwmon *hwmon = xe->hwmon;
> + u32 val_sku_unit = 0;
> + int ret;
> +
> + ret = xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU_UNIT, REG_READ, &val_sku_unit, 0, 0);
> + /*
> + * The contents of register PKG_POWER_SKU_UNIT do not change,
> + * so read it once and store the shift values.
> + */
> + if (!ret)
> + hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
> +}
> +
> +void xe_hwmon_register(struct xe_device *xe)
> +{
> + struct device *dev = xe->drm.dev;
> + struct xe_hwmon *hwmon;
> +
> + /* hwmon is available only for dGfx */
> + if (!IS_DGFX(xe))
> + return;
> +
> + hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
> + if (!hwmon)
> + return;
> +
> + xe->hwmon = hwmon;
> +
> + drmm_mutex_init(&xe->drm, &hwmon->hwmon_lock);
> +
> + /* primary GT to access device level properties */
> + hwmon->gt = xe->tiles[0].primary_gt;
> +
> + xe_hwmon_get_preregistration_info(xe);
> +
> + drm_dbg(&xe->drm, "Register xe hwmon interface\n");
> +
> + /* hwmon_dev points to device hwmon<i> */
> + hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev,
> + "xe",
> + hwmon,
> + &hwmon_chip_info,
> + NULL);
> + if (IS_ERR(hwmon->hwmon_dev)) {
> + drm_warn(&xe->drm, "Failed to register xe hwmon (%pe)\n", hwmon->hwmon_dev);
> + xe->hwmon = NULL;
> + return;
> + }
> +}
> +
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.h b/drivers/gpu/drm/xe/xe_hwmon.h
> new file mode 100644
> index 000000000000..1ec45cf1d19b
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_hwmon.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: MIT */
> +
Remove blank line
With the above comments
Reviewed-by: Riana Tauro <riana.tauro@intel.com>
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef _XE_HWMON_H_
> +#define _XE_HWMON_H_
> +
> +#include <linux/types.h>
> +
> +struct xe_device;
> +
> +#if IS_REACHABLE(CONFIG_HWMON)
> +void xe_hwmon_register(struct xe_device *xe);
> +#else
> +static inline void xe_hwmon_register(struct xe_device *xe) { };
> +#endif
> +
> +#endif /* _XE_HWMON_H_ */
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Intel-xe] [PATCH v5 2/6] drm/xe/hwmon: Expose power attributes
2023-09-21 10:25 ` [PATCH v5 2/6] drm/xe/hwmon: Expose power attributes Badal Nilawar
2023-09-21 13:22 ` Riana Tauro
@ 2023-09-21 16:25 ` Rodrigo Vivi
2023-09-22 9:57 ` Nilawar, Badal
2023-09-22 17:24 ` Andi Shyti
2 siblings, 1 reply; 24+ messages in thread
From: Rodrigo Vivi @ 2023-09-21 16:25 UTC (permalink / raw)
To: Badal Nilawar; +Cc: intel-xe, linux-hwmon, linux
On Thu, Sep 21, 2023 at 03:55:15PM +0530, Badal Nilawar wrote:
> Expose Card reactive sustained (pl1) power limit as power_max and
> card default power limit (tdp) as power_rated_max.
>
> v2:
> - Fix review comments (Riana)
> v3:
> - Use drmm_mutex_init (Matt Brost)
> - Print error value (Matt Brost)
> - Convert enums to uppercase (Matt Brost)
> - Avoid extra reg read in hwmon_is_visible function (Riana)
> - Use xe_device_assert_mem_access when applicable (Matt Brost)
> - Add intel-xe@lists.freedesktop.org in Documentation (Matt Brost)
> v4:
> - Use prefix xe_hwmon prefix for all functions (Matt Brost/Andi)
> - %s/hwmon_reg/xe_hwmon_reg (Andi)
> - Fix review comments (Guenter/Andi)
> v5:
> - Fix review comments (Riana)
>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
> .../ABI/testing/sysfs-driver-intel-xe-hwmon | 22 ++
> drivers/gpu/drm/xe/Makefile | 3 +
> drivers/gpu/drm/xe/regs/xe_gt_regs.h | 4 +
> drivers/gpu/drm/xe/regs/xe_mchbar_regs.h | 33 ++
> drivers/gpu/drm/xe/xe_device.c | 3 +
> drivers/gpu/drm/xe/xe_device_types.h | 2 +
> drivers/gpu/drm/xe/xe_hwmon.c | 358 ++++++++++++++++++
> drivers/gpu/drm/xe/xe_hwmon.h | 20 +
> 8 files changed, 445 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> create mode 100644 drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> create mode 100644 drivers/gpu/drm/xe/xe_hwmon.c
> create mode 100644 drivers/gpu/drm/xe/xe_hwmon.h
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> new file mode 100644
> index 000000000000..da0197a29fe4
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> @@ -0,0 +1,22 @@
> +What: /sys/devices/.../hwmon/hwmon<i>/power1_max
> +Date: September 2023
> +KernelVersion: 6.5
> +Contact: intel-xe@lists.freedesktop.org
> +Description: RW. Card reactive sustained (PL1) power limit in microwatts.
> +
> + The power controller will throttle the operating frequency
> + if the power averaged over a window (typically seconds)
> + exceeds this limit. A read value of 0 means that the PL1
> + power limit is disabled, writing 0 disables the
> + limit. Writing values > 0 and <= TDP will enable the power limit.
> +
> + Only supported for particular Intel xe graphics platforms.
> +
> +What: /sys/devices/.../hwmon/hwmon<i>/power1_rated_max
> +Date: September 2023
> +KernelVersion: 6.5
> +Contact: intel-xe@lists.freedesktop.org
> +Description: RO. Card default power limit (default TDP setting).
> +
> + Only supported for particular Intel xe graphics platforms.
> +
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 86c8bd4c05a3..ca77aff60d48 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -117,6 +117,9 @@ xe-y += xe_bb.o \
> xe_wa.o \
> xe_wopcm.o
>
> +# graphics hardware monitoring (HWMON) support
> +xe-$(CONFIG_HWMON) += xe_hwmon.o
> +
> # i915 Display compat #defines and #includes
> subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \
> -I$(srctree)/$(src)/display/ext \
> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> index e13fbbdf6929..679cdba9f383 100644
> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> @@ -410,4 +410,8 @@
> #define XEHPC_BCS5_BCS6_INTR_MASK XE_REG(0x190118)
> #define XEHPC_BCS7_BCS8_INTR_MASK XE_REG(0x19011c)
>
> +#define PVC_GT0_PACKAGE_RAPL_LIMIT XE_REG(0x281008)
> +#define PVC_GT0_PACKAGE_POWER_SKU_UNIT XE_REG(0x281068)
> +#define PVC_GT0_PACKAGE_POWER_SKU XE_REG(0x281080)
> +
> #endif
> diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> new file mode 100644
> index 000000000000..27f1d42baf6d
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef _XE_MCHBAR_REGS_H_
> +#define _XE_MCHBAR_REGS_H_
> +
> +#include "regs/xe_reg_defs.h"
> +
> +/*
> + * MCHBAR mirror.
> + *
> + * This mirrors the MCHBAR MMIO space whose location is determined by
> + * device 0 function 0's pci config register 0x44 or 0x48 and matches it in
> + * every way.
> + */
> +
> +#define MCHBAR_MIRROR_BASE_SNB 0x140000
> +
> +#define PCU_CR_PACKAGE_POWER_SKU XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5930)
> +#define PKG_TDP GENMASK_ULL(14, 0)
> +#define PKG_MIN_PWR GENMASK_ULL(30, 16)
> +#define PKG_MAX_PWR GENMASK_ULL(46, 32)
> +
> +#define PCU_CR_PACKAGE_POWER_SKU_UNIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
> +#define PKG_PWR_UNIT REG_GENMASK(3, 0)
> +
> +#define PCU_CR_PACKAGE_RAPL_LIMIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
> +#define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
> +#define PKG_PWR_LIM_1_EN REG_BIT(15)
> +
> +#endif /* _XE_MCHBAR_REGS_H_ */
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index b6bcb6c3482e..2acdc22a6027 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -34,6 +34,7 @@
> #include "xe_vm.h"
> #include "xe_vm_madvise.h"
> #include "xe_wait_user_fence.h"
> +#include "xe_hwmon.h"
>
> #ifdef CONFIG_LOCKDEP
> struct lockdep_map xe_device_mem_access_lockdep_map = {
> @@ -337,6 +338,8 @@ int xe_device_probe(struct xe_device *xe)
>
> xe_pmu_register(&xe->pmu);
>
> + xe_hwmon_register(xe);
> +
> err = drmm_add_action_or_reset(&xe->drm, xe_device_sanitize, xe);
> if (err)
> return err;
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index a82f28c6a3a0..d1e319f305ef 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -365,6 +365,8 @@ struct xe_device {
> /** @pmu: performance monitoring unit */
> struct xe_pmu pmu;
>
> + struct xe_hwmon *hwmon;
> +
> /* private: */
>
> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> new file mode 100644
> index 000000000000..7f4599d98541
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -0,0 +1,358 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#include <linux/hwmon.h>
> +
> +#include <drm/drm_managed.h>
> +#include "regs/xe_gt_regs.h"
> +#include "regs/xe_mchbar_regs.h"
> +#include "xe_device.h"
> +#include "xe_gt.h"
> +#include "xe_hwmon.h"
> +#include "xe_mmio.h"
> +
> +enum xe_hwmon_reg {
> + REG_PKG_RAPL_LIMIT,
> + REG_PKG_POWER_SKU,
> + REG_PKG_POWER_SKU_UNIT,
> +};
> +
> +enum xe_hwmon_reg_operation {
> + REG_READ,
> + REG_WRITE,
> + REG_RMW,
> +};
> +
> +/*
> + * SF_* - scale factors for particular quantities according to hwmon spec.
> + */
> +#define SF_POWER 1000000 /* microwatts */
> +
> +struct xe_hwmon {
> + struct device *hwmon_dev;
> + struct xe_gt *gt;
> + struct mutex hwmon_lock; /* rmw operations*/
> + int scl_shift_power;
> +};
> +
> +static u32 xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg)
> +{
> + 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_DG2)
> + reg = PCU_CR_PACKAGE_RAPL_LIMIT;
> + else if (xe->info.platform == XE_PVC)
> + reg = PVC_GT0_PACKAGE_RAPL_LIMIT;
> + break;
> + case REG_PKG_POWER_SKU:
> + if (xe->info.platform == XE_DG2)
> + reg = PCU_CR_PACKAGE_POWER_SKU;
> + else if (xe->info.platform == XE_PVC)
> + reg = PVC_GT0_PACKAGE_POWER_SKU;
> + break;
> + case REG_PKG_POWER_SKU_UNIT:
> + if (xe->info.platform == XE_DG2)
> + reg = PCU_CR_PACKAGE_POWER_SKU_UNIT;
> + else if (xe->info.platform == XE_PVC)
> + reg = PVC_GT0_PACKAGE_POWER_SKU_UNIT;
> + break;
> + default:
> + XE_MISSING_CASE(hwmon_reg);
> + break;
> + }
> +
> + return reg.raw;
> +}
> +
> +static int xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
> + enum xe_hwmon_reg_operation operation, u32 *value,
> + u32 clr, u32 set)
> +{
> + struct xe_reg reg;
> +
> + reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
> +
> + if (!reg.raw)
> + return -EOPNOTSUPP;
> +
> + switch (operation) {
> + case REG_READ:
> + *value = xe_mmio_read32(hwmon->gt, reg);
> + return 0;
> + case REG_WRITE:
> + xe_mmio_write32(hwmon->gt, reg, *value);
> + return 0;
> + case REG_RMW:
> + *value = xe_mmio_rmw32(hwmon->gt, reg, clr, set);
> + return 0;
> + default:
> + XE_MISSING_CASE(operation);
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +int xe_hwmon_process_reg_read64(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg, u64 *value)
> +{
> + struct xe_reg reg;
> +
> + reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
> +
> + if (!reg.raw)
> + return -EOPNOTSUPP;
> +
> + *value = xe_mmio_read64_2x32(hwmon->gt, reg);
> +
> + return 0;
> +}
> +
> +#define PL1_DISABLE 0
> +
> +/*
> + * HW allows arbitrary PL1 limits to be set but silently clamps these values to
> + * "typical but not guaranteed" min/max values in REG_PKG_POWER_SKU. Follow the
> + * same pattern for sysfs, allow arbitrary PL1 limits to be set but display
> + * clamped values when read.
> + */
> +static int xe_hwmon_power_max_read(struct xe_hwmon *hwmon, long *value)
> +{
> + u32 reg_val;
> + u64 reg_val64, min, max;
> +
> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, ®_val, 0, 0);
> + /* Check if PL1 limit is disabled */
> + if (!(reg_val & PKG_PWR_LIM_1_EN)) {
> + *value = PL1_DISABLE;
> + return 0;
> + }
> +
> + 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_read64(hwmon, REG_PKG_POWER_SKU, ®_val64);
> + min = REG_FIELD_GET(PKG_MIN_PWR, reg_val64);
> + min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
> + max = REG_FIELD_GET(PKG_MAX_PWR, reg_val64);
> + max = mul_u64_u32_shr(max, SF_POWER, hwmon->scl_shift_power);
> +
> + if (min && max)
> + *value = clamp_t(u64, *value, min, max);
> +
> + return 0;
> +}
> +
> +static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
> +{
> + u32 reg_val;
> +
> + /* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
> + if (value == PL1_DISABLE) {
> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, ®_val,
> + PKG_PWR_LIM_1_EN, 0);
> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, ®_val,
> + PKG_PWR_LIM_1_EN, 0);
> +
> + if (reg_val & PKG_PWR_LIM_1_EN)
> + return -ENODEV;
> + }
> +
> + /* Computation in 64-bits to avoid overflow. Round to nearest. */
> + reg_val = DIV_ROUND_CLOSEST_ULL((u64)value << hwmon->scl_shift_power, SF_POWER);
> + reg_val = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, reg_val);
> +
> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, ®_val,
> + PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val);
> +
> + return 0;
> +}
> +
> +static int xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, long *value)
> +{
> + u32 reg_val;
> +
> + xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU, REG_READ, ®_val, 0, 0);
> + reg_val = REG_FIELD_GET(PKG_TDP, reg_val);
> + *value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
> +
> + return 0;
> +}
> +
> +static const struct hwmon_channel_info *hwmon_info[] = {
> + HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX),
> + NULL
> +};
> +
> +static umode_t
> +xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int chan)
> +{
> + switch (attr) {
> + case hwmon_power_max:
> + return xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT) ? 0664 : 0;
> + case hwmon_power_rated_max:
> + return xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU) ? 0444 : 0;
> + default:
> + return 0;
> + }
> +}
> +
> +static int
> +xe_hwmon_power_read(struct xe_hwmon *hwmon, u32 attr, int chan, long *val)
> +{
> + switch (attr) {
> + case hwmon_power_max:
> + return xe_hwmon_power_max_read(hwmon, val);
> + case hwmon_power_rated_max:
> + return xe_hwmon_power_rated_max_read(hwmon, val);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int
> +xe_hwmon_power_write(struct xe_hwmon *hwmon, u32 attr, int chan, long val)
> +{
> + switch (attr) {
> + case hwmon_power_max:
> + return xe_hwmon_power_max_write(hwmon, val);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static umode_t
> +xe_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + struct xe_hwmon *hwmon = (struct xe_hwmon *)drvdata;
> + int ret;
> +
> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> +
> + switch (type) {
> + case hwmon_power:
> + ret = xe_hwmon_power_is_visible(hwmon, attr, channel);
> + break;
> + default:
> + ret = 0;
> + break;
> + }
> +
> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> +
> + return ret;
> +}
> +
> +static int
> +xe_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, long *val)
> +{
> + struct xe_hwmon *hwmon = dev_get_drvdata(dev);
> + int ret;
> +
> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> +
> + switch (type) {
> + case hwmon_power:
> + ret = xe_hwmon_power_read(hwmon, attr, channel, val);
> + break;
> + default:
> + ret = -EOPNOTSUPP;
> + break;
> + }
> +
> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> +
> + return ret;
> +}
> +
> +static int
> +xe_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, long val)
> +{
> + struct xe_hwmon *hwmon = dev_get_drvdata(dev);
> + int ret;
> +
> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> +
> + switch (type) {
> + case hwmon_power:
> + ret = xe_hwmon_power_write(hwmon, attr, channel, val);
> + break;
> + default:
> + ret = -EOPNOTSUPP;
> + break;
> + }
> +
> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> +
> + return ret;
> +}
> +
> +static const struct hwmon_ops hwmon_ops = {
> + .is_visible = xe_hwmon_is_visible,
> + .read = xe_hwmon_read,
> + .write = xe_hwmon_write,
> +};
> +
> +static const struct hwmon_chip_info hwmon_chip_info = {
> + .ops = &hwmon_ops,
> + .info = hwmon_info,
> +};
> +
> +static void
> +xe_hwmon_get_preregistration_info(struct xe_device *xe)
> +{
> + struct xe_hwmon *hwmon = xe->hwmon;
> + u32 val_sku_unit = 0;
> + int ret;
> +
> + ret = xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU_UNIT, REG_READ, &val_sku_unit, 0, 0);
> + /*
> + * The contents of register PKG_POWER_SKU_UNIT do not change,
> + * so read it once and store the shift values.
> + */
> + if (!ret)
> + hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
> +}
> +
> +void xe_hwmon_register(struct xe_device *xe)
> +{
> + struct device *dev = xe->drm.dev;
> + struct xe_hwmon *hwmon;
> +
> + /* hwmon is available only for dGfx */
> + if (!IS_DGFX(xe))
> + return;
> +
> + hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
> + if (!hwmon)
> + return;
> +
> + xe->hwmon = hwmon;
> +
> + drmm_mutex_init(&xe->drm, &hwmon->hwmon_lock);
> +
> + /* primary GT to access device level properties */
> + hwmon->gt = xe->tiles[0].primary_gt;
So, what happens with the other tiles?
We should get them in since the beginning since that will impose
interface changes and bigger changes on IGT tests.
> +
> + xe_hwmon_get_preregistration_info(xe);
> +
> + drm_dbg(&xe->drm, "Register xe hwmon interface\n");
> +
> + /* hwmon_dev points to device hwmon<i> */
> + hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev,
> + "xe",
> + hwmon,
> + &hwmon_chip_info,
> + NULL);
> + if (IS_ERR(hwmon->hwmon_dev)) {
> + drm_warn(&xe->drm, "Failed to register xe hwmon (%pe)\n", hwmon->hwmon_dev);
> + xe->hwmon = NULL;
> + return;
> + }
> +}
> +
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.h b/drivers/gpu/drm/xe/xe_hwmon.h
> new file mode 100644
> index 000000000000..1ec45cf1d19b
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_hwmon.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * Copyright © 2023 Intel Corporation
> + */
> +
> +#ifndef _XE_HWMON_H_
> +#define _XE_HWMON_H_
> +
> +#include <linux/types.h>
> +
> +struct xe_device;
> +
> +#if IS_REACHABLE(CONFIG_HWMON)
> +void xe_hwmon_register(struct xe_device *xe);
> +#else
> +static inline void xe_hwmon_register(struct xe_device *xe) { };
> +#endif
> +
> +#endif /* _XE_HWMON_H_ */
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Intel-xe] [PATCH v5 2/6] drm/xe/hwmon: Expose power attributes
2023-09-21 16:25 ` [Intel-xe] " Rodrigo Vivi
@ 2023-09-22 9:57 ` Nilawar, Badal
0 siblings, 0 replies; 24+ messages in thread
From: Nilawar, Badal @ 2023-09-22 9:57 UTC (permalink / raw)
To: Rodrigo Vivi; +Cc: intel-xe, linux-hwmon, linux
On 21-09-2023 21:55, Rodrigo Vivi wrote:
> On Thu, Sep 21, 2023 at 03:55:15PM +0530, Badal Nilawar wrote:
>> Expose Card reactive sustained (pl1) power limit as power_max and
>> card default power limit (tdp) as power_rated_max.
>>
>> v2:
>> - Fix review comments (Riana)
>> v3:
>> - Use drmm_mutex_init (Matt Brost)
>> - Print error value (Matt Brost)
>> - Convert enums to uppercase (Matt Brost)
>> - Avoid extra reg read in hwmon_is_visible function (Riana)
>> - Use xe_device_assert_mem_access when applicable (Matt Brost)
>> - Add intel-xe@lists.freedesktop.org in Documentation (Matt Brost)
>> v4:
>> - Use prefix xe_hwmon prefix for all functions (Matt Brost/Andi)
>> - %s/hwmon_reg/xe_hwmon_reg (Andi)
>> - Fix review comments (Guenter/Andi)
>> v5:
>> - Fix review comments (Riana)
>>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> ---
>> .../ABI/testing/sysfs-driver-intel-xe-hwmon | 22 ++
>> drivers/gpu/drm/xe/Makefile | 3 +
>> drivers/gpu/drm/xe/regs/xe_gt_regs.h | 4 +
>> drivers/gpu/drm/xe/regs/xe_mchbar_regs.h | 33 ++
>> drivers/gpu/drm/xe/xe_device.c | 3 +
>> drivers/gpu/drm/xe/xe_device_types.h | 2 +
>> drivers/gpu/drm/xe/xe_hwmon.c | 358 ++++++++++++++++++
>> drivers/gpu/drm/xe/xe_hwmon.h | 20 +
>> 8 files changed, 445 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
>> create mode 100644 drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> create mode 100644 drivers/gpu/drm/xe/xe_hwmon.c
>> create mode 100644 drivers/gpu/drm/xe/xe_hwmon.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
>> new file mode 100644
>> index 000000000000..da0197a29fe4
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
>> @@ -0,0 +1,22 @@
>> +What: /sys/devices/.../hwmon/hwmon<i>/power1_max
>> +Date: September 2023
>> +KernelVersion: 6.5
>> +Contact: intel-xe@lists.freedesktop.org
>> +Description: RW. Card reactive sustained (PL1) power limit in microwatts.
>> +
>> + The power controller will throttle the operating frequency
>> + if the power averaged over a window (typically seconds)
>> + exceeds this limit. A read value of 0 means that the PL1
>> + power limit is disabled, writing 0 disables the
>> + limit. Writing values > 0 and <= TDP will enable the power limit.
>> +
>> + Only supported for particular Intel xe graphics platforms.
>> +
>> +What: /sys/devices/.../hwmon/hwmon<i>/power1_rated_max
>> +Date: September 2023
>> +KernelVersion: 6.5
>> +Contact: intel-xe@lists.freedesktop.org
>> +Description: RO. Card default power limit (default TDP setting).
>> +
>> + Only supported for particular Intel xe graphics platforms.
>> +
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index 86c8bd4c05a3..ca77aff60d48 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -117,6 +117,9 @@ xe-y += xe_bb.o \
>> xe_wa.o \
>> xe_wopcm.o
>>
>> +# graphics hardware monitoring (HWMON) support
>> +xe-$(CONFIG_HWMON) += xe_hwmon.o
>> +
>> # i915 Display compat #defines and #includes
>> subdir-ccflags-$(CONFIG_DRM_XE_DISPLAY) += \
>> -I$(srctree)/$(src)/display/ext \
>> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> index e13fbbdf6929..679cdba9f383 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
>> @@ -410,4 +410,8 @@
>> #define XEHPC_BCS5_BCS6_INTR_MASK XE_REG(0x190118)
>> #define XEHPC_BCS7_BCS8_INTR_MASK XE_REG(0x19011c)
>>
>> +#define PVC_GT0_PACKAGE_RAPL_LIMIT XE_REG(0x281008)
>> +#define PVC_GT0_PACKAGE_POWER_SKU_UNIT XE_REG(0x281068)
>> +#define PVC_GT0_PACKAGE_POWER_SKU XE_REG(0x281080)
>> +
>> #endif
>> diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> new file mode 100644
>> index 000000000000..27f1d42baf6d
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> @@ -0,0 +1,33 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_MCHBAR_REGS_H_
>> +#define _XE_MCHBAR_REGS_H_
>> +
>> +#include "regs/xe_reg_defs.h"
>> +
>> +/*
>> + * MCHBAR mirror.
>> + *
>> + * This mirrors the MCHBAR MMIO space whose location is determined by
>> + * device 0 function 0's pci config register 0x44 or 0x48 and matches it in
>> + * every way.
>> + */
>> +
>> +#define MCHBAR_MIRROR_BASE_SNB 0x140000
>> +
>> +#define PCU_CR_PACKAGE_POWER_SKU XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5930)
>> +#define PKG_TDP GENMASK_ULL(14, 0)
>> +#define PKG_MIN_PWR GENMASK_ULL(30, 16)
>> +#define PKG_MAX_PWR GENMASK_ULL(46, 32)
>> +
>> +#define PCU_CR_PACKAGE_POWER_SKU_UNIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
>> +#define PKG_PWR_UNIT REG_GENMASK(3, 0)
>> +
>> +#define PCU_CR_PACKAGE_RAPL_LIMIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
>> +#define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
>> +#define PKG_PWR_LIM_1_EN REG_BIT(15)
>> +
>> +#endif /* _XE_MCHBAR_REGS_H_ */
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index b6bcb6c3482e..2acdc22a6027 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -34,6 +34,7 @@
>> #include "xe_vm.h"
>> #include "xe_vm_madvise.h"
>> #include "xe_wait_user_fence.h"
>> +#include "xe_hwmon.h"
>>
>> #ifdef CONFIG_LOCKDEP
>> struct lockdep_map xe_device_mem_access_lockdep_map = {
>> @@ -337,6 +338,8 @@ int xe_device_probe(struct xe_device *xe)
>>
>> xe_pmu_register(&xe->pmu);
>>
>> + xe_hwmon_register(xe);
>> +
>> err = drmm_add_action_or_reset(&xe->drm, xe_device_sanitize, xe);
>> if (err)
>> return err;
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index a82f28c6a3a0..d1e319f305ef 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -365,6 +365,8 @@ struct xe_device {
>> /** @pmu: performance monitoring unit */
>> struct xe_pmu pmu;
>>
>> + struct xe_hwmon *hwmon;
>> +
>> /* private: */
>>
>> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
>> new file mode 100644
>> index 000000000000..7f4599d98541
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -0,0 +1,358 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#include <linux/hwmon.h>
>> +
>> +#include <drm/drm_managed.h>
>> +#include "regs/xe_gt_regs.h"
>> +#include "regs/xe_mchbar_regs.h"
>> +#include "xe_device.h"
>> +#include "xe_gt.h"
>> +#include "xe_hwmon.h"
>> +#include "xe_mmio.h"
>> +
>> +enum xe_hwmon_reg {
>> + REG_PKG_RAPL_LIMIT,
>> + REG_PKG_POWER_SKU,
>> + REG_PKG_POWER_SKU_UNIT,
>> +};
>> +
>> +enum xe_hwmon_reg_operation {
>> + REG_READ,
>> + REG_WRITE,
>> + REG_RMW,
>> +};
>> +
>> +/*
>> + * SF_* - scale factors for particular quantities according to hwmon spec.
>> + */
>> +#define SF_POWER 1000000 /* microwatts */
>> +
>> +struct xe_hwmon {
>> + struct device *hwmon_dev;
>> + struct xe_gt *gt;
>> + struct mutex hwmon_lock; /* rmw operations*/
>> + int scl_shift_power;
>> +};
>> +
>> +static u32 xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg)
>> +{
>> + 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_DG2)
>> + reg = PCU_CR_PACKAGE_RAPL_LIMIT;
>> + else if (xe->info.platform == XE_PVC)
>> + reg = PVC_GT0_PACKAGE_RAPL_LIMIT;
>> + break;
>> + case REG_PKG_POWER_SKU:
>> + if (xe->info.platform == XE_DG2)
>> + reg = PCU_CR_PACKAGE_POWER_SKU;
>> + else if (xe->info.platform == XE_PVC)
>> + reg = PVC_GT0_PACKAGE_POWER_SKU;
>> + break;
>> + case REG_PKG_POWER_SKU_UNIT:
>> + if (xe->info.platform == XE_DG2)
>> + reg = PCU_CR_PACKAGE_POWER_SKU_UNIT;
>> + else if (xe->info.platform == XE_PVC)
>> + reg = PVC_GT0_PACKAGE_POWER_SKU_UNIT;
>> + break;
>> + default:
>> + XE_MISSING_CASE(hwmon_reg);
>> + break;
>> + }
>> +
>> + return reg.raw;
>> +}
>> +
>> +static int xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
>> + enum xe_hwmon_reg_operation operation, u32 *value,
>> + u32 clr, u32 set)
>> +{
>> + struct xe_reg reg;
>> +
>> + reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
>> +
>> + if (!reg.raw)
>> + return -EOPNOTSUPP;
>> +
>> + switch (operation) {
>> + case REG_READ:
>> + *value = xe_mmio_read32(hwmon->gt, reg);
>> + return 0;
>> + case REG_WRITE:
>> + xe_mmio_write32(hwmon->gt, reg, *value);
>> + return 0;
>> + case REG_RMW:
>> + *value = xe_mmio_rmw32(hwmon->gt, reg, clr, set);
>> + return 0;
>> + default:
>> + XE_MISSING_CASE(operation);
>> + return -EOPNOTSUPP;
>> + }
>> +}
>> +
>> +int xe_hwmon_process_reg_read64(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg, u64 *value)
>> +{
>> + struct xe_reg reg;
>> +
>> + reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
>> +
>> + if (!reg.raw)
>> + return -EOPNOTSUPP;
>> +
>> + *value = xe_mmio_read64_2x32(hwmon->gt, reg);
>> +
>> + return 0;
>> +}
>> +
>> +#define PL1_DISABLE 0
>> +
>> +/*
>> + * HW allows arbitrary PL1 limits to be set but silently clamps these values to
>> + * "typical but not guaranteed" min/max values in REG_PKG_POWER_SKU. Follow the
>> + * same pattern for sysfs, allow arbitrary PL1 limits to be set but display
>> + * clamped values when read.
>> + */
>> +static int xe_hwmon_power_max_read(struct xe_hwmon *hwmon, long *value)
>> +{
>> + u32 reg_val;
>> + u64 reg_val64, min, max;
>> +
>> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, ®_val, 0, 0);
>> + /* Check if PL1 limit is disabled */
>> + if (!(reg_val & PKG_PWR_LIM_1_EN)) {
>> + *value = PL1_DISABLE;
>> + return 0;
>> + }
>> +
>> + 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_read64(hwmon, REG_PKG_POWER_SKU, ®_val64);
>> + min = REG_FIELD_GET(PKG_MIN_PWR, reg_val64);
>> + min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
>> + max = REG_FIELD_GET(PKG_MAX_PWR, reg_val64);
>> + max = mul_u64_u32_shr(max, SF_POWER, hwmon->scl_shift_power);
>> +
>> + if (min && max)
>> + *value = clamp_t(u64, *value, min, max);
>> +
>> + return 0;
>> +}
>> +
>> +static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
>> +{
>> + u32 reg_val;
>> +
>> + /* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
>> + if (value == PL1_DISABLE) {
>> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, ®_val,
>> + PKG_PWR_LIM_1_EN, 0);
>> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, ®_val,
>> + PKG_PWR_LIM_1_EN, 0);
>> +
>> + if (reg_val & PKG_PWR_LIM_1_EN)
>> + return -ENODEV;
>> + }
>> +
>> + /* Computation in 64-bits to avoid overflow. Round to nearest. */
>> + reg_val = DIV_ROUND_CLOSEST_ULL((u64)value << hwmon->scl_shift_power, SF_POWER);
>> + reg_val = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, reg_val);
>> +
>> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, ®_val,
>> + PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val);
>> +
>> + return 0;
>> +}
>> +
>> +static int xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, long *value)
>> +{
>> + u32 reg_val;
>> +
>> + xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU, REG_READ, ®_val, 0, 0);
>> + reg_val = REG_FIELD_GET(PKG_TDP, reg_val);
>> + *value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct hwmon_channel_info *hwmon_info[] = {
>> + HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX),
>> + NULL
>> +};
>> +
>> +static umode_t
>> +xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int chan)
>> +{
>> + switch (attr) {
>> + case hwmon_power_max:
>> + return xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT) ? 0664 : 0;
>> + case hwmon_power_rated_max:
>> + return xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU) ? 0444 : 0;
>> + default:
>> + return 0;
>> + }
>> +}
>> +
>> +static int
>> +xe_hwmon_power_read(struct xe_hwmon *hwmon, u32 attr, int chan, long *val)
>> +{
>> + switch (attr) {
>> + case hwmon_power_max:
>> + return xe_hwmon_power_max_read(hwmon, val);
>> + case hwmon_power_rated_max:
>> + return xe_hwmon_power_rated_max_read(hwmon, val);
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +}
>> +
>> +static int
>> +xe_hwmon_power_write(struct xe_hwmon *hwmon, u32 attr, int chan, long val)
>> +{
>> + switch (attr) {
>> + case hwmon_power_max:
>> + return xe_hwmon_power_max_write(hwmon, val);
>> + default:
>> + return -EOPNOTSUPP;
>> + }
>> +}
>> +
>> +static umode_t
>> +xe_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>> + u32 attr, int channel)
>> +{
>> + struct xe_hwmon *hwmon = (struct xe_hwmon *)drvdata;
>> + int ret;
>> +
>> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
>> +
>> + switch (type) {
>> + case hwmon_power:
>> + ret = xe_hwmon_power_is_visible(hwmon, attr, channel);
>> + break;
>> + default:
>> + ret = 0;
>> + break;
>> + }
>> +
>> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
>> +
>> + return ret;
>> +}
>> +
>> +static int
>> +xe_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> + int channel, long *val)
>> +{
>> + struct xe_hwmon *hwmon = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
>> +
>> + switch (type) {
>> + case hwmon_power:
>> + ret = xe_hwmon_power_read(hwmon, attr, channel, val);
>> + break;
>> + default:
>> + ret = -EOPNOTSUPP;
>> + break;
>> + }
>> +
>> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
>> +
>> + return ret;
>> +}
>> +
>> +static int
>> +xe_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> + int channel, long val)
>> +{
>> + struct xe_hwmon *hwmon = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
>> +
>> + switch (type) {
>> + case hwmon_power:
>> + ret = xe_hwmon_power_write(hwmon, attr, channel, val);
>> + break;
>> + default:
>> + ret = -EOPNOTSUPP;
>> + break;
>> + }
>> +
>> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
>> +
>> + return ret;
>> +}
>> +
>> +static const struct hwmon_ops hwmon_ops = {
>> + .is_visible = xe_hwmon_is_visible,
>> + .read = xe_hwmon_read,
>> + .write = xe_hwmon_write,
>> +};
>> +
>> +static const struct hwmon_chip_info hwmon_chip_info = {
>> + .ops = &hwmon_ops,
>> + .info = hwmon_info,
>> +};
>> +
>> +static void
>> +xe_hwmon_get_preregistration_info(struct xe_device *xe)
>> +{
>> + struct xe_hwmon *hwmon = xe->hwmon;
>> + u32 val_sku_unit = 0;
>> + int ret;
>> +
>> + ret = xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU_UNIT, REG_READ, &val_sku_unit, 0, 0);
>> + /*
>> + * The contents of register PKG_POWER_SKU_UNIT do not change,
>> + * so read it once and store the shift values.
>> + */
>> + if (!ret)
>> + hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
>> +}
>> +
>> +void xe_hwmon_register(struct xe_device *xe)
>> +{
>> + struct device *dev = xe->drm.dev;
>> + struct xe_hwmon *hwmon;
>> +
>> + /* hwmon is available only for dGfx */
>> + if (!IS_DGFX(xe))
>> + return;
>> +
>> + hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
>> + if (!hwmon)
>> + return;
>> +
>> + xe->hwmon = hwmon;
>> +
>> + drmm_mutex_init(&xe->drm, &hwmon->hwmon_lock);
>> +
>> + /* primary GT to access device level properties */
>> + hwmon->gt = xe->tiles[0].primary_gt;
>
> So, what happens with the other tiles?
> We should get them in since the beginning since that will impose
> interface changes and bigger changes on IGT tests.
For now gt specific attributes (Gt energy) are not handled in this
series. When those are added those will be new entries with label and
shouldn't affect existing entries. As discussed offline when lables are
implemented for energy will consider adding it for other entries as
well. As suggested I will drop first patch and resend the series by
adding your acked-by.
Thanks,
Badal
>
>> +
>> + xe_hwmon_get_preregistration_info(xe);
>> +
>> + drm_dbg(&xe->drm, "Register xe hwmon interface\n");
>> +
>> + /* hwmon_dev points to device hwmon<i> */
>> + hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev,
>> + "xe",
>> + hwmon,
>> + &hwmon_chip_info,
>> + NULL);
>> + if (IS_ERR(hwmon->hwmon_dev)) {
>> + drm_warn(&xe->drm, "Failed to register xe hwmon (%pe)\n", hwmon->hwmon_dev);
>> + xe->hwmon = NULL;
>> + return;
>> + }
>> +}
>> +
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.h b/drivers/gpu/drm/xe/xe_hwmon.h
>> new file mode 100644
>> index 000000000000..1ec45cf1d19b
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.h
>> @@ -0,0 +1,20 @@
>> +/* SPDX-License-Identifier: MIT */
>> +
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_HWMON_H_
>> +#define _XE_HWMON_H_
>> +
>> +#include <linux/types.h>
>> +
>> +struct xe_device;
>> +
>> +#if IS_REACHABLE(CONFIG_HWMON)
>> +void xe_hwmon_register(struct xe_device *xe);
>> +#else
>> +static inline void xe_hwmon_register(struct xe_device *xe) { };
>> +#endif
>> +
>> +#endif /* _XE_HWMON_H_ */
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/6] drm/xe/hwmon: Expose power attributes
2023-09-21 10:25 ` [PATCH v5 2/6] drm/xe/hwmon: Expose power attributes Badal Nilawar
2023-09-21 13:22 ` Riana Tauro
2023-09-21 16:25 ` [Intel-xe] " Rodrigo Vivi
@ 2023-09-22 17:24 ` Andi Shyti
2023-09-25 4:34 ` Nilawar, Badal
2 siblings, 1 reply; 24+ messages in thread
From: Andi Shyti @ 2023-09-22 17:24 UTC (permalink / raw)
To: Badal Nilawar
Cc: intel-xe, linux-hwmon, anshuman.gupta, ashutosh.dixit, linux,
andi.shyti, riana.tauro, matthew.brost, rodrigo.vivi
Hi Badal,
[...]
> +static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
> +{
> + u32 reg_val;
> +
> + /* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
> + if (value == PL1_DISABLE) {
> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, ®_val,
> + PKG_PWR_LIM_1_EN, 0);
> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, ®_val,
> + PKG_PWR_LIM_1_EN, 0);
> +
> + if (reg_val & PKG_PWR_LIM_1_EN)
> + return -ENODEV;
so, here you are trying to disable PL1 and check then if it's
disabled. Shall we try at least twice before returning error?
And why ENODEV? It might be that we failed to write on the
register but it doesn't mean that the device is wrong.
> + }
> +
> + /* Computation in 64-bits to avoid overflow. Round to nearest. */
> + reg_val = DIV_ROUND_CLOSEST_ULL((u64)value << hwmon->scl_shift_power, SF_POWER);
> + reg_val = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, reg_val);
> +
> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, ®_val,
> + PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val);
> +
> + return 0;
> +}
[...]
> + /* hwmon_dev points to device hwmon<i> */
> + hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev,
> + "xe",
> + hwmon,
> + &hwmon_chip_info,
> + NULL);
here the allignment is a bit fancy... in this cases I wouldn't
mind going up to 100 characters or not align under the bracket.
I would write it like this
hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev,
"xe", hwmon, &hwmon_chip_info, NULL);
but, of course, it's a matter of taste. Up to you.
Andi
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 2/6] drm/xe/hwmon: Expose power attributes
2023-09-22 17:24 ` Andi Shyti
@ 2023-09-25 4:34 ` Nilawar, Badal
0 siblings, 0 replies; 24+ messages in thread
From: Nilawar, Badal @ 2023-09-25 4:34 UTC (permalink / raw)
To: Andi Shyti
Cc: intel-xe, linux-hwmon, anshuman.gupta, ashutosh.dixit, linux,
riana.tauro, matthew.brost, rodrigo.vivi
Hi Andi,
On 22-09-2023 22:54, Andi Shyti wrote:
> Hi Badal,
>
> [...]
>
>> +static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
>> +{
>> + u32 reg_val;
>> +
>> + /* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
>> + if (value == PL1_DISABLE) {
>> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, ®_val,
>> + PKG_PWR_LIM_1_EN, 0);
>> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, ®_val,
>> + PKG_PWR_LIM_1_EN, 0);
>> +
>> + if (reg_val & PKG_PWR_LIM_1_EN)
>> + return -ENODEV;
>
> so, here you are trying to disable PL1 and check then if it's
> disabled. Shall we try at least twice before returning error?
I think lets keep write once only.
>
> And why ENODEV? It might be that we failed to write on the
> register but it doesn't mean that the device is wrong.
Will return ENOSUPP.
>
>> + }
>> +
>> + /* Computation in 64-bits to avoid overflow. Round to nearest. */
>> + reg_val = DIV_ROUND_CLOSEST_ULL((u64)value << hwmon->scl_shift_power, SF_POWER);
>> + reg_val = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, reg_val);
>> +
>> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, ®_val,
>> + PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val);
>> +
>> + return 0;
>> +}
>
> [...]
>
>> + /* hwmon_dev points to device hwmon<i> */
>> + hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev,
>> + "xe",
>> + hwmon,
>> + &hwmon_chip_info,
>> + NULL);
>
> here the allignment is a bit fancy... in this cases I wouldn't
> mind going up to 100 characters or not align under the bracket.
>
> I would write it like this
>
> hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev,
> "xe", hwmon, &hwmon_chip_info, NULL);
>
> but, of course, it's a matter of taste. Up to you.
Not sure why this is fancy. Above suggestion checkpatch --strict throws
CHECK: Alignment should match open parenthesis.
If I write like this checkpatch goes fine.
hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev, "xe",
hwmon,
&hwmon_chip_info,
NULL);
Regards,
Badal
>
> Andi
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 3/6] drm/xe/hwmon: Expose card reactive critical power
2023-09-21 10:25 [PATCH v5 0/6] Add HWMON support for DGFX Badal Nilawar
2023-09-21 10:25 ` [PATCH v5 1/6] drm/xe: Add XE_MISSING_CASE macro Badal Nilawar
2023-09-21 10:25 ` [PATCH v5 2/6] drm/xe/hwmon: Expose power attributes Badal Nilawar
@ 2023-09-21 10:25 ` Badal Nilawar
2023-09-21 10:25 ` [PATCH v5 4/6] drm/xe/hwmon: Expose input voltage attribute Badal Nilawar
` (2 subsequent siblings)
5 siblings, 0 replies; 24+ messages in thread
From: Badal Nilawar @ 2023-09-21 10:25 UTC (permalink / raw)
To: intel-xe, linux-hwmon
Cc: anshuman.gupta, ashutosh.dixit, linux, andi.shyti, riana.tauro,
matthew.brost, rodrigo.vivi
Expose the card reactive critical (I1) power. I1 is exposed as
power1_crit in microwatts (typically for client products) or as
curr1_crit in milliamperes (typically for server).
v2: Move PCODE_MBOX macro to pcode file (Riana)
v3: s/IS_DG2/(gt_to_xe(gt)->info.platform == XE_DG2)
v4: Fix review comments (Andi)
Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
Reviewed-by: Riana Tauro <riana.tauro@intel.com>
---
.../ABI/testing/sysfs-driver-intel-xe-hwmon | 26 +++++
drivers/gpu/drm/xe/xe_hwmon.c | 105 +++++++++++++++++-
drivers/gpu/drm/xe/xe_pcode.h | 5 +
drivers/gpu/drm/xe/xe_pcode_api.h | 7 ++
4 files changed, 142 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
index da0197a29fe4..37263b09b6e4 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
@@ -20,3 +20,29 @@ Description: RO. Card default power limit (default TDP setting).
Only supported for particular Intel xe graphics platforms.
+What: /sys/devices/.../hwmon/hwmon<i>/power1_crit
+Date: September 2023
+KernelVersion: 6.5
+Contact: intel-xe@lists.freedesktop.org
+Description: RW. Card reactive critical (I1) power limit in microwatts.
+
+ Card reactive critical (I1) power limit in microwatts is exposed
+ for client products. The power controller will throttle the
+ operating frequency if the power averaged over a window exceeds
+ this limit.
+
+ Only supported for particular Intel xe graphics platforms.
+
+What: /sys/devices/.../hwmon/hwmon<i>/curr1_crit
+Date: September 2023
+KernelVersion: 6.5
+Contact: intel-xe@lists.freedesktop.org
+Description: RW. Card reactive critical (I1) power limit in milliamperes.
+
+ Card reactive critical (I1) power limit in milliamperes is
+ exposed for server products. The power controller will throttle
+ the operating frequency if the power averaged over a window
+ exceeds this limit.
+
+ Only supported for particular Intel xe graphics platforms.
+
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
index 7f4599d98541..ef0893bf1268 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -12,6 +12,8 @@
#include "xe_gt.h"
#include "xe_hwmon.h"
#include "xe_mmio.h"
+#include "xe_pcode.h"
+#include "xe_pcode_api.h"
enum xe_hwmon_reg {
REG_PKG_RAPL_LIMIT,
@@ -29,6 +31,7 @@ enum xe_hwmon_reg_operation {
* SF_* - scale factors for particular quantities according to hwmon spec.
*/
#define SF_POWER 1000000 /* microwatts */
+#define SF_CURR 1000 /* milliamperes */
struct xe_hwmon {
struct device *hwmon_dev;
@@ -182,18 +185,43 @@ static int xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, long *value)
}
static const struct hwmon_channel_info *hwmon_info[] = {
- HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX),
+ HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
+ HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
NULL
};
+/* I1 is exposed as power_crit or as curr_crit depending on bit 31 */
+static int xe_hwmon_pcode_read_i1(struct xe_gt *gt, u32 *uval)
+{
+ /* Avoid Illegal Subcommand error */
+ if (gt_to_xe(gt)->info.platform == XE_DG2)
+ return -ENXIO;
+
+ return xe_pcode_read(gt, PCODE_MBOX(PCODE_POWER_SETUP,
+ POWER_SETUP_SUBCOMMAND_READ_I1, 0),
+ uval, 0);
+}
+
+static int xe_hwmon_pcode_write_i1(struct xe_gt *gt, u32 uval)
+{
+ return xe_pcode_write(gt, PCODE_MBOX(PCODE_POWER_SETUP,
+ POWER_SETUP_SUBCOMMAND_WRITE_I1, 0),
+ uval);
+}
+
static umode_t
xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int chan)
{
+ u32 uval;
+
switch (attr) {
case hwmon_power_max:
return xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT) ? 0664 : 0;
case hwmon_power_rated_max:
return xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU) ? 0444 : 0;
+ case hwmon_power_crit:
+ return (xe_hwmon_pcode_read_i1(hwmon->gt, &uval) ||
+ !(uval & POWER_SETUP_I1_WATTS)) ? 0 : 0644;
default:
return 0;
}
@@ -202,11 +230,23 @@ xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int chan)
static int
xe_hwmon_power_read(struct xe_hwmon *hwmon, u32 attr, int chan, long *val)
{
+ int ret;
+ u32 uval;
+
switch (attr) {
case hwmon_power_max:
return xe_hwmon_power_max_read(hwmon, val);
case hwmon_power_rated_max:
return xe_hwmon_power_rated_max_read(hwmon, val);
+ case hwmon_power_crit:
+ ret = xe_hwmon_pcode_read_i1(hwmon->gt, &uval);
+ if (ret)
+ return ret;
+ if (!(uval & POWER_SETUP_I1_WATTS))
+ return -ENODEV;
+ *val = mul_u64_u32_shr(REG_FIELD_GET(POWER_SETUP_I1_DATA_MASK, uval),
+ SF_POWER, POWER_SETUP_I1_SHIFT);
+ return 0;
default:
return -EOPNOTSUPP;
}
@@ -215,9 +255,63 @@ xe_hwmon_power_read(struct xe_hwmon *hwmon, u32 attr, int chan, long *val)
static int
xe_hwmon_power_write(struct xe_hwmon *hwmon, u32 attr, int chan, long val)
{
+ u32 uval;
+
switch (attr) {
case hwmon_power_max:
return xe_hwmon_power_max_write(hwmon, val);
+ case hwmon_power_crit:
+ uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_POWER);
+ return xe_hwmon_pcode_write_i1(hwmon->gt, uval);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static umode_t
+xe_hwmon_curr_is_visible(const struct xe_hwmon *hwmon, u32 attr)
+{
+ u32 uval;
+
+ switch (attr) {
+ case hwmon_curr_crit:
+ return (xe_hwmon_pcode_read_i1(hwmon->gt, &uval) ||
+ (uval & POWER_SETUP_I1_WATTS)) ? 0 : 0644;
+ default:
+ return 0;
+ }
+}
+
+static int
+xe_hwmon_curr_read(struct xe_hwmon *hwmon, u32 attr, long *val)
+{
+ int ret;
+ u32 uval;
+
+ switch (attr) {
+ case hwmon_curr_crit:
+ ret = xe_hwmon_pcode_read_i1(hwmon->gt, &uval);
+ if (ret)
+ return ret;
+ if (uval & POWER_SETUP_I1_WATTS)
+ return -ENODEV;
+ *val = mul_u64_u32_shr(REG_FIELD_GET(POWER_SETUP_I1_DATA_MASK, uval),
+ SF_CURR, POWER_SETUP_I1_SHIFT);
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int
+xe_hwmon_curr_write(struct xe_hwmon *hwmon, u32 attr, long val)
+{
+ u32 uval;
+
+ switch (attr) {
+ case hwmon_curr_crit:
+ uval = DIV_ROUND_CLOSEST_ULL(val << POWER_SETUP_I1_SHIFT, SF_CURR);
+ return xe_hwmon_pcode_write_i1(hwmon->gt, uval);
default:
return -EOPNOTSUPP;
}
@@ -236,6 +330,9 @@ xe_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
case hwmon_power:
ret = xe_hwmon_power_is_visible(hwmon, attr, channel);
break;
+ case hwmon_curr:
+ ret = xe_hwmon_curr_is_visible(hwmon, attr);
+ break;
default:
ret = 0;
break;
@@ -259,6 +356,9 @@ xe_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
case hwmon_power:
ret = xe_hwmon_power_read(hwmon, attr, channel, val);
break;
+ case hwmon_curr:
+ ret = xe_hwmon_curr_read(hwmon, attr, val);
+ break;
default:
ret = -EOPNOTSUPP;
break;
@@ -282,6 +382,9 @@ xe_hwmon_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
case hwmon_power:
ret = xe_hwmon_power_write(hwmon, attr, channel, val);
break;
+ case hwmon_curr:
+ ret = xe_hwmon_curr_write(hwmon, attr, val);
+ break;
default:
ret = -EOPNOTSUPP;
break;
diff --git a/drivers/gpu/drm/xe/xe_pcode.h b/drivers/gpu/drm/xe/xe_pcode.h
index 3b4aa8c1a3ba..08cb1d047cba 100644
--- a/drivers/gpu/drm/xe/xe_pcode.h
+++ b/drivers/gpu/drm/xe/xe_pcode.h
@@ -22,4 +22,9 @@ int xe_pcode_write_timeout(struct xe_gt *gt, u32 mbox, u32 val,
int xe_pcode_request(struct xe_gt *gt, u32 mbox, u32 request,
u32 reply_mask, u32 reply, int timeout_ms);
+#define PCODE_MBOX(mbcmd, param1, param2)\
+ (FIELD_PREP(PCODE_MB_COMMAND, mbcmd)\
+ | FIELD_PREP(PCODE_MB_PARAM1, param1)\
+ | FIELD_PREP(PCODE_MB_PARAM2, param2))
+
#endif
diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
index 837ff7c71280..5935cfe30204 100644
--- a/drivers/gpu/drm/xe/xe_pcode_api.h
+++ b/drivers/gpu/drm/xe/xe_pcode_api.h
@@ -35,6 +35,13 @@
#define DGFX_GET_INIT_STATUS 0x0
#define DGFX_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 */
+#define POWER_SETUP_I1_DATA_MASK REG_GENMASK(15, 0)
+
struct pcode_err_decode {
int errno;
const char *str;
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 4/6] drm/xe/hwmon: Expose input voltage attribute
2023-09-21 10:25 [PATCH v5 0/6] Add HWMON support for DGFX Badal Nilawar
` (2 preceding siblings ...)
2023-09-21 10:25 ` [PATCH v5 3/6] drm/xe/hwmon: Expose card reactive critical power Badal Nilawar
@ 2023-09-21 10:25 ` Badal Nilawar
2023-09-21 10:25 ` [PATCH v5 5/6] drm/xe/hwmon: Expose hwmon energy attribute Badal Nilawar
2023-09-21 10:25 ` [PATCH v5 6/6] drm/xe/hwmon: Expose power1_max_interval Badal Nilawar
5 siblings, 0 replies; 24+ messages in thread
From: Badal Nilawar @ 2023-09-21 10:25 UTC (permalink / raw)
To: intel-xe, linux-hwmon
Cc: anshuman.gupta, ashutosh.dixit, linux, andi.shyti, riana.tauro,
matthew.brost, rodrigo.vivi
Use Xe HWMON subsystem to display the input voltage.
v2:
- Rename hwm_get_vltg to hwm_get_voltage (Riana)
- Use scale factor SF_VOLTAGE (Riana)
v3:
- %s/gt_perf_status/REG_GT_PERF_STATUS/
- Remove platform check from hwmon_get_voltage()
v4:
- Fix review comments (Andi)
Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
Reviewed-by: Riana Tauro <riana.tauro@intel.com>
---
.../ABI/testing/sysfs-driver-intel-xe-hwmon | 6 ++
drivers/gpu/drm/xe/regs/xe_gt_regs.h | 3 +
drivers/gpu/drm/xe/xe_hwmon.c | 58 +++++++++++++++++++
3 files changed, 67 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
index 37263b09b6e4..7f9407c20864 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
@@ -44,5 +44,11 @@ Description: RW. Card reactive critical (I1) power limit in milliamperes.
the operating frequency if the power averaged over a window
exceeds this limit.
+What: /sys/devices/.../hwmon/hwmon<i>/in0_input
+Date: September 2023
+KernelVersion: 6.5
+Contact: intel-xe@lists.freedesktop.org
+Description: RO. Current Voltage in millivolt.
+
Only supported for particular Intel xe graphics platforms.
diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
index 679cdba9f383..102663cbc320 100644
--- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
@@ -374,6 +374,9 @@
#define GT_GFX_RC6_LOCKED XE_REG(0x138104)
#define GT_GFX_RC6 XE_REG(0x138108)
+#define GT_PERF_STATUS XE_REG(0x1381b4)
+#define VOLTAGE_MASK REG_GENMASK(10, 0)
+
#define GT_INTR_DW(x) XE_REG(0x190018 + ((x) * 4))
#define GUC_SG_INTR_ENABLE XE_REG(0x190038)
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
index ef0893bf1268..431995045faa 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -3,7 +3,9 @@
* Copyright © 2023 Intel Corporation
*/
+#include <linux/hwmon-sysfs.h>
#include <linux/hwmon.h>
+#include <linux/types.h>
#include <drm/drm_managed.h>
#include "regs/xe_gt_regs.h"
@@ -19,6 +21,7 @@ enum xe_hwmon_reg {
REG_PKG_RAPL_LIMIT,
REG_PKG_POWER_SKU,
REG_PKG_POWER_SKU_UNIT,
+ REG_GT_PERF_STATUS,
};
enum xe_hwmon_reg_operation {
@@ -32,6 +35,7 @@ enum xe_hwmon_reg_operation {
*/
#define SF_POWER 1000000 /* microwatts */
#define SF_CURR 1000 /* milliamperes */
+#define SF_VOLTAGE 1000 /* millivolts */
struct xe_hwmon {
struct device *hwmon_dev;
@@ -64,6 +68,10 @@ static u32 xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg)
else if (xe->info.platform == XE_PVC)
reg = PVC_GT0_PACKAGE_POWER_SKU_UNIT;
break;
+ case REG_GT_PERF_STATUS:
+ if (xe->info.platform == XE_DG2)
+ reg = GT_PERF_STATUS;
+ break;
default:
XE_MISSING_CASE(hwmon_reg);
break;
@@ -187,6 +195,7 @@ static int xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, long *value)
static const struct hwmon_channel_info *hwmon_info[] = {
HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
+ HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
NULL
};
@@ -209,6 +218,18 @@ static int xe_hwmon_pcode_write_i1(struct xe_gt *gt, u32 uval)
uval);
}
+static int xe_hwmon_get_voltage(struct xe_hwmon *hwmon, long *value)
+{
+ u32 reg_val;
+
+ xe_hwmon_process_reg(hwmon, REG_GT_PERF_STATUS,
+ REG_READ, ®_val, 0, 0);
+ /* HW register value in units of 2.5 millivolt */
+ *value = DIV_ROUND_CLOSEST(REG_FIELD_GET(VOLTAGE_MASK, reg_val) * 2500, SF_VOLTAGE);
+
+ return 0;
+}
+
static umode_t
xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int chan)
{
@@ -317,6 +338,37 @@ xe_hwmon_curr_write(struct xe_hwmon *hwmon, u32 attr, long val)
}
}
+static umode_t
+xe_hwmon_in_is_visible(struct xe_hwmon *hwmon, u32 attr)
+{
+ switch (attr) {
+ case hwmon_in_input:
+ return xe_hwmon_get_reg(hwmon, REG_GT_PERF_STATUS) ? 0444 : 0;
+ default:
+ return 0;
+ }
+}
+
+static int
+xe_hwmon_in_read(struct xe_hwmon *hwmon, u32 attr, long *val)
+{
+ int ret;
+
+ xe_device_mem_access_get(gt_to_xe(hwmon->gt));
+
+ switch (attr) {
+ case hwmon_in_input:
+ ret = xe_hwmon_get_voltage(hwmon, val);
+ break;
+ default:
+ ret = -EOPNOTSUPP;
+ }
+
+ xe_device_mem_access_put(gt_to_xe(hwmon->gt));
+
+ return ret;
+}
+
static umode_t
xe_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
u32 attr, int channel)
@@ -333,6 +385,9 @@ xe_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
case hwmon_curr:
ret = xe_hwmon_curr_is_visible(hwmon, attr);
break;
+ case hwmon_in:
+ ret = xe_hwmon_in_is_visible(hwmon, attr);
+ break;
default:
ret = 0;
break;
@@ -359,6 +414,9 @@ xe_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
case hwmon_curr:
ret = xe_hwmon_curr_read(hwmon, attr, val);
break;
+ case hwmon_in:
+ ret = xe_hwmon_in_read(hwmon, attr, val);
+ break;
default:
ret = -EOPNOTSUPP;
break;
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 5/6] drm/xe/hwmon: Expose hwmon energy attribute
2023-09-21 10:25 [PATCH v5 0/6] Add HWMON support for DGFX Badal Nilawar
` (3 preceding siblings ...)
2023-09-21 10:25 ` [PATCH v5 4/6] drm/xe/hwmon: Expose input voltage attribute Badal Nilawar
@ 2023-09-21 10:25 ` Badal Nilawar
2023-09-21 14:09 ` Riana Tauro
2023-09-21 10:25 ` [PATCH v5 6/6] drm/xe/hwmon: Expose power1_max_interval Badal Nilawar
5 siblings, 1 reply; 24+ messages in thread
From: Badal Nilawar @ 2023-09-21 10:25 UTC (permalink / raw)
To: intel-xe, linux-hwmon
Cc: anshuman.gupta, ashutosh.dixit, linux, andi.shyti, riana.tauro,
matthew.brost, rodrigo.vivi
Expose hwmon energy attribute to show device level energy usage
v2:
- %s/hwm_/hwmon_/
- Convert enums to upper case
v3:
- %s/hwmon_/xe_hwmon
- Remove gt specific hwmon attributes
v4:
- %s/REG_PKG_ENERGY_STATUS/REG_ENERGY_STATUS_ALL (Riana)
Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
.../ABI/testing/sysfs-driver-intel-xe-hwmon | 7 ++
drivers/gpu/drm/xe/regs/xe_gt_regs.h | 2 +
drivers/gpu/drm/xe/regs/xe_mchbar_regs.h | 3 +
drivers/gpu/drm/xe/xe_hwmon.c | 105 +++++++++++++++++-
4 files changed, 116 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
index 7f9407c20864..1a7a6c23e141 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
@@ -52,3 +52,10 @@ Description: RO. Current Voltage in millivolt.
Only supported for particular Intel xe graphics platforms.
+What: /sys/devices/.../hwmon/hwmon<i>/energy1_input
+Date: September 2023
+KernelVersion: 6.5
+Contact: intel-xe@lists.freedesktop.org
+Description: RO. Energy input of device in microjoules.
+
+ Only supported for particular Intel xe graphics platforms.
diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
index 102663cbc320..9e6ce74fdd68 100644
--- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
@@ -413,8 +413,10 @@
#define XEHPC_BCS5_BCS6_INTR_MASK XE_REG(0x190118)
#define XEHPC_BCS7_BCS8_INTR_MASK XE_REG(0x19011c)
+#define PVC_GT0_PACKAGE_ENERGY_STATUS XE_REG(0x281004)
#define PVC_GT0_PACKAGE_RAPL_LIMIT XE_REG(0x281008)
#define PVC_GT0_PACKAGE_POWER_SKU_UNIT XE_REG(0x281068)
+#define PVC_GT0_PLATFORM_ENERGY_STATUS XE_REG(0x28106c)
#define PVC_GT0_PACKAGE_POWER_SKU XE_REG(0x281080)
#endif
diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
index 27f1d42baf6d..d8ecbe1858d1 100644
--- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
@@ -25,6 +25,9 @@
#define PCU_CR_PACKAGE_POWER_SKU_UNIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
#define PKG_PWR_UNIT REG_GENMASK(3, 0)
+#define PKG_ENERGY_UNIT REG_GENMASK(12, 8)
+
+#define PCU_CR_PACKAGE_ENERGY_STATUS XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x593c)
#define PCU_CR_PACKAGE_RAPL_LIMIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
#define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
index 431995045faa..cb75b9a386c0 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -22,6 +22,7 @@ enum xe_hwmon_reg {
REG_PKG_POWER_SKU,
REG_PKG_POWER_SKU_UNIT,
REG_GT_PERF_STATUS,
+ REG_PKG_ENERGY_STATUS,
};
enum xe_hwmon_reg_operation {
@@ -36,12 +37,20 @@ enum xe_hwmon_reg_operation {
#define SF_POWER 1000000 /* microwatts */
#define SF_CURR 1000 /* milliamperes */
#define SF_VOLTAGE 1000 /* millivolts */
+#define SF_ENERGY 1000000 /* microjoules */
+
+struct hwmon_energy_info {
+ u32 reg_val_prev;
+ long accum_energy; /* Accumulated energy for energy1_input */
+};
struct xe_hwmon {
struct device *hwmon_dev;
struct xe_gt *gt;
struct mutex hwmon_lock; /* rmw operations*/
int scl_shift_power;
+ int scl_shift_energy;
+ struct hwmon_energy_info ei; /* Energy info for energy1_input */
};
static u32 xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg)
@@ -72,6 +81,12 @@ static u32 xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg)
if (xe->info.platform == XE_DG2)
reg = GT_PERF_STATUS;
break;
+ case REG_PKG_ENERGY_STATUS:
+ if (xe->info.platform == XE_DG2)
+ reg = PCU_CR_PACKAGE_ENERGY_STATUS;
+ else if (xe->info.platform == XE_PVC)
+ reg = PVC_GT0_PLATFORM_ENERGY_STATUS;
+ break;
default:
XE_MISSING_CASE(hwmon_reg);
break;
@@ -192,10 +207,59 @@ static int xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, long *value)
return 0;
}
+/*
+ * xe_hwmon_energy_get - Obtain energy value
+ *
+ * The underlying energy hardware register is 32-bits and is subject to
+ * overflow. How long before overflow? For example, with an example
+ * scaling bit shift of 14 bits (see register *PACKAGE_POWER_SKU_UNIT) and
+ * a power draw of 1000 watts, the 32-bit counter will overflow in
+ * approximately 4.36 minutes.
+ *
+ * Examples:
+ * 1 watt: (2^32 >> 14) / 1 W / (60 * 60 * 24) secs/day -> 3 days
+ * 1000 watts: (2^32 >> 14) / 1000 W / 60 secs/min -> 4.36 minutes
+ *
+ * The function significantly increases overflow duration (from 4.36
+ * minutes) by accumulating the energy register into a 'long' as allowed by
+ * the hwmon API. Using x86_64 128 bit arithmetic (see mul_u64_u32_shr()),
+ * a 'long' of 63 bits, SF_ENERGY of 1e6 (~20 bits) and
+ * hwmon->scl_shift_energy of 14 bits we have 57 (63 - 20 + 14) bits before
+ * energy1_input overflows. This at 1000 W is an overflow duration of 278 years.
+ */
+static void
+xe_hwmon_energy_get(struct xe_hwmon *hwmon, long *energy)
+{
+ struct hwmon_energy_info *ei = &hwmon->ei;
+ u32 reg_val;
+
+ xe_device_mem_access_get(gt_to_xe(hwmon->gt));
+
+ mutex_lock(&hwmon->hwmon_lock);
+
+ xe_hwmon_process_reg(hwmon, REG_PKG_ENERGY_STATUS, REG_READ,
+ ®_val, 0, 0);
+
+ if (reg_val >= ei->reg_val_prev)
+ ei->accum_energy += reg_val - ei->reg_val_prev;
+ else
+ ei->accum_energy += UINT_MAX - ei->reg_val_prev + reg_val;
+
+ ei->reg_val_prev = reg_val;
+
+ *energy = mul_u64_u32_shr(ei->accum_energy, SF_ENERGY,
+ hwmon->scl_shift_energy);
+
+ mutex_unlock(&hwmon->hwmon_lock);
+
+ xe_device_mem_access_put(gt_to_xe(hwmon->gt));
+}
+
static const struct hwmon_channel_info *hwmon_info[] = {
HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
+ HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
NULL
};
@@ -369,6 +433,29 @@ xe_hwmon_in_read(struct xe_hwmon *hwmon, u32 attr, long *val)
return ret;
}
+static umode_t
+xe_hwmon_energy_is_visible(struct xe_hwmon *hwmon, u32 attr)
+{
+ switch (attr) {
+ case hwmon_energy_input:
+ return xe_hwmon_get_reg(hwmon, REG_PKG_ENERGY_STATUS) ? 0444 : 0;
+ default:
+ return 0;
+ }
+}
+
+static int
+xe_hwmon_energy_read(struct xe_hwmon *hwmon, u32 attr, long *val)
+{
+ switch (attr) {
+ case hwmon_energy_input:
+ xe_hwmon_energy_get(hwmon, val);
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
static umode_t
xe_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
u32 attr, int channel)
@@ -388,6 +475,9 @@ xe_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
case hwmon_in:
ret = xe_hwmon_in_is_visible(hwmon, attr);
break;
+ case hwmon_energy:
+ ret = xe_hwmon_energy_is_visible(hwmon, attr);
+ break;
default:
ret = 0;
break;
@@ -417,6 +507,9 @@ xe_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
case hwmon_in:
ret = xe_hwmon_in_read(hwmon, attr, val);
break;
+ case hwmon_energy:
+ ret = xe_hwmon_energy_read(hwmon, attr, val);
+ break;
default:
ret = -EOPNOTSUPP;
break;
@@ -468,6 +561,7 @@ static void
xe_hwmon_get_preregistration_info(struct xe_device *xe)
{
struct xe_hwmon *hwmon = xe->hwmon;
+ long energy;
u32 val_sku_unit = 0;
int ret;
@@ -476,8 +570,17 @@ xe_hwmon_get_preregistration_info(struct xe_device *xe)
* The contents of register PKG_POWER_SKU_UNIT do not change,
* so read it once and store the shift values.
*/
- if (!ret)
+ if (!ret) {
hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
+ hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT, val_sku_unit);
+ }
+
+ /*
+ * Initialize 'struct hwmon_energy_info', i.e. set fields to the
+ * first value of the energy register read
+ */
+ if (xe_hwmon_is_visible(hwmon, hwmon_energy, hwmon_energy_input, 0))
+ xe_hwmon_energy_get(hwmon, &energy);
}
void xe_hwmon_register(struct xe_device *xe)
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v5 5/6] drm/xe/hwmon: Expose hwmon energy attribute
2023-09-21 10:25 ` [PATCH v5 5/6] drm/xe/hwmon: Expose hwmon energy attribute Badal Nilawar
@ 2023-09-21 14:09 ` Riana Tauro
0 siblings, 0 replies; 24+ messages in thread
From: Riana Tauro @ 2023-09-21 14:09 UTC (permalink / raw)
To: Badal Nilawar, intel-xe, linux-hwmon
Cc: anshuman.gupta, ashutosh.dixit, linux, andi.shyti, matthew.brost,
rodrigo.vivi
Hi Badal
On 9/21/2023 3:55 PM, Badal Nilawar wrote:
> Expose hwmon energy attribute to show device level energy usage
>
> v2:
> - %s/hwm_/hwmon_/
> - Convert enums to upper case
> v3:
> - %s/hwmon_/xe_hwmon
> - Remove gt specific hwmon attributes
> v4:
> - %s/REG_PKG_ENERGY_STATUS/REG_ENERGY_STATUS_ALL (Riana)
>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
> .../ABI/testing/sysfs-driver-intel-xe-hwmon | 7 ++
> drivers/gpu/drm/xe/regs/xe_gt_regs.h | 2 +
> drivers/gpu/drm/xe/regs/xe_mchbar_regs.h | 3 +
> drivers/gpu/drm/xe/xe_hwmon.c | 105 +++++++++++++++++-
> 4 files changed, 116 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> index 7f9407c20864..1a7a6c23e141 100644
> --- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> @@ -52,3 +52,10 @@ Description: RO. Current Voltage in millivolt.
>
> Only supported for particular Intel xe graphics platforms.
>
> +What: /sys/devices/.../hwmon/hwmon<i>/energy1_input
> +Date: September 2023
> +KernelVersion: 6.5
> +Contact: intel-xe@lists.freedesktop.org
> +Description: RO. Energy input of device in microjoules.
> +
> + Only supported for particular Intel xe graphics platforms.
> diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> index 102663cbc320..9e6ce74fdd68 100644
> --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> @@ -413,8 +413,10 @@
> #define XEHPC_BCS5_BCS6_INTR_MASK XE_REG(0x190118)
> #define XEHPC_BCS7_BCS8_INTR_MASK XE_REG(0x19011c)
>
> +#define PVC_GT0_PACKAGE_ENERGY_STATUS XE_REG(0x281004)
This is not used in the file
> #define PVC_GT0_PACKAGE_RAPL_LIMIT XE_REG(0x281008)
> #define PVC_GT0_PACKAGE_POWER_SKU_UNIT XE_REG(0x281068)
> +#define PVC_GT0_PLATFORM_ENERGY_STATUS XE_REG(0x28106c)
> #define PVC_GT0_PACKAGE_POWER_SKU XE_REG(0x281080)
>
> #endif
> diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> index 27f1d42baf6d..d8ecbe1858d1 100644
> --- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> @@ -25,6 +25,9 @@
>
> #define PCU_CR_PACKAGE_POWER_SKU_UNIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
> #define PKG_PWR_UNIT REG_GENMASK(3, 0)
> +#define PKG_ENERGY_UNIT REG_GENMASK(12, 8)
> +
> +#define PCU_CR_PACKAGE_ENERGY_STATUS XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x593c)
>
> #define PCU_CR_PACKAGE_RAPL_LIMIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
> #define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 431995045faa..cb75b9a386c0 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -22,6 +22,7 @@ enum xe_hwmon_reg {
> REG_PKG_POWER_SKU,
> REG_PKG_POWER_SKU_UNIT,
> REG_GT_PERF_STATUS,
> + REG_PKG_ENERGY_STATUS,
PVC register above has it PLATFORM. Is PKG okay incase the other
register is added later?
> };
>
> enum xe_hwmon_reg_operation {
> @@ -36,12 +37,20 @@ enum xe_hwmon_reg_operation {
> #define SF_POWER 1000000 /* microwatts */
> #define SF_CURR 1000 /* milliamperes */
> #define SF_VOLTAGE 1000 /* millivolts */
> +#define SF_ENERGY 1000000 /* microjoules */
> +
> +struct hwmon_energy_info {
Better to retain xe prefix since all the enums and structs have the
prefix across the file
With the above changes
Reviewed-by: Riana Tauro <riana.tauro@intel.com>
> + u32 reg_val_prev;
> + long accum_energy; /* Accumulated energy for energy1_input */
> +};
>
> struct xe_hwmon {
> struct device *hwmon_dev;
> struct xe_gt *gt;
> struct mutex hwmon_lock; /* rmw operations*/
> int scl_shift_power;
> + int scl_shift_energy;
> + struct hwmon_energy_info ei; /* Energy info for energy1_input */
> };
>
> static u32 xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg)
> @@ -72,6 +81,12 @@ static u32 xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg)
> if (xe->info.platform == XE_DG2)
> reg = GT_PERF_STATUS;
> break;
> + case REG_PKG_ENERGY_STATUS:
> + if (xe->info.platform == XE_DG2)
> + reg = PCU_CR_PACKAGE_ENERGY_STATUS;
> + else if (xe->info.platform == XE_PVC)
> + reg = PVC_GT0_PLATFORM_ENERGY_STATUS;
> + break;
> default:
> XE_MISSING_CASE(hwmon_reg);
> break;
> @@ -192,10 +207,59 @@ static int xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon, long *value)
> return 0;
> }
>
> +/*
> + * xe_hwmon_energy_get - Obtain energy value
> + *
> + * The underlying energy hardware register is 32-bits and is subject to
> + * overflow. How long before overflow? For example, with an example
> + * scaling bit shift of 14 bits (see register *PACKAGE_POWER_SKU_UNIT) and
> + * a power draw of 1000 watts, the 32-bit counter will overflow in
> + * approximately 4.36 minutes.
> + *
> + * Examples:
> + * 1 watt: (2^32 >> 14) / 1 W / (60 * 60 * 24) secs/day -> 3 days
> + * 1000 watts: (2^32 >> 14) / 1000 W / 60 secs/min -> 4.36 minutes
> + *
> + * The function significantly increases overflow duration (from 4.36
> + * minutes) by accumulating the energy register into a 'long' as allowed by
> + * the hwmon API. Using x86_64 128 bit arithmetic (see mul_u64_u32_shr()),
> + * a 'long' of 63 bits, SF_ENERGY of 1e6 (~20 bits) and
> + * hwmon->scl_shift_energy of 14 bits we have 57 (63 - 20 + 14) bits before
> + * energy1_input overflows. This at 1000 W is an overflow duration of 278 years.
> + */
> +static void
> +xe_hwmon_energy_get(struct xe_hwmon *hwmon, long *energy)
> +{
> + struct hwmon_energy_info *ei = &hwmon->ei;
> + u32 reg_val;
> +
> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> +
> + mutex_lock(&hwmon->hwmon_lock);
> +
> + xe_hwmon_process_reg(hwmon, REG_PKG_ENERGY_STATUS, REG_READ,
> + ®_val, 0, 0);
> +
> + if (reg_val >= ei->reg_val_prev)
> + ei->accum_energy += reg_val - ei->reg_val_prev;
> + else
> + ei->accum_energy += UINT_MAX - ei->reg_val_prev + reg_val;
> +
> + ei->reg_val_prev = reg_val;
> +
> + *energy = mul_u64_u32_shr(ei->accum_energy, SF_ENERGY,
> + hwmon->scl_shift_energy);
> +
> + mutex_unlock(&hwmon->hwmon_lock);
> +
> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> +}
> +
> static const struct hwmon_channel_info *hwmon_info[] = {
> HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
> HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
> HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
> + HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
> NULL
> };
>
> @@ -369,6 +433,29 @@ xe_hwmon_in_read(struct xe_hwmon *hwmon, u32 attr, long *val)
> return ret;
> }
>
> +static umode_t
> +xe_hwmon_energy_is_visible(struct xe_hwmon *hwmon, u32 attr)
> +{
> + switch (attr) {
> + case hwmon_energy_input:
> + return xe_hwmon_get_reg(hwmon, REG_PKG_ENERGY_STATUS) ? 0444 : 0;
> + default:
> + return 0;
> + }
> +}
> +
> +static int
> +xe_hwmon_energy_read(struct xe_hwmon *hwmon, u32 attr, long *val)
> +{
> + switch (attr) {
> + case hwmon_energy_input:
> + xe_hwmon_energy_get(hwmon, val);
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> static umode_t
> xe_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> u32 attr, int channel)
> @@ -388,6 +475,9 @@ xe_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> case hwmon_in:
> ret = xe_hwmon_in_is_visible(hwmon, attr);
> break;
> + case hwmon_energy:
> + ret = xe_hwmon_energy_is_visible(hwmon, attr);
> + break;
> default:
> ret = 0;
> break;
> @@ -417,6 +507,9 @@ xe_hwmon_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> case hwmon_in:
> ret = xe_hwmon_in_read(hwmon, attr, val);
> break;
> + case hwmon_energy:
> + ret = xe_hwmon_energy_read(hwmon, attr, val);
> + break;
> default:
> ret = -EOPNOTSUPP;
> break;
> @@ -468,6 +561,7 @@ static void
> xe_hwmon_get_preregistration_info(struct xe_device *xe)
> {
> struct xe_hwmon *hwmon = xe->hwmon;
> + long energy;
> u32 val_sku_unit = 0;
> int ret;
>
> @@ -476,8 +570,17 @@ xe_hwmon_get_preregistration_info(struct xe_device *xe)
> * The contents of register PKG_POWER_SKU_UNIT do not change,
> * so read it once and store the shift values.
> */
> - if (!ret)
> + if (!ret) {
> hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
> + hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT, val_sku_unit);
> + }
> +
> + /*
> + * Initialize 'struct hwmon_energy_info', i.e. set fields to the
> + * first value of the energy register read
> + */
> + if (xe_hwmon_is_visible(hwmon, hwmon_energy, hwmon_energy_input, 0))
> + xe_hwmon_energy_get(hwmon, &energy);
> }
>
> void xe_hwmon_register(struct xe_device *xe)
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 6/6] drm/xe/hwmon: Expose power1_max_interval
2023-09-21 10:25 [PATCH v5 0/6] Add HWMON support for DGFX Badal Nilawar
` (4 preceding siblings ...)
2023-09-21 10:25 ` [PATCH v5 5/6] drm/xe/hwmon: Expose hwmon energy attribute Badal Nilawar
@ 2023-09-21 10:25 ` Badal Nilawar
2023-09-21 11:43 ` [Intel-xe] " Ghimiray, Himal Prasad
5 siblings, 1 reply; 24+ messages in thread
From: Badal Nilawar @ 2023-09-21 10:25 UTC (permalink / raw)
To: intel-xe, linux-hwmon
Cc: anshuman.gupta, ashutosh.dixit, linux, andi.shyti, riana.tauro,
matthew.brost, rodrigo.vivi
Expose power1_max_interval, that is the tau corresponding to PL1, as a
custom hwmon attribute. Some bit manipulation is needed because of the
format of PKG_PWR_LIM_1_TIME in
PACKAGE_RAPL_LIMIT register (1.x * power(2,y))
v2: Get rpm wake ref while accessing power1_max_interval
v3: %s/hwmon/xe_hwmon/
Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
---
.../ABI/testing/sysfs-driver-intel-xe-hwmon | 11 ++
drivers/gpu/drm/xe/regs/xe_mchbar_regs.h | 8 +
drivers/gpu/drm/xe/xe_hwmon.c | 138 +++++++++++++++++-
3 files changed, 156 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
index 1a7a6c23e141..9ceb9c04b52b 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
@@ -59,3 +59,14 @@ Contact: intel-xe@lists.freedesktop.org
Description: RO. Energy input of device in microjoules.
Only supported for particular Intel xe graphics platforms.
+
+What: /sys/devices/.../hwmon/hwmon<i>/power1_max_interval
+Date: September 2023
+KernelVersion: 6.5
+Contact: intel-xe@lists.freedesktop.org
+Description: RW. Sustained power limit interval (Tau in PL1/Tau) in
+ milliseconds over which sustained power is averaged.
+
+ Only supported for particular Intel xe graphics platforms.
+
+
diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
index d8ecbe1858d1..519dd1067a19 100644
--- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
@@ -22,15 +22,23 @@
#define PKG_TDP GENMASK_ULL(14, 0)
#define PKG_MIN_PWR GENMASK_ULL(30, 16)
#define PKG_MAX_PWR GENMASK_ULL(46, 32)
+#define PKG_MAX_WIN GENMASK_ULL(54, 48)
+#define PKG_MAX_WIN_X GENMASK_ULL(54, 53)
+#define PKG_MAX_WIN_Y GENMASK_ULL(52, 48)
+
#define PCU_CR_PACKAGE_POWER_SKU_UNIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
#define PKG_PWR_UNIT REG_GENMASK(3, 0)
#define PKG_ENERGY_UNIT REG_GENMASK(12, 8)
+#define PKG_TIME_UNIT REG_GENMASK(19, 16)
#define PCU_CR_PACKAGE_ENERGY_STATUS XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x593c)
#define PCU_CR_PACKAGE_RAPL_LIMIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
#define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
#define PKG_PWR_LIM_1_EN REG_BIT(15)
+#define PKG_PWR_LIM_1_TIME REG_GENMASK(23, 17)
+#define PKG_PWR_LIM_1_TIME_X REG_GENMASK(23, 22)
+#define PKG_PWR_LIM_1_TIME_Y REG_GENMASK(21, 17)
#endif /* _XE_MCHBAR_REGS_H_ */
diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
index cb75b9a386c0..dfa638942d47 100644
--- a/drivers/gpu/drm/xe/xe_hwmon.c
+++ b/drivers/gpu/drm/xe/xe_hwmon.c
@@ -38,6 +38,7 @@ enum xe_hwmon_reg_operation {
#define SF_CURR 1000 /* milliamperes */
#define SF_VOLTAGE 1000 /* millivolts */
#define SF_ENERGY 1000000 /* microjoules */
+#define SF_TIME 1000 /* milliseconds */
struct hwmon_energy_info {
u32 reg_val_prev;
@@ -50,6 +51,7 @@ struct xe_hwmon {
struct mutex hwmon_lock; /* rmw operations*/
int scl_shift_power;
int scl_shift_energy;
+ int scl_shift_time;
struct hwmon_energy_info ei; /* Energy info for energy1_input */
};
@@ -255,6 +257,138 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, long *energy)
xe_device_mem_access_put(gt_to_xe(hwmon->gt));
}
+static ssize_t
+xe_hwmon_power1_max_interval_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct xe_hwmon *hwmon = dev_get_drvdata(dev);
+ u32 r, x, y, x_w = 2; /* 2 bits */
+ u64 tau4, out;
+
+ xe_device_mem_access_get(gt_to_xe(hwmon->gt));
+
+ xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
+ REG_READ, &r, 0, 0);
+
+ xe_device_mem_access_put(gt_to_xe(hwmon->gt));
+
+ x = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_X, r);
+ y = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_Y, r);
+ /*
+ * tau = 1.x * power(2,y), x = bits(23:22), y = bits(21:17)
+ * = (4 | x) << (y - 2)
+ * where (y - 2) ensures a 1.x fixed point representation of 1.x
+ * However because y can be < 2, we compute
+ * tau4 = (4 | x) << y
+ * but add 2 when doing the final right shift to account for units
+ */
+ tau4 = ((1 << x_w) | x) << y;
+ /* val in hwmon interface units (millisec) */
+ out = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w);
+
+ return sysfs_emit(buf, "%llu\n", out);
+}
+
+static ssize_t
+xe_hwmon_power1_max_interval_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct xe_hwmon *hwmon = dev_get_drvdata(dev);
+ u32 x, y, rxy, x_w = 2; /* 2 bits */
+ u64 tau4, r, max_win;
+ unsigned long val;
+ int ret;
+
+ ret = kstrtoul(buf, 0, &val);
+ if (ret)
+ return ret;
+
+ /*
+ * Max HW supported tau in '1.x * power(2,y)' format, x = 0, y = 0x12
+ * The hwmon->scl_shift_time default of 0xa results in a max tau of 256 seconds
+ */
+#define PKG_MAX_WIN_DEFAULT 0x12ull
+
+ /*
+ * val must be < max in hwmon interface units. The steps below are
+ * explained in xe_hwmon_power1_max_interval_show()
+ */
+ r = FIELD_PREP(PKG_MAX_WIN, PKG_MAX_WIN_DEFAULT);
+ x = REG_FIELD_GET(PKG_MAX_WIN_X, r);
+ y = REG_FIELD_GET(PKG_MAX_WIN_Y, r);
+ tau4 = ((1 << x_w) | x) << y;
+ max_win = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w);
+
+ if (val > max_win)
+ return -EINVAL;
+
+ /* val in hw units */
+ val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME);
+ /* Convert to 1.x * power(2,y) */
+ if (!val) {
+ /* Avoid ilog2(0) */
+ y = 0;
+ x = 0;
+ } else {
+ y = ilog2(val);
+ /* x = (val - (1 << y)) >> (y - 2); */
+ x = (val - (1ul << y)) << x_w >> y;
+ }
+
+ rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y);
+
+ xe_device_mem_access_get(gt_to_xe(hwmon->gt));
+
+ mutex_lock(&hwmon->hwmon_lock);
+
+ xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, (u32 *)&r,
+ PKG_PWR_LIM_1_TIME, rxy);
+
+ mutex_unlock(&hwmon->hwmon_lock);
+
+ xe_device_mem_access_put(gt_to_xe(hwmon->gt));
+
+ return count;
+}
+
+static SENSOR_DEVICE_ATTR(power1_max_interval, 0664,
+ xe_hwmon_power1_max_interval_show,
+ xe_hwmon_power1_max_interval_store, 0);
+
+static struct attribute *hwmon_attributes[] = {
+ &sensor_dev_attr_power1_max_interval.dev_attr.attr,
+ NULL
+};
+
+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);
+ u32 reg_val;
+ int ret = 0;
+
+ xe_device_mem_access_get(gt_to_xe(hwmon->gt));
+
+ if (attr == &sensor_dev_attr_power1_max_interval.dev_attr.attr)
+ ret = xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
+ REG_READ, ®_val, 0, 0) ? 0 : attr->mode;
+
+ xe_device_mem_access_put(gt_to_xe(hwmon->gt));
+
+ return ret;
+}
+
+static const struct attribute_group hwmon_attrgroup = {
+ .attrs = hwmon_attributes,
+ .is_visible = xe_hwmon_attributes_visible,
+};
+
+static const struct attribute_group *hwmon_groups[] = {
+ &hwmon_attrgroup,
+ NULL
+};
+
static const struct hwmon_channel_info *hwmon_info[] = {
HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
@@ -573,6 +707,7 @@ xe_hwmon_get_preregistration_info(struct xe_device *xe)
if (!ret) {
hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT, val_sku_unit);
+ hwmon->scl_shift_time = REG_FIELD_GET(PKG_TIME_UNIT, val_sku_unit);
}
/*
@@ -612,7 +747,8 @@ void xe_hwmon_register(struct xe_device *xe)
"xe",
hwmon,
&hwmon_chip_info,
- NULL);
+ hwmon_groups);
+
if (IS_ERR(hwmon->hwmon_dev)) {
drm_warn(&xe->drm, "Failed to register xe hwmon (%pe)\n", hwmon->hwmon_dev);
xe->hwmon = NULL;
--
2.25.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Intel-xe] [PATCH v5 6/6] drm/xe/hwmon: Expose power1_max_interval
2023-09-21 10:25 ` [PATCH v5 6/6] drm/xe/hwmon: Expose power1_max_interval Badal Nilawar
@ 2023-09-21 11:43 ` Ghimiray, Himal Prasad
2023-09-22 9:49 ` Nilawar, Badal
0 siblings, 1 reply; 24+ messages in thread
From: Ghimiray, Himal Prasad @ 2023-09-21 11:43 UTC (permalink / raw)
To: Badal Nilawar, intel-xe, linux-hwmon; +Cc: linux, rodrigo.vivi
On 21-09-2023 15:55, Badal Nilawar wrote:
> Expose power1_max_interval, that is the tau corresponding to PL1, as a
> custom hwmon attribute. Some bit manipulation is needed because of the
> format of PKG_PWR_LIM_1_TIME in
> PACKAGE_RAPL_LIMIT register (1.x * power(2,y))
>
> v2: Get rpm wake ref while accessing power1_max_interval
> v3: %s/hwmon/xe_hwmon/
>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
> .../ABI/testing/sysfs-driver-intel-xe-hwmon | 11 ++
> drivers/gpu/drm/xe/regs/xe_mchbar_regs.h | 8 +
> drivers/gpu/drm/xe/xe_hwmon.c | 138 +++++++++++++++++-
> 3 files changed, 156 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> index 1a7a6c23e141..9ceb9c04b52b 100644
> --- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> @@ -59,3 +59,14 @@ Contact: intel-xe@lists.freedesktop.org
> Description: RO. Energy input of device in microjoules.
>
> Only supported for particular Intel xe graphics platforms.
> +
> +What: /sys/devices/.../hwmon/hwmon<i>/power1_max_interval
> +Date: September 2023
> +KernelVersion: 6.5
> +Contact: intel-xe@lists.freedesktop.org
> +Description: RW. Sustained power limit interval (Tau in PL1/Tau) in
> + milliseconds over which sustained power is averaged.
> +
> + Only supported for particular Intel xe graphics platforms.
> +
> +
> diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> index d8ecbe1858d1..519dd1067a19 100644
> --- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> @@ -22,15 +22,23 @@
> #define PKG_TDP GENMASK_ULL(14, 0)
> #define PKG_MIN_PWR GENMASK_ULL(30, 16)
> #define PKG_MAX_PWR GENMASK_ULL(46, 32)
> +#define PKG_MAX_WIN GENMASK_ULL(54, 48)
> +#define PKG_MAX_WIN_X GENMASK_ULL(54, 53)
> +#define PKG_MAX_WIN_Y GENMASK_ULL(52, 48)
> +
>
> #define PCU_CR_PACKAGE_POWER_SKU_UNIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
> #define PKG_PWR_UNIT REG_GENMASK(3, 0)
> #define PKG_ENERGY_UNIT REG_GENMASK(12, 8)
> +#define PKG_TIME_UNIT REG_GENMASK(19, 16)
>
> #define PCU_CR_PACKAGE_ENERGY_STATUS XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x593c)
>
> #define PCU_CR_PACKAGE_RAPL_LIMIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
> #define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
> #define PKG_PWR_LIM_1_EN REG_BIT(15)
> +#define PKG_PWR_LIM_1_TIME REG_GENMASK(23, 17)
> +#define PKG_PWR_LIM_1_TIME_X REG_GENMASK(23, 22)
> +#define PKG_PWR_LIM_1_TIME_Y REG_GENMASK(21, 17)
>
> #endif /* _XE_MCHBAR_REGS_H_ */
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index cb75b9a386c0..dfa638942d47 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -38,6 +38,7 @@ enum xe_hwmon_reg_operation {
> #define SF_CURR 1000 /* milliamperes */
> #define SF_VOLTAGE 1000 /* millivolts */
> #define SF_ENERGY 1000000 /* microjoules */
> +#define SF_TIME 1000 /* milliseconds */
>
> struct hwmon_energy_info {
> u32 reg_val_prev;
> @@ -50,6 +51,7 @@ struct xe_hwmon {
> struct mutex hwmon_lock; /* rmw operations*/
> int scl_shift_power;
> int scl_shift_energy;
> + int scl_shift_time;
> struct hwmon_energy_info ei; /* Energy info for energy1_input */
> };
>
> @@ -255,6 +257,138 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, long *energy)
> xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> }
>
> +static ssize_t
> +xe_hwmon_power1_max_interval_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct xe_hwmon *hwmon = dev_get_drvdata(dev);
> + u32 r, x, y, x_w = 2; /* 2 bits */
> + u64 tau4, out;
> +
> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> +
> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
> + REG_READ, &r, 0, 0);
> +
> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> +
> + x = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_X, r);
> + y = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_Y, r);
> + /*
> + * tau = 1.x * power(2,y), x = bits(23:22), y = bits(21:17)
> + * = (4 | x) << (y - 2)
> + * where (y - 2) ensures a 1.x fixed point representation of 1.x
> + * However because y can be < 2, we compute
> + * tau4 = (4 | x) << y
> + * but add 2 when doing the final right shift to account for units
> + */
> + tau4 = ((1 << x_w) | x) << y;
> + /* val in hwmon interface units (millisec) */
> + out = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w);
> +
> + return sysfs_emit(buf, "%llu\n", out);
> +}
> +
> +static ssize_t
> +xe_hwmon_power1_max_interval_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct xe_hwmon *hwmon = dev_get_drvdata(dev);
> + u32 x, y, rxy, x_w = 2; /* 2 bits */
> + u64 tau4, r, max_win;
> + unsigned long val;
> + int ret;
> +
> + ret = kstrtoul(buf, 0, &val);
> + if (ret)
> + return ret;
> +
> + /*
> + * Max HW supported tau in '1.x * power(2,y)' format, x = 0, y = 0x12
> + * The hwmon->scl_shift_time default of 0xa results in a max tau of 256 seconds
> + */
> +#define PKG_MAX_WIN_DEFAULT 0x12ull
Do we need to determine r, x, y etc for max limit ?
Why cant we simply define MAX_LIMIT 256 ?
BR
Himal
> +
> + /*
> + * val must be < max in hwmon interface units. The steps below are
> + * explained in xe_hwmon_power1_max_interval_show()
> + */
> + r = FIELD_PREP(PKG_MAX_WIN, PKG_MAX_WIN_DEFAULT);
> + x = REG_FIELD_GET(PKG_MAX_WIN_X, r);
> + y = REG_FIELD_GET(PKG_MAX_WIN_Y, r);
> + tau4 = ((1 << x_w) | x) << y;
> + max_win = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w);
> +
> + if (val > max_win)
> + return -EINVAL;
> +
> + /* val in hw units */
> + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME);
> + /* Convert to 1.x * power(2,y) */
> + if (!val) {
> + /* Avoid ilog2(0) */
> + y = 0;
> + x = 0;
> + } else {
> + y = ilog2(val);
> + /* x = (val - (1 << y)) >> (y - 2); */
> + x = (val - (1ul << y)) << x_w >> y;
> + }
> +
> + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y);
> +
> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> +
> + mutex_lock(&hwmon->hwmon_lock);
> +
> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, (u32 *)&r,
> + PKG_PWR_LIM_1_TIME, rxy);
> +
> + mutex_unlock(&hwmon->hwmon_lock);
> +
> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> +
> + return count;
> +}
> +
> +static SENSOR_DEVICE_ATTR(power1_max_interval, 0664,
> + xe_hwmon_power1_max_interval_show,
> + xe_hwmon_power1_max_interval_store, 0);
> +
> +static struct attribute *hwmon_attributes[] = {
> + &sensor_dev_attr_power1_max_interval.dev_attr.attr,
> + NULL
> +};
> +
> +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);
> + u32 reg_val;
> + int ret = 0;
> +
> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> +
> + if (attr == &sensor_dev_attr_power1_max_interval.dev_attr.attr)
> + ret = xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
> + REG_READ, ®_val, 0, 0) ? 0 : attr->mode;
> +
> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> +
> + return ret;
> +}
> +
> +static const struct attribute_group hwmon_attrgroup = {
> + .attrs = hwmon_attributes,
> + .is_visible = xe_hwmon_attributes_visible,
> +};
> +
> +static const struct attribute_group *hwmon_groups[] = {
> + &hwmon_attrgroup,
> + NULL
> +};
> +
> static const struct hwmon_channel_info *hwmon_info[] = {
> HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
> HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
> @@ -573,6 +707,7 @@ xe_hwmon_get_preregistration_info(struct xe_device *xe)
> if (!ret) {
> hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);
> hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT, val_sku_unit);
> + hwmon->scl_shift_time = REG_FIELD_GET(PKG_TIME_UNIT, val_sku_unit);
> }
>
> /*
> @@ -612,7 +747,8 @@ void xe_hwmon_register(struct xe_device *xe)
> "xe",
> hwmon,
> &hwmon_chip_info,
> - NULL);
> + hwmon_groups);
> +
> if (IS_ERR(hwmon->hwmon_dev)) {
> drm_warn(&xe->drm, "Failed to register xe hwmon (%pe)\n", hwmon->hwmon_dev);
> xe->hwmon = NULL;
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Intel-xe] [PATCH v5 6/6] drm/xe/hwmon: Expose power1_max_interval
2023-09-21 11:43 ` [Intel-xe] " Ghimiray, Himal Prasad
@ 2023-09-22 9:49 ` Nilawar, Badal
2023-09-22 12:43 ` Ghimiray, Himal Prasad
0 siblings, 1 reply; 24+ messages in thread
From: Nilawar, Badal @ 2023-09-22 9:49 UTC (permalink / raw)
To: Ghimiray, Himal Prasad, intel-xe, linux-hwmon; +Cc: linux, rodrigo.vivi
On 21-09-2023 17:13, Ghimiray, Himal Prasad wrote:
>
> On 21-09-2023 15:55, Badal Nilawar wrote:
>> Expose power1_max_interval, that is the tau corresponding to PL1, as a
>> custom hwmon attribute. Some bit manipulation is needed because of the
>> format of PKG_PWR_LIM_1_TIME in
>> PACKAGE_RAPL_LIMIT register (1.x * power(2,y))
>>
>> v2: Get rpm wake ref while accessing power1_max_interval
>> v3: %s/hwmon/xe_hwmon/
>>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> ---
>> .../ABI/testing/sysfs-driver-intel-xe-hwmon | 11 ++
>> drivers/gpu/drm/xe/regs/xe_mchbar_regs.h | 8 +
>> drivers/gpu/drm/xe/xe_hwmon.c | 138 +++++++++++++++++-
>> 3 files changed, 156 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
>> b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
>> index 1a7a6c23e141..9ceb9c04b52b 100644
>> --- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
>> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
>> @@ -59,3 +59,14 @@ Contact: intel-xe@lists.freedesktop.org
>> Description: RO. Energy input of device in microjoules.
>> Only supported for particular Intel xe graphics platforms.
>> +
>> +What: /sys/devices/.../hwmon/hwmon<i>/power1_max_interval
>> +Date: September 2023
>> +KernelVersion: 6.5
>> +Contact: intel-xe@lists.freedesktop.org
>> +Description: RW. Sustained power limit interval (Tau in PL1/Tau) in
>> + milliseconds over which sustained power is averaged.
>> +
>> + Only supported for particular Intel xe graphics platforms.
>> +
>> +
>> diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> index d8ecbe1858d1..519dd1067a19 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
>> @@ -22,15 +22,23 @@
>> #define PKG_TDP GENMASK_ULL(14, 0)
>> #define PKG_MIN_PWR GENMASK_ULL(30, 16)
>> #define PKG_MAX_PWR GENMASK_ULL(46, 32)
>> +#define PKG_MAX_WIN GENMASK_ULL(54, 48)
>> +#define PKG_MAX_WIN_X GENMASK_ULL(54, 53)
>> +#define PKG_MAX_WIN_Y GENMASK_ULL(52, 48)
>> +
>> #define PCU_CR_PACKAGE_POWER_SKU_UNIT
>> XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
>> #define PKG_PWR_UNIT REG_GENMASK(3, 0)
>> #define PKG_ENERGY_UNIT REG_GENMASK(12, 8)
>> +#define PKG_TIME_UNIT REG_GENMASK(19, 16)
>> #define PCU_CR_PACKAGE_ENERGY_STATUS
>> XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x593c)
>> #define PCU_CR_PACKAGE_RAPL_LIMIT
>> XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
>> #define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
>> #define PKG_PWR_LIM_1_EN REG_BIT(15)
>> +#define PKG_PWR_LIM_1_TIME REG_GENMASK(23, 17)
>> +#define PKG_PWR_LIM_1_TIME_X REG_GENMASK(23, 22)
>> +#define PKG_PWR_LIM_1_TIME_Y REG_GENMASK(21, 17)
>> #endif /* _XE_MCHBAR_REGS_H_ */
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c
>> b/drivers/gpu/drm/xe/xe_hwmon.c
>> index cb75b9a386c0..dfa638942d47 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -38,6 +38,7 @@ enum xe_hwmon_reg_operation {
>> #define SF_CURR 1000 /* milliamperes */
>> #define SF_VOLTAGE 1000 /* millivolts */
>> #define SF_ENERGY 1000000 /* microjoules */
>> +#define SF_TIME 1000 /* milliseconds */
>> struct hwmon_energy_info {
>> u32 reg_val_prev;
>> @@ -50,6 +51,7 @@ struct xe_hwmon {
>> struct mutex hwmon_lock; /* rmw operations*/
>> int scl_shift_power;
>> int scl_shift_energy;
>> + int scl_shift_time;
>> struct hwmon_energy_info ei; /* Energy info for
>> energy1_input */
>> };
>> @@ -255,6 +257,138 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, long
>> *energy)
>> xe_device_mem_access_put(gt_to_xe(hwmon->gt));
>> }
>> +static ssize_t
>> +xe_hwmon_power1_max_interval_show(struct device *dev, struct
>> device_attribute *attr,
>> + char *buf)
>> +{
>> + struct xe_hwmon *hwmon = dev_get_drvdata(dev);
>> + u32 r, x, y, x_w = 2; /* 2 bits */
>> + u64 tau4, out;
>> +
>> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
>> +
>> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> + REG_READ, &r, 0, 0);
>> +
>> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
>> +
>> + x = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_X, r);
>> + y = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_Y, r);
>> + /*
>> + * tau = 1.x * power(2,y), x = bits(23:22), y = bits(21:17)
>> + * = (4 | x) << (y - 2)
>> + * where (y - 2) ensures a 1.x fixed point representation of 1.x
>> + * However because y can be < 2, we compute
>> + * tau4 = (4 | x) << y
>> + * but add 2 when doing the final right shift to account for units
>> + */
>> + tau4 = ((1 << x_w) | x) << y;
>> + /* val in hwmon interface units (millisec) */
>> + out = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w);
>> +
>> + return sysfs_emit(buf, "%llu\n", out);
>> +}
>> +
>> +static ssize_t
>> +xe_hwmon_power1_max_interval_store(struct device *dev, struct
>> device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct xe_hwmon *hwmon = dev_get_drvdata(dev);
>> + u32 x, y, rxy, x_w = 2; /* 2 bits */
>> + u64 tau4, r, max_win;
>> + unsigned long val;
>> + int ret;
>> +
>> + ret = kstrtoul(buf, 0, &val);
>> + if (ret)
>> + return ret;
>> +
>> + /*
>> + * Max HW supported tau in '1.x * power(2,y)' format, x = 0, y =
>> 0x12
>> + * The hwmon->scl_shift_time default of 0xa results in a max tau
>> of 256 seconds
>> + */
>> +#define PKG_MAX_WIN_DEFAULT 0x12ull
>
> Do we need to determine r, x, y etc for max limit ?
>
> Why cant we simply define MAX_LIMIT 256 ?
PKG_MAX_WIN is field of PKG_PWR_SKU but it is observed that for some
platforms it is invalid and for some not populated. So fixed the max
limit. For future platforms if PKG_MAX_WIN is valid then it is prefered
to read from register, in that case below equation is needed to
calculate max limit.
Regards,
Badal
>
> BR
>
> Himal
>
>> +
>> + /*
>> + * val must be < max in hwmon interface units. The steps below are
>> + * explained in xe_hwmon_power1_max_interval_show()
>> + */
>> + r = FIELD_PREP(PKG_MAX_WIN, PKG_MAX_WIN_DEFAULT);
>> + x = REG_FIELD_GET(PKG_MAX_WIN_X, r);
>> + y = REG_FIELD_GET(PKG_MAX_WIN_Y, r);
>> + tau4 = ((1 << x_w) | x) << y;
>> + max_win = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time +
>> x_w);
>> +
>> + if (val > max_win)
>> + return -EINVAL;
>> +
>> + /* val in hw units */
>> + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time,
>> SF_TIME);
>> + /* Convert to 1.x * power(2,y) */
>> + if (!val) {
>> + /* Avoid ilog2(0) */
>> + y = 0;
>> + x = 0;
>> + } else {
>> + y = ilog2(val);
>> + /* x = (val - (1 << y)) >> (y - 2); */
>> + x = (val - (1ul << y)) << x_w >> y;
>> + }
>> +
>> + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) |
>> REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y);
>> +
>> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
>> +
>> + mutex_lock(&hwmon->hwmon_lock);
>> +
>> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, (u32 *)&r,
>> + PKG_PWR_LIM_1_TIME, rxy);
>> +
>> + mutex_unlock(&hwmon->hwmon_lock);
>> +
>> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
>> +
>> + return count;
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(power1_max_interval, 0664,
>> + xe_hwmon_power1_max_interval_show,
>> + xe_hwmon_power1_max_interval_store, 0);
>> +
>> +static struct attribute *hwmon_attributes[] = {
>> + &sensor_dev_attr_power1_max_interval.dev_attr.attr,
>> + NULL
>> +};
>> +
>> +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);
>> + u32 reg_val;
>> + int ret = 0;
>> +
>> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
>> +
>> + if (attr == &sensor_dev_attr_power1_max_interval.dev_attr.attr)
>> + ret = xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> + REG_READ, ®_val, 0, 0) ? 0 : attr->mode;
>> +
>> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
>> +
>> + return ret;
>> +}
>> +
>> +static const struct attribute_group hwmon_attrgroup = {
>> + .attrs = hwmon_attributes,
>> + .is_visible = xe_hwmon_attributes_visible,
>> +};
>> +
>> +static const struct attribute_group *hwmon_groups[] = {
>> + &hwmon_attrgroup,
>> + NULL
>> +};
>> +
>> static const struct hwmon_channel_info *hwmon_info[] = {
>> HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX |
>> HWMON_P_CRIT),
>> HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT),
>> @@ -573,6 +707,7 @@ xe_hwmon_get_preregistration_info(struct xe_device
>> *xe)
>> if (!ret) {
>> hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT,
>> val_sku_unit);
>> hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT,
>> val_sku_unit);
>> + hwmon->scl_shift_time = REG_FIELD_GET(PKG_TIME_UNIT,
>> val_sku_unit);
>> }
>> /*
>> @@ -612,7 +747,8 @@ void xe_hwmon_register(struct xe_device *xe)
>> "xe",
>> hwmon,
>> &hwmon_chip_info,
>> - NULL);
>> + hwmon_groups);
>> +
>> if (IS_ERR(hwmon->hwmon_dev)) {
>> drm_warn(&xe->drm, "Failed to register xe hwmon (%pe)\n",
>> hwmon->hwmon_dev);
>> xe->hwmon = NULL;
^ permalink raw reply [flat|nested] 24+ messages in thread* RE: [Intel-xe] [PATCH v5 6/6] drm/xe/hwmon: Expose power1_max_interval
2023-09-22 9:49 ` Nilawar, Badal
@ 2023-09-22 12:43 ` Ghimiray, Himal Prasad
0 siblings, 0 replies; 24+ messages in thread
From: Ghimiray, Himal Prasad @ 2023-09-22 12:43 UTC (permalink / raw)
To: Nilawar, Badal, intel-xe@lists.freedesktop.org,
linux-hwmon@vger.kernel.org
Cc: linux@roeck-us.net, Vivi, Rodrigo
> -----Original Message-----
> From: Nilawar, Badal <badal.nilawar@intel.com>
> Sent: 22 September 2023 15:19
> To: Ghimiray, Himal Prasad <himal.prasad.ghimiray@intel.com>; intel-
> xe@lists.freedesktop.org; linux-hwmon@vger.kernel.org
> Cc: linux@roeck-us.net; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Subject: Re: [Intel-xe] [PATCH v5 6/6] drm/xe/hwmon: Expose
> power1_max_interval
>
>
>
> On 21-09-2023 17:13, Ghimiray, Himal Prasad wrote:
> >
> > On 21-09-2023 15:55, Badal Nilawar wrote:
> >> Expose power1_max_interval, that is the tau corresponding to PL1, as
> >> a custom hwmon attribute. Some bit manipulation is needed because of
> >> the format of PKG_PWR_LIM_1_TIME in PACKAGE_RAPL_LIMIT register
> (1.x
> >> * power(2,y))
> >>
> >> v2: Get rpm wake ref while accessing power1_max_interval
> >> v3: %s/hwmon/xe_hwmon/
> >>
> >> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> >> ---
> >> .../ABI/testing/sysfs-driver-intel-xe-hwmon | 11 ++
> >> drivers/gpu/drm/xe/regs/xe_mchbar_regs.h | 8 +
> >> drivers/gpu/drm/xe/xe_hwmon.c | 138
> >> +++++++++++++++++-
> >> 3 files changed, 156 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> >> b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> >> index 1a7a6c23e141..9ceb9c04b52b 100644
> >> --- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> >> +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> >> @@ -59,3 +59,14 @@ Contact: intel-xe@lists.freedesktop.org
> >> Description: RO. Energy input of device in microjoules.
> >> Only supported for particular Intel xe graphics platforms.
> >> +
> >> +What: /sys/devices/.../hwmon/hwmon<i>/power1_max_interval
> >> +Date: September 2023
> >> +KernelVersion: 6.5
> >> +Contact: intel-xe@lists.freedesktop.org
> >> +Description: RW. Sustained power limit interval (Tau in PL1/Tau)
> >> +in
> >> + milliseconds over which sustained power is averaged.
> >> +
> >> + Only supported for particular Intel xe graphics platforms.
> >> +
> >> +
> >> diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> >> b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> >> index d8ecbe1858d1..519dd1067a19 100644
> >> --- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> >> +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> >> @@ -22,15 +22,23 @@
> >> #define PKG_TDP GENMASK_ULL(14, 0)
> >> #define PKG_MIN_PWR GENMASK_ULL(30, 16)
> >> #define PKG_MAX_PWR GENMASK_ULL(46, 32)
> >> +#define PKG_MAX_WIN GENMASK_ULL(54, 48) #define
> >> +PKG_MAX_WIN_X GENMASK_ULL(54, 53) #define
> >> +PKG_MAX_WIN_Y GENMASK_ULL(52, 48)
> >> +
> >> #define PCU_CR_PACKAGE_POWER_SKU_UNIT
> >> XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938)
> >> #define PKG_PWR_UNIT REG_GENMASK(3, 0)
> >> #define PKG_ENERGY_UNIT REG_GENMASK(12, 8)
> >> +#define PKG_TIME_UNIT REG_GENMASK(19, 16)
> >> #define PCU_CR_PACKAGE_ENERGY_STATUS
> >> XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x593c)
> >> #define PCU_CR_PACKAGE_RAPL_LIMIT
> >> XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
> >> #define PKG_PWR_LIM_1 REG_GENMASK(14, 0)
> >> #define PKG_PWR_LIM_1_EN REG_BIT(15)
> >> +#define PKG_PWR_LIM_1_TIME REG_GENMASK(23, 17) #define
> >> +PKG_PWR_LIM_1_TIME_X REG_GENMASK(23, 22) #define
> >> +PKG_PWR_LIM_1_TIME_Y REG_GENMASK(21, 17)
> >> #endif /* _XE_MCHBAR_REGS_H_ */
> >> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c
> >> b/drivers/gpu/drm/xe/xe_hwmon.c index cb75b9a386c0..dfa638942d47
> >> 100644
> >> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> >> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> >> @@ -38,6 +38,7 @@ enum xe_hwmon_reg_operation {
> >> #define SF_CURR 1000 /* milliamperes */
> >> #define SF_VOLTAGE 1000 /* millivolts */
> >> #define SF_ENERGY 1000000 /* microjoules */
> >> +#define SF_TIME 1000 /* milliseconds */
> >> struct hwmon_energy_info {
> >> u32 reg_val_prev;
> >> @@ -50,6 +51,7 @@ struct xe_hwmon {
> >> struct mutex hwmon_lock; /* rmw operations*/
> >> int scl_shift_power;
> >> int scl_shift_energy;
> >> + int scl_shift_time;
> >> struct hwmon_energy_info ei; /* Energy info for
> >> energy1_input */
> >> };
> >> @@ -255,6 +257,138 @@ xe_hwmon_energy_get(struct xe_hwmon
> *hwmon,
> >> long
> >> *energy)
> >> xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> >> }
> >> +static ssize_t
> >> +xe_hwmon_power1_max_interval_show(struct device *dev, struct
> >> device_attribute *attr,
> >> + char *buf)
> >> +{
> >> + struct xe_hwmon *hwmon = dev_get_drvdata(dev);
> >> + u32 r, x, y, x_w = 2; /* 2 bits */
> >> + u64 tau4, out;
> >> +
> >> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> >> +
> >> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
> >> + REG_READ, &r, 0, 0);
> >> +
> >> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> >> +
> >> + x = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_X, r);
> >> + y = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_Y, r);
> >> + /*
> >> + * tau = 1.x * power(2,y), x = bits(23:22), y = bits(21:17)
> >> + * = (4 | x) << (y - 2)
> >> + * where (y - 2) ensures a 1.x fixed point representation of 1.x
> >> + * However because y can be < 2, we compute
> >> + * tau4 = (4 | x) << y
> >> + * but add 2 when doing the final right shift to account for
> >> +units
> >> + */
> >> + tau4 = ((1 << x_w) | x) << y;
> >> + /* val in hwmon interface units (millisec) */
> >> + out = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time +
> >> +x_w);
> >> +
> >> + return sysfs_emit(buf, "%llu\n", out); }
> >> +
> >> +static ssize_t
> >> +xe_hwmon_power1_max_interval_store(struct device *dev, struct
> >> device_attribute *attr,
> >> + const char *buf, size_t count) {
> >> + struct xe_hwmon *hwmon = dev_get_drvdata(dev);
> >> + u32 x, y, rxy, x_w = 2; /* 2 bits */
> >> + u64 tau4, r, max_win;
> >> + unsigned long val;
> >> + int ret;
> >> +
> >> + ret = kstrtoul(buf, 0, &val);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /*
> >> + * Max HW supported tau in '1.x * power(2,y)' format, x = 0, y =
> >> 0x12
> >> + * The hwmon->scl_shift_time default of 0xa results in a max tau
> >> of 256 seconds
> >> + */
> >> +#define PKG_MAX_WIN_DEFAULT 0x12ull
> >
> > Do we need to determine r, x, y etc for max limit ?
> >
> > Why cant we simply define MAX_LIMIT 256 ?
> PKG_MAX_WIN is field of PKG_PWR_SKU but it is observed that for some
> platforms it is invalid and for some not populated. So fixed the max limit. For
> future platforms if PKG_MAX_WIN is valid then it is prefered to read from
> register, in that case below equation is needed to calculate max limit.
That makes sense. LGTM
Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>
> Regards,
> Badal
> >
> > BR
> >
> > Himal
> >
> >> +
> >> + /*
> >> + * val must be < max in hwmon interface units. The steps below
> >> +are
> >> + * explained in xe_hwmon_power1_max_interval_show()
> >> + */
> >> + r = FIELD_PREP(PKG_MAX_WIN, PKG_MAX_WIN_DEFAULT);
> >> + x = REG_FIELD_GET(PKG_MAX_WIN_X, r);
> >> + y = REG_FIELD_GET(PKG_MAX_WIN_Y, r);
> >> + tau4 = ((1 << x_w) | x) << y;
> >> + max_win = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time +
> >> x_w);
> >> +
> >> + if (val > max_win)
> >> + return -EINVAL;
> >> +
> >> + /* val in hw units */
> >> + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time,
> >> SF_TIME);
> >> + /* Convert to 1.x * power(2,y) */
> >> + if (!val) {
> >> + /* Avoid ilog2(0) */
> >> + y = 0;
> >> + x = 0;
> >> + } else {
> >> + y = ilog2(val);
> >> + /* x = (val - (1 << y)) >> (y - 2); */
> >> + x = (val - (1ul << y)) << x_w >> y;
> >> + }
> >> +
> >> + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) |
> >> REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y);
> >> +
> >> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> >> +
> >> + mutex_lock(&hwmon->hwmon_lock);
> >> +
> >> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW,
> (u32
> >> +*)&r,
> >> + PKG_PWR_LIM_1_TIME, rxy);
> >> +
> >> + mutex_unlock(&hwmon->hwmon_lock);
> >> +
> >> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> >> +
> >> + return count;
> >> +}
> >> +
> >> +static SENSOR_DEVICE_ATTR(power1_max_interval, 0664,
> >> + xe_hwmon_power1_max_interval_show,
> >> + xe_hwmon_power1_max_interval_store, 0);
> >> +
> >> +static struct attribute *hwmon_attributes[] = {
> >> + &sensor_dev_attr_power1_max_interval.dev_attr.attr,
> >> + NULL
> >> +};
> >> +
> >> +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);
> >> + u32 reg_val;
> >> + int ret = 0;
> >> +
> >> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> >> +
> >> + if (attr == &sensor_dev_attr_power1_max_interval.dev_attr.attr)
> >> + ret = xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
> >> + REG_READ, ®_val, 0, 0) ? 0 : attr->mode;
> >> +
> >> + xe_device_mem_access_put(gt_to_xe(hwmon->gt));
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static const struct attribute_group hwmon_attrgroup = {
> >> + .attrs = hwmon_attributes,
> >> + .is_visible = xe_hwmon_attributes_visible, };
> >> +
> >> +static const struct attribute_group *hwmon_groups[] = {
> >> + &hwmon_attrgroup,
> >> + NULL
> >> +};
> >> +
> >> static const struct hwmon_channel_info *hwmon_info[] = {
> >> HWMON_CHANNEL_INFO(power, HWMON_P_MAX |
> HWMON_P_RATED_MAX |
> >> HWMON_P_CRIT),
> >> HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT), @@ -573,6 +707,7
> @@
> >> xe_hwmon_get_preregistration_info(struct xe_device
> >> *xe)
> >> if (!ret) {
> >> hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT,
> >> val_sku_unit);
> >> hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT,
> >> val_sku_unit);
> >> + hwmon->scl_shift_time = REG_FIELD_GET(PKG_TIME_UNIT,
> >> val_sku_unit);
> >> }
> >> /*
> >> @@ -612,7 +747,8 @@ void xe_hwmon_register(struct xe_device *xe)
> >> "xe",
> >> hwmon,
> >> &hwmon_chip_info,
> >> - NULL);
> >> + hwmon_groups);
> >> +
> >> if (IS_ERR(hwmon->hwmon_dev)) {
> >> drm_warn(&xe->drm, "Failed to register xe hwmon (%pe)\n",
> >> hwmon->hwmon_dev);
> >> xe->hwmon = NULL;
^ permalink raw reply [flat|nested] 24+ messages in thread