Message-ID: <20081223105006.GE1614@pengutronix.de>
Date:	Tue, 23 Dec 2008 11:50:06 +0100
From:	Sascha Hauer <s.hauer@...gutronix.de>
To:	Guennadi Liakhovetski <g.liakhovetski@....de>
Cc:	linux-kernel@...r.kernel.org,
	linux-fbdev-devel@...ts.sourceforge.net, adaplas@...il.com,
	linux-arm-kernel@...ts.arm.linux.org.uk,
	Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH 2/4 v4] i.MX31: Image Processing Unit DMA and IRQ
	drivers

On Mon, Dec 22, 2008 at 09:10:03PM +0100, Guennadi Liakhovetski wrote:
> As you surely have seen, I just posted v5 of the patchset.
> 
> On Mon, 22 Dec 2008, Sascha Hauer wrote:
> 
> > On Mon, Dec 22, 2008 at 12:56:29PM +0100, Guennadi Liakhovetski wrote:
> > > Hi Sascha,
> > > 
> > > Thanks for the review and for the comments! I'm fixing them all, except 
> > > for a couple points which I'm not sure I agree with:
> > > 
> > > On Thu, 18 Dec 2008, Sascha Hauer wrote:
> > > 
> > > > Hi Guennadi,
> > > > 
> > > > On Thu, Dec 18, 2008 at 02:26:19PM +0100, Guennadi Liakhovetski wrote:
> > > > > From: Guennadi Liakhovetski <lg@...x.de>
> > > > > 
> > > > > i.MX3x SoCs contain an Image Processing Unit, consisting of a Control
> > > > > Module (CM), Display Interface (DI), Synchronous Display Controller (SDC),
> > > > > Asynchronous Display Controller (ADC), Image Converter (IC), Post-Filter
> > > > > (PF), Camera Sensor Interface (CSI), and an Image DMA Controller (IDMAC).
> > > > > CM contains, among other blocks, an Interrupt Generator (IG) and a Clock
> > > > > and Reset Control Unit (CRCU). This driver serves IDMAC and IG. They are
> > > > > supported over dmaengine and irq-chip APIs respectively.
> > > > > 
> > > > > IDMAC is a specialised DMA controller, its DMA channels cannot be used for
> > > > > general-purpose operations, even though it might be possible to configure
> > > > > a memory-to-memory channel for memcpy operation. This driver will not work
> > > > > with generic dmaengine clients, clients, wishing to use it must use
> > > > > respective wrapper structures, they also must specify which channels they
> > > > > require, as channels are hard-wired to specific IPU functions.
> > > > 
> > > > As a place for this driver /me votes for drivers/dma/ Though it does not
> > > > seem like a perfect place for it, it still uses the API provided there.
> > > 
> > > Yes, I also considered that. Dan, would you accept that? What makes it 
> > > defferent from other drivers/dma drivers, is that this one also has an irq 
> > > driver.
> > > 
> > > > A general note: Can we get rid of the function names starting with an
> > > > underscore?
> > > 
> > > Yeah... They are used pretty consistent now throughout the driver, and are 
> > > used for functions internal, static, maybe only called once... But if you 
> > > prefer, I can remove underscores, sure.
> > > 
> > > > > +/*
> > > > > + * There can be only one, we could allocate it dynamically, but then we'd have
> > > > > + * to add an extra parameter to some functions, and use something as ugly as
> > > > > + *	struct ipu *ipu = to_ipu(to_idmac(ichan->dma_chan.device));
> > > > 
> > > > still you use it in one place
> > > 
> > > It is used in more than one place - implicitly in others. I did try to use 
> > > the pointer everywhere in the new code where it wasn't too expensive, to 
> > > make it at least easier if anyone ever decides to switch to dynamic 
> > > allocation. So, I wouldn't throw away all those "ugly" calculations and 
> > > replace them with the static variable, but if you think that'd be better, 
> > > I can do that.
> > 
> > No, it was more the comment that was irritating me.
> 
> Hm, well, it could be removed of course, I hope this wouldn't be a 
> K.O. criterium:-)

Of course not ;)

> 
> > > > > +		spin_lock_init(&ichan->lock);
> > > > > +		mutex_init(&ichan->chan_mutex);
> > > > 
> > > > Having two locking methods for one dma channel looks like a recipe for
> > > > trouble.
> > > 
> > > Why? You wouldn't take a mutex under spinlock, and the other way round is 
> > > fine:-). I think both are needed - spinlock for ISR, atomic contexts for 
> > > atomic operations like modifying registers, lists. And the mutex for 
> > > sleeping contexts.
> > 
> > I just remember reading some document which said that using too many
> > locks is not a good idea. Well I guess you're prepared for bug reports
> > ;)
> 
> Sure, you can start now:-)
> 
> > > > > +#define IPU_IRQ_NR_FN_IRQS (32 + 32 + 24)
> > > > > +#define IPU_IRQ_NR_ERR_IRQS (32 + 17)
> > > > > +#define IPU_IRQ_NR_IRQS (IPU_IRQ_NR_ERR_IRQS + IPU_IRQ_NR_FN_IRQS)
> > > > 
> > > > Do we really need an interrupt handler behind each status bit the ipu
> > > > has to offer? This looks quite expensive
> > > 
> > > What are you worried about? The size of irq_desc[]? Well, we could make 
> > > error interrupts dependent on a CONFIG_ macro, and let drivers that need 
> > > them select that option, because so far they are not used.
> > 
> > Sounds good, they are unused in the Freescale BSP aswell-
> > 
> > > Otherwise, I 
> > > think it is good to have all those bits on separate IRQs. What would you 
> > > do? Request only two IRQs - function and error and then demux them 
> > > manually, reinventing the generic irq subsystem? Doesn't seem very optimal 
> > > to me...
> > 
> > No indeed not. My concern is that of these 137 interrupts only 7 are
> > used in the Freescale BSP which shoud be quite feature complete.
> > I think we should check the potential use of these interrupts before
> > adding ~10k of struct irq_desc to the kernel.
> > What about the *_EOF interrupts? They are now demultiplexed in a chained
> > interrupt handler and then merged back together to a single handler in
> > ipu_idmac.c. Is there a potential use of these interrupts outside this file?
> 
> Ok, so, what would we like to have there? We agree that the proper way to 
> serve them is a irq-chip driver, right?

In case of the *_EOF interrupts when they can be used outside the idmac
driver then yes. If not then not for the reasons I explained.

> We could make error IRQs and 
> new frame IRQs optional. That would leave 56 IRQs, would that be ok?
> 
> > I just had a short look at your framebuffer driver. Am I understanding
> > it right that you setup two descriptors and pass them to the DMA engine
> > alternately? Wouldn't it be easier to pass just one descriptor and chain
> > the end to the beginning? I mean both descriptors read from the same
> > memory anyway.
> 
> No, addresses are not necessarily the same. With panning addresses are 
> different.

Ah, now I see. The model with alternating descriptors is only used for
panning.


> 
> > Another thing is the overlay framebuffer. I think it's mainly useful to
> > display video streams. Maybe it's better to implement this as a v4l
> > device as unlike the framebuffer API the v4l API is designed to handle
> > different image buffers.
> > These things just popped up in my mind, I don't know if they are
> > practical, I still have a quite limited understanding of the whole
> > imaging stuff on the MX31.
> 
> You mean an output v4l device? I think overlays are handled by framebuffer 
> drivers... But I'm also not quite sure about it, however, handling overlay 
> as another framebuffer seems logical to me.

Well the DMA engine seems to suggest that frames should be passed around
whereas the framebuffer API only has a single frame. That would fit
better into the v4l API. Also the IPU can do things like colourspace
conversion and hw scaling which would fit into the V4L API.
OTOH there are not many examples of drivers doing this (ivtv, zoran in
kernel and an Omap driver found on lists). And I haven't found any
application supporting a V4L output device.
BTW is the overlay framebuffer useful in it's current implementation?
There seems to be no way to adjust the x/y offset or the blending modes.

> 
> If there are no other problems with v5, could we maybe take it as a basis 
> and then I would submit a patch to reduce the number of IRQs?

Please understand my concerns with this driver. It's a quite complex
beast and experience shows that once a driver is in the kernel it is far
more complicated to change it than to do it right the first way. You
know that I'm also interested in having a MX31 framebuffer (and camera)
driver in kernel but I want to make sure that it works properly and leaves
room for feature enhancements without having to refactor the whole
driver.
It would be good if someone else could throw his 2 cents into this
discussion.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists