[2/2] dma: xilinx: Add driver for Video Framebuffer IP

[Vinod Koul <vinod.koul@intel.com>]

On Fri, Dec 22, 2017 at 08:20:52AM +0000, Vishal Sagar wrote:
> > 
> > The subsytem name is dmaengine!
> 
> [Vishal Sagar] 

You don't need these tags when replying inline style

> Apologies. This is my first instance of sending across patches to community. 
> I saw the precedence for this in other xilinx driver commit messages so kept it as it is.
> I will make the changes as "dmaengine: xilinx: " in next version.

which precedence?

$ git log --online drivers/dma/xilinx/ |grep " dma:" |wc -l
2
$ git log --online drivers/dma/xilinx/ |grep " dmaengine: " |wc -l
52

> > > +static LIST_HEAD(frmbuf_chan_list);
> > > +static DEFINE_MUTEX(frmbuf_chan_list_lock);
> > 
> > why do you need these globals?

Please fix your MUA to wrap at 80 columns, I have reflowed your text to fit.

> The frame buffer read and write are Soft IPs which can be configured to
> support different video formats.  Please refer to PG278 Product Guide for
> Video Frame Buffer Read and Write v2.0.

huh why should I refer to that!

> The client driver would want to
> know what are the video formats supported and set the format accordingly.
> For this there are APIs exposed by this driver. 

You are *not* the first driver to support these kind of use cases.

> The frmbuf_chan_list is a global list so the driver can determine if a
> dma_chan object can safely be unwrapped into a xilinx_frmbuf_chan object.
> When a client calls xilinx_xdma_set_config(), we assume it does so without
> knowing for sure if the dma_chan instance it is passing in actually
> represents a Framebuffer channel or some other channel.   By keeping a
> global list of framebuffer channel objects that have been registered
> during probe, we can compare the dma_chan reference to one associated with
> a framebuffer channel object.   
> 
> If they match, then we know that the dma_chan object instance represents a
> framebuffer and we can proceed to safely unwrap this and get at the
> underlying Framebuffer channel to assign the video format.   
> 
> If it is not a match, then this dma channel likely represents some other
> dma type (e.g. vdma) and this call just becomes a no-op. 

This sound a bad design to me. You have dma_chan which xilinx_chan would
wrap and you should have everything in that. Making them globally and then
looking up doesn't make sense to me.

> > am no DT expert, but this doesn't look right to me. Care to explain what this
> > struct does?
> > 
> [Vishal Sagar] 
> 
> This table is contains metadata for all the different video formats these
> soft IPs support.  One or more strings matching the .dts_name member shall
> be present in the device tree node representing the video formats selected
> by system designer to optimize logic footprint of the IP.
> 
> Based on the string names present, a bitmask enabled_vid_fmts is created
> using the .fmt_bitmask member of video formats mentioned in device tree.
> When a client requests for a particular pixel format, the corresponding
> bit is checked against this bitmask to ensure it is valid.
> 
> We support a private API that clients can call to get a list of supported
> video formats.  We use the bitmask to find out which formats are
> supported, then pull either the DRM or V4L2 (depending on client type)
> pixel format codes from this table into an array returned to the client.

DMAengine should not care about it. Its job is to do DMA, video formats etc
doesn't seem to belong to it, are we missing a video client driver for this?

> > > +static const struct of_device_id xilinx_frmbuf_of_ids[] = {
> > > +	{ .compatible = "xlnx,axi-frmbuf-wr-v2",
> > > +		.data = (void *)DMA_DEV_TO_MEM},
> > > +	{ .compatible = "xlnx,axi-frmbuf-rd-v2",
> > > +		.data = (void *)DMA_MEM_TO_DEV},
> > 
> > is the direction a hw property or configurable?
> [Vishal Sagar] 
> The direction is a fixed hw property. Framebuffer Write IP will write AXI4
> stream data to memory.  Framebuffer Read IP will read frame data from
> memory and convert to AXI4 stream.

So during prep_xxx, do you fail if requested txn is not of same
direction?

> > > +static int frmbuf_verify_format(struct dma_chan *chan, u32 fourcc,
> > > +u32 type) {
> > > +	struct xilinx_frmbuf_chan *xil_chan = to_xilinx_chan(chan);
> > > +	u32 i, sz = ARRAY_SIZE(xilinx_frmbuf_formats);
> > > +
> > > +	for (i = 0; i < sz; i++) {
> > > +		if ((type == XDMA_DRM &&
> > > +		     fourcc != xilinx_frmbuf_formats[i].drm_fmt) ||
> > > +		   (type == XDMA_V4L2 &&
> > > +		    fourcc != xilinx_frmbuf_formats[i].v4l2_fmt))
> > 
> > this is very hard to read..
> [Vishal Sagar] 
> Here we are checking if the fourcc video format matches the corresponding
> format based on the DMA client ie DRM or V4L2.  Do you suggest some other
> way?

Yes more readability, perhaps moving the check to a helper!

> > > +static enum dma_status xilinx_frmbuf_tx_status(struct dma_chan
> > *dchan,
> > > +					       dma_cookie_t cookie,
> > > +					       struct dma_tx_state *txstate) {
> > > +	return dma_cookie_status(dchan, cookie, txstate);
> > 
> > no residue caln?
> > 
> [Vishal Sagar] 

> I didn't understand your question. Are you asking if we can return
> residual bytes transferred?  The IP doesn't support how many bytes were
> sent in this frame before it was stopped.

then set tx_status as dma_cookie_status, why use a wrapper?

> > > +	err = of_property_read_u32(node, "xlnx,dma-addr-width",
> > 
> > why not device_property_read_u32
> > 
> [Vishal Sagar] 
> Of_property_read_u32 is more familiar. Let me know if this is ok or I should change it.

You can get familiar with device_property_read_u32() which is firmware agnostic
read mechanism!