> >> + 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");
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
