Apparently UVD doesn't yet work everywhere - so allow it to be disabled. Shaves off some reboot and suspend/resume time on machines where it doesn't work. Might be useful for problematic chips in the future as well.
Patch attached as well as inline below.
Signed-off-by: Parag Warudkar parag.lkml@gmail.com
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 1442ce7..f131d8f 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -79,6 +79,7 @@ * Modules parameters. */ extern int radeon_no_wb; +extern int radeon_no_uvd; extern int radeon_modeset; extern int radeon_dynclks; extern int radeon_r4xx_atom; diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index d33f484..7e5b171 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -147,6 +147,7 @@ static inline void radeon_unregister_atpx_handler(void) {} #endif
int radeon_no_wb; +int radeon_no_uvd; int radeon_modeset = 1; int radeon_dynclks = -1; int radeon_r4xx_atom = 0; @@ -168,6 +169,9 @@ int radeon_fastfb = 0; MODULE_PARM_DESC(no_wb, "Disable AGP writeback for scratch registers"); module_param_named(no_wb, radeon_no_wb, int, 0444);
+MODULE_PARM_DESC(no_uvd, "Disable UVD"); +module_param_named(no_uvd, radeon_no_uvd, int, 0444); + MODULE_PARM_DESC(modeset, "Disable/Enable modesetting"); module_param_named(modeset, radeon_modeset, int, 0400);
diff --git a/drivers/gpu/drm/radeon/radeon_drv.h b/drivers/gpu/drm/radeon/radeon_drv.h index b369d42..4320973 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.h +++ b/drivers/gpu/drm/radeon/radeon_drv.h @@ -329,6 +329,7 @@ typedef struct drm_radeon_kcmd_buffer { } drm_radeon_kcmd_buffer_t;
extern int radeon_no_wb; +extern int radeon_no_uvd; extern struct drm_ioctl_desc radeon_ioctls[]; extern int radeon_max_ioctl;
diff --git a/drivers/gpu/drm/radeon/radeon_uvd.c b/drivers/gpu/drm/radeon/radeon_uvd.c index 906e5c0..93a7dbb 100644 --- a/drivers/gpu/drm/radeon/radeon_uvd.c +++ b/drivers/gpu/drm/radeon/radeon_uvd.c @@ -58,7 +58,8 @@ int radeon_uvd_init(struct radeon_device *rdev) unsigned long bo_size; const char *fw_name; int i, r; - + if (radeon_no_uvd) + return -EINVAL; INIT_DELAYED_WORK(&rdev->uvd.idle_work, radeon_uvd_idle_work_handler);
pdev = platform_device_register_simple("radeon_uvd", 0, NULL, 0);
On Mon, 2013-05-06 at 19:11 -0400, Parag Warudkar wrote:
Apparently UVD doesn't yet work everywhere - so allow it to be disabled. Shaves off some reboot and suspend/resume time on machines where it doesn't work. Might be useful for problematic chips in the future as well.
Patch attached as well as inline below.
Signed-off-by: Parag Warudkar parag.lkml@gmail.com
[...]
@@ -168,6 +169,9 @@ int radeon_fastfb = 0; MODULE_PARM_DESC(no_wb, "Disable AGP writeback for scratch registers"); module_param_named(no_wb, radeon_no_wb, int, 0444);
+MODULE_PARM_DESC(no_uvd, "Disable UVD"); +module_param_named(no_uvd, radeon_no_uvd, int, 0444);
No more negative boolean options please.
diff --git a/drivers/gpu/drm/radeon/radeon_uvd.c b/drivers/gpu/drm/radeon/radeon_uvd.c index 906e5c0..93a7dbb 100644 --- a/drivers/gpu/drm/radeon/radeon_uvd.c +++ b/drivers/gpu/drm/radeon/radeon_uvd.c @@ -58,7 +58,8 @@ int radeon_uvd_init(struct radeon_device *rdev) unsigned long bo_size; const char *fw_name; int i, r;
- if (radeon_no_uvd)
- return -EINVAL; INIT_DELAYED_WORK(&rdev->uvd.idle_work, radeon_uvd_idle_work_handler);
Returning -EINVAL here results in rather misleading dmesg output I think. This should probably be handled more gracefully.
Anyway, the return statement would need to be indented per the kernel coding style, and please leave empty lines before the if and after the return.
Am 07.05.2013 09:02, schrieb Michel Dänzer:
On Mon, 2013-05-06 at 19:11 -0400, Parag Warudkar wrote:
Apparently UVD doesn't yet work everywhere - so allow it to be disabled. Shaves off some reboot and suspend/resume time on machines where it doesn't work. Might be useful for problematic chips in the future as well.
Patch attached as well as inline below.
Signed-off-by: Parag Warudkar parag.lkml@gmail.com
The patch shouldn't be necessary because just removing the firmware should have pretty much the same effect.
On the other hand we only have three reports of failed VCPU bootup, while it just seems to be a setup problems in two of those cases (identical hardware seems to work for other people).
The only case where we indeed seems to have a problem are Macs with integrated cards, and we can always just blacklist those if the problem doesn't seems to be solvable.
Christian.
[...]
@@ -168,6 +169,9 @@ int radeon_fastfb = 0; MODULE_PARM_DESC(no_wb, "Disable AGP writeback for scratch registers"); module_param_named(no_wb, radeon_no_wb, int, 0444);
+MODULE_PARM_DESC(no_uvd, "Disable UVD"); +module_param_named(no_uvd, radeon_no_uvd, int, 0444);
No more negative boolean options please.
diff --git a/drivers/gpu/drm/radeon/radeon_uvd.c b/drivers/gpu/drm/radeon/radeon_uvd.c index 906e5c0..93a7dbb 100644 --- a/drivers/gpu/drm/radeon/radeon_uvd.c +++ b/drivers/gpu/drm/radeon/radeon_uvd.c @@ -58,7 +58,8 @@ int radeon_uvd_init(struct radeon_device *rdev) unsigned long bo_size; const char *fw_name; int i, r;
- if (radeon_no_uvd)
- return -EINVAL; INIT_DELAYED_WORK(&rdev->uvd.idle_work, radeon_uvd_idle_work_handler);
Returning -EINVAL here results in rather misleading dmesg output I think. This should probably be handled more gracefully.
Anyway, the return statement would need to be indented per the kernel coding style, and please leave empty lines before the if and after the return.
On Tue, May 7, 2013 at 4:44 AM, Christian König deathsimple@vodafone.de wrote:
The patch shouldn't be necessary because just removing the firmware should have pretty much the same effect.
Soon distros will ship the UVD firmware by default and then users will need to manually remove it and then do the same with every update. Besides, I just discovered that when UVD is enabled suspend resume breaks - tried 3 times with SUMO_uvd loaded - machine suspends but resumes instantly. Without SUMO_uvd.bin - suspends fine and only wakes up when I want it to.
The only case where we indeed seems to have a problem are Macs with integrated cards, and we can always just blacklist those if the problem doesn't seems to be solvable.
I happen to have the problematic card in my iMac - I'd be glad to provide any info or try any patches. Just let me know. For now I will remove the firmware - I reboot /suspend-resume often and it is a bit annoying to have to go through those mdelays only to fail.
Thanks, Parag
Am 07.05.2013 23:13, schrieb Parag Warudkar:
On Tue, May 7, 2013 at 4:44 AM, Christian König deathsimple@vodafone.de wrote:
The patch shouldn't be necessary because just removing the firmware should have pretty much the same effect.
Soon distros will ship the UVD firmware by default and then users will need to manually remove it and then do the same with every update. Besides, I just discovered that when UVD is enabled suspend resume breaks - tried 3 times with SUMO_uvd loaded - machine suspends but resumes instantly. Without SUMO_uvd.bin - suspends fine and only wakes up when I want it to.
Hui? Wait a second, the firmware doesn't work but still causes an instant resume on suspend? Very strange.
The only case where we indeed seems to have a problem are Macs with integrated cards, and we can always just blacklist those if the problem doesn't seems to be solvable.
I happen to have the problematic card in my iMac - I'd be glad to provide any info or try any patches. Just let me know. For now I will remove the firmware - I reboot /suspend-resume often and it is a bit annoying to have to go through those mdelays only to fail.
Yeah, perfectly understandable.
My best guess is that it has something todo with a different clock routing on Macs, but without access to the hardware (or precise documentation from Apple what the heck they did different) I don't really see a chance to solve that problem.
If you want to hack a bit on it you could try commenting out the calls to "radeon_set_uvd_clocks" in radeon_uvd.c. That should give you the default clocks of 100Mhz, not enough for usable decoding, but on SUMO based UVD blocks a very failsafe default.
Whatever it is, please send me an output of lspci, so I can blacklist the offending chip.
Christian.
Thanks, Parag
On Wed, 8 May 2013, Christian König wrote:
Am 07.05.2013 23:13, schrieb Parag Warudkar:
On Tue, May 7, 2013 at 4:44 AM, Christian König deathsimple@vodafone.de wrote: Without SUMO_uvd.bin - suspends fine and only wakes up when I want it to.
Hui? Wait a second, the firmware doesn't work but still causes an instant resume on suspend? Very strange.
Yes, I tested this multiple times - if firmware loads and the init bails out, machine resume instantly, like this -
[ 3631.441257] PM: Entering mem sleep [ 3631.441274] Suspending console(s) (use no_console_suspend to debug) [ 3631.441825] sd 0:0:0:0: [sda] Synchronizing SCSI cache [ 3631.442003] sd 0:0:0:0: [sda] Stopping disk [ 3631.755076] PM: suspend of devices complete after 313.257 msecs [ 3631.755219] PM: late suspend of devices complete after 0.134 msecs [ 3631.795185] PM: noirq suspend of devices complete after 39.904 msecs [ 3631.821922] pcieport 0000:00:1c.1: power state changed by ACPI to D0 [ 3631.915378] PM: noirq resume of devices complete after 119.999 msecs [ 3631.915459] PM: early resume of devices complete after 0.062 msecs [ 3631.915525] ahci 0000:00:1f.2: setting latency timer to 64 [ 3631.915609] ehci-pci 0000:00:1a.7: setting latency timer to 64 [ 3631.915654] uhci_hcd 0000:00:1a.0: setting latency timer to 64 [ 3631.915690] usb usb1: root hub lost power or was reset [ 3631.915709] snd_hda_intel 0000:00:1b.0: irq 46 for MSI/MSI-X [ 3631.915770] uhci_hcd 0000:00:1d.0: setting latency timer to 64 [ 3631.915792] usb usb2: root hub lost power or was reset [ 3631.915804] ehci-pci 0000:00:1d.7: setting latency timer to 64 [ 3631.915821] snd_hda_intel 0000:01:00.1: irq 48 for MSI/MSI-X [ 3631.918662] [drm] probing gen 2 caps for device 8086:101 = 2212d02/0 [ 3631.918665] [drm] PCIE gen 2 link speeds already enabled [ 3631.921479] [drm] PCIE GART of 512M enabled (table at 0x0000000000040000). [ 3631.921584] radeon 0000:01:00.0: WB enabled
Right now, I have removed SUMO_uvd.bin and suspend resume is working without fail. Strange indeed, and I only noticed it when the machine did not instant resume when running with the UVD disable patch and no_uvd=1 which skips uvd init just like firmware lodaing failure does I suppose.
My best guess is that it has something todo with a different clock routing on Macs, but without access to the hardware (or precise documentation from Apple what the heck they did different) I don't really see a chance to solve that problem.
Hopefully it is not that hard and we'll find a way! I will experiment the clocks and see how that goes.
If you want to hack a bit on it you could try commenting out the calls to "radeon_set_uvd_clocks" in radeon_uvd.c. That should give you the default clocks of 100Mhz, not enough for usable decoding, but on SUMO based UVD blocks a very failsafe default.
Whatever it is, please send me an output of lspci, so I can blacklist the offending chip.
Here is the lspci -nn output :
01:00.0 VGA compatible controller [0300]: Advanced Micro Devices [AMD] nee ATI Whistler [Radeon HD 6600M/6700M/7600M Series] [1002:6741]
Parag
On Wed, 8 May 2013, Parag Warudkar wrote:
On Wed, 8 May 2013, Christian König wrote:
If you want to hack a bit on it you could try commenting out the calls to "radeon_set_uvd_clocks" in radeon_uvd.c. That should give you the default clocks of 100Mhz, not enough for usable decoding, but on SUMO based UVD blocks a very failsafe default.
Commenting out the two calls to radeon_set_uvd_clocks() did not make any difference - still fails to initialize. BTW, this is also an EFI install. Sounds like for some people BIOS mode works based on the bug report.
Also confirming that the suspend/resume issue persists with the SUMO_uvd.bin firmware loaded even if the UVD init fails. It is only by removing the firmware I can get the machine to suspend reliably.
Parag
dri-devel@lists.freedesktop.org
-
Christian König -
Michel Dänzer -
Parag Warudkar