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

2019-05-15 Thread 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  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 
---
  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(>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

2019-05-14 Thread 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  
> 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 
> > ---
> >  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(>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

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

2019-05-14 Thread Rob Herring 
On Mon, May 13, 2019 at 12:56 PM Ezequiel Garcia  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 
> ---
>  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(>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

[PATCH] drm/panfrost: Make devfreq truly optional

2019-05-13 Thread 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 
---
 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(>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