Hi,
this is a revised patchset after thinking of Daniel's suggestion. In this version, drm_fb_helper_blank() is modified to treat the error code from the connector dpms callback, so that the caller side, fbcon, can fall back to the generic blank code gracefully for the drivers that don't support DPMS properly.
Takashi
===
Takashi Iwai (5): drm: Propagate error from connector dpms function in drm_fb_helper_blank() drm/bochs: Return an error from connector dpms callback drm/virtio: Return an error from connector dpms callback drm/qxl: Return an error from connector dpms callback drm/cirrus: Return an error from connector dpms callback
drivers/gpu/drm/bochs/bochs_kms.c | 9 ++++++++- drivers/gpu/drm/cirrus/cirrus_mode.c | 9 ++++++++- drivers/gpu/drm/drm_fb_helper.c | 30 ++++++++++++++++++++---------- drivers/gpu/drm/qxl/qxl_display.c | 9 ++++++++- drivers/gpu/drm/virtio/virtgpu_display.c | 9 ++++++++- 5 files changed, 52 insertions(+), 14 deletions(-)
Currently the DRM fbcon helper for console blank, drm_fb_helper_blank(), simply calls drm_fb_helper_dpms() and always returns zero, supposing the driver dealing with DPMS properly for blanking the screen. However, it turned out that the console blank doesn't work at all on KVM/QEMU when DRM driver is used: most of the relevant drivers (bochs, qxl, and virtio) just ignore DPMS, and even cirrus driver doesn't work because the DPMS register bits the driver fiddles with are also ignored by KVM/QEMU.
A simple fix for this problem would be not to rely on DPMS but let fbcon performs the generic blank code. This can be achieved just by returning an error from drm_fb_helper_blank().
In this patch, we change the drm_fb_helper_dpms() to give back an error code returned from the connector dpms callback, so that the error is propagated to drm_fb_helper_blank(). After this change, each driver needs just to return an error to fall back to the generic fbcon blank mode.
Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/gpu/drm/drm_fb_helper.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 574af01d3ce9..db31747ae598 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -581,21 +581,22 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { }; #endif
-static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) +static int drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) { struct drm_fb_helper *fb_helper = info->par; struct drm_device *dev = fb_helper->dev; struct drm_crtc *crtc; struct drm_connector *connector; int i, j; + int ret = 0;
/* * For each CRTC in this fb, turn the connectors on/off. */ drm_modeset_lock_all(dev); if (!drm_fb_helper_is_bound(fb_helper)) { - drm_modeset_unlock_all(dev); - return; + ret = -ENODEV; + goto out; }
for (i = 0; i < fb_helper->crtc_count; i++) { @@ -607,12 +608,16 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) /* Walk the connectors & encoders on this fb turning them on/off */ drm_fb_helper_for_each_connector(fb_helper, j) { connector = fb_helper->connector_info[j]->connector; - connector->funcs->dpms(connector, dpms_mode); + ret = connector->funcs->dpms(connector, dpms_mode); + if (ret < 0) + goto out; drm_object_property_set_value(&connector->base, dev->mode_config.dpms_property, dpms_mode); } } + out: drm_modeset_unlock_all(dev); + return ret; }
/** @@ -622,32 +627,37 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) */ int drm_fb_helper_blank(int blank, struct fb_info *info) { + int dpms_mode; + if (oops_in_progress) return -EBUSY;
switch (blank) { /* Display: On; HSync: On, VSync: On */ case FB_BLANK_UNBLANK: - drm_fb_helper_dpms(info, DRM_MODE_DPMS_ON); + dpms_mode = DRM_MODE_DPMS_ON; break; /* Display: Off; HSync: On, VSync: On */ case FB_BLANK_NORMAL: - drm_fb_helper_dpms(info, DRM_MODE_DPMS_STANDBY); + dpms_mode = DRM_MODE_DPMS_STANDBY; break; /* Display: Off; HSync: Off, VSync: On */ case FB_BLANK_HSYNC_SUSPEND: - drm_fb_helper_dpms(info, DRM_MODE_DPMS_STANDBY); + dpms_mode = DRM_MODE_DPMS_STANDBY; break; /* Display: Off; HSync: On, VSync: Off */ case FB_BLANK_VSYNC_SUSPEND: - drm_fb_helper_dpms(info, DRM_MODE_DPMS_SUSPEND); + dpms_mode = DRM_MODE_DPMS_SUSPEND; break; /* Display: Off; HSync: Off, VSync: Off */ case FB_BLANK_POWERDOWN: - drm_fb_helper_dpms(info, DRM_MODE_DPMS_OFF); + dpms_mode = DRM_MODE_DPMS_OFF; break; + default: + return 0; /* ignored */ } - return 0; + + return drm_fb_helper_dpms(info, dpms_mode); } EXPORT_SYMBOL(drm_fb_helper_blank);
On Wed, Jul 26, 2017 at 10:56:32PM +0200, Takashi Iwai wrote:
Currently the DRM fbcon helper for console blank, drm_fb_helper_blank(), simply calls drm_fb_helper_dpms() and always returns zero, supposing the driver dealing with DPMS properly for blanking the screen. However, it turned out that the console blank doesn't work at all on KVM/QEMU when DRM driver is used: most of the relevant drivers (bochs, qxl, and virtio) just ignore DPMS, and even cirrus driver doesn't work because the DPMS register bits the driver fiddles with are also ignored by KVM/QEMU.
A simple fix for this problem would be not to rely on DPMS but let fbcon performs the generic blank code. This can be achieved just by returning an error from drm_fb_helper_blank().
In this patch, we change the drm_fb_helper_dpms() to give back an error code returned from the connector dpms callback, so that the error is propagated to drm_fb_helper_blank(). After this change, each driver needs just to return an error to fall back to the generic fbcon blank mode.
Signed-off-by: Takashi Iwai tiwai@suse.de
This needs to be rebased onto -next, this code changed a lot. -Daniel
drivers/gpu/drm/drm_fb_helper.c | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 574af01d3ce9..db31747ae598 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -581,21 +581,22 @@ static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { static struct sysrq_key_op sysrq_drm_fb_helper_restore_op = { }; #endif
-static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) +static int drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) { struct drm_fb_helper *fb_helper = info->par; struct drm_device *dev = fb_helper->dev; struct drm_crtc *crtc; struct drm_connector *connector; int i, j;
int ret = 0;
/*
- For each CRTC in this fb, turn the connectors on/off.
*/ drm_modeset_lock_all(dev); if (!drm_fb_helper_is_bound(fb_helper)) {
drm_modeset_unlock_all(dev);return;
ret = -ENODEV;goto out;}
for (i = 0; i < fb_helper->crtc_count; i++) {
@@ -607,12 +608,16 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) /* Walk the connectors & encoders on this fb turning them on/off */ drm_fb_helper_for_each_connector(fb_helper, j) { connector = fb_helper->connector_info[j]->connector;
connector->funcs->dpms(connector, dpms_mode);
ret = connector->funcs->dpms(connector, dpms_mode);if (ret < 0)goto out; drm_object_property_set_value(&connector->base, dev->mode_config.dpms_property, dpms_mode);- out: drm_modeset_unlock_all(dev);
- return ret;
}
/** @@ -622,32 +627,37 @@ static void drm_fb_helper_dpms(struct fb_info *info, int dpms_mode) */ int drm_fb_helper_blank(int blank, struct fb_info *info) {
int dpms_mode;
if (oops_in_progress) return -EBUSY;
switch (blank) { /* Display: On; HSync: On, VSync: On */ case FB_BLANK_UNBLANK:
drm_fb_helper_dpms(info, DRM_MODE_DPMS_ON);
dpms_mode = DRM_MODE_DPMS_ON;
drm_fb_helper_dpms(info, DRM_MODE_DPMS_STANDBY);
dpms_mode = DRM_MODE_DPMS_STANDBY;
drm_fb_helper_dpms(info, DRM_MODE_DPMS_STANDBY);
dpms_mode = DRM_MODE_DPMS_STANDBY;
drm_fb_helper_dpms(info, DRM_MODE_DPMS_SUSPEND);
dpms_mode = DRM_MODE_DPMS_SUSPEND;
drm_fb_helper_dpms(info, DRM_MODE_DPMS_OFF);
dpms_mode = DRM_MODE_DPMS_OFF;- default:
return 0; /* ignored */
- return 0;
- return drm_fb_helper_dpms(info, dpms_mode);
} EXPORT_SYMBOL(drm_fb_helper_blank);
-- 2.13.3
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
The bochs drm driver doesn't treat with DPMS, so we should return an error from the connector dpms callback so that the fbcon can fall back to the generic blank mode.
Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/gpu/drm/bochs/bochs_kms.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c index 6a91e62da2f4..a60d1a05950f 100644 --- a/drivers/gpu/drm/bochs/bochs_kms.c +++ b/drivers/gpu/drm/bochs/bochs_kms.c @@ -217,6 +217,13 @@ bochs_connector_best_encoder(struct drm_connector *connector) return NULL; }
+static int bochs_connector_dpms(struct drm_connector *connector, int mode) +{ + drm_helper_connector_dpms(connector, mode); + /* FIXME: return error to make fbcon generic blank working */ + return -EINVAL; +} + static const struct drm_connector_helper_funcs bochs_connector_connector_helper_funcs = { .get_modes = bochs_connector_get_modes, .mode_valid = bochs_connector_mode_valid, @@ -224,7 +231,7 @@ static const struct drm_connector_helper_funcs bochs_connector_connector_helper_ };
static const struct drm_connector_funcs bochs_connector_connector_funcs = { - .dpms = drm_helper_connector_dpms, + .dpms = bochs_connector_dpms, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = drm_connector_cleanup, };
On Wed, Jul 26, 2017 at 10:56:33PM +0200, Takashi Iwai wrote:
The bochs drm driver doesn't treat with DPMS, so we should return an error from the connector dpms callback so that the fbcon can fall back to the generic blank mode.
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/bochs/bochs_kms.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c b/drivers/gpu/drm/bochs/bochs_kms.c index 6a91e62da2f4..a60d1a05950f 100644 --- a/drivers/gpu/drm/bochs/bochs_kms.c +++ b/drivers/gpu/drm/bochs/bochs_kms.c @@ -217,6 +217,13 @@ bochs_connector_best_encoder(struct drm_connector *connector) return NULL; }
+static int bochs_connector_dpms(struct drm_connector *connector, int mode) +{
- drm_helper_connector_dpms(connector, mode);
- /* FIXME: return error to make fbcon generic blank working */
- return -EINVAL;
Thought about this some more, disabling the screen and return -EINVAL (to userspace even) feels like rather bad semantics. Could we just do an -EINVAL instead?
I thought the DPMS off change doesn't do anything anyway, so this is pure dead code. -Daniel
+}
static const struct drm_connector_helper_funcs bochs_connector_connector_helper_funcs = { .get_modes = bochs_connector_get_modes, .mode_valid = bochs_connector_mode_valid, @@ -224,7 +231,7 @@ static const struct drm_connector_helper_funcs bochs_connector_connector_helper_ };
static const struct drm_connector_funcs bochs_connector_connector_funcs = {
- .dpms = drm_helper_connector_dpms,
- .dpms = bochs_connector_dpms, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = drm_connector_cleanup,
};
2.13.3
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
The virtio drm driver doesn't treat with DPMS, so we should return an error from the connector dpms callback so that the fbcon can fall back to the generic blank mode.
Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/gpu/drm/virtio/virtgpu_display.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index d51bd4521f17..77d5bad49c5f 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -249,8 +249,15 @@ static void virtio_gpu_conn_destroy(struct drm_connector *connector) kfree(virtio_gpu_output); }
+static int virtio_gpu_conn_dpms(struct drm_connector *connector, int mode) +{ + drm_atomic_helper_connector_dpms(connector, mode); + /* FIXME: return error to make fbcon generic blank working */ + return -EINVAL; +} + static const struct drm_connector_funcs virtio_gpu_connector_funcs = { - .dpms = drm_atomic_helper_connector_dpms, + .dpms = virtio_gpu_conn_dpms, .detect = virtio_gpu_conn_detect, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = virtio_gpu_conn_destroy,
On Wed, Jul 26, 2017 at 10:56:34PM +0200, Takashi Iwai wrote:
The virtio drm driver doesn't treat with DPMS, so we should return an error from the connector dpms callback so that the fbcon can fall back to the generic blank mode.
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/virtio/virtgpu_display.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index d51bd4521f17..77d5bad49c5f 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -249,8 +249,15 @@ static void virtio_gpu_conn_destroy(struct drm_connector *connector) kfree(virtio_gpu_output); }
+static int virtio_gpu_conn_dpms(struct drm_connector *connector, int mode) +{
- drm_atomic_helper_connector_dpms(connector, mode);
- /* FIXME: return error to make fbcon generic blank working */
Why FIXME here? You just fixed that issue, feels like should just be a normal comment. I'd just say
/* no DPMS for virtual drivers, it would close the window, making * the guest inaccesible */
- return -EINVAL;
Ok, I've changed my plans for properties and DPMS a bit for atomic drivers, and the ->dpms hook will be gone really soon. There's also the problem that rejecting DPMS through the legacy interface, but allowing it through the atomic interface isn't good api (and yes we have igts to check for that).
I think it'd be better to reject this properly in the atomic_check phase in the crtc_helper_funcs->atomic_check function, with something like this:
if (crtc->state->enable && !crtc->state->active) return -EINVAL;
That should result in an -EINVAL for any caller who tries to update the DPMS property. Which is important, since with the new atomic fbdev helper code, fbdev does directly call into the atomic code and entirely bypasses the ->dpms hook (that part is merged already, and why you need to rebase patch 1).
That would also make sure that we don't return -EINVAL and still shut down the screen (atomic does that for you, there's no difference between DPMS off and fully disabling it).
Sorry that I'm dragging you all over with this for yet another respin :-/
Thanks, Daniel
+}
static const struct drm_connector_funcs virtio_gpu_connector_funcs = {
- .dpms = drm_atomic_helper_connector_dpms,
- .dpms = virtio_gpu_conn_dpms, .detect = virtio_gpu_conn_detect, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = virtio_gpu_conn_destroy,
-- 2.13.3
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, 27 Jul 2017 08:52:48 +0200, Daniel Vetter wrote:
On Wed, Jul 26, 2017 at 10:56:34PM +0200, Takashi Iwai wrote:
The virtio drm driver doesn't treat with DPMS, so we should return an error from the connector dpms callback so that the fbcon can fall back to the generic blank mode.
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/virtio/virtgpu_display.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index d51bd4521f17..77d5bad49c5f 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -249,8 +249,15 @@ static void virtio_gpu_conn_destroy(struct drm_connector *connector) kfree(virtio_gpu_output); }
+static int virtio_gpu_conn_dpms(struct drm_connector *connector, int mode) +{
- drm_atomic_helper_connector_dpms(connector, mode);
- /* FIXME: return error to make fbcon generic blank working */
Why FIXME here? You just fixed that issue, feels like should just be a normal comment. I'd just say
/* no DPMS for virtual drivers, it would close the window, making * the guest inaccesible */
- return -EINVAL;
Ok, I've changed my plans for properties and DPMS a bit for atomic drivers, and the ->dpms hook will be gone really soon. There's also the problem that rejecting DPMS through the legacy interface, but allowing it through the atomic interface isn't good api (and yes we have igts to check for that).
I think it'd be better to reject this properly in the atomic_check phase in the crtc_helper_funcs->atomic_check function, with something like this:
if (crtc->state->enable && !crtc->state->active) return -EINVAL;
That should result in an -EINVAL for any caller who tries to update the DPMS property. Which is important, since with the new atomic fbdev helper code, fbdev does directly call into the atomic code and entirely bypasses the ->dpms hook (that part is merged already, and why you need to rebase patch 1).
That would also make sure that we don't return -EINVAL and still shut down the screen (atomic does that for you, there's no difference between DPMS off and fully disabling it).
Sorry that I'm dragging you all over with this for yet another respin :-/
OK, no problem, my patchset was a quite easy one.
The atomic fb helper change looks going to a right direction. When the above check is implemented in the helper side, is still anything needed in each driver side?
Also, for legacy drivers, we still need tricks as done in this patchset, right?
thanks,
Takashi
On Thu, Jul 27, 2017 at 10:22 AM, Takashi Iwai tiwai@suse.de wrote:
On Thu, 27 Jul 2017 08:52:48 +0200, Daniel Vetter wrote:
On Wed, Jul 26, 2017 at 10:56:34PM +0200, Takashi Iwai wrote:
The virtio drm driver doesn't treat with DPMS, so we should return an error from the connector dpms callback so that the fbcon can fall back to the generic blank mode.
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/virtio/virtgpu_display.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index d51bd4521f17..77d5bad49c5f 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -249,8 +249,15 @@ static void virtio_gpu_conn_destroy(struct drm_connector *connector) kfree(virtio_gpu_output); }
+static int virtio_gpu_conn_dpms(struct drm_connector *connector, int mode) +{
- drm_atomic_helper_connector_dpms(connector, mode);
- /* FIXME: return error to make fbcon generic blank working */
Why FIXME here? You just fixed that issue, feels like should just be a normal comment. I'd just say
/* no DPMS for virtual drivers, it would close the window, making * the guest inaccesible */
- return -EINVAL;
Ok, I've changed my plans for properties and DPMS a bit for atomic drivers, and the ->dpms hook will be gone really soon. There's also the problem that rejecting DPMS through the legacy interface, but allowing it through the atomic interface isn't good api (and yes we have igts to check for that).
I think it'd be better to reject this properly in the atomic_check phase in the crtc_helper_funcs->atomic_check function, with something like this:
if (crtc->state->enable && !crtc->state->active) return -EINVAL;That should result in an -EINVAL for any caller who tries to update the DPMS property. Which is important, since with the new atomic fbdev helper code, fbdev does directly call into the atomic code and entirely bypasses the ->dpms hook (that part is merged already, and why you need to rebase patch 1).
That would also make sure that we don't return -EINVAL and still shut down the screen (atomic does that for you, there's no difference between DPMS off and fully disabling it).
Sorry that I'm dragging you all over with this for yet another respin :-/
OK, no problem, my patchset was a quite easy one.
The atomic fb helper change looks going to a right direction. When the above check is implemented in the helper side, is still anything needed in each driver side?
The above check would need to be added to the crtc_funcs->atomic_check callback for vritio and qxl, not the shared atomic helpers.
Also, for legacy drivers, we still need tricks as done in this patchset, right?
Yes, those still need a dpms callback that just returns -EINVAL. -Daniel
On Thu, 27 Jul 2017 10:25:24 +0200, Daniel Vetter wrote:
On Thu, Jul 27, 2017 at 10:22 AM, Takashi Iwai tiwai@suse.de wrote:
On Thu, 27 Jul 2017 08:52:48 +0200, Daniel Vetter wrote:
On Wed, Jul 26, 2017 at 10:56:34PM +0200, Takashi Iwai wrote:
The virtio drm driver doesn't treat with DPMS, so we should return an error from the connector dpms callback so that the fbcon can fall back to the generic blank mode.
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/gpu/drm/virtio/virtgpu_display.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index d51bd4521f17..77d5bad49c5f 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -249,8 +249,15 @@ static void virtio_gpu_conn_destroy(struct drm_connector *connector) kfree(virtio_gpu_output); }
+static int virtio_gpu_conn_dpms(struct drm_connector *connector, int mode) +{
- drm_atomic_helper_connector_dpms(connector, mode);
- /* FIXME: return error to make fbcon generic blank working */
Why FIXME here? You just fixed that issue, feels like should just be a normal comment. I'd just say
/* no DPMS for virtual drivers, it would close the window, making * the guest inaccesible */
- return -EINVAL;
Ok, I've changed my plans for properties and DPMS a bit for atomic drivers, and the ->dpms hook will be gone really soon. There's also the problem that rejecting DPMS through the legacy interface, but allowing it through the atomic interface isn't good api (and yes we have igts to check for that).
I think it'd be better to reject this properly in the atomic_check phase in the crtc_helper_funcs->atomic_check function, with something like this:
if (crtc->state->enable && !crtc->state->active) return -EINVAL;That should result in an -EINVAL for any caller who tries to update the DPMS property. Which is important, since with the new atomic fbdev helper code, fbdev does directly call into the atomic code and entirely bypasses the ->dpms hook (that part is merged already, and why you need to rebase patch 1).
That would also make sure that we don't return -EINVAL and still shut down the screen (atomic does that for you, there's no difference between DPMS off and fully disabling it).
Sorry that I'm dragging you all over with this for yet another respin :-/
OK, no problem, my patchset was a quite easy one.
The atomic fb helper change looks going to a right direction. When the above check is implemented in the helper side, is still anything needed in each driver side?
The above check would need to be added to the crtc_funcs->atomic_check callback for vritio and qxl, not the shared atomic helpers.
Ah, now it's clearly understood.
Also, for legacy drivers, we still need tricks as done in this patchset, right?
Yes, those still need a dpms callback that just returns -EINVAL.
OK.
thanks,
Takashi
The qxl drm driver doesn't treat with DPMS, so we should return an error from the connector dpms callback so that the fbcon can fall back to the generic blank mode.
Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/gpu/drm/qxl/qxl_display.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c index 03fe182203ce..48db53d0285d 100644 --- a/drivers/gpu/drm/qxl/qxl_display.c +++ b/drivers/gpu/drm/qxl/qxl_display.c @@ -1019,8 +1019,15 @@ static void qxl_conn_destroy(struct drm_connector *connector) kfree(qxl_output); }
+static int qxl_conn_dpms(struct drm_connector *connector, int mode) +{ + drm_helper_connector_dpms(connector, mode); + /* FIXME: return error to make fbcon generic blank working */ + return -EINVAL; +} + static const struct drm_connector_funcs qxl_connector_funcs = { - .dpms = drm_helper_connector_dpms, + .dpms = qxl_conn_dpms, .detect = qxl_conn_detect, .fill_modes = drm_helper_probe_single_connector_modes, .set_property = qxl_conn_set_property,
Although the cirrus drm driver handles DPMS and sets the register bits accordingly, currently KVM/QEMU ignores it, resulting in the non-functional console blank. This patch makes the connector dpms callback always returning an error so that the fbcon can fall back to the generic blank mode.
Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/gpu/drm/cirrus/cirrus_mode.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/cirrus/cirrus_mode.c b/drivers/gpu/drm/cirrus/cirrus_mode.c index 53f6f0f84206..7412660bb014 100644 --- a/drivers/gpu/drm/cirrus/cirrus_mode.c +++ b/drivers/gpu/drm/cirrus/cirrus_mode.c @@ -506,13 +506,20 @@ static void cirrus_connector_destroy(struct drm_connector *connector) kfree(connector); }
+static int cirrus_connector_dpms(struct drm_connector *connector, int mode) +{ + drm_helper_connector_dpms(connector, mode); + /* FIXME: return error to make fbcon generic blank working */ + return -EINVAL; +} + static const struct drm_connector_helper_funcs cirrus_vga_connector_helper_funcs = { .get_modes = cirrus_vga_get_modes, .best_encoder = cirrus_connector_best_encoder, };
static const struct drm_connector_funcs cirrus_vga_connector_funcs = { - .dpms = drm_helper_connector_dpms, + .dpms = cirrus_connector_dpms, .fill_modes = drm_helper_probe_single_connector_modes, .destroy = cirrus_connector_destroy, };
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Takashi Iwai