On Mon, Sep 19, 2011 at 2:44 AM, Inki Dae inki.dae@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