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

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

On Wed, Jan 10, 2018 at 11:53:21AM +0000, Vishal Sagar wrote:
> 
> 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. 

Did you follow that? Esp the wrapping part. Whatever tool you use, pls make
sure replies are within 80chars

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

Sorry but not my problem. I should understand and be able to review the
patch. Please create and add comments to satisfy 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.
>

80 char wrap issue again :(
 
> 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; 
> };

again, you are not the first ones proposing this and sorry we don't go that
route anymore.

Please see other video drivers on how they set this

> 
> 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 */
> }

this is all video specfic stuff, why should a dmaengine driver care about
this. DMA care about transferring data.

As I said last time, you should split video stuff and dmaengine stuff to
separate drivers. Dmaengine should only worry about transferring data and
video driver should worry about video formats

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

who is the client, video driver right?