Message ID | 1439373533-23299-1-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1ZPSoM-0000ME-HZ; Wed, 12 Aug 2015 11:59:06 +0200 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.76/mailfrontend-7) with esmtp id 1ZPSoK-0007rn-13; Wed, 12 Aug 2015 11:59:06 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753997AbbHLJ7B (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Wed, 12 Aug 2015 05:59:01 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:39136 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751843AbbHLJ7A (ORCPT <rfc822;linux-media@vger.kernel.org>); Wed, 12 Aug 2015 05:59:00 -0400 Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout4.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0NSY00DECRQAEJ20@mailout4.w1.samsung.com> for linux-media@vger.kernel.org; Wed, 12 Aug 2015 10:58:58 +0100 (BST) X-AuditID: cbfec7f5-f794b6d000001495-1c-55cb18e27650 Received: from eusync3.samsung.com ( [203.254.199.213]) by eucpsbgm2.samsung.com (EUCPMTA) with SMTP id 56.F9.05269.2E81BC55; Wed, 12 Aug 2015 10:58:58 +0100 (BST) Received: from amdc1339.digital.local ([106.116.147.30]) by eusync3.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0NSY00BK6RQ7GK80@eusync3.samsung.com>; Wed, 12 Aug 2015 10:58:58 +0100 (BST) From: Marek Szyprowski <m.szyprowski@samsung.com> To: linux-media@vger.kernel.org Cc: Marek Szyprowski <m.szyprowski@samsung.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com> Subject: [PATCH v2] media: videobuf2-dc: set properly dma_max_segment_size Date: Wed, 12 Aug 2015 11:58:53 +0200 Message-id: <1439373533-23299-1-git-send-email-m.szyprowski@samsung.com> X-Mailer: git-send-email 1.9.2 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrHJMWRmVeSWpSXmKPExsVy+t/xq7qPJE6HGhybz2rROXEJu0XPhq2s FmuP3GV3YPaY3TGT1aNvyypGj8+b5AKYo7hsUlJzMstSi/TtErgypnccYSto5amYMf0bUwPj X84uRk4OCQETiR2NW1ghbDGJC/fWs3UxcnEICSxllDh/6D0LhNPEJHG14RtYFZuAoUTX2y42 EFtEQF7iSe8NMJtZIF3iz/LjTCC2sICXxOXzSxlBbBYBVYllvXdZQGxeAQ+Jhx9/sEFsk5P4 /3IF0wRG7gWMDKsYRVNLkwuKk9JzjfSKE3OLS/PS9ZLzczcxQnz9dQfj0mNWhxgFOBiVeHhv 9J0KFWJNLCuuzD3EKMHBrCTC23MfKMSbklhZlVqUH19UmpNafIhRmoNFSZx35q73IUIC6Ykl qdmpqQWpRTBZJg5OqQZGluNrlVaufbdXJ3pC47OfZlGdJUZW/LH7itas2+1ZzHhV6zGvwKlP faeZHxod67s5da8US9jj7zsSrm+2PLRcR9X/hsMsThWljxv9PxeKO1VN8NvlekXzv9fsH7+D xRfuecS/XiC2VeWU1M+tz95eyNWOmH1vbsz+G+qGiziO7gxY+1FciFngpRJLcUaioRZzUXEi AK9cQs7xAQAA Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2015.8.12.94515 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_1600_1699 0, BODY_SIZE_2000_LESS 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, NO_URI_HTTPS 0, SINGLE_URI_IN_BODY 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __SANE_MSGID 0, __SINGLE_URI_TEXT 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_IN_BODY 0, __URI_NO_WWW 0, __URI_NS ' |
Commit Message
Marek Szyprowski
Aug. 12, 2015, 9:58 a.m. UTC
If device has no DMA max_seg_size set, we assume that there is no limit
and it is safe to force it to use DMA_BIT_MASK(32) as max_seg_size to
let DMA-mapping API always create contiguous mappings in DMA address
space. This is essential for all devices, which use dma-contig
videobuf2 memory allocator.
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
Changelog:
v2:
- set max segment size only if a new dma params structure has been
allocated, as suggested by Laurent Pinchart
---
drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
Comments
On 08/12/2015 11:58 AM, Marek Szyprowski wrote: > If device has no DMA max_seg_size set, we assume that there is no limit > and it is safe to force it to use DMA_BIT_MASK(32) as max_seg_size to > let DMA-mapping API always create contiguous mappings in DMA address > space. This is essential for all devices, which use dma-contig > videobuf2 memory allocator. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > Changelog: > v2: > - set max segment size only if a new dma params structure has been > allocated, as suggested by Laurent Pinchart > --- > drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c > index 94c1e64..455e925 100644 > --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c > +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c > @@ -862,6 +862,21 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops); > void *vb2_dma_contig_init_ctx(struct device *dev) > { > struct vb2_dc_conf *conf; > + int err; > + > + /* > + * if device has no max_seg_size set, we assume that there is no limit > + * and force it to DMA_BIT_MASK(32) to always use contiguous mappings > + * in DMA address space > + */ > + if (!dev->dma_parms) { > + dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL); > + if (!dev->dma_parms) > + return ERR_PTR(-ENOMEM); > + err = dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); > + if (err) > + return ERR_PTR(err); > + } I'm not sure if this is such a good idea. The DMA provider is responsible for setting this up. We shouldn't be overwriting this here on the DMA consumer side. This will just mask the bug that the provider didn't setup this correctly and might cause bugs on its own if it is not correct. It will lead to conflicts with DMA providers that have multiple consumers (e.g. shared DMA core). And also the current assumption is that if a driver doesn't set this up explicitly the maximum segement size is 65536. > > conf = kzalloc(sizeof *conf, GFP_KERNEL); > if (!conf) > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On 2015-08-13 14:40, Lars-Peter Clausen wrote: > On 08/12/2015 11:58 AM, Marek Szyprowski wrote: >> If device has no DMA max_seg_size set, we assume that there is no limit >> and it is safe to force it to use DMA_BIT_MASK(32) as max_seg_size to >> let DMA-mapping API always create contiguous mappings in DMA address >> space. This is essential for all devices, which use dma-contig >> videobuf2 memory allocator. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> Changelog: >> v2: >> - set max segment size only if a new dma params structure has been >> allocated, as suggested by Laurent Pinchart >> --- >> drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c >> index 94c1e64..455e925 100644 >> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c >> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c >> @@ -862,6 +862,21 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops); >> void *vb2_dma_contig_init_ctx(struct device *dev) >> { >> struct vb2_dc_conf *conf; >> + int err; >> + >> + /* >> + * if device has no max_seg_size set, we assume that there is no limit >> + * and force it to DMA_BIT_MASK(32) to always use contiguous mappings >> + * in DMA address space >> + */ >> + if (!dev->dma_parms) { >> + dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL); >> + if (!dev->dma_parms) >> + return ERR_PTR(-ENOMEM); >> + err = dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); >> + if (err) >> + return ERR_PTR(err); >> + } > I'm not sure if this is such a good idea. The DMA provider is responsible > for setting this up. We shouldn't be overwriting this here on the DMA > consumer side. This will just mask the bug that the provider didn't setup > this correctly and might cause bugs on its own if it is not correct. It will > lead to conflicts with DMA providers that have multiple consumers (e.g. > shared DMA core). And also the current assumption is that if a driver > doesn't set this up explicitly the maximum segement size is 65536. The problem is that there is no good place for changing this extremely low default value. V4L2 media devices, which use videobuf2-dc expects to get buffers mapped contiguous in the DMA/IO address space. Initially I wanted to have a code for setting dma max segment size directly in the dma-mapping subsystem. This however causeed problems in the other places, as mentioned in the following mail: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html It looks that there are drivers or subsystems which rely on this strange 64k value, rending the whole concept rather useless. Best regards
On Thu, Aug 13, 2015 at 7:49 AM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hello, > > > On 2015-08-13 14:40, Lars-Peter Clausen wrote: >> >> On 08/12/2015 11:58 AM, Marek Szyprowski wrote: >>> >>> If device has no DMA max_seg_size set, we assume that there is no limit >>> and it is safe to force it to use DMA_BIT_MASK(32) as max_seg_size to >>> let DMA-mapping API always create contiguous mappings in DMA address >>> space. This is essential for all devices, which use dma-contig >>> videobuf2 memory allocator. >>> >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> --- >>> Changelog: >>> v2: >>> - set max segment size only if a new dma params structure has been >>> allocated, as suggested by Laurent Pinchart >>> --- >>> drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c >>> b/drivers/media/v4l2-core/videobuf2-dma-contig.c >>> index 94c1e64..455e925 100644 >>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c >>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c >>> @@ -862,6 +862,21 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops); >>> void *vb2_dma_contig_init_ctx(struct device *dev) >>> { >>> struct vb2_dc_conf *conf; >>> + int err; >>> + >>> + /* >>> + * if device has no max_seg_size set, we assume that there is no >>> limit >>> + * and force it to DMA_BIT_MASK(32) to always use contiguous >>> mappings >>> + * in DMA address space >>> + */ >>> + if (!dev->dma_parms) { >>> + dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), >>> GFP_KERNEL); >>> + if (!dev->dma_parms) >>> + return ERR_PTR(-ENOMEM); >>> + err = dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); >>> + if (err) >>> + return ERR_PTR(err); >>> + } >> >> I'm not sure if this is such a good idea. The DMA provider is responsible >> for setting this up. We shouldn't be overwriting this here on the DMA >> consumer side. This will just mask the bug that the provider didn't setup >> this correctly and might cause bugs on its own if it is not correct. It >> will >> lead to conflicts with DMA providers that have multiple consumers (e.g. >> shared DMA core). And also the current assumption is that if a driver >> doesn't set this up explicitly the maximum segement size is 65536. > > > The problem is that there is no good place for changing this extremely low > default > value. V4L2 media devices, which use videobuf2-dc expects to get buffers > mapped > contiguous in the DMA/IO address space. Initially I wanted to have a code > for > setting dma max segment size directly in the dma-mapping subsystem. This > however > causeed problems in the other places, as mentioned in the following mail: > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html > > It looks that there are drivers or subsystems which rely on this strange 64k > value, > rending the whole concept rather useless. > Would this approach make the driver not work on some architectures? Very few drives tweak this value. I found 19 calls to the dma_set_max_seg_size() functions directly and a handful via pci_set_dma_max_seg_size(). Is this one those, "use if you absolutely have to" interfaces? thanks, -- Shuah -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/13/2015 03:49 PM, Marek Szyprowski wrote: > Hello, > > On 2015-08-13 14:40, Lars-Peter Clausen wrote: >> On 08/12/2015 11:58 AM, Marek Szyprowski wrote: >>> If device has no DMA max_seg_size set, we assume that there is no limit >>> and it is safe to force it to use DMA_BIT_MASK(32) as max_seg_size to >>> let DMA-mapping API always create contiguous mappings in DMA address >>> space. This is essential for all devices, which use dma-contig >>> videobuf2 memory allocator. >>> >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> --- >>> Changelog: >>> v2: >>> - set max segment size only if a new dma params structure has been >>> allocated, as suggested by Laurent Pinchart >>> --- >>> drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c >>> b/drivers/media/v4l2-core/videobuf2-dma-contig.c >>> index 94c1e64..455e925 100644 >>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c >>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c >>> @@ -862,6 +862,21 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops); >>> void *vb2_dma_contig_init_ctx(struct device *dev) >>> { >>> struct vb2_dc_conf *conf; >>> + int err; >>> + >>> + /* >>> + * if device has no max_seg_size set, we assume that there is no limit >>> + * and force it to DMA_BIT_MASK(32) to always use contiguous mappings >>> + * in DMA address space >>> + */ >>> + if (!dev->dma_parms) { >>> + dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL); >>> + if (!dev->dma_parms) >>> + return ERR_PTR(-ENOMEM); >>> + err = dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); >>> + if (err) >>> + return ERR_PTR(err); >>> + } >> I'm not sure if this is such a good idea. The DMA provider is responsible >> for setting this up. We shouldn't be overwriting this here on the DMA >> consumer side. This will just mask the bug that the provider didn't setup >> this correctly and might cause bugs on its own if it is not correct. It will >> lead to conflicts with DMA providers that have multiple consumers (e.g. >> shared DMA core). And also the current assumption is that if a driver >> doesn't set this up explicitly the maximum segement size is 65536. > > The problem is that there is no good place for changing this extremely low > default > value. V4L2 media devices, which use videobuf2-dc expects to get buffers mapped > contiguous in the DMA/IO address space. Initially I wanted to have a code for > setting dma max segment size directly in the dma-mapping subsystem. This > however > causeed problems in the other places, as mentioned in the following mail: > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913.html And the same reasoning in that reply still applies here. Try to fix the DMA provider drivers to setup the correct value. - Lars -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 13 August 2015 16:53:56 Lars-Peter Clausen wrote: > On 08/13/2015 03:49 PM, Marek Szyprowski wrote: > > On 2015-08-13 14:40, Lars-Peter Clausen wrote: > >> On 08/12/2015 11:58 AM, Marek Szyprowski wrote: > >>> If device has no DMA max_seg_size set, we assume that there is no limit > >>> and it is safe to force it to use DMA_BIT_MASK(32) as max_seg_size to > >>> let DMA-mapping API always create contiguous mappings in DMA address > >>> space. This is essential for all devices, which use dma-contig > >>> videobuf2 memory allocator. > >>> > >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >>> --- > >>> Changelog: > >>> v2: > >>> - set max segment size only if a new dma params structure has been > >>> > >>> allocated, as suggested by Laurent Pinchart > >>> > >>> --- > >>> > >>> drivers/media/v4l2-core/videobuf2-dma-contig.c | 15 +++++++++++++++ > >>> 1 file changed, 15 insertions(+) > >>> > >>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c > >>> b/drivers/media/v4l2-core/videobuf2-dma-contig.c > >>> index 94c1e64..455e925 100644 > >>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c > >>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c > >>> @@ -862,6 +862,21 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops); > >>> void *vb2_dma_contig_init_ctx(struct device *dev) > >>> { > >>> struct vb2_dc_conf *conf; > >>> + int err; > >>> + > >>> + /* > >>> + * if device has no max_seg_size set, we assume that there is no > >>> limit > >>> + * and force it to DMA_BIT_MASK(32) to always use contiguous > >>> mappings > >>> + * in DMA address space > >>> + */ > >>> + if (!dev->dma_parms) { > >>> + dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL); > >>> + if (!dev->dma_parms) > >>> + return ERR_PTR(-ENOMEM); > >>> + err = dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); > >>> + if (err) > >>> + return ERR_PTR(err); > >>> + } > >> > >> I'm not sure if this is such a good idea. The DMA provider is responsible > >> for setting this up. We shouldn't be overwriting this here on the DMA > >> consumer side. This will just mask the bug that the provider didn't setup > >> this correctly and might cause bugs on its own if it is not correct. It > >> will lead to conflicts with DMA providers that have multiple consumers > >> (e.g. shared DMA core). And also the current assumption is that if a > >> driver doesn't set this up explicitly the maximum segement size is > >> 65536. > > > > The problem is that there is no good place for changing this extremely low > > default value. V4L2 media devices, which use videobuf2-dc expects to get > > buffers mapped contiguous in the DMA/IO address space. Initially I wanted > > to have a code for setting dma max segment size directly in the dma- > > mapping subsystem. This however causeed problems in the other places, as > > mentioned in the following mail: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/305913 > > .html > > And the same reasoning in that reply still applies here. Try to fix the DMA > provider drivers to setup the correct value. Would it make sense to create a helper functions that drivers could use ? I expect all drivers that require contiguous memory buffers larger than 64kB to be broken if they don't set the maximum segment size, which very few of them do.
diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c index 94c1e64..455e925 100644 --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c @@ -862,6 +862,21 @@ EXPORT_SYMBOL_GPL(vb2_dma_contig_memops); void *vb2_dma_contig_init_ctx(struct device *dev) { struct vb2_dc_conf *conf; + int err; + + /* + * if device has no max_seg_size set, we assume that there is no limit + * and force it to DMA_BIT_MASK(32) to always use contiguous mappings + * in DMA address space + */ + if (!dev->dma_parms) { + dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL); + if (!dev->dma_parms) + return ERR_PTR(-ENOMEM); + err = dma_set_max_seg_size(dev, DMA_BIT_MASK(32)); + if (err) + return ERR_PTR(err); + } conf = kzalloc(sizeof *conf, GFP_KERNEL); if (!conf)