| Message ID | 87fuvgndr4.fsf@free-electrons.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
On Thu, Mar 24, 2016 at 03:37:19PM +0100, Gregory CLEMENT wrote: > Hi Dan, > > On mar., mars 22 2016, Dan Williams <dan.j.williams@intel.com> wrote: > > > On Tue, Mar 22, 2016 at 10:55 AM, Gregory CLEMENT > > <gregory.clement@free-electrons.com> wrote: > >> Hi, > >> > >> On mar., mars 22 2016, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > >> > >>> Hi Dan, > >>> > >>> On mar., mars 22 2016, Dan Williams <dan.j.williams@intel.com> wrote: > >>> > >>>> On Tue, Mar 22, 2016 at 8:36 AM, Gregory CLEMENT > >>>> <gregory.clement@free-electrons.com> wrote: > >>>>> > >>>>> Hi, > >>>>> > >>>>> while using the dmatest module to test the mv_xor.c driver, I got > >>>>> failures. I finally found that when the "noverify" parameter is not set, > >>>>> then the buffer length used is totally random (modulo the maximum size > >>>>> of the buffer). But in the mv_xor_prep_dma_xor there is a test about the > >>>>> minimal size the dmaengine can use. So when the length was under this > >>>>> minimum size then I got some test failures like the following: "dmatest: > >>>>> dma1chan0-copy0: result #11: 'prep error' with src_off=0x29c9 > >>>>> dst_off=0x1c51 len=0x33 (0)". > >>>>> > >>>>> Unless the drivers are supposed to accept all buffer size I think it is > >>>>> not an error, and I would like to be able to use this dmatest for > >>>>> automatic test and not to have to check if the error is a real one or > >>>>> not. > >>>>> > >>>>> I think that the following patch could improve the dmatest module. > >>>>> > >>>>> What do you think about it? > >>>>> > >>>>> Did I miss something or is a good ieda to send a proper patch? > >>>> > >>>> Looks like a useful patch to me. > >>> > >>> Thanks for your prompt answer, so I am going to send patch on the ML to > >>> have a real review. > >> > >> In the meantime, my colleague Thomas Petazzoni found that it would make > >> more sens to make the framework aware of this limitation instead of > >> having to know the minimum size for each dma test. We could do it in the > >> same way that it was done for the alignment constraint. > >> > >> What do you think of it? > >> > > > > I think we should just enforce the minimum alignment as the minimum > > transfer size. I don't think we have a need to support unaligned > > transfer sizes. > > So I checked and for this XOR engine there is a real minimum transfer > size, whereas there is no constraint on the alignment. (Actually it is > better to be aligned on the burst size, but it is not mandatory at all) > > So I think it would make sens to add this information, as for the > alignment the modules using the dma engine still can ignore this but it > could be an interesting information. If it's really a minimum transfer size, no one should ignore it, and the framework should reject any transfer size lower than that. Maxime
Hi Maxime, On sam., mars 26 2016, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Thu, Mar 24, 2016 at 03:37:19PM +0100, Gregory CLEMENT wrote: >> Hi Dan, >> >> On mar., mars 22 2016, Dan Williams <dan.j.williams@intel.com> wrote: >> >> > On Tue, Mar 22, 2016 at 10:55 AM, Gregory CLEMENT >> > <gregory.clement@free-electrons.com> wrote: >> >> Hi, >> >> >> >> On mar., mars 22 2016, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: >> >> >> >>> Hi Dan, >> >>> >> >>> On mar., mars 22 2016, Dan Williams <dan.j.williams@intel.com> wrote: >> >>> >> >>>> On Tue, Mar 22, 2016 at 8:36 AM, Gregory CLEMENT >> >>>> <gregory.clement@free-electrons.com> wrote: >> >>>>> >> >>>>> Hi, >> >>>>> >> >>>>> while using the dmatest module to test the mv_xor.c driver, I got >> >>>>> failures. I finally found that when the "noverify" parameter is not set, >> >>>>> then the buffer length used is totally random (modulo the maximum size >> >>>>> of the buffer). But in the mv_xor_prep_dma_xor there is a test about the >> >>>>> minimal size the dmaengine can use. So when the length was under this >> >>>>> minimum size then I got some test failures like the following: "dmatest: >> >>>>> dma1chan0-copy0: result #11: 'prep error' with src_off=0x29c9 >> >>>>> dst_off=0x1c51 len=0x33 (0)". >> >>>>> >> >>>>> Unless the drivers are supposed to accept all buffer size I think it is >> >>>>> not an error, and I would like to be able to use this dmatest for >> >>>>> automatic test and not to have to check if the error is a real one or >> >>>>> not. >> >>>>> >> >>>>> I think that the following patch could improve the dmatest module. >> >>>>> >> >>>>> What do you think about it? >> >>>>> >> >>>>> Did I miss something or is a good ieda to send a proper patch? >> >>>> >> >>>> Looks like a useful patch to me. >> >>> >> >>> Thanks for your prompt answer, so I am going to send patch on the ML to >> >>> have a real review. >> >> >> >> In the meantime, my colleague Thomas Petazzoni found that it would make >> >> more sens to make the framework aware of this limitation instead of >> >> having to know the minimum size for each dma test. We could do it in the >> >> same way that it was done for the alignment constraint. >> >> >> >> What do you think of it? >> >> >> > >> > I think we should just enforce the minimum alignment as the minimum >> > transfer size. I don't think we have a need to support unaligned >> > transfer sizes. >> >> So I checked and for this XOR engine there is a real minimum transfer >> size, whereas there is no constraint on the alignment. (Actually it is >> better to be aligned on the burst size, but it is not mandatory at all) >> >> So I think it would make sens to add this information, as for the >> alignment the modules using the dma engine still can ignore this but it >> could be an interesting information. > > If it's really a minimum transfer size, no one should ignore it, and > the framework should reject any transfer size lower than that. Currently it is the driver who rejects it. What I thought is if the user can have this information it is possible to optimize the transfer. Instead of using a fallback if the size is too small, then the user can either chose to directly not using the DMA or to use a bigger buffer. The first user for this flag will be the dmatest module which won't try to perform DMA transfer with too small buffer. Gregory > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com
On Thu, Mar 24, 2016 at 7:37 AM, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: > Hi Dan, > > On mar., mars 22 2016, Dan Williams <dan.j.williams@intel.com> wrote: > >> On Tue, Mar 22, 2016 at 10:55 AM, Gregory CLEMENT >> <gregory.clement@free-electrons.com> wrote: >>> Hi, >>> >>> On mar., mars 22 2016, Gregory CLEMENT <gregory.clement@free-electrons.com> wrote: >>> >>>> Hi Dan, >>>> >>>> On mar., mars 22 2016, Dan Williams <dan.j.williams@intel.com> wrote: >>>> >>>>> On Tue, Mar 22, 2016 at 8:36 AM, Gregory CLEMENT >>>>> <gregory.clement@free-electrons.com> wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> while using the dmatest module to test the mv_xor.c driver, I got >>>>>> failures. I finally found that when the "noverify" parameter is not set, >>>>>> then the buffer length used is totally random (modulo the maximum size >>>>>> of the buffer). But in the mv_xor_prep_dma_xor there is a test about the >>>>>> minimal size the dmaengine can use. So when the length was under this >>>>>> minimum size then I got some test failures like the following: "dmatest: >>>>>> dma1chan0-copy0: result #11: 'prep error' with src_off=0x29c9 >>>>>> dst_off=0x1c51 len=0x33 (0)". >>>>>> >>>>>> Unless the drivers are supposed to accept all buffer size I think it is >>>>>> not an error, and I would like to be able to use this dmatest for >>>>>> automatic test and not to have to check if the error is a real one or >>>>>> not. >>>>>> >>>>>> I think that the following patch could improve the dmatest module. >>>>>> >>>>>> What do you think about it? >>>>>> >>>>>> Did I miss something or is a good ieda to send a proper patch? >>>>> >>>>> Looks like a useful patch to me. >>>> >>>> Thanks for your prompt answer, so I am going to send patch on the ML to >>>> have a real review. >>> >>> In the meantime, my colleague Thomas Petazzoni found that it would make >>> more sens to make the framework aware of this limitation instead of >>> having to know the minimum size for each dma test. We could do it in the >>> same way that it was done for the alignment constraint. >>> >>> What do you think of it? >>> >> >> I think we should just enforce the minimum alignment as the minimum >> transfer size. I don't think we have a need to support unaligned >> transfer sizes. > > So I checked and for this XOR engine there is a real minimum transfer > size, whereas there is no constraint on the alignment. (Actually it is > better to be aligned on the burst size, but it is not mandatory at all) > > So I think it would make sens to add this information, as for the > alignment the modules using the dma engine still can ignore this but it > could be an interesting information. > > I think to something like that: > > diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h > index 017433712833..a6a2f99fa9f9 100644 > --- a/include/linux/dmaengine.h > +++ b/include/linux/dmaengine.h > @@ -707,6 +707,10 @@ struct dma_device { > enum dmaengine_alignment xor_align; > enum dmaengine_alignment pq_align; > enum dmaengine_alignment fill_align; > + size_t copy_min_len; > + size_t xor_min_len; > + size_t pq_min_len; > + size_t fill_min_len; Perhaps we can get away with only one minimum size and tracker and use it for all cases. I'm concerned about the cache footprint of this structure since it needs to be consulted on every submission. Note that for xor and pq the kernel raid code will never go smaller than 4K. For the 'fill' variable do we even have any engines that export 'fill' capabilities anymore? Let's just do min_len and only add the others if it becomes necessary. -- 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
Hi, On Tue, Mar 29, 2016 at 06:28:07PM +0200, Gregory CLEMENT wrote: > >> So I checked and for this XOR engine there is a real minimum transfer > >> size, whereas there is no constraint on the alignment. (Actually it is > >> better to be aligned on the burst size, but it is not mandatory at all) > >> > >> So I think it would make sens to add this information, as for the > >> alignment the modules using the dma engine still can ignore this but it > >> could be an interesting information. > > > > If it's really a minimum transfer size, no one should ignore it, and > > the framework should reject any transfer size lower than that. > > Currently it is the driver who rejects it. What I thought is if the user > can have this information it is possible to optimize the > transfer. Instead of using a fallback if the size is too small, then the > user can either chose to directly not using the DMA or to use a bigger > buffer. Yes, having it available to the users is something that we definitely need, but you'll also have to check that the users cooperate. And duplicating that code over and over again in the drivers seems counter-productive. Maxime
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 017433712833..a6a2f99fa9f9 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -707,6 +707,10 @@ struct dma_device { enum dmaengine_alignment xor_align; enum dmaengine_alignment pq_align; enum dmaengine_alignment fill_align; + size_t copy_min_len; + size_t xor_min_len; + size_t pq_min_len; + size_t fill_min_len; #define DMA_HAS_PQ_CONTINUE (1 << 15) int dev_id;