* [PATCH] drm/msm/gpu: Fix crash on devices without devfreq support
@ 2022-02-19 18:33 Rob Clark
2022-02-19 18:37 ` Rob Clark
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Rob Clark @ 2022-02-19 18:33 UTC (permalink / raw)
To: dri-devel
Cc: Rob Clark, Anders Roxell, David Airlie, linux-arm-msm,
Abhinav Kumar, open list, Sean Paul,
Linux Kernel Functional Testing, freedreno
From: Rob Clark <robdclark@chromium.org>
Avoid going down devfreq paths on devices where devfreq is not
initialized.
Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Reported-by: Anders Roxell <anders.roxell@linaro.org>
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
drivers/gpu/drm/msm/msm_gpu_devfreq.c | 31 +++++++++++++++++++++------
1 file changed, 25 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
index 9bf319be11f6..26a3669a97b3 100644
--- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
+++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
@@ -83,12 +83,17 @@ static struct devfreq_dev_profile msm_devfreq_profile = {
static void msm_devfreq_boost_work(struct kthread_work *work);
static void msm_devfreq_idle_work(struct kthread_work *work);
+static bool has_devfreq(struct msm_gpu *gpu)
+{
+ return !!gpu->funcs->gpu_busy;
+}
+
void msm_devfreq_init(struct msm_gpu *gpu)
{
struct msm_gpu_devfreq *df = &gpu->devfreq;
/* We need target support to do devfreq */
- if (!gpu->funcs->gpu_busy)
+ if (!has_devfreq(gpu))
return;
dev_pm_qos_add_request(&gpu->pdev->dev, &df->idle_freq,
@@ -149,6 +154,9 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
{
struct msm_gpu_devfreq *df = &gpu->devfreq;
+ if (!has_devfreq(gpu))
+ return;
+
devfreq_cooling_unregister(gpu->cooling);
dev_pm_qos_remove_request(&df->boost_freq);
dev_pm_qos_remove_request(&df->idle_freq);
@@ -156,16 +164,24 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
void msm_devfreq_resume(struct msm_gpu *gpu)
{
- gpu->devfreq.busy_cycles = 0;
- gpu->devfreq.time = ktime_get();
+ struct msm_gpu_devfreq *df = &gpu->devfreq;
- devfreq_resume_device(gpu->devfreq.devfreq);
+ if (!has_devfreq(gpu))
+ return;
+
+ df->busy_cycles = 0;
+ df->time = ktime_get();
+
+ devfreq_resume_device(df->devfreq);
}
void msm_devfreq_suspend(struct msm_gpu *gpu)
{
struct msm_gpu_devfreq *df = &gpu->devfreq;
+ if (!has_devfreq(gpu))
+ return;
+
devfreq_suspend_device(df->devfreq);
cancel_idle_work(df);
@@ -185,6 +201,9 @@ void msm_devfreq_boost(struct msm_gpu *gpu, unsigned factor)
struct msm_gpu_devfreq *df = &gpu->devfreq;
uint64_t freq;
+ if (!has_devfreq(gpu))
+ return;
+
freq = get_freq(gpu);
freq *= factor;
@@ -207,7 +226,7 @@ void msm_devfreq_active(struct msm_gpu *gpu)
struct devfreq_dev_status status;
unsigned int idle_time;
- if (!df->devfreq)
+ if (!has_devfreq(gpu))
return;
/*
@@ -253,7 +272,7 @@ void msm_devfreq_idle(struct msm_gpu *gpu)
{
struct msm_gpu_devfreq *df = &gpu->devfreq;
- if (!df->devfreq)
+ if (!has_devfreq(gpu))
return;
msm_hrtimer_queue_work(&df->idle_work, ms_to_ktime(1),
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/msm/gpu: Fix crash on devices without devfreq support
2022-02-19 18:33 [PATCH] drm/msm/gpu: Fix crash on devices without devfreq support Rob Clark
@ 2022-02-19 18:37 ` Rob Clark
2022-02-23 3:11 ` Dmitry Baryshkov
2022-03-07 8:37 ` Naresh Kamboju
2 siblings, 0 replies; 7+ messages in thread
From: Rob Clark @ 2022-02-19 18:37 UTC (permalink / raw)
To: dri-devel
Cc: Rob Clark, freedreno, Anders Roxell, David Airlie, linux-arm-msm,
Abhinav Kumar, open list, Linux Kernel Functional Testing,
Sean Paul
On Sat, Feb 19, 2022 at 10:32 AM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Avoid going down devfreq paths on devices where devfreq is not
> initialized.
>
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Reported-by: Anders Roxell <anders.roxell@linaro.org>
forgot to add:
Fixes: 6aa89ae1fb04 ("drm/msm/gpu: Cancel idle/boost work on suspend")
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
> drivers/gpu/drm/msm/msm_gpu_devfreq.c | 31 +++++++++++++++++++++------
> 1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> index 9bf319be11f6..26a3669a97b3 100644
> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> @@ -83,12 +83,17 @@ static struct devfreq_dev_profile msm_devfreq_profile = {
> static void msm_devfreq_boost_work(struct kthread_work *work);
> static void msm_devfreq_idle_work(struct kthread_work *work);
>
> +static bool has_devfreq(struct msm_gpu *gpu)
> +{
> + return !!gpu->funcs->gpu_busy;
> +}
> +
> void msm_devfreq_init(struct msm_gpu *gpu)
> {
> struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> /* We need target support to do devfreq */
> - if (!gpu->funcs->gpu_busy)
> + if (!has_devfreq(gpu))
> return;
>
> dev_pm_qos_add_request(&gpu->pdev->dev, &df->idle_freq,
> @@ -149,6 +154,9 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
> {
> struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> + if (!has_devfreq(gpu))
> + return;
> +
> devfreq_cooling_unregister(gpu->cooling);
> dev_pm_qos_remove_request(&df->boost_freq);
> dev_pm_qos_remove_request(&df->idle_freq);
> @@ -156,16 +164,24 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
>
> void msm_devfreq_resume(struct msm_gpu *gpu)
> {
> - gpu->devfreq.busy_cycles = 0;
> - gpu->devfreq.time = ktime_get();
> + struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> - devfreq_resume_device(gpu->devfreq.devfreq);
> + if (!has_devfreq(gpu))
> + return;
> +
> + df->busy_cycles = 0;
> + df->time = ktime_get();
> +
> + devfreq_resume_device(df->devfreq);
> }
>
> void msm_devfreq_suspend(struct msm_gpu *gpu)
> {
> struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> + if (!has_devfreq(gpu))
> + return;
> +
> devfreq_suspend_device(df->devfreq);
>
> cancel_idle_work(df);
> @@ -185,6 +201,9 @@ void msm_devfreq_boost(struct msm_gpu *gpu, unsigned factor)
> struct msm_gpu_devfreq *df = &gpu->devfreq;
> uint64_t freq;
>
> + if (!has_devfreq(gpu))
> + return;
> +
> freq = get_freq(gpu);
> freq *= factor;
>
> @@ -207,7 +226,7 @@ void msm_devfreq_active(struct msm_gpu *gpu)
> struct devfreq_dev_status status;
> unsigned int idle_time;
>
> - if (!df->devfreq)
> + if (!has_devfreq(gpu))
> return;
>
> /*
> @@ -253,7 +272,7 @@ void msm_devfreq_idle(struct msm_gpu *gpu)
> {
> struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> - if (!df->devfreq)
> + if (!has_devfreq(gpu))
> return;
>
> msm_hrtimer_queue_work(&df->idle_work, ms_to_ktime(1),
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/msm/gpu: Fix crash on devices without devfreq support
2022-02-19 18:33 [PATCH] drm/msm/gpu: Fix crash on devices without devfreq support Rob Clark
2022-02-19 18:37 ` Rob Clark
@ 2022-02-23 3:11 ` Dmitry Baryshkov
2022-02-23 15:46 ` Rob Clark
2022-03-07 8:37 ` Naresh Kamboju
2 siblings, 1 reply; 7+ messages in thread
From: Dmitry Baryshkov @ 2022-02-23 3:11 UTC (permalink / raw)
To: Rob Clark, dri-devel
Cc: Rob Clark, freedreno, Anders Roxell, David Airlie, linux-arm-msm,
Abhinav Kumar, open list, Linux Kernel Functional Testing,
Sean Paul
On 19/02/2022 21:33, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> Avoid going down devfreq paths on devices where devfreq is not
> initialized.
>
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Reported-by: Anders Roxell <anders.roxell@linaro.org>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
> drivers/gpu/drm/msm/msm_gpu_devfreq.c | 31 +++++++++++++++++++++------
> 1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> index 9bf319be11f6..26a3669a97b3 100644
> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> @@ -83,12 +83,17 @@ static struct devfreq_dev_profile msm_devfreq_profile = {
> static void msm_devfreq_boost_work(struct kthread_work *work);
> static void msm_devfreq_idle_work(struct kthread_work *work);
>
> +static bool has_devfreq(struct msm_gpu *gpu)
> +{
> + return !!gpu->funcs->gpu_busy;
I see that devfreq init will be skipped if gpu_busy is NULL.
Can we use gpu->devfreq instead of this condition?
I noticed that you have replaced some of gpu->devfreq checks with
has_devreq() calls. Is there any difference?
> +}
> +
> void msm_devfreq_init(struct msm_gpu *gpu)
> {
> struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> /* We need target support to do devfreq */
> - if (!gpu->funcs->gpu_busy)
> + if (!has_devfreq(gpu))
> return;
>
> dev_pm_qos_add_request(&gpu->pdev->dev, &df->idle_freq,
> @@ -149,6 +154,9 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
> {
> struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> + if (!has_devfreq(gpu))
> + return;
> +
> devfreq_cooling_unregister(gpu->cooling);
> dev_pm_qos_remove_request(&df->boost_freq);
> dev_pm_qos_remove_request(&df->idle_freq);
> @@ -156,16 +164,24 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
>
> void msm_devfreq_resume(struct msm_gpu *gpu)
> {
> - gpu->devfreq.busy_cycles = 0;
> - gpu->devfreq.time = ktime_get();
> + struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> - devfreq_resume_device(gpu->devfreq.devfreq);
> + if (!has_devfreq(gpu))
> + return;
> +
> + df->busy_cycles = 0;
> + df->time = ktime_get();
> +
> + devfreq_resume_device(df->devfreq);
> }
>
> void msm_devfreq_suspend(struct msm_gpu *gpu)
> {
> struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> + if (!has_devfreq(gpu))
> + return;
> +
> devfreq_suspend_device(df->devfreq);
>
> cancel_idle_work(df);
> @@ -185,6 +201,9 @@ void msm_devfreq_boost(struct msm_gpu *gpu, unsigned factor)
> struct msm_gpu_devfreq *df = &gpu->devfreq;
> uint64_t freq;
>
> + if (!has_devfreq(gpu))
> + return;
> +
> freq = get_freq(gpu);
> freq *= factor;
>
> @@ -207,7 +226,7 @@ void msm_devfreq_active(struct msm_gpu *gpu)
> struct devfreq_dev_status status;
> unsigned int idle_time;
>
> - if (!df->devfreq)
> + if (!has_devfreq(gpu))
> return;
>
> /*
> @@ -253,7 +272,7 @@ void msm_devfreq_idle(struct msm_gpu *gpu)
> {
> struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> - if (!df->devfreq)
> + if (!has_devfreq(gpu))
> return;
>
> msm_hrtimer_queue_work(&df->idle_work, ms_to_ktime(1),
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/msm/gpu: Fix crash on devices without devfreq support
2022-02-23 3:11 ` Dmitry Baryshkov
@ 2022-02-23 15:46 ` Rob Clark
2022-03-03 7:06 ` Dmitry Baryshkov
0 siblings, 1 reply; 7+ messages in thread
From: Rob Clark @ 2022-02-23 15:46 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Rob Clark, Anders Roxell, David Airlie, linux-arm-msm,
Abhinav Kumar, dri-devel, open list, Sean Paul,
Linux Kernel Functional Testing, freedreno
On Tue, Feb 22, 2022 at 7:11 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On 19/02/2022 21:33, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Avoid going down devfreq paths on devices where devfreq is not
> > initialized.
> >
> > Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> > Reported-by: Anders Roxell <anders.roxell@linaro.org>
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> > drivers/gpu/drm/msm/msm_gpu_devfreq.c | 31 +++++++++++++++++++++------
> > 1 file changed, 25 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> > index 9bf319be11f6..26a3669a97b3 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> > +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> > @@ -83,12 +83,17 @@ static struct devfreq_dev_profile msm_devfreq_profile = {
> > static void msm_devfreq_boost_work(struct kthread_work *work);
> > static void msm_devfreq_idle_work(struct kthread_work *work);
> >
> > +static bool has_devfreq(struct msm_gpu *gpu)
> > +{
> > + return !!gpu->funcs->gpu_busy;
>
> I see that devfreq init will be skipped if gpu_busy is NULL.
> Can we use gpu->devfreq instead of this condition?
We could, but then we couldn't also use the same has_devfreq() helper
in msm_devfreq_init(). I thought it was clearer to use the same
helper everywhere.
> I noticed that you have replaced some of gpu->devfreq checks with
> has_devreq() calls. Is there any difference?
It amounts to the same thing because if you don't have gpu_busy, then
devfreq is never initialized. I just thought it clearer to use the
same check in all places.
BR,
-R
> > +}
> > +
> > void msm_devfreq_init(struct msm_gpu *gpu)
> > {
> > struct msm_gpu_devfreq *df = &gpu->devfreq;
> >
> > /* We need target support to do devfreq */
> > - if (!gpu->funcs->gpu_busy)
> > + if (!has_devfreq(gpu))
> > return;
> >
> > dev_pm_qos_add_request(&gpu->pdev->dev, &df->idle_freq,
> > @@ -149,6 +154,9 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
> > {
> > struct msm_gpu_devfreq *df = &gpu->devfreq;
> >
> > + if (!has_devfreq(gpu))
> > + return;
> > +
> > devfreq_cooling_unregister(gpu->cooling);
> > dev_pm_qos_remove_request(&df->boost_freq);
> > dev_pm_qos_remove_request(&df->idle_freq);
> > @@ -156,16 +164,24 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
> >
> > void msm_devfreq_resume(struct msm_gpu *gpu)
> > {
> > - gpu->devfreq.busy_cycles = 0;
> > - gpu->devfreq.time = ktime_get();
> > + struct msm_gpu_devfreq *df = &gpu->devfreq;
> >
> > - devfreq_resume_device(gpu->devfreq.devfreq);
> > + if (!has_devfreq(gpu))
> > + return;
> > +
> > + df->busy_cycles = 0;
> > + df->time = ktime_get();
> > +
> > + devfreq_resume_device(df->devfreq);
> > }
> >
> > void msm_devfreq_suspend(struct msm_gpu *gpu)
> > {
> > struct msm_gpu_devfreq *df = &gpu->devfreq;
> >
> > + if (!has_devfreq(gpu))
> > + return;
> > +
> > devfreq_suspend_device(df->devfreq);
> >
> > cancel_idle_work(df);
> > @@ -185,6 +201,9 @@ void msm_devfreq_boost(struct msm_gpu *gpu, unsigned factor)
> > struct msm_gpu_devfreq *df = &gpu->devfreq;
> > uint64_t freq;
> >
> > + if (!has_devfreq(gpu))
> > + return;
> > +
> > freq = get_freq(gpu);
> > freq *= factor;
> >
> > @@ -207,7 +226,7 @@ void msm_devfreq_active(struct msm_gpu *gpu)
> > struct devfreq_dev_status status;
> > unsigned int idle_time;
> >
> > - if (!df->devfreq)
> > + if (!has_devfreq(gpu))
> > return;
> >
> > /*
> > @@ -253,7 +272,7 @@ void msm_devfreq_idle(struct msm_gpu *gpu)
> > {
> > struct msm_gpu_devfreq *df = &gpu->devfreq;
> >
> > - if (!df->devfreq)
> > + if (!has_devfreq(gpu))
> > return;
> >
> > msm_hrtimer_queue_work(&df->idle_work, ms_to_ktime(1),
>
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/msm/gpu: Fix crash on devices without devfreq support
2022-02-23 15:46 ` Rob Clark
@ 2022-03-03 7:06 ` Dmitry Baryshkov
0 siblings, 0 replies; 7+ messages in thread
From: Dmitry Baryshkov @ 2022-03-03 7:06 UTC (permalink / raw)
To: Rob Clark
Cc: Rob Clark, Anders Roxell, David Airlie, linux-arm-msm,
Abhinav Kumar, dri-devel, open list, Sean Paul,
Linux Kernel Functional Testing, freedreno
On Wed, 23 Feb 2022 at 18:46, Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Feb 22, 2022 at 7:11 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On 19/02/2022 21:33, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > Avoid going down devfreq paths on devices where devfreq is not
> > > initialized.
> > >
> > > Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> > > Reported-by: Anders Roxell <anders.roxell@linaro.org>
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > > drivers/gpu/drm/msm/msm_gpu_devfreq.c | 31 +++++++++++++++++++++------
> > > 1 file changed, 25 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> > > index 9bf319be11f6..26a3669a97b3 100644
> > > --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> > > +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> > > @@ -83,12 +83,17 @@ static struct devfreq_dev_profile msm_devfreq_profile = {
> > > static void msm_devfreq_boost_work(struct kthread_work *work);
> > > static void msm_devfreq_idle_work(struct kthread_work *work);
> > >
> > > +static bool has_devfreq(struct msm_gpu *gpu)
> > > +{
> > > + return !!gpu->funcs->gpu_busy;
> >
> > I see that devfreq init will be skipped if gpu_busy is NULL.
> > Can we use gpu->devfreq instead of this condition?
>
> We could, but then we couldn't also use the same has_devfreq() helper
> in msm_devfreq_init(). I thought it was clearer to use the same
> helper everywhere.
Well... It is not clear at first glance how gpu_busy is related to
devfreq. On the other hand, if gpu->devfreq is NULL, it's obvious that
devfreq is not initialized.
I'd propose to use if (gpu->funcs->gpu_busy) to check if we can init
devfreq and after that to check (gpu->devfreq) as a way to know
whether the devfreq is available.
>
> > I noticed that you have replaced some of gpu->devfreq checks with
> > has_devreq() calls. Is there any difference?
>
> It amounts to the same thing because if you don't have gpu_busy, then
> devfreq is never initialized. I just thought it clearer to use the
> same check in all places.
See my comment above.
>
> BR,
> -R
>
> > > +}
> > > +
> > > void msm_devfreq_init(struct msm_gpu *gpu)
> > > {
> > > struct msm_gpu_devfreq *df = &gpu->devfreq;
> > >
> > > /* We need target support to do devfreq */
> > > - if (!gpu->funcs->gpu_busy)
> > > + if (!has_devfreq(gpu))
> > > return;
> > >
> > > dev_pm_qos_add_request(&gpu->pdev->dev, &df->idle_freq,
> > > @@ -149,6 +154,9 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
> > > {
> > > struct msm_gpu_devfreq *df = &gpu->devfreq;
> > >
> > > + if (!has_devfreq(gpu))
> > > + return;
> > > +
> > > devfreq_cooling_unregister(gpu->cooling);
> > > dev_pm_qos_remove_request(&df->boost_freq);
> > > dev_pm_qos_remove_request(&df->idle_freq);
> > > @@ -156,16 +164,24 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
> > >
> > > void msm_devfreq_resume(struct msm_gpu *gpu)
> > > {
> > > - gpu->devfreq.busy_cycles = 0;
> > > - gpu->devfreq.time = ktime_get();
> > > + struct msm_gpu_devfreq *df = &gpu->devfreq;
> > >
> > > - devfreq_resume_device(gpu->devfreq.devfreq);
> > > + if (!has_devfreq(gpu))
> > > + return;
> > > +
> > > + df->busy_cycles = 0;
> > > + df->time = ktime_get();
> > > +
> > > + devfreq_resume_device(df->devfreq);
> > > }
> > >
> > > void msm_devfreq_suspend(struct msm_gpu *gpu)
> > > {
> > > struct msm_gpu_devfreq *df = &gpu->devfreq;
> > >
> > > + if (!has_devfreq(gpu))
> > > + return;
> > > +
> > > devfreq_suspend_device(df->devfreq);
> > >
> > > cancel_idle_work(df);
> > > @@ -185,6 +201,9 @@ void msm_devfreq_boost(struct msm_gpu *gpu, unsigned factor)
> > > struct msm_gpu_devfreq *df = &gpu->devfreq;
> > > uint64_t freq;
> > >
> > > + if (!has_devfreq(gpu))
> > > + return;
> > > +
> > > freq = get_freq(gpu);
> > > freq *= factor;
> > >
> > > @@ -207,7 +226,7 @@ void msm_devfreq_active(struct msm_gpu *gpu)
> > > struct devfreq_dev_status status;
> > > unsigned int idle_time;
> > >
> > > - if (!df->devfreq)
> > > + if (!has_devfreq(gpu))
> > > return;
> > >
> > > /*
> > > @@ -253,7 +272,7 @@ void msm_devfreq_idle(struct msm_gpu *gpu)
> > > {
> > > struct msm_gpu_devfreq *df = &gpu->devfreq;
> > >
> > > - if (!df->devfreq)
> > > + if (!has_devfreq(gpu))
> > > return;
> > >
> > > msm_hrtimer_queue_work(&df->idle_work, ms_to_ktime(1),
> >
> >
> > --
> > With best wishes
> > Dmitry
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/msm/gpu: Fix crash on devices without devfreq support
2022-02-19 18:33 [PATCH] drm/msm/gpu: Fix crash on devices without devfreq support Rob Clark
2022-02-19 18:37 ` Rob Clark
2022-02-23 3:11 ` Dmitry Baryshkov
@ 2022-03-07 8:37 ` Naresh Kamboju
2022-03-14 12:31 ` Naresh Kamboju
2 siblings, 1 reply; 7+ messages in thread
From: Naresh Kamboju @ 2022-03-07 8:37 UTC (permalink / raw)
To: Rob Clark
Cc: Rob Clark, Anders Roxell, David Airlie, linux-arm-msm,
Abhinav Kumar, dri-devel, open list, Sean Paul,
Linux Kernel Functional Testing, freedreno
Hi Rob,
On Sun, 20 Feb 2022 at 00:02, Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Avoid going down devfreq paths on devices where devfreq is not
> initialized.
>
> Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Reported-by: Anders Roxell <anders.roxell@linaro.org>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
I have tested this patch and the reported kernel crash is fixed [1].
Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
> ---
> drivers/gpu/drm/msm/msm_gpu_devfreq.c | 31 +++++++++++++++++++++------
> 1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> index 9bf319be11f6..26a3669a97b3 100644
> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> @@ -83,12 +83,17 @@ static struct devfreq_dev_profile msm_devfreq_profile = {
> static void msm_devfreq_boost_work(struct kthread_work *work);
> static void msm_devfreq_idle_work(struct kthread_work *work);
>
> +static bool has_devfreq(struct msm_gpu *gpu)
> +{
> + return !!gpu->funcs->gpu_busy;
> +}
> +
> void msm_devfreq_init(struct msm_gpu *gpu)
> {
> struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> /* We need target support to do devfreq */
> - if (!gpu->funcs->gpu_busy)
> + if (!has_devfreq(gpu))
> return;
>
> dev_pm_qos_add_request(&gpu->pdev->dev, &df->idle_freq,
> @@ -149,6 +154,9 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
> {
> struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> + if (!has_devfreq(gpu))
> + return;
> +
> devfreq_cooling_unregister(gpu->cooling);
> dev_pm_qos_remove_request(&df->boost_freq);
> dev_pm_qos_remove_request(&df->idle_freq);
> @@ -156,16 +164,24 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
>
> void msm_devfreq_resume(struct msm_gpu *gpu)
> {
> - gpu->devfreq.busy_cycles = 0;
> - gpu->devfreq.time = ktime_get();
> + struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> - devfreq_resume_device(gpu->devfreq.devfreq);
> + if (!has_devfreq(gpu))
> + return;
> +
> + df->busy_cycles = 0;
> + df->time = ktime_get();
> +
> + devfreq_resume_device(df->devfreq);
> }
>
> void msm_devfreq_suspend(struct msm_gpu *gpu)
> {
> struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> + if (!has_devfreq(gpu))
> + return;
> +
> devfreq_suspend_device(df->devfreq);
>
> cancel_idle_work(df);
> @@ -185,6 +201,9 @@ void msm_devfreq_boost(struct msm_gpu *gpu, unsigned factor)
> struct msm_gpu_devfreq *df = &gpu->devfreq;
> uint64_t freq;
>
> + if (!has_devfreq(gpu))
> + return;
> +
> freq = get_freq(gpu);
> freq *= factor;
>
> @@ -207,7 +226,7 @@ void msm_devfreq_active(struct msm_gpu *gpu)
> struct devfreq_dev_status status;
> unsigned int idle_time;
>
> - if (!df->devfreq)
> + if (!has_devfreq(gpu))
> return;
>
> /*
> @@ -253,7 +272,7 @@ void msm_devfreq_idle(struct msm_gpu *gpu)
> {
> struct msm_gpu_devfreq *df = &gpu->devfreq;
>
> - if (!df->devfreq)
> + if (!has_devfreq(gpu))
> return;
>
> msm_hrtimer_queue_work(&df->idle_work, ms_to_ktime(1),
> --
> 2.34.1
--
Linaro LKFT
https://lkft.linaro.org
[1] https://lkft.validation.linaro.org/scheduler/job/4664600#L1894
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/msm/gpu: Fix crash on devices without devfreq support
2022-03-07 8:37 ` Naresh Kamboju
@ 2022-03-14 12:31 ` Naresh Kamboju
0 siblings, 0 replies; 7+ messages in thread
From: Naresh Kamboju @ 2022-03-14 12:31 UTC (permalink / raw)
To: Rob Clark, Linus Torvalds
Cc: Rob Clark, Anders Roxell, David Airlie, linux-arm-msm,
Abhinav Kumar, dri-devel, open list, Sean Paul,
Linux Kernel Functional Testing, freedreno
Hi Rob and Linus,
On Mon, 7 Mar 2022 at 14:07, Naresh Kamboju <naresh.kamboju@linaro.org> wrote:
>
> Hi Rob,
>
> On Sun, 20 Feb 2022 at 00:02, Rob Clark <robdclark@gmail.com> wrote:
> >
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Avoid going down devfreq paths on devices where devfreq is not
> > initialized.
> >
> > Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
> > Reported-by: Anders Roxell <anders.roxell@linaro.org>
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
>
> I have tested this patch and the reported kernel crash is fixed [1].
>
> Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
>
> > ---
> > drivers/gpu/drm/msm/msm_gpu_devfreq.c | 31 +++++++++++++++++++++------
> > 1 file changed, 25 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> > index 9bf319be11f6..26a3669a97b3 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> > +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> > @@ -83,12 +83,17 @@ static struct devfreq_dev_profile msm_devfreq_profile = {
> > static void msm_devfreq_boost_work(struct kthread_work *work);
> > static void msm_devfreq_idle_work(struct kthread_work *work);
> >
> > +static bool has_devfreq(struct msm_gpu *gpu)
> > +{
> > + return !!gpu->funcs->gpu_busy;
> > +}
> > +
> > void msm_devfreq_init(struct msm_gpu *gpu)
> > {
> > struct msm_gpu_devfreq *df = &gpu->devfreq;
> >
> > /* We need target support to do devfreq */
> > - if (!gpu->funcs->gpu_busy)
> > + if (!has_devfreq(gpu))
> > return;
> >
> > dev_pm_qos_add_request(&gpu->pdev->dev, &df->idle_freq,
> > @@ -149,6 +154,9 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
> > {
> > struct msm_gpu_devfreq *df = &gpu->devfreq;
> >
> > + if (!has_devfreq(gpu))
> > + return;
> > +
> > devfreq_cooling_unregister(gpu->cooling);
> > dev_pm_qos_remove_request(&df->boost_freq);
> > dev_pm_qos_remove_request(&df->idle_freq);
> > @@ -156,16 +164,24 @@ void msm_devfreq_cleanup(struct msm_gpu *gpu)
> >
> > void msm_devfreq_resume(struct msm_gpu *gpu)
> > {
> > - gpu->devfreq.busy_cycles = 0;
> > - gpu->devfreq.time = ktime_get();
> > + struct msm_gpu_devfreq *df = &gpu->devfreq;
> >
> > - devfreq_resume_device(gpu->devfreq.devfreq);
> > + if (!has_devfreq(gpu))
> > + return;
> > +
> > + df->busy_cycles = 0;
> > + df->time = ktime_get();
> > +
> > + devfreq_resume_device(df->devfreq);
> > }
> >
> > void msm_devfreq_suspend(struct msm_gpu *gpu)
> > {
> > struct msm_gpu_devfreq *df = &gpu->devfreq;
> >
> > + if (!has_devfreq(gpu))
> > + return;
> > +
> > devfreq_suspend_device(df->devfreq);
> >
> > cancel_idle_work(df);
> > @@ -185,6 +201,9 @@ void msm_devfreq_boost(struct msm_gpu *gpu, unsigned factor)
> > struct msm_gpu_devfreq *df = &gpu->devfreq;
> > uint64_t freq;
> >
> > + if (!has_devfreq(gpu))
> > + return;
> > +
> > freq = get_freq(gpu);
> > freq *= factor;
> >
> > @@ -207,7 +226,7 @@ void msm_devfreq_active(struct msm_gpu *gpu)
> > struct devfreq_dev_status status;
> > unsigned int idle_time;
> >
> > - if (!df->devfreq)
> > + if (!has_devfreq(gpu))
> > return;
> >
> > /*
> > @@ -253,7 +272,7 @@ void msm_devfreq_idle(struct msm_gpu *gpu)
> > {
> > struct msm_gpu_devfreq *df = &gpu->devfreq;
> >
> > - if (!df->devfreq)
> > + if (!has_devfreq(gpu))
> > return;
> >
> > msm_hrtimer_queue_work(&df->idle_work, ms_to_ktime(1),
FYI,
This patch is missing on Linux 5.17-rc8 [1].
kernel crash log on arm64 db410c device [2] and details [3].
metadata:
git_describe: v5.17-rc8
git_ref: master
git_repo: https://gitlab.com/Linaro/lkft/mirrors/torvalds/linux-mainline
git_sha: 09688c0166e76ce2fb85e86b9d99be8b0084cdf9
kernel-config: https://builds.tuxbuild.com/26LbbVLHqxjh5w5ZtjBMjGmh92P/config
build: https://builds.tuxbuild.com/26LbbVLHqxjh5w5ZtjBMjGmh92P
- Naresh
[1] https://lore.kernel.org/dri-devel/20220219183310.557435-1-robdclark@gmail.com/
[2] https://lkft.validation.linaro.org/scheduler/job/4714905#L2795
[3] https://qa-reports.linaro.org/lkft/linux-mainline-master/build/v5.17-rc8/testrun/8446224/suite/linux-log-parser/test/check-kernel-oops-4714905/details/
>
> --
> Linaro LKFT
> https://lkft.linaro.org
>
> [1] https://lkft.validation.linaro.org/scheduler/job/4664600#L1894
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-03-14 12:32 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-19 18:33 [PATCH] drm/msm/gpu: Fix crash on devices without devfreq support Rob Clark
2022-02-19 18:37 ` Rob Clark
2022-02-23 3:11 ` Dmitry Baryshkov
2022-02-23 15:46 ` Rob Clark
2022-03-03 7:06 ` Dmitry Baryshkov
2022-03-07 8:37 ` Naresh Kamboju
2022-03-14 12:31 ` Naresh Kamboju
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).