[PATCH] RFCv2: omapdrm DRM/KMS driver for TI OMAP platforms
Rob Clark
rob.clark at linaro.org
Thu Sep 22 13:21:59 PDT 2011
On Wed, Sep 21, 2011 at 5:32 PM, Konrad Rzeszutek Wilk
<konrad.wilk at oracle.com> wrote:
> So I did a cursory look and stopped at omap_drv.h
> due to the lack of time.
>
> Some comments below.
>
>> +++ b/drivers/staging/omapdrm/Kconfig
>> @@ -0,0 +1,24 @@
>> +
>> +config DRM_OMAP
>> + tristate "OMAP DRM (EXPERIMENTAL)"
>> + depends on DRM && !CONFIG_FB_OMAP2
>
> Since it says EXPERIMENTAL should it depend on that config option as well?
>
> The CONFIG_EXPERIMENTAL one.
Well.. I guess it is already in staging. Not sure if that is
redundant to be both staging and experimental. I'll just remove the
text, it is somewhat less experimental now than when I first started
(and wrote the Kconfig file)
>
>> + select DRM_KMS_HELPER
>> + select OMAP2_DSS
>> + select FB_SYS_FILLRECT
>> + select FB_SYS_COPYAREA
>> + select FB_SYS_IMAGEBLIT
>> + select FB_SYS_FOPS
>> + default n
>> + help
>> + DRM display driver for OMAP2/3/4 based boards.
>> +
>> +config DRM_OMAP_NUM_CRTCS
>> + int "Number of CRTCs"
>> + range 1 10
>> + default 1 if ARCH_OMAP2 || ARCH_OMAP3
>> + default 2 if ARCH_OMAP4
>> + depends on DRM_OMAP
>> + help
>> + Select the number of video overlays which can be used as framebuffers.
>> + The remaining overlays are reserved for video.
>> +
[snip]
>> +struct omap_crtc {
>> + struct drm_crtc base;
>> + struct omap_overlay *ovl;
>
> Where is this defined?
in include/video/omapdss.h
>> + struct omap_overlay_info info;
>> + int id;
>> +
>> + /* if there is a pending flip, this will be non-null: */
>> + struct drm_pending_vblank_event *event;
>> +};
>> +
>> +/* push changes down to dss2 */
>> +static int commit(struct drm_crtc *crtc)
>> +{
>> + struct drm_device *dev = crtc->dev;
>> + struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> + struct omap_overlay *ovl = omap_crtc->ovl;
>> + struct omap_overlay_info *info = &omap_crtc->info;
>> + int ret;
>> +
>> + DBG("%s", omap_crtc->ovl->name);
>> + DBG("%dx%d -> %dx%d (%d)", info->width, info->height, info->out_width,
>> + info->out_height, info->screen_width);
>> + DBG("%d,%d %p %08x", info->pos_x, info->pos_y, info->vaddr,
>> + info->paddr);
>> +
>> + /* NOTE: do we want to do this at all here, or just wait
>> + * for dpms(ON) since other CRTC's may not have their mode
>> + * set yet, so fb dimensions may still change..
>> + */
>> + ret = ovl->set_overlay_info(ovl, info);
>
> Any chance that olv->set_overlayinfo could be NULL?
I don't believe so.. actually, I'm not entirely sure why it needs to
be a fxn ptr, it seems to always be the same fxn. But it is
unconditionally populated when the omap_overlay object is constructed.
(See dss_init_overlays() in drivers/video/omap2/dss/overlay.c)
[snip]
>> +/* update parameters that are dependent on the framebuffer dimensions and
>> + * position within the fb that this crtc scans out from. This is called
>> + * when framebuffer dimensions or x,y base may have changed, either due
>> + * to our mode, or a change in another crtc that is scanning out of the
>> + * same fb.
>> + */
>> +static void update_scanout(struct drm_crtc *crtc)
>> +{
>> + struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> + dma_addr_t paddr;
>> + void __iomem *vaddr;
>> + int screen_width;
>
> Not unsigned int? Can widths become negative?
I suppose not ;-)
>> +
>> + omap_framebuffer_get_buffer(crtc->fb, crtc->x, crtc->y,
>> + &vaddr, &paddr, &screen_width);
>> +
>> + DBG("%s: %d,%d: %p %08x (%d)", omap_crtc->ovl->name,
>> + crtc->x, crtc->y, vaddr, (u32)paddr, screen_width);
>
> So.. never going to run this on a box with more than 4GB? Perhaps
> the (u32) casting should be dropped.
>
> Does it make sense to report also the return value from omap_framebuffer_get_buffer?
it is just returning the buffer size.. which is needed in another
place that calls get_buffer(), but not here
>> +
>> + omap_crtc->info.paddr = paddr;
>> + omap_crtc->info.vaddr = vaddr;
>> + omap_crtc->info.screen_width = screen_width;
>> +}
>> +
>> +static void omap_crtc_gamma_set(struct drm_crtc *crtc,
>> + u16 *red, u16 *green, u16 *blue, uint32_t start, uint32_t size)
>> +{
>> + struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> + DBG("%s", omap_crtc->ovl->name);
>> +}
>> +
>> +static void omap_crtc_destroy(struct drm_crtc *crtc)
>> +{
>> + struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> + DBG("%s", omap_crtc->ovl->name);
>> + drm_crtc_cleanup(crtc);
>> + kfree(omap_crtc);
>> +}
>> +
>> +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);
>> + omap_crtc->info.enabled = true;
>> + } else {
>> + omap_crtc->info.enabled = false;
>> + }
>> +
>> + commit(crtc);
>
> So no checking of its return value.. Should you at least wrap
> it with WARN_ON(?)
Is it safe to rely on the "payload" of the WARN_ON() always being
evaluated, or is there any scenario that you could have something like
#define WARN_ON(X)
ie., is this safe:
WARN_ON(commit(crtc));
it looked like different archs can provide their own WARN_ON, so
wasn't sure how much to trust it..
[snip]
>> +static void page_flip_cb(void *arg)
>> +{
>> + struct drm_crtc *crtc = arg;
>> + struct drm_device *dev = crtc->dev;
>> + struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>> + struct drm_pending_vblank_event *event = omap_crtc->event;
>> + struct timeval now;
>> + unsigned long flags;
>> +
>> + WARN_ON(!event);
>> +
>> + omap_crtc->event = NULL;
>> +
>> + update_scanout(crtc);
>> + commit(crtc);
>> +
>> + /* wakeup userspace */
>> + // TODO: this should happen *after* flip.. somehow..
>> + if (event) {
>> + spin_lock_irqsave(&dev->event_lock, flags);
>
> So this can be called from an IRQ handler? Why the need to disable
> the IRQs? Can you just use spin_lock?
not currently.. OTOH, it should be moved somewhere that would be
called from an IRQ..
>> + event->event.sequence =
>> + drm_vblank_count_and_time(dev, omap_crtc->id, &now);
>> + event->event.tv_sec = now.tv_sec;
>> + event->event.tv_usec = now.tv_usec;
>> + list_add_tail(&event->base.link,
>> + &event->base.file_priv->event_list);
>> + wake_up_interruptible(&event->base.file_priv->event_wait);
>> + spin_unlock_irqrestore(&dev->event_lock, flags);
>> + }
>> +}
>> +
[snip]
>> +
>> +/* keep track of whether we are already loaded.. we may need to call
>> + * plugin's load() if they register after we are already loaded
>> + */
>> +static bool loaded = false;
>
> Add __read_mostly.. You don't need to set false as by default it will
> be 0 (false).
>
>> +
>> +/*
>> + * mode config funcs
>> + */
>> +
>> +/* Notes about mapping DSS and DRM entities:
>> + * CRTC: overlay
>> + * encoder: manager.. with some extension to allow one primary CRTC
>> + * and zero or more video CRTC's to be mapped to one encoder?
>> + * connector: dssdev.. manager can be attached/detached from different
>> + * devices
>> + */
>> +
>> +static void omap_fb_output_poll_changed(struct drm_device *dev)
>> +{
>> + struct omap_drm_private *priv = dev->dev_private;
>> + DBG("dev=%p", dev);
>> + if (priv->fbdev) {
>> + drm_fb_helper_hotplug_event(priv->fbdev);
>> + }
>> +}
>> +
>> +static struct drm_mode_config_funcs omap_mode_config_funcs = {
>> + .fb_create = omap_framebuffer_create,
>> + .output_poll_changed = omap_fb_output_poll_changed,
>> +};
>> +
>> +static int get_connector_type(struct omap_dss_device *dssdev)
>> +{
>> + switch (dssdev->type) {
>> + case OMAP_DISPLAY_TYPE_HDMI:
>> + return DRM_MODE_CONNECTOR_HDMIA;
>> + case OMAP_DISPLAY_TYPE_DPI:
>> + if (!strcmp(dssdev->name, "dvi"))
>
> strncmp
In this case, since one of the args is a string literal, I know it is
properly terminated..
(or, well, unless it is possible to corrupt .text/.rodata.. although
in that case your n value is probably also corrupted and you are more
or less up s**t creek..)
>
>> + return DRM_MODE_CONNECTOR_DVID;
>> + default:
>> + return DRM_MODE_CONNECTOR_Unknown;
>> + }
>> +}
>> +
>> +#if 0 /* enable when dss2 supports hotplug */
>> +static int omap_drm_notifier(struct notifier_block *nb,
>> + unsigned long evt, void *arg)
>> +{
>> + switch (evt) {
>> + case OMAP_DSS_SIZE_CHANGE:
>> + case OMAP_DSS_HOTPLUG_CONNECT:
>> + case OMAP_DSS_HOTPLUG_DISCONNECT: {
>> + struct drm_device *dev = drm_device;
>> + DBG("hotplug event: evt=%d, dev=%p", evt, dev);
>> + if (dev) {
>> + drm_sysfs_hotplug_event(dev);
>> + }
>> + return NOTIFY_OK;
>> + }
>> + default: /* don't care about other events for now */
>> + return NOTIFY_DONE;
>> + }
>> +}
>> +#endif
>> +
>> +static void dump_video_chains(void)
>> +{
>> + int i;
>> +
>> + DBG("dumping video chains: ");
>> + for (i = 0; i < omap_dss_get_num_overlays(); i++) {
>
> Any chance omap_dss_get_num_overlays can return a negative value?
no
>> + struct omap_overlay *ovl = omap_dss_get_overlay(i);
>
> and if so, this could cause this guy to explode.
>> + struct omap_overlay_manager *mgr = ovl->manager;
>> + struct omap_dss_device *dssdev = mgr ? mgr->device : NULL;
>> + if (dssdev) {
>> + DBG("%d: %s -> %s -> %s", i, ovl->name, mgr->name,
>> + dssdev->name);
>> + } else if (mgr) {
>> + DBG("%d: %s -> %s", i, ovl->name, mgr->name);
>> + } else {
>> + DBG("%d: %s", i, ovl->name);
>> + }
>> + }
>> +}
>> +
>> +static int omap_modeset_init(struct drm_device *dev)
>> +{
>> + const struct omap_drm_platform_data *pdata = dev->dev->platform_data;
>> + struct omap_drm_private *priv = dev->dev_private;
>> + struct omap_dss_device *dssdev = NULL;
>> + int i, j;
>> + unsigned int connected_connectors = 0;
>> +
>> + /* create encoders for each manager */
>> + int create_encoder(int i) {
>> + struct omap_overlay_manager *mgr =
>> + omap_dss_get_overlay_manager(i);
>> + struct drm_encoder *encoder = omap_encoder_init(dev, mgr);
>> +
>> + if (!encoder) {
>> + dev_err(dev->dev, "could not create encoder\n");
>> + return -ENOMEM;
>> + }
>> +
>> + priv->encoders[priv->num_encoders++] = encoder;
>
> Should you check priv->num_encoders to be sure you are not
> referencing past the priv->encoders size? Or if num_encoders is -1 then
> hitting some other code and potentially causing a security hole.
>
currently the array size for crtc/encoder/connectors is ~2x the # of
corresponding physical resources. But I guess it doesn't hurt to have
some BUG_ON()'s..
>> +
>> + return 0;
>> + }
>> +
>> + /* create connectors for each display device */
>> + int create_connector(struct omap_dss_device *dssdev) {
>> + static struct notifier_block *notifier;
>> + struct drm_connector *connector;
>> +
>> + if (!dssdev->driver) {
>> + dev_warn(dev->dev, "%s has no driver.. skipping it\n",
>> + dssdev->name);
>> + return 0;
>> + }
>> +
>> + if (!(dssdev->driver->get_timings ||
>> + dssdev->driver->read_edid)) {
>> + dev_warn(dev->dev, "%s driver does not support "
>> + "get_timings or read_edid.. skipping it!\n",
>> + dssdev->name);
>> + return 0;
>> + }
>> +
>> + connector = omap_connector_init(dev,
>> + get_connector_type(dssdev), dssdev);
>> +
>> + if (!connector) {
>> + dev_err(dev->dev, "could not create connector\n");
>> + return -ENOMEM;
>> + }
>> +
>> + /* track what is already connected.. rather than looping thru
>> + * all connectors twice later, first for connected then for
>> + * remainder (which could be a race condition if connected
>> + * status changes)
>> + */
>> + if (omap_connector_detect(connector, true) ==
>> + connector_status_connected) {
>> + connected_connectors |= (1 << priv->num_connectors);
>> + }
>> +
>> + priv->connectors[priv->num_connectors++] = connector;
>
> Ditto on this check?
>> +
>> +#if 0 /* enable when dss2 supports hotplug */
>> + notifier = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
>> + notifier->notifier_call = omap_drm_notifier;
>> + omap_dss_add_notify(dssdev, notifier);
>> +#else
>> + notifier = NULL;
>> +#endif
>> +
>> + for (j = 0; j < priv->num_encoders; j++) {
>> + struct omap_overlay_manager *mgr =
>> + omap_encoder_get_manager(priv->encoders[j]);
>> + if (mgr->device == dssdev) {
>> + drm_mode_connector_attach_encoder(connector,
>> + priv->encoders[j]);
>> + }
>> + }
>> +
>> + return 0;
>> + }
>> +
>> + /* create up to max_overlays CRTCs mapping to overlays.. by default,
>> + * connect the overlays to different managers/encoders, giving priority
>> + * to encoders connected to connectors with a detected connection
>> + */
>> + int create_crtc(int i) {
>> + struct omap_overlay *ovl = omap_dss_get_overlay(i);
>> + struct omap_overlay_manager *mgr = NULL;
>> + struct drm_crtc *crtc;
>> +
>> + if (ovl->manager) {
>> + DBG("disconnecting %s from %s", ovl->name,
>> + ovl->manager->name);
>> + ovl->unset_manager(ovl);
>> + }
>> +
>> + /* find next best connector, ones with detected connection first
>> + */
>> + while (j < priv->num_connectors && !mgr) {
>> + if (connected_connectors & (1 << j)) {
>> + struct drm_encoder * encoder =
>> + omap_connector_attached_encoder(
>> + priv->connectors[j]);
>> + if (encoder) {
>> + mgr = omap_encoder_get_manager(encoder);
>> + }
>> + }
>> + j++;
>> + }
>> +
>> + /* if we couldn't find another connected connector, lets start
>> + * looking at the unconnected connectors:
>> + */
>> + while (j < 2 * priv->num_connectors && !mgr) {
>> + int idx = j - priv->num_connectors;
>
> You might want to use:
> unsigned int idx = max(0, j - priv->num_connectors);
>
> jsut ot make sure you don't go negative.
It can't (currently) happen, because you need to go all the way thru
the first loop to enter the second loop. Although maybe that bit of
code is a bit less than self-apparent.. but I hadn't thought of a way
to simplify it.
>
>> + if (!(connected_connectors & (1 << idx))) {
>> + struct drm_encoder * encoder =
>> + omap_connector_attached_encoder(
>> + priv->connectors[idx]);
>
> .. and reference in the unknown memory location.
>
>> + if (encoder) {
>> + mgr = omap_encoder_get_manager(encoder);
>> + }
>> + }
>> + j++;
>> + }
>> +
>> + if (mgr) {
>> + DBG("connecting %s to %s", ovl->name, mgr->name);
>> + ovl->set_manager(ovl, mgr);
>> + }
>> +
>> + crtc = omap_crtc_init(dev, ovl, priv->num_crtcs);
>> +
>> + if (!crtc) {
>> + dev_err(dev->dev, "could not create CRTC\n");
>> + return -ENOMEM;
>> + }
>> +
>> + priv->crtcs[priv->num_crtcs++] = crtc;
>> +
>> + return 0;
>> + }
>> +
>> + drm_mode_config_init(dev);
>> +
>> + if (pdata) {
>> + /* if platform data is provided by the board file, use it to
>> + * control which overlays, managers, and devices we own.
>> + */
>> + for (i = 0; i < pdata->mgr_cnt; i++) {
>> + create_encoder(pdata->mgr_ids[i]);
>
> No check for return value? What if we get -ENOMEM?
I'm a bit undecided on some of this error handling at startup.. I
guess ENOMEM is clear enough. But some of the other parts, like
connector initialization, could fail just because some hw is not
present/populated. Like missing some LCD. I guess that it is best to
try and limp along as best as possible and hope to get some pixels on
some screen that the user can see. If you give up and all the
screens/monitors/etc stay dark, it might be a bit inconvenient to
debug..
BR,
-R
>> + }
>> +
>> + for (i = 0; i < pdata->dev_cnt; i++) {
>> + int m(struct omap_dss_device *dssdev, void *data) {
>> + return ! strcmp(dssdev->name, data);
>> + }
>> + struct omap_dss_device *dssdev =
>> + omap_dss_find_device(
>> + (void *)pdata->dev_names[i], m);
>> + if (!dssdev) {
>> + dev_warn(dev->dev, "no such dssdev: %s\n",
>> + pdata->dev_names[i]);
>> + continue;
>> + }
>> + create_connector(dssdev);
>> + }
>> +
>> + j = 0;
>> + for (i = 0; i < pdata->ovl_cnt; i++) {
>> + create_crtc(pdata->ovl_ids[i]);
>> + }
>> + } else {
>> + /* otherwise just grab up to CONFIG_DRM_OMAP_NUM_CRTCS and try
>> + * to make educated guesses about everything else
>> + */
>> + int max_overlays = min(omap_dss_get_num_overlays(),
>> + CONFIG_DRM_OMAP_NUM_CRTCS);
>> +
>> + for (i = 0; i < omap_dss_get_num_overlay_managers(); i++) {
>> + create_encoder(i);
>> + }
>> +
>> + for_each_dss_dev(dssdev) {
>> + create_connector(dssdev);
>> + }
>> +
>> + j = 0;
>> + for (i = 0; i < max_overlays; i++) {
>> + create_crtc(i);
>> + }
>> + }
>> +
>> + /* for now keep the mapping of CRTCs and encoders static.. */
>> + for (i = 0; i < priv->num_encoders; i++) {
>> + struct drm_encoder *encoder = priv->encoders[i];
>> + struct omap_overlay_manager *mgr =
>> + omap_encoder_get_manager(encoder);
>> +
>> + encoder->possible_crtcs = 0;
>> +
>> + for (j = 0; j < priv->num_crtcs; j++) {
>> + struct omap_overlay *ovl =
>> + omap_crtc_get_overlay(priv->crtcs[j]);
>> + if (ovl->manager == mgr) {
>> + encoder->possible_crtcs |= (1 << j);
>> + }
>> + }
>> +
>> + DBG("%s: possible_crtcs=%08x", mgr->name,
>> + encoder->possible_crtcs);
>> + }
>> +
>> + dump_video_chains();
>> +
>> + dev->mode_config.min_width = 256;
>> + dev->mode_config.min_height = 256;
>> +
>> + /* note: pvr can't currently handle dst surfaces larger than 2k by 2k */
>> + dev->mode_config.max_width = 2048;
>> + dev->mode_config.max_height = 2048;
>> +
>> + dev->mode_config.funcs = &omap_mode_config_funcs;
>> +
>> + return 0;
>> +}
>> +
More information about the dri-devel
mailing list