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

Tue Oct 2 09:50:47 UTC 2018

O
n Tue, Oct 2, 2018 at 11:25 AM Hans de Goede <hdegoede at redhat.com> wrote:
>
> Hi,
>
> On 02-10-18 00:01, Daniel Vetter wrote:
> > On Mon, Oct 1, 2018 at 11:14 PM Hans de Goede <hdegoede at 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 at 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.
>
> Currently my atomic_disable for the crtc is empty, so that may well be
> the culprit. Can I just make this point to drm_atomic_helper_disable_planes_on_crtc ?
> and do I need to do anything in atomic_enable to undo this then or will
> the planes get re-enabled by the atomic core?

Yup, that should work, assuming your plane->atomic_update code will
(re)enable the plane.

> > 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.
>
> I do not think that that is correct, from include/drm/drm_atomic_helper.h:
>
> static inline bool
> drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state,
>                             struct drm_plane_state *new_plane_state)
> {
>          /*
>           * When disabling a plane, CRTC and FB should always be NULL together.
>           * Anything else should be considered a bug in the atomic core, so we
>           * gently warn about it.
>           */
>          WARN_ON((new_plane_state->crtc == NULL && new_plane_state->fb != NULL) |
>                  (new_plane_state->crtc != NULL && new_plane_state->fb == NULL));
>
>          return old_plane_state->crtc && !new_plane_state->crtc;
> }
>
> So crtc->primary->state->fb will be NULL when crtc->primary->state->crtc is NULL.
>
> I guess both could be non NULL but crtc->state->enable is false ?

Ah right, that's the case that can happen. crtc_state->enable == false
still implies crtc_state->active == false, so would still work. But
the ->active check is still redundant.

Cheers, Daniel

> > 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.
>
> Ok, I will take a look at this, I probably will not get around to this next
> week. ATM I'm fixing some high prio bootsplash (plymouth) bugs related to
> the flickerfree work for Fedora 29: https://hansdegoede.livejournal.com/19224.html
>
> >> 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 :-)
>
> Regards,
>
> Hans

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch