[PATCH] RFCv2: omapdrm DRM/KMS driver for TI OMAP platforms

[Rob Clark]

On Mon, Sep 19, 2011 at 2:44 AM, Inki Dae <inki.dae at samsung.com> wrote:
> Hello Rob.
>
> Is there some reason that you don't head your driver gpu/drm, just staging?
> and below is my short comments. thank you.
>

mainly because it is still a work-in-progress.. I expect some
re-shuffling of the mode-setting code when drm_plane support is added,
the plugin layer, etc.  And perhaps at some point merging of the
omapdss and omapdrm layers.

I don't expect the ioctl ABI to change, and the driver is useful
as-is, but I still think there is enough changes that will happen that
staging is perhaps the better place for it to start life..


[snip]

>> +/* initialize connector */
>> +struct drm_connector * omap_connector_init(struct drm_device *dev,
>> +             int connector_type, struct omap_dss_device *dssdev)
>> +{
>> +     struct drm_connector *connector = NULL;
>> +     struct omap_connector *omap_connector;
>> +
>> +     DBG("%s", dssdev->name);
>> +
>> +     omap_dss_get_device(dssdev);
>> +
>> +     omap_connector = kzalloc(sizeof(struct omap_connector), GFP_KERNEL);
>> +     if (!omap_connector) {
>> +             dev_err(dev->dev, "could not allocate connector\n");
>> +             goto fail;
>> +     }
>> +
>> +     omap_connector->dssdev = dssdev;
>> +     connector = &omap_connector->base;
>> +
>> +     drm_connector_init(dev, connector, &omap_connector_funcs,
>> +                             connector_type);
>> +     drm_connector_helper_add(connector, &omap_connector_helper_funcs);
>> +
>> +#if 0 /* enable when dss2 supports hotplug */
>> +     if (dssdev->caps & OMAP_DSS_DISPLAY_CAP_HPD)
>> +             connector->polled = 0;
>> +     else
>> +#endif
>
> How about removing the codes above until dss2 supports hotplug?
>

easier not to forget about them this way ;-)

if someone has a strong opinion, I can remove them.. but I guess it
should be not too distant future when support lands in dss2..  I guess
I should put a note in the TODO about re-enabling hotplug code


>> +             connector->polled = DRM_CONNECTOR_POLL_CONNECT |
>> +                             DRM_CONNECTOR_POLL_DISCONNECT;
>> +
>> +     connector->interlace_allowed = 1;
>> +     connector->doublescan_allowed = 0;
>> +
>> +     drm_sysfs_connector_add(connector);
>> +
>> +     return connector;
>> +
>> +fail:
>> +     if (connector) {
>> +             omap_connector_destroy(connector);
>> +     }
>> +
>> +     return NULL;
>> +}

[snip]

>> +static void omap_crtc_dpms(struct drm_crtc *crtc, int mode)
>> +{
>> +     struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> +
>> +     DBG("%s: %d", omap_crtc->ovl->name, mode);
>> +
>> +     if (mode == DRM_MODE_DPMS_ON) {
>> +             update_scanout(crtc);
>
> Is there any reason that update_scanout function should be called here to
> update buffer address.? I think it's enough to call this function at
> mode_set callback.


In theory it should not be needed.. IIRC at some point I was hitting
some problem that the buffer address wasn't set yet, but that was w/
older version of the driver, older kernel, and different xorg driver..


>> +             omap_crtc->info.enabled = true;
>> +     } else {
>> +             omap_crtc->info.enabled = false;
>> +     }
>> +
>> +     commit(crtc);
>
> and commit callback to apply those data on hw.
>
>> +}
>> +

[snip]

>> +
>> +/**
>> + * load - setup chip and create an initial config
>> + * @dev: DRM device
>> + * @flags: startup flags
>> + *
>> + * The driver load routine has to do several things:
>> + *   - initialize the memory manager
>> + *   - allocate initial config memory
>> + *   - setup the DRM framebuffer with the allocated memory
>> + */
>> +static int dev_load(struct drm_device *dev, unsigned long flags)
>> +{
>> +     struct omap_drm_private *priv;
>> +     int ret;
>> +
>> +     DBG("load: dev=%p", dev);
>> +
>> +     drm_device = dev;
>> +
>> +     priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>> +     if (!priv) {
>> +             dev_err(dev->dev, "could not allocate priv\n");
>> +             return -1;
>> +     }
>> +
>> +     dev->dev_private = priv;
>> +
>> +     ret = omap_modeset_init(dev);
>> +     if (ret) {
>> +             dev_err(dev->dev, "omap_modeset_init failed: ret=%d\n",
> ret);
>> +             // hmm
>> +             //return ret;
>
> Doesn't it need return? you output error message using dev_err().

Well.. currently omap_modeset_init() never returns !=0

I'm still a bit undecided on some of the error cases.. for example if
we don't successfully initialize everything, but do at least have >1
of each crtc/encoder/connector, maybe it makes sense to continue in
some reduced capacity..

But "check error handling/cleanup paths" is still in the TODO file ;-)

>> +     }
>> +
>> +     priv->fbdev = omap_fbdev_init(dev);
>> +     if (!priv->fbdev) {
>> +             dev_err(dev->dev, "omap_fbdev_init failed\n");
>> +             ret = -ENOMEM;
>> +             // hmm
>> +             //return ret;
>
> Ditto. and also you would have to release some resources. with
> omap_modeset_init call, connector, encoder, crtc and so on would be created.

Currently we continue on w/out an fbdev.. for X11, that is fine, it
can function without an fbdev device.  So I'm undecided about how
fatal of an error this should be


BR,
-R