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

[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 at lists.freedesktop.org
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at 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.

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!).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch