mbox series

[v3,0/7] thermal: processor_thermal: Suport workload hint

Message ID 20230829002346.2104251-1-srinivas.pandruvada@linux.intel.com
Headers show
Series thermal: processor_thermal: Suport workload hint | expand

Message

Srinivas Pandruvada Aug. 29, 2023, 12:23 a.m. UTC
Add support for Meteor Lake workload hints. Before adding this support,
some reorganization and clean up is required.
First four changes are for clean up and to reorganize code to add
support for workload hint. The last patch adds a test program as part
of self tests.

v3:
Changes in the commit log
Rename of files for using WT instead of WLT
Address comments from Rafael on v2

v2:
Changes in comments and commit log
Self test program is improved to disable workloadtype notification
on exit

Srinivas Pandruvada (7):
  thermal: int340x: processor_thermal: Move mailbox code to common
    module
  thermal: int340x: processor_thermal: Add interrupt configuration
  thermal: int340x: processor_thermal: Use non MSI interrupts by default
  thermal/drivers/int340x: Remove PROC_THERMAL_FEATURE_WLT_REQ for
    Meteor Lake
  thermal: int340x: processor_thermal: Add workload type hint interface
  thermal/drivers/int340x: Support workload hint interrupts
  selftests/thermel/intel: Add test to read workload hint

 .../driver-api/thermal/intel_dptf.rst         |  51 ++++
 .../thermal/intel/int340x_thermal/Makefile    |   2 +
 .../processor_thermal_device.c                |  17 +-
 .../processor_thermal_device.h                |  21 +-
 .../processor_thermal_device_pci.c            |  79 ++++--
 .../processor_thermal_device_pci_legacy.c     |   3 +-
 .../int340x_thermal/processor_thermal_mbox.c  | 179 ++++---------
 .../processor_thermal_wt_hint.c               | 252 ++++++++++++++++++
 .../processor_thermal_wt_req.c                | 136 ++++++++++
 tools/testing/selftests/Makefile              |   1 +
 .../thermal/intel/workload_hint/Makefile      |  12 +
 .../intel/workload_hint/workload_hint_test.c  | 157 +++++++++++
 12 files changed, 752 insertions(+), 158 deletions(-)
 create mode 100644 drivers/thermal/intel/int340x_thermal/processor_thermal_wt_hint.c
 create mode 100644 drivers/thermal/intel/int340x_thermal/processor_thermal_wt_req.c
 create mode 100644 tools/testing/selftests/thermal/intel/workload_hint/Makefile
 create mode 100644 tools/testing/selftests/thermal/intel/workload_hint/workload_hint_test.c

Comments

Rafael J. Wysocki Sept. 12, 2023, 2:09 p.m. UTC | #1
On Tue, Aug 29, 2023 at 2:23 AM Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> Add support for Meteor Lake workload hints. Before adding this support,
> some reorganization and clean up is required.
> First four changes are for clean up and to reorganize code to add
> support for workload hint. The last patch adds a test program as part
> of self tests.
>
> v3:
> Changes in the commit log
> Rename of files for using WT instead of WLT
> Address comments from Rafael on v2
>
> v2:
> Changes in comments and commit log
> Self test program is improved to disable workloadtype notification
> on exit
>
> Srinivas Pandruvada (7):
>   thermal: int340x: processor_thermal: Move mailbox code to common
>     module
>   thermal: int340x: processor_thermal: Add interrupt configuration
>   thermal: int340x: processor_thermal: Use non MSI interrupts by default
>   thermal/drivers/int340x: Remove PROC_THERMAL_FEATURE_WLT_REQ for
>     Meteor Lake
>   thermal: int340x: processor_thermal: Add workload type hint interface
>   thermal/drivers/int340x: Support workload hint interrupts
>   selftests/thermel/intel: Add test to read workload hint
>
>  .../driver-api/thermal/intel_dptf.rst         |  51 ++++
>  .../thermal/intel/int340x_thermal/Makefile    |   2 +
>  .../processor_thermal_device.c                |  17 +-
>  .../processor_thermal_device.h                |  21 +-
>  .../processor_thermal_device_pci.c            |  79 ++++--
>  .../processor_thermal_device_pci_legacy.c     |   3 +-
>  .../int340x_thermal/processor_thermal_mbox.c  | 179 ++++---------
>  .../processor_thermal_wt_hint.c               | 252 ++++++++++++++++++
>  .../processor_thermal_wt_req.c                | 136 ++++++++++
>  tools/testing/selftests/Makefile              |   1 +
>  .../thermal/intel/workload_hint/Makefile      |  12 +
>  .../intel/workload_hint/workload_hint_test.c  | 157 +++++++++++
>  12 files changed, 752 insertions(+), 158 deletions(-)
>  create mode 100644 drivers/thermal/intel/int340x_thermal/processor_thermal_wt_hint.c
>  create mode 100644 drivers/thermal/intel/int340x_thermal/processor_thermal_wt_req.c
>  create mode 100644 tools/testing/selftests/thermal/intel/workload_hint/Makefile
>  create mode 100644 tools/testing/selftests/thermal/intel/workload_hint/workload_hint_test.c
>
> --

There is a slight issue with the patch ordering in this series,
because the interface to enable the interrupt should only be provided
after implementing the interrupt handlers.  I don't think that anyone
will apply the series partially and try to enable the feature, though.

Also, I'm not actually sure if proc_thermal_wt_intr_callback() can run
safely against the work item scheduled in proc_thermal_irq_handler()
in case the workload hint one triggers along with a thermal threshold
one.  I think that the access to MMIO is cached, so what if they both
try to update the same cache line at the same time?  Or are they
guaranteed to be different cache lines?

Anyway, tentatively applied as 6.7 material, but I've changed the
second patch somewhat, because I couldn't convince myself that the
implicit type conversions in processor_thermal_mbox_interrupt_config()
would always do the right thing regardless of the numbers involved, so
please check the result in my bleeding-edge branch.

Thanks!
Srinivas Pandruvada Sept. 12, 2023, 7:44 p.m. UTC | #2
Hi Rafael,

On Tue, 2023-09-12 at 16:09 +0200, Rafael J. Wysocki wrote:
> On Tue, Aug 29, 2023 at 2:23 AM Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > 

[...]

> > --
> 
> There is a slight issue with the patch ordering in this series,
> because the interface to enable the interrupt should only be provided
> after implementing the interrupt handlers.  I don't think that anyone
> will apply the series partially and try to enable the feature,
> though.
Thanks!

> 
> Also, I'm not actually sure if proc_thermal_wt_intr_callback() can
> run
> safely against the work item scheduled in proc_thermal_irq_handler()
> in case the workload hint one triggers along with a thermal threshold
> one.  I think that the access to MMIO is cached, so what if they both
> try to update the same cache line at the same time?  Or are they
> guaranteed to be different cache lines?
These two registers are 90 cache lines apart. Looking at all the
registers on this bar for status offsets, they are several cache lines
apart. Also this bar is non prefetchable, so continuous data can't be
fetched ahead.


> 
> Anyway, tentatively applied as 6.7 material, but I've changed the
> second patch somewhat, because I couldn't convince myself that the
> implicit type conversions in
> processor_thermal_mbox_interrupt_config()
> would always do the right thing regardless of the numbers involved,
> so
> please check the result in my bleeding-edge branch.
> 
If I diff, there is only one change in processor_thermal_mbox.c. Tested
that change and works fine.

Thanks,
Srinivas


> Thanks!
Rafael J. Wysocki Sept. 13, 2023, 8:15 a.m. UTC | #3
On Tue, Sep 12, 2023 at 9:44 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> Hi Rafael,
>
> On Tue, 2023-09-12 at 16:09 +0200, Rafael J. Wysocki wrote:
> > On Tue, Aug 29, 2023 at 2:23 AM Srinivas Pandruvada
> > <srinivas.pandruvada@linux.intel.com> wrote:
> > >
> > >
>
> [...]
>
> > > --
> >
> > There is a slight issue with the patch ordering in this series,
> > because the interface to enable the interrupt should only be provided
> > after implementing the interrupt handlers.  I don't think that anyone
> > will apply the series partially and try to enable the feature,
> > though.
> Thanks!
>
> >
> > Also, I'm not actually sure if proc_thermal_wt_intr_callback() can
> > run
> > safely against the work item scheduled in proc_thermal_irq_handler()
> > in case the workload hint one triggers along with a thermal threshold
> > one.  I think that the access to MMIO is cached, so what if they both
> > try to update the same cache line at the same time?  Or are they
> > guaranteed to be different cache lines?
> These two registers are 90 cache lines apart. Looking at all the
> registers on this bar for status offsets, they are several cache lines
> apart. Also this bar is non prefetchable, so continuous data can't be
> fetched ahead.

OK

> >
> > Anyway, tentatively applied as 6.7 material, but I've changed the
> > second patch somewhat, because I couldn't convince myself that the
> > implicit type conversions in
> > processor_thermal_mbox_interrupt_config()
> > would always do the right thing regardless of the numbers involved,
> > so
> > please check the result in my bleeding-edge branch.
> >
> If I diff, there is only one change in processor_thermal_mbox.c. Tested
> that change and works fine.

Good, thanks!