Re: [PATCH 01/13] drm/atomic: Only update crtc->x/y if it's part of the state.

From: Daniel Vetter <daniel@ffwll.ch>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 01/13] drm/atomic: Only update crtc->x/y if it's part of the state.
Date: Thu, 16 Jul 2015 14:34:50 +0200	[thread overview]
Message-ID: <20150716123450.GJ6223@phenom.ffwll.local> (raw)
In-Reply-To: <55A77B8A.1090106@linux.intel.com>

On Thu, Jul 16, 2015 at 11:38:18AM +0200, Maarten Lankhorst wrote:
> Op 16-07-15 om 11:29 schreef Daniel Vetter:
> > On Thu, Jul 16, 2015 at 11:17:29AM +0200, Maarten Lankhorst wrote:
> >> Op 16-07-15 om 11:19 schreef Daniel Vetter:
> >>> On Thu, Jul 16, 2015 at 10:59:14AM +0200, Maarten Lankhorst wrote:
> >>>> Cc: dri-devel@lists.freedesktop.org
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/drm_atomic_helper.c | 7 +++++--
> >>>>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>> index 0898afbc9e23..70e69904291d 100644
> >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>> @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
> >>>>  	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> >>>>  		crtc->mode = crtc->state->mode;
> >>>>  		crtc->enabled = crtc->state->enable;
> >>>> -		crtc->x = crtc->primary->state->src_x >> 16;
> >>>> -		crtc->y = crtc->primary->state->src_y >> 16;
> >>>> +
> >>>> +		if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {
> >>>> +			crtc->x = crtc->primary->state->src_x >> 16;
> >>>> +			crtc->y = crtc->primary->state->src_y >> 16;
> >>>> +		}
> >>> What's the benefit here of only updating when something changed? The
> >>> atomic state should be the master source so copying a few too many times
> >>> shouldn't matter really.
> >> Because you might not be holding plane lock, so primary->state may be garbage.
> > Anyone who wants to touch primary plane must grab the crtc lock, so crtc
> > lock would give you an implicit read lock. At least that's been my
> > thinking, but it could be that the primary plane is used on some other
> > crtc, and then this is indeed garbage.
> This is only true if the plane is active. If there is none you can still update properties and
> swap the plane state without locking the crtc.

Ah right, so still possible to chase a being-freed primary->state pointer.

> > So maybe we need even more checks than what you propose:
> >
> >       if (drm_atomic_get_existing_plane_state(old_state, crtc->primary) &&
> >           crtc->primary->state->crtc == crtc) {
> >       	crtc->x = crtc->primary->state->src_x >> 16;
> >       	crtc->y = crtc->primary->state->src_y >> 16;
> >       }
> >
> > I think a comment explaining this would help (or at least in the commit
> > message!).
> But the primary and cursor planes are not allowed to move between crtc's?

They are allowed to do that actually. crtc->primary and crtc->cursor is
only really a hint to implement backwards compatibility. If you have
generic plane hw with 2 crtc and planes can be freely assigned it would be
silly to artificially restrict the backwards compat planes to 1 crtc.
Otherwise we'd force 2 planes to be unusable when there's no external
screen plugged in, defeating a lot of the value of making planes freely
assignable.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel