Op 04-01-17 om 12:28 schreef Daniel Vetter:
> On Wed, Jan 04, 2017 at 11:22:54AM +0000, Chris Wilson wrote:
>> On Wed, Jan 04, 2017 at 12:15:55PM +0100, Maarten Lankhorst wrote:
>>> Op 15-12-16 om 16:19 schreef Daniel Vetter:
>>>> On Thu, Dec 15, 2016 at 03:29:42PM +0100, Maarten Lankhorst wrote:
>>>>> drm_atomic_state_put is called unconditionally, so TEST_ONLY is no
>>>>> different from commit.
>>>>>
>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>>> I think it'd be good to update the kerneldoc for the atomic_commit
>>>> callback to mention that drivers should grab their own references using
>>>> drm_atomic_state_get() when they need it.
>>>>
>>>> Applied this one meanwhile, thanks.
>>> --->8---
>>> Commit 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
>>> adds reference counting to atomic state, but didn't update the comments
>>> in drm_atomic_(nonblocking_)commit. Clarify lifetime a bit more.
>>>
>>> Fixes: 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
>>> Cc: <drm-intel-fixes@xxxxxxxxxxxxxxxxxxxxx> # v4.10-rc1+
>>> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>> ---
>>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>>> index 681d5f97639d..6492546476b4 100644
>>> --- a/drivers/gpu/drm/drm_atomic.c
>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>> @@ -1599,10 +1599,9 @@ EXPORT_SYMBOL(drm_atomic_check_only);
>>> * more locks but encountered a deadlock. The caller must then do the usual w/w
>>> * backoff dance and restart. All other errors are fatal.
>>> *
>>> - * Also note that on successful execution ownership of @state is transferred
>>> - * from the caller of this function to the function itself. The caller must not
>>> - * free or in any other way access @state. If the function fails then the caller
>>> - * must clean up @state itself.
>>> + * In earlier versions of the atomic api, the caller was handing its reference
>>> + * of @state over to this function on success. This is no longer the case, and
>>> + * callers should always call drm_atomic_state_put().
>>> *
>>> * Returns:
>>> * 0 on success, negative error code on failure.
>>> @@ -1630,10 +1629,9 @@ EXPORT_SYMBOL(drm_atomic_commit);
>>> * more locks but encountered a deadlock. The caller must then do the usual w/w
>>> * backoff dance and restart. All other errors are fatal.
>>> *
>>> - * Also note that on successful execution ownership of @state is transferred
>>> - * from the caller of this function to the function itself. The caller must not
>>> - * free or in any other way access @state. If the function fails then the caller
>>> - * must clean up @state itself.
>>> + * In earlier versions of the atomic api, the caller was handing its reference
>>> + * of @state over to this function on success. This is no longer the case, and
>>> + * callers should always call drm_atomic_state_put().
>> Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>>
>> But do we really want the confusion of mentioning what the api doesn't do
>> any more?
> Agreed, kerneldoc should state what is (and why), maybe with a FIXME if it
> only makes sense with some hystorical context and what should be done
> instead. git blame is for figuring out what was.
>
> Maarten, can you pls respin?
> -Daniel
----8<---
Commit 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
adds reference counting to atomic state, but didn't update the comments
in drm_atomic_(nonblocking_)commit. Clarify lifetime a bit more.
Fixes: 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state")
Cc: <drm-intel-fixes@xxxxxxxxxxxxxxxxxxxxx> # v4.10-rc1+
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
---
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 681d5f97639d..672f1de84d6c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1599,10 +1599,8 @@ EXPORT_SYMBOL(drm_atomic_check_only);
* more locks but encountered a deadlock. The caller must then do the usual w/w
* backoff dance and restart. All other errors are fatal.
*
- * Also note that on successful execution ownership of @state is transferred
- * from the caller of this function to the function itself. The caller must not
- * free or in any other way access @state. If the function fails then the caller
- * must clean up @state itself.
+ * This function will take its own reference on @state.
+ * Callers should always release their reference with drm_atomic_state_put().
*
* Returns:
* 0 on success, negative error code on failure.
@@ -1630,10 +1628,8 @@ EXPORT_SYMBOL(drm_atomic_commit);
* more locks but encountered a deadlock. The caller must then do the usual w/w
* backoff dance and restart. All other errors are fatal.
*
- * Also note that on successful execution ownership of @state is transferred
- * from the caller of this function to the function itself. The caller must not
- * free or in any other way access @state. If the function fails then the caller
- * must clean up @state itself.
+ * This function will take its own reference on @state.
+ * Callers should always release their reference with drm_atomic_state_put().
*
* Returns:
* 0 on success, negative error code on failure.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
