On Mon, Sep 19, 2011 at 2:44 AM, Inki Dae <inki.dae@xxxxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
