Re: [PATCH 11/15] staging: vboxvideo: Fix DPMS support after atomic conversion

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 1, 2018 at 11:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 01-10-18 18:52, Daniel Vetter wrote:
> > On Mon, Oct 01, 2018 at 11:37:29AM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 01-10-18 09:53, Daniel Vetter wrote:
> >>> On Wed, Sep 26, 2018 at 09:42:02PM +0200, Hans de Goede wrote:
> >>>> Atomic modesetting does not use the traditional dpms call backs, instead
> >>>> we should check crtc_state->active.
> >>>>
> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>
> >>> Are you sure this does what you want it to do? Atomic helpers fully shut
> >>> down the screen when you do a dpms off, "just blanked" kinda doesn't exist
> >>> as a state by default.
> >>
> >> Yes I'm sure I tested "xset dpms force off" and before this would
> >> result in in the virtual monitors (just windows on the host) to resize to
> >> 640x480 and in that 640x480 still show part of the old contents.
> >
> > Hm, this sounds a bit like a bug in your code somewhere, or at least not
> > 100% converted over to atomic. From a driver pov it should be 100%
> > equivalent code between dpms off and the xrandr --off. If dpms shows some
> > garbage without this, then something is wrong.
>
> I believe the 2 paths are different, xrandr --off actually does a modeset
> disabling the crtc, where as xset dpms force off just changes the DPMS
> property on the connector using the non atomic property ioctls leading to
> the following call graph:
>
> drm_mode_obj_set_property_ioctl()
> set_property_atomic()
> drm_atomic_connector_commit_dpms()
>
> With the last one iterating over all connectors of the crtc to which
> the connector in question is connected and then setting
> crtc_state->active = false if all connectors have their dpms value
> set to a value != DRM_MODE_DPMS_ON.
>
> Followed by a drm_atomic_commit() so all that changes in this code
> path is the active property of the crtc_state. Where as with a --off the
> primary plane_state will get its fb (and crtc) set to NULL.

You're supposed to scan out black in this case, that's correct. But
you're also supposed to switch of the screen (well, scan out black
since that's all vbox allows from the guest side) if your
crtc_funcs->atomic_disable is called.

I think the correct fix is to just shut down the display
unconditionally in atomic_disable, and ignore ->active. The helpers
should take care of things for you already.

Also: plane going NULL is a side effect of the SETCRTC legacy2atomic
code only, for an atomic CRTC OFF you can see a disabled CRTC, but the
primary plane still having an attached framebuffer. Iirc
drm_plane_state->crtc will be set to NULL. So you need this code in
both cases, not only when active changes (but active will be clamped
to false when disabling the CRTC, so checking for ->active is simply
redundant).

> > The only difference is that for dpms off the helpers don't call
> > ->cleanup_plane (and then ->prepare_plane on re-enabling), since the
> > framebuffers are supposed to stick around. Are you perchance doing some
> > modeset programming in there? That would be a bug in the atomic
> > implementation.
> >
> > crtc_state->active should only be consulted from atomic_check callbacks.
> > I've added some pretty lengthy kerneldoc for this just recently, to
> > explain better how this is supposed to work.
>
> See above, I believe that the code path which I point out changes
> crtc_state->active without making any further changes.

Yup, that's all correct. It's just that your driver code shouldn't
need to look at this. See the kernel-doc for drm_crtc->active in
latest upstream.

> I'm pretty new to all this, so I could be wrong, but this is what
> I believe is happening.

No worries. Imo questions = great opportunity to improve the docs :-)

Cheers, Daniel


>
> Regards,
>
> Hans
>
>
>
>
> >
> >> After this patch they become black instead.
> >>
> >> Note somewhat related, Virtualbox does not allow closing a window from
> >> within the guest, if the user wants to stop using an (extra) virtual monitor
> >> it needs to be disabled in the VMs UI. Turning of a monitor through e.g.
> >> "xrandr --output foo --off" just makes the window black.
> >
> > Ok that's funny, but shouldn't be related to what's going on here.
> > -Daniel
> >
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>
> >>
> >>
> >>> -Daniel
> >>>
> >>>> ---
> >>>>    drivers/staging/vboxvideo/vbox_drv.h  |  1 -
> >>>>    drivers/staging/vboxvideo/vbox_mode.c | 28 ++-------------------------
> >>>>    2 files changed, 2 insertions(+), 27 deletions(-)
> >>>>
> >>>> diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
> >>>> index fccb3851d6a3..9cc20c182df1 100644
> >>>> --- a/drivers/staging/vboxvideo/vbox_drv.h
> >>>> +++ b/drivers/staging/vboxvideo/vbox_drv.h
> >>>> @@ -139,7 +139,6 @@ struct vbox_connector {
> >>>>    struct vbox_crtc {
> >>>>            struct drm_crtc base;
> >>>> -  bool blanked;
> >>>>            bool disconnected;
> >>>>            unsigned int crtc_id;
> >>>>            u32 fb_offset;
> >>>> diff --git a/drivers/staging/vboxvideo/vbox_mode.c b/drivers/staging/vboxvideo/vbox_mode.c
> >>>> index c4ec3fa49782..49ff9c4a6302 100644
> >>>> --- a/drivers/staging/vboxvideo/vbox_mode.c
> >>>> +++ b/drivers/staging/vboxvideo/vbox_mode.c
> >>>> @@ -84,14 +84,13 @@ static void vbox_do_modeset(struct drm_crtc *crtc)
> >>>>            }
> >>>>            flags = VBVA_SCREEN_F_ACTIVE;
> >>>> -  flags |= (fb && !vbox_crtc->blanked) ? 0 : VBVA_SCREEN_F_BLANK;
> >>>> +  flags |= (fb && crtc->state->active) ? 0 : VBVA_SCREEN_F_BLANK;
> >>>>            flags |= vbox_crtc->disconnected ? VBVA_SCREEN_F_DISABLED : 0;
> >>>>            hgsmi_process_display_info(vbox->guest_pool, vbox_crtc->crtc_id,
> >>>>                                       x_offset, y_offset,
> >>>>                                       vbox_crtc->x * bpp / 8 +
> >>>>                                                            vbox_crtc->y * pitch,
> >>>> -                             pitch, width, height,
> >>>> -                             vbox_crtc->blanked ? 0 : bpp, flags);
> >>>> +                             pitch, width, height, bpp, flags);
> >>>>    }
> >>>>    static int vbox_set_view(struct drm_crtc *crtc)
> >>>> @@ -128,27 +127,6 @@ static int vbox_set_view(struct drm_crtc *crtc)
> >>>>            return 0;
> >>>>    }
> >>>> -static void vbox_crtc_dpms(struct drm_crtc *crtc, int mode)
> >>>> -{
> >>>> -  struct vbox_crtc *vbox_crtc = to_vbox_crtc(crtc);
> >>>> -  struct vbox_private *vbox = crtc->dev->dev_private;
> >>>> -
> >>>> -  switch (mode) {
> >>>> -  case DRM_MODE_DPMS_ON:
> >>>> -          vbox_crtc->blanked = false;
> >>>> -          break;
> >>>> -  case DRM_MODE_DPMS_STANDBY:
> >>>> -  case DRM_MODE_DPMS_SUSPEND:
> >>>> -  case DRM_MODE_DPMS_OFF:
> >>>> -          vbox_crtc->blanked = true;
> >>>> -          break;
> >>>> -  }
> >>>> -
> >>>> -  mutex_lock(&vbox->hw_mutex);
> >>>> -  vbox_do_modeset(crtc);
> >>>> -  mutex_unlock(&vbox->hw_mutex);
> >>>> -}
> >>>> -
> >>>>    /*
> >>>>     * Try to map the layout of virtual screens to the range of the input device.
> >>>>     * Return true if we need to re-set the crtc modes due to screen offset
> >>>> @@ -276,7 +254,6 @@ static void vbox_crtc_atomic_flush(struct drm_crtc *crtc,
> >>>>    }
> >>>>    static const struct drm_crtc_helper_funcs vbox_crtc_helper_funcs = {
> >>>> -  .dpms = vbox_crtc_dpms,
> >>>>            .disable = vbox_crtc_disable,
> >>>>            .commit = vbox_crtc_commit,
> >>>>            .atomic_flush = vbox_crtc_atomic_flush,
> >>>> @@ -861,7 +838,6 @@ static const struct drm_connector_helper_funcs vbox_connector_helper_funcs = {
> >>>>    };
> >>>>    static const struct drm_connector_funcs vbox_connector_funcs = {
> >>>> -  .dpms = drm_helper_connector_dpms,
> >>>>            .detect = vbox_connector_detect,
> >>>>            .fill_modes = vbox_fill_modes,
> >>>>            .destroy = vbox_connector_destroy,
> >>>> --
> >>>> 2.19.0
> >>>>
> >>>> _______________________________________________
> >>>> dri-devel mailing list
> >>>> dri-devel@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>>
> >



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel