Re: VT console blank ignored by DRM drivers on QEMU

[Alexander Graf <agraf@suse.de>]

On 07/10/2017 11:37 AM, Takashi Iwai wrote:
> On Mon, 10 Jul 2017 11:27:01 +0200,
> Daniel Vetter wrote:
>> On Mon, Jul 10, 2017 at 10:53 AM, Takashi Iwai <tiwai@suse.de> wrote:
>>> we've casually found a weird behavior of DRM drivers on QEMU (cirrus,
>>> bochs, virtio) via openQA: namely, VT console blank is ignored on such
>>> drivers.  The whole screen is kept shown while the cursor blinking
>>> stops.
>>>
>>> I took a closer look, and it seems that drm_fb_helper_blank() just
>>> calls drm_fb_helper_dpms(), by a naive assumption that every driver
>>> properly implements DPMS handling.  Meanwhile bochs driver has a
>>> code to ignore the whole DPMS hilariously.  Ditto for virtio.
>>>
>>> (The cirrus is a bit different story; it has a DPMS implementation,
>>>   but QEMU side seems ignoring it.)
>>>
>>> In the fbcon side, there is a fallback to the explicit clear when the
>>> fb_blank() returns an error, so we should be able to handle it if we
>>> return an error properly.  But the dpms callback is a void function,
>>> so the driver doesn't tell it for now.
>>>
>>>
>>> I guess we have several options to address it.  An easy one would be
>>> to provide an own fb_blank function for returning an error and use it
>>> for the drivers for VM.  The driver can't use any longer
>>> DRM_FB_HELPER_DEFAULT_OPS, thus it'll a bit ugly, though.
>>>
>>> Another is to change dpms callback allowing to return an error.  But
>>> it'd affect so many codes.
>>>
>>> Yet another option would be to define some flag and let
>>> drm_fb_helper_blank() returns an error.  But I also hesitate to do it
>>> just for such a workaround.
>> DPMS should be an error anyway, we want that to be able to properly
>> thread the acquire_ctx EDEADLK backoff stuff through that we need for
>> atomic. That would be the best long-term plan I think.
> So it implies the conversions of the whole legacy stuff?
> That'd be great but take a long way :)
>
>> But aside from that, can't we just teach these drivers to properly do
>> dpms? With the atomic framework dpms is implement as simply turning
>> the screen off, any driver should be able to support that properly.
> It seems that QEMU doesn't support it yet?  We'd need to implement it
> at first there.
>
>> For the fbcon issue, can we perhaps just unconditionally ask fbcon to
>> clear the screen when blanking? It's not really perf critical, so

I think that would be a really good change, yes.

>> doing that for everyone shouldn't hurt.
> I quickly hacked the code and the patch below seems working.
> If this kind of change is acceptable, I'll cook up and submit a proper
> patch.
>
>
> thanks,
>
> Takashi
>
> --- a/drivers/gpu/drm/bochs/bochs_fbdev.c
> +++ b/drivers/gpu/drm/bochs/bochs_fbdev.c
> @@ -22,7 +22,17 @@ static int bochsfb_mmap(struct fb_info *info,
>   
>   static struct fb_ops bochsfb_ops = {
>   	.owner = THIS_MODULE,
> -	DRM_FB_HELPER_DEFAULT_OPS,
> +
> +	/* can't use DRM_FB_HELPER_DEFAULT_OPS due to lack of fb_blank */
> +	.fb_check_var	= drm_fb_helper_check_var,
> +	.fb_set_par	= drm_fb_helper_set_par,
> +	.fb_setcmap	= drm_fb_helper_setcmap,
> +	.fb_blank	= NULL, /* DPMS not working on QEMU */

Is DPMS even specified in the BOCHS VGA adapter? If it's not in the 
spec, there's not a lot QEMU can do about it :).

> +	.fb_pan_display	= drm_fb_helper_pan_display,
> +	.fb_debug_enter = drm_fb_helper_debug_enter,
> +	.fb_debug_leave = drm_fb_helper_debug_leave,
> +	.fb_ioctl	= drm_fb_helper_ioctl,
> +
>   	.fb_fillrect = drm_fb_helper_sys_fillrect,
>   	.fb_copyarea = drm_fb_helper_sys_copyarea,
>   	.fb_imageblit = drm_fb_helper_sys_imageblit,
> diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
> index 7fa58eeadc9d..b0e057628157 100644
> --- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
> +++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
> @@ -128,7 +128,7 @@ static struct fb_ops cirrusfb_ops = {
>   	.fb_copyarea = cirrus_copyarea,
>   	.fb_imageblit = cirrus_imageblit,
>   	.fb_pan_display = drm_fb_helper_pan_display,
> -	.fb_blank = drm_fb_helper_blank,
> +	.fb_blank = NULL, /* DPMS not working on QEMU */

I'm torn on this one. For Cirrus, it might be better to fix QEMU and 
support power saving there. If nothing else at least by switching to a 
blank pane.

>   	.fb_setcmap = drm_fb_helper_setcmap,
>   };
>   
> diff --git a/drivers/gpu/drm/virtio/virtgpu_fb.c b/drivers/gpu/drm/virtio/virtgpu_fb.c
> index 33df067b11c1..ee3a33ce257f 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_fb.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_fb.c
> @@ -200,7 +200,17 @@ static void virtio_gpu_3d_imageblit(struct fb_info *info,
>   
>   static struct fb_ops virtio_gpufb_ops = {
>   	.owner = THIS_MODULE,
> -	DRM_FB_HELPER_DEFAULT_OPS,
> +
> +	/* can't use DRM_FB_HELPER_DEFAULT_OPS due lack of fb_blank */
> +	.fb_check_var	= drm_fb_helper_check_var,
> +	.fb_set_par	= drm_fb_helper_set_par,
> +	.fb_setcmap	= drm_fb_helper_setcmap,
> +	.fb_blank	= NULL, /* DPMS not working on QEMU */

The same spec argument applies here. If the virtio-gpu spec doesn't 
specific DPMS, there's not a lot we can do about it in QEMU today.


Alex

> +	.fb_pan_display	= drm_fb_helper_pan_display,
> +	.fb_debug_enter = drm_fb_helper_debug_enter,
> +	.fb_debug_leave = drm_fb_helper_debug_leave,
> +	.fb_ioctl	= drm_fb_helper_ioctl,
> +
>   	.fb_fillrect = virtio_gpu_3d_fillrect,
>   	.fb_copyarea = virtio_gpu_3d_copyarea,
>   	.fb_imageblit = virtio_gpu_3d_imageblit,


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel