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

[Konrad Rzeszutek Wilk]

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

Hmm, good question. I assumed so, but you got me thinking.
> 
>   #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..

Ok, so laying the ground work for it. That is OK - you might want to
mention that in the TODO just in case .

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

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

Thank you.

.. snip..
> >> +             /* 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.

A think expanding on the comment will suffice.
.. snip..
> >> +             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..

Perhaps then just do WARN_ON, like:

if (create_encode(..))
    WARN_ON(1,"Danger Danger! Limping along\n");