* [PATCH 1/1] media: pci: meye: set error code on failures
From: Pan Bian @ 2016-12-04 5:40 UTC (permalink / raw)
To: Mauro Carvalho Chehab, linux-media; +Cc: linux-kernel, Pan Bian
From: Pan Bian <bianpan2016@163.com>
The value of return variable ret is 0 on some error paths, for example,
when pci_resource_start() returns a NULL pointer. 0 means no error in
this context, which is contrary to the fact. This patch fixes the bug.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=189011
Signed-off-by: Pan Bian <bianpan2016@163.com>
---
drivers/media/pci/meye/meye.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/media/pci/meye/meye.c b/drivers/media/pci/meye/meye.c
index ba887e8..115e141 100644
--- a/drivers/media/pci/meye/meye.c
+++ b/drivers/media/pci/meye/meye.c
@@ -1669,6 +1669,7 @@ static int meye_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
goto outenabledev;
}
+ ret = -EIO;
mchip_adr = pci_resource_start(meye.mchip_dev,0);
if (!mchip_adr) {
v4l2_err(v4l2_dev, "meye: mchip has no device base address\n");
--
1.9.1
^ permalink raw reply related
* [PATCH 1/1] media: pci: meye: set error code on failures
From: Pan Bian @ 2016-12-04 5:39 UTC (permalink / raw)
To: Mauro Carvalho Chehab, linux-media; +Cc: linux-kernel, Pan Bian
From: Pan Bian <bianpan2016@163.com>
The value of return variable ret is 0 on some error paths, for example,
when pci_resource_start() returns a NULL pointer. 0 means no error in
this context, which is contrary to the fact. This patch fixes the bug.
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=189011
Signed-off-by: Pan Bian <bianpan2016@163.com>
---
drivers/media/pci/meye/meye.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/media/pci/meye/meye.c b/drivers/media/pci/meye/meye.c
index ba887e8..115e141 100644
--- a/drivers/media/pci/meye/meye.c
+++ b/drivers/media/pci/meye/meye.c
@@ -1669,6 +1669,7 @@ static int meye_probe(struct pci_dev *pcidev, const struct pci_device_id *ent)
goto outenabledev;
}
+ ret = -EIO;
mchip_adr = pci_resource_start(meye.mchip_dev,0);
if (!mchip_adr) {
v4l2_err(v4l2_dev, "meye: mchip has no device base address\n");
--
1.9.1
^ permalink raw reply related
* [linux-next:master 7190/10372] htmldocs: Documentation/doc-guide/sphinx.rst:110: ERROR: Unknown target name: "sphinx c domain".
From: kbuild test robot @ 2016-12-04 6:39 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: kbuild-all, linux-media, Jonathan Corbet, linux-doc
[-- Attachment #1: Type: text/plain, Size: 6111 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head: e05f574a0bb1f4502a4b2264fdb0ef6419cf3772
commit: 1dc4bbf0b268246f6202c761016735933b6f0b99 [7190/10372] docs-rst: doc-guide: split the kernel-documentation.rst contents
reproduce: make htmldocs
All errors (new ones prefixed by >>):
make[3]: warning: jobserver unavailable: using -j1. Add '+' to parent make rule.
include/linux/init.h:1: warning: no structured comments found
include/linux/kthread.h:26: warning: Excess function parameter '...' description in 'kthread_create'
kernel/sys.c:1: warning: no structured comments found
Error: Cannot open file drivers/dma-buf/dma-fence.c
Error: Cannot open file drivers/dma-buf/dma-fence.c
WARNING: kernel-doc 'scripts/kernel-doc -rst -enable-lineno -export drivers/dma-buf/dma-fence.c' failed with return code 2
Error: Cannot open file include/linux/dma-fence.h
Error: Cannot open file include/linux/dma-fence.h
WARNING: kernel-doc 'scripts/kernel-doc -rst -enable-lineno -internal include/linux/dma-fence.h' failed with return code 2
drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
Error: Cannot open file drivers/dma-buf/dma-fence-array.c
Error: Cannot open file drivers/dma-buf/dma-fence-array.c
WARNING: kernel-doc 'scripts/kernel-doc -rst -enable-lineno -export drivers/dma-buf/dma-fence-array.c' failed with return code 2
Error: Cannot open file include/linux/dma-fence-array.h
Error: Cannot open file include/linux/dma-fence-array.h
WARNING: kernel-doc 'scripts/kernel-doc -rst -enable-lineno -internal include/linux/dma-fence-array.h' failed with return code 2
include/uapi/linux/vtpm_proxy.h:1: warning: no structured comments found
drivers/char/tpm/tpm_vtpm_proxy.c:71: warning: No description found for parameter 'filp'
drivers/char/tpm/tpm_vtpm_proxy.c:71: warning: No description found for parameter 'buf'
drivers/char/tpm/tpm_vtpm_proxy.c:71: warning: No description found for parameter 'count'
drivers/char/tpm/tpm_vtpm_proxy.c:71: warning: No description found for parameter 'off'
drivers/char/tpm/tpm_vtpm_proxy.c:121: warning: No description found for parameter 'filp'
drivers/char/tpm/tpm_vtpm_proxy.c:121: warning: No description found for parameter 'buf'
drivers/char/tpm/tpm_vtpm_proxy.c:121: warning: No description found for parameter 'count'
drivers/char/tpm/tpm_vtpm_proxy.c:121: warning: No description found for parameter 'off'
drivers/char/tpm/tpm_vtpm_proxy.c:201: warning: No description found for parameter 'proxy_dev'
drivers/char/tpm/tpm_vtpm_proxy.c:1: warning: no structured comments found
include/sound/compress_driver.h:162: warning: No description found for parameter 'id[64]'
include/sound/compress_driver.h:162: warning: No description found for parameter 'proc_root'
include/sound/compress_driver.h:162: warning: No description found for parameter 'proc_info_entry'
include/net/mac80211.h:3207: ERROR: Unexpected indentation.
include/net/mac80211.h:3210: WARNING: Block quote ends without a blank line; unexpected unindent.
include/net/mac80211.h:3212: ERROR: Unexpected indentation.
include/net/mac80211.h:3213: WARNING: Block quote ends without a blank line; unexpected unindent.
include/net/mac80211.h:1772: ERROR: Unexpected indentation.
include/net/mac80211.h:1776: WARNING: Block quote ends without a blank line; unexpected unindent.
>> Documentation/doc-guide/sphinx.rst:110: ERROR: Unknown target name: "sphinx c domain".
kernel/sched/fair.c:7259: WARNING: Inline emphasis start-string without end-string.
kernel/time/timer.c:1240: ERROR: Unexpected indentation.
kernel/time/timer.c:1242: ERROR: Unexpected indentation.
kernel/time/timer.c:1243: WARNING: Block quote ends without a blank line; unexpected unindent.
include/linux/wait.h:121: WARNING: Block quote ends without a blank line; unexpected unindent.
include/linux/wait.h:124: ERROR: Unexpected indentation.
include/linux/wait.h:126: WARNING: Block quote ends without a blank line; unexpected unindent.
kernel/time/hrtimer.c:1021: WARNING: Block quote ends without a blank line; unexpected unindent.
kernel/signal.c:317: WARNING: Inline literal start-string without end-string.
drivers/base/firmware_class.c:1348: WARNING: Bullet list ends without a blank line; unexpected unindent.
drivers/message/fusion/mptbase.c:5054: WARNING: Definition list ends without a blank line; unexpected unindent.
drivers/tty/serial/serial_core.c:1893: WARNING: Definition list ends without a blank line; unexpected unindent.
include/linux/spi/spi.h:369: ERROR: Unexpected indentation.
drivers/usb/core/message.c:478: ERROR: Unexpected indentation.
drivers/usb/core/message.c:479: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/driver-api/usb.rst:623: ERROR: Unknown target name: "usb_type".
Documentation/driver-api/usb.rst:623: ERROR: Unknown target name: "usb_dir".
Documentation/driver-api/usb.rst:623: ERROR: Unknown target name: "usb_recip".
Documentation/driver-api/usb.rst:689: ERROR: Unknown target name: "usbdevfs_urb_type".
sound/pci/ac97/ac97_codec.c:1908: WARNING: Inline emphasis start-string without end-string.
sound/soc/soc-core.c:2460: ERROR: Unknown target name: "snd_soc_daifmt".
sound/core/jack.c:312: ERROR: Unknown target name: "snd_jack_btn".
Documentation/translations/ko_KR/howto.rst:293: WARNING: Inline emphasis start-string without end-string.
WARNING: dvipng command 'dvipng' cannot be run (needed for math display), check the imgmath_dvipng setting
vim +110 Documentation/doc-guide/sphinx.rst
104 it easier to follow the documents.
105
106
107 the C domain
108 ------------
109
> 110 The `Sphinx C Domain`_ (name c) is suited for documentation of C API. E.g. a
111 function prototype:
112
113 .. code-block:: rst
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6425 bytes --]
^ permalink raw reply
* Re: Enabling peer to peer device transactions for PCIe devices
From: Haggai Eran @ 2016-12-04 7:33 UTC (permalink / raw)
To: Serguei Sagalovitch, Jason Gunthorpe
Cc: linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-nvdimm@ml01.01.org, christian.koenig@amd.com,
Suravee.Suthikulpanit@amd.com, John.Bridgman@amd.com,
Alexander.Deucher@amd.com, Linux-media@vger.kernel.org,
dan.j.williams@intel.com, logang@deltatee.com,
dri-devel@lists.freedesktop.org, Max Gurtovoy,
linux-pci@vger.kernel.org, Paul.Blinzer@amd.com,
Felix.Kuehling@amd.com, ben.sander@amd.com
In-Reply-To: <2560aab2-426c-6e58-cb4f-77ec76e0c941@amd.com>
On 11/30/2016 7:28 PM, Serguei Sagalovitch wrote:
> On 2016-11-30 11:23 AM, Jason Gunthorpe wrote:
>>> Yes, that sounds fine. Can we simply kill the process from the GPU driver?
>>> Or do we need to extend the OOM killer to manage GPU pages?
>> I don't know..
> We could use send_sig_info to send signal from kernel to user space. So theoretically GPU driver
> could issue KILL signal to some process.
>
>> On Wed, Nov 30, 2016 at 12:45:58PM +0200, Haggai Eran wrote:
>>> I think we can achieve the kernel's needs with ZONE_DEVICE and DMA-API support
>>> for peer to peer. I'm not sure we need vmap. We need a way to have a scatterlist
>>> of MMIO pfns, and ZONE_DEVICE allows that.
> I do not think that using DMA-API as it is is the best solution (at least in the current form):
>
> - It deals with handles/fd for the whole allocation but client could/will use sub-allocation as
> well as theoretically possible to "merge" several allocations in one from GPU perspective.
> - It require knowledge to export but because "sharing" is controlled from user space it
> means that we must "export" all allocation by default
> - It deals with 'fd'/handles but user application may work with addresses/pointers.
Aren't you confusing DMABUF and DMA-API? DMA-API is how you program the IOMMU (dma_map_page/dma_map_sg/etc.).
The comment above is just about the need to extend this API to allow mapping peer device pages to bus addresses.
In the past I sent an RFC for using DMABUF for peer to peer. I think it had some
advantages for legacy devices. I agree that working with addresses and pointers through
something like HMM/ODP is much more flexible and easier to program from user-space.
For legacy, DMABUF would have allowed you a way to pin the pages so the GPU knows not to
move them. However, that can probably also be achieved simply via the reference count
on ZONE_DEVICE pages. The other nice thing about DMABUF is that it migrate the buffer
itself during attachment according to the requirements of the device that is attaching,
so you can automatically decide in the exporter whether to use p2p or a staging buffer.
>
> Also current DMA-API force each time to do all DMA table programming unrelated if
> location was changed or not. With vma / mmu we are able to install notifier to intercept
> changes in location and update translation tables only as needed (we do not need to keep
> get_user_pages() lock).
I agree.
^ permalink raw reply
* Re: Enabling peer to peer device transactions for PCIe devices
From: Haggai Eran @ 2016-12-04 7:42 UTC (permalink / raw)
To: Logan Gunthorpe, Jason Gunthorpe
Cc: linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-nvdimm@ml01.01.org, christian.koenig@amd.com,
Suravee.Suthikulpanit@amd.com, John.Bridgman@amd.com,
Alexander.Deucher@amd.com, Linux-media@vger.kernel.org,
dan.j.williams@intel.com, dri-devel@lists.freedesktop.org,
Max Gurtovoy, linux-pci@vger.kernel.org,
serguei.sagalovitch@amd.com, Paul.Blinzer@amd.com,
Felix.Kuehling@amd.com, ben.sander@amd.com
In-Reply-To: <5f5b7989-84f5-737e-47c8-831f752d6280@deltatee.com>
On 11/30/2016 8:01 PM, Logan Gunthorpe wrote:
>
>
> On 30/11/16 09:23 AM, Jason Gunthorpe wrote:
>>> Two cases I can think of are RDMA access to an NVMe device's controller
>>> memory buffer,
>>
>> I'm not sure on the use model there..
>
> The NVMe fabrics stuff could probably make use of this. It's an
> in-kernel system to allow remote access to an NVMe device over RDMA. So
> they ought to be able to optimize their transfers by DMAing directly to
> the NVMe's CMB -- no userspace interface would be required but there
> would need some kernel infrastructure.
Yes, that's what I was thinking. The NVMe/f driver needs to map the CMB for
RDMA. I guess if it used ZONE_DEVICE like in the iopmem patches it would be
relatively easy to do.
^ permalink raw reply
* Re: Enabling peer to peer device transactions for PCIe devices
From: Haggai Eran @ 2016-12-04 7:53 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-nvdimm@ml01.01.org, christian.koenig@amd.com,
Suravee.Suthikulpanit@amd.com, John.Bridgman@amd.com,
Alexander.Deucher@amd.com, Linux-media@vger.kernel.org,
dan.j.williams@intel.com, logang@deltatee.com,
dri-devel@lists.freedesktop.org, Max Gurtovoy,
linux-pci@vger.kernel.org, serguei.sagalovitch@amd.com,
Paul.Blinzer@amd.com, Felix.Kuehling@amd.com, ben.sander@amd.com
In-Reply-To: <20161130162353.GA24639@obsidianresearch.com>
On 11/30/2016 6:23 PM, Jason Gunthorpe wrote:
>> and O_DIRECT operations that access GPU memory.
> This goes through user space so there is still a VMA..
>
>> Also, HMM's migration between two GPUs could use peer to peer in the
>> kernel, although that is intended to be handled by the GPU driver if
>> I understand correctly.
> Hum, presumably these migrations are VMA backed as well...
I guess so.
>>> Presumably in-kernel could use a vmap or something and the same basic
>>> flow?
>> I think we can achieve the kernel's needs with ZONE_DEVICE and DMA-API support
>> for peer to peer. I'm not sure we need vmap. We need a way to have a scatterlist
>> of MMIO pfns, and ZONE_DEVICE allows that.
> Well, if there is no virtual map then we are back to how do you do
> migrations and other things people seem to want to do on these
> pages. Maybe the loose 'struct page' flow is not for those users.
I was thinking that kernel use cases would disallow migration, similar to how
non-ODP MRs would work. Either they are short-lived (like an O_DIRECT transfer)
or they can be longed lived but non-migratable (like perhaps a CMB staging buffer).
> But I think if you want kGPU or similar then you probably need vmaps
> or something similar to represent the GPU pages in kernel memory.
Right, although sometimes the GPU pages are simply inaccessible to the CPU.
In any case, I haven't thought about kGPU as a use-case.
^ permalink raw reply
* Hopefully
From: Rita Micheal @ 2016-12-04 11:45 UTC (permalink / raw)
Hi friend I am a banker in ADB BANK. I want to transfer an abandoned
$25.5Million to your Bank account. 40/percent will be your share.
No risks involved but keep it as secret. Contact me for more details
And also acknowledge receipt of this message in acceptance of my
mutual business Endeavour by furnishing me with the following:
1. Your Full Names and Address.
2. Direct Telephone and Fax numbers
Please reply in my private email address (michealrita17@gmail.com) for
security and confidential reasons.
Yours
Miss Rita Micheal
^ permalink raw reply
* Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.
From: Marcel Hasler @ 2016-12-04 13:01 UTC (permalink / raw)
To: Ezequiel Garcia; +Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-media
In-Reply-To: <CAAEAJfCmQnQHWy+7kS4wuuBK7mubiKRpiDYCm9BHYjVR4yHGgA@mail.gmail.com>
Hello
2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
> <mchehab@s-opensource.com> wrote:
>> Em Sun, 27 Nov 2016 12:11:48 +0100
>> Marcel Hasler <mahasler@gmail.com> escreveu:
>>
>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
>>> driver also sets this to 8 without any possibility for changing it.
>>
>> The problem of removing the mixer is that you need this kind of
>> crap to setup the volumes on a non-standard way.
>>
>
> Right, that's a good point.
>
>> NACK.
>>
>> Instead, keep the alsa mixer. The way other drivers do (for example,
>> em28xx) is that they configure the mixer when an input is selected,
>> increasing the volume of the active audio channel to 100% and muting
>> the other audio channels. Yet, as the alsa mixer is exported, users
>> can change the mixer settings in runtime using some alsa (or pa)
>> mixer application.
>>
>
> Yeah, the AC97 mixer we are currently leveraging
> exposes many controls that have no meaning in this device,
> so removing that still looks like an improvement.
>
> I guess the proper way is creating our own mixer
> (not using snd_ac97_mixer) exposing only the record
> gain knob.
>
> Marcel, what do you think?
> --
> Ezequiel García, VanguardiaSur
> www.vanguardiasur.com.ar
As I have written before, the recording gain isn't actually meant to
be changed by the user. In the official Windows driver this value is
hard-coded to 8 and cannot be changed in any way. And there really is
no good reason why anyone should need to mess with it in the first
place. The default value will give the best results in pretty much all
cases and produces approximately the same volume as the internal 8-bit
ADC whose gain cannot be changed at all, not even by a driver.
I had considered writing a mixer but chose not to. If the gain setting
is openly exposed to mixer applications, how do you tell the users
that the value set by the driver already is the optimal and
recommended value and that they shouldn't mess with the controls
unless they really have to? By having a module parameter, this setting
is practically hidden from the normal user but still is available to
power-users if they think they really need it. In the end it's really
just a compromise between hiding it completely and exposing it openly.
Also, this way the driver guarantees reproducible results, since
there's no need to remember the positions of any volume sliders.
Either way, if you still think this solution is "crap", feel free to
modify the patches in any way you see fit. I've wasted too much time
on this already, and since I'm not being paid for it, I don't intend
to put any more effort into this.
Best regards
Marcel
^ permalink raw reply
* Re: Enabling peer to peer device transactions for PCIe devices
From: Stephen Bates @ 2016-12-04 13:06 UTC (permalink / raw)
To: Haggai Eran
Cc: Logan Gunthorpe, Jason Gunthorpe, linux-kernel@vger.kernel.org,
linux-rdma@vger.kernel.org, linux-nvdimm@ml01.01.org,
christian.koenig@amd.com, Suravee.Suthikulpanit@amd.com,
John.Bridgman@amd.com, Alexander.Deucher@amd.com,
Linux-media@vger.kernel.org, dan.j.williams@intel.com,
dri-devel@lists.freedesktop.org, Max Gurtovoy,
linux-pci@vger.kernel.org, serguei.sagalovitch@amd.com,
Paul.Blinzer@amd.com, Felix.Kuehling@amd.com, ben.sander@amd.com
In-Reply-To: <c1ead8a0-6850-fc84-2793-b986f5c1f726@mellanox.com>
>>
>> The NVMe fabrics stuff could probably make use of this. It's an
>> in-kernel system to allow remote access to an NVMe device over RDMA. So
>> they ought to be able to optimize their transfers by DMAing directly to
>> the NVMe's CMB -- no userspace interface would be required but there
>> would need some kernel infrastructure.
>
> Yes, that's what I was thinking. The NVMe/f driver needs to map the CMB
> for RDMA. I guess if it used ZONE_DEVICE like in the iopmem patches it
> would be relatively easy to do.
>
Haggai, yes that was one of the use cases we considered when we put
together the patchset.
^ permalink raw reply
* Re: Enabling peer to peer device transactions for PCIe devices
From: Stephen Bates @ 2016-12-04 13:23 UTC (permalink / raw)
To: Haggai Eran
Cc: Logan Gunthorpe, Jason Gunthorpe, linux-kernel@vger.kernel.org,
linux-rdma@vger.kernel.org, linux-nvdimm@ml01.01.org,
christian.koenig@amd.com, Suravee.Suthikulpanit@amd.com,
John.Bridgman@amd.com, Alexander.Deucher@amd.com,
Linux-media@vger.kernel.org, dan.j.williams@intel.com,
dri-devel@lists.freedesktop.org, Max Gurtovoy,
linux-pci@vger.kernel.org, serguei.sagalovitch@amd.com,
Paul.Blinzer@amd.com, Felix.Kuehling@amd.com, ben.sander@amd.com
In-Reply-To: <c1ead8a0-6850-fc84-2793-b986f5c1f726@mellanox.com>
Hi All
This has been a great thread (thanks to Alex for kicking it off) and I
wanted to jump in and maybe try and put some summary around the
discussion. I also wanted to propose we include this as a topic for LFS/MM
because I think we need more discussion on the best way to add this
functionality to the kernel.
As far as I can tell the people looking for P2P support in the kernel fall
into two main camps:
1. Those who simply want to expose static BARs on PCIe devices that can be
used as the source/destination for DMAs from another PCIe device. This
group has no need for memory invalidation and are happy to use
physical/bus addresses and not virtual addresses.
2. Those who want to support devices that suffer from occasional memory
pressure and need to invalidate memory regions from time to time. This
camp also would like to use virtual addresses rather than physical ones to
allow for things like migration.
I am wondering if people agree with this assessment?
I think something like the iopmem patches Logan and I submitted recently
come close to addressing use case 1. There are some issues around
routability but based on feedback to date that does not seem to be a
show-stopper for an initial inclusion.
For use-case 2 it looks like there are several options and some of them
(like HMM) have been around for quite some time without gaining
acceptance. I think there needs to be more discussion on this usecase and
it could be some time before we get something upstreamable.
I for one, would really like to see use case 1 get addressed soon because
we have consumers for it coming soon in the form of CMBs for NVMe devices.
Long term I think Jason summed it up really well. CPU vendors will put
high-speed, open, switchable, coherent buses on their processors and all
these problems will vanish. But I ain't holding my breathe for that to
happen ;-).
Cheers
Stephen
^ permalink raw reply
* Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.
From: Ezequiel Garcia @ 2016-12-04 18:25 UTC (permalink / raw)
To: Marcel Hasler; +Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-media
In-Reply-To: <CAOJOY2Nhi6aev=jwVeyuQMxKUAk-MfT0YLKsFfrUsAcZtdrysQ@mail.gmail.com>
On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:
> Hello
>
> 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
>> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
>> <mchehab@s-opensource.com> wrote:
>>> Em Sun, 27 Nov 2016 12:11:48 +0100
>>> Marcel Hasler <mahasler@gmail.com> escreveu:
>>>
>>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
>>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
>>>> driver also sets this to 8 without any possibility for changing it.
>>>
>>> The problem of removing the mixer is that you need this kind of
>>> crap to setup the volumes on a non-standard way.
>>>
>>
>> Right, that's a good point.
>>
>>> NACK.
>>>
>>> Instead, keep the alsa mixer. The way other drivers do (for example,
>>> em28xx) is that they configure the mixer when an input is selected,
>>> increasing the volume of the active audio channel to 100% and muting
>>> the other audio channels. Yet, as the alsa mixer is exported, users
>>> can change the mixer settings in runtime using some alsa (or pa)
>>> mixer application.
>>>
>>
>> Yeah, the AC97 mixer we are currently leveraging
>> exposes many controls that have no meaning in this device,
>> so removing that still looks like an improvement.
>>
>> I guess the proper way is creating our own mixer
>> (not using snd_ac97_mixer) exposing only the record
>> gain knob.
>>
>> Marcel, what do you think?
>> --
>> Ezequiel García, VanguardiaSur
>> www.vanguardiasur.com.ar
>
> As I have written before, the recording gain isn't actually meant to
> be changed by the user. In the official Windows driver this value is
> hard-coded to 8 and cannot be changed in any way. And there really is
> no good reason why anyone should need to mess with it in the first
> place. The default value will give the best results in pretty much all
> cases and produces approximately the same volume as the internal 8-bit
> ADC whose gain cannot be changed at all, not even by a driver.
>
> I had considered writing a mixer but chose not to. If the gain setting
> is openly exposed to mixer applications, how do you tell the users
> that the value set by the driver already is the optimal and
> recommended value and that they shouldn't mess with the controls
> unless they really have to? By having a module parameter, this setting
> is practically hidden from the normal user but still is available to
> power-users if they think they really need it. In the end it's really
> just a compromise between hiding it completely and exposing it openly.
> Also, this way the driver guarantees reproducible results, since
> there's no need to remember the positions of any volume sliders.
>
Hm, right. I've never changed the record gain, and it's true that it
doens't really improve the volume. So, I would be OK with having
a module parameter.
On the other side, we are exposing it currently, through the "Capture"
mixer control:
Simple mixer control 'Capture',0
Capabilities: cvolume cswitch cswitch-joined
Capture channels: Front Left - Front Right
Limits: Capture 0 - 15
Front Left: Capture 10 [67%] [15.00dB] [on]
Front Right: Capture 8 [53%] [12.00dB] [on]
So, it would be user-friendly to keep the user interface and continue
to expose the same knob - even if the default is the optimal, etc.
To be completely honest, I don't think any user is really relying
on any REC_GAIN / Capture setting, and I'm completely OK
with having a mixer control or a module parameter. It doesn't matter.
What matters here, is getting rid of the stupid AC97 mixer,
with a dozen of playback and capture controls that have no meaning
whatsoever.
> Either way, if you still think this solution is "crap", feel free to
> modify the patches in any way you see fit. I've wasted too much time
> on this already, and since I'm not being paid for it, I don't intend
> to put any more effort into this.
>
FWIW, I don't think your patches are crap. Quite the opposite,
it's refreshing to see such good stuff being submitted.
After the click noise you fixed in snd-usb-audio, removing the mixer
is the last TODO thing in this driver. So I really appreciate all the time
you have put in it.
--
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar
^ permalink raw reply
* [PATCH] [media] pctv452e: fix double lock bug
From: Iago Abal @ 2016-12-04 16:32 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, Iago Abal
From: Iago Abal <mail@iagoabal.eu>
Commit 73d5c5c864f4 protected the body of `tt3650_ci_msg' with state->ca_mutex.
This is not needed: 1) this function is always called from a context where that
lock is held; and 2) there exists a `tt3650_ci_msg_locked' wrapper that does
exactly the same.
This leads to double lock as reported by the EBA (http://www.iagoabal.eu/eba)
static bug finder:
a) Function `tt3650_ci_msg_locked' takes state->ca_mutex in line 156 and
then calls `tt3650_ci_msg' in line 157.
b) Function `tt3650_ci_slot_reset' takes state->ca_mutex in line 297 and
then calls `tt3650_ci_msg' in line 299.
Fixes: 73d5c5c864f4 ("[media] pctv452e: don't do DMA on stack")
Signed-off-by: Iago Abal <mail@iagoabal.eu>
---
drivers/media/usb/dvb-usb/pctv452e.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/media/usb/dvb-usb/pctv452e.c b/drivers/media/usb/dvb-usb/pctv452e.c
index 07fa08b..c718fd9 100644
--- a/drivers/media/usb/dvb-usb/pctv452e.c
+++ b/drivers/media/usb/dvb-usb/pctv452e.c
@@ -114,7 +114,6 @@ static int tt3650_ci_msg(struct dvb_usb_device *d, u8 cmd, u8 *data,
return -EIO;
}
- mutex_lock(&state->ca_mutex);
id = state->c++;
state->data[0] = SYNC_BYTE_OUT;
@@ -136,7 +135,6 @@ static int tt3650_ci_msg(struct dvb_usb_device *d, u8 cmd, u8 *data,
memcpy(data, state->data + 4, read_len);
- mutex_unlock(&state->ca_mutex);
return 0;
failed:
--
1.9.1
^ permalink raw reply related
* cron job: media_tree daily build: ERRORS
From: Hans Verkuil @ 2016-12-05 5:11 UTC (permalink / raw)
To: linux-media
This message is generated daily by a cron job that builds media_tree for
the kernels and architectures in the list below.
Results of the daily build of media_tree:
date: Mon Dec 5 05:00:16 CET 2016
media-tree git hash: 365fe4e0ce218dc5ad10df17b150a366b6015499
media_build git hash: 1606032398b1d79149c1507be2029e1a00d8dff0
v4l-utils git hash: 063d1f5d5e60783002d781e8a23911acbda65e99
gcc version: i686-linux-gcc (GCC) 6.2.0
sparse version: v0.5.0-3553-g78b2ea6
smatch version: v0.5.0-3553-g78b2ea6
host hardware: x86_64
host os: 4.8.0-164
linux-git-arm-at91: OK
linux-git-arm-davinci: OK
linux-git-arm-multi: OK
linux-git-arm-pxa: OK
linux-git-blackfin-bf561: OK
linux-git-i686: OK
linux-git-m32r: OK
linux-git-mips: OK
linux-git-powerpc64: OK
linux-git-sh: OK
linux-git-x86_64: OK
linux-2.6.36.4-i686: WARNINGS
linux-2.6.37.6-i686: WARNINGS
linux-2.6.38.8-i686: WARNINGS
linux-2.6.39.4-i686: WARNINGS
linux-3.0.60-i686: WARNINGS
linux-3.1.10-i686: WARNINGS
linux-3.2.37-i686: WARNINGS
linux-3.3.8-i686: WARNINGS
linux-3.4.27-i686: WARNINGS
linux-3.5.7-i686: WARNINGS
linux-3.6.11-i686: WARNINGS
linux-3.7.4-i686: WARNINGS
linux-3.8-i686: WARNINGS
linux-3.9.2-i686: WARNINGS
linux-3.10.1-i686: WARNINGS
linux-3.11.1-i686: OK
linux-3.12.67-i686: OK
linux-3.13.11-i686: WARNINGS
linux-3.14.9-i686: WARNINGS
linux-3.15.2-i686: WARNINGS
linux-3.16.7-i686: WARNINGS
linux-3.17.8-i686: WARNINGS
linux-3.18.7-i686: WARNINGS
linux-3.19-i686: WARNINGS
linux-4.0.9-i686: WARNINGS
linux-4.1.33-i686: WARNINGS
linux-4.2.8-i686: WARNINGS
linux-4.3.6-i686: WARNINGS
linux-4.4.22-i686: WARNINGS
linux-4.5.7-i686: WARNINGS
linux-4.6.7-i686: WARNINGS
linux-4.7.5-i686: WARNINGS
linux-4.8-i686: OK
linux-4.9-rc5-i686: OK
linux-2.6.36.4-x86_64: WARNINGS
linux-2.6.37.6-x86_64: WARNINGS
linux-2.6.38.8-x86_64: WARNINGS
linux-2.6.39.4-x86_64: WARNINGS
linux-3.0.60-x86_64: WARNINGS
linux-3.1.10-x86_64: WARNINGS
linux-3.2.37-x86_64: WARNINGS
linux-3.3.8-x86_64: WARNINGS
linux-3.4.27-x86_64: WARNINGS
linux-3.5.7-x86_64: WARNINGS
linux-3.6.11-x86_64: WARNINGS
linux-3.7.4-x86_64: WARNINGS
linux-3.8-x86_64: WARNINGS
linux-3.9.2-x86_64: WARNINGS
linux-3.10.1-x86_64: WARNINGS
linux-3.11.1-x86_64: OK
linux-3.12.67-x86_64: OK
linux-3.13.11-x86_64: WARNINGS
linux-3.14.9-x86_64: WARNINGS
linux-3.15.2-x86_64: WARNINGS
linux-3.16.7-x86_64: WARNINGS
linux-3.17.8-x86_64: WARNINGS
linux-3.18.7-x86_64: WARNINGS
linux-3.19-x86_64: WARNINGS
linux-4.0.9-x86_64: WARNINGS
linux-4.1.33-x86_64: WARNINGS
linux-4.2.8-x86_64: WARNINGS
linux-4.3.6-x86_64: WARNINGS
linux-4.4.22-x86_64: WARNINGS
linux-4.5.7-x86_64: WARNINGS
linux-4.6.7-x86_64: WARNINGS
linux-4.7.5-x86_64: WARNINGS
linux-4.8-x86_64: OK
linux-4.9-rc5-x86_64: OK
apps: WARNINGS
spec-git: OK
smatch: ERRORS
sparse: WARNINGS
Detailed results are available here:
http://www.xs4all.nl/~hverkuil/logs/Monday.log
Full logs are available here:
http://www.xs4all.nl/~hverkuil/logs/Monday.tar.bz2
The Media Infrastructure API from this daily build is here:
http://www.xs4all.nl/~hverkuil/spec/index.html
^ permalink raw reply
* [PATCH] UVC Module - Support Intel RealSense SR300 Depth Camera formats
From: Raikhel, Evgeni @ 2016-12-05 10:06 UTC (permalink / raw)
To: linux-media@vger.kernel.org; +Cc: laurent.pinchart@ideasonboard.com
[-- Attachment #1: Type: text/plain, Size: 558 bytes --]
Specify GUID and FourCC codes mapping for Depth-related pixel formats advertised by Intel RealSense(tm) SR300 depth camera.
Provide documentation for the new INZI pixel format introduced.
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-UVC-Add-support-for-Intel-SR300-depth-camera.patch --]
[-- Type: text/x-patch; name="0001-UVC-Add-support-for-Intel-SR300-depth-camera.patch", Size: 3227 bytes --]
From e9693dd008bdaaafa2e2b57762cb75f84649de2e Mon Sep 17 00:00:00 2001
From: Aviv Greenberg <aviv.d.greenberg@intel.com>
Date: Tue, 15 Nov 2016 12:10:09 +0200
Subject: [PATCH 1/2] UVC: Add support for Intel SR300 depth camera
Add support for Intel SR300 depth camera in uvc driver.
This includes adding three uvc GUIDs for the required pixel formats,
adding a new V4L pixel format definition to user api headers,
and updating the uvc driver GUID-to-4cc tables with the new formats.
Signed-off-by: Aviv Greenberg <aviv.d.greenberg@intel.com>
Signed-off-by: Evgeni Raikhel <evgeni.raikhel@intel.com>
---
drivers/media/usb/uvc/uvc_driver.c | 15 +++++++++++++++
drivers/media/usb/uvc/uvcvideo.h | 9 +++++++++
include/uapi/linux/videodev2.h | 1 +
3 files changed, 25 insertions(+)
diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index 11744f92097b..5b96a89f29ae 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -168,6 +168,21 @@ static struct uvc_format_desc uvc_fmts[] = {
.guid = UVC_GUID_FORMAT_RW10,
.fcc = V4L2_PIX_FMT_SRGGB10P,
},
+ {
+ .name = "Depth data 16-bit (Z16)",
+ .guid = UVC_GUID_FORMAT_INVZ,
+ .fcc = V4L2_PIX_FMT_Z16,
+ },
+ {
+ .name = "IR:Depth 26-bit (INZI)",
+ .guid = UVC_GUID_FORMAT_INZI,
+ .fcc = V4L2_PIX_FMT_INZI,
+ },
+ {
+ .name = "Greyscale 10-bit (Y10 )",
+ .guid = UVC_GUID_FORMAT_INVI,
+ .fcc = V4L2_PIX_FMT_Y10,
+ },
};
/* ------------------------------------------------------------------------
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 7e4d3eea371b..460b99ca99b7 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -131,6 +131,15 @@
#define UVC_GUID_FORMAT_RW10 \
{ 'R', 'W', '1', '0', 0x00, 0x00, 0x10, 0x00, \
0x80, 0x00, 0x00, 0xaa, 0x00, 0x38, 0x9b, 0x71}
+#define UVC_GUID_FORMAT_INVZ \
+ { 'I', 'N', 'V', 'Z', 0x90, 0x2d, 0x58, 0x4a, \
+ 0x92, 0x0b, 0x77, 0x3f, 0x1f, 0x2c, 0x55, 0x6b}
+#define UVC_GUID_FORMAT_INZI \
+ { 'I', 'N', 'Z', 'I', 0x66, 0x1a, 0x42, 0xa2, \
+ 0x90, 0x65, 0xd0, 0x18, 0x14, 0xa8, 0xef, 0x8a}
+#define UVC_GUID_FORMAT_INVI \
+ { 'I', 'N', 'V', 'I', 0xdb, 0x57, 0x49, 0x5e, \
+ 0x8e, 0x3f, 0xf4, 0x79, 0x53, 0x2b, 0x94, 0x6f}
/* ------------------------------------------------------------------------
* Driver specific constants.
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index d3f613e2c54a..4ab995bbec5b 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -659,6 +659,7 @@ struct v4l2_pix_format {
#define V4L2_PIX_FMT_Y12I v4l2_fourcc('Y', '1', '2', 'I') /* Greyscale 12-bit L/R interleaved */
#define V4L2_PIX_FMT_Z16 v4l2_fourcc('Z', '1', '6', ' ') /* Depth data 16-bit */
#define V4L2_PIX_FMT_MT21C v4l2_fourcc('M', 'T', '2', '1') /* Mediatek compressed block mode */
+#define V4L2_PIX_FMT_INZI v4l2_fourcc('I', 'N', 'Z', 'I') /* Intel Infrared 10-bit linked with Depth 16-bit */
/* SDR formats - used only for Software Defined Radio devices */
#define V4L2_SDR_FMT_CU8 v4l2_fourcc('C', 'U', '0', '8') /* IQ u8 */
--
2.7.4
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Document-Intel-SR300-Depth-camera-INZI-format.patch --]
[-- Type: text/x-patch; name="0002-Document-Intel-SR300-Depth-camera-INZI-format.patch", Size: 3556 bytes --]
From 581f4c3e60d8e7895bc34f9e0e90476eed31fa8d Mon Sep 17 00:00:00 2001
From: Evgeni Raikhel <evgeni.raikhel@intel.com>
Date: Wed, 16 Nov 2016 11:53:49 +0200
Subject: [PATCH 2/2] Document Intel SR300 Depth camera INZI format
Provide the frame structure and data layout of V4L2-PIX-FMT-INZI
format utilized by Intel SR300 Depth camera.
This is a complimentary patch for:
[PATCH] UVC: Add support for Intel SR300 depth camera
Signed-off-by: Evgeni Raikhel <evgeni.raikhel@intel.com>
---
Documentation/media/uapi/v4l/pixfmt-inzi.rst | 40 ++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
create mode 100644 Documentation/media/uapi/v4l/pixfmt-inzi.rst
diff --git a/Documentation/media/uapi/v4l/pixfmt-inzi.rst b/Documentation/media/uapi/v4l/pixfmt-inzi.rst
new file mode 100644
index 000000000000..cdfdeae4a664
--- /dev/null
+++ b/Documentation/media/uapi/v4l/pixfmt-inzi.rst
@@ -0,0 +1,40 @@
+.. -*- coding: utf-8; mode: rst -*-
+
+.. _V4L2-PIX-FMT-INZI:
+
+**************************
+V4L2_PIX_FMT_INZI ('INZI')
+**************************
+
+Infrared 10-bit linked with Depth 16-bit images
+
+
+Description
+===========
+
+Custom multi-planar format used by Intel SR300 Depth cameras, comprise of Infrared image followed by Depth data.
+The pixel definition is 32-bpp, with the Depth and Infrared Data split into separate continuous planes of identical dimensions.
+
+The first plane - Infrared data - is stored in V4L2_PIX_FMT_Y10 (see :ref:`pixfmt-y10`) greyscale format. Each pixel is 16-bit cell, with actual data present in the 10 LSBs with values in range 0 to 1023. The six remaining MSBs are padded with zeros.
+
+The second plane provides 16-bit per-pixel Depth data in V4L2_PIX_FMT_Z16 (:ref:`pixfmt-z16`) format.
+
+
+**Frame Structure.**
+Each cell is a 16-bit word with the significant data byte is stored at lower memory address (little-endian).
+
++-----------------+-----------------+-----------------+-----------------+-----------------+-----------------+
+| Ir\ :sub:`0` | Ir\ :sub:`1` | Ir\ :sub:`2` | ... | ... | ... |
++-----------------+-----------------+-----------------+-----------------+-----------------+-----------------+
+| ... ... ... |
+| Infrared Data |
+| ... ... ... |
++-----------------+-----------------+-----------------+-----------------+-----------------+-----------------+
+| Ir\ :sub:`n-3` | Ir\ :sub:`n-2` | Ir\ :sub:`n-1` | Depth\ :sub:`0` | Depth\ :sub:`1` | Depth\ :sub:`2` |
++-----------------+-----------------+-----------------+-----------------+-----------------+-----------------+
+| ... ... ... |
+| Depth Data |
+| ... ... ... |
++-----------------+-----------------+-----------------+-----------------+-----------------+-----------------+
+| ... | ... | ... |Depth\ :sub:`n-3`|Depth\ :sub:`n-2`|Depth\ :sub:`n-1`|
++-----------------+-----------------+-----------------+-----------------+-----------------+-----------------+
--
2.7.4
^ permalink raw reply related
* [PATCH v2] v4l: async: make v4l2 coexist with devicetree nodes in a dt overlay
From: Javi Merino @ 2016-12-05 10:09 UTC (permalink / raw)
To: Javier Martinez Canillas, Sakari Ailus
Cc: linux-media, linux-kernel, devicetree, Javi Merino,
Mauro Carvalho Chehab
In asds configured with V4L2_ASYNC_MATCH_OF, the v4l2 subdev can be
part of a devicetree overlay, for example:
&media_bridge {
...
my_port: port@0 {
#address-cells = <1>;
#size-cells = <0>;
reg = <0>;
ep: endpoint@0 {
remote-endpoint = <&camera0>;
};
};
};
/ {
fragment@0 {
target = <&i2c0>;
__overlay__ {
my_cam {
compatible = "foo,bar";
port {
camera0: endpoint {
remote-endpoint = <&my_port>;
...
};
};
};
};
};
};
Each time the overlay is applied, its of_node pointer will be
different. We are not interested in matching the pointer, what we
want to match is that the path is the one we are expecting. Change to
use of_node_cmp() so that we continue matching after the overlay has
been removed and reapplied.
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Javier Martinez Canillas <javier@osg.samsung.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Javi Merino <javi.merino@kernel.org>
---
drivers/media/v4l2-core/v4l2-async.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 5bada20..d33a17c 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -42,7 +42,8 @@ static bool match_devname(struct v4l2_subdev *sd,
static bool match_of(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
{
- return sd->of_node == asd->match.of.node;
+ return !of_node_cmp(of_node_full_name(sd->of_node),
+ of_node_full_name(asd->match.of.node));
}
static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd)
--
2.1.4
^ permalink raw reply related
* Re: [PATCH v3 00/10] Add support for DELTA video decoder of STMicroelectronics STiH4xx SoC series
From: Hans Verkuil @ 2016-12-05 10:18 UTC (permalink / raw)
To: Hugues Fruchet, linux-media; +Cc: Benjamin Gaignard, Jean-Christophe Trotin
In-Reply-To: <1479830007-29767-1-git-send-email-hugues.fruchet@st.com>
Hi Hugues,
On 11/22/2016 04:53 PM, Hugues Fruchet wrote:
> This patchset introduces a basic support for DELTA multi-format video
> decoder of STMicroelectronics STiH4xx SoC series.
>
> DELTA hardware IP is controlled by a remote firmware loaded in a ST231
> coprocessor. Communication with firmware is done within an IPC layer
> using rpmsg kernel framework and a shared memory for messages handling.
> This driver is compatible with firmware version 21.1-3.
> While a single firmware is loaded in ST231 coprocessor, it is composed
> of several firmwares, one per video format family.
>
> This DELTA V4L2 driver is designed around files:
> - delta-v4l2.c : handles V4L2 APIs using M2M framework and calls decoder ops
> - delta-<codec>* : implements <codec> decoder calling its associated
> video firmware (for ex. MJPEG) using IPC layer
> - delta-ipc.c : IPC layer which handles communication with firmware using rpmsg
>
> This first basic support implements only MJPEG hardware acceleration but
> the driver structure is in place to support all the features of the
> DELTA video decoder hardware IP.
>
> This driver depends on:
> - ST remoteproc/rpmsg: patchset posted at https://lkml.org/lkml/2016/9/6/77
> - ST DELTA firmware: its license is under review. When available,
> pull request will be done on linux-firmware.
>
> ===========
> = history =
> ===========
> version 3
> - update after v2 review:
> - fixed m2m_buf_done missing on start_streaming error case
> - fixed q->dev missing in queue_init()
> - removed unsupported s_selection
> - refactored string namings in delta-debug.c
> - fixed space before comment
> - all commits have commit messages
> - reword memory allocator helper commit
>
> version 2
> - update after v1 review:
> - simplified tracing
> - G_/S_SELECTION reworked to fit COMPOSE(CAPTURE)
> - fixed m2m_buf_done missing on start_streaming error case
> - fixed q->dev missing in queue_init()
> - switch to kernel-4.9 rpmsg API
> - DELTA support added in multi_v7_defconfig
> - minor typo fixes & code cleanup
>
> version 1:
> - Initial submission
>
> ===================
> = v4l2-compliance =
> ===================
> Below is the v4l2-compliance report for the version 3 of the DELTA video
> decoder driver. v4l2-compliance has been build from SHA1:
> 600492351ddf40cc524aab73802153674d7d287b (libdvb5: Fix multiple definition of dvb_dev_remote_init linking error)
Can you update v4l-utils and run this test again? The S_SELECTION compliance
test has been fixed to allow for ENOTTY as the error code.
If the output looks good, then I'll merge this driver (will be for 4.11).
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v3 07/10] [media] st-delta: rpmsg ipc support
From: Hans Verkuil @ 2016-12-05 10:47 UTC (permalink / raw)
To: Hugues Fruchet, linux-media; +Cc: Benjamin Gaignard, Jean-Christophe Trotin
In-Reply-To: <1479830007-29767-8-git-send-email-hugues.fruchet@st.com>
On 11/22/2016 04:53 PM, Hugues Fruchet wrote:
> IPC (Inter Process Communication) support for communication with
> DELTA coprocessor firmware using rpmsg kernel framework.
> Based on 4 services open/set_stream/decode/close and their associated
> rpmsg messages.
> The messages structures are duplicated on both host and firmware
> side and are packed (use only of 32 bits size fields in messages
> structures to ensure packing).
> Each service is synchronous; service returns only when firmware
> acknowledges the associated command message.
> Due to significant parameters size exchanged from host to copro,
> parameters are not inserted in rpmsg messages. Instead, parameters are
> stored in physical memory shared between host and coprocessor.
> Memory is non-cacheable, so no special operation is required
> to ensure memory coherency on host and on coprocessor side.
> Multi-instance support and re-entrance are ensured using host_hdl and
> copro_hdl in message header exchanged between both host and coprocessor.
> This avoids to manage tables on both sides to get back the running context
> of each instance.
>
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
> drivers/media/platform/Kconfig | 1 +
> drivers/media/platform/sti/delta/Makefile | 2 +-
> drivers/media/platform/sti/delta/delta-ipc.c | 590 ++++++++++++++++++++++++++
> drivers/media/platform/sti/delta/delta-ipc.h | 76 ++++
> drivers/media/platform/sti/delta/delta-v4l2.c | 11 +
> drivers/media/platform/sti/delta/delta.h | 21 +
> 6 files changed, 700 insertions(+), 1 deletion(-)
> create mode 100644 drivers/media/platform/sti/delta/delta-ipc.c
> create mode 100644 drivers/media/platform/sti/delta/delta-ipc.h
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index f494f01..5519442 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -303,6 +303,7 @@ config VIDEO_STI_DELTA
> depends on VIDEO_DEV && VIDEO_V4L2
> depends on ARCH_STI || COMPILE_TEST
> depends on HAS_DMA
> + depends on RPMSG
This should be 'select', not 'depends on'.
> select VIDEOBUF2_DMA_CONTIG
> select V4L2_MEM2MEM_DEV
> help
Can you make a v3.1 of this patch correcting this?
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v2 3/3] uvcvideo: add a metadata device node
From: Laurent Pinchart @ 2016-12-05 10:53 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: Linux Media Mailing List, Hans Verkuil
In-Reply-To: <Pine.LNX.4.64.1612021141110.11357@axis700.grange>
Hi Guennadi,
Thank you for the patch.
On Friday 02 Dec 2016 11:53:23 Guennadi Liakhovetski wrote:
> Some UVC video cameras contain metadata in their payload headers. This
> patch extracts that data, skipping the standard part of the header, on
> both bulk and isochronous endpoints and makes it available to the user
> space on a separate video node, using the V4L2_CAP_META_CAPTURE
> capability and the V4L2_BUF_TYPE_META_CAPTURE buffer queue type. Even
> though different cameras will have different metadata formats, we use
> the same V4L2_META_FMT_UVC pixel format for all of them. Users have to
> parse data, based on the specific camera model information.
>
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> ---
>
> v2:
> - updated to the current media/master
> - removed superfluous META capability from capture nodes
> - now the complete UVC payload header is copied to buffers, including
> standard fields
>
> Still open for discussion: is this really OK to always create an
> additional metadata node for each UVC camera or UVC video interface.
>
> IIUC, Laurent's metadata node patch
> https://patchwork.linuxtv.org/patch/36810/ has been acked by Hans and the
> only thing, that's preventing it from being merged it the lack of
> documentation. While waiting for documentation, I'd appreciate some
> discussion of this patch to beat it into shape soon enough and have it
> ready for merge soon after Laurent's patches are pulled in.
>
> Thanks
> Guennadi
>
> drivers/media/usb/uvc/Makefile | 2 +-
> drivers/media/usb/uvc/uvc_driver.c | 10 ++
> drivers/media/usb/uvc/uvc_isight.c | 2 +-
> drivers/media/usb/uvc/uvc_metadata.c | 228 +++++++++++++++++++++++++++++++
> drivers/media/usb/uvc/uvc_video.c | 47 ++++++--
> drivers/media/usb/uvc/uvcvideo.h | 12 +-
> drivers/media/v4l2-core/v4l2-ioctl.c | 1 +
> include/uapi/linux/uvcvideo.h | 10 ++
> include/uapi/linux/videodev2.h | 3 +
> 9 files changed, 304 insertions(+), 11 deletions(-)
> create mode 100644 drivers/media/usb/uvc/uvc_metadata.c
[snip]
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 04bf350..edb67ac 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1866,6 +1866,9 @@ static void uvc_unregister_video(struct uvc_device
> *dev)
>
> video_unregister_device(&stream->vdev);
>
> + if (video_is_registered(&stream->meta.vdev))
> + video_unregister_device(&stream->meta.vdev);
video_unregister_device() contains a video_is_registered(), no need to
duplicate it.
> +
> uvc_debugfs_cleanup_stream(stream);
> }
>
> @@ -1926,6 +1929,13 @@ static int uvc_register_video(struct uvc_device *dev,
> return ret;
> }
>
> + /*
> + * Register a metadata node. TODO: shall this only be enabled for some
> + * cameras?
> + */
> + if (!(dev->quirks & UVC_QUIRK_BUILTIN_ISIGHT))
> + uvc_meta_register(stream);
> +
I think so, only for the cameras that can produce metadata.
> if (stream->type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> stream->chain->caps |= V4L2_CAP_VIDEO_CAPTURE;
> else
[snip]
> diff --git a/drivers/media/usb/uvc/uvc_metadata.c
> b/drivers/media/usb/uvc/uvc_metadata.c new file mode 100644
> index 0000000..ddf77d9
> --- /dev/null
> +++ b/drivers/media/usb/uvc/uvc_metadata.c
> @@ -0,0 +1,228 @@
> +/*
> + * uvc_metadata.c -- USB Video Class driver - Metadata handling
> + *
> + * Copyright (C) 2016
> + * Guennadi Liakhovetski (guennadi.liakhovetski@intel.com)
> + *
> + * This program is free software; you can redistribute it and/or
> modify
> + * it under the terms of the GNU General Public License as published
> by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +#include <linux/usb.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-v4l2.h>
> +#include <media/videobuf2-vmalloc.h>
> +
> +#include "uvcvideo.h"
> +
> +static inline struct uvc_buffer *to_uvc_buffer(struct vb2_v4l2_buffer
> *vbuf)
> +{
> + return container_of(vbuf, struct uvc_buffer, buf);
> +}
You can move this function to uvcvideo.h and use it in uvc_queue.c (a separate
patch would be best).
> +/* ------------------------------------------------------------------------
> + * videobuf2 Queue Operations
> + */
> +
> +static int meta_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
> + unsigned int *nplanes, unsigned int sizes[],
> + struct device *alloc_devs[])
> +{
> + if (*nplanes) {
> + if (*nplanes != 1)
> + return -EINVAL;
> +
> + if (sizes[0] < UVC_PAYLOAD_HEADER_MAX_SIZE)
> + return -EINVAL;
> +
> + return 0;
> + }
> +
> + *nplanes = 1;
> + sizes[0] = UVC_PAYLOAD_HEADER_MAX_SIZE;
> +
> + return 0;
> +}
> +
> +static int meta_buffer_prepare(struct vb2_buffer *vb)
> +{
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + struct uvc_buffer *buf = to_uvc_buffer(vbuf);
> +
> + if (vb->num_planes != 1)
> + return -EINVAL;
> +
> + if (vb2_plane_size(vb, 0) < UVC_PAYLOAD_HEADER_MAX_SIZE)
> + return -EINVAL;
> +
> + buf->state = UVC_BUF_STATE_QUEUED;
> + buf->error = 0;
> + buf->mem = vb2_plane_vaddr(vb, 0);
> + buf->length = vb2_plane_size(vb, 0);
> + buf->bytesused = 0;
> +
> + return 0;
> +}
> +
> +static void meta_buffer_queue(struct vb2_buffer *vb)
> +{
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + struct uvc_video_queue *queue = vb2_get_drv_priv(vb->vb2_queue);
> + struct uvc_buffer *buf = to_uvc_buffer(vbuf);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&queue->irqlock, flags);
Shouldn't you handle the UVC_QUEUE_DISCONNECTED flag here ?
> + list_add_tail(&buf->queue, &queue->irqqueue);
> + spin_unlock_irqrestore(&queue->irqlock, flags);
> +}
> +
> +static int meta_start_streaming(struct vb2_queue *vq, unsigned int count)
> +{
> + return 0;
> +}
> +
> +static void meta_stop_streaming(struct vb2_queue *vq)
> +{
> + struct uvc_video_queue *queue = vb2_get_drv_priv(vq);
> + struct uvc_buffer *buffer;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&queue->irqlock, flags);
> +
> + /* Remove all buffers from the IRQ queue. */
> + list_for_each_entry(buffer, &queue->irqqueue, queue)
> + vb2_buffer_done(&buffer->buf.vb2_buf, VB2_BUF_STATE_ERROR);
> + INIT_LIST_HEAD(&queue->irqqueue);
> +
> + spin_unlock_irqrestore(&queue->irqlock, flags);
> +}
> +
> +static struct vb2_ops uvc_meta_queue_ops = {
> + .queue_setup = meta_queue_setup,
> + .buf_prepare = meta_buffer_prepare,
> + .buf_queue = meta_buffer_queue,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> + .start_streaming = meta_start_streaming,
> + .stop_streaming = meta_stop_streaming,
> +};
How about reusing the uvc_queue.c implementation, with a few new checks for
metadata buffers where needed, instead of duplicating code ? At first sight
the changes don't seem to be too intrusive (but I might have overlooked
something).
> +/* ------------------------------------------------------------------------
> + * V4L2 ioctls
> + */
> +
> +static int meta_v4l2_querycap(struct file *file, void *fh,
> + struct v4l2_capability *cap)
> +{
> + struct v4l2_fh *vfh = file->private_data;
> + struct uvc_streaming *stream = video_get_drvdata(vfh->vdev);
> +
> + cap->device_caps = V4L2_CAP_META_CAPTURE
> + | V4L2_CAP_STREAMING;
> + cap->capabilities = V4L2_CAP_DEVICE_CAPS | cap->device_caps
> + | stream->chain->caps;
If you set the device_caps field of struct video_device you won't need the
above four lines.
> + strlcpy(cap->driver, "uvcvideo", sizeof(cap->driver));
> + strlcpy(cap->card, vfh->vdev->name, sizeof(cap->card));
> + usb_make_path(stream->dev->udev, cap->bus_info, sizeof(cap-
>bus_info));
> +
> + return 0;
> +}
> +
> +static int meta_v4l2_get_format(struct file *file, void *fh,
> + struct v4l2_format *format)
> +{
> + struct v4l2_fh *vfh = file->private_data;
> + struct v4l2_meta_format *fmt = &format->fmt.meta;
> +
> + if (format->type != vfh->vdev->queue->type)
> + return -EINVAL;
> +
> + memset(fmt, 0, sizeof(*fmt));
> +
> + fmt->dataformat = V4L2_META_FMT_UVC;
> + fmt->buffersize = UVC_PAYLOAD_HEADER_MAX_SIZE;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops uvc_meta_ioctl_ops = {
> + .vidioc_querycap = meta_v4l2_querycap,
> + .vidioc_g_fmt_meta_cap = meta_v4l2_get_format,
> + .vidioc_s_fmt_meta_cap = meta_v4l2_get_format,
> + .vidioc_try_fmt_meta_cap = meta_v4l2_get_format,
> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
> + .vidioc_querybuf = vb2_ioctl_querybuf,
> + .vidioc_qbuf = vb2_ioctl_qbuf,
> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> + .vidioc_streamon = vb2_ioctl_streamon,
> + .vidioc_streamoff = vb2_ioctl_streamoff,
> +};
> +
> +/* ------------------------------------------------------------------------
> + * V4L2 File Operations
> + */
> +
> +static struct v4l2_file_operations uvc_meta_fops = {
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = video_ioctl2,
> + .open = v4l2_fh_open,
> + .release = vb2_fop_release,
> + .poll = vb2_fop_poll,
> + .mmap = vb2_fop_mmap,
> +};
> +
> +int uvc_meta_register(struct uvc_streaming *stream)
> +{
> + struct uvc_device *dev = stream->dev;
> + struct uvc_meta_dev *meta = &stream->meta;
> + struct video_device *vdev = &meta->vdev;
> + struct uvc_video_queue *quvc = &meta->queue;
> + struct vb2_queue *queue = &quvc->queue;
> + int ret;
> +
> + vdev->v4l2_dev = &dev->vdev;
> + vdev->fops = &uvc_meta_fops;
> + vdev->ioctl_ops = &uvc_meta_ioctl_ops;
> + vdev->release = video_device_release_empty;
> + vdev->prio = &stream->chain->prio;
> + vdev->vfl_dir = VFL_DIR_RX;
> + strlcpy(vdev->name, dev->name, sizeof(vdev->name));
> +
> + video_set_drvdata(vdev, stream);
> +
> + /* Initialize the video buffer queue. */
> + queue->type = V4L2_BUF_TYPE_META_CAPTURE;
> + queue->io_modes = VB2_MMAP | VB2_USERPTR;
> + queue->drv_priv = quvc;
> + queue->buf_struct_size = sizeof(struct uvc_buffer);
> + queue->ops = &uvc_meta_queue_ops;
> + queue->mem_ops = &vb2_vmalloc_memops;
> + queue->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC
> + | V4L2_BUF_FLAG_TSTAMP_SRC_SOE;
> + queue->lock = &quvc->mutex;
> + ret = vb2_queue_init(queue);
> + if (ret < 0)
> + return ret;
> +
> + mutex_init(&quvc->mutex);
> + spin_lock_init(&quvc->irqlock);
> + INIT_LIST_HEAD(&quvc->irqqueue);
> +
> + vdev->queue = queue;
You can set vdev->queue above with the rest of the vdev initialization.
> + ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> + if (ret < 0)
> + uvc_printk(KERN_ERR, "Failed to register metadata device (%d).
\n", ret);
> +
> + return ret;
> +}
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index b5589d5..1bda8e1 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1158,8 +1158,30 @@ static void uvc_video_validate_buffer(const struct
> uvc_streaming *stream, /*
> * Completion handler for video URBs.
> */
> +static void uvc_video_decode_meta(struct uvc_streaming *stream,
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf,
> + u8 *mem, int length)
I don't think length can be negative, you can make it an unsigned int and
replace min_t with min below.
> +{
> + size_t nbytes;
> +
> + if (!meta_buf || !length)
> + return;
> +
> + nbytes = min_t(unsigned int, length, meta_buf->length);
> +
> + meta_buf->buf.sequence = buf->buf.sequence;
> + meta_buf->buf.field = buf->buf.field;
> + meta_buf->buf.vb2_buf.timestamp = buf->buf.vb2_buf.timestamp;
> +
> + memcpy(meta_buf->mem, mem, nbytes);
Do you need the whole header ? Shouldn't you strip the part already handled by
the driver ?
> + meta_buf->bytesused = nbytes;
> + meta_buf->state = UVC_BUF_STATE_READY;
> +
> + uvc_queue_next_buffer(&stream->meta.queue, meta_buf);
This essentially means that you'll need one buffer per isochronous packet.
Given the number of isochronous packets that make a complete image the cost
seem prohibitive to me. You should instead gather metadata from all headers
into a single buffer.
> +}
> +
> static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
> *stream,
> - struct uvc_buffer *buf)
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
> {
> u8 *mem;
> int ret, i;
> @@ -1189,6 +1211,8 @@ static void uvc_video_decode_isoc(struct urb *urb,
> struct uvc_streaming *stream, if (ret < 0)
> continue;
>
> + uvc_video_decode_meta(stream, buf, meta_buf, mem, ret);
> +
> /* Decode the payload data. */
> uvc_video_decode_data(stream, buf, mem + ret,
> urb->iso_frame_desc[i].actual_length - ret);
> @@ -1205,7 +1229,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
> struct uvc_streaming *stream, }
>
> static void uvc_video_decode_bulk(struct urb *urb, struct uvc_streaming
> *stream,
> - struct uvc_buffer *buf)
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
> {
> u8 *mem;
> int len, ret;
> @@ -1239,6 +1263,8 @@ static void uvc_video_decode_bulk(struct urb *urb,
> struct uvc_streaming *stream, memcpy(stream->bulk.header, mem, ret);
> stream->bulk.header_size = ret;
>
> + uvc_video_decode_meta(stream, buf, meta_buf, mem,
ret);
> +
> mem += ret;
> len -= ret;
> }
> @@ -1262,8 +1288,7 @@ static void uvc_video_decode_bulk(struct urb *urb,
> struct uvc_streaming *stream, uvc_video_decode_end(stream, buf,
> stream->bulk.header,
> stream->bulk.payload_size);
> if (buf->state == UVC_BUF_STATE_READY)
> - buf = uvc_queue_next_buffer(&stream->queue,
> - buf);
> + uvc_queue_next_buffer(&stream->queue, buf);
This could be split to a separate patch.
> }
>
> stream->bulk.header_size = 0;
> @@ -1273,7 +1298,7 @@ static void uvc_video_decode_bulk(struct urb *urb,
> struct uvc_streaming *stream, }
>
> static void uvc_video_encode_bulk(struct urb *urb, struct uvc_streaming
> *stream,
> - struct uvc_buffer *buf)
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf)
> {
> u8 *mem = urb->transfer_buffer;
> int len = stream->urb_size, ret;
> @@ -1319,7 +1344,8 @@ static void uvc_video_complete(struct urb *urb)
> {
> struct uvc_streaming *stream = urb->context;
> struct uvc_video_queue *queue = &stream->queue;
> - struct uvc_buffer *buf = NULL;
> + struct uvc_video_queue *qmeta = &stream->meta.queue;
> + struct uvc_buffer *buf = NULL, *buf_meta = NULL;
One variable per line please.
> unsigned long flags;
> int ret;
>
> @@ -1338,6 +1364,7 @@ static void uvc_video_complete(struct urb *urb)
> case -ECONNRESET: /* usb_unlink_urb() called. */
> case -ESHUTDOWN: /* The endpoint is being disabled. */
> uvc_queue_cancel(queue, urb->status == -ESHUTDOWN);
> + uvc_queue_cancel(qmeta, urb->status == -ESHUTDOWN);
> return;
> }
>
> @@ -1347,7 +1374,13 @@ static void uvc_video_complete(struct urb *urb)
> queue);
> spin_unlock_irqrestore(&queue->irqlock, flags);
>
> - stream->decode(urb, stream, buf);
> + spin_lock_irqsave(&qmeta->irqlock, flags);
> + if (!list_empty(&qmeta->irqqueue))
> + buf_meta = list_first_entry(&qmeta->irqqueue, struct
uvc_buffer,
> + queue);
> + spin_unlock_irqrestore(&qmeta->irqlock, flags);
> +
> + stream->decode(urb, stream, buf, buf_meta);
>
> if ((ret = usb_submit_urb(urb, GFP_ATOMIC)) < 0) {
> uvc_printk(KERN_ERR, "Failed to resubmit video URB (%d).\n",
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index 3d6cc62..ebff4b6 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -464,6 +464,11 @@ struct uvc_stats_stream {
> unsigned int max_sof; /* Maximum STC.SOF value */
> };
>
> +struct uvc_meta_dev {
All other structures use _device, not _dev. Let's be consistent.
> + struct video_device vdev;
> + struct uvc_video_queue queue;
> +};
> +
> struct uvc_streaming {
> struct list_head list;
> struct uvc_device *dev;
> @@ -495,7 +500,9 @@ struct uvc_streaming {
> unsigned int frozen : 1;
> struct uvc_video_queue queue;
> void (*decode) (struct urb *urb, struct uvc_streaming *video,
> - struct uvc_buffer *buf);
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf);
> +
> + struct uvc_meta_dev meta;
>
> /* Context data used by the bulk completion handler. */
> struct {
> @@ -700,6 +707,7 @@ extern int uvc_query_ctrl(struct uvc_device *dev, __u8
> query, __u8 unit, void uvc_video_clock_update(struct uvc_streaming *stream,
> struct vb2_v4l2_buffer *vbuf,
> struct uvc_buffer *buf);
> +int uvc_meta_register(struct uvc_streaming *stream);
>
> /* Status */
> extern int uvc_status_init(struct uvc_device *dev);
> @@ -754,7 +762,7 @@ extern struct usb_host_endpoint *uvc_find_endpoint(
>
> /* Quirks support */
> void uvc_video_decode_isight(struct urb *urb, struct uvc_streaming *stream,
> - struct uvc_buffer *buf);
> + struct uvc_buffer *buf, struct uvc_buffer *meta_buf);
>
> /* debugfs and statistics */
> int uvc_debugfs_init(void);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c index 44a29af..1618be4 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1232,6 +1232,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> case V4L2_TCH_FMT_DELTA_TD08: descr = "8-bit signed deltas"; break; case
> V4L2_TCH_FMT_TU16: descr = "16-bit unsigned touch data"; break;
case
> V4L2_TCH_FMT_TU08: descr = "8-bit unsigned touch data"; break;
> + case
> V4L2_META_FMT_UVC: descr = "UVC payload header metadata"; break;
>
> default:
> /* Compressed formats */
> diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> index 3b08186..e98de14 100644
> --- a/include/uapi/linux/uvcvideo.h
> +++ b/include/uapi/linux/uvcvideo.h
> @@ -67,4 +67,14 @@ struct uvc_xu_control_query {
> #define UVCIOC_CTRL_MAP _IOWR('u', 0x20, struct
uvc_xu_control_mapping)
> #define UVCIOC_CTRL_QUERY _IOWR('u', 0x21, struct uvc_xu_control_query)
>
> +/*
> + * Metadata node
> + */
> +
> +/*
> + * Actually 255 bytes, but 256 is just a nicer number. We keep the buffer
> size
> + * constant and just set .usedbytes accordingly
> + */
> +#define UVC_PAYLOAD_HEADER_MAX_SIZE 256
Why is lying about the size better ?
> #endif
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] UVC Module - Support Intel RealSense SR300 Depth Camera formats
From: Laurent Pinchart @ 2016-12-05 11:01 UTC (permalink / raw)
To: Raikhel, Evgeni; +Cc: linux-media@vger.kernel.org
In-Reply-To: <AA09C8071EEEFC44A7852ADCECA86673A1E6E7@hasmsx108.ger.corp.intel.com>
Hi Evgeni,
Thank you for the patch.
On Monday 05 Dec 2016 10:06:55 Raikhel, Evgeni wrote:
> Specify GUID and FourCC codes mapping for Depth-related pixel formats
> advertised by Intel RealSense(tm) SR300 depth camera. Provide documentation
> for the new INZI pixel format introduced.
Could you please resend the patches inline instead of as attachments ? See
Documentation/SubmittingPatches for more information.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v4 1/9] media: v4l2-mem2mem: extend m2m APIs for more accurate buffer management
From: Hans Verkuil @ 2016-12-05 11:25 UTC (permalink / raw)
To: Stanimir Varbanov, Mauro Carvalho Chehab
Cc: Andy Gross, Bjorn Andersson, Stephen Boyd, Srinivas Kandagatla,
linux-media, linux-kernel, linux-arm-msm
In-Reply-To: <1480583001-32236-2-git-send-email-stanimir.varbanov@linaro.org>
On 12/01/2016 10:03 AM, Stanimir Varbanov wrote:
> this add functions for:
> - remove buffers from src/dst queue by index
> - remove exact buffer from src/dst queue
>
> also extends m2m API to iterate over a list of src/dst buffers
> in safely and non-safely manner.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
> drivers/media/v4l2-core/v4l2-mem2mem.c | 37 +++++++++++++++
> include/media/v4l2-mem2mem.h | 83 ++++++++++++++++++++++++++++++++++
> 2 files changed, 120 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 6bc27e7b2a33..d689e7bb964f 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -126,6 +126,43 @@ void *v4l2_m2m_buf_remove(struct v4l2_m2m_queue_ctx *q_ctx)
> }
> EXPORT_SYMBOL_GPL(v4l2_m2m_buf_remove);
>
> +void v4l2_m2m_buf_remove_exact(struct v4l2_m2m_queue_ctx *q_ctx,
> + struct vb2_v4l2_buffer *vbuf)
I'd call this v4l2_m2m_buf_remove_by_buf to be consistent with _by_idx.
Other than that, this looks OK.
Regards,
Hans
> +{
> + struct v4l2_m2m_buffer *b;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&q_ctx->rdy_spinlock, flags);
> + b = container_of(vbuf, struct v4l2_m2m_buffer, vb);
> + list_del(&b->list);
> + q_ctx->num_rdy--;
> + spin_unlock_irqrestore(&q_ctx->rdy_spinlock, flags);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_buf_remove_exact);
> +
> +struct vb2_v4l2_buffer *
> +v4l2_m2m_buf_remove_by_idx(struct v4l2_m2m_queue_ctx *q_ctx, unsigned int idx)
> +
> +{
> + struct v4l2_m2m_buffer *b, *tmp;
> + struct vb2_v4l2_buffer *ret = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&q_ctx->rdy_spinlock, flags);
> + list_for_each_entry_safe(b, tmp, &q_ctx->rdy_queue, list) {
> + if (b->vb.vb2_buf.index == idx) {
> + list_del(&b->list);
> + q_ctx->num_rdy--;
> + ret = &b->vb;
> + break;
> + }
> + }
> + spin_unlock_irqrestore(&q_ctx->rdy_spinlock, flags);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_buf_remove_by_idx);
> +
> /*
> * Scheduling handlers
> */
> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> index 3ccd01bd245e..c8632c52d5e2 100644
> --- a/include/media/v4l2-mem2mem.h
> +++ b/include/media/v4l2-mem2mem.h
> @@ -437,6 +437,41 @@ static inline void *v4l2_m2m_next_dst_buf(struct v4l2_m2m_ctx *m2m_ctx)
> }
>
> /**
> + * v4l2_m2m_for_each_dst_buf() - iterate over a list of destination ready
> + * buffers
> + *
> + * @m2m_ctx: m2m context assigned to the instance given by struct &v4l2_m2m_ctx
> + */
> +#define v4l2_m2m_for_each_dst_buf(m2m_ctx, b) \
> + list_for_each_entry(b, &m2m_ctx->cap_q_ctx.rdy_queue, list)
> +
> +/**
> + * v4l2_m2m_for_each_src_buf() - iterate over a list of source ready buffers
> + *
> + * @m2m_ctx: m2m context assigned to the instance given by struct &v4l2_m2m_ctx
> + */
> +#define v4l2_m2m_for_each_src_buf(m2m_ctx, b) \
> + list_for_each_entry(b, &m2m_ctx->out_q_ctx.rdy_queue, list)
> +
> +/**
> + * v4l2_m2m_for_each_dst_buf_safe() - iterate over a list of destination ready
> + * buffers safely
> + *
> + * @m2m_ctx: m2m context assigned to the instance given by struct &v4l2_m2m_ctx
> + */
> +#define v4l2_m2m_for_each_dst_buf_safe(m2m_ctx, b, n) \
> + list_for_each_entry_safe(b, n, &m2m_ctx->cap_q_ctx.rdy_queue, list)
> +
> +/**
> + * v4l2_m2m_for_each_src_buf_safe() - iterate over a list of source ready
> + * buffers safely
> + *
> + * @m2m_ctx: m2m context assigned to the instance given by struct &v4l2_m2m_ctx
> + */
> +#define v4l2_m2m_for_each_src_buf_safe(m2m_ctx, b, n) \
> + list_for_each_entry_safe(b, n, &m2m_ctx->out_q_ctx.rdy_queue, list)
> +
> +/**
> * v4l2_m2m_get_src_vq() - return vb2_queue for source buffers
> *
> * @m2m_ctx: m2m context assigned to the instance given by struct &v4l2_m2m_ctx
> @@ -488,6 +523,54 @@ static inline void *v4l2_m2m_dst_buf_remove(struct v4l2_m2m_ctx *m2m_ctx)
> return v4l2_m2m_buf_remove(&m2m_ctx->cap_q_ctx);
> }
>
> +/**
> + * v4l2_m2m_buf_remove_exact() - take off exact buffer from the list of ready
> + * buffers
> + *
> + * @q_ctx: pointer to struct @v4l2_m2m_queue_ctx
> + */
> +void v4l2_m2m_buf_remove_exact(struct v4l2_m2m_queue_ctx *q_ctx,
> + struct vb2_v4l2_buffer *vbuf);
> +
> +/**
> + * v4l2_m2m_src_buf_remove_exact() - take off exact source buffer from the list
> + * of ready buffers
> + *
> + * @m2m_ctx: m2m context assigned to the instance given by struct &v4l2_m2m_ctx
> + */
> +static inline void v4l2_m2m_src_buf_remove_exact(struct v4l2_m2m_ctx *m2m_ctx,
> + struct vb2_v4l2_buffer *vbuf)
> +{
> + v4l2_m2m_buf_remove_exact(&m2m_ctx->out_q_ctx, vbuf);
> +}
> +
> +/**
> + * v4l2_m2m_src_buf_remove_exact() - take off exact destination buffer from the
> + * list of ready buffers
> + *
> + * @m2m_ctx: m2m context assigned to the instance given by struct &v4l2_m2m_ctx
> + */
> +static inline void v4l2_m2m_dst_buf_remove_exact(struct v4l2_m2m_ctx *m2m_ctx,
> + struct vb2_v4l2_buffer *vbuf)
> +{
> + v4l2_m2m_buf_remove_exact(&m2m_ctx->cap_q_ctx, vbuf);
> +}
> +
> +struct vb2_v4l2_buffer *
> +v4l2_m2m_buf_remove_by_idx(struct v4l2_m2m_queue_ctx *q_ctx, unsigned int idx);
> +
> +static inline struct vb2_v4l2_buffer *
> +v4l2_m2m_src_buf_remove_by_idx(struct v4l2_m2m_ctx *m2m_ctx, unsigned int idx)
> +{
> + return v4l2_m2m_buf_remove_by_idx(&m2m_ctx->out_q_ctx, idx);
> +}
> +
> +static inline struct vb2_v4l2_buffer *
> +v4l2_m2m_dst_buf_remove_by_idx(struct v4l2_m2m_ctx *m2m_ctx, unsigned int idx)
> +{
> + return v4l2_m2m_buf_remove_by_idx(&m2m_ctx->cap_q_ctx, idx);
> +}
> +
> /* v4l2 ioctl helpers */
>
> int v4l2_m2m_ioctl_reqbufs(struct file *file, void *priv,
>
^ permalink raw reply
* Re: [PATCH v4 5/9] media: venus: vdec: add video decoder files
From: Hans Verkuil @ 2016-12-05 11:32 UTC (permalink / raw)
To: Stanimir Varbanov, Mauro Carvalho Chehab
Cc: Andy Gross, Bjorn Andersson, Stephen Boyd, Srinivas Kandagatla,
linux-media, linux-kernel, linux-arm-msm
In-Reply-To: <1480583001-32236-6-git-send-email-stanimir.varbanov@linaro.org>
I have two comments (and the same two comments apply to the video encoder patch
as well):
On 12/01/2016 10:03 AM, Stanimir Varbanov wrote:
> This consists of video decoder implementation plus decoder
> controls.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
> drivers/media/platform/qcom/venus/vdec.c | 976 +++++++++++++++++++++++++
> drivers/media/platform/qcom/venus/vdec.h | 32 +
> drivers/media/platform/qcom/venus/vdec_ctrls.c | 149 ++++
> 3 files changed, 1157 insertions(+)
> create mode 100644 drivers/media/platform/qcom/venus/vdec.c
> create mode 100644 drivers/media/platform/qcom/venus/vdec.h
> create mode 100644 drivers/media/platform/qcom/venus/vdec_ctrls.c
>
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> new file mode 100644
> index 000000000000..9f585a1e0ff1
> --- /dev/null
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -0,0 +1,976 @@
<snip>
> +static int
> +vdec_s_selection(struct file *file, void *fh, struct v4l2_selection *s)
> +{
> + struct venus_inst *inst = to_inst(file);
> +
> + if (s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE ||
> + s->target != V4L2_SEL_TGT_COMPOSE)
> + return -EINVAL;
> +
> + switch (s->target) {
> + case V4L2_SEL_TGT_COMPOSE:
> + s->r.width = inst->out_width;
> + s->r.height = inst->out_height;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + s->r.top = 0;
> + s->r.left = 0;
> +
> + return 0;
> +}
This doesn't actually set anything, so what's the point of this function?
I've fixed the corresponding test in v4l2-compliance so you can now drop this
op and v4l2-compliance won't complain anymore.
> +static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> + struct venus_inst *inst = vb2_get_drv_priv(q);
> + struct venus_core *core = inst->core;
> + struct device *dev = core->dev;
> + u32 ptype;
> + int ret;
> +
> + mutex_lock(&inst->lock);
> +
> + if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> + inst->streamon_out = 1;
> + else
> + inst->streamon_cap = 1;
> +
> + if (!(inst->streamon_out & inst->streamon_cap)) {
> + mutex_unlock(&inst->lock);
> + return 0;
> + }
> +
> + inst->reconfig = false;
> + inst->sequence = 0;
> + inst->codec_cfg = false;
> +
> + ret = pm_runtime_get_sync(dev);
> + if (ret < 0)
> + return ret;
This should be a goto so that 'helper_buffers_done(inst, VB2_BUF_STATE_QUEUED);'
is called on error.
It's wrong anyway since you don't unlock the mutex in this return path either.
> +
> + ret = vdec_init_session(inst);
> + if (ret)
> + goto put_sync;
> +
> + ret = vdec_set_properties(inst);
> + if (ret)
> + goto deinit_sess;
> +
> + if (core->res->hfi_version == HFI_VERSION_3XX) {
> + struct hfi_buffer_size_actual buf_sz;
> +
> + ptype = HFI_PROPERTY_PARAM_BUFFER_SIZE_ACTUAL;
> + buf_sz.type = HFI_BUFFER_OUTPUT;
> + buf_sz.size = inst->output_buf_size;
> +
> + ret = hfi_session_set_property(inst, ptype, &buf_sz);
> + if (ret)
> + goto deinit_sess;
> + }
> +
> + ret = vdec_verify_conf(inst);
> + if (ret)
> + goto deinit_sess;
> +
> + ret = helper_set_num_bufs(inst, inst->num_input_bufs,
> + inst->num_output_bufs);
> + if (ret)
> + goto deinit_sess;
> +
> + ret = helper_vb2_start_streaming(inst);
> + if (ret)
> + goto deinit_sess;
> +
> + mutex_unlock(&inst->lock);
> +
> + return 0;
> +
> +deinit_sess:
> + hfi_session_deinit(inst);
> +put_sync:
> + pm_runtime_put_sync(dev);
> + helper_buffers_done(inst, VB2_BUF_STATE_QUEUED);
> + if (q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> + inst->streamon_out = 0;
> + else
> + inst->streamon_cap = 0;
> + mutex_unlock(&inst->lock);
> + return ret;
> +}
Regards,
Hans
^ permalink raw reply
* Re: [PATCH] [media] usbtv: add a new usbid
From: Lubomir Rintel @ 2016-12-05 11:45 UTC (permalink / raw)
To: Icenowy Zheng, Mauro Carvalho Chehab, Federico Simoncelli,
Hans Verkuil
Cc: linux-media, linux-kernel
In-Reply-To: <20161204135943.34465-1-icenowy@aosc.xyz>
On Sun, 2016-12-04 at 21:59 +0800, Icenowy Zheng wrote:
> A new usbid of UTV007 is found in a newly bought device.
>
> The usbid is 1f71:3301.
>
> The ID on the chip is:
> UTV007
> A89029.1
> 1520L18K1
>
> Both video and audio is tested with the modified usbtv driver.
Thank you.
Acked-by: Lubomir Rintel <lkundrak@v3.sk>
Also, it may make sense to add
Tested-by: Icenowy Zheng <icenowy@aosc.xyz>
>
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> ---
> drivers/media/usb/usbtv/usbtv-core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/usb/usbtv/usbtv-core.c
> b/drivers/media/usb/usbtv/usbtv-core.c
> index dc76fd4..0324633 100644
> --- a/drivers/media/usb/usbtv/usbtv-core.c
> +++ b/drivers/media/usb/usbtv/usbtv-core.c
> @@ -141,6 +141,7 @@ static void usbtv_disconnect(struct usb_interface
> *intf)
>
> static struct usb_device_id usbtv_id_table[] = {
> { USB_DEVICE(0x1b71, 0x3002) },
> + { USB_DEVICE(0x1f71, 0x3301) },
> {}
> };
> MODULE_DEVICE_TABLE(usb, usbtv_id_table);
^ permalink raw reply
* Re: [PATCH] [media] usbtv: add a new usbid
From: Lubomir Rintel @ 2016-12-05 11:49 UTC (permalink / raw)
To: Icenowy Zheng, Mauro Carvalho Chehab, Federico Simoncelli,
Hans Verkuil
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <14085351480863591@web18g.yandex.ru>
On Sun, 2016-12-04 at 22:59 +0800, Icenowy Zheng wrote:
>
> 04.12.2016, 22:00, "Icenowy Zheng" <icenowy@aosc.xyz>:
> > A new usbid of UTV007 is found in a newly bought device.
> >
> > The usbid is 1f71:3301.
> >
> > The ID on the chip is:
> > UTV007
> > A89029.1
> > 1520L18K1
> >
>
> Seems that my device come with more capabilities.
>
> I tested it under Windows, and I got wireless Analog TV
> and FM radio functions. (An antenna is shipped with my device)
>
> Maybe a new radio function is be added, combined with the
> new USB ID.
>
> But at least Composite AV function works well with current usbtv
> driver and XawTV.
Well, someone with the hardware would need to capture the traffic from
the Windows driver (and ideally also extend the driver). Would you mind
giving it a try?
Do you have a link to some further details about the device you got?
Perhaps if it's available cheaply from dealextreme or somewhere I could
take a look too.
Lubo
^ permalink raw reply
* Re: [PATCH v4 8/9] media: venus: hfi: add Venus HFI files
From: Hans Verkuil @ 2016-12-05 12:05 UTC (permalink / raw)
To: Stanimir Varbanov, Mauro Carvalho Chehab
Cc: Andy Gross, Bjorn Andersson, Stephen Boyd, Srinivas Kandagatla,
linux-media, linux-kernel, linux-arm-msm
In-Reply-To: <1480583001-32236-9-git-send-email-stanimir.varbanov@linaro.org>
On 12/01/2016 10:03 AM, Stanimir Varbanov wrote:
> Here is the implementation of Venus video accelerator low-level
> functionality. It contanins code which setup the registers and
> startup uthe processor, allocate and manipulates with the shared
> memory used for sending commands and receiving messages.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
> drivers/media/platform/qcom/venus/hfi_venus.c | 1508 ++++++++++++++++++++++
> drivers/media/platform/qcom/venus/hfi_venus.h | 23 +
> drivers/media/platform/qcom/venus/hfi_venus_io.h | 98 ++
> 3 files changed, 1629 insertions(+)
> create mode 100644 drivers/media/platform/qcom/venus/hfi_venus.c
> create mode 100644 drivers/media/platform/qcom/venus/hfi_venus.h
> create mode 100644 drivers/media/platform/qcom/venus/hfi_venus_io.h
>
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> new file mode 100644
> index 000000000000..f004a9a80d85
> --- /dev/null
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -0,0 +1,1508 @@
> +static int venus_tzbsp_set_video_state(enum tzbsp_video_state state)
> +{
> + return qcom_scm_video_set_state(state, 0);
This functions doesn't seem to exist. Is there a prerequisite patch series that
introduces this function?
> +}
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v3 3/4] stk1160: Add module param for setting the record gain.
From: Mauro Carvalho Chehab @ 2016-12-05 12:12 UTC (permalink / raw)
To: Ezequiel Garcia; +Cc: Marcel Hasler, Mauro Carvalho Chehab, linux-media
In-Reply-To: <CAAEAJfAoZCzh5c=C+8Um-KaZkRs_ip1kX04xZRm2bWrGLmMwjA@mail.gmail.com>
Em Sun, 4 Dec 2016 15:25:25 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:
> On 4 December 2016 at 10:01, Marcel Hasler <mahasler@gmail.com> wrote:
> > Hello
> >
> > 2016-12-03 21:46 GMT+01:00 Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>:
> >> On 2 December 2016 at 08:05, Mauro Carvalho Chehab
> >> <mchehab@s-opensource.com> wrote:
> >>> Em Sun, 27 Nov 2016 12:11:48 +0100
> >>> Marcel Hasler <mahasler@gmail.com> escreveu:
> >>>
> >>>> Allow setting a custom record gain for the internal AC97 codec (if available). This can be
> >>>> a value between 0 and 15, 8 is the default and should be suitable for most users. The Windows
> >>>> driver also sets this to 8 without any possibility for changing it.
> >>>
> >>> The problem of removing the mixer is that you need this kind of
> >>> crap to setup the volumes on a non-standard way.
> >>>
> >>
> >> Right, that's a good point.
> >>
> >>> NACK.
> >>>
> >>> Instead, keep the alsa mixer. The way other drivers do (for example,
> >>> em28xx) is that they configure the mixer when an input is selected,
> >>> increasing the volume of the active audio channel to 100% and muting
> >>> the other audio channels. Yet, as the alsa mixer is exported, users
> >>> can change the mixer settings in runtime using some alsa (or pa)
> >>> mixer application.
> >>>
> >>
> >> Yeah, the AC97 mixer we are currently leveraging
> >> exposes many controls that have no meaning in this device,
> >> so removing that still looks like an improvement.
> >>
> >> I guess the proper way is creating our own mixer
> >> (not using snd_ac97_mixer) exposing only the record
> >> gain knob.
> >>
> >> Marcel, what do you think?
> >> --
> >> Ezequiel García, VanguardiaSur
> >> www.vanguardiasur.com.ar
> >
> > As I have written before, the recording gain isn't actually meant to
> > be changed by the user. In the official Windows driver this value is
> > hard-coded to 8 and cannot be changed in any way. And there really is
> > no good reason why anyone should need to mess with it in the first
> > place. The default value will give the best results in pretty much all
> > cases and produces approximately the same volume as the internal 8-bit
> > ADC whose gain cannot be changed at all, not even by a driver.
> >
> > I had considered writing a mixer but chose not to. If the gain setting
> > is openly exposed to mixer applications, how do you tell the users
> > that the value set by the driver already is the optimal and
> > recommended value and that they shouldn't mess with the controls
> > unless they really have to? By having a module parameter, this setting
> > is practically hidden from the normal user but still is available to
> > power-users if they think they really need it. In the end it's really
> > just a compromise between hiding it completely and exposing it openly.
> > Also, this way the driver guarantees reproducible results, since
> > there's no need to remember the positions of any volume sliders.
> >
>
> Hm, right. I've never changed the record gain, and it's true that it
> doens't really improve the volume. So, I would be OK with having
> a module parameter.
>
> On the other side, we are exposing it currently, through the "Capture"
> mixer control:
>
> Simple mixer control 'Capture',0
> Capabilities: cvolume cswitch cswitch-joined
> Capture channels: Front Left - Front Right
> Limits: Capture 0 - 15
> Front Left: Capture 10 [67%] [15.00dB] [on]
> Front Right: Capture 8 [53%] [12.00dB] [on]
>
> So, it would be user-friendly to keep the user interface and continue
> to expose the same knob - even if the default is the optimal, etc.
>
> To be completely honest, I don't think any user is really relying
> on any REC_GAIN / Capture setting, and I'm completely OK
> with having a mixer control or a module parameter. It doesn't matter.
If you're positive that *all* stk1160 use the ac97 mixer the
same way, and that there's no sense on having a mixer for it,
then it would be ok to remove it.
In such case, then why you need a modprobe parameter to allow
setting the record level? If this mixer entry is not used,
just set it to zero and be happy with that.
Regards,
Mauro
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox