From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH] RFCv2: omapdrm DRM/KMS driver for TI OMAP platforms Date: Mon, 26 Sep 2011 09:55:44 -0400 Message-ID: <20110926135544.GA4102@phenom.oracle.com> References: <1316295129-3600-1-git-send-email-rob.clark@linaro.org> <20110921223210.GA26166@phenom.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from rcsinet15.oracle.com (rcsinet15.oracle.com [148.87.113.117]) by gabe.freedesktop.org (Postfix) with ESMTP id BE1CC9E76B for ; Mon, 26 Sep 2011 06:56:01 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Rob Clark Cc: Inki Dae , dri-devel@lists.freedesktop.org, patches@linaro.org List-Id: dri-devel@lists.freedesktop.org > >> + =A0 =A0 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) > >> +{ > >> + =A0 =A0 struct drm_crtc *crtc =3D arg; > >> + =A0 =A0 struct drm_device *dev =3D crtc->dev; > >> + =A0 =A0 struct omap_crtc *omap_crtc =3D to_omap_crtc(crtc); > >> + =A0 =A0 struct drm_pending_vblank_event *event =3D omap_crtc->event; > >> + =A0 =A0 struct timeval now; > >> + =A0 =A0 unsigned long flags; > >> + > >> + =A0 =A0 WARN_ON(!event); > >> + > >> + =A0 =A0 omap_crtc->event =3D NULL; > >> + > >> + =A0 =A0 update_scanout(crtc); > >> + =A0 =A0 commit(crtc); > >> + > >> + =A0 =A0 /* wakeup userspace */ > >> + =A0 =A0 // TODO: this should happen *after* flip.. somehow.. > >> + =A0 =A0 if (event) { > >> + =A0 =A0 =A0 =A0 =A0 =A0 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 . > = > >> + =A0 =A0 =A0 =A0 =A0 =A0 event->event.sequence =3D > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 drm_vblank_c= ount_and_time(dev, omap_crtc->id, &now); > >> + =A0 =A0 =A0 =A0 =A0 =A0 event->event.tv_sec =3D now.tv_sec; > >> + =A0 =A0 =A0 =A0 =A0 =A0 event->event.tv_usec =3D now.tv_usec; > >> + =A0 =A0 =A0 =A0 =A0 =A0 list_add_tail(&event->base.link, > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 &event->base= .file_priv->event_list); > >> + =A0 =A0 =A0 =A0 =A0 =A0 wake_up_interruptible(&event->base.file_priv= ->event_wait); > >> + =A0 =A0 =A0 =A0 =A0 =A0 spin_unlock_irqrestore(&dev->event_lock, fla= gs); > >> + =A0 =A0 } > >> +} > >> + > = > [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 =3D 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: > >> + * =A0 =A0CRTC: =A0 =A0 =A0 =A0overlay > >> + * =A0 =A0encoder: =A0 =A0 manager.. with some extension to allow one= primary CRTC > >> + * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 and zero or more video CRTC's to b= e mapped to one encoder? > >> + * =A0 =A0connector: =A0 dssdev.. manager can be attached/detached fr= om different > >> + * =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 devices > >> + */ > >> + > >> +static void omap_fb_output_poll_changed(struct drm_device *dev) > >> +{ > >> + =A0 =A0 struct omap_drm_private *priv =3D dev->dev_private; > >> + =A0 =A0 DBG("dev=3D%p", dev); > >> + =A0 =A0 if (priv->fbdev) { > >> + =A0 =A0 =A0 =A0 =A0 =A0 drm_fb_helper_hotplug_event(priv->fbdev); > >> + =A0 =A0 } > >> +} > >> + > >> +static struct drm_mode_config_funcs omap_mode_config_funcs =3D { > >> + =A0 =A0 .fb_create =3D omap_framebuffer_create, > >> + =A0 =A0 .output_poll_changed =3D omap_fb_output_poll_changed, > >> +}; > >> + > >> +static int get_connector_type(struct omap_dss_device *dssdev) > >> +{ > >> + =A0 =A0 switch (dssdev->type) { > >> + =A0 =A0 case OMAP_DISPLAY_TYPE_HDMI: > >> + =A0 =A0 =A0 =A0 =A0 =A0 return DRM_MODE_CONNECTOR_HDMIA; > >> + =A0 =A0 case OMAP_DISPLAY_TYPE_DPI: > >> + =A0 =A0 =A0 =A0 =A0 =A0 if (!strcmp(dssdev->name, "dvi")) > > > > strncmp > = > In this case, since one of the args is a string literal, I know it is > properly terminated.. > > > > 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.. > >> + =A0 =A0 =A0 =A0 =A0 =A0 /* if we couldn't find another connected con= nector, lets start > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0* looking at the unconnected connectors: > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0*/ > >> + =A0 =A0 =A0 =A0 =A0 =A0 while (j < 2 * priv->num_connectors && !mgr)= { > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int idx =3D j - priv->num_co= nnectors; > > > > You might want to use: > > =A0 =A0unsigned int idx =A0=3D max(0, =A0j - 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.. > >> + =A0 =A0 =A0 =A0 =A0 =A0 for (i =3D 0; i < pdata->mgr_cnt; i++) { > >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 create_encoder(pdata->mgr_id= s[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");