[03/10] drm/amdgpu: handle runtime pm in fbcon

Submitted by Alex Deucher on Aug. 31, 2016, 10:08 p.m.

Details

Message ID 1472681336-30106-3-git-send-email-alexander.deucher@amd.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Alex Deucher Aug. 31, 2016, 10:08 p.m.
Ported from nouveau.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
index 9191467..eaa5dcc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c
@@ -26,6 +26,7 @@ 
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/fb.h>
+#include <linux/pm_runtime.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc.h>
@@ -48,8 +49,32 @@  struct amdgpu_fbdev {
 	struct amdgpu_device *adev;
 };
 
+static int
+amdgpufb_open(struct fb_info *info, int user)
+{
+	struct amdgpu_fbdev *rfbdev = info->par;
+	struct amdgpu_device *adev = rfbdev->adev;
+	int ret = pm_runtime_get_sync(adev->ddev->dev);
+	if (ret < 0 && ret != -EACCES)
+		return ret;
+	return 0;
+}
+
+static int
+amdgpufb_release(struct fb_info *info, int user)
+{
+	struct amdgpu_fbdev *rfbdev = info->par;
+	struct amdgpu_device *adev = rfbdev->adev;
+
+	pm_runtime_mark_last_busy(adev->ddev->dev);
+	pm_runtime_put_autosuspend(adev->ddev->dev);
+	return 0;
+}
+
 static struct fb_ops amdgpufb_ops = {
 	.owner = THIS_MODULE,
+	.fb_open = amdgpufb_open,
+	.fb_release = amdgpufb_release,
 	.fb_check_var = drm_fb_helper_check_var,
 	.fb_set_par = drm_fb_helper_set_par,
 	.fb_fillrect = drm_fb_helper_cfb_fillrect,

Comments

On 01/09/16 07:08 AM, Alex Deucher wrote:
> Ported from nouveau.
> 
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

[...]

> +static int
> +amdgpufb_open(struct fb_info *info, int user)
> +{
> +	struct amdgpu_fbdev *rfbdev = info->par;
> +	struct amdgpu_device *adev = rfbdev->adev;
> +	int ret = pm_runtime_get_sync(adev->ddev->dev);
> +	if (ret < 0 && ret != -EACCES)
> +		return ret;
> +	return 0;
> +}
> +
> +static int
> +amdgpufb_release(struct fb_info *info, int user)
> +{
> +	struct amdgpu_fbdev *rfbdev = info->par;
> +	struct amdgpu_device *adev = rfbdev->adev;
> +
> +	pm_runtime_mark_last_busy(adev->ddev->dev);
> +	pm_runtime_put_autosuspend(adev->ddev->dev);
> +	return 0;
> +}

If pm_runtime_get_sync returns -EACCES, won't there be an imbalance with
the pm_runtime_put_autosuspend call, which might result in the GPU
powering off even when there's something else which is supposed to keep
it on?

Same for patch 8.
On Wed, Aug 31, 2016 at 9:04 PM, Michel Dänzer <michel@daenzer.net> wrote:
> On 01/09/16 07:08 AM, Alex Deucher wrote:
>> Ported from nouveau.
>>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>
> [...]
>
>> +static int
>> +amdgpufb_open(struct fb_info *info, int user)
>> +{
>> +     struct amdgpu_fbdev *rfbdev = info->par;
>> +     struct amdgpu_device *adev = rfbdev->adev;
>> +     int ret = pm_runtime_get_sync(adev->ddev->dev);
>> +     if (ret < 0 && ret != -EACCES)
>> +             return ret;
>> +     return 0;
>> +}
>> +
>> +static int
>> +amdgpufb_release(struct fb_info *info, int user)
>> +{
>> +     struct amdgpu_fbdev *rfbdev = info->par;
>> +     struct amdgpu_device *adev = rfbdev->adev;
>> +
>> +     pm_runtime_mark_last_busy(adev->ddev->dev);
>> +     pm_runtime_put_autosuspend(adev->ddev->dev);
>> +     return 0;
>> +}
>
> If pm_runtime_get_sync returns -EACCES, won't there be an imbalance with
> the pm_runtime_put_autosuspend call, which might result in the GPU
> powering off even when there's something else which is supposed to keep
> it on?
>
> Same for patch 8.

pm_runtime_get_sync only returns -EACCES when runtime pm has been
disabled in which case the GPU is on and pm_runtime_put_autosuspend
won't do anything.

Alex

>
>
> --
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer
On 02/09/16 05:44 AM, Alex Deucher wrote:
> On Wed, Aug 31, 2016 at 9:04 PM, Michel Dänzer <michel@daenzer.net> wrote:
>> On 01/09/16 07:08 AM, Alex Deucher wrote:
>>> Ported from nouveau.
>>>
>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>
>> [...]
>>
>>> +static int
>>> +amdgpufb_open(struct fb_info *info, int user)
>>> +{
>>> +     struct amdgpu_fbdev *rfbdev = info->par;
>>> +     struct amdgpu_device *adev = rfbdev->adev;
>>> +     int ret = pm_runtime_get_sync(adev->ddev->dev);
>>> +     if (ret < 0 && ret != -EACCES)
>>> +             return ret;
>>> +     return 0;
>>> +}
>>> +
>>> +static int
>>> +amdgpufb_release(struct fb_info *info, int user)
>>> +{
>>> +     struct amdgpu_fbdev *rfbdev = info->par;
>>> +     struct amdgpu_device *adev = rfbdev->adev;
>>> +
>>> +     pm_runtime_mark_last_busy(adev->ddev->dev);
>>> +     pm_runtime_put_autosuspend(adev->ddev->dev);
>>> +     return 0;
>>> +}
>>
>> If pm_runtime_get_sync returns -EACCES, won't there be an imbalance with
>> the pm_runtime_put_autosuspend call, which might result in the GPU
>> powering off even when there's something else which is supposed to keep
>> it on?
>>
>> Same for patch 8.
> 
> pm_runtime_get_sync only returns -EACCES when runtime pm has been
> disabled in which case the GPU is on and pm_runtime_put_autosuspend
> won't do anything.

It decreases dev->power.usage_count, and my worry was that
pm_runtime_get_sync might not have increased it when it returns an
error. But it turns out that it does increase it unconditionally.


However, I'm afraid there seems to be another related problem: If
amdgpufb_open returns an error, amdgpufb_release won't be called AFAICT.
In which case amdgpufb_open needs to call pm_runtime_put_autosuspend in
the error case, or the GPU can never power off after that.
> -----Original Message-----

> From: Michel Dänzer [mailto:michel@daenzer.net]

> Sent: Thursday, September 01, 2016 9:47 PM

> To: Alex Deucher

> Cc: amd-gfx list; Deucher, Alexander

> Subject: Re: [PATCH 03/10] drm/amdgpu: handle runtime pm in fbcon

> 

> On 02/09/16 05:44 AM, Alex Deucher wrote:

> > On Wed, Aug 31, 2016 at 9:04 PM, Michel Dänzer <michel@daenzer.net>

> wrote:

> >> On 01/09/16 07:08 AM, Alex Deucher wrote:

> >>> Ported from nouveau.

> >>>

> >>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

> >>

> >> [...]

> >>

> >>> +static int

> >>> +amdgpufb_open(struct fb_info *info, int user)

> >>> +{

> >>> +     struct amdgpu_fbdev *rfbdev = info->par;

> >>> +     struct amdgpu_device *adev = rfbdev->adev;

> >>> +     int ret = pm_runtime_get_sync(adev->ddev->dev);

> >>> +     if (ret < 0 && ret != -EACCES)

> >>> +             return ret;

> >>> +     return 0;

> >>> +}

> >>> +

> >>> +static int

> >>> +amdgpufb_release(struct fb_info *info, int user)

> >>> +{

> >>> +     struct amdgpu_fbdev *rfbdev = info->par;

> >>> +     struct amdgpu_device *adev = rfbdev->adev;

> >>> +

> >>> +     pm_runtime_mark_last_busy(adev->ddev->dev);

> >>> +     pm_runtime_put_autosuspend(adev->ddev->dev);

> >>> +     return 0;

> >>> +}

> >>

> >> If pm_runtime_get_sync returns -EACCES, won't there be an imbalance

> with

> >> the pm_runtime_put_autosuspend call, which might result in the GPU

> >> powering off even when there's something else which is supposed to

> keep

> >> it on?

> >>

> >> Same for patch 8.

> >

> > pm_runtime_get_sync only returns -EACCES when runtime pm has been

> > disabled in which case the GPU is on and pm_runtime_put_autosuspend

> > won't do anything.

> 

> It decreases dev->power.usage_count, and my worry was that

> pm_runtime_get_sync might not have increased it when it returns an

> error. But it turns out that it does increase it unconditionally.

> 

> 

> However, I'm afraid there seems to be another related problem: If

> amdgpufb_open returns an error, amdgpufb_release won't be called

> AFAICT.

> In which case amdgpufb_open needs to call pm_runtime_put_autosuspend

> in

> the error case, or the GPU can never power off after that.


Good point.  I'll fix that up.

Alex

> 

> 

> --

> Earthling Michel Dänzer               |               http://www.amd.com

> Libre software enthusiast             |             Mesa and X developer