| Message ID | 1437676792-13465-2-git-send-email-vinod.koul@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Vinod Koul <vinod.koul@intel.com> writes: > This adds new descriptor flag for reusing a descriptor by submitting > multiple times by a client, for example video buffer. > Add helper APIs for this as well > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> Hi Vinod, Thanks for carrying this feature. > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index e2f5eb419976..2adcd3c1ae48 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -183,6 +183,8 @@ struct dma_interleaved_template { > * operation it continues the calculation with new sources > * @DMA_PREP_FENCE - tell the driver that subsequent operations depend > * on the result of this operation > + * @DMA_CTRL_REUSE: Client can reuse the descriptor and submit again till Wouldn't it be better for homogeneity to have ? : + * @DMA_CTRL_REUSE - client can reuse the descriptor and submit again till > +static inline bool dmaengine_desc_test_reuse(struct dma_async_tx_descriptor *tx) > +{ > + return (tx->flags & DMA_CTRL_REUSE) == DMA_CTRL_REUSE; > +} > + > +static inline int dmaengine_desc_free(struct dma_async_tx_descriptor *desc) > +{ > + /* this is supported for reusable desc, so check that */ > + if (!dmaengine_desc_test_reuse(desc)) Isn't that test inverted, ie. shouldn't this be : + if (dmaengine_desc_test_reuse(desc)) I'm thinking this because of my understanding of the flag, and the documentation you added : + - Explicitly invoking dmaengine_desc_free(), this can succeed only + when DMA_CTRL_REUSE is already set Cheers.
On Thu, Jul 23, 2015 at 09:38:39PM +0200, Robert Jarzmik wrote: > Vinod Koul <vinod.koul@intel.com> writes: > > > This adds new descriptor flag for reusing a descriptor by submitting > > multiple times by a client, for example video buffer. > > Add helper APIs for this as well > > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > Hi Vinod, > > Thanks for carrying this feature. > > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > > index e2f5eb419976..2adcd3c1ae48 100644 > > --- a/include/linux/dmaengine.h > > +++ b/include/linux/dmaengine.h > > @@ -183,6 +183,8 @@ struct dma_interleaved_template { > > * operation it continues the calculation with new sources > > * @DMA_PREP_FENCE - tell the driver that subsequent operations depend > > * on the result of this operation > > + * @DMA_CTRL_REUSE: Client can reuse the descriptor and submit again till > Wouldn't it be better for homogeneity to have ? : > + * @DMA_CTRL_REUSE - client can reuse the descriptor and submit again till > > > +static inline bool dmaengine_desc_test_reuse(struct dma_async_tx_descriptor *tx) > > +{ > > + return (tx->flags & DMA_CTRL_REUSE) == DMA_CTRL_REUSE; > > +} > > + > > +static inline int dmaengine_desc_free(struct dma_async_tx_descriptor *desc) > > +{ > > + /* this is supported for reusable desc, so check that */ > > + if (!dmaengine_desc_test_reuse(desc)) > Isn't that test inverted, ie. shouldn't this be : > + if (dmaengine_desc_test_reuse(desc)) > > I'm thinking this because of my understanding of the flag, and the documentation > you added : Yes the check got inverted, will fix that
On 07/23/2015 08:39 PM, Vinod Koul wrote: [...] > +static inline int dmaengine_desc_set_reuse(struct dma_async_tx_descriptor *tx) > +{ > + struct dma_slave_caps caps; > + > + dma_get_slave_caps(tx->chan, &caps); > + > + if (caps.descriptor_reuse) { > + tx->flags |= DMA_CTRL_REUSE; > + return 0; > + } else { > + return -EPERM; > + } > +} > + > +static inline void dmaengine_desc_clear_reuse(struct dma_async_tx_descriptor *tx) > +{ > + tx->flags &= ~DMA_CTRL_REUSE; > +} I don' think we need set set and clear functions. We need them for the ACK flag because the ACK flag can be set later on. But the REUSE flag should be specified when the descriptor is created so the driver can make the necessary preparations it might need to support re-usable descriptors. Maybe we should call the flag DMA_PREP_REUSABLE (like the other DMA_PREP_*) flag to reflect this. > + > +static inline bool dmaengine_desc_test_reuse(struct dma_async_tx_descriptor *tx) > +{ > + return (tx->flags & DMA_CTRL_REUSE) == DMA_CTRL_REUSE; > +} > + > +static inline int dmaengine_desc_free(struct dma_async_tx_descriptor *desc) > +{ > + /* this is supported for reusable desc, so check that */ > + if (!dmaengine_desc_test_reuse(desc)) > + return desc->desc_free(desc); > + else > + return -EPERM; > +} > + > static inline bool dmaengine_check_align(u8 align, size_t off1, size_t off2, size_t len) > { > size_t mask; > -- 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
On Fri, Jul 24, 2015 at 10:28:45AM +0200, Lars-Peter Clausen wrote: > On 07/23/2015 08:39 PM, Vinod Koul wrote: > [...] > >+static inline int dmaengine_desc_set_reuse(struct dma_async_tx_descriptor *tx) > >+{ > >+ struct dma_slave_caps caps; > >+ > >+ dma_get_slave_caps(tx->chan, &caps); > >+ > >+ if (caps.descriptor_reuse) { > >+ tx->flags |= DMA_CTRL_REUSE; > >+ return 0; > >+ } else { > >+ return -EPERM; > >+ } > >+} > >+ > >+static inline void dmaengine_desc_clear_reuse(struct dma_async_tx_descriptor *tx) > >+{ > >+ tx->flags &= ~DMA_CTRL_REUSE; > >+} > > I don' think we need set set and clear functions. We need them for > the ACK flag because the ACK flag can be set later on. But the REUSE > flag should be specified when the descriptor is created so the > driver can make the necessary preparations it might need to support > re-usable descriptors. Maybe we should call the flag > DMA_PREP_REUSABLE (like the other DMA_PREP_*) flag to reflect this. The flow I have depicted is that descriptor prepare should not be linked to it's reuse. The submit operation specifies if a descriptor can be reused or not, so driver knows what to do after txn completion While reusing if client wants to stop reusing it can clear reuse flag and submit so that descriptor is freed up Also this is tied to caps so you are allowed to do so only if caps supports it. From these points we do need set/clear/test_reuse APIs
Vinod Koul <vinod.koul@intel.com> writes: > On Fri, Jul 24, 2015 at 10:28:45AM +0200, Lars-Peter Clausen wrote: >> I don' think we need set set and clear functions. We need them for >> the ACK flag because the ACK flag can be set later on. But the REUSE >> flag should be specified when the descriptor is created so the >> driver can make the necessary preparations it might need to support >> re-usable descriptors. Maybe we should call the flag >> DMA_PREP_REUSABLE (like the other DMA_PREP_*) flag to reflect this. > The flow I have depicted is that descriptor prepare should not be linked to > it's reuse. The submit operation specifies if a descriptor can be reused or > not, so driver knows what to do after txn completion > While reusing if client wants to stop reusing it can clear reuse flag and > submit so that descriptor is freed up I think the same way as Vinod. Giving the dmaengine user the flexibility to reuse or not the descriptor is great. Moreover, this flexibility will extend to userspace, as some dmaengine users, such as v4l2, don't know if a descriptor will be reused : it's a userspace decision whever to requeue the transmission or not, up to the driver. > Also this is tied to caps so you are allowed to do so only if caps supports > it. From these points we do need set/clear/test_reuse APIs +1. Cheers.
On 07/24/2015 09:12 PM, Robert Jarzmik wrote: > Vinod Koul <vinod.koul@intel.com> writes: > >> On Fri, Jul 24, 2015 at 10:28:45AM +0200, Lars-Peter Clausen wrote: >>> I don' think we need set set and clear functions. We need them for >>> the ACK flag because the ACK flag can be set later on. But the REUSE >>> flag should be specified when the descriptor is created so the >>> driver can make the necessary preparations it might need to support >>> re-usable descriptors. Maybe we should call the flag >>> DMA_PREP_REUSABLE (like the other DMA_PREP_*) flag to reflect this. >> The flow I have depicted is that descriptor prepare should not be linked to >> it's reuse. The submit operation specifies if a descriptor can be reused or >> not, so driver knows what to do after txn completion >> While reusing if client wants to stop reusing it can clear reuse flag and >> submit so that descriptor is freed up > > I think the same way as Vinod. > > Giving the dmaengine user the flexibility to reuse or not the descriptor is > great. Moreover, this flexibility will extend to userspace, as some dmaengine > users, such as v4l2, don't know if a descriptor will be reused : it's a > userspace decision whever to requeue the transmission or not, up to the driver. Yeah, but typically you don't know whether the descriptor is going to be re-used or not when submitting it. E.g. v4l2. The application will allocate the buffer and then call QBUF ioctl which will submit the buffer. The DMA fills the buffer and once it finishes the application uses the DQBUF ioctl to dequeue it. Now the application might stop capturing, or it might continue. In the later case it will use the QBUF ioctl again and the previously allocated descriptor will be re-used. When the initial QBUF is performed it is not known whether the buffer will be queued a second time and whether it should have the re-usable flag set. So the re-usable flag should an indicator the DMAengine driver that the client might re-use the descriptor, not that it will definitely re-use it. For some drivers/controllers a re-usable descriptors might have special requirements and the DMAengine driver needs to know this during allocation of the descriptor to meet those requirements. Which is why I think that re-usability should be a preparation flag. If it was set during allocation the client is allowed to submit the descriptor multiple times (But does not have to), otherwise not. -- 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
On Mon, Jul 27, 2015 at 01:46:41PM +0200, Lars-Peter Clausen wrote: > On 07/24/2015 09:12 PM, Robert Jarzmik wrote: > >Vinod Koul <vinod.koul@intel.com> writes: > > > >>On Fri, Jul 24, 2015 at 10:28:45AM +0200, Lars-Peter Clausen wrote: > >>>I don' think we need set set and clear functions. We need them for > >>>the ACK flag because the ACK flag can be set later on. But the REUSE > >>>flag should be specified when the descriptor is created so the > >>>driver can make the necessary preparations it might need to support > >>>re-usable descriptors. Maybe we should call the flag > >>>DMA_PREP_REUSABLE (like the other DMA_PREP_*) flag to reflect this. > >>The flow I have depicted is that descriptor prepare should not be linked to > >>it's reuse. The submit operation specifies if a descriptor can be reused or > >>not, so driver knows what to do after txn completion > >>While reusing if client wants to stop reusing it can clear reuse flag and > >>submit so that descriptor is freed up > > > >I think the same way as Vinod. > > > >Giving the dmaengine user the flexibility to reuse or not the descriptor is > >great. Moreover, this flexibility will extend to userspace, as some dmaengine > >users, such as v4l2, don't know if a descriptor will be reused : it's a > >userspace decision whever to requeue the transmission or not, up to the driver. > > Yeah, but typically you don't know whether the descriptor is going > to be re-used or not when submitting it. > > E.g. v4l2. The application will allocate the buffer and then call > QBUF ioctl which will submit the buffer. The DMA fills the buffer > and once it finishes the application uses the DQBUF ioctl to dequeue > it. Now the application might stop capturing, or it might continue. > In the later case it will use the QBUF ioctl again and the > previously allocated descriptor will be re-used. When the initial > QBUF is performed it is not known whether the buffer will be queued > a second time and whether it should have the re-usable flag set. > > So the re-usable flag should an indicator the DMAengine driver that > the client might re-use the descriptor, not that it will definitely > re-use it. For some drivers/controllers a re-usable descriptors > might have special requirements and the DMAengine driver needs to > know this during allocation of the descriptor to meet those > requirements. > > Which is why I think that re-usability should be a preparation flag. > If it was set during allocation the client is allowed to submit the > descriptor multiple times (But does not have to), otherwise not. So in this case you will always set the descriptor as reuse as you dont know while submitting. Such users can always set the flag, while we get benfit from the cases which know about this.
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index e2f5eb419976..2adcd3c1ae48 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -183,6 +183,8 @@ struct dma_interleaved_template { * operation it continues the calculation with new sources * @DMA_PREP_FENCE - tell the driver that subsequent operations depend * on the result of this operation + * @DMA_CTRL_REUSE: Client can reuse the descriptor and submit again till + * cleared or freed */ enum dma_ctrl_flags { DMA_PREP_INTERRUPT = (1 << 0), @@ -191,6 +193,7 @@ enum dma_ctrl_flags { DMA_PREP_PQ_DISABLE_Q = (1 << 3), DMA_PREP_CONTINUE = (1 << 4), DMA_PREP_FENCE = (1 << 5), + DMA_CTRL_REUSE = (1 << 6), }; /** @@ -400,6 +403,8 @@ enum dma_residue_granularity { * @cmd_pause: true, if pause and thereby resume is supported * @cmd_terminate: true, if terminate cmd is supported * @residue_granularity: granularity of the reported transfer residue + * @descriptor_reuse: if a descriptor can be reused by client and + * resubmitted multiple times */ struct dma_slave_caps { u32 src_addr_widths; @@ -408,6 +413,7 @@ struct dma_slave_caps { bool cmd_pause; bool cmd_terminate; enum dma_residue_granularity residue_granularity; + bool descriptor_reuse; }; static inline const char *dma_chan_name(struct dma_chan *chan) @@ -467,6 +473,7 @@ struct dma_async_tx_descriptor { dma_addr_t phys; struct dma_chan *chan; dma_cookie_t (*tx_submit)(struct dma_async_tx_descriptor *tx); + int (*desc_free)(struct dma_async_tx_descriptor *tx); dma_async_tx_callback callback; void *callback_param; struct dmaengine_unmap_data *unmap; @@ -833,6 +840,41 @@ static inline dma_cookie_t dmaengine_submit(struct dma_async_tx_descriptor *desc return desc->tx_submit(desc); } +int dma_get_slave_caps(struct dma_chan *chan, struct dma_slave_caps *caps); + +static inline int dmaengine_desc_set_reuse(struct dma_async_tx_descriptor *tx) +{ + struct dma_slave_caps caps; + + dma_get_slave_caps(tx->chan, &caps); + + if (caps.descriptor_reuse) { + tx->flags |= DMA_CTRL_REUSE; + return 0; + } else { + return -EPERM; + } +} + +static inline void dmaengine_desc_clear_reuse(struct dma_async_tx_descriptor *tx) +{ + tx->flags &= ~DMA_CTRL_REUSE; +} + +static inline bool dmaengine_desc_test_reuse(struct dma_async_tx_descriptor *tx) +{ + return (tx->flags & DMA_CTRL_REUSE) == DMA_CTRL_REUSE; +} + +static inline int dmaengine_desc_free(struct dma_async_tx_descriptor *desc) +{ + /* this is supported for reusable desc, so check that */ + if (!dmaengine_desc_test_reuse(desc)) + return desc->desc_free(desc); + else + return -EPERM; +} + static inline bool dmaengine_check_align(u8 align, size_t off1, size_t off2, size_t len) { size_t mask;
This adds new descriptor flag for reusing a descriptor by submitting multiple times by a client, for example video buffer. Add helper APIs for this as well Signed-off-by: Vinod Koul <vinod.koul@intel.com> --- include/linux/dmaengine.h | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+)