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

[Daniel Vetter]

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.

> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch