Re: [Mjpeg-users] [PATCH] media: zoran: move to dma-mapping interface
On Mon, May 7, 2018 at 5:05 AM, Hans Verkuil wrote: > On 25/04/18 19:22, Mauro Carvalho Chehab wrote: >> Em Wed, 25 Apr 2018 17:58:25 +0200 >> Arnd Bergmann escreveu: >> >>> On Wed, Apr 25, 2018 at 5:26 PM, Christoph Hellwig >>> wrote: On Wed, Apr 25, 2018 at 01:15:18PM +0200, Arnd Bergmann wrote: > That thought had occurred to me as well. I removed the oldest ISDN > drivers already some years ago, and the OSS sound drivers > got removed as well, and comedi got converted to the dma-mapping > interfaces, so there isn't much left at all now. This is what we > have as of v4.17-rc1: Yes, I've been looking at various grotty old bits to purge. Usually I've been looking for some non-tree wide patches and CCed the last active people to see if they care. In a few cases people do, but most often no one does. >>> >>> Let's start with this one (zoran) then, as Mauro is keen on having >>> all media drivers compile-testable on x86-64 and arm. >>> >>> Trent Piepho and Hans Verkuil both worked on this driver in the >>> 2008/2009 timeframe and those were the last commits from anyone >>> who appears to have tested their patches on actual hardware. >> >> Zoran is a driver for old hardware. I don't doubt that are people >> out there still using it, but who knows? >> >> I have a few those boards packed somewhere. I haven't work with PCI >> hardware for a while. If needed, I can try to seek for them and >> do some tests. I need first to unpack a machine with PCI slots... >> the NUCs I generally use for development don't have any :-) >> >> Anyway, except for virt_to_bus() and related stuff, I think that this >> driver is in good shape, as Hans did a lot of work in the past to >> make it to use the current media framework. >> >>> >>> Trent, Hans: do you have reason to believe that there might still >>> be users out there? > > I have no way of knowing this. However, I think they are easily replaced > by much cheaper USB alternatives today. > > I did some work on the zoran driver several years ago, but it doesn't use > the vb2 framework (and not even the older vb1 framework!) so I'm sure there > are all sorts of bugs in that driver. > > Personally I would be fine with moving this driver to staging and removing > it by, say, the end of the year. > > Nobody is going to work on it and I think it is time to retire it. Based on the link to the old discussed that Bernhard provided, it seems that the driver was already in a barely usable state 5 years ago (the remaining users decided to use an even older 2.6 kernel instead), and nobody has worked on fixing it since, so moving it to staging or immediately removing it would both seem appropriate. Arnd -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Mjpeg-users mailing list Mjpeg-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mjpeg-users
Re: [Mjpeg-users] [PATCH] media: zoran: move to dma-mapping interface
On 25/04/18 19:22, Mauro Carvalho Chehab wrote: > Em Wed, 25 Apr 2018 17:58:25 +0200 > Arnd Bergmann escreveu: > >> On Wed, Apr 25, 2018 at 5:26 PM, Christoph Hellwig >> wrote: >>> On Wed, Apr 25, 2018 at 01:15:18PM +0200, Arnd Bergmann wrote: That thought had occurred to me as well. I removed the oldest ISDN drivers already some years ago, and the OSS sound drivers got removed as well, and comedi got converted to the dma-mapping interfaces, so there isn't much left at all now. This is what we have as of v4.17-rc1: >>> >>> Yes, I've been looking at various grotty old bits to purge. Usually >>> I've been looking for some non-tree wide patches and CCed the last >>> active people to see if they care. In a few cases people do, but >>> most often no one does. >> >> Let's start with this one (zoran) then, as Mauro is keen on having >> all media drivers compile-testable on x86-64 and arm. >> >> Trent Piepho and Hans Verkuil both worked on this driver in the >> 2008/2009 timeframe and those were the last commits from anyone >> who appears to have tested their patches on actual hardware. > > Zoran is a driver for old hardware. I don't doubt that are people > out there still using it, but who knows? > > I have a few those boards packed somewhere. I haven't work with PCI > hardware for a while. If needed, I can try to seek for them and > do some tests. I need first to unpack a machine with PCI slots... > the NUCs I generally use for development don't have any :-) > > Anyway, except for virt_to_bus() and related stuff, I think that this > driver is in good shape, as Hans did a lot of work in the past to > make it to use the current media framework. > >> >> Trent, Hans: do you have reason to believe that there might still >> be users out there? I have no way of knowing this. However, I think they are easily replaced by much cheaper USB alternatives today. I did some work on the zoran driver several years ago, but it doesn't use the vb2 framework (and not even the older vb1 framework!) so I'm sure there are all sorts of bugs in that driver. Personally I would be fine with moving this driver to staging and removing it by, say, the end of the year. Nobody is going to work on it and I think it is time to retire it. Regards, Hans -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Mjpeg-users mailing list Mjpeg-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mjpeg-users
Re: [Mjpeg-users] [PATCH] media: zoran: move to dma-mapping interface
Hi Arnd,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.17-rc2 next-20180426]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Arnd-Bergmann/media-zoran-move-to-dma-mapping-interface/20180426-032120
base: git://linuxtv.org/media_tree.git master
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
>> drivers/media/pci/zoran/zoran_driver.c:419:33: sparse: incorrect type in
>> argument 2 (different base types) @@expected unsigned long long
>> [unsigned] [usertype] addr @@got nsigned long long [unsigned] [usertype]
>> addr @@
drivers/media/pci/zoran/zoran_driver.c:419:33:expected unsigned long
long [unsigned] [usertype] addr
drivers/media/pci/zoran/zoran_driver.c:419:33:got restricted __le32
[assigned] [usertype] frag_tab
vim +419 drivers/media/pci/zoran/zoran_driver.c
395
396 /* free the MJPEG grab buffers */
397 static void jpg_fbuffer_free(struct zoran_fh *fh)
398 {
399 struct zoran *zr = fh->zr;
400 int i, j, off;
401 unsigned char *mem;
402 __le32 frag_tab;
403 struct zoran_buffer *buffer;
404
405 dprintk(4, KERN_DEBUG "%s: %s\n", ZR_DEVNAME(zr), __func__);
406
407 for (i = 0, buffer = &fh->buffers.buffer[0];
408 i < fh->buffers.num_buffers; i++, buffer++) {
409 if (!buffer->jpg.frag_tab)
410 continue;
411
412 if (fh->buffers.need_contiguous) {
413 frag_tab = buffer->jpg.frag_tab[0];
414
415 if (frag_tab) {
416 mem = buffer->jpg.frag_virt_tab[0];
417 for (off = 0; off <
fh->buffers.buffer_size; off += PAGE_SIZE)
418
ClearPageReserved(virt_to_page(mem + off));
> 419 dma_unmap_single(&zr->pci_dev->dev,
> frag_tab, PAGE_SIZE, DMA_FROM_DEVICE);
420 kfree(mem);
421 buffer->jpg.frag_tab[0] = 0;
422 buffer->jpg.frag_tab[1] = 0;
423 }
424 } else {
425 for (j = 0; j < fh->buffers.buffer_size /
PAGE_SIZE; j++) {
426 frag_tab = buffer->jpg.frag_tab[2 * j];
427
428 if (!frag_tab)
429 break;
430
ClearPageReserved(virt_to_page(buffer->jpg.frag_virt_tab[j]));
431 dma_unmap_single(&zr->pci_dev->dev,
le32_to_cpu(frag_tab), PAGE_SIZE, DMA_FROM_DEVICE);
432 free_page((unsigned
long)buffer->jpg.frag_virt_tab[j]);
433 buffer->jpg.frag_virt_tab[j] = NULL;
434 buffer->jpg.frag_tab[2 * j] = 0;
435 buffer->jpg.frag_tab[2 * j + 1] = 0;
436 }
437 }
438
439 dma_unmap_single(&zr->pci_dev->dev,
buffer->jpg.frag_tab_dma, PAGE_SIZE, DMA_TO_DEVICE);
440 free_page((unsigned long)buffer->jpg.frag_tab);
441 free_page((unsigned long)buffer->jpg.frag_virt_tab);
442 buffer->jpg.frag_virt_tab = NULL;
443 buffer->jpg.frag_tab = NULL;
444 }
445
446 fh->buffers.allocated = 0;
447 }
448
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Mjpeg-users mailing list
Mjpeg-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mjpeg-users
Re: [Mjpeg-users] [PATCH] media: zoran: move to dma-mapping interface
Hallo Christoph Hellwig wrote: On Wed, Apr 25, 2018 at 09:08:13AM +0200, Arnd Bergmann wrote: That probably also means it can use dma_mmap_coherent instead of the handcrafted remap_pfn_range loop and the PageReserved abuse. I'd rather not touch that code. How about adding a comment about the fact that it should use dma_mmap_coherent()? Maybe the real question is if there is anyone that actually cares for this driver, or if we are better off just removing it? Same is true for various other virt_to_bus using drivers, e.g. the grotty atm drivers. I would appreciate if somebody would removes the driver from the linux kernel. I suggested that some years ago 2014-06-23 (just scroll down): https://sourceforge.net/p/mjpeg/mailman/mjpeg-users/?viewmonth=201406 You should also find that thread in the kernel-janit...@vger.kernel.org and linux-me...@vger.kernel.org Mailinglist archive. I know of one person that is still using this old type of card's but he uses a 2.6.x Kernel on a old machine, using old software releases. https://sourceforge.net/p/mjpeg/mailman/mjpeg-users/?viewmonth=201709 That type of card were introduced about ~20 years ago. I think it is a good moment to say goodbye to that type of cards. auf hoffentlich bald, Berni the Chaos of Woodquarter Email: shadowl...@utanet.at www: http://www.lysator.liu.se/~gz/bernhard -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Mjpeg-users mailing list Mjpeg-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mjpeg-users
Re: [Mjpeg-users] [PATCH] media: zoran: move to dma-mapping interface
On Tue, Apr 24, 2018 at 10:40:45PM +0200, Arnd Bergmann wrote:
> No drivers should use virt_to_bus() any more. This converts
> one of the few remaining ones to the DMA mapping interface.
>
> Signed-off-by: Arnd Bergmann
> ---
> drivers/media/pci/zoran/Kconfig| 2 +-
> drivers/media/pci/zoran/zoran.h| 10 +--
> drivers/media/pci/zoran/zoran_card.c | 10 +--
> drivers/media/pci/zoran/zoran_device.c | 16 +-
> drivers/media/pci/zoran/zoran_driver.c | 54
> +-
> 5 files changed, 63 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/media/pci/zoran/Kconfig b/drivers/media/pci/zoran/Kconfig
> index 39ec35bd21a5..26f40e124a32 100644
> --- a/drivers/media/pci/zoran/Kconfig
> +++ b/drivers/media/pci/zoran/Kconfig
> @@ -1,6 +1,6 @@
> config VIDEO_ZORAN
> tristate "Zoran ZR36057/36067 Video For Linux"
> - depends on PCI && I2C_ALGOBIT && VIDEO_V4L2 && VIRT_TO_BUS
> + depends on PCI && I2C_ALGOBIT && VIDEO_V4L2
> depends on !ALPHA
> help
> Say Y for support for MJPEG capture cards based on the Zoran
> diff --git a/drivers/media/pci/zoran/zoran.h b/drivers/media/pci/zoran/zoran.h
> index 9bb3c21aa275..9ff3a9acb60a 100644
> --- a/drivers/media/pci/zoran/zoran.h
> +++ b/drivers/media/pci/zoran/zoran.h
> @@ -183,13 +183,14 @@ struct zoran_buffer {
> struct zoran_sync bs; /* DONE: info to return to application
> */
> union {
> struct {
> - __le32 *frag_tab; /* addresses of frag table */
> - u32 frag_tab_bus; /* same value cached to save
> time in ISR */
> + __le32 *frag_tab; /* DMA addresses of frag table
> */
> + void **frag_virt_tab; /* virtual addresses of frag
> table */
> + dma_addr_t frag_tab_dma;/* same value cached to save
> time in ISR */
> } jpg;
> struct {
> char *fbuffer; /* virtual address of frame
> buffer */
> unsigned long fbuffer_phys;/* physical address of frame
> buffer */
> - unsigned long fbuffer_bus;/* bus address of frame
> buffer */
> + dma_addr_t fbuffer_dma;/* bus address of frame buffer */
> } v4l;
> };
> };
> @@ -221,6 +222,7 @@ struct zoran_fh {
>
> struct zoran_overlay_settings overlay_settings;
> u32 *overlay_mask; /* overlay mask */
> + dma_addr_t overlay_mask_dma;
> enum zoran_lock_activity overlay_active;/* feature currently in use? */
>
> struct zoran_buffer_col buffers;/* buffers' info */
> @@ -307,6 +309,7 @@ struct zoran {
>
> struct zoran_overlay_settings overlay_settings;
> u32 *overlay_mask; /* overlay mask */
> + dma_addr_t overlay_mask_dma;
> enum zoran_lock_activity overlay_active;/* feature currently in
> use? */
>
> wait_queue_head_t v4l_capq;
> @@ -346,6 +349,7 @@ struct zoran {
>
> /* zr36057's code buffer table */
> __le32 *stat_com; /* stat_com[i] is indexed by
> dma_head/tail & BUZ_MASK_STAT_COM */
> + dma_addr_t stat_com_dma;
>
> /* (value & BUZ_MASK_FRAME) corresponds to index in pend[] queue */
> int jpg_pend[BUZ_MAX_FRAME];
> diff --git a/drivers/media/pci/zoran/zoran_card.c
> b/drivers/media/pci/zoran/zoran_card.c
> index a6b9ebd20263..dabd8bf77472 100644
> --- a/drivers/media/pci/zoran/zoran_card.c
> +++ b/drivers/media/pci/zoran/zoran_card.c
> @@ -890,6 +890,7 @@ zoran_open_init_params (struct zoran *zr)
> /* User must explicitly set a window */
> zr->overlay_settings.is_set = 0;
> zr->overlay_mask = NULL;
> + zr->overlay_mask_dma = 0;
I don't think this zeroing is required, and given that 0 is a valid
dma address on some platforms is also is rather confusing.:w
>
> mask_line_size = (BUZ_MAX_WIDTH + 31) / 32;
> - reg = virt_to_bus(zr->overlay_mask);
> + reg = zr->overlay_mask_dma;
> btwrite(reg, ZR36057_MMTR);
> - reg = virt_to_bus(zr->overlay_mask + mask_line_size);
> + reg = zr->overlay_mask_dma + mask_line_size;
I think this would be easier to read if the reg assignments got
removed, e.g.
btwrite(zr->overlay_mask_dma, ZR36057_MMTR);
btwrite(zr->overlay_mask_dma + mask_line_size, ZR36057_MMBR);
Same in a few other places.
> + virt_tab = (void *)get_zeroed_page(GFP_KERNEL);
> + if (!mem || !virt_tab) {
> dprintk(1,
> KERN_ERR
> "%s: %s - get_zeroed_page (frag_tab) failed for
> buffer %d\n",
> ZR_DEVNAME(zr), __func__, i);
> + kfree(mem);
> + kfree(virt_tab);
> jpg_fbuffer_free(fh);
>
[Mjpeg-users] [PATCH] media: zoran: move to dma-mapping interface
No drivers should use virt_to_bus() any more. This converts
one of the few remaining ones to the DMA mapping interface.
Signed-off-by: Arnd Bergmann
---
drivers/media/pci/zoran/Kconfig| 2 +-
drivers/media/pci/zoran/zoran.h| 10 +--
drivers/media/pci/zoran/zoran_card.c | 10 +--
drivers/media/pci/zoran/zoran_device.c | 16 +-
drivers/media/pci/zoran/zoran_driver.c | 54 +-
5 files changed, 63 insertions(+), 29 deletions(-)
diff --git a/drivers/media/pci/zoran/Kconfig b/drivers/media/pci/zoran/Kconfig
index 39ec35bd21a5..26f40e124a32 100644
--- a/drivers/media/pci/zoran/Kconfig
+++ b/drivers/media/pci/zoran/Kconfig
@@ -1,6 +1,6 @@
config VIDEO_ZORAN
tristate "Zoran ZR36057/36067 Video For Linux"
- depends on PCI && I2C_ALGOBIT && VIDEO_V4L2 && VIRT_TO_BUS
+ depends on PCI && I2C_ALGOBIT && VIDEO_V4L2
depends on !ALPHA
help
Say Y for support for MJPEG capture cards based on the Zoran
diff --git a/drivers/media/pci/zoran/zoran.h b/drivers/media/pci/zoran/zoran.h
index 9bb3c21aa275..9ff3a9acb60a 100644
--- a/drivers/media/pci/zoran/zoran.h
+++ b/drivers/media/pci/zoran/zoran.h
@@ -183,13 +183,14 @@ struct zoran_buffer {
struct zoran_sync bs; /* DONE: info to return to application
*/
union {
struct {
- __le32 *frag_tab; /* addresses of frag table */
- u32 frag_tab_bus; /* same value cached to save
time in ISR */
+ __le32 *frag_tab; /* DMA addresses of frag table
*/
+ void **frag_virt_tab; /* virtual addresses of frag
table */
+ dma_addr_t frag_tab_dma;/* same value cached to save
time in ISR */
} jpg;
struct {
char *fbuffer; /* virtual address of frame
buffer */
unsigned long fbuffer_phys;/* physical address of frame
buffer */
- unsigned long fbuffer_bus;/* bus address of frame
buffer */
+ dma_addr_t fbuffer_dma;/* bus address of frame buffer */
} v4l;
};
};
@@ -221,6 +222,7 @@ struct zoran_fh {
struct zoran_overlay_settings overlay_settings;
u32 *overlay_mask; /* overlay mask */
+ dma_addr_t overlay_mask_dma;
enum zoran_lock_activity overlay_active;/* feature currently in use? */
struct zoran_buffer_col buffers;/* buffers' info */
@@ -307,6 +309,7 @@ struct zoran {
struct zoran_overlay_settings overlay_settings;
u32 *overlay_mask; /* overlay mask */
+ dma_addr_t overlay_mask_dma;
enum zoran_lock_activity overlay_active;/* feature currently in
use? */
wait_queue_head_t v4l_capq;
@@ -346,6 +349,7 @@ struct zoran {
/* zr36057's code buffer table */
__le32 *stat_com; /* stat_com[i] is indexed by
dma_head/tail & BUZ_MASK_STAT_COM */
+ dma_addr_t stat_com_dma;
/* (value & BUZ_MASK_FRAME) corresponds to index in pend[] queue */
int jpg_pend[BUZ_MAX_FRAME];
diff --git a/drivers/media/pci/zoran/zoran_card.c
b/drivers/media/pci/zoran/zoran_card.c
index a6b9ebd20263..dabd8bf77472 100644
--- a/drivers/media/pci/zoran/zoran_card.c
+++ b/drivers/media/pci/zoran/zoran_card.c
@@ -890,6 +890,7 @@ zoran_open_init_params (struct zoran *zr)
/* User must explicitly set a window */
zr->overlay_settings.is_set = 0;
zr->overlay_mask = NULL;
+ zr->overlay_mask_dma = 0;
zr->overlay_active = ZORAN_FREE;
zr->v4l_memgrab_active = 0;
@@ -1028,7 +1029,8 @@ static int zr36057_init (struct zoran *zr)
/* allocate memory *before* doing anything to the hardware
* in case allocation fails */
- zr->stat_com = kzalloc(BUZ_NUM_STAT_COM * 4, GFP_KERNEL);
+ zr->stat_com = dma_alloc_coherent(&zr->pci_dev->dev,
+ BUZ_NUM_STAT_COM * 4, &zr->stat_com_dma,
GFP_KERNEL);
zr->video_dev = video_device_alloc();
if (!zr->stat_com || !zr->video_dev) {
dprintk(1,
@@ -1072,7 +1074,8 @@ static int zr36057_init (struct zoran *zr)
return 0;
exit_free:
- kfree(zr->stat_com);
+ dma_free_coherent(&zr->pci_dev->dev, BUZ_NUM_STAT_COM * 4,
+ zr->stat_com, zr->stat_com_dma);
kfree(zr->video_dev);
return err;
}
@@ -1107,7 +1110,8 @@ static void zoran_remove(struct pci_dev *pdev)
btwrite(0, ZR36057_SPGPPCR);
free_irq(zr->pci_dev->irq, zr);
/* unmap and free memory */
- kfree(zr->stat_com);
+ dma_free_coherent(&zr->pci_dev->dev, BUZ_NUM_STAT_COM * 4,
+ zr->stat_com, zr->stat_com_dma);
zoran_proc_cleanup(zr);
iounmap(zr->zr36057_mem);
pci_disable_device(zr->pci_dev)
Re: [Mjpeg-users] [PATCH] media: zoran: move to dma-mapping interface
On Wed, Apr 25, 2018 at 9:21 AM, Christoph Hellwig wrote: > On Wed, Apr 25, 2018 at 09:08:13AM +0200, Arnd Bergmann wrote: >> > That probably also means it can use dma_mmap_coherent instead of the >> > handcrafted remap_pfn_range loop and the PageReserved abuse. >> >> I'd rather not touch that code. How about adding a comment about >> the fact that it should use dma_mmap_coherent()? > > Maybe the real question is if there is anyone that actually cares > for this driver, or if we are better off just removing it? > > Same is true for various other virt_to_bus using drivers, e.g. the > grotty atm drivers. That thought had occurred to me as well. I removed the oldest ISDN drivers already some years ago, and the OSS sound drivers got removed as well, and comedi got converted to the dma-mapping interfaces, so there isn't much left at all now. This is what we have as of v4.17-rc1: $ git grep -wl '\' drivers/ drivers/atm/ambassador.c drivers/atm/firestream.c drivers/atm/horizon.c drivers/atm/zatm.c drivers/block/swim3.c # power mac specific drivers/gpu/drm/mga/mga_dma.c # commented out drivers/infiniband/hw/nes/nes_verbs.c # commented out drivers/isdn/hisax/netjet.c drivers/net/appletalk/ltpc.c drivers/net/ethernet/amd/au1000_eth.c # mips specific drivers/net/ethernet/amd/ni65.c # only in comment drivers/net/ethernet/apple/bmac.c # power mac specific drivers/net/ethernet/apple/mace.c # power mac specific drivers/net/ethernet/dec/tulip/de4x5.c # trivially fixable drivers/net/ethernet/i825xx/82596.c # m68k specific drivers/net/ethernet/i825xx/lasi_82596.c # parisc specific drivers/net/ethernet/i825xx/lib82596.c # only in comment drivers/net/ethernet/sgi/ioc3-eth.c # mips specific drivers/net/wan/cosa.c drivers/net/wan/lmc/lmc_main.c drivers/net/wan/z85230.c drivers/scsi/3w-.c # only in comment drivers/scsi/BusLogic.c drivers/scsi/a2091.c # m68k specific drivers/scsi/a3000.c # m68k specific drivers/scsi/dc395x.c # only in comment drivers/scsi/dpt_i2o.c drivers/scsi/gvp11.c # m68k specific drivers/scsi/mvme147.c # m68k specific drivers/scsi/qla1280.c # comment only drivers/staging/netlogic/xlr_net.c # mips specific drivers/tty/serial/cpm_uart/cpm_uart_cpm2.c # ppc32 specific drivers/vme/bridges/vme_ca91cx42.c My feeling is that we want to keep most of the arch specific ones, in particular removing the m68k drivers would break a whole class of machines. For the ones that don't have a comment on them, they probably won't be missed much, but it's hard to know for sure. What we do know is that they never worked on x86_64, and most of them are for ISA cards. Arnd -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Mjpeg-users mailing list Mjpeg-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mjpeg-users
Re: [Mjpeg-users] [PATCH] media: zoran: move to dma-mapping interface
On Wed, Apr 25, 2018 at 09:08:13AM +0200, Arnd Bergmann wrote: > > That probably also means it can use dma_mmap_coherent instead of the > > handcrafted remap_pfn_range loop and the PageReserved abuse. > > I'd rather not touch that code. How about adding a comment about > the fact that it should use dma_mmap_coherent()? Maybe the real question is if there is anyone that actually cares for this driver, or if we are better off just removing it? Same is true for various other virt_to_bus using drivers, e.g. the grotty atm drivers. -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Mjpeg-users mailing list Mjpeg-users@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/mjpeg-users
Re: [Mjpeg-users] [PATCH] media: zoran: move to dma-mapping interface
On Wed, Apr 25, 2018 at 8:15 AM, Christoph Hellwig wrote:
> On Tue, Apr 24, 2018 at 10:40:45PM +0200, Arnd Bergmann wrote:
>> @@ -221,6 +222,7 @@ struct zoran_fh {
>>
>> struct zoran_overlay_settings overlay_settings;
>> u32 *overlay_mask; /* overlay mask */
>> + dma_addr_t overlay_mask_dma;
>> enum zoran_lock_activity overlay_active;/* feature currently in use? */
>>
>> struct zoran_buffer_col buffers;/* buffers' info */
>> @@ -307,6 +309,7 @@ struct zoran {
>>
>> struct zoran_overlay_settings overlay_settings;
>> u32 *overlay_mask; /* overlay mask */
>> + dma_addr_t overlay_mask_dma;
>> enum zoran_lock_activity overlay_active;/* feature currently
>> in use? */
>>
>> wait_queue_head_t v4l_capq;
>> @@ -346,6 +349,7 @@ struct zoran {
>>
>> /* zr36057's code buffer table */
>> __le32 *stat_com; /* stat_com[i] is indexed by
>> dma_head/tail & BUZ_MASK_STAT_COM */
>> + dma_addr_t stat_com_dma;
>>
>> /* (value & BUZ_MASK_FRAME) corresponds to index in pend[] queue */
>> int jpg_pend[BUZ_MAX_FRAME];
>> diff --git a/drivers/media/pci/zoran/zoran_card.c
>> b/drivers/media/pci/zoran/zoran_card.c
>> index a6b9ebd20263..dabd8bf77472 100644
>> --- a/drivers/media/pci/zoran/zoran_card.c
>> +++ b/drivers/media/pci/zoran/zoran_card.c
>> @@ -890,6 +890,7 @@ zoran_open_init_params (struct zoran *zr)
>> /* User must explicitly set a window */
>> zr->overlay_settings.is_set = 0;
>> zr->overlay_mask = NULL;
>> + zr->overlay_mask_dma = 0;
>
> I don't think this zeroing is required, and given that 0 is a valid
> dma address on some platforms is also is rather confusing.:w
The driver does this everywhere, which I also noticed as unnecessary,
but it felt better to be consistent with the rest of the driver here than
to follow common practice.
There are some explicit 'if (zr->overlay_mask)' checks in the driver
that require overlay_mask to be initialized. I could set it to
DMA_ERROR_CODE, but you removed that ;-)
It's easy enough to remove the re-zeroing of the number though
if you still think that's better.
>> mask_line_size = (BUZ_MAX_WIDTH + 31) / 32;
>> - reg = virt_to_bus(zr->overlay_mask);
>> + reg = zr->overlay_mask_dma;
>> btwrite(reg, ZR36057_MMTR);
>> - reg = virt_to_bus(zr->overlay_mask + mask_line_size);
>> + reg = zr->overlay_mask_dma + mask_line_size;
>
> I think this would be easier to read if the reg assignments got
> removed, e.g.
>
> btwrite(zr->overlay_mask_dma, ZR36057_MMTR);
> btwrite(zr->overlay_mask_dma + mask_line_size, ZR36057_MMBR);
>
> Same in a few other places.
Right, good idea.
>> + virt_tab = (void *)get_zeroed_page(GFP_KERNEL);
>> + if (!mem || !virt_tab) {
>> dprintk(1,
>> KERN_ERR
>> "%s: %s - get_zeroed_page (frag_tab) failed
>> for buffer %d\n",
>> ZR_DEVNAME(zr), __func__, i);
>> + kfree(mem);
>> + kfree(virt_tab);
>> jpg_fbuffer_free(fh);
>> return -ENOBUFS;
>> }
>> fh->buffers.buffer[i].jpg.frag_tab = (__le32 *)mem;
>> - fh->buffers.buffer[i].jpg.frag_tab_bus = virt_to_bus(mem);
>> + fh->buffers.buffer[i].jpg.frag_virt_tab = virt_tab;
>
> From a quick look it seems like frag_tab should be a dma_alloc_coherent
> allocation, or you would need a lot of cache sync operations.
Do you mean the table or the buffers it points to? In the code you
quote, we initialize the table with static data before mapping it,
so I would expect either interface to work fine here.
For the actual buffers, I tried to retain the current behavior and
regular kernel memory, in particular because I wasn't too sure
about changing the mmap() function without understanding what
it really does. ;-)
It looks like the buffers are never accessed from the kernel but
only from hardware and user space, so we don't care about whether
we get a mapping that is coherent between the kernel and hardware,
but we probably should ensure that the page table attributes are the
same in kernel and user space (at least powerpc gets a checkstop
for unmatched cacheability flags).
> That probably also means it can use dma_mmap_coherent instead of the
> handcrafted remap_pfn_range loop and the PageReserved abuse.
I'd rather not touch that code. How about adding a comment about
the fact that it should use dma_mmap_coherent()?
Arnd
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Mjpeg-users mailing list
Mjpeg-users@
