[PATCH xserver] modesetting: Properly cleanup fb for reverse-prime-offload

[Hans de Goede]

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 at 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