Message-ID: <CAF6AEGsqbFcS_QwihrjpVLQK1Y+h1iJSyu+GFV0Govp3XHYbOw@mail.gmail.com>
Date:	Wed, 31 Aug 2011 11:02:07 -0500
From:	Rob Clark <robdclark@...il.com>
To:	Inki Dae <inki.dae@...sung.com>
Cc:	airlied@...ux.ie, dri-devel@...ts.freedesktop.org,
	sw0312.kim@...sung.com, linux-kernel@...r.kernel.org,
	kyungmin.park@...sung.com, linux-arm-kernel@...ts.infradead.org
Subject: Re: [RFC][PATCH v3] DRM: add DRM Driver for Samsung SoC EXYNOS4210.

On Wed, Aug 31, 2011 at 1:51 AM, Inki Dae <inki.dae@...sung.com> wrote:
> Hello, Rob.
> Below is my answers and questions. and could you please include me as CC
> when you post your driver?

sure thing


>> > +static int samsung_drm_connector_get_modes(struct drm_connector
>> *connector)
>> > +{
>> > +       struct samsung_drm_connector *samsung_connector =
>> > +               to_samsung_connector(connector);
>> > +       struct samsung_drm_display *display =
>> > +               samsung_drm_get_manager(samsung_connector->encoder)-
>> >display;
>> > +       unsigned int count;
>> > +
>> > +       DRM_DEBUG_KMS("%s\n", __FILE__);
>> > +
>> > +       if (display && display->get_edid) {
>> > +               void *edid = kzalloc(MAX_EDID, GFP_KERNEL);
>> > +               if (!edid) {
>> > +                       DRM_ERROR("failed to allocate edid\n");
>> > +                       return 0;
>> > +               }
>> > +
>> > +               display->get_edid(connector, edid, MAX_EDID);
>> > +
>> > +               drm_mode_connector_update_edid_property(connector,
> edid);
>> > +               count = drm_add_edid_modes(connector, edid);
>> > +
>> > +               kfree(connector->display_info.raw_edid);
>> > +               connector->display_info.raw_edid = edid;
>> > +       } else {
>> > +               struct drm_display_mode *mode =
> drm_mode_create(connector-
>> >dev);
>> > +               struct fb_videomode *timing;
>> > +
>> > +               if (display && display->get_timing)
>> > +                       timing = display->get_timing();
>>
>> also seems a bit weird to not do: display->get_timings(display)..
>>
>> maybe you don't have the case of multiple instances of the same
>> display time.. but maybe someday you need that.  (Maybe this is just
>> an artifact of how the API of your current lower layer driver is.. but
>> maybe now is a good time to think about those APIs)
>>
>
> display is static object set by hardware specific driver such as display
> controller or hdmi. each hardware specific driver has their own display
> object statically. You can refer to the definition in samsung_drm_fimd.c.
> and I understood a encoder and a connector is 1:1 relationship, when any
> hardware specific driver is probed, they would be created with a manager
> that includes their own display object as pointer. Could you please give me
> more comments why display object should be considered for multiple
> instances? I am afraid there is any part I don't care.
>
> thank you.

Just thinking hypothetically.. what if some future device had two hdmi
controllers.  Then you'd want two instances of the same display
object.

Although it seems this API is just internal to the DRM driver (which I
had not realized earlier), so it should be pretty easy to change later
if needed.


>> > +static void samsung_drm_connector_destroy(struct drm_connector
>> *connector)
>> > +{
>> > +       struct samsung_drm_connector *samsung_connector =
>> > +               to_samsung_connector(connector);
>> > +
>> > +       DRM_DEBUG_KMS("%s\n", __FILE__);
>> > +
>> > +       drm_sysfs_connector_remove(connector);
>> > +       drm_connector_cleanup(connector);
>> > +       connector->dev->mode_config.num_connector--;
>>
>> I wonder if num_connector-- should be in drm_connector_cleanup()..
>>
>> I missed this in OMAP driver, but it seems that none of the other drm
>> drivers are directly decrementing this.. and it would seem more
>> symmetric for it to be in drm_connector_cleanup().  But would be
>
> For this, it was already commented by Joonyoun Shim.

And it seems (which I hadn't noticed earlier) already merged :-)

So I think this (and similar bit in encoder destructor) can be removed


>> > +static void samsung_drm_fb_destroy(struct drm_framebuffer *fb)
>> > +{
>> > +       struct samsung_drm_fb *samsung_fb = to_samsung_fb(fb);
>> > +       int ret;
>> > +
>> > +       DRM_DEBUG_KMS("%s\n", __FILE__);
>> > +
>> > +       drm_framebuffer_cleanup(fb);
>> > +
>> > +       if (samsung_fb->is_default) {
>> > +               ret = drm_gem_handle_delete(samsung_fb->file_priv,
>> > +                               samsung_fb->gem_handle);
>>
>> why not keep the gem buffer ptr, and do something like:
>>
>>   drm_gem_object_unreference_unlocked(samsung_fb->bo)..
>>
>> this way, you get the right behavior if someone somewhere else took a
>> ref to the gem buffer object?  And it avoids needing to keep the
>> file_priv ptr in the fb (which seems a bit strange)
>>
>>
> Yes, at booting time, one gem object is created. this is for linux
> framebuffer and used as default buffer. register_framebuffer function is
> called one time at booting time by drm driver. but when this driver is built
> as modules, this gem object should be released with rmmod modules. and the
> reason fb has file point is that drm_gem_handle_delete requests it to
> release a gem object. our driver considered modularization also. If there is
> any point I missed, give me any comment please. Thank you.

Oh, I missed the point that drm_gem_handle_delete() is calling
drm_gem_object_unreference_unlocked().

But this still seems a bit odd, but I guess the difference is you are
keeping the buffer handle, rather than the 'struct drm_gem_object'
ptr.  I'm not sure if there is a point to keeping track of the buffer
in handle form on the kernel side.  If instead you just keep a GEM
object ptr, you can just drm_gem_object_unreference{_unlocked}() when
you are done with it without having to special-case is_default stuff.

>> > +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 ...


BR,
-R
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists