From: Vinod Koul <vinod.koul@intel.com>
To: Kedareswara rao Appana <appana.durga.rao@xilinx.com>
Cc: robh+dt@kernel.org, mark.rutland@arm.com,
dan.j.williams@intel.com, michal.simek@xilinx.com,
soren.brinkmann@xilinx.com, appanad@xilinx.com,
moritz.fischer@ettus.com, laurent.pinchart@ideasonboard.com,
luis@debethencourt.com, Jose.Abreu@synopsys.com,
dmaengine@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH v6 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma
Date: Wed, 25 Jan 2017 11:45:06 +0530 [thread overview]
Message-ID: <20170125061506.GA3573@localhost> (raw)
In-Reply-To: <1484372155-19423-3-git-send-email-appanad@xilinx.com>
On Sat, Jan 14, 2017 at 11:05:54AM +0530, Kedareswara rao Appana wrote:
> When VDMA is configured for more than one frame in the h/w.
> For example h/w is configured for n number of frames, user
> Submits n number of frames and triggered the DMA using issue_pending API.
>
> In the current driver flow we are submitting one frame at a time,
> But we should submit all the n number of frames at one time
> As the h/w is configured for n number of frames.
Is there a specific reason why you continue to start lines with Title cases, I
have already told you to not do that last time!
>
> This patch fixes this issue.
>
> Acked-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Jose Abreu <joabreu@synopsys.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Changes for v6:
> ---> Added Rob Acked-by
> ---> Updated commit message as suggested by Vinod.
> Changes for v5:
> ---> Updated xlnx,fstore-config property to xlnx,fstore-enable
> and updated description as suggested by Rob.
> Changes for v4:
> ---> Add Check for framestore configuration on Transmit case as well
> as suggested by Jose Abreu.
> ---> Modified the dev_dbg checks to dev_warn checks as suggested
> by Jose Abreu.
> Changes for v3:
> ---> Added Checks for frame store configuration. If frame store
> Configuration is not present at the h/w level and user
> Submits less frames added debug prints in the driver as relevant.
> Changes for v2:
> ---> Fixed race conditions in the driver as suggested by Jose Abreu
> ---> Fixed unnecessray if else checks in the vdma_start_transfer
> as suggested by Laurent Pinchart.
>
> .../devicetree/bindings/dma/xilinx/xilinx_dma.txt | 2 +
> drivers/dma/xilinx/xilinx_dma.c | 78 +++++++++++++++-------
> 2 files changed, 57 insertions(+), 23 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> index a2b8bfa..e951c09 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_dma.txt
> @@ -66,6 +66,8 @@ Optional child node properties:
> Optional child node properties for VDMA:
> - xlnx,genlock-mode: Tells Genlock synchronization is
> enabled/disabled in hardware.
> +- xlnx,fstore-enable: boolean; if defined, it indicates that controller
> + supports frame store configuration.
> Optional child node properties for AXI DMA:
> -dma-channels: Number of dma channels in child node.
>
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 5eeea57..edb5b71 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -322,6 +322,7 @@ struct xilinx_dma_tx_descriptor {
> * @genlock: Support genlock mode
> * @err: Channel has errors
> * @idle: Check for channel idle
> + * @has_fstoreen: Check for frame store configuration
> * @tasklet: Cleanup work after irq
> * @config: Device configuration info
> * @flush_on_fsync: Flush on Frame sync
> @@ -353,6 +354,7 @@ struct xilinx_dma_chan {
> bool genlock;
> bool err;
> bool idle;
> + bool has_fstoreen;
> struct tasklet_struct tasklet;
> struct xilinx_vdma_config config;
> bool flush_on_fsync;
> @@ -990,6 +992,27 @@ static void xilinx_vdma_start_transfer(struct xilinx_dma_chan *chan)
> if (list_empty(&chan->pending_list))
> return;
>
> + /*
> + * Note: When VDMA is built with default h/w configuration
> + * User should submit frames upto H/W configured.
> + * If users submits less than h/w configured
> + * VDMA engine tries to write to a invalid location
> + * Results undefined behaviour/memory corruption.
> + *
> + * If user would like to submit frames less than h/w capable
> + * On S2MM side please enable debug info 13 at the h/w level
> + * On MM2S side please enable debug info 6 at the h/w level
> + * It will allows the frame buffers numbers to be modified at runtime.
> + */
> + if (!chan->has_fstoreen &&
> + chan->desc_pendingcount < chan->num_frms) {
> + dev_warn(chan->dev, "Frame Store Configuration is not enabled at the\n");
> + dev_warn(chan->dev, "H/w level enable Debug info 13 or 6 at the h/w level\n");
> + dev_warn(chan->dev, "OR Submit the frames upto h/w Capable\n\r");
> +
> + return;
> + }
> +
> desc = list_first_entry(&chan->pending_list,
> struct xilinx_dma_tx_descriptor, node);
> tail_desc = list_last_entry(&chan->pending_list,
> @@ -1052,25 +1075,38 @@ static void xilinx_vdma_start_transfer(struct xilinx_dma_chan *chan)
> if (chan->has_sg) {
> dma_ctrl_write(chan, XILINX_DMA_REG_TAILDESC,
> tail_segment->phys);
> + list_splice_tail_init(&chan->pending_list, &chan->active_list);
> + chan->desc_pendingcount = 0;
> } else {
> struct xilinx_vdma_tx_segment *segment, *last = NULL;
> - int i = 0;
> + int i = 0, j = 0;
>
> if (chan->desc_submitcount < chan->num_frms)
> i = chan->desc_submitcount;
>
> - list_for_each_entry(segment, &desc->segments, node) {
> - if (chan->ext_addr)
> - vdma_desc_write_64(chan,
> - XILINX_VDMA_REG_START_ADDRESS_64(i++),
> - segment->hw.buf_addr,
> - segment->hw.buf_addr_msb);
> - else
> - vdma_desc_write(chan,
> - XILINX_VDMA_REG_START_ADDRESS(i++),
> - segment->hw.buf_addr);
> -
> - last = segment;
> + for (j = 0; j < chan->num_frms; ) {
> + list_for_each_entry(segment, &desc->segments, node) {
> + if (chan->ext_addr)
> + vdma_desc_write_64(chan,
> + XILINX_VDMA_REG_START_ADDRESS_64(i++),
> + segment->hw.buf_addr,
> + segment->hw.buf_addr_msb);
> + else
> + vdma_desc_write(chan,
> + XILINX_VDMA_REG_START_ADDRESS(i++),
> + segment->hw.buf_addr);
> +
> + last = segment;
> + }
> + list_del(&desc->node);
> + list_add_tail(&desc->node, &chan->active_list);
> + j++;
> + if (list_empty(&chan->pending_list) ||
> + (i == chan->num_frms))
> + break;
> + desc = list_first_entry(&chan->pending_list,
> + struct xilinx_dma_tx_descriptor,
> + node);
> }
>
> if (!last)
> @@ -1081,20 +1117,14 @@ static void xilinx_vdma_start_transfer(struct xilinx_dma_chan *chan)
> vdma_desc_write(chan, XILINX_DMA_REG_FRMDLY_STRIDE,
> last->hw.stride);
> vdma_desc_write(chan, XILINX_DMA_REG_VSIZE, last->hw.vsize);
> - }
>
> - chan->idle = false;
> - if (!chan->has_sg) {
> - list_del(&desc->node);
> - list_add_tail(&desc->node, &chan->active_list);
> - chan->desc_submitcount++;
> - chan->desc_pendingcount--;
> + chan->desc_submitcount += j;
> + chan->desc_pendingcount -= j;
> if (chan->desc_submitcount == chan->num_frms)
> chan->desc_submitcount = 0;
> - } else {
> - list_splice_tail_init(&chan->pending_list, &chan->active_list);
> - chan->desc_pendingcount = 0;
> }
> +
> + chan->idle = false;
> }
>
> /**
> @@ -1342,6 +1372,7 @@ static int xilinx_dma_reset(struct xilinx_dma_chan *chan)
>
> chan->err = false;
> chan->idle = true;
> + chan->desc_submitcount = 0;
>
> return err;
> }
> @@ -2320,6 +2351,7 @@ static int xilinx_dma_chan_probe(struct xilinx_dma_device *xdev,
> has_dre = of_property_read_bool(node, "xlnx,include-dre");
>
> chan->genlock = of_property_read_bool(node, "xlnx,genlock-mode");
> + chan->has_fstoreen = of_property_read_bool(node, "xlnx,fstore-enable");
>
> err = of_property_read_u32(node, "xlnx,datawidth", &value);
> if (err) {
> --
> 2.1.2
>
--
~Vinod
next prev parent reply other threads:[~2017-01-25 6:14 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-01-14 5:35 [PATCH v6 0/3] dmaengine: xilinx_dma: Bug fixes Kedareswara rao Appana
2017-01-14 5:35 ` [PATCH v6 1/3] dmaengine: xilinx_dma: Check for channel idle state before submitting dma descriptor Kedareswara rao Appana
2017-01-14 5:35 ` [PATCH v6 2/3] dmaeninge: xilinx_dma: Fix bug in multiple frame stores scenario in vdma Kedareswara rao Appana
2017-01-23 7:26 ` Mike Looijmans
[not found] ` <CY1PR02MB169299C7AA719D4CE56F328DDC3B0@CY1PR02MB1692.namprd02.prod.outlook.com>
2017-12-06 4:11 ` Appana Durga Kedareswara Rao
2017-01-25 6:15 ` Vinod Koul [this message]
2017-01-14 5:35 ` [PATCH v6 3/3] dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario Kedareswara rao Appana
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170125061506.GA3573@localhost \
--to=vinod.koul@intel.com \
--cc=Jose.Abreu@synopsys.com \
--cc=appana.durga.rao@xilinx.com \
--cc=appanad@xilinx.com \
--cc=dan.j.williams@intel.com \
--cc=devicetree@vger.kernel.org \
--cc=dmaengine@vger.kernel.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luis@debethencourt.com \
--cc=mark.rutland@arm.com \
--cc=michal.simek@xilinx.com \
--cc=moritz.fischer@ettus.com \
--cc=robh+dt@kernel.org \
--cc=soren.brinkmann@xilinx.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).