Hi Vinod, Thanks for your reply. Please see my comments inline > -----Original Message----- > From: dmaengine-owner@xxxxxxxxxxxxxxx [mailto:dmaengine- > owner@xxxxxxxxxxxxxxx] On Behalf Of Vinod Koul > Sent: Wednesday, December 20, 2017 10:36 PM > To: Vishal Sagar <vishal.sagar@xxxxxxxxxx> > Cc: dmaengine@xxxxxxxxxxxxxxx; Dinesh Kumar <dineshk@xxxxxxxxxx>; > michal.simek@xxxxxxxxxx; Jeff Mouroux <jmouroux@xxxxxxxxxx>; Radhey > Shyam Pandey <radheys@xxxxxxxxxx>; Hyun Kwon <hyunk@xxxxxxxxxx>; > Maurice Penners <mpenner@xxxxxxxxxx>; Vishal Sagar > <vsagar@xxxxxxxxxx>; John Nichols <jnichol@xxxxxxxxxx>; Hyun Kwon > <hyunk@xxxxxxxxxx> > Subject: Re: [PATCH 2/2] dma: xilinx: Add driver for Video Framebuffer IP > > On Wed, Dec 20, 2017 at 02:00:18PM +0530, Vishal Sagar wrote: > > The Video Framebuffer IP is available in two forms: read or write. > > This driver supports either form of the IP. > > Each IP is a single channel DMA and is video format aware. > > It can read/write video data arranged from/to memory as packed or > > semi-planar way based on the selected video format. > > To get list of supported video formats, clients can call certain APIs > > exposed by the driver. This driver introduces support for these IPs > > and integrates with the DMA Engine Framework. > > The subsytem name is dmaengine! [Vishal Sagar] 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. > > > Signed-off-by: Radhey Shyam Pandey <radheys@xxxxxxxxxx> > > Signed-off-by: John Nichols <jnichol@xxxxxxxxxx> > > Signed-off-by: Jeffrey Mouroux <jmouroux@xxxxxxxxxx> > > Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx> > > Signed-off-by: Hyun Kwon <hyun.kwon@xxxxxxxxxx> > > Signed-off-by: Vishal Sagar <vsagar@xxxxxxxxxx> > > --- > > drivers/dma/Kconfig | 14 +- > > drivers/dma/xilinx/Makefile | 1 + > > drivers/dma/xilinx/xilinx_frmbuf.c | 1155 > > ++++++++++++++++++++++++++++++++++++ > > Thats a very large file for review, do consider splitting stuff > > > @@ -342,7 +342,7 @@ config MOXART_DMA > > select DMA_VIRTUAL_CHANNELS > > help > > Enable support for the MOXA ART SoC DMA controller. > > - > > + > > why this noise, this patch has some many sign-off's but didnt anyone see > this noise? [Vishal Sagar] I introduced this. My mistake. I will fix it in next version. > > > +// SPDX-License-Identifier: GPL-2.0 > > Copyright needs to follow this in C99 style > > > +/* > > + * DMAEngine driver for Xilinx Framebuffer IP > > + * > > + * Copyright (C) 2016 - 2017 Xilinx, Inc. All rights reserved. > > + * > > + * Authors: Radhey Shyam Pandey <radheys@xxxxxxxxxx> > > + * John Nichols <jnichol@xxxxxxxxxx> > > + * Jeffrey Mouroux <jmouroux@xxxxxxxxxx> > > + * > > + * Based on the Freescale DMA driver. > > + * > > + * Description: > > + * The AXI Framebuffer core is a soft Xilinx IP core that > > + * provides high-bandwidth direct memory access between memory > > + * and AXI4-Stream. > > + * > > + * This program is free software: you can redistribute it and/or > > +modify > > + * it under the terms of the GNU General Public License as published > > +by > > + * the Free Software Foundation, either version 2 of the License, or > > + * (at your option) any later version. > > why do you need this license text? With SPDX tag this is implied.. > [Vishal Sagar] I will fix it once I get a clarification internally. > > +#include <linux/bitops.h> > > +#include <linux/dma/xilinx_frmbuf.h> > > +#include <linux/dmapool.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/init.h> > > +#include <linux/interrupt.h> > > +#include <linux/io.h> > > +#include <linux/iopoll.h> > > +#include <linux/module.h> > > +#include <linux/of_address.h> > > +#include <linux/of_dma.h> > > +#include <linux/of_irq.h> > > +#include <linux/of_platform.h> > > +#include <linux/slab.h> > > +#include <linux/videodev2.h> > > do you need so many headers? > [Vishal Sagar] I will remove ones not required in next version. > > +struct xilinx_frmbuf_chan { > > + struct xilinx_frmbuf_device *xdev; > > + /* Descriptor operation lock */ > > this is duplicate comment [Vishal Sagar] Will fix in next version. > > > +static LIST_HEAD(frmbuf_chan_list); > > +static DEFINE_MUTEX(frmbuf_chan_list_lock); > > why do you need these globals? > [Vishal Sagar] 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. 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. 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. > > +static const struct xilinx_frmbuf_format_desc xilinx_frmbuf_formats[] = > { > > + { > > + .dts_name = "xbgr8888", > > + .id = XILINX_FRMBUF_FMT_RGBX8, > > + .bpp = 4, > > + .num_planes = 1, > > + .drm_fmt = DRM_FORMAT_XBGR8888, > > + .v4l2_fmt = 0, > > + .fmt_bitmask = BIT(0), > > + }, > > + { > > + .dts_name = "unsupported", > > + .id = XILINX_FRMBUF_FMT_YUVX8, > > + .bpp = 4, > > + .num_planes = 1, > > + .drm_fmt = 0, > > + .v4l2_fmt = 0, > > + .fmt_bitmask = BIT(1), > > + }, > > 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. > > +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. > > > +static inline void frmbuf_write(struct xilinx_frmbuf_chan *chan, u32 reg, > > + u32 value) > > right justfied pls, here and other places [Vishal Sagar] Will fix these in next version. > > > +{ > > + iowrite32(value, chan->xdev->regs + reg); } > > + > > +static inline void frmbuf_writeq(struct xilinx_frmbuf_chan *chan, u32 > reg, > > + u64 value) > > +{ > > + iowrite32(lower_32_bits(value), chan->xdev->regs + reg); > > + iowrite32(upper_32_bits(value), chan->xdev->regs + reg + 4); } > > + > > +static void writeq_addr(struct xilinx_frmbuf_chan *chan, u32 reg, > > + dma_addr_t addr) > > +{ > > + frmbuf_writeq(chan, reg, (u64)addr); > > why the cast? > [Vishal Sagar] Not required. I will remove in next version. > > > +static struct xilinx_frmbuf_device *frmbuf_find_dev(struct dma_chan > > +*chan) { > > + struct xilinx_frmbuf_chan *xchan, *temp; > > + struct xilinx_frmbuf_device *xdev; > > + bool is_frmbuf_chan = false; > > + > > + list_for_each_entry_safe(xchan, temp, &frmbuf_chan_list, > chan_node) { > > + if (chan == &xchan->common) > > + is_frmbuf_chan = true; > > + } > > + > > + if (!is_frmbuf_chan) > > + return ERR_PTR(-ENODEV); > > + > > + xchan = to_xilinx_chan(chan); > > + xdev = container_of(xchan, struct xilinx_frmbuf_device, chan); > > + > > + return xdev; > > hmm this sounds strange to me, care to explain this pls? [Vishal Sagar] This is a helper function related to the global list referred to earlier. This is the function which traverses the list to see if the dma_chan object pointer represents a Framebuffer or some other DMA object > > > +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? > > > +static void xilinx_xdma_set_config(struct dma_chan *chan, u32 fourcc, > > +u32 type) { > > + struct xilinx_frmbuf_chan *xil_chan; > > + bool found_xchan = false; > > + int ret; > > + > > + mutex_lock(&frmbuf_chan_list_lock); > > + list_for_each_entry(xil_chan, &frmbuf_chan_list, chan_node) { > > + if (chan == &xil_chan->common) { > > + found_xchan = true; > > + break; > > + } > > + } > > again this seaching and am not sure why? > [Vishal Sagar] Please refer to earlier explanation for global list. > > +void xilinx_xdma_drm_config(struct dma_chan *chan, u32 drm_fourcc) > { > > + xilinx_xdma_set_config(chan, drm_fourcc, XDMA_DRM); > > + > > +} EXPORT_SYMBOL_GPL(xilinx_xdma_drm_config); > > first EXPORT_SYMBOL_GPL should be in different line, second what is being > set? some documentation for EXPORT_SYMBOLs is required.. > [Vishal Sagar] Ok will add documentation in next version for all EXPORT_SYMBOL. > > + > > +void xilinx_xdma_v4l2_config(struct dma_chan *chan, u32 v4l2_fourcc) > > +{ > > + xilinx_xdma_set_config(chan, v4l2_fourcc, XDMA_V4L2); > > + > > +} EXPORT_SYMBOL_GPL(xilinx_xdma_v4l2_config); > > here too.. > [Vishal Sagar] Ok will add documentation in next version for all EXPORT_SYMBOL. > > + > > +int xilinx_xdma_get_drm_vid_fmts(struct dma_chan *chan, u32 > *fmt_cnt, > > + u32 **fmts) > > +{ > > + struct xilinx_frmbuf_device *xdev; > > + > > + xdev = frmbuf_find_dev(chan); > > + > > + if (IS_ERR(xdev)) > > + return PTR_ERR(xdev); > > + > > + *fmt_cnt = xdev->drm_fmt_cnt; > > + *fmts = xdev->drm_memory_fmts; > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(xilinx_xdma_get_drm_vid_fmts); > > and here > [Vishal Sagar] Ok will add documentation in next version for all EXPORT_SYMBOL. > > + > > +int xilinx_xdma_get_v4l2_vid_fmts(struct dma_chan *chan, u32 > *fmt_cnt, > > + u32 **fmts) > > +{ > > + struct xilinx_frmbuf_device *xdev; > > + > > + xdev = frmbuf_find_dev(chan); > > + > > + if (IS_ERR(xdev)) > > + return PTR_ERR(xdev); > > + > > + *fmt_cnt = xdev->v4l2_fmt_cnt; > > + *fmts = xdev->v4l2_memory_fmts; > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(xilinx_xdma_get_v4l2_vid_fmts); > > and this > [Vishal Sagar] Ok will add documentation in next version for all EXPORT_SYMBOL. > > +/* > > +--------------------------------------------------------------------- > > +-------- > > do you mind getting rid of these, at best they are distraction > [Vishal Sagar] I will remove these in next version. > > +static struct xilinx_frmbuf_tx_descriptor * > > +xilinx_frmbuf_alloc_tx_descriptor(struct xilinx_frmbuf_chan *chan) { > > + struct xilinx_frmbuf_tx_descriptor *desc; > > + > > + desc = kzalloc(sizeof(*desc), GFP_KERNEL); > > + if (!desc) > > + return NULL; > > this can be skipped here! [Vishal Sagar] Agree. I will update this in next version. > > > +static void xilinx_frmbuf_free_descriptors(struct xilinx_frmbuf_chan > > +*chan) { > > + unsigned long flags; > > + > > + spin_lock_irqsave(&chan->lock, flags); > > + > > + xilinx_frmbuf_free_desc_list(chan, &chan->pending_list); > > + xilinx_frmbuf_free_desc_list(chan, &chan->done_list); > > + kfree(chan->active_desc); > > + kfree(chan->staged_desc); > > + > > + chan->staged_desc = NULL; > > + chan->active_desc = NULL; > > + INIT_LIST_HEAD(&chan->pending_list); > > + INIT_LIST_HEAD(&chan->done_list); > > initializing lists in free? > [Vishal Sagar] We wanted to the list to be initial state when it will be used again. I think it would be good to move it to alloc_chan_resources instead. > > +static int xilinx_frmbuf_alloc_chan_resources(struct dma_chan *dchan) > > +{ > > + dma_cookie_init(dchan); > > this can be done in your probe and not on alloc.. and consequently you can > get rid of alloc.. > [Vishal Sagar] Agree. I will move this to probe() in next version. > > +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. > > +static int xilinx_frmbuf_chan_probe(struct xilinx_frmbuf_device *xdev, > > + struct device_node *node) > > +{ > > + struct xilinx_frmbuf_chan *chan; > > + int err; > > + u32 dma_addr_size; > > + > > + chan = &xdev->chan; > > + > > + chan->dev = xdev->dev; > > + chan->xdev = xdev; > > + chan->idle = true; > > + > > + 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. > > +static int xilinx_frmbuf_probe(struct platform_device *pdev) { > > + struct device_node *node = pdev->dev.of_node; > > + struct xilinx_frmbuf_device *xdev; > > + struct resource *io; > > + enum dma_transfer_direction dma_dir; > > + const struct of_device_id *match; > > + int err; > > + u32 i, j; > > + int hw_vid_fmt_cnt; > > + const char *vid_fmts[ARRAY_SIZE(xilinx_frmbuf_formats)]; > > inverted x-mas tree helps in readability [Vishal Sagar] Ok. I will update it in next version. > > > + INIT_LIST_HEAD(&xdev->common.channels); > > + dma_cap_set(DMA_SLAVE, xdev->common.cap_mask); > > why slave and why not DMA_INTERLEAVE? [Vishal Sagar] This is coming from the earlier xilinx driver. I will update it to DMA_INTERLEAVE as this describes the capabilities better. > > > + dma_cap_set(DMA_PRIVATE, xdev->common.cap_mask); > > + > > + /* Initialize the channels */ > > + err = xilinx_frmbuf_chan_probe(xdev, node); > > + if (err < 0) > > + return err; > > + > > + xdev->chan.direction = dma_dir; > > + > > + if (xdev->chan.direction == DMA_DEV_TO_MEM) { > > + xdev->common.directions = BIT(DMA_DEV_TO_MEM); > > + dev_info(&pdev->dev, "Xilinx AXI frmbuf > DMA_DEV_TO_MEM\n"); > > + } else if (xdev->chan.direction == DMA_MEM_TO_DEV) { > > + xdev->common.directions = BIT(DMA_MEM_TO_DEV); > > + dev_info(&pdev->dev, "Xilinx AXI frmbuf > DMA_MEM_TO_DEV\n"); > > + } else { > > + xilinx_frmbuf_chan_remove(&xdev->chan); > > why not do the checks first! [Vishal Sagar] Agree. I will update it in next patch. > > > +MODULE_AUTHOR("Xilinx, Inc."); > > +MODULE_DESCRIPTION("Xilinx Framebuffer driver"); > MODULE_LICENSE("GPL > > +v2"); > > no MODULE_ALIAS, how did you test this? [Vishal Sagar] This driver was enabled in the kernel config and device tree and built as part of kernel. > > > +#if IS_ENABLED(CONFIG_XILINX_FRMBUF) > > +/** > > + * xilinx_xdma_drm_config - configure video format in video aware DMA > > + * @chan: dma channel instance > > + * @drm_fourcc: DRM fourcc code describing the memory layout of > video > > +data > > + * > > + * This routine is used when utilizing "video format aware" Xilinx > > +DMA IP > > + * (such as Video Framebuffer Read or Video Framebuffer Write). This > > +call > > + * must be made prior to dma_async_issue_pending() to enstablish the > > +video > > + * data memory format within the hardware DMA. > > + */ > > pls move this to src.. [Vishal Sagar] Ok. I will update this in the next version. Regards Vishal Sagar | Design Engineer | System Solutions | Xilinx Hyderabad > -- > ~Vinod > -- > To unsubscribe from this list: send the line "unsubscribe dmaengine" in the > body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info > at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
