| Message ID | 7f9a208d-519c-c392-e338-eac5f37d212a@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> > > 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@lists.freedesktop.org> # v4.10-rc1+ > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > 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@chris-wilson.co.uk> But do we really want the confusion of mentioning what the api doesn't do any more? -Chris
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@linux.intel.com> > > > 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@lists.freedesktop.org> # v4.10-rc1+ > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > --- > > 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@chris-wilson.co.uk> > > 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
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(). * * Returns: * 0 on success, negative error code on failure.