[PATCH] radeon: Allow disabling UVD

[PATCH] radeon: Allow disabling UVD

[Parag Warudkar]

[-- Attachment #1: Type: text/plain, Size: 2406 bytes --]

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);

[-- Attachment #2: radeon-option-no-uvd.patch --]
[-- Type: application/octet-stream, Size: 2155 bytes --]

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);

Re: [PATCH] radeon: Allow disabling UVD

[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>

[...]

> @@ -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.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer

Re: [PATCH] radeon: Allow disabling UVD

[Christian König]

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.
>
>

Re: [PATCH] radeon: Allow disabling UVD

[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.

> 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

Re: [PATCH] radeon: Allow disabling UVD

[Christian König]

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
>

Re: [PATCH] radeon: Allow disabling UVD

[Parag Warudkar]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3144 bytes --]



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

Re: [PATCH] radeon: Allow disabling UVD

[Parag Warudkar]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 797 bytes --]



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