Re: [PATCH v3 08/20] drm: omapdrm: Handle OCP error IRQ directly

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v3 08/20] drm: omapdrm: Handle OCP error IRQ directly
Date: Tue, 13 Dec 2016 00:09:48 +0200	[thread overview]
Message-ID: <17646775.sZElGbsKsq@avalon> (raw)
In-Reply-To: <ca5fa805-1958-0529-68be-c054ad6da4e0@ti.com>

Hi Tomi,

On Tuesday 20 Sep 2016 16:34:37 Tomi Valkeinen wrote:
> On 19/09/16 15:27, Laurent Pinchart wrote:
> > Instead of going through a complicated registration mechanism, just
> > call the OCP error IRQ handler directly from the main IRQ handler.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> > 
> > - Rename IRQ handler to omap_irq_ocp_error_handler()
> > - Replace hex error value with "OCP error" message
> > ---
> > 
> >  drivers/gpu/drm/omapdrm/omap_drv.h |  1 -
> >  drivers/gpu/drm/omapdrm/omap_irq.c | 28 ++++++++++------------------
> >  2 files changed, 10 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
> > b/drivers/gpu/drm/omapdrm/omap_drv.h index 7a1d8384147c..05375bc40091
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_drv.h
> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.h
> > @@ -103,7 +103,6 @@ struct omap_drm_private {
> > 
> >  	/* irq handling: */
> >  	struct list_head irq_list;    /* list of omap_drm_irq */
> >  	uint32_t irq_mask;		/* enabled irqs in addition to 
irq_list */
> > 
> > -	struct omap_drm_irq error_handler;
> > 
> >  	/* atomic commit */
> >  	struct {
> > 
> > diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c
> > b/drivers/gpu/drm/omapdrm/omap_irq.c index f0f110faf1e6..257c1f3c76f3
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_irq.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_irq.c
> > @@ -21,12 +21,6 @@
> > 
> >  static DEFINE_SPINLOCK(list_lock);
> > 
> > -static void omap_irq_error_handler(struct omap_drm_irq *irq,
> > -		uint32_t irqstatus)
> > -{
> > -	DRM_ERROR("errors: %08x\n", irqstatus);
> > -}
> > -
> > 
> >  /* call with list_lock and dispc runtime held */
> >  static void omap_irq_update(struct drm_device *dev)
> >  {
> > 
> > @@ -224,6 +218,14 @@ static void omap_irq_fifo_underflow(struct
> > omap_drm_private *priv,> 
> >  	pr_cont("(0x%08x)\n", irqstatus);
> >  
> >  }
> > 
> > +static void omap_irq_ocp_error_handler(u32 irqstatus)
> > +{
> > +	if (!(irqstatus & DISPC_IRQ_OCP_ERR))
> > +		return;
> > +
> > +	DRM_ERROR("OCP error\n");
> > +}
> > +
> > 
> >  static irqreturn_t omap_irq_handler(int irq, void *arg)
> >  {
> >  
> >  	struct drm_device *dev = (struct drm_device *) arg;
> > 
> > @@ -250,6 +252,7 @@ static irqreturn_t omap_irq_handler(int irq, void
> > *arg)
> > 
> >  			omap_crtc_error_irq(crtc, irqstatus);
> >  	
> >  	}
> > 
> > +	omap_irq_ocp_error_handler(irqstatus);
> > 
> >  	omap_irq_fifo_underflow(priv, irqstatus);
> >  	
> >  	spin_lock_irqsave(&list_lock, flags);
> > 
> > @@ -282,7 +285,6 @@ static const u32 error_irqs[] = {
> > 
> >  int omap_drm_irq_install(struct drm_device *dev)
> >  {
> >  
> >  	struct omap_drm_private *priv = dev->dev_private;
> > 
> > -	struct omap_drm_irq *error_handler = &priv->error_handler;
> > 
> >  	unsigned int num_mgrs = dss_feat_get_num_mgrs();
> >  	unsigned int max_planes;
> >  	unsigned int i;
> > 
> > @@ -290,7 +292,7 @@ int omap_drm_irq_install(struct drm_device *dev)
> > 
> >  	INIT_LIST_HEAD(&priv->irq_list);
> > 
> > -	priv->irq_mask = 0;
> > +	priv->irq_mask = DISPC_IRQ_OCP_ERR;
> > 
> >  	max_planes = min(ARRAY_SIZE(priv->planes), ARRAY_SIZE(error_irqs));
> >  	for (i = 0; i < max_planes; ++i) {
> > 
> > @@ -309,16 +311,6 @@ int omap_drm_irq_install(struct drm_device *dev)
> > 
> >  	if (ret < 0)
> >  	
> >  		return ret;
> > 
> > -	error_handler->irq = omap_irq_error_handler;
> > -	error_handler->irqmask = DISPC_IRQ_OCP_ERR;
> > -
> > -	/* for now ignore DISPC_IRQ_SYNC_LOST_DIGIT.. really I think
> > -	 * we just need to ignore it while enabling tv-out
> > -	 */
> > -	error_handler->irqmask &= ~DISPC_IRQ_SYNC_LOST_DIGIT;
> > -
> > -	omap_irq_register(dev, error_handler);
> > -
> > 
> >  	dev->irq_enabled = true;
> >  	
> >  	return 0;
> 
> The removal of DISPC_IRQ_SYNC_LOST_DIGIT code above feels a bit out of
> place for this patch.

It's a no-op already, as irqmask is set to DISPC_IRQ_OCP_ERR (bit 9), and 
immediately after that bit 15 is masked out. I could split the removal out, 
but I think it wouldn't be very useful.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel