| Message ID | 20170726205636.19144-4-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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,
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(-)