[PATCH] drm/atomic: Call ww_acquire_done after check phase is complete

[PATCH] drm/atomic: Call ww_acquire_done after check phase is complete

[Daniel Vetter]

We want to make sure that no one tries to acquire more locks and
states, and ww mutexes provide debug facilities for that. So use them.

Cc: Rob Clark <robdclark@gmail.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 422183e7ee7d..5ea609f270ea 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1268,6 +1268,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 		}
 	}
 
+	ww_acquire_done(&state->acquire_ctx->ww_ctx);
+
 	return ret;
 }
 EXPORT_SYMBOL(drm_atomic_check_only);
-- 
2.5.0

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

Re: [PATCH] drm/atomic: Call ww_acquire_done after check phase is complete

[Maarten Lankhorst]

Hey,

Op 05-08-15 om 15:08 schreef Daniel Vetter:
> We want to make sure that no one tries to acquire more locks and
> states, and ww mutexes provide debug facilities for that. So use them.
>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c | 2 ++
>  1 file changed, 2 insertions(+)
I like the idea, played with the thought myself, but I think it might need to be slightly less strict for transitional drivers.

~Maarten
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 422183e7ee7d..5ea609f270ea 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1268,6 +1268,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  		}
>  	}
>  
> +	ww_acquire_done(&state->acquire_ctx->ww_ctx);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(drm_atomic_check_only);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Call ww_acquire_done after check phase is complete

[Daniel Vetter]

On Wed, Aug 5, 2015 at 4:57 PM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
>
> Op 05-08-15 om 15:08 schreef Daniel Vetter:
>> We want to make sure that no one tries to acquire more locks and
>> states, and ww mutexes provide debug facilities for that. So use them.
>>
>> Cc: Rob Clark <robdclark@gmail.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c | 2 ++
>>  1 file changed, 2 insertions(+)
> I like the idea, played with the thought myself, but I think it might need to be slightly less strict for transitional drivers.

What would blow up? This should only be called fairly late in the
transition when most of the atomic handling is correctly done. And
i915 is probably the most extreme example of a conversion, so if it
works out for us I think everyone else should be fine too.

Generally drivers only started to do fancy stuff with get_*_state once
converted to atomic to start exploiting it, not before the transition
is completed. i915 is different since we have a lot of our own modeset
code ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: Call ww_acquire_done after check phase is complete

[Maarten Lankhorst]

Op 05-08-15 om 17:03 schreef Daniel Vetter:
> On Wed, Aug 5, 2015 at 4:57 PM, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> Op 05-08-15 om 15:08 schreef Daniel Vetter:
>>> We want to make sure that no one tries to acquire more locks and
>>> states, and ww mutexes provide debug facilities for that. So use them.
>>>
>>> Cc: Rob Clark <robdclark@gmail.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>> ---
>>>  drivers/gpu/drm/drm_atomic.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>> I like the idea, played with the thought myself, but I think it might need to be slightly less strict for transitional drivers.
> What would blow up? This should only be called fairly late in the
> transition when most of the atomic handling is correctly done. And
> i915 is probably the most extreme example of a conversion, so if it
> works out for us I think everyone else should be fine too.
Might blow up with transitional helpers, though not 100% sure if it would.

Also if atomic_check returns -EDEADLK you would still say acquire_done, that will definitely blow up in legitimate cases..

If it doesn't blow up transitional helpers and you fix the -EDEADLK, acked-by. :-)
> Generally drivers only started to do fancy stuff with get_*_state once
> converted to atomic to start exploiting it, not before the transition
> is completed. i915 is different since we have a lot of our own modeset
Should we electrify drm_atomic_get_{*}_state too, to force everyone to use the get_existing_state versions?

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Call ww_acquire_done after check phase is complete

[Daniel Vetter]

On Wed, Aug 5, 2015 at 8:13 PM, Maarten Lankhorst
<maarten.lankhorst@linux.intel.com> wrote:
> Op 05-08-15 om 17:03 schreef Daniel Vetter:
>> On Wed, Aug 5, 2015 at 4:57 PM, Maarten Lankhorst
>> <maarten.lankhorst@linux.intel.com> wrote:
>>> Op 05-08-15 om 15:08 schreef Daniel Vetter:
>>>> We want to make sure that no one tries to acquire more locks and
>>>> states, and ww mutexes provide debug facilities for that. So use them.
>>>>
>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_atomic.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>> I like the idea, played with the thought myself, but I think it might need to be slightly less strict for transitional drivers.
>> What would blow up? This should only be called fairly late in the
>> transition when most of the atomic handling is correctly done. And
>> i915 is probably the most extreme example of a conversion, so if it
>> works out for us I think everyone else should be fine too.
> Might blow up with transitional helpers, though not 100% sure if it would.

Transitional helpers don't use the top-level atomic_commit/check entry
points and hence don't use this function here at all.

> Also if atomic_check returns -EDEADLK you would still say acquire_done, that will definitely blow up in legitimate cases..
>
> If it doesn't blow up transitional helpers and you fix the -EDEADLK, acked-by. :-)

Yeah that needs to be fixed ;-)

>> Generally drivers only started to do fancy stuff with get_*_state once
>> converted to atomic to start exploiting it, not before the transition
>> is completed. i915 is different since we have a lot of our own modeset
> Should we electrify drm_atomic_get_{*}_state too, to force everyone to use the get_existing_state versions?

And I think this is the killer - we unconditionally take the locks
again, taking advantage of -EALREADY. But with this patch that will
blow and we need to patch up all the existing code to use the
get_existing_state functions. That will be a bigger series I guess ...

I'll make a note about this and defer this for now. But the
get_*_state vs. get_existing_*_state thing will actually make these
two functions more distinctive, so I think this patch here will be
really useful. But needs driver fixes and kerneldoc updates too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: Call ww_acquire_done after check phase is complete

[Maarten Lankhorst]

Op 06-08-15 om 00:25 schreef Daniel Vetter:
> On Wed, Aug 5, 2015 at 8:13 PM, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> Op 05-08-15 om 17:03 schreef Daniel Vetter:
>>> On Wed, Aug 5, 2015 at 4:57 PM, Maarten Lankhorst
>>> <maarten.lankhorst@linux.intel.com> wrote:
>>>> Op 05-08-15 om 15:08 schreef Daniel Vetter:
>>>>> We want to make sure that no one tries to acquire more locks and
>>>>> states, and ww mutexes provide debug facilities for that. So use them.
>>>>>
>>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_atomic.c | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>> I like the idea, played with the thought myself, but I think it might need to be slightly less strict for transitional drivers.
>>> What would blow up? This should only be called fairly late in the
>>> transition when most of the atomic handling is correctly done. And
>>> i915 is probably the most extreme example of a conversion, so if it
>>> works out for us I think everyone else should be fine too.
>> Might blow up with transitional helpers, though not 100% sure if it would.
> Transitional helpers don't use the top-level atomic_commit/check entry
> points and hence don't use this function here at all.
>
>> Also if atomic_check returns -EDEADLK you would still say acquire_done, that will definitely blow up in legitimate cases..
>>
>> If it doesn't blow up transitional helpers and you fix the -EDEADLK, acked-by. :-)
> Yeah that needs to be fixed ;-)
After some sleep I think only when ret == 0 we should call acquire_done.
>>> Generally drivers only started to do fancy stuff with get_*_state once
>>> converted to atomic to start exploiting it, not before the transition
>>> is completed. i915 is different since we have a lot of our own modeset
>> Should we electrify drm_atomic_get_{*}_state too, to force everyone to use the get_existing_state versions?
> And I think this is the killer - we unconditionally take the locks
> again, taking advantage of -EALREADY. But with this patch that will
> blow and we need to patch up all the existing code to use the
> get_existing_state functions. That will be a bigger series I guess ...
Depends, this only happens when the object can not be found in the state the locks are taken for planes and crtc's.
But it seems for connectors it happens unconditionally.
> I'll make a note about this and defer this for now. But the
> get_*_state vs. get_existing_*_state thing will actually make these
> two functions more distinctive, so I think this patch here will be
> really useful. But needs driver fixes and kerneldoc updates too.
I think this is fine; grabbing existing state for crtc's and planes will work and grabbing new state during atomic_commit is a bug anyway.
The only driver that uses drm_atomic_get_connector_state is i915, which uses it for the setup phase.

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/atomic: Call ww_acquire_done after check phase is complete

[Daniel Vetter]

On Thu, Aug 06, 2015 at 06:17:28AM +0200, Maarten Lankhorst wrote:
> Op 06-08-15 om 00:25 schreef Daniel Vetter:
> > On Wed, Aug 5, 2015 at 8:13 PM, Maarten Lankhorst
> > <maarten.lankhorst@linux.intel.com> wrote:
> >> Op 05-08-15 om 17:03 schreef Daniel Vetter:
> >>> On Wed, Aug 5, 2015 at 4:57 PM, Maarten Lankhorst
> >>> <maarten.lankhorst@linux.intel.com> wrote:
> >>>> Op 05-08-15 om 15:08 schreef Daniel Vetter:
> >>>>> We want to make sure that no one tries to acquire more locks and
> >>>>> states, and ww mutexes provide debug facilities for that. So use them.
> >>>>>
> >>>>> Cc: Rob Clark <robdclark@gmail.com>
> >>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/drm_atomic.c | 2 ++
> >>>>>  1 file changed, 2 insertions(+)
> >>>> I like the idea, played with the thought myself, but I think it might need to be slightly less strict for transitional drivers.
> >>> What would blow up? This should only be called fairly late in the
> >>> transition when most of the atomic handling is correctly done. And
> >>> i915 is probably the most extreme example of a conversion, so if it
> >>> works out for us I think everyone else should be fine too.
> >> Might blow up with transitional helpers, though not 100% sure if it would.
> > Transitional helpers don't use the top-level atomic_commit/check entry
> > points and hence don't use this function here at all.
> >
> >> Also if atomic_check returns -EDEADLK you would still say acquire_done, that will definitely blow up in legitimate cases..
> >>
> >> If it doesn't blow up transitional helpers and you fix the -EDEADLK, acked-by. :-)
> > Yeah that needs to be fixed ;-)
> After some sleep I think only when ret == 0 we should call acquire_done.
> >>> Generally drivers only started to do fancy stuff with get_*_state once
> >>> converted to atomic to start exploiting it, not before the transition
> >>> is completed. i915 is different since we have a lot of our own modeset
> >> Should we electrify drm_atomic_get_{*}_state too, to force everyone to use the get_existing_state versions?
> > And I think this is the killer - we unconditionally take the locks
> > again, taking advantage of -EALREADY. But with this patch that will
> > blow and we need to patch up all the existing code to use the
> > get_existing_state functions. That will be a bigger series I guess ...
> Depends, this only happens when the object can not be found in the state the locks are taken for planes and crtc's.
> But it seems for connectors it happens unconditionally.
> > I'll make a note about this and defer this for now. But the
> > get_*_state vs. get_existing_*_state thing will actually make these
> > two functions more distinctive, so I think this patch here will be
> > really useful. But needs driver fixes and kerneldoc updates too.
> I think this is fine; grabbing existing state for crtc's and planes will work and grabbing new state during atomic_commit is a bug anyway.
> The only driver that uses drm_atomic_get_connector_state is i915, which uses it for the setup phase.

Yeah I've done a full audit too, everything seems to be fine.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: Call ww_acquire_done after check phase is complete

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7067
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -2              302/302              300/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                 -1              283/283              282/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@flip-vs-rmfb-interruptible      PASS(1)      DMESG_WARN(1)
*ILK  igt@kms_flip@wf_vblank-vs-modeset-interruptible      PASS(1)      DMESG_WARN(1)
*BYT  igt@gem_partial_pwrite_pread@reads-uncached      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/atomic: Call ww_acquire_done after check phase is complete

[Daniel Vetter]

We want to make sure that no one tries to acquire more locks and
states, and ww mutexes provide debug facilities for that. So use them.

v2: Only call acquire_done when ->atomic_check was successful to avoid
falling over an -EDEADLK (spotted by Maarten).

Cc: Rob Clark <robdclark@gmail.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 422183e7ee7d..750ee96097b9 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1268,6 +1268,9 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 		}
 	}
 
+	if (ret == 0)
+		ww_acquire_done(&state->acquire_ctx->ww_ctx);
+
 	return ret;
 }
 EXPORT_SYMBOL(drm_atomic_check_only);
-- 
2.5.0

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

Re: [PATCH] drm/atomic: Call ww_acquire_done after check phase is complete

[Daniel Vetter]

On Thu, Aug 06, 2015 at 03:06:40PM +0200, Daniel Vetter wrote:
> We want to make sure that no one tries to acquire more locks and
> states, and ww mutexes provide debug facilities for that. So use them.
> 
> v2: Only call acquire_done when ->atomic_check was successful to avoid
> falling over an -EDEADLK (spotted by Maarten).
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Picked up to drm-misc with Maarten's irc r-b.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx