Hi,
On 02-10-18 00:01, Daniel Vetter wrote:
On Mon, Oct 1, 2018 at 11:14 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
