Hello Rob.
Below is my comments.

-----Original Message-----
From: Rob Clark [mailto:robdclark at gmail.com]
Sent: Friday, September 02, 2011 10:18 AM
To: Inki Dae
Cc: airlied at linux.ie; dri-devel at lists.freedesktop.org;
sw0312.kim at samsung.com; linux-kernel at vger.kernel.org;
kyungmin.park at samsung.com; linux-arm-kernel at 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 [off-list ref] wrote:

quoted

quoted

quoted

quoted

quoted

+struct samsung_drm_gem_obj *
+ ? ? ? ? ? ? ? find_samsung_drm_gem_object(struct drm_file

*file_priv,

quoted

quoted

quoted

quoted

+ ? ? ? ? ? ? ? ? ? ? ? struct drm_device *dev, unsigned int

handle)

quoted

quoted

quoted

quoted

quoted

+{
+ ? ? ? 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

quoted

quoted

quoted

quoted

lookup.\n");

quoted

+ ? ? ? ? ? ? ? return NULL;
+ ? ? ? }
+
+ ? ? ? /**
+ ? ? ? ?* unreference refcount of the gem object.
+ ? ? ? ?* at drm_gem_object_lookup(), the gem object was

referenced.

quoted

quoted

quoted

quoted

quoted

+ ? ? ? ?*/
+ ? ? ? 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

quoted

found then drm_gem_object_reference is called to increase refcount of

this

quoted

object. if there is any missing point, give me any comment please.

thank

quoted

quoted

quoted

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

quoted

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

quoted

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