O n Tue, Oct 2, 2018 at 11:25 AM Hans de Goede hdegoede@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@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?
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