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

[Vishal Sagar <vsagar@xilinx.com>]

Hi Vinod

Thanks for your feedback. 

> -----Original Message-----
> From: Vinod Koul [mailto:vinod.koul@intel.com]
> Sent: Friday, December 22, 2017 3:25 PM
> To: Vishal Sagar <vsagar@xilinx.com>
> Cc: Vishal Sagar <vishal.sagar@xilinx.com>; dmaengine@vger.kernel.org;
> Dinesh Kumar <dineshk@xilinx.com>; michal.simek@xilinx.com; Jeff
> Mouroux <jmouroux@xilinx.com>; Radhey Shyam Pandey
> <radheys@xilinx.com>; Hyun Kwon <hyunk@xilinx.com>; Maurice Penners
> <mpenner@xilinx.com>; John Nichols <jnichol@xilinx.com>
> Subject: Re: [PATCH 2/2] dma: xilinx: Add driver for Video Framebuffer IP
> 
> 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
> 

Ok noted.

> > 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
> 

My mistake. It was in our git repo. I will take care in future.

> > > > +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.

Ok thanks. I am using Outlook to send mails and have setup the client as 
per https://elinux.org/Mail_client_tips. 

> 
> > 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 product guide gives details about the memory layout of buffer for 
every format supported by these IPs.

> 
> > 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.

Ok. We discussed internally about this. Instead of using custom APIs we could use 
device_config() in the client and send custom data by wrapping it around the struct dma_slave_config.
=====================================================================
enum {
    UNKNOWN,
    XFRMBUF,
} xdma_type;

struct xilinx_dma_config {
	struct dma_slave_config config;
	enum xdma_type type; 
	union {
	 struct framebuffer_config;
	   /* Other future dma configs here */
	} xdma_config; 
};

enum {
    UNKNOWN,
    V4L2,
    DRM
} framework_type;

struct framebuffer_config {
	video_format;  /* input param */
	enum framework_type fwtype;
	supported_v4l2_video_formats[];   /* output param */
	supported_drm_video_formats[];  /* output param */
}
=====================================================================
First time client sets type = UNKNOWN and calls device_config(). 
The driver sets the type back as XFRMBUF and populates the supported v4l2 and drm video formats and returns.
Client calls it second time with type = XFRMBUF and video_format and fwtype set to one of the supported formats.
This will help framebuffer driver correctly configure the video format.

> 
> > 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?

I agree that DMAengine driver shouldn't have to care about the video formats.
But the only reason to include the translation in the framebuffer driver is to 
avoid replicating the translation logic in future client drivers.

Currently we have modified the Xilinx Video pipeline driver 
(drivers/media/platform/xilinx/ xilinx-dma.* xilinx-vipp.* xilinx-vip.*) to test 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?

Yes. 

> 
> > > > +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!

Ok. I will do this in next patch.

> 
> > > > +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?

Understood. I will do this in next patch. 

> 
> > > > +	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!
> 

Ok I will do this in next patch.

- Vishal Sagar

> --
> ~Vinod
---
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html