- 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");