| Message ID | 1464807856-9213-1-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | b8ef71fb07a8ba9587aeaca942b4de20b59266ca |
| Headers | show |
| Series |
"modesetting: Properly cleanup fb for reverse-prime-offload"
( rev:
1
)
in
X.org (DEPRECATED - USE GITLAB) |
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 5b90369..4c55c4e 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -643,11 +643,17 @@ drmmode_set_scanout_pixmap_gpu(xf86CrtcPtr crtc, PixmapPtr ppix) PixmapPtr screenpix = screen->GetScreenPixmap(screen); xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; + drmmode_ptr drmmode = drmmode_crtc->drmmode; int c, total_width = 0, max_height = 0, this_x = 0; if (!ppix) { - if (crtc->randr_crtc->scanout_pixmap) + if (crtc->randr_crtc->scanout_pixmap) { PixmapStopDirtyTracking(crtc->randr_crtc->scanout_pixmap, screenpix); + if (drmmode->fb_id) { + drmModeRmFB(drmmode->fd, drmmode->fb_id); + drmmode->fb_id = 0; + } + } drmmode_crtc->prime_pixmap_x = 0; return TRUE; }
Reviewed-by: Dave Airlie <airlied@redhat.com> On 2 June 2016 at 05:04, Hans de Goede <hdegoede@redhat.com> wrote: > drmmode_set_scanout_pixmap_gpu(pix) adds drmmod->fb_id through a call > to drmmode_xf86crtc_resize(), but on a subsequent > drmmode_set_scanout_pixmap_gpu(NULL) it would not remove the fb. > > This keeps the crtc marked as busy, which causes the dgpu to not > being able to runtime suspend, after an output attached to the dgpu > has been used once. Which causes burning through an additional 10W > of power and the laptop to run quite hot. > > This commit adds the missing remove fb call, allowing the dgpu to runtime > suspend after an external monitor has been plugged into the laptop. > > Note this also makes drmmode_set_scanout_pixmap_gpu(NULL) match the > behavior of drmmode_set_scanout_pixmap_cpu(NULL) which was already > removing the fb. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > hw/xfree86/drivers/modesetting/drmmode_display.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c > index 5b90369..4c55c4e 100644 > --- a/hw/xfree86/drivers/modesetting/drmmode_display.c > +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c > @@ -643,11 +643,17 @@ drmmode_set_scanout_pixmap_gpu(xf86CrtcPtr crtc, PixmapPtr ppix) > PixmapPtr screenpix = screen->GetScreenPixmap(screen); > xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); > drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; > + drmmode_ptr drmmode = drmmode_crtc->drmmode; > int c, total_width = 0, max_height = 0, this_x = 0; > > if (!ppix) { > - if (crtc->randr_crtc->scanout_pixmap) > + if (crtc->randr_crtc->scanout_pixmap) { > PixmapStopDirtyTracking(crtc->randr_crtc->scanout_pixmap, screenpix); > + if (drmmode->fb_id) { > + drmModeRmFB(drmmode->fd, drmmode->fb_id); > + drmmode->fb_id = 0; > + } > + } > drmmode_crtc->prime_pixmap_x = 0; > return TRUE; > } > -- > 2.7.4 > > _______________________________________________ > xorg-devel@lists.x.org: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: https://lists.x.org/mailman/listinfo/xorg-devel
On 02.06.2016 04:04, Hans de Goede wrote: > drmmode_set_scanout_pixmap_gpu(pix) adds drmmod->fb_id through a call > to drmmode_xf86crtc_resize(), but on a subsequent > drmmode_set_scanout_pixmap_gpu(NULL) it would not remove the fb. > > This keeps the crtc marked as busy, which causes the dgpu to not > being able to runtime suspend, after an output attached to the dgpu > has been used once. Which causes burning through an additional 10W > of power and the laptop to run quite hot. > > This commit adds the missing remove fb call, allowing the dgpu to runtime > suspend after an external monitor has been plugged into the laptop. > > Note this also makes drmmode_set_scanout_pixmap_gpu(NULL) match the > behavior of drmmode_set_scanout_pixmap_cpu(NULL) which was already > removing the fb. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > hw/xfree86/drivers/modesetting/drmmode_display.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c > index 5b90369..4c55c4e 100644 > --- a/hw/xfree86/drivers/modesetting/drmmode_display.c > +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c > @@ -643,11 +643,17 @@ drmmode_set_scanout_pixmap_gpu(xf86CrtcPtr crtc, PixmapPtr ppix) > PixmapPtr screenpix = screen->GetScreenPixmap(screen); > xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); > drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; > + drmmode_ptr drmmode = drmmode_crtc->drmmode; > int c, total_width = 0, max_height = 0, this_x = 0; > > if (!ppix) { > - if (crtc->randr_crtc->scanout_pixmap) > + if (crtc->randr_crtc->scanout_pixmap) { > PixmapStopDirtyTracking(crtc->randr_crtc->scanout_pixmap, screenpix); > + if (drmmode->fb_id) { > + drmModeRmFB(drmmode->fd, drmmode->fb_id); > + drmmode->fb_id = 0; > + } > + } Can't drmmode->fb_id still be needed for other CRTCs?
On 22.06.2016 12:47, Michel Dänzer wrote: > On 02.06.2016 04:04, Hans de Goede wrote: >> drmmode_set_scanout_pixmap_gpu(pix) adds drmmod->fb_id through a call >> to drmmode_xf86crtc_resize(), but on a subsequent >> drmmode_set_scanout_pixmap_gpu(NULL) it would not remove the fb. >> >> This keeps the crtc marked as busy, which causes the dgpu to not >> being able to runtime suspend, after an output attached to the dgpu >> has been used once. Which causes burning through an additional 10W >> of power and the laptop to run quite hot. >> >> This commit adds the missing remove fb call, allowing the dgpu to runtime >> suspend after an external monitor has been plugged into the laptop. >> >> Note this also makes drmmode_set_scanout_pixmap_gpu(NULL) match the >> behavior of drmmode_set_scanout_pixmap_cpu(NULL) which was already >> removing the fb. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> hw/xfree86/drivers/modesetting/drmmode_display.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c >> index 5b90369..4c55c4e 100644 >> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c >> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c >> @@ -643,11 +643,17 @@ drmmode_set_scanout_pixmap_gpu(xf86CrtcPtr crtc, PixmapPtr ppix) >> PixmapPtr screenpix = screen->GetScreenPixmap(screen); >> xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); >> drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; >> + drmmode_ptr drmmode = drmmode_crtc->drmmode; >> int c, total_width = 0, max_height = 0, this_x = 0; >> >> if (!ppix) { >> - if (crtc->randr_crtc->scanout_pixmap) >> + if (crtc->randr_crtc->scanout_pixmap) { >> PixmapStopDirtyTracking(crtc->randr_crtc->scanout_pixmap, screenpix); >> + if (drmmode->fb_id) { >> + drmModeRmFB(drmmode->fd, drmmode->fb_id); >> + drmmode->fb_id = 0; >> + } >> + } > > Can't drmmode->fb_id still be needed for other CRTCs? I did some experiments, and AFAICT this drmModeRmFB call will turn off all CRTCs which are currently scanning out from the removed framebuffer. Is there code which re-enables any other CRTCs which are still supposed to be on?
Hi, On 22-06-16 09:09, Michel Dänzer wrote: > On 22.06.2016 12:47, Michel Dänzer wrote: >> On 02.06.2016 04:04, Hans de Goede wrote: >>> drmmode_set_scanout_pixmap_gpu(pix) adds drmmod->fb_id through a call >>> to drmmode_xf86crtc_resize(), but on a subsequent >>> drmmode_set_scanout_pixmap_gpu(NULL) it would not remove the fb. >>> >>> This keeps the crtc marked as busy, which causes the dgpu to not >>> being able to runtime suspend, after an output attached to the dgpu >>> has been used once. Which causes burning through an additional 10W >>> of power and the laptop to run quite hot. >>> >>> This commit adds the missing remove fb call, allowing the dgpu to runtime >>> suspend after an external monitor has been plugged into the laptop. >>> >>> Note this also makes drmmode_set_scanout_pixmap_gpu(NULL) match the >>> behavior of drmmode_set_scanout_pixmap_cpu(NULL) which was already >>> removing the fb. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> hw/xfree86/drivers/modesetting/drmmode_display.c | 8 +++++++- >>> 1 file changed, 7 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c >>> index 5b90369..4c55c4e 100644 >>> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c >>> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c >>> @@ -643,11 +643,17 @@ drmmode_set_scanout_pixmap_gpu(xf86CrtcPtr crtc, PixmapPtr ppix) >>> PixmapPtr screenpix = screen->GetScreenPixmap(screen); >>> xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); >>> drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; >>> + drmmode_ptr drmmode = drmmode_crtc->drmmode; >>> int c, total_width = 0, max_height = 0, this_x = 0; >>> >>> if (!ppix) { >>> - if (crtc->randr_crtc->scanout_pixmap) >>> + if (crtc->randr_crtc->scanout_pixmap) { >>> PixmapStopDirtyTracking(crtc->randr_crtc->scanout_pixmap, screenpix); >>> + if (drmmode->fb_id) { >>> + drmModeRmFB(drmmode->fd, drmmode->fb_id); >>> + drmmode->fb_id = 0; >>> + } >>> + } >> >> Can't drmmode->fb_id still be needed for other CRTCs? > > I did some experiments, and AFAICT this drmModeRmFB call will turn off > all CRTCs which are currently scanning out from the removed framebuffer. > Is there code which re-enables any other CRTCs which are still supposed > to be on? Correct, this is intentional drmmode_set_scanout_pixmap(NULL) means that the pixmap which underlies the shared scanout_buffer is being destroyed, see RRCrtcDetachScanoutPixmap() in randr/rrcrtc.c, as such disabling all crtc-s is the right thing to do. Note that calling RRReplaceScanoutPixmap() may result in a rrCrtcSetScanoutPixmap(NULL) call too, but this function always applies to all crtcs of a screen, either setting a new one for all enabled crtcs, or removing it for all enabled crtcs (again because the underlying buffer is being destroyed). TL;DR: Disabling all crtcs on a rrCrtcSetScanoutPixmap(NULL) call is the right thing to do. Regards, Hans
drmmode_set_scanout_pixmap_gpu(pix) adds drmmod->fb_id through a call to drmmode_xf86crtc_resize(), but on a subsequent drmmode_set_scanout_pixmap_gpu(NULL) it would not remove the fb. This keeps the crtc marked as busy, which causes the dgpu to not being able to runtime suspend, after an output attached to the dgpu has been used once. Which causes burning through an additional 10W of power and the laptop to run quite hot. This commit adds the missing remove fb call, allowing the dgpu to runtime suspend after an external monitor has been plugged into the laptop. Note this also makes drmmode_set_scanout_pixmap_gpu(NULL) match the behavior of drmmode_set_scanout_pixmap_cpu(NULL) which was already removing the fb. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- hw/xfree86/drivers/modesetting/drmmode_display.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)