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

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 02-10-18 00:01, Daniel Vetter wrote:
> 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.

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?

> 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 ?

> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel