Hello Rob. Below is my comments. > -----Original Message----- > From: Rob Clark [mailto:robdclark@xxxxxxxxx] > Sent: Friday, September 02, 2011 10:18 AM > To: Inki Dae > Cc: airlied@xxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; > sw0312.kim@xxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > kyungmin.park@xxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > 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@xxxxxxxxxxx> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
