| Message ID | 20180424204158.2764095-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <arnd@arndb.de> > --- > 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); > 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. That probably also means it can use dma_mmap_coherent instead of the handcrafted remap_pfn_range loop and the PageReserved abuse.
On Wed, Apr 25, 2018 at 8:15 AM, Christoph Hellwig <hch@infradead.org> 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
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.
On Wed, Apr 25, 2018 at 9:21 AM, Christoph Hellwig <hch@infradead.org> 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 '\<bus_to_virt\|virt_to_bus\>' 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-xxxx.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
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. > 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 arch specific ones it would good to just ping the relevant maintainers. Especially m68k and parisc folks seems to be very responsive.
On Wed, Apr 25, 2018 at 5:26 PM, Christoph Hellwig <hch@infradead.org> 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. Trent, Hans: do you have reason to believe that there might still be users out there? Arnd
Em Wed, 25 Apr 2018 17:58:25 +0200 Arnd Bergmann <arnd@arndb.de> escreveu: > On Wed, Apr 25, 2018 at 5:26 PM, Christoph Hellwig <hch@infradead.org> 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? > > Arnd Thanks, Mauro
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-janitors@vger.kernel.org and linux-media@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: shadowlord@utanet.at www: http://www.lysator.liu.se/~gz/bernhard
On Wed, 2018-04-25 at 14:22 -0300, Mauro Carvalho Chehab wrote: > Em Wed, 25 Apr 2018 17:58:25 +0200 > Arnd Bergmann <arnd@arndb.de> escreveu: > > > On Wed, Apr 25, 2018 at 5:26 PM, Christoph Hellwig <hch@infradead.o > > rg> 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. I still have a zoran board. And my recently purchased ryzen system has PCI slots. To my surprise they are not uncommon on new socket AM4 boards. However, I think the zoran board I have is 5V PCI and that is rather uncommon. Also becoming uncommon is analog NTSC/PAL video that this chip is designed for! If anyone is using these still, they would be in legacy systems for these legacy video formats.
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 infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 25/04/18 19:22, Mauro Carvalho Chehab wrote: > Em Wed, 25 Apr 2018 17:58:25 +0200 > Arnd Bergmann <arnd@arndb.de> escreveu: > >> On Wed, Apr 25, 2018 at 5:26 PM, Christoph Hellwig <hch@infradead.org> 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
On Mon, May 7, 2018 at 5:05 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote: > On 25/04/18 19:22, Mauro Carvalho Chehab wrote: >> Em Wed, 25 Apr 2018 17:58:25 +0200 >> Arnd Bergmann <arnd@arndb.de> escreveu: >> >>> On Wed, Apr 25, 2018 at 5:26 PM, Christoph Hellwig <hch@infradead.org> 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
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); diff --git a/drivers/media/pci/zoran/zoran_device.c b/drivers/media/pci/zoran/zoran_device.c index 40adceebca7e..1ac7810ddd25 100644 --- a/drivers/media/pci/zoran/zoran_device.c +++ b/drivers/media/pci/zoran/zoran_device.c @@ -430,9 +430,9 @@ zr36057_set_vfe (struct zoran *zr, * zr->overlay_settings.width instead of video_width */ 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; btwrite(reg, ZR36057_MMBR); reg = mask_line_size - (zr->overlay_settings.width + @@ -775,7 +775,7 @@ zr36057_set_jpg (struct zoran *zr, //btor(ZR36057_VFESPFR_VCLKPol, ZR36057_VFESPFR); /* code base address */ - reg = virt_to_bus(zr->stat_com); + reg = zr->stat_com_dma; btwrite(reg, ZR36057_JCBA); /* FIFO threshold (FIFO is 160. double words) */ @@ -1097,7 +1097,7 @@ zoran_feed_stat_com (struct zoran *zr) if (!(zr->stat_com[i] & cpu_to_le32(1))) break; zr->stat_com[i] = - cpu_to_le32(zr->jpg_buffers.buffer[frame].jpg.frag_tab_bus); + cpu_to_le32(zr->jpg_buffers.buffer[frame].jpg.frag_tab_dma); } else { /* fill 2 stat_com entries */ i = ((zr->jpg_dma_head - @@ -1105,9 +1105,9 @@ zoran_feed_stat_com (struct zoran *zr) if (!(zr->stat_com[i] & cpu_to_le32(1))) break; zr->stat_com[i] = - cpu_to_le32(zr->jpg_buffers.buffer[frame].jpg.frag_tab_bus); + cpu_to_le32(zr->jpg_buffers.buffer[frame].jpg.frag_tab_dma); zr->stat_com[i + 1] = - cpu_to_le32(zr->jpg_buffers.buffer[frame].jpg.frag_tab_bus); + cpu_to_le32(zr->jpg_buffers.buffer[frame].jpg.frag_tab_dma); } zr->jpg_buffers.buffer[frame].state = BUZ_STATE_DMA; zr->jpg_dma_head++; @@ -1272,7 +1272,7 @@ error_handler (struct zoran *zr, printk(KERN_INFO "stat_com frames:"); for (j = 0; j < BUZ_NUM_STAT_COM; j++) { for (i = 0; i < zr->jpg_buffers.num_buffers; i++) { - if (le32_to_cpu(zr->stat_com[j]) == zr->jpg_buffers.buffer[i].jpg.frag_tab_bus) + if (le32_to_cpu(zr->stat_com[j]) == zr->jpg_buffers.buffer[i].jpg.frag_tab_dma) printk(KERN_CONT "% d->%d", j, i); } } @@ -1411,7 +1411,7 @@ zoran_irq (int irq, /* Buffer address */ - reg = zr->v4l_buffers.buffer[frame].v4l.fbuffer_bus; + reg = zr->v4l_buffers.buffer[frame].v4l.fbuffer_dma; btwrite(reg, ZR36057_VDTR); if (zr->v4l_settings.height > BUZ_MAX_HEIGHT / 2) reg += zr->v4l_settings.bytesperline; diff --git a/drivers/media/pci/zoran/zoran_driver.c b/drivers/media/pci/zoran/zoran_driver.c index 4b6466961b41..dad1fb02ced2 100644 --- a/drivers/media/pci/zoran/zoran_driver.c +++ b/drivers/media/pci/zoran/zoran_driver.c @@ -235,15 +235,20 @@ static int v4l_fbuffer_alloc(struct zoran_fh *fh) } fh->buffers.buffer[i].v4l.fbuffer = mem; fh->buffers.buffer[i].v4l.fbuffer_phys = virt_to_phys(mem); - fh->buffers.buffer[i].v4l.fbuffer_bus = virt_to_bus(mem); + fh->buffers.buffer[i].v4l.fbuffer_dma = + dma_map_single(&zr->pci_dev->dev, mem, + fh->buffers.buffer_size, + DMA_FROM_DEVICE); + if (!fh->buffers.buffer[i].v4l.fbuffer_dma) + return -ENXIO; for (off = 0; off < fh->buffers.buffer_size; off += PAGE_SIZE) SetPageReserved(virt_to_page(mem + off)); dprintk(4, KERN_INFO - "%s: %s - V4L frame %d mem %p (bus: 0x%llx)\n", + "%s: %s - V4L frame %d mem %p (bus: %pad)\n", ZR_DEVNAME(zr), __func__, i, mem, - (unsigned long long)virt_to_bus(mem)); + &fh->buffers.buffer[i].v4l.fbuffer_dma); } fh->buffers.allocated = 1; @@ -308,6 +313,7 @@ static int jpg_fbuffer_alloc(struct zoran_fh *fh) struct zoran *zr = fh->zr; int i, j, off; u8 *mem; + void **virt_tab; for (i = 0; i < fh->buffers.num_buffers; i++) { if (fh->buffers.buffer[i].jpg.frag_tab) @@ -319,16 +325,19 @@ static int jpg_fbuffer_alloc(struct zoran_fh *fh) /* Allocate fragment table for this buffer */ mem = (void *)get_zeroed_page(GFP_KERNEL); - if (!mem) { + 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; if (fh->buffers.need_contiguous) { mem = kmalloc(fh->buffers.buffer_size, GFP_KERNEL); @@ -340,8 +349,9 @@ static int jpg_fbuffer_alloc(struct zoran_fh *fh) jpg_fbuffer_free(fh); return -ENOBUFS; } + fh->buffers.buffer[i].jpg.frag_virt_tab[0] = mem; fh->buffers.buffer[i].jpg.frag_tab[0] = - cpu_to_le32(virt_to_bus(mem)); + cpu_to_le32(dma_map_single(&zr->pci_dev->dev, mem, fh->buffers.buffer_size, DMA_FROM_DEVICE)); fh->buffers.buffer[i].jpg.frag_tab[1] = cpu_to_le32((fh->buffers.buffer_size >> 1) | 1); for (off = 0; off < fh->buffers.buffer_size; off += PAGE_SIZE) @@ -359,8 +369,9 @@ static int jpg_fbuffer_alloc(struct zoran_fh *fh) return -ENOBUFS; } + fh->buffers.buffer[i].jpg.frag_virt_tab[j] = mem; fh->buffers.buffer[i].jpg.frag_tab[2 * j] = - cpu_to_le32(virt_to_bus(mem)); + cpu_to_le32(dma_map_single(&zr->pci_dev->dev, mem, fh->buffers.buffer_size, DMA_FROM_DEVICE)); fh->buffers.buffer[i].jpg.frag_tab[2 * j + 1] = cpu_to_le32((PAGE_SIZE >> 2) << 1); SetPageReserved(virt_to_page(mem)); @@ -368,6 +379,8 @@ static int jpg_fbuffer_alloc(struct zoran_fh *fh) fh->buffers.buffer[i].jpg.frag_tab[2 * j - 1] |= cpu_to_le32(1); } + + fh->buffers.buffer[i].jpg.frag_tab_dma = dma_map_single(&zr->pci_dev->dev, mem, PAGE_SIZE, DMA_TO_DEVICE); } dprintk(4, @@ -400,9 +413,10 @@ static void jpg_fbuffer_free(struct zoran_fh *fh) frag_tab = buffer->jpg.frag_tab[0]; if (frag_tab) { - mem = bus_to_virt(le32_to_cpu(frag_tab)); + mem = buffer->jpg.frag_virt_tab[0]; for (off = 0; off < fh->buffers.buffer_size; off += PAGE_SIZE) ClearPageReserved(virt_to_page(mem + off)); + dma_unmap_single(&zr->pci_dev->dev, frag_tab, PAGE_SIZE, DMA_FROM_DEVICE); kfree(mem); buffer->jpg.frag_tab[0] = 0; buffer->jpg.frag_tab[1] = 0; @@ -413,14 +427,19 @@ static void jpg_fbuffer_free(struct zoran_fh *fh) if (!frag_tab) break; - ClearPageReserved(virt_to_page(bus_to_virt(le32_to_cpu(frag_tab)))); - free_page((unsigned long)bus_to_virt(le32_to_cpu(frag_tab))); + ClearPageReserved(virt_to_page(buffer->jpg.frag_virt_tab[j])); + dma_unmap_single(&zr->pci_dev->dev, le32_to_cpu(frag_tab), PAGE_SIZE, DMA_FROM_DEVICE); + free_page((unsigned long)buffer->jpg.frag_virt_tab[j]); + buffer->jpg.frag_virt_tab[j] = NULL; buffer->jpg.frag_tab[2 * j] = 0; buffer->jpg.frag_tab[2 * j + 1] = 0; } } + dma_unmap_single(&zr->pci_dev->dev, buffer->jpg.frag_tab_dma, PAGE_SIZE, DMA_TO_DEVICE); free_page((unsigned long)buffer->jpg.frag_tab); + free_page((unsigned long)buffer->jpg.frag_virt_tab); + buffer->jpg.frag_virt_tab = NULL; buffer->jpg.frag_tab = NULL; } @@ -873,6 +892,7 @@ static void zoran_close_end_session(struct zoran_fh *fh) if (!zr->v4l_memgrab_active) zr36057_overlay(zr, 0); zr->overlay_mask = NULL; + zr->overlay_mask_dma = 0; } if (fh->map_mode == ZORAN_MAP_MODE_RAW) { @@ -940,8 +960,10 @@ static int zoran_open(struct file *file) /* used to be BUZ_MAX_WIDTH/HEIGHT, but that gives overflows * on norm-change! */ - fh->overlay_mask = - kmalloc(((768 + 31) / 32) * 576 * 4, GFP_KERNEL); + fh->overlay_mask = dma_alloc_wc(&zr->pci_dev->dev, + ((768 + 31) / 32) * 576 * 4, + &fh->overlay_mask_dma, + GFP_KERNEL); if (!fh->overlay_mask) { dprintk(1, KERN_ERR @@ -1016,6 +1038,7 @@ zoran_close(struct file *file) zr->v4l_overlay_active = 0; zr36057_overlay(zr, 0); zr->overlay_mask = NULL; + zr->overlay_mask_dma = 0; /* capture off */ wake_up_interruptible(&zr->v4l_capq); @@ -1033,7 +1056,8 @@ zoran_close(struct file *file) v4l2_fh_del(&fh->fh); v4l2_fh_exit(&fh->fh); - kfree(fh->overlay_mask); + dma_free_wc(&zr->pci_dev->dev, ((768 + 31) / 32) * 576 * 4, + fh->overlay_mask, fh->overlay_mask_dma); kfree(fh); dprintk(4, KERN_INFO "%s: %s done\n", ZR_DEVNAME(zr), __func__); @@ -1284,6 +1308,7 @@ static int setup_overlay(struct zoran_fh *fh, int on) if (!zr->v4l_memgrab_active) zr36057_overlay(zr, 0); zr->overlay_mask = NULL; + zr->overlay_mask_dma = 0; } else { if (!zr->vbuf_base || !fh->overlay_settings.is_set) { dprintk(1, @@ -1302,6 +1327,7 @@ static int setup_overlay(struct zoran_fh *fh, int on) zr->overlay_active = fh->overlay_active = ZORAN_LOCKED; zr->v4l_overlay_active = 1; zr->overlay_mask = fh->overlay_mask; + zr->overlay_mask_dma = fh->overlay_mask_dma; zr->overlay_settings = fh->overlay_settings; if (!zr->v4l_memgrab_active) zr36057_overlay(zr, 1); @@ -2763,7 +2789,7 @@ zoran_mmap (struct file *file, le32_to_cpu(fh->buffers. buffer[i].jpg.frag_tab[2 * j]); /* should just be pos on i386 */ - page = virt_to_phys(bus_to_virt(pos)) + page = virt_to_phys(fh->buffers.buffer[i].jpg.frag_virt_tab[j]) >> PAGE_SHIFT; if (remap_pfn_range(vma, start, page, todo, PAGE_SHARED)) {
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 <arnd@arndb.de> --- 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(-)