[PATCH 3/5] drm/virtio: Return an error from connector dpms callback

[Takashi Iwai]

On Thu, 27 Jul 2017 10:25:24 +0200,
Daniel Vetter wrote:
> 
> On Thu, Jul 27, 2017 at 10:22 AM, Takashi Iwai <tiwai at 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 at 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