On Wed, Jan 04, 2017 at 12:34:00PM +0100, Maarten Lankhorst wrote:
> 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>
Applied to drm-misc, thanks.
-Daniel
> ---
> 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.
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
