[PATCH] drm/panfrost: Make devfreq truly optional

[PATCH] drm/panfrost: Make devfreq truly optional

[Ezequiel Garcia]

Currently, there is some logic to make devfreq optional,
but it fails to cover some cases such as !CONFIG_PM_DEVFREQ.

Moreover, depending on return codes is not resilient to change,
so let's take a different approach, introducing proper
stubs and only conditionally compiling the devfreq support.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/gpu/drm/panfrost/Makefile           |  3 ++-
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 13 ++-----------
 drivers/gpu/drm/panfrost/panfrost_devfreq.h | 19 +++++++++++++++++--
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
index 6de72d13c58f..8b690672f9d9 100644
--- a/drivers/gpu/drm/panfrost/Makefile
+++ b/drivers/gpu/drm/panfrost/Makefile
@@ -3,10 +3,11 @@
 panfrost-y := \
 	panfrost_drv.o \
 	panfrost_device.o \
-	panfrost_devfreq.o \
 	panfrost_gem.o \
 	panfrost_gpu.o \
 	panfrost_job.o \
 	panfrost_mmu.o
 
+panfrost-$(CONFIG_PM_DEVFREQ) += panfrost_devfreq.o
+
 obj-$(CONFIG_DRM_PANFROST) += panfrost.o
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 238bd1d89d43..29fcffdf2d57 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -140,8 +140,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 		return 0;
 
 	ret = dev_pm_opp_of_add_table(&pfdev->pdev->dev);
-	if (ret == -ENODEV) /* Optional, continue without devfreq */
-		return 0;
+	if (ret)
+		return ret;
 
 	panfrost_devfreq_reset(pfdev);
 
@@ -170,9 +170,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev)
 {
 	int i;
 
-	if (!pfdev->devfreq.devfreq)
-		return;
-
 	panfrost_devfreq_reset(pfdev);
 	for (i = 0; i < NUM_JOB_SLOTS; i++)
 		pfdev->devfreq.slot[i].busy = false;
@@ -182,9 +179,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev)
 
 void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
 {
-	if (!pfdev->devfreq.devfreq)
-		return;
-
 	devfreq_suspend_device(pfdev->devfreq.devfreq);
 }
 
@@ -194,9 +188,6 @@ static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, i
 	ktime_t now;
 	ktime_t last;
 
-	if (!pfdev->devfreq.devfreq)
-		return;
-
 	now = ktime_get();
 	last = pfdev->devfreq.slot[slot].time_last_update;
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
index eb999531ed90..76b56a8de6e3 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
@@ -4,11 +4,26 @@
 #ifndef __PANFROST_DEVFREQ_H__
 #define __PANFROST_DEVFREQ_H__
 
+#if defined(CONFIG_PM_DEVFREQ)
 int panfrost_devfreq_init(struct panfrost_device *pfdev);
-
 void panfrost_devfreq_resume(struct panfrost_device *pfdev);
 void panfrost_devfreq_suspend(struct panfrost_device *pfdev);
-
 void panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot);
+#else
+static inline int panfrost_devfreq_init(struct panfrost_device *pfdev)
+{
+	return 0;
+}
+
+static inline void panfrost_devfreq_resume(struct panfrost_device *pfdev)
+{}
+
+static inline void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
+{}
+
+static inline void
+panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot)
+{}
+#endif
 
 #endif /* __PANFROST_DEVFREQ_H__ */
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/panfrost: Make devfreq truly optional

[Rob Herring]

On Mon, May 13, 2019 at 12:56 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>
> Currently, there is some logic to make devfreq optional,
> but it fails to cover some cases such as !CONFIG_PM_DEVFREQ.

Fails how? compiling? runtime? Or just builds extra code?

> Moreover, depending on return codes is not resilient to change,
> so let's take a different approach, introducing proper
> stubs and only conditionally compiling the devfreq support.

The downside here is another build time configuration to test. Isn't
devfreq essentially going to be required to run the GPU at higher
frequencies and to not melt it?

>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/Makefile           |  3 ++-
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 13 ++-----------
>  drivers/gpu/drm/panfrost/panfrost_devfreq.h | 19 +++++++++++++++++--
>  3 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
> index 6de72d13c58f..8b690672f9d9 100644
> --- a/drivers/gpu/drm/panfrost/Makefile
> +++ b/drivers/gpu/drm/panfrost/Makefile
> @@ -3,10 +3,11 @@
>  panfrost-y := \
>         panfrost_drv.o \
>         panfrost_device.o \
> -       panfrost_devfreq.o \
>         panfrost_gem.o \
>         panfrost_gpu.o \
>         panfrost_job.o \
>         panfrost_mmu.o
>
> +panfrost-$(CONFIG_PM_DEVFREQ) += panfrost_devfreq.o
> +
>  obj-$(CONFIG_DRM_PANFROST) += panfrost.o
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 238bd1d89d43..29fcffdf2d57 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -140,8 +140,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>                 return 0;
>
>         ret = dev_pm_opp_of_add_table(&pfdev->pdev->dev);
> -       if (ret == -ENODEV) /* Optional, continue without devfreq */
> -               return 0;
> +       if (ret)
> +               return ret;
>
>         panfrost_devfreq_reset(pfdev);
>
> @@ -170,9 +170,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev)
>  {
>         int i;
>
> -       if (!pfdev->devfreq.devfreq)
> -               return;
> -
>         panfrost_devfreq_reset(pfdev);
>         for (i = 0; i < NUM_JOB_SLOTS; i++)
>                 pfdev->devfreq.slot[i].busy = false;
> @@ -182,9 +179,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev)
>
>  void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
>  {
> -       if (!pfdev->devfreq.devfreq)
> -               return;
> -
>         devfreq_suspend_device(pfdev->devfreq.devfreq);
>  }
>
> @@ -194,9 +188,6 @@ static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, i
>         ktime_t now;
>         ktime_t last;
>
> -       if (!pfdev->devfreq.devfreq)
> -               return;
> -
>         now = ktime_get();
>         last = pfdev->devfreq.slot[slot].time_last_update;
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> index eb999531ed90..76b56a8de6e3 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> @@ -4,11 +4,26 @@
>  #ifndef __PANFROST_DEVFREQ_H__
>  #define __PANFROST_DEVFREQ_H__
>
> +#if defined(CONFIG_PM_DEVFREQ)
>  int panfrost_devfreq_init(struct panfrost_device *pfdev);
> -
>  void panfrost_devfreq_resume(struct panfrost_device *pfdev);
>  void panfrost_devfreq_suspend(struct panfrost_device *pfdev);
> -
>  void panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot);
> +#else
> +static inline int panfrost_devfreq_init(struct panfrost_device *pfdev)
> +{
> +       return 0;
> +}
> +
> +static inline void panfrost_devfreq_resume(struct panfrost_device *pfdev)
> +{}
> +
> +static inline void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
> +{}
> +
> +static inline void
> +panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot)
> +{}
> +#endif
>
>  #endif /* __PANFROST_DEVFREQ_H__ */
> --
> 2.20.1
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/panfrost: Make devfreq truly optional

[Ezequiel Garcia]

On Tue, 2019-05-14 at 08:38 -0500, Rob Herring wrote:
> On Mon, May 13, 2019 at 12:56 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
> > Currently, there is some logic to make devfreq optional,
> > but it fails to cover some cases such as !CONFIG_PM_DEVFREQ.
> 
> Fails how? compiling? runtime? Or just builds extra code?
> 

Well, given we currently don't enforce PM_DEVFREQ, the driver fails
to probe. Specifically,

panfrost_devfreq_init
  devfreq_recommended_opp
    -EINVAL
 
> > Moreover, depending on return codes is not resilient to change,
> > so let's take a different approach, introducing proper
> > stubs and only conditionally compiling the devfreq support.
> 
> The downside here is another build time configuration to test. Isn't
> devfreq essentially going to be required to run the GPU at higher
> frequencies and to not melt it?
> 

Right. So the alternative is to select or depend on PM_DEVFREQ.

Otherwise, it's quite confusing for developers to choose the driver
and have it fail to probe because some other option is not selected.

Let me know what you think and I'll cook a v2.

Thanks,
Eze

> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/gpu/drm/panfrost/Makefile           |  3 ++-
> >  drivers/gpu/drm/panfrost/panfrost_devfreq.c | 13 ++-----------
> >  drivers/gpu/drm/panfrost/panfrost_devfreq.h | 19 +++++++++++++++++--
> >  3 files changed, 21 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
> > index 6de72d13c58f..8b690672f9d9 100644
> > --- a/drivers/gpu/drm/panfrost/Makefile
> > +++ b/drivers/gpu/drm/panfrost/Makefile
> > @@ -3,10 +3,11 @@
> >  panfrost-y := \
> >         panfrost_drv.o \
> >         panfrost_device.o \
> > -       panfrost_devfreq.o \
> >         panfrost_gem.o \
> >         panfrost_gpu.o \
> >         panfrost_job.o \
> >         panfrost_mmu.o
> > 
> > +panfrost-$(CONFIG_PM_DEVFREQ) += panfrost_devfreq.o
> > +
> >  obj-$(CONFIG_DRM_PANFROST) += panfrost.o
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > index 238bd1d89d43..29fcffdf2d57 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > @@ -140,8 +140,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> >                 return 0;
> > 
> >         ret = dev_pm_opp_of_add_table(&pfdev->pdev->dev);
> > -       if (ret == -ENODEV) /* Optional, continue without devfreq */
> > -               return 0;
> > +       if (ret)
> > +               return ret;
> > 
> >         panfrost_devfreq_reset(pfdev);
> > 
> > @@ -170,9 +170,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev)
> >  {
> >         int i;
> > 
> > -       if (!pfdev->devfreq.devfreq)
> > -               return;
> > -
> >         panfrost_devfreq_reset(pfdev);
> >         for (i = 0; i < NUM_JOB_SLOTS; i++)
> >                 pfdev->devfreq.slot[i].busy = false;
> > @@ -182,9 +179,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev)
> > 
> >  void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
> >  {
> > -       if (!pfdev->devfreq.devfreq)
> > -               return;
> > -
> >         devfreq_suspend_device(pfdev->devfreq.devfreq);
> >  }
> > 
> > @@ -194,9 +188,6 @@ static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, i
> >         ktime_t now;
> >         ktime_t last;
> > 
> > -       if (!pfdev->devfreq.devfreq)
> > -               return;
> > -
> >         now = ktime_get();
> >         last = pfdev->devfreq.slot[slot].time_last_update;
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > index eb999531ed90..76b56a8de6e3 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > @@ -4,11 +4,26 @@
> >  #ifndef __PANFROST_DEVFREQ_H__
> >  #define __PANFROST_DEVFREQ_H__
> > 
> > +#if defined(CONFIG_PM_DEVFREQ)
> >  int panfrost_devfreq_init(struct panfrost_device *pfdev);
> > -
> >  void panfrost_devfreq_resume(struct panfrost_device *pfdev);
> >  void panfrost_devfreq_suspend(struct panfrost_device *pfdev);
> > -
> >  void panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot);
> > +#else
> > +static inline int panfrost_devfreq_init(struct panfrost_device *pfdev)
> > +{
> > +       return 0;
> > +}
> > +
> > +static inline void panfrost_devfreq_resume(struct panfrost_device *pfdev)
> > +{}
> > +
> > +static inline void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
> > +{}
> > +
> > +static inline void
> > +panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot)
> > +{}
> > +#endif
> > 
> >  #endif /* __PANFROST_DEVFREQ_H__ */
> > --
> > 2.20.1
> > 


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/panfrost: Make devfreq truly optional

[Tomeu Vizoso]

On 5/14/19 5:36 PM, Ezequiel Garcia wrote:
> On Tue, 2019-05-14 at 08:38 -0500, Rob Herring wrote:
>> On Mon, May 13, 2019 at 12:56 PM Ezequiel Garcia <ezequiel@collabora.com> wrote:
>>> Currently, there is some logic to make devfreq optional,
>>> but it fails to cover some cases such as !CONFIG_PM_DEVFREQ.
>>
>> Fails how? compiling? runtime? Or just builds extra code?
>>
> 
> Well, given we currently don't enforce PM_DEVFREQ, the driver fails
> to probe. Specifically,
> 
> panfrost_devfreq_init
>    devfreq_recommended_opp
>      -EINVAL
>   
>>> Moreover, depending on return codes is not resilient to change,
>>> so let's take a different approach, introducing proper
>>> stubs and only conditionally compiling the devfreq support.
>>
>> The downside here is another build time configuration to test. Isn't
>> devfreq essentially going to be required to run the GPU at higher
>> frequencies and to not melt it?
>>
> 
> Right. So the alternative is to select or depend on PM_DEVFREQ.
> 
> Otherwise, it's quite confusing for developers to choose the driver
> and have it fail to probe because some other option is not selected.

Yeah, I don't see a good reason to not use devfreq, so I think we should 
require it.

Thanks,

Tomeu

> Let me know what you think and I'll cook a v2.
> 
> Thanks,
> Eze
> 
>>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> ---
>>>   drivers/gpu/drm/panfrost/Makefile           |  3 ++-
>>>   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 13 ++-----------
>>>   drivers/gpu/drm/panfrost/panfrost_devfreq.h | 19 +++++++++++++++++--
>>>   3 files changed, 21 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
>>> index 6de72d13c58f..8b690672f9d9 100644
>>> --- a/drivers/gpu/drm/panfrost/Makefile
>>> +++ b/drivers/gpu/drm/panfrost/Makefile
>>> @@ -3,10 +3,11 @@
>>>   panfrost-y := \
>>>          panfrost_drv.o \
>>>          panfrost_device.o \
>>> -       panfrost_devfreq.o \
>>>          panfrost_gem.o \
>>>          panfrost_gpu.o \
>>>          panfrost_job.o \
>>>          panfrost_mmu.o
>>>
>>> +panfrost-$(CONFIG_PM_DEVFREQ) += panfrost_devfreq.o
>>> +
>>>   obj-$(CONFIG_DRM_PANFROST) += panfrost.o
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
>>> index 238bd1d89d43..29fcffdf2d57 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
>>> @@ -140,8 +140,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>>>                  return 0;
>>>
>>>          ret = dev_pm_opp_of_add_table(&pfdev->pdev->dev);
>>> -       if (ret == -ENODEV) /* Optional, continue without devfreq */
>>> -               return 0;
>>> +       if (ret)
>>> +               return ret;
>>>
>>>          panfrost_devfreq_reset(pfdev);
>>>
>>> @@ -170,9 +170,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev)
>>>   {
>>>          int i;
>>>
>>> -       if (!pfdev->devfreq.devfreq)
>>> -               return;
>>> -
>>>          panfrost_devfreq_reset(pfdev);
>>>          for (i = 0; i < NUM_JOB_SLOTS; i++)
>>>                  pfdev->devfreq.slot[i].busy = false;
>>> @@ -182,9 +179,6 @@ void panfrost_devfreq_resume(struct panfrost_device *pfdev)
>>>
>>>   void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
>>>   {
>>> -       if (!pfdev->devfreq.devfreq)
>>> -               return;
>>> -
>>>          devfreq_suspend_device(pfdev->devfreq.devfreq);
>>>   }
>>>
>>> @@ -194,9 +188,6 @@ static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev, i
>>>          ktime_t now;
>>>          ktime_t last;
>>>
>>> -       if (!pfdev->devfreq.devfreq)
>>> -               return;
>>> -
>>>          now = ktime_get();
>>>          last = pfdev->devfreq.slot[slot].time_last_update;
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
>>> index eb999531ed90..76b56a8de6e3 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
>>> @@ -4,11 +4,26 @@
>>>   #ifndef __PANFROST_DEVFREQ_H__
>>>   #define __PANFROST_DEVFREQ_H__
>>>
>>> +#if defined(CONFIG_PM_DEVFREQ)
>>>   int panfrost_devfreq_init(struct panfrost_device *pfdev);
>>> -
>>>   void panfrost_devfreq_resume(struct panfrost_device *pfdev);
>>>   void panfrost_devfreq_suspend(struct panfrost_device *pfdev);
>>> -
>>>   void panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot);
>>> +#else
>>> +static inline int panfrost_devfreq_init(struct panfrost_device *pfdev)
>>> +{
>>> +       return 0;
>>> +}
>>> +
>>> +static inline void panfrost_devfreq_resume(struct panfrost_device *pfdev)
>>> +{}
>>> +
>>> +static inline void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
>>> +{}
>>> +
>>> +static inline void
>>> +panfrost_devfreq_record_transition(struct panfrost_device *pfdev, int slot)
>>> +{}
>>> +#endif
>>>
>>>   #endif /* __PANFROST_DEVFREQ_H__ */
>>> --
>>> 2.20.1
>>>
> 
> 
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel