Re: [PATCH 1/1] smiapp: Implement power-on and power-off sequences without runtime PM
Hi Alan, On Saturday 26 Nov 2016 15:10:28 Alan Stern wrote: > On Fri, 25 Nov 2016, Laurent Pinchart wrote: > > On Friday 25 Nov 2016 10:21:21 Alan Stern wrote: > >> On Fri, 25 Nov 2016, Sakari Ailus wrote: > >>> On Thu, Nov 24, 2016 at 09:15:39PM -0500, Alan Stern wrote: > On Fri, 25 Nov 2016, Laurent Pinchart wrote: > > Dear linux-pm developers, what's the suggested way to ensure that a > > runtime- pm-enabled driver can run fine on a system with CONFIG_PM > > disabled ? > > The exact point of your question isn't entirely clear. In the most > literal sense, the best ways to ensure this are (1) audit the code, > and (2) actually try it. > > I have a feeling this doesn't quite answer your question, however. > :-) > >>> > >>> The question is related to devices that require certain power-up and > >>> power-down sequences that are now implemented as PM runtime hooks > >>> that, without CONFIG_PM defined, will not be executed. Is there a > >>> better way than to handle this than have an implementation in the > >>> driver for the PM runtime and non-PM runtime case separately? > >> > >> Yes, there is a better way. For the initial power-up and final > >> power-down sequences, don't rely on the PM core to invoke the > >> callbacks. Just call them directly, yourself. > >> > >> For example, as part of the probe routine, instead of doing this: > >>pm_runtime_set_suspended(dev); > >>pm_runtime_enable(dev); > >>pm_runtime_get_sync(dev); > >> > >> Do this: > >>pm_runtime_set_active(dev); > >>pm_runtime_get_noresume(dev); > >>pm_runtime_enable(dev); > >>/* > >> * In case CONFIG_PM is disabled, invoke the runtime-resume > >> * callback directly. > >> */ > >>my_runtime_resume(dev); > > > > Wouldn't it be cleaner for drivers not to have to handle this manually > > (which gives an opportunity to get it wrong) but instead have > > pm_runtime_enable() call the runtime resume callback when CONFIG_PM is > > disabled ? > > Well, I admit it would be nicer if drivers didn't have to worry about > whether or not CONFIG_PM was enabled. A slightly cleaner approach > from the one outlined above would have the probe routine do this: > > my_power_up(dev); > pm_runtime_set_active(dev); > pm_runtime_get_noresume(dev); > pm_runtime_enable(dev); > > and have the runtime-resume callback routine call my_power_up() to do > its work. (Or make my_power_up() actually be the runtime-resume > callback routine.) That's pretty straightforward and hard to mess up. You'd be surprised how easy drivers can mess simple things up ;-) We'd still have to get the message out there, that would be the most difficult part. > In theory, we could have pm_runtime_enable() invoke the runtime-resume > callback when CONFIG_PM is disabled. In practice, it would be rather > awkward. drivers/base/power/runtime.c, which is where > pm_runtime_enable() is defined and the runtime-PM callbacks are > invoked, doesn't even get compiled if CONFIG_PM is off. Sure, but that can easily be fixed. > (Also, it would run against the grain. CONFIG_PM=n means the kernel > ignores runtime PM, so pm_runtime_enable() shouldn't do anything.) I'd argue that CONFIG_PM=n should mean that the runtime PM API doesn't perform runtime PM, not that it should do absolutely nothing. If semantics is the biggest concern, we could introduce a helper (whose name is TBD) that would enable runtime PM when CONFIG_PM=y or power on the device when CONFIG_PM=n I want to make it as easy as possible for drivers to make sure they won't get this wrong, which in my opinion requires a simple and straightforward API with no code in the driver that would depend on the value of CONFIG_PM. > There's a corollary aspect to this. If you depend on runtime PM for > powering up your device during probe, does that mean you also depend on > runtime PM for powering down the device during remove? That is likely > not to work, because the user can prevent runtime suspends by writing > to /sys/.../power/control. Yes, I do, and I expect most runtime PM-enabled driver to do the same. When runtime suspend is disabled through /sys/.../power/control does pm_runtime_disable() invoke the runtime PM suspend handler if the device is powered on ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
dip7000p: division by zero
Hi, I have this device: Bus 004 Device 018: ID 2304:0229 Pinnacle Systems, Inc. PCTV Dual DVB-T 2001e connected to ARM based Turris Omnia router running 4.4.13 kernel. I meet this issue more than often: 2016-11-27T16:42:34+01:00 err kernel[]: [17010.721803] Division by zero in kernel. 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.725975] CPU: 0 PID: 12589 Comm: kdvb-ad-1-fe-0 Not tainted 4.4.13-05df79f63527051ea0071350f86faf76-9 #1 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.725980] Hardware name: Marvell Armada 380/385 (Device Tree) 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.725983] Backtrace: 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.725995] [] (dump_backtrace) from [] (show_stack+0x18/0x1c) 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.725998] r6: r5:600e0113 r4:c0688208 r3: 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726011] [] (show_stack) from [] (dump_stack+0x98/0xac) 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726018] [] (dump_stack) from [] (__div0+0x1c/0x20) 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726020] r6: r5: r4:e4f73000 r3:0006 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726030] [] (__div0) from [] (Ldiv0+0x8/0x10) 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726064] [] (dib7000p_set_frontend [dib7000p]) from [] (dvb_ca_en50221_thread+0x8e0/0x9e0 [dvb_core] ) 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726067] r10:c06a8808 r9:e05b2000 r8:e4f73244 r7:2b82ea80 r6:0002 r5:e4f73000 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726076] r4:e05b2000 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726094] [] (dvb_ca_en50221_thread [dvb_core]) from [] (dvb_frontend_sleep_until+0xc30/0xcb0 [dvb_co re]) 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726097] r9:e05b2000 r8:e05b21b0 r7: r6:c51ddf1c r5:e4f73000 r4:e05b2000 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726118] [] (dvb_frontend_sleep_until [dvb_core]) from [] (dvb_frontend_thread+0x398/0x528 [dvb_core ]) 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726121] r6:c51ddf1c r5:e4f73000 r4: 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726135] [] (dvb_frontend_thread [dvb_core]) from [] (kthread+0xfc/0x100) 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726138] r10: r9: r8: r7:bf8772c0 r6:e4f73000 r5: 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726147] r4:c5d01a00 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726154] [] (kthread) from [] (ret_from_fork+0x14/0x3c) 2016-11-27T16:42:34+01:00 warning kernel[]: [17010.726157] r7: r6: r5:c004291c r4:c5d01a00 2016-11-27T16:42:34+01:00 debug kernel[]: [17010.726164] DiB7000P: setting a frequency offset of 0kHz internal freq = 0 invert = 0 As I was checking the code, this can be in path dib7000p_set_frontend() dib7000p_agc_startup() dib7000p_set_dds() dib7000p_get_internal_freq() dib7000p_read_word() There is lot of I2C read errors resembling dprintk from the last function which is probably related to root cause (which I haven't found yet). IMHO dib7000p_read_word() lacks error reporting to higher levels. In case of read failure last content is returned. Then dib7000p_get_internal_freq() does some calculations but it returns 0 if the registers were "read" as 0. If dib7000p_read_word() fails, this is the place to provide sane value. dib7000p_set_dds() uses returned value for divison. When I use this tuner with my x86_64 based notebook running 4.8.6 kernel it works perfectly so that rules out HW issue. So, do you agree with my findings? Any ideas why there is so many I2C read errors? Thanks in advance. Best regards, Tomas Cech -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Enabling peer to peer device transactions for PCIe devices
+Qiang, who is working on it. On 2016年11月27日 22:07, Christian König wrote: Am 27.11.2016 um 15:02 schrieb Haggai Eran: On 11/25/2016 9:32 PM, Jason Gunthorpe wrote: On Fri, Nov 25, 2016 at 02:22:17PM +0100, Christian König wrote: Like you say below we have to handle short lived in the usual way, and that covers basically every device except IB MRs, including the command queue on a NVMe drive. Well a problem which wasn't mentioned so far is that while GPUs do have a page table to mirror the CPU page table, they usually can't recover from page faults. So what we do is making sure that all memory accessed by the GPU Jobs stays in place while those jobs run (pretty much the same pinning you do for the DMA). Yes, it is DMA, so this is a valid approach. But, you don't need page faults from the GPU to do proper coherent page table mirroring. Basically when the driver submits the work to the GPU it 'faults' the pages into the CPU and mirror translation table (instead of pinning). Like in ODP, MMU notifiers/HMM are used to monitor for translation changes. If a change comes in the GPU driver checks if an executing command is touching those pages and blocks the MMU notifier until the command flushes, then unfaults the page (blocking future commands) and unblocks the mmu notifier. I think blocking mmu notifiers against something that is basically controlled by user-space can be problematic. This can block things like memory reclaim. If you have user-space access to the device's queues, user-space can block the mmu notifier forever. Really good point. I think this means the bare minimum if we don't have recoverable page faults is to have preemption support like Felix described in his answer as well. Going to keep that in mind, Christian. ___ dri-devel mailing list dri-de...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
cron job: media_tree daily build: ERRORS
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 Nov 28 05:00:16 CET 2016 media-tree git hash:d3d83ee20afda16ad0133ba00f63c11a8d842a35 media_build git hash: 1606032398b1d79149c1507be2029e1a00d8dff0 v4l-utils git hash: dab9bf5687eddea2f4cb8cdb38b3bbc5b079a037 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: ir-keytable: infinite loops, segfaults
On Fri, Nov 25, 2016 at 07:59:21PM +1100, Vincent McIntyre wrote: > On 11/25/16, Sean Young wrote: > > > > So if I understand you correctly, if you change the keymap, like you > > changed 0xfe47 to KEY_PAUSE, then "ir-keytable -s rc1 -t" show you the > > correct (new) key? So as far as ir-keytable is concerned, everything > > works? > > > > However when you try to use the new mapping in some application then > > it does not work? > > That's correct. ir-keytable seems to be doing the right thing, mapping > the scancode to the input-event-codes.h key code I asked it to. ir-keytable reads from the input layer, so that's the key being sent. The problem is elsewhere. > The application I am trying to use it with is the mythtv frontend. I > am doing the keycode munging from an SSH session while myth is still > running on the main screen. I didn't think this would matter (since it > worked for KEY_OK->KEY_ENTER) but perhaps it does. Obviously > ir-keytable -t intercepts the scancodes when it is running, but when I > kill it myth responds normally to some keys, but not all. X and keycodes is a bit messy. You might need xmodmap mappings. You can check them xev. I don't know much about this, I'm afraid. What linux distribution, version and keyboard layout are you using? I could try and see if I can reproduce/fix this. > I wanted to mention that the IR protocol is still showing as unknown. > Is there anything that can be done to sort that out? It would be nice if that could be sorted out, although that would be a separate patch. So all we know right now is what scancode the IR receiver hardware produces but we have no idea what IR protocol is being used. In order to figure this out we need a recording of the IR the remote sends, for which a different IR receiver is needed. Neither your imon nor your dvb_usb_af9035 can do this, something like a mce usb IR receiver would be best. Do you have access to one? One with an IR emitter would be best. So with that we can have a recording of the IR the remote sends, and with the emitter we can see which IR protocols the IR receiver understands. Sean -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [media] lirc: fix error paths in lirc_cdev_add()
"c77d17c0 [media] lirc: use-after free" introduces two problems:
cdev_del() can be called with a NULL argument, and the kobject_put()
path will cause a double free.
Reported-by: Dan Carpenter
Signed-off-by: Sean Young
---
drivers/media/rc/lirc_dev.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index d3039ef..3854809 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -157,13 +157,13 @@ static const struct file_operations lirc_dev_fops = {
static int lirc_cdev_add(struct irctl *ir)
{
- int retval = -ENOMEM;
struct lirc_driver *d = &ir->d;
struct cdev *cdev;
+ int retval;
cdev = cdev_alloc();
if (!cdev)
- goto err_out;
+ return -ENOMEM;
if (d->fops) {
cdev->ops = d->fops;
@@ -177,10 +177,8 @@ static int lirc_cdev_add(struct irctl *ir)
goto err_out;
retval = cdev_add(cdev, MKDEV(MAJOR(lirc_base_dev), d->minor), 1);
- if (retval) {
- kobject_put(&cdev->kobj);
+ if (retval)
goto err_out;
- }
ir->cdev = cdev;
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
VIDIOC_QUERYCTRL ioctl( ) failing with errno 5 (EIO, Input/output error)
Hi, When making a VIDIOC_QUERYCTRL ioctl( ) call on a UVC video capture device (USB camera), occasionally I get an Input/Output error (errno=5, EIO). However, it is not defined as a possible returned error either in the VIDIOC_QUERYCTRL docs here https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-queryctrl.html , nor as a generic error for ioctl( ) calls or for v4l2 calls. After digging in the code, it looks that this ioctl( ) eventually makes a USB request and calls usb_start_wait_urb( ), which, I guess, is what ends up returning EIO if something goes wrong. So my question is, shouldn't this be documented in the link above? At least for the UVC case? Luis -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Enabling peer to peer device transactions for PCIe devices
On 11/25/2016 9:32 PM, Jason Gunthorpe wrote: > On Fri, Nov 25, 2016 at 02:22:17PM +0100, Christian König wrote: > >>> Like you say below we have to handle short lived in the usual way, and >>> that covers basically every device except IB MRs, including the >>> command queue on a NVMe drive. >> >> Well a problem which wasn't mentioned so far is that while GPUs do have a >> page table to mirror the CPU page table, they usually can't recover from >> page faults. > >> So what we do is making sure that all memory accessed by the GPU Jobs stays >> in place while those jobs run (pretty much the same pinning you do for the >> DMA). > > Yes, it is DMA, so this is a valid approach. > > But, you don't need page faults from the GPU to do proper coherent > page table mirroring. Basically when the driver submits the work to > the GPU it 'faults' the pages into the CPU and mirror translation > table (instead of pinning). > > Like in ODP, MMU notifiers/HMM are used to monitor for translation > changes. If a change comes in the GPU driver checks if an executing > command is touching those pages and blocks the MMU notifier until the > command flushes, then unfaults the page (blocking future commands) and > unblocks the mmu notifier. I think blocking mmu notifiers against something that is basically controlled by user-space can be problematic. This can block things like memory reclaim. If you have user-space access to the device's queues, user-space can block the mmu notifier forever. On PeerDirect, we have some kind of a middle-ground solution for pinning GPU memory. We create a non-ODP MR pointing to VRAM but rely on user-space and the GPU not to migrate it. If they do, the MR gets destroyed immediately. This should work on legacy devices without ODP support, and allows the system to safely terminate a process that misbehaves. The downside of course is that it cannot transparently migrate memory but I think for user-space RDMA doing that transparently requires hardware support for paging, via something like HMM. ... > I'm hearing most people say ZONE_DEVICE is the way to handle this, > which means the missing remaing piece for RDMA is some kind of DMA > core support for p2p address translation.. Yes, this is definitely something we need. I think Will Davis's patches are a good start. Another thing I think is that while HMM is good for user-space applications, for kernel p2p use there is no need for that. Using ZONE_DEVICE with or without something like DMA-BUF to pin and unpin pages for the short duration as you wrote above could work fine for kernel uses in which we can guarantee they are short. Haggai -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] [media] uvcvideo: freeing an error pointer
Hi Dan, On Fri, Nov 25, 2016 at 10:20:24PM +0300, Dan Carpenter wrote: > On Fri, Nov 25, 2016 at 06:02:45PM +0200, Laurent Pinchart wrote: > > Sakari Ailus (CC'ed) has expressed the opinion that we might want to go one > > step further and treat error pointers the same way we treat NULL or ZERO > > pointers today, by just returning without logging anything. The reasoning > > is > > that accepting a NULL pointer in kfree() was decided before we made > > extensive > > use of allocation APIs returning error pointers, so it could be time to > > update > > kfree() based on the current allocation usage patterns. > > Just don't free things that haven't been allocated. That honestly seems > like a simple rule to me, whenever I touch error handling code it feels > better and simpler after I fix the bugs. Error handling doesn't have to > be complicated if you just follow the rules. kfree() explicitly allows passing a NULL pointer to it; drivers often call kfree() on objects possibly allocated using kmalloc() and friends. This makes error handling easier in drivers which in turn decreases the probability of bugs, the other side of which we've already seen in form of the bug this patch fixes. Previously interfaces that allocated memory tended to either allocate that memory or in failing to do so, returned error in form of a NULL pointer. memdup_user() breaks that assumption by returning a negative error value as a pointer instead. I suppose one of the motivations of memdup_user() has been to reduce complexity of driver code as well as framework code dealing with implementing IOCTLs but at least in this case the end result was an introduction of a bug. This would not have happened in the first place if the API of functions dealing with releasing memory had been updated as well. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: DVB: Unable to find symbol af9013_attach() (probably not a bug)
hello guys. At the end the problem seems related to the use of the CONFIG_TRIM_UNUSED_KSYMS option. Now everything works fine. Thanks, Enrico -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Enabling peer to peer device transactions for PCIe devices
On 11/25/2016 9:32 PM, Jason Gunthorpe wrote: > On Fri, Nov 25, 2016 at 02:22:17PM +0100, Christian König wrote: > >>> Like you say below we have to handle short lived in the usual way, and >>> that covers basically every device except IB MRs, including the >>> command queue on a NVMe drive. >> >> Well a problem which wasn't mentioned so far is that while GPUs do have a >> page table to mirror the CPU page table, they usually can't recover from >> page faults. > >> So what we do is making sure that all memory accessed by the GPU Jobs stays >> in place while those jobs run (pretty much the same pinning you do for the >> DMA). > > Yes, it is DMA, so this is a valid approach. > > But, you don't need page faults from the GPU to do proper coherent > page table mirroring. Basically when the driver submits the work to > the GPU it 'faults' the pages into the CPU and mirror translation > table (instead of pinning). > > Like in ODP, MMU notifiers/HMM are used to monitor for translation > changes. If a change comes in the GPU driver checks if an executing > command is touching those pages and blocks the MMU notifier until the > command flushes, then unfaults the page (blocking future commands) and > unblocks the mmu notifier. I think blocking mmu notifiers against something that is basically controlled by user-space can be problematic. This can block things like memory reclaim. If you have user-space access to the device's queues, user-space can block the mmu notifier forever. On PeerDirect, we have some kind of a middle-ground solution for pinning GPU memory. We create a non-ODP MR pointing to VRAM but rely on user-space and the GPU not to migrate it. If they do, the MR gets destroyed immediately. This should work on legacy devices without ODP support, and allows the system to safely terminate a process that misbehaves. The downside of course is that it cannot transparently migrate memory but I think for user-space RDMA doing that transparently requires hardware support for paging, via something like HMM. ... > I'm hearing most people say ZONE_DEVICE is the way to handle this, > which means the missing remaing piece for RDMA is some kind of DMA > core support for p2p address translation.. Yes, this is definitely something we need. I think Will Davis's patches are a good start. Another thing I think is that while HMM is good for user-space applications, for kernel p2p use there is no need for that. Using ZONE_DEVICE with or without something like DMA-BUF to pin and unpin pages for the short duration as you wrote above could work fine for kernel uses in which we can guarantee they are short. Haggai -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Enabling peer to peer device transactions for PCIe devices
Am 27.11.2016 um 15:02 schrieb Haggai Eran: On 11/25/2016 9:32 PM, Jason Gunthorpe wrote: On Fri, Nov 25, 2016 at 02:22:17PM +0100, Christian König wrote: Like you say below we have to handle short lived in the usual way, and that covers basically every device except IB MRs, including the command queue on a NVMe drive. Well a problem which wasn't mentioned so far is that while GPUs do have a page table to mirror the CPU page table, they usually can't recover from page faults. So what we do is making sure that all memory accessed by the GPU Jobs stays in place while those jobs run (pretty much the same pinning you do for the DMA). Yes, it is DMA, so this is a valid approach. But, you don't need page faults from the GPU to do proper coherent page table mirroring. Basically when the driver submits the work to the GPU it 'faults' the pages into the CPU and mirror translation table (instead of pinning). Like in ODP, MMU notifiers/HMM are used to monitor for translation changes. If a change comes in the GPU driver checks if an executing command is touching those pages and blocks the MMU notifier until the command flushes, then unfaults the page (blocking future commands) and unblocks the mmu notifier. I think blocking mmu notifiers against something that is basically controlled by user-space can be problematic. This can block things like memory reclaim. If you have user-space access to the device's queues, user-space can block the mmu notifier forever. Really good point. I think this means the bare minimum if we don't have recoverable page faults is to have preemption support like Felix described in his answer as well. Going to keep that in mind, Christian. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 4/4] stk1160: Give the chip some time to retrieve data from AC97 codec.
The STK1160 needs some time to transfer data from the AC97 registers into its own. On some systems reading the chip's own registers to soon will return wrong values. The "proper" way to handle this would be to poll STK1160_AC97CTL_0 after every read or write command until the command bit has been cleared, but this may not be worth the hassle. Signed-off-by: Marcel Hasler --- drivers/media/usb/stk1160/stk1160-ac97.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c b/drivers/media/usb/stk1160/stk1160-ac97.c index 60327af..b39f51b 100644 --- a/drivers/media/usb/stk1160/stk1160-ac97.c +++ b/drivers/media/usb/stk1160/stk1160-ac97.c @@ -23,6 +23,7 @@ * */ +#include #include #include "stk1160.h" @@ -64,6 +65,14 @@ static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg) */ stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8b); + /* +* Give the chip some time to transfer the data. +* The proper way would be to poll STK1160_AC97CTL_0 +* until the command bit has been cleared, but this +* may not be worth the hassle. +*/ + usleep_range(20, 40); + /* Retrieve register value */ stk1160_read_reg(dev, STK1160_AC97_CMD, &vall); stk1160_read_reg(dev, STK1160_AC97_CMD + 1, &valh); -- 2.10.2 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/4] stk1160: Add module param for setting the record gain.
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.
Signed-off-by: Marcel Hasler
---
drivers/media/usb/stk1160/stk1160-ac97.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c
b/drivers/media/usb/stk1160/stk1160-ac97.c
index 95648ac..60327af 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -28,6 +28,11 @@
#include "stk1160.h"
#include "stk1160-reg.h"
+static u8 gain = 8;
+
+module_param(gain, byte, 0444);
+MODULE_PARM_DESC(gain, "Set capture gain level if AC97 codec is available
(0-15, default: 8)");
+
static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value)
{
/* Set codec register address */
@@ -136,7 +141,10 @@ void stk1160_ac97_setup(struct stk1160 *dev)
stk1160_write_ac97(dev, 0x16, 0x0808); /* Aux volume */
stk1160_write_ac97(dev, 0x1a, 0x0404); /* Record select */
stk1160_write_ac97(dev, 0x02, 0x); /* Master volume */
- stk1160_write_ac97(dev, 0x1c, 0x0808); /* Record gain */
+
+ /* Record gain */
+ gain = (gain > 15) ? 15 : gain;
+ stk1160_write_ac97(dev, 0x1c, (gain<<8) | gain);
#ifdef DEBUG
stk1160_ac97_dump_regs(dev);
--
2.10.2
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/4] stk1160: Check whether to use AC97 codec.
Some STK1160-based devices use the chip's internal 8-bit ADC. This is
configured through a strap
pin. The value of this and other pins can be read through the POSVA register.
If the internal
ADC is used, or if audio is disabled altogether, there's no point trying to
setup the AC97 codec.
Signed-off-by: Marcel Hasler
---
drivers/media/usb/stk1160/stk1160-ac97.c | 26 ++
drivers/media/usb/stk1160/stk1160-core.c | 3 +--
drivers/media/usb/stk1160/stk1160-reg.h | 8
3 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c
b/drivers/media/usb/stk1160/stk1160-ac97.c
index 63ade1b..95648ac 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -93,8 +93,34 @@ void stk1160_ac97_dump_regs(struct stk1160 *dev)
}
#endif
+int stk1160_has_audio(struct stk1160 *dev)
+{
+ u8 value;
+
+ stk1160_read_reg(dev, STK1160_POSV_L, &value);
+ return !(value & STK1160_POSV_L_ACDOUT);
+}
+
+int stk1160_has_ac97(struct stk1160 *dev)
+{
+ u8 value;
+
+ stk1160_read_reg(dev, STK1160_POSV_L, &value);
+ return !(value & STK1160_POSV_L_ACSYNC);
+}
+
void stk1160_ac97_setup(struct stk1160 *dev)
{
+ if (!stk1160_has_audio(dev)) {
+ stk1160_info("Device doesn't support audio, skipping AC97
setup.");
+ return;
+ }
+
+ if (!stk1160_has_ac97(dev)) {
+ stk1160_info("Device uses internal 8-bit ADC, skipping AC97
setup.");
+ return;
+ }
+
/* Two-step reset AC97 interface and hardware codec */
stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x94);
stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c);
diff --git a/drivers/media/usb/stk1160/stk1160-core.c
b/drivers/media/usb/stk1160/stk1160-core.c
index f3c9b8a..c86eb61 100644
--- a/drivers/media/usb/stk1160/stk1160-core.c
+++ b/drivers/media/usb/stk1160/stk1160-core.c
@@ -20,8 +20,7 @@
*
* TODO:
*
- * 1. (Try to) detect if we must register ac97 mixer
- * 2. Support stream at lower speed: lower frame rate or lower frame size.
+ * 1. Support stream at lower speed: lower frame rate or lower frame size.
*
*/
diff --git a/drivers/media/usb/stk1160/stk1160-reg.h
b/drivers/media/usb/stk1160/stk1160-reg.h
index 81ff3a1..296a9e7 100644
--- a/drivers/media/usb/stk1160/stk1160-reg.h
+++ b/drivers/media/usb/stk1160/stk1160-reg.h
@@ -26,6 +26,14 @@
/* Remote Wakup Control */
#define STK1160_RMCTL 0x00c
+/* Power-on Strapping Data */
+#define STK1160_POSVA 0x010
+#define STK1160_POSV_L 0x010
+#define STK1160_POSV_M 0x011
+#define STK1160_POSV_H 0x012
+#define STK1160_POSV_L_ACDOUT BIT(3)
+#define STK1160_POSV_L_ACSYNC BIT(2)
+
/*
* Decoder Control Register:
* This byte controls capture start/stop
--
2.10.2
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/4] stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically.
Exposing all the channels of the device's internal AC97 codec to userspace is
unnecessary and
confusing. Instead the driver should setup the codec with proper values. This
patch removes the
mixer and sets up the codec using optimal values, i.e. the same values set by
the Windows
driver. This also makes the device work out-of-the-box, without the need for
the user to
reconfigure the device every time it's plugged in.
Signed-off-by: Marcel Hasler
---
drivers/media/usb/stk1160/Kconfig| 10 +--
drivers/media/usb/stk1160/Makefile | 4 +-
drivers/media/usb/stk1160/stk1160-ac97.c | 124 ---
drivers/media/usb/stk1160/stk1160-core.c | 5 +-
drivers/media/usb/stk1160/stk1160.h | 9 +--
5 files changed, 50 insertions(+), 102 deletions(-)
diff --git a/drivers/media/usb/stk1160/Kconfig
b/drivers/media/usb/stk1160/Kconfig
index 95584c1..22dff4f 100644
--- a/drivers/media/usb/stk1160/Kconfig
+++ b/drivers/media/usb/stk1160/Kconfig
@@ -8,17 +8,9 @@ config VIDEO_STK1160_COMMON
To compile this driver as a module, choose M here: the
module will be called stk1160
-config VIDEO_STK1160_AC97
- bool "STK1160 AC97 codec support"
- depends on VIDEO_STK1160_COMMON && SND
-
- ---help---
- Enables AC97 codec support for stk1160 driver.
-
config VIDEO_STK1160
tristate
- depends on (!VIDEO_STK1160_AC97 || (SND='n') || SND) &&
VIDEO_STK1160_COMMON
+ depends on VIDEO_STK1160_COMMON
default y
select VIDEOBUF2_VMALLOC
select VIDEO_SAA711X
- select SND_AC97_CODEC if SND
diff --git a/drivers/media/usb/stk1160/Makefile
b/drivers/media/usb/stk1160/Makefile
index dfe3e90..42d0546 100644
--- a/drivers/media/usb/stk1160/Makefile
+++ b/drivers/media/usb/stk1160/Makefile
@@ -1,10 +1,8 @@
-obj-stk1160-ac97-$(CONFIG_VIDEO_STK1160_AC97) := stk1160-ac97.o
-
stk1160-y := stk1160-core.o \
stk1160-v4l.o \
stk1160-video.o \
stk1160-i2c.o \
- $(obj-stk1160-ac97-y)
+ stk1160-ac97.o
obj-$(CONFIG_VIDEO_STK1160) += stk1160.o
diff --git a/drivers/media/usb/stk1160/stk1160-ac97.c
b/drivers/media/usb/stk1160/stk1160-ac97.c
index 2dd308f..63ade1b 100644
--- a/drivers/media/usb/stk1160/stk1160-ac97.c
+++ b/drivers/media/usb/stk1160/stk1160-ac97.c
@@ -4,6 +4,9 @@
* Copyright (C) 2012 Ezequiel Garcia
*
*
+ * Copyright (C) 2016 Marcel Hasler
+ *
+ *
* Based on Easycap driver by R.M. Thomas
* Copyright (C) 2010 R.M. Thomas
*
@@ -21,19 +24,12 @@
*/
#include
-#include
-#include
-#include
#include "stk1160.h"
#include "stk1160-reg.h"
-static struct snd_ac97 *stk1160_ac97;
-
-static void stk1160_write_ac97(struct snd_ac97 *ac97, u16 reg, u16 value)
+static void stk1160_write_ac97(struct stk1160 *dev, u16 reg, u16 value)
{
- struct stk1160 *dev = ac97->private_data;
-
/* Set codec register address */
stk1160_write_reg(dev, STK1160_AC97_ADDR, reg);
@@ -48,9 +44,9 @@ static void stk1160_write_ac97(struct snd_ac97 *ac97, u16
reg, u16 value)
stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x8c);
}
-static u16 stk1160_read_ac97(struct snd_ac97 *ac97, u16 reg)
+#ifdef DEBUG
+static u16 stk1160_read_ac97(struct stk1160 *dev, u16 reg)
{
- struct stk1160 *dev = ac97->private_data;
u8 vall = 0;
u8 valh = 0;
@@ -70,81 +66,53 @@ static u16 stk1160_read_ac97(struct snd_ac97 *ac97, u16 reg)
return (valh << 8) | vall;
}
-static void stk1160_reset_ac97(struct snd_ac97 *ac97)
+void stk1160_ac97_dump_regs(struct stk1160 *dev)
{
- struct stk1160 *dev = ac97->private_data;
- /* Two-step reset AC97 interface and hardware codec */
- stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x94);
- stk1160_write_reg(dev, STK1160_AC97CTL_0, 0x88);
+ u16 value;
- /* Set 16-bit audio data and choose L&R channel*/
- stk1160_write_reg(dev, STK1160_AC97CTL_1 + 2, 0x01);
-}
+ value = stk1160_read_ac97(dev, 0x12); /* CD volume */
+ stk1160_dbg("0x12 == 0x%04x", value);
-static struct snd_ac97_bus_ops stk1160_ac97_ops = {
- .read = stk1160_read_ac97,
- .write = stk1160_write_ac97,
- .reset = stk1160_reset_ac97,
-};
+ value = stk1160_read_ac97(dev, 0x10); /* Line-in volume */
+ stk1160_dbg("0x10 == 0x%04x", value);
-int stk1160_ac97_register(struct stk1160 *dev)
-{
- struct snd_card *card = NULL;
- struct snd_ac97_bus *ac97_bus;
- struct snd_ac97_template ac97_template;
- int rc;
+ value = stk1160_read_ac97(dev, 0x0e); /* MIC volume (mono) */
+ stk1160_dbg("0x0e == 0x%04x", value);
- /*
-* Just want a card to access ac96 controls,
-* the actual capture interface will be handled by snd-usb-audio
-*/
- rc = snd_card_new(dev->dev, SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1,
- THIS_MODULE, 0,
[PATCH v3 0/4] stk1160: Let the driver setup the device's internal AC97 codec
This patchset is a result of my attempt to fix a bug (https://bugzilla.kernel.org/show_bug.cgi?id=180071) that eventually turned out to be caused by a missing quirk in snd-usb-audio. My idea was to remove the AC97 interface and setup the codec using the same values and in the same order as the Windows driver does, hoping there might be some "magic" sequence that would make the sound work the way it should. Although this didn't help to fix the problem, I found these changes to be useful nevertheless. IMHO, having all of the AC97 codec's channels exposed to userspace is confusing since most of them have no meaning for this device anyway. Changing these values in alsamixer has either no effect at all or may even reduce the sound quality since it can actually increase the line-in DC offset (slightly). In addition, having to re-select the correct capture channel everytime the device has been plugged in is annoying. At least on my systems the mixer setup is only saved if the device is plugged in during shutdown/reboot. I also get error messages in my kernel log when I unplug the device because some process (probably the AC97 driver) ist trying to read from the device after it has been removed. Either way the device should work out-of-the-box without the need for the user to manually setup channels. The first patch in the set therefore removes the 'stk1160-mixer' and lets the driver setup the AC97 codec using the same values as the Windows driver. Although some of the values seem to be defaults I let the driver set them either way, just to be sure. The second patch adds a check to determine whether the device is strapped to use the internal 8-bit ADC or an external chip. There's currently no check in place to determine whether the device uses AC-link or I2S, but then again I haven't heard of any of these devices actually using an I2S chip. If the device uses the internal ADC the AC97 setup can be skipped. I implemented the check inside stk1160-ac97. It could just as well be in stk1160-core but this way just seemed cleaner. If at some point the need arises to check other power-on strap values, it might make sense to refactor this then. The third patch adds a new module parameter for setting the record gain manually since the AC97 chip is no longer exposed to userspace. The Windows driver doesn't allow this value to be changed but instead always sets it to 8 (of 15). While this should be fine for most users, some may prefer something higher. The fourth patch addresses an issue when reading from the AC97 chip too soon, resulting in corrupt data. Changes from version 2: * Added copyright notice * Added defines for POSVA bytes and bits * Added check for ACDOUT bit to determine whether audio is disabled completely * Removed info output for gain setting * Added fourth patch which had been submitted independently before * Expanded comment on AC97 read delay Marcel Hasler (4): stk1160: Remove stk1160-mixer and setup internal AC97 codec automatically. stk1160: Check whether to use AC97 codec. stk1160: Add module param for setting the record gain. stk1160: Give the chip some time to retrieve data from AC97 codec. drivers/media/usb/stk1160/Kconfig| 10 +- drivers/media/usb/stk1160/Makefile | 4 +- drivers/media/usb/stk1160/stk1160-ac97.c | 159 +-- drivers/media/usb/stk1160/stk1160-core.c | 8 +- drivers/media/usb/stk1160/stk1160-reg.h | 8 ++ drivers/media/usb/stk1160/stk1160.h | 9 +- 6 files changed, 98 insertions(+), 100 deletions(-) -- 2.10.2 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Xmas Offer
You are a recipient to Mrs Julie Leach Donation of $3 million USD. Contact ( julieleac...@gmail.com ) for claims. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
