dma: xilinx: Add driver for Video Framebuffer IP
PATCH 2/2 dma: xilinx: Add driver for Video Framebuffer IP
Quoted 4 lines · Vishal Sagar
Quoted 2 lines
The subsytem name is dmaengine!
[Vishal Sagar]
You don't need these tags when replying inline style
Quoted 3 lines
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
Quoted 4 lines
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+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.
Quoted 3 lines
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!
Quoted 3 lines
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.
Quoted 9 lines
The
frmbuf_chan_listis a global list so the driver can determine if adma_chanobject can safely be unwrapped into axilinx_frmbuf_chanobject. When a client callsxilinx_xdma_set_config(), we assume it does so without knowing for sure if thedma_chaninstance 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 thedma_chanreference to one associated with a framebuffer channel object.
Quoted 6 lines
If they match, then we know that the
dma_chanobject 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.
Quoted 15 lines
Quoted 3 lines
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_fmtsis 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.
Quoted 4 lines
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?
Quoted 11 lines
Quoted 7 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+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?
Quoted 12 lines
Quoted 4 lines
Quoted 4 lines
(quoted diff) +4
-0+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);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ + 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..
Quoted 4 lines
[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!
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
+static enum
dma_statusxilinx_frmbuf_tx_status(structdma_chan*dchan,
Review
Quoted 3 lines
(quoted diff) +3
-0+ dma_cookie_t cookie, + struct dma_tx_state *txstate) { + return dma_cookie_status(dchan, cookie, txstate);no residue caln?
Quoted 5 lines
[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?
Quoted 6 lines
Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1
-0+ err = of_property_read_u32(node, "xlnx,dma-addr-width",why not
device_property_read_u32[Vishal Sagar]
Of_property_read_u32is 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!