| Message ID | s5hk23g4opr.wl-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
On Mon, Jul 10, 2017 at 11:37 AM, Takashi Iwai <tiwai@suse.de> wrote: >> 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. I meant to say that adding an error code to the dpms callback seems like a good idea, because we need that anyway. You can ignore the blabla about why exactly atomic drivers need it, and ofc I'm not going to suggest that you convert all your drivers over to atomic first. -Daniel
On Mon, Jul 10, 2017 at 4:56 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Jul 10, 2017 at 11:37 AM, Takashi Iwai <tiwai@suse.de> wrote: >>> 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. > > I meant to say that adding an error code to the dpms callback seems > like a good idea, because we need that anyway. You can ignore the > blabla about why exactly atomic drivers need it, and ofc I'm not going > to suggest that you convert all your drivers over to atomic first. I just realized that we've switched the dpms callback from void to int return type a while ago. So only thing you'd need to do is wire up the return code through the fbdev helpers, and fix up the virtual drivers to not allow dpms. -Daniel
On Tue, 11 Jul 2017 19:35:36 +0200, Daniel Vetter wrote: > > On Mon, Jul 10, 2017 at 4:56 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Jul 10, 2017 at 11:37 AM, Takashi Iwai <tiwai@suse.de> wrote: > >>> 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. > > > > I meant to say that adding an error code to the dpms callback seems > > like a good idea, because we need that anyway. You can ignore the > > blabla about why exactly atomic drivers need it, and ofc I'm not going > > to suggest that you convert all your drivers over to atomic first. > > I just realized that we've switched the dpms callback from void to int > return type a while ago. So only thing you'd need to do is wire up the > return code through the fbdev helpers, and fix up the virtual drivers > to not allow dpms. Hmm, as of 4.13-rc1, I see some inconsistencies: In drm_connector.h: struct drm_connector_funcs { int (*dpms)(struct drm_connector *connector, int mode); In drm_encoder_slave.h: struct drm_encoder_slave_funcs { void (*dpms)(struct drm_encoder *encoder, int mode); In drm_modeset_helper_vtables.h: struct drm_crtc_helper_funcs { void (*dpms)(struct drm_crtc *crtc, int mode); struct drm_encoder_helper_funcs { void (*dpms)(struct drm_encoder *encoder, int mode); Takashi
On Tue, Jul 11, 2017 at 8:10 PM, Takashi Iwai <tiwai@suse.de> wrote: > On Tue, 11 Jul 2017 19:35:36 +0200, > Daniel Vetter wrote: >> >> On Mon, Jul 10, 2017 at 4:56 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> > On Mon, Jul 10, 2017 at 11:37 AM, Takashi Iwai <tiwai@suse.de> wrote: >> >>> 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. >> > >> > I meant to say that adding an error code to the dpms callback seems >> > like a good idea, because we need that anyway. You can ignore the >> > blabla about why exactly atomic drivers need it, and ofc I'm not going >> > to suggest that you convert all your drivers over to atomic first. >> >> I just realized that we've switched the dpms callback from void to int >> return type a while ago. So only thing you'd need to do is wire up the >> return code through the fbdev helpers, and fix up the virtual drivers >> to not allow dpms. > > Hmm, as of 4.13-rc1, I see some inconsistencies: > > In drm_connector.h: > struct drm_connector_funcs { > int (*dpms)(struct drm_connector *connector, int mode); This is the driver interface. > In drm_encoder_slave.h: > struct drm_encoder_slave_funcs { > void (*dpms)(struct drm_encoder *encoder, int mode); > > In drm_modeset_helper_vtables.h: > struct drm_crtc_helper_funcs { > void (*dpms)(struct drm_crtc *crtc, int mode); > > struct drm_encoder_helper_funcs { > void (*dpms)(struct drm_encoder *encoder, int mode); These are just helpers used by the legacy modeset infrastructure and deprecated in atomic. As long as you overwrite the connector->dmps function with your own special one you can return an error code. You still have to carry around the dummy functions doing nothing, because the legacy helpers suck that way (and I'm definitely not going to spend timing cleaning them up, just port to atomic instead). -Daniel
--- 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 */ + .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 */ .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 */ + .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,