Hello Rob. Below is my comments.
-----Original Message----- From: Rob Clark [mailto:robdclark@gmail.com] Sent: Friday, September 02, 2011 10:18 AM To: Inki Dae Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org; sw0312.kim@samsung.com; linux-kernel@vger.kernel.org; kyungmin.park@samsung.com; linux-arm-kernel@lists.infradead.org Subject: Re: [RFC][PATCH v3] DRM: add DRM Driver for Samsung SoC EXYNOS4210.
On Thu, Sep 1, 2011 at 8:06 AM, Inki Dae inki.dae@samsung.com wrote:
+struct samsung_drm_gem_obj *
- find_samsung_drm_gem_object(struct drm_file
*file_priv,
- struct drm_device *dev, unsigned int
handle)
+{
- struct drm_gem_object *gem_obj;
- gem_obj = drm_gem_object_lookup(dev, file_priv, handle);
- if (!gem_obj) {
- DRM_LOG_KMS("a invalid gem object not registered
to
lookup.\n");
- return NULL;
- }
- /**
- * unreference refcount of the gem object.
- * at drm_gem_object_lookup(), the gem object was
referenced.
- */
- drm_gem_object_unreference(gem_obj);
this doesn't seem right, to drop the reference before you use the buffer elsewhere..
No, see drm_gem_object_lookup fxn. at this function, if there is a
object
found then drm_gem_object_reference is called to increase refcount of
this
object. if there is any missing point, give me any comment please.
thank
you.
Right, but I think there is a reason it takes a reference... so that the object doesn't get free'd from under your feet. So pattern should, I think, be:
obj = lookup(...); ... do stuff w/ obj ... unreference(obj)
so the caller who is using the looked up obj should unref it when done
Instead, you have:
obj = lookup(...); unreference(obj); ... do stuff w/ obj ...
Generally right, but in this case, it is just used to get specific gem object through find_samsung_drm_gem_object() so doesn't reference this
gem
object anywhere. therefore reference and unreference should be done within find_samsung_drm_gem_object(). if there is any point I missed then let
me
know please. thank you.
Still, it seems like find_samsung_drm_gem_object() is encouraging the wrong usage-pattern, even if it works fine today because you know somewhere else is holding a reference to the object. Later if you expand your use of GEM objects, this fxn might come back to bite you. There is a good reason that drm_gem_object_lookup() takes a reference to the object, and it feels wrong to intentionally subvert that.
(I'm perfectly willing to be overridden on the subject.. there are plenty of folks on this list who have been doing the GEM thing longer than I have. But it just seems better to use APIs like drm_gem_object_lookup() the way they were intended.)
Ah, you are right. I misunderstanded it. as you pointed out, a gem object should be unreferenced after doing something with the gem object. so I will remove find_samsung_drm_gem_object() and use drm_gem_object_lookup() directly to get a gem object instead. of course, the gem object will be unreferenced after doing something with it. thank you for your explanation. :)
BR, -R