Op 16-07-15 om 14:34 schreef Daniel Vetter:
> 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.
Ok, in that case your change looks reasonable. I'll respin.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
