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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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@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.
Ok, in that case your change looks reasonable. I'll respin.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx