* [PATCH] drm: Release reference from blob lookup after replacing property @ 2016-10-25 19:46 Chris Wilson 2016-10-25 21:27 ` [Intel-gfx] " Sean Paul 2016-10-25 21:28 ` [PATCH v2] " Chris Wilson 0 siblings, 2 replies; 6+ messages in thread From: Chris Wilson @ 2016-10-25 19:46 UTC (permalink / raw) To: dri-devel; +Cc: intel-gfx drm_property_lookup_blob() returns a reference to the returned blob, and drm_atomic_replace_property_blob() takes a references to the blob it stores, so afterwards we are left owning a reference to the new_blob that we never release, and thus leak memory every time we update a property such as during drm_atomic_helper_legacy_gamma_set(). Based on a patch by Felix Monninger <felix.monninger@gmail.com> Reported-by: Felix Monninger <felix.monninger@gmail.com> References: https://bugs.freedesktop.org/show_bug.cgi?id=98420 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/drm_atomic.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 1b5a32df9a9a..3b35ab793100 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -416,19 +416,24 @@ drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc, ssize_t expected_size, bool *replaced) { - struct drm_device *dev = crtc->dev; struct drm_property_blob *new_blob = NULL; if (blob_id != 0) { - new_blob = drm_property_lookup_blob(dev, blob_id); + new_blob = drm_property_lookup_blob(crtc->dev, blob_id); if (new_blob == NULL) return -EINVAL; - if (expected_size > 0 && expected_size != new_blob->length) + + if (expected_size > 0 && expected_size != new_blob->length) { + drm_property_unreference_blob(new_blob); return -EINVAL; + } } drm_atomic_replace_property_blob(blob, new_blob, replaced); + if (new_blob) + drm_property_unreference_blob(new_blob); + return 0; } -- 2.10.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: Release reference from blob lookup after replacing property 2016-10-25 19:46 [PATCH] drm: Release reference from blob lookup after replacing property Chris Wilson @ 2016-10-25 21:27 ` Sean Paul 2016-10-25 21:45 ` Chris Wilson 2016-10-25 21:28 ` [PATCH v2] " Chris Wilson 1 sibling, 1 reply; 6+ messages in thread From: Sean Paul @ 2016-10-25 21:27 UTC (permalink / raw) To: Chris Wilson; +Cc: Intel Graphics Development, dri-devel On Tue, Oct 25, 2016 at 3:46 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > drm_property_lookup_blob() returns a reference to the returned blob, and > drm_atomic_replace_property_blob() takes a references to the blob it > stores, so afterwards we are left owning a reference to the new_blob that > we never release, and thus leak memory every time we update a property > such as during drm_atomic_helper_legacy_gamma_set(). > > Based on a patch by Felix Monninger <felix.monninger@gmail.com> > > Reported-by: Felix Monninger <felix.monninger@gmail.com> > References: https://bugs.freedesktop.org/show_bug.cgi?id=98420 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/drm_atomic.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 1b5a32df9a9a..3b35ab793100 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -416,19 +416,24 @@ drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc, > ssize_t expected_size, > bool *replaced) > { > - struct drm_device *dev = crtc->dev; > struct drm_property_blob *new_blob = NULL; > > if (blob_id != 0) { > - new_blob = drm_property_lookup_blob(dev, blob_id); > + new_blob = drm_property_lookup_blob(crtc->dev, blob_id); I think this could be further simplified by making use of drm_property_lookup_blob() returning NULL for blob_id == 0 Then you could do something like: static int drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc, struct drm_property_blob **old_blob, uint64_t blob_id, ssize_t expected_size, bool *replaced) { struct drm_property_blob *blob = NULL; int ret = 0; blob = drm_property_lookup_blob(crtc->dev, blob_id); if (blob && expected_size > 0 && expected_size != blob->length) { ret = -EINVAL; goto out; } } drm_atomic_replace_property_blob(blob, blob, replaced); out: drm_property_unreference_blob(blob); return 0; } > if (new_blob == NULL) > return -EINVAL; > - if (expected_size > 0 && expected_size != new_blob->length) > + > + if (expected_size > 0 && expected_size != new_blob->length) { > + drm_property_unreference_blob(new_blob); > return -EINVAL; > + } > } > > drm_atomic_replace_property_blob(blob, new_blob, replaced); > > + if (new_blob) > + drm_property_unreference_blob(new_blob); > + > return 0; > } > > -- > 2.10.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm: Release reference from blob lookup after replacing property 2016-10-25 21:27 ` [Intel-gfx] " Sean Paul @ 2016-10-25 21:45 ` Chris Wilson 0 siblings, 0 replies; 6+ messages in thread From: Chris Wilson @ 2016-10-25 21:45 UTC (permalink / raw) To: Sean Paul; +Cc: Intel Graphics Development, dri-devel On Tue, Oct 25, 2016 at 05:27:21PM -0400, Sean Paul wrote: > On Tue, Oct 25, 2016 at 3:46 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > drm_property_lookup_blob() returns a reference to the returned blob, and > > drm_atomic_replace_property_blob() takes a references to the blob it > > stores, so afterwards we are left owning a reference to the new_blob that > > we never release, and thus leak memory every time we update a property > > such as during drm_atomic_helper_legacy_gamma_set(). > > > > Based on a patch by Felix Monninger <felix.monninger@gmail.com> > > > > Reported-by: Felix Monninger <felix.monninger@gmail.com> > > References: https://bugs.freedesktop.org/show_bug.cgi?id=98420 > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/drm_atomic.c | 11 ++++++++--- > > 1 file changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 1b5a32df9a9a..3b35ab793100 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -416,19 +416,24 @@ drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc, > > ssize_t expected_size, > > bool *replaced) > > { > > - struct drm_device *dev = crtc->dev; > > struct drm_property_blob *new_blob = NULL; > > > > if (blob_id != 0) { > > - new_blob = drm_property_lookup_blob(dev, blob_id); > > + new_blob = drm_property_lookup_blob(crtc->dev, blob_id); > > I think this could be further simplified by making use of > drm_property_lookup_blob() returning NULL for blob_id == 0 > > Then you could do something like: > > static int > drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc, > struct drm_property_blob **old_blob, > uint64_t blob_id, > ssize_t expected_size, > bool *replaced) > { > struct drm_property_blob *blob = NULL; > int ret = 0; > > blob = drm_property_lookup_blob(crtc->dev, blob_id); Not sure. I think the orignal code would have been clearer as blob = NULL; if (id) { blob = drm_property_lookup_blob(dev, id); if (!blob) return -ENOENT; if (blob->length != expected_size) return -EINVAL; } i.e. the code currently reports if the blob_id doesn't match an existing blob, and only removes the current blob if passed in 0. Otherwise it becomes like: struct drm_property_blob *blob; int ret = -EINVAL; blob = drm_property_lookup_blob(crtc->dev, blob_id); if (!blob_id || (blob && (expected_size == 0 || expected_size == blob->length))) { drm_atomic_replace_property_blob(old_blob, blob, replaced); ret = 0; } drm_property_unreference_blob(blob); for which I'm actually favouring the existing code for the extra whitespace. If we insisted on a single return path: struct drm_property_blob *new_blob = NULL; int ret = -EINVAL; if (blob_id != 0) { new_blob = drm_property_lookup_blob(crtc->dev, blob_id); if (new_blob == NULL) goto out; if (expected_size > 0 && expected_size != new_blob->length) goto out; } drm_atomic_replace_property_blob(blob, new_blob, replaced); ret = 0; out: drm_property_unreference_blob(new_blob); return ret; -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] drm: Release reference from blob lookup after replacing property 2016-10-25 19:46 [PATCH] drm: Release reference from blob lookup after replacing property Chris Wilson 2016-10-25 21:27 ` [Intel-gfx] " Sean Paul @ 2016-10-25 21:28 ` Chris Wilson 2016-10-26 7:48 ` Ville Syrjälä 1 sibling, 1 reply; 6+ messages in thread From: Chris Wilson @ 2016-10-25 21:28 UTC (permalink / raw) To: dri-devel; +Cc: intel-gfx, Felix Monninger From: Felix Monninger <felix.monninger@gmail.com> drm_property_lookup_blob() returns a reference to the returned blob, and drm_atomic_replace_property_blob() takes a references to the blob it stores, so afterwards we are left owning a reference to the new_blob that we never release, and thus leak memory every time we update a property such as during drm_atomic_helper_legacy_gamma_set(). v2: update credentials, drm_property_unreference_blob() is NULL safe and NULL is passed consistently to it throughout drm_atomic.c so do so here. Reported-by: Felix Monninger <felix.monninger@gmail.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98420 Signed-off-by: Felix Monninger <felix.monninger@gmail.com> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/drm_atomic.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 1b5a32df9a9a..e0760c138355 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -416,18 +416,21 @@ drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc, ssize_t expected_size, bool *replaced) { - struct drm_device *dev = crtc->dev; struct drm_property_blob *new_blob = NULL; if (blob_id != 0) { - new_blob = drm_property_lookup_blob(dev, blob_id); + new_blob = drm_property_lookup_blob(crtc->dev, blob_id); if (new_blob == NULL) return -EINVAL; - if (expected_size > 0 && expected_size != new_blob->length) + + if (expected_size > 0 && expected_size != new_blob->length) { + drm_property_unreference_blob(new_blob); return -EINVAL; + } } drm_atomic_replace_property_blob(blob, new_blob, replaced); + drm_property_unreference_blob(new_blob); return 0; } -- 2.10.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm: Release reference from blob lookup after replacing property 2016-10-25 21:28 ` [PATCH v2] " Chris Wilson @ 2016-10-26 7:48 ` Ville Syrjälä 2016-10-26 8:29 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Ville Syrjälä @ 2016-10-26 7:48 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, dri-devel, Felix Monninger On Tue, Oct 25, 2016 at 10:28:08PM +0100, Chris Wilson wrote: > From: Felix Monninger <felix.monninger@gmail.com> > > drm_property_lookup_blob() returns a reference to the returned blob, and > drm_atomic_replace_property_blob() takes a references to the blob it > stores, so afterwards we are left owning a reference to the new_blob that > we never release, and thus leak memory every time we update a property > such as during drm_atomic_helper_legacy_gamma_set(). > > v2: update credentials, drm_property_unreference_blob() is NULL safe and > NULL is passed consistently to it throughout drm_atomic.c so do so here. > > Reported-by: Felix Monninger <felix.monninger@gmail.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98420 > Signed-off-by: Felix Monninger <felix.monninger@gmail.com> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: stable@vger.kernel.org Fixes: 5488dc16fde7 ("drm: introduce pipe color correction properties") Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> The single return path bikeshed looked OK to me too, so my r-b can be applied there as well, if people prefer that version. > --- > drivers/gpu/drm/drm_atomic.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 1b5a32df9a9a..e0760c138355 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -416,18 +416,21 @@ drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc, > ssize_t expected_size, > bool *replaced) > { > - struct drm_device *dev = crtc->dev; > struct drm_property_blob *new_blob = NULL; > > if (blob_id != 0) { > - new_blob = drm_property_lookup_blob(dev, blob_id); > + new_blob = drm_property_lookup_blob(crtc->dev, blob_id); > if (new_blob == NULL) > return -EINVAL; > - if (expected_size > 0 && expected_size != new_blob->length) > + > + if (expected_size > 0 && expected_size != new_blob->length) { > + drm_property_unreference_blob(new_blob); > return -EINVAL; > + } > } > > drm_atomic_replace_property_blob(blob, new_blob, replaced); > + drm_property_unreference_blob(new_blob); > > return 0; > } > -- > 2.10.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm: Release reference from blob lookup after replacing property 2016-10-26 7:48 ` Ville Syrjälä @ 2016-10-26 8:29 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2016-10-26 8:29 UTC (permalink / raw) To: Ville Syrjälä; +Cc: intel-gfx, Felix Monninger, dri-devel On Wed, Oct 26, 2016 at 10:48:07AM +0300, Ville Syrjälä wrote: > On Tue, Oct 25, 2016 at 10:28:08PM +0100, Chris Wilson wrote: > > From: Felix Monninger <felix.monninger@gmail.com> > > > > drm_property_lookup_blob() returns a reference to the returned blob, and > > drm_atomic_replace_property_blob() takes a references to the blob it > > stores, so afterwards we are left owning a reference to the new_blob that > > we never release, and thus leak memory every time we update a property > > such as during drm_atomic_helper_legacy_gamma_set(). > > > > v2: update credentials, drm_property_unreference_blob() is NULL safe and > > NULL is passed consistently to it throughout drm_atomic.c so do so here. > > > > Reported-by: Felix Monninger <felix.monninger@gmail.com> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98420 > > Signed-off-by: Felix Monninger <felix.monninger@gmail.com> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: stable@vger.kernel.org > Fixes: 5488dc16fde7 ("drm: introduce pipe color correction properties") > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The single return path bikeshed looked OK to me too, so my r-b can > be applied there as well, if people prefer that version. Applied to drm-misc-fixes (yes I'm trying out something new). -Daniel > > > --- > > drivers/gpu/drm/drm_atomic.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 1b5a32df9a9a..e0760c138355 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -416,18 +416,21 @@ drm_atomic_replace_property_blob_from_id(struct drm_crtc *crtc, > > ssize_t expected_size, > > bool *replaced) > > { > > - struct drm_device *dev = crtc->dev; > > struct drm_property_blob *new_blob = NULL; > > > > if (blob_id != 0) { > > - new_blob = drm_property_lookup_blob(dev, blob_id); > > + new_blob = drm_property_lookup_blob(crtc->dev, blob_id); > > if (new_blob == NULL) > > return -EINVAL; > > - if (expected_size > 0 && expected_size != new_blob->length) > > + > > + if (expected_size > 0 && expected_size != new_blob->length) { > > + drm_property_unreference_blob(new_blob); > > return -EINVAL; > > + } > > } > > > > drm_atomic_replace_property_blob(blob, new_blob, replaced); > > + drm_property_unreference_blob(new_blob); > > > > return 0; > > } > > -- > > 2.10.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-26 8:29 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-10-25 19:46 [PATCH] drm: Release reference from blob lookup after replacing property Chris Wilson 2016-10-25 21:27 ` [Intel-gfx] " Sean Paul 2016-10-25 21:45 ` Chris Wilson 2016-10-25 21:28 ` [PATCH v2] " Chris Wilson 2016-10-26 7:48 ` Ville Syrjälä 2016-10-26 8:29 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox