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@xxxxxxxxxxxxxxxxxxxxx
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >>>> ---
> >>>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
