[Bug 66888] New: [radeonsi] Need support GL_EXT_framebuffer_multisample

2013-07-14 Thread bugzilla-daemon
https://bugs.freedesktop.org/show_bug.cgi?id=66888

  Priority: medium
Bug ID: 66888
  Assignee: dri-devel@lists.freedesktop.org
   Summary: [radeonsi] Need support GL_EXT_framebuffer_multisample
  Severity: enhancement
Classification: Unclassified
OS: Linux (All)
  Reporter: granti...@gmail.com
  Hardware: x86 (IA32)
Status: NEW
   Version: git
 Component: Drivers/Gallium/radeonsi
   Product: Mesa

Created attachment 82401
  --> https://bugs.freedesktop.org/attachment.cgi?id=82401&action=edit
graphical glitch

unigine-tropics and unigine-sanctuary don't runs. Required extension
GL_EXT_framebuffer_multisample is not supported.

ArchLinux x86; kernel 3.10; mesa - git; xf86-video-ati - git; llvm - 3.3

Radeon HD 7950

OpenGL renderer string: Gallium 0.4 on AMD TAHITI
OpenGL version string: 2.1 Mesa 9.2.0-devel (git-796b73d)
OpenGL shading language version string: 1.30

If run MESA_EXTENSION_OVERRIDE="GL_EXT_framebuffer_multisample"
unigine-tropics, test runs but have graphical glitch.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Bug 66888] [radeonsi] Need support GL_EXT_framebuffer_multisample

2013-07-14 Thread bugzilla-daemon
https://bugs.freedesktop.org/show_bug.cgi?id=66888

Vladimir Ysikov  changed:

   What|Removed |Added

  Attachment #82401|text/plain  |image/png
  mime type||

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Bug 66425] "failed testing IB on ring 5" when suspending to disk

2013-07-14 Thread bugzilla-daemon
https://bugs.freedesktop.org/show_bug.cgi?id=66425

--- Comment #24 from Austin Lund  ---
Patch tested and works on my machine.  I now have problems for "processors"
when doing pm_test, so I still cannot actually test this on a full resume from
disk, but at least pm_test with "devices" works.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Bug 66425] "failed testing IB on ring 5" when suspending to disk

2013-07-14 Thread bugzilla-daemon
https://bugs.freedesktop.org/show_bug.cgi?id=66425

Christian König  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED

--- Comment #25 from Christian König  ---
Sounds like we can merk this as resolved now.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Bug 66714] Mobility Radeon HD 5650 doesn't boot with kernel 3.10 (and newer) when using radeon.audio=1

2013-07-14 Thread bugzilla-daemon
https://bugs.freedesktop.org/show_bug.cgi?id=66714

Marco Trevisan (Treviño)  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED

--- Comment #4 from Marco Trevisan (Treviño)  ---
So, I've tried the patch on a 3.10 kernel that was affected and... Great, it
Works! :)
FYI, I've also added a couple of error logs in the code to see where exactly
the failure was and this was the result:

[2.073137] [drm] fb mappable at 0xC035F000
[2.073139] [drm] vram apper at 0xC000
[2.073140] [drm] size 8294400
[2.073141] [drm] fb depth is 24
[2.073142] [drm]pitch is 7680
[2.073252] fbcon: radeondrmfb (fb0) is primary device
[2.073323] [drm:evergreen_hdmi_enable] *ERROR* Invalid DIG (dig:
880229d0fc00, dig->afmt: (null))
/home/marco/Dev/debs/linux-3.10.0/drivers/gpu/drm/radeon/evergreen_hdmi.c:298
[2.573589] [drm:evergreen_hdmi_enable] *ERROR* Invalid DIG (dig:
880229d0fc00, dig->afmt: (null))
/home/marco/Dev/debs/linux-3.10.0/drivers/gpu/drm/radeon/evergreen_hdmi.c:298
[2.607449] Console: switching to colour frame buffer device 170x48
[2.610160] radeon :01:00.0: fb0: radeondrmfb frame buffer device
[2.610162] radeon :01:00.0: registered panic notifier
[2.610237] [drm] Initialized radeon 2.33.0 20080528 for :01:00.0 on
minor 0

Full dmesg at http://pastebin.ubuntu.com/5873667/
I don't know if this may be only the side effect of another issue (wrong
initialization order?).

One more thing I discovered in the past days, is that the kernel crash was not
happening in case that the HDMI was plugged after that ligthtdm was running
(attaching it anytime before was leading to a freeze).

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Bug 66714] Mobility Radeon HD 5650 doesn't boot with kernel 3.10 (and newer) when using radeon.audio=1

2013-07-14 Thread bugzilla-daemon
https://bugs.freedesktop.org/show_bug.cgi?id=66714

--- Comment #5 from Marco Trevisan (Treviño)  ---
Comment on attachment 82200
  --> https://bugs.freedesktop.org/attachment.cgi?id=82200
possible fix

Review of attachment 82200:
-

Why not including some DRM_ERROR logs as well?

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH] radeon kms: do not flush uninitialized hotplug work

2013-07-14 Thread Sergey Senozhatsky
Fix a warning from lockdep caused by calling flush_work() for
uninitialized hotplug work. Initialize hotplug_work, audio_work
and reset_work upon successful radeon_irq_kms_init() completion
and thus perform hotplug flush_work only when rdev->irq.installed
is true.

[4.790019] [drm] Loading CEDAR Microcode
[4.790943] r600_cp: Failed to load firmware "radeon/CEDAR_smc.bin"
[4.791152] [drm:evergreen_startup] *ERROR* Failed to load firmware!
[4.791330] radeon :01:00.0: disabling GPU acceleration

[4.792633] INFO: trying to register non-static key.
[4.792792] the code is fine but needs lockdep annotation.
[4.792953] turning off the locking correctness validator.

[4.793114] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 
3.11.0-rc0-dbg-10676-gfe56456-dirty #1816
[4.793314] Hardware name: Acer Aspire 5741G/Aspire 5741G
, BIOS V1.20 02/08/2011
[4.793507]  821fd810 8801530b9a18 8160434e 
0002
[4.794155]  8801530b9ad8 810b8404 8801530b0798 
8801530b
[4.794789]  8801530b9b00 0046 04c0 

[4.795418] Call Trace:
[4.795573]  [] dump_stack+0x4e/0x82
[4.795731]  [] __lock_acquire+0x1a64/0x1d30
[4.795893]  [] ? dev_vprintk_emit+0x50/0x60
[4.796034]  [] lock_acquire+0xa4/0x200
[4.796216]  [] ? flush_work+0x5/0x280
[4.796375]  [] flush_work+0x3d/0x280
[4.796520]  [] ? flush_work+0x5/0x280
[4.796682]  [] ? trace_hardirqs_on_caller+0xfd/0x1c0
[4.796862]  [] ? delay_tsc+0x95/0xf0
[4.797024]  [] radeon_irq_kms_fini+0x2b/0x70
[4.797186]  [] evergreen_init+0x2a9/0x2e0
[4.797347]  [] radeon_device_init+0x5ef/0x700
[4.797511]  [] ? pci_find_capability+0x47/0x50
[4.797672]  [] radeon_driver_load_kms+0x8d/0x150
[4.797843]  [] drm_get_pci_dev+0x166/0x280
[4.798007]  [] ? kfree+0xf5/0x2e0
[4.798168]  [] ? radeon_pci_probe+0x98/0xd0
[4.798329]  [] radeon_pci_probe+0xaa/0xd0
[4.798489]  [] pci_device_probe+0x84/0xe0
[4.798644]  [] driver_probe_device+0x76/0x240
[4.798805]  [] __driver_attach+0x93/0xa0
[4.798948]  [] ? __device_attach+0x40/0x40
[4.799126]  [] bus_for_each_dev+0x6b/0xb0
[4.799272]  [] driver_attach+0x1e/0x20
[4.799434]  [] bus_add_driver+0x1f0/0x280
[4.799596]  [] driver_register+0x74/0x150
[4.799758]  [] __pci_register_driver+0x5d/0x60
[4.799936]  [] ? ttm_init+0x67/0x67
[4.800081]  [] drm_pci_init+0x115/0x130
[4.800243]  [] ? ttm_init+0x67/0x67
[4.800405]  [] radeon_init+0x9c/0xba
[4.800586]  [] do_one_initcall+0xfa/0x150
[4.800746]  [] ? parse_args+0x120/0x330
[4.800909]  [] kernel_init_freeable+0x111/0x191
[4.801052]  [] ? do_early_param+0x88/0x88
[4.801233]  [] ? rest_init+0x140/0x140
[4.801393]  [] kernel_init+0xe/0x180
[4.801556]  [] ret_from_fork+0x7c/0xb0
[4.801718]  [] ? rest_init+0x140/0x140


Signed-off-by: Sergey Senozhatsky 

---

 drivers/gpu/drm/radeon/radeon_irq_kms.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c 
b/drivers/gpu/drm/radeon/radeon_irq_kms.c
index bcdefd1..081886b 100644
--- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
@@ -260,10 +260,6 @@ int radeon_irq_kms_init(struct radeon_device *rdev)
 {
int r = 0;
 
-   INIT_WORK(&rdev->hotplug_work, radeon_hotplug_work_func);
-   INIT_WORK(&rdev->audio_work, r600_audio_update_hdmi);
-   INIT_WORK(&rdev->reset_work, radeon_irq_reset_work_func);
-
spin_lock_init(&rdev->irq.lock);
r = drm_vblank_init(rdev->ddev, rdev->num_crtc);
if (r) {
@@ -285,6 +281,11 @@ int radeon_irq_kms_init(struct radeon_device *rdev)
rdev->irq.installed = false;
return r;
}
+
+   INIT_WORK(&rdev->hotplug_work, radeon_hotplug_work_func);
+   INIT_WORK(&rdev->audio_work, r600_audio_update_hdmi);
+   INIT_WORK(&rdev->reset_work, radeon_irq_reset_work_func);
+
DRM_INFO("radeon: irq initialized.\n");
return 0;
 }
@@ -304,8 +305,8 @@ void radeon_irq_kms_fini(struct radeon_device *rdev)
rdev->irq.installed = false;
if (rdev->msi_enabled)
pci_disable_msi(rdev->pdev);
+   flush_work(&rdev->hotplug_work);
}
-   flush_work(&rdev->hotplug_work);
 }
 
 /**

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Bug 63935] TURKS [drm:r600_uvd_init] *ERROR* UVD not responding, trying to reset the VCPU!!!

2013-07-14 Thread bugzilla-daemon
https://bugs.freedesktop.org/show_bug.cgi?id=63935

Christian König  changed:

   What|Removed |Added

 Status|REOPENED|RESOLVED
 Resolution|--- |FIXED

--- Comment #57 from Christian König  ---
The suspend/resume issue was something completely different, so we probably can
now close this bugreport.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Bug 66714] Mobility Radeon HD 5650 doesn't boot with kernel 3.10 (and newer) when using radeon.audio=1

2013-07-14 Thread bugzilla-daemon
https://bugs.freedesktop.org/show_bug.cgi?id=66714

Christian König  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED

--- Comment #6 from Christian König  ---
(In reply to comment #5)
> Comment on attachment 82200 [details] [review]
> possible fix
> 
> Review of attachment 82200 [details] [review]:
> -
> 
> Why not including some DRM_ERROR logs as well?

Because that isn't a bug.

You just have an LVDS pannel connected to a DIG encoder and so this encoder
doesn't have an audio block.

Alex patch is already quite right.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] radeon kms: do not flush uninitialized hotplug work

2013-07-14 Thread Alex Deucher
On Sun, Jul 14, 2013 at 7:03 AM, Sergey Senozhatsky
 wrote:
> Fix a warning from lockdep caused by calling flush_work() for
> uninitialized hotplug work. Initialize hotplug_work, audio_work
> and reset_work upon successful radeon_irq_kms_init() completion
> and thus perform hotplug flush_work only when rdev->irq.installed
> is true.
>
> [4.790019] [drm] Loading CEDAR Microcode
> [4.790943] r600_cp: Failed to load firmware "radeon/CEDAR_smc.bin"
> [4.791152] [drm:evergreen_startup] *ERROR* Failed to load firmware!
> [4.791330] radeon :01:00.0: disabling GPU acceleration
>
> [4.792633] INFO: trying to register non-static key.
> [4.792792] the code is fine but needs lockdep annotation.
> [4.792953] turning off the locking correctness validator.
>
> [4.793114] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 
> 3.11.0-rc0-dbg-10676-gfe56456-dirty #1816
> [4.793314] Hardware name: Acer Aspire 5741G/Aspire 5741G  
>   , BIOS V1.20 02/08/2011
> [4.793507]  821fd810 8801530b9a18 8160434e 
> 0002
> [4.794155]  8801530b9ad8 810b8404 8801530b0798 
> 8801530b
> [4.794789]  8801530b9b00 0046 04c0 
> 
> [4.795418] Call Trace:
> [4.795573]  [] dump_stack+0x4e/0x82
> [4.795731]  [] __lock_acquire+0x1a64/0x1d30
> [4.795893]  [] ? dev_vprintk_emit+0x50/0x60
> [4.796034]  [] lock_acquire+0xa4/0x200
> [4.796216]  [] ? flush_work+0x5/0x280
> [4.796375]  [] flush_work+0x3d/0x280
> [4.796520]  [] ? flush_work+0x5/0x280
> [4.796682]  [] ? trace_hardirqs_on_caller+0xfd/0x1c0
> [4.796862]  [] ? delay_tsc+0x95/0xf0
> [4.797024]  [] radeon_irq_kms_fini+0x2b/0x70
> [4.797186]  [] evergreen_init+0x2a9/0x2e0
> [4.797347]  [] radeon_device_init+0x5ef/0x700
> [4.797511]  [] ? pci_find_capability+0x47/0x50
> [4.797672]  [] radeon_driver_load_kms+0x8d/0x150
> [4.797843]  [] drm_get_pci_dev+0x166/0x280
> [4.798007]  [] ? kfree+0xf5/0x2e0
> [4.798168]  [] ? radeon_pci_probe+0x98/0xd0
> [4.798329]  [] radeon_pci_probe+0xaa/0xd0
> [4.798489]  [] pci_device_probe+0x84/0xe0
> [4.798644]  [] driver_probe_device+0x76/0x240
> [4.798805]  [] __driver_attach+0x93/0xa0
> [4.798948]  [] ? __device_attach+0x40/0x40
> [4.799126]  [] bus_for_each_dev+0x6b/0xb0
> [4.799272]  [] driver_attach+0x1e/0x20
> [4.799434]  [] bus_add_driver+0x1f0/0x280
> [4.799596]  [] driver_register+0x74/0x150
> [4.799758]  [] __pci_register_driver+0x5d/0x60
> [4.799936]  [] ? ttm_init+0x67/0x67
> [4.800081]  [] drm_pci_init+0x115/0x130
> [4.800243]  [] ? ttm_init+0x67/0x67
> [4.800405]  [] radeon_init+0x9c/0xba
> [4.800586]  [] do_one_initcall+0xfa/0x150
> [4.800746]  [] ? parse_args+0x120/0x330
> [4.800909]  [] kernel_init_freeable+0x111/0x191
> [4.801052]  [] ? do_early_param+0x88/0x88
> [4.801233]  [] ? rest_init+0x140/0x140
> [4.801393]  [] kernel_init+0xe/0x180
> [4.801556]  [] ret_from_fork+0x7c/0xb0
> [4.801718]  [] ? rest_init+0x140/0x140
>
>
> Signed-off-by: Sergey Senozhatsky 
>

Applied to by fixes tree.  thanks!

Alex

> ---
>
>  drivers/gpu/drm/radeon/radeon_irq_kms.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c 
> b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> index bcdefd1..081886b 100644
> --- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> @@ -260,10 +260,6 @@ int radeon_irq_kms_init(struct radeon_device *rdev)
>  {
> int r = 0;
>
> -   INIT_WORK(&rdev->hotplug_work, radeon_hotplug_work_func);
> -   INIT_WORK(&rdev->audio_work, r600_audio_update_hdmi);
> -   INIT_WORK(&rdev->reset_work, radeon_irq_reset_work_func);
> -
> spin_lock_init(&rdev->irq.lock);
> r = drm_vblank_init(rdev->ddev, rdev->num_crtc);
> if (r) {
> @@ -285,6 +281,11 @@ int radeon_irq_kms_init(struct radeon_device *rdev)
> rdev->irq.installed = false;
> return r;
> }
> +
> +   INIT_WORK(&rdev->hotplug_work, radeon_hotplug_work_func);
> +   INIT_WORK(&rdev->audio_work, r600_audio_update_hdmi);
> +   INIT_WORK(&rdev->reset_work, radeon_irq_reset_work_func);
> +
> DRM_INFO("radeon: irq initialized.\n");
> return 0;
>  }
> @@ -304,8 +305,8 @@ void radeon_irq_kms_fini(struct radeon_device *rdev)
> rdev->irq.installed = false;
> if (rdev->msi_enabled)
> pci_disable_msi(rdev->pdev);
> +   flush_work(&rdev->hotplug_work);
> }
> -   flush_work(&rdev->hotplug_work);
>  }
>
>  /**
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
__

[PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume

2013-07-14 Thread Konstantin Khlebnikov
This patch fixes regression in power consumtion of sandy bridge gpu, which
exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that
it's extremely busy. After that it never reaches rc6 state.

Bug was introduce by commit b4ae3f22d238617ca11610b29fde16cf8c0bc6e0
("drm/i915: load boot context at driver init time"). Without documentation
it's not clear what is happening here, probably this breaks internal state of
hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake
during whole initialization sequence in gen6_init_clock_gating() fixes this bug.

References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
Signed-off-by: Konstantin Khlebnikov 
---
 drivers/gpu/drm/i915/intel_pm.c |4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index aa01128..839a43f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3640,6 +3640,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)
int pipe;
uint32_t dspclk_gate = ILK_VRHUNIT_CLOCK_GATE_DISABLE;
 
+   gen6_gt_force_wake_get(dev_priv);
+
I915_WRITE(ILK_DSPCLK_GATE_D, dspclk_gate);
 
I915_WRITE(ILK_DISPLAY_CHICKEN2,
@@ -3728,6 +3730,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)
cpt_init_clock_gating(dev);
 
gen6_check_mch_setup(dev);
+
+   gen6_gt_force_wake_put(dev_priv);
 }
 
 static void gen7_setup_fixed_func_scheduler(struct drm_i915_private *dev_priv)

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume

2013-07-14 Thread Daniel Vetter
On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov
 wrote:
> This patch fixes regression in power consumtion of sandy bridge gpu, which
> exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that
> it's extremely busy. After that it never reaches rc6 state.
>
> Bug was introduce by commit b4ae3f22d238617ca11610b29fde16cf8c0bc6e0
> ("drm/i915: load boot context at driver init time"). Without documentation
> it's not clear what is happening here, probably this breaks internal state of
> hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake
> during whole initialization sequence in gen6_init_clock_gating() fixes this 
> bug.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
> Signed-off-by: Konstantin Khlebnikov 

We already hold an forcewake reference while setting up the rps stuff,
should we maybe hold the forcewake for the entire duration, i.e. grab
it here in clock_gating and release it only in gen6/vlv_enable_rps?
Can you please test that version, too?

In any case the forcewake grabbing here in the clock gating function
needs a big comment that otherwise setting the MCTL register might
break rc6 entry.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_pm.c |4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index aa01128..839a43f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3640,6 +3640,8 @@ static void gen6_init_clock_gating(struct drm_device 
> *dev)
> int pipe;
> uint32_t dspclk_gate = ILK_VRHUNIT_CLOCK_GATE_DISABLE;
>
> +   gen6_gt_force_wake_get(dev_priv);
> +
> I915_WRITE(ILK_DSPCLK_GATE_D, dspclk_gate);
>
> I915_WRITE(ILK_DISPLAY_CHICKEN2,
> @@ -3728,6 +3730,8 @@ static void gen6_init_clock_gating(struct drm_device 
> *dev)
> cpt_init_clock_gating(dev);
>
> gen6_check_mch_setup(dev);
> +
> +   gen6_gt_force_wake_put(dev_priv);
>  }
>
>  static void gen7_setup_fixed_func_scheduler(struct drm_i915_private 
> *dev_priv)
>



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Bug 64850] Second screen black on Pitcairn PRO

2013-07-14 Thread bugzilla-daemon
https://bugs.freedesktop.org/show_bug.cgi?id=64850

--- Comment #23 from Jakob Nixdorf  ---
Just tested the first kernel with RadeonSI support (3.4.0 to my knowledge) with
the same result.
Second screen stays black in TTY and in Xorg.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume

2013-07-14 Thread Konstantin Khlebnikov

Daniel Vetter wrote:

On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov
  wrote:

This patch fixes regression in power consumtion of sandy bridge gpu, which
exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that
it's extremely busy. After that it never reaches rc6 state.

Bug was introduce by commit b4ae3f22d238617ca11610b29fde16cf8c0bc6e0
("drm/i915: load boot context at driver init time"). Without documentation
it's not clear what is happening here, probably this breaks internal state of
hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake
during whole initialization sequence in gen6_init_clock_gating() fixes this bug.

References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
Signed-off-by: Konstantin Khlebnikov


We already hold an forcewake reference while setting up the rps stuff,
should we maybe hold the forcewake for the entire duration, i.e. grab
it here in clock_gating and release it only in gen6/vlv_enable_rps?
Can you please test that version, too?


This will be racy because rps stuff is done in separate work which might be 
canceled
if intel_disable_gt_powersave() happens before its completion.



In any case the forcewake grabbing here in the clock gating function
needs a big comment that otherwise setting the MCTL register might
break rc6 entry.
-Daniel


---
  drivers/gpu/drm/i915/intel_pm.c |4 
  1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index aa01128..839a43f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3640,6 +3640,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)
 int pipe;
 uint32_t dspclk_gate = ILK_VRHUNIT_CLOCK_GATE_DISABLE;

+   gen6_gt_force_wake_get(dev_priv);
+
 I915_WRITE(ILK_DSPCLK_GATE_D, dspclk_gate);

 I915_WRITE(ILK_DISPLAY_CHICKEN2,
@@ -3728,6 +3730,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)
 cpt_init_clock_gating(dev);

 gen6_check_mch_setup(dev);
+
+   gen6_gt_force_wake_put(dev_priv);
  }

  static void gen7_setup_fixed_func_scheduler(struct drm_i915_private *dev_priv)





--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume

2013-07-14 Thread Chris Wilson
On Sun, Jul 14, 2013 at 06:52:39PM +0200, Daniel Vetter wrote:
> On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov
>  wrote:
> > This patch fixes regression in power consumtion of sandy bridge gpu, which
> > exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking 
> > that
> > it's extremely busy. After that it never reaches rc6 state.
> >
> > Bug was introduce by commit b4ae3f22d238617ca11610b29fde16cf8c0bc6e0
> > ("drm/i915: load boot context at driver init time"). Without documentation
> > it's not clear what is happening here, probably this breaks internal state 
> > of
> > hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake
> > during whole initialization sequence in gen6_init_clock_gating() fixes this 
> > bug.
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
> > Signed-off-by: Konstantin Khlebnikov 
> 
> We already hold an forcewake reference while setting up the rps stuff,
> should we maybe hold the forcewake for the entire duration, i.e. grab
> it here in clock_gating and release it only in gen6/vlv_enable_rps?
> Can you please test that version, too?
> 
> In any case the forcewake grabbing here in the clock gating function
> needs a big comment that otherwise setting the MCTL register might
> break rc6 entry.

It is not clear why the forcewake works, but is easy to imagine one of
the operations in that sequence requires the GPU to be awake at the time
of programming for it to succeed. MBCTL:EnableBootFetch does seem the most
suspicious from its wording in the bspec. I guess all instances of
poking this bit should be protected similary (snb, ivb, vlv, hsw).

Based on that reasoning and that waking the GPU up here has no negative
consequences, and so long as all paths are fixed, I am happy to give this
an Acked-by: Chris Wilson 

Also, we need to reapply the w/a after a Function Level Reset, in other
words we do need to repeat the init_clock_gating after
intel_gpu_reset().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: DT binding review for Armada display subsystem

2013-07-14 Thread Russell King - ARM Linux
On Sat, Jul 13, 2013 at 08:25:15AM -0600, Daniel Drake wrote:
> I guess the IRE falls into the same category as the DCON - we won't
> implement it for now, but knowing where it might fit in is useful.

I don't see much need at the moment for IRE.  IRE isn't going to be
useful for general graphics rotation as it only supports up to 16bpp
RGB.

It looks to me like this module was designed to rotate YUV/camera
images.  If we wanted to rotate a video image, then it would probably
be more efficient to use this, but it will cause a conversion of the
image format in doing so.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: DT binding review for Armada display subsystem

2013-07-14 Thread Sebastian Hesselbarth

On 07/13/2013 04:25 PM, Daniel Drake wrote:

On Sat, Jul 13, 2013 at 2:35 AM, Jean-Francois Moine  wrote:

I use my Cubox for daily jobs as a desktop computer. My kernel is a DT
driven 3.10.0. The dove-drm, tda998x and si5351 (clock) are kernel
modules. I set 3 clocks in the DT for the LCD0: lcdclk, axi and extclk0
(si5351). Normally, the external clock is used, but, sometimes, the
si5351 module is not yet initialized when the drm driver starts. So,
for 1920x1080p, it uses the lcdclk which sets the LCD clock to 13
(40/3) instead of 148500. As a result, display and sound still work
correctly on my TV set thru HDMI.

So, it would be nice to have 2 usable clocks per LCD, until we find a
good way to initialize the modules in the right order at startup time.


Having multiple usable clocks is implemented in the patch I referred
you to. However it doesn't solve the "better clock turns up after
initializing the driver" problem, which seems like a tricky one.

Maybe the best solution is to defer probe until all DT-defined clocks
become available. That means that whoever compiles the kernel must
take care to not forget to build the clock drivers for all the clocks
referenced in this part of the DT, but perhaps that is a reasonable
thing to require.

On the other hand, this problem acts in support of a simpler approach
mentioned by Sebastian: developers figure out what the best clock is
for each CRTC on each board, and the DT only specifies that one clock.
The driver uses that clock if it is available and defers probe if it
is not.


Daniel,

sorry for the late response, I just came back from a boat trip around
Capri :D

About the clocks and deferred probing, the issue here is that you might
have trouble to find out if that clock will ever come up. Therefore, I
suggested the easiest heuristic which is "let the DT author decide".

I am fine with allowing more than one clock from DT and get or wait for
all/one clock.


Back to the specific case of the Cubox with new ideas.

The Cubox is based on the Armada 510 (Dove), so, all the hardware must
be described in dove.dtsi. This includes the LCDs and DCON/IRE, the
possible clocks and the (static) v4l2 links:


As a sidenote, I think we have concluded that we are not going to
interact with the armada 510 DCON in any way at the moment. The driver
will not have code that touches it, and the DT will not mention it.
The first person who actually needs to use the DCON will have the
responsibility for doing that work. Nevertheless it makes sense for us
to pick a DT design where we know that the DCON could be slotted in
later.


True, do not link dcon node with any of lcd, tda998x or anything else.
If there is support for it in the driver we just use
of_find_compatible_node and let the driver do the magic. From a physical
POV, lcd0 is directly connected to tda998x through dumb RGB pins. DCON
can control data flow to those pins but should not be DT linked.


 lcd0: lcd-controller@82 {
 compatible = "marvell,dove-lcd0";
 reg = <0x82 0x1c8>;
 interrupts = <47>;
 clocks = <&core_clk 3>, <&lcdclk>;
 clock-names = "axi", "lcdclk";


About clock names for Armada 510 LCD: I suggest "axiclk", "lcdpll", 
"extclk0", "extclk1". IIRC axiclk is 333MHz (or maybe 2xTCLK),

lcdpll can be derived from 2GHz core PLL with min integer divider
of 5. extclk0/1 are dedicated pins and can be used from both lcd0
and lcd1.


 marvell-output = <&dcon 0>;
 status = "disabled";
 };

 lcd1: lcd-controller@81 {
 compatible = "marvell,dove-lcd1";
 reg = <0x81 0x1c8>;
 interrupts = <46>;
 clocks = <&core_clk 3>, <&lcdclk>;
 clock-names = "axi", "lcdclk";
 marvell-output = <&dcon 1>;
 status = "disabled";
 };

 /* display controller and image rotation engine */
 dcon: display-controller@83 {
 compatible = "marvell,dove-dcon";
 reg = <0x83 0xc4>,  /* DCON */
   <0x831000 0x8c>;  /* IRE */
 interrupts = <45>;
 marvell-input = <&lcd0>, <&lcd1>;
 status = "disabled";
 };


I guess the IRE falls into the same category as the DCON - we won't
implement it for now, but knowing where it might fit in is useful.

Why would you put it in the same node as the DCON though? It has its
own separate register space and additionally it is a component shared
with the MMP boards (whereas the DCON is not).


DCON and IRE are summarized in the same register description in Dove FS.
Maybe we can split them up and have two nodes or just one if registers
overlap. Anyway, not that important for now.


The specific Cubox hardware (tda998x and si5351) is described in
dove-cubox.dts, with the new clocks and 

Re: DT binding review for Armada display subsystem

2013-07-14 Thread Sebastian Hesselbarth

On 07/13/2013 01:12 PM, Russell King - ARM Linux wrote:

On Sat, Jul 13, 2013 at 12:56:50PM +0200, Sylwester Nawrocki wrote:

On 07/13/2013 10:35 AM, Jean-Francois Moine wrote:

On Fri, 12 Jul 2013 13:00:23 -0600 Daniel Drake  wrote:

On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois Moine  wrote:



- the phandles to the clocks does not tell how the clock may be set by
the driver (it is an array index in the 510).


In the other threads on clock selection, we decided that that exact
information on how to select a clock (i.e. register bits/values) must
be in the driver, not in the DT. What was suggested there is a
well-documented scheme for clock naming, so the driver knows which
clock is which. That is defined in the proposed DT binding though the
"valid clock names" part. For an example driver implementation of this
you can see my patch "armada_drm clock selection - try 2".


OK.

Sorry to go back to this thread.

I use my Cubox for daily jobs as a desktop computer. My kernel is a DT
driven 3.10.0. The dove-drm, tda998x and si5351 (clock) are kernel
modules. I set 3 clocks in the DT for the LCD0: lcdclk, axi and extclk0


Hmm, a bit off topic question, does it work when the si5351 module gets
removed ? I can't see anything preventing clock provider module from being
removed why any of its clocks is used and clk_unregister() function is
currently unimplemented.


When I designed the clk API, I arranged for things like clk_get() to
take a reference on the module if the clock was supplied by a module.
You'll see some evidence of that by looking back in the git history
for arch/arm/mach-integrator/include/mach/clkdev.h.


Last time I checked, clkdev API neither implements referencing nor
unregister. This is on Mike's list and IIRC there already has been
a proposal for unregister. Si5351 was the first clkdev driver ever
that could possibly be unloaded, so there may be still some features
missing.


If the common clk API has been designed without any thought to clocks
supplied by modules and making sure that in-use clocks don't go away,
then it's going to be a real pain to sort that out.  I don't think
refcounting clocks makes sense (what do you do with an enabled clock
that you then remove the module for but it's still being used?  Do you
just shut it down anyway?  Do you leave it running?  What about
subsequent calls?).


Good point, first I thought always disable on last user dropping it but
that may most likely break some platforms. Second thought, get back to
the state when the driver was loaded.


I think this is one case where taking a reference on the module supplying
it makes total sense.


(si5351). Normally, the external clock is used, but, sometimes, the
si5351 module is not yet initialized when the drm driver starts. So,
for 1920x1080p, it uses the lcdclk which sets the LCD clock to 13
(40/3) instead of 148500. As a result, display and sound still work
correctly on my TV set thru HDMI.

So, it would be nice to have 2 usable clocks per LCD, until we find a
good way to initialize the modules in the right order at startup time.


Doesn't deferred probing help here ?


Indeed it does.  Just because one TV works with such a wrong clock does not
mean that all TVs and HDMI compatible monitors will do.  It's 11% out.

The reason that audio works is because of the way the HDMI transmitter works
- it can calculate the CTS value to send (by measuring it) and it sends that
value over the link so the TV can regenerate the correct audio clock from
the HDMI clock.


True, wrong clock will most likely not work on all monitors or TV. My
impression for TVs is that the the "cheaper" the brand is, the more
likely they accept any mode/clock combination ;)


Whether that's going to be universally true, I don't know - it depends on
how much audio data gets sent via each frame.  As the HDMI clock is slower,
there would need to be more audio data sent.


Also: audio/video clock relation is sent in AVI packets over HDMI.
Picking a clock with 11% off may not even allow you to send (valid) CTS
information.


lcd0: lcd-controller@82 {
compatible = "marvell,dove-lcd0";

[...]

};

lcd1: lcd-controller@81 {
compatible = "marvell,dove-lcd1";

[...]

};


Using different compatible strings to indicate multiple instances of same
hardware doesn't seem right. Unless LCD0, LCD1 are really different pieces


They aren't.  They're 100% identical in the Armada 510.


of hardware incompatible with each other I think it would be more correct
to use same compatible property and DT node aliases, similarly as is now
done with e.g. I2C busses:

aliases {
lcd0 = &lcd_0;  
lcd1 = &lcd_1;  
};

lcd_0: lcd-controller@82 {
compatible = "marvell,dove-lcd";


I'd much prefer marvell,armada-510-lcd rather than using the codenames for
the devices.  Otherwise we're going to run into total

Re: DT binding review for Armada display subsystem

2013-07-14 Thread Sylwester Nawrocki

On 07/13/2013 07:44 PM, Sebastian Hesselbarth wrote:

On 07/13/2013 01:12 PM, Russell King - ARM Linux wrote:

On Sat, Jul 13, 2013 at 12:56:50PM +0200, Sylwester Nawrocki wrote:

On 07/13/2013 10:35 AM, Jean-Francois Moine wrote:

On Fri, 12 Jul 2013 13:00:23 -0600 Daniel Drake wrote:

On Fri, Jul 12, 2013 at 12:39 PM, Jean-Francois
Moine wrote:

[...]

I use my Cubox for daily jobs as a desktop computer. My kernel is a DT
driven 3.10.0. The dove-drm, tda998x and si5351 (clock) are kernel
modules. I set 3 clocks in the DT for the LCD0: lcdclk, axi and extclk0


Hmm, a bit off topic question, does it work when the si5351 module gets
removed ? I can't see anything preventing clock provider module from
being
removed why any of its clocks is used and clk_unregister() function is
currently unimplemented.


When I designed the clk API, I arranged for things like clk_get() to
take a reference on the module if the clock was supplied by a module.
You'll see some evidence of that by looking back in the git history
for arch/arm/mach-integrator/include/mach/clkdev.h.


Last time I checked, clkdev API neither implements referencing nor
unregister. This is on Mike's list and IIRC there already has been
a proposal for unregister. Si5351 was the first clkdev driver ever


I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
they're working on v4 with clock object reference counting. Presumably we
need both clk_get() to be taking reference on the module and reference
counted clk free, e.g. in cases where clock provider is a hot-pluggable
device. It might be too paranoid though, I haven't seen hardware
configurations where a clock source could be unplugged safely when whole
system is running.


that could possibly be unloaded, so there may be still some features
missing.


I guess there should be a warning in e.g. Kconfig saying that it is safe
to use this driver only with CONFIG_MODULE_UNLOAD or something similar.
Otherwise users might not be happy and might start filling bug reports. :)


If the common clk API has been designed without any thought to clocks
supplied by modules and making sure that in-use clocks don't go away,
then it's going to be a real pain to sort that out. I don't think
refcounting clocks makes sense (what do you do with an enabled clock
that you then remove the module for but it's still being used? Do you
just shut it down anyway? Do you leave it running? What about
subsequent calls?).


Good point, first I thought always disable on last user dropping it but
that may most likely break some platforms. Second thought, get back to
the state when the driver was loaded.


When last user drops the reference it could be already too late to change
anything in hardware, since the clock provider module is already unloaded.

Anyway I suppose it's best to take reference on the clk provider module
in clk_get(). This can easily lead to circular dependencies though, when
e.g. a module A is a clock consumer of a clock provided by module B, and
module B calls back into module A, so it also takes reference on A.
The only solution I could think of is to call clk_get() only before a clock
is enabled and clk_put() right after it is disabled and stops being used.


I think this is one case where taking a reference on the module supplying
it makes total sense.


(si5351). Normally, the external clock is used, but, sometimes, the
si5351 module is not yet initialized when the drm driver starts. So,
for 1920x1080p, it uses the lcdclk which sets the LCD clock to 13
(40/3) instead of 148500. As a result, display and sound still work
correctly on my TV set thru HDMI.

So, it would be nice to have 2 usable clocks per LCD, until we find a
good way to initialize the modules in the right order at startup time.


Doesn't deferred probing help here ?


Indeed it does. Just because one TV works with such a wrong clock does
not
mean that all TVs and HDMI compatible monitors will do. It's 11% out.

The reason that audio works is because of the way the HDMI transmitter
works
- it can calculate the CTS value to send (by measuring it) and it
sends that
value over the link so the TV can regenerate the correct audio clock from
the HDMI clock.


True, wrong clock will most likely not work on all monitors or TV. My
impression for TVs is that the the "cheaper" the brand is, the more
likely they accept any mode/clock combination ;)


Whether that's going to be universally true, I don't know - it depends on
how much audio data gets sent via each frame. As the HDMI clock is
slower,
there would need to be more audio data sent.


Also: audio/video clock relation is sent in AVI packets over HDMI.
Picking a clock with 11% off may not even allow you to send (valid) CTS
information.


lcd0: lcd-controller@82 {
compatible = "marvell,dove-lcd0";

[...]

};

lcd1: lcd-controller@81 {
compatible = "marvell,dove-lcd1";

[...]

};


Using different compatible strings to indicate multiple instances of
same
hardware doesn't

Re: DT binding review for Armada display subsystem

2013-07-14 Thread Russell King - ARM Linux
On Sat, Jul 13, 2013 at 07:44:58PM +0200, Sebastian Hesselbarth wrote:
> On 07/13/2013 01:12 PM, Russell King - ARM Linux wrote:
>> When I designed the clk API, I arranged for things like clk_get() to
>> take a reference on the module if the clock was supplied by a module.
>> You'll see some evidence of that by looking back in the git history
>> for arch/arm/mach-integrator/include/mach/clkdev.h.
>
> Last time I checked, clkdev API neither implements referencing nor
> unregister.

*Sigh*.

a613163dff04cbfcb7d66b06ef4a5f65498ee59b:
arch/arm/mach-integrator/include/mach/clkdev.h:
-struct clk {
-   unsigned long   rate;
-   const struct clk_ops*ops;
-   struct module   *owner;
-   const struct icst_params *params;
-   void __iomem*vcoreg;
-   void*data;
-};
-
-static inline int __clk_get(struct clk *clk)
-{
-   return try_module_get(clk->owner);
-}
-
-static inline void __clk_put(struct clk *clk)
-{
-   module_put(clk->owner);
-}

70ee65771424829fd092a1df9afcc7e24c94004b:
arch/arm/mach-integrator/impd1.c:
 static int impd1_probe(struct lm_device *dev)
...
-   for (i = 0; i < ARRAY_SIZE(impd1->vcos); i++) {
-   impd1->vcos[i].ops = &impd1_clk_ops,
-   impd1->vcos[i].owner = THIS_MODULE,
-   impd1->vcos[i].params = &impd1_vco_params,
-   impd1->vcos[i].data = impd1;
-   }
-   impd1->vcos[0].vcoreg = impd1->base + IMPD1_OSC1;
-   impd1->vcos[1].vcoreg = impd1->base + IMPD1_OSC2;
-
-   impd1->clks[0] = clkdev_alloc(&impd1->vcos[0], NULL, "lm%x:01000",
-   dev->id);
-   impd1->clks[1] = clkdev_alloc(&fixed_14745600, NULL, "lm%x:00100",
-   dev->id);
-   impd1->clks[2] = clkdev_alloc(&fixed_14745600, NULL, "lm%x:00200",
-   dev->id);
-   for (i = 0; i < ARRAY_SIZE(impd1->clks); i++)
-   clkdev_add(impd1->clks[i]);
...
 static void impd1_remove(struct lm_device *dev)
...
-   for (i = 0; i < ARRAY_SIZE(impd1->clks); i++)
-   clkdev_drop(impd1->clks[i]);

drivers/clk/clkdev.c:
/*
 * clkdev_drop - remove a clock dynamically allocated
 */
void clkdev_drop(struct clk_lookup *cl)
{
mutex_lock(&clocks_mutex);
list_del(&cl->node);
mutex_unlock(&clocks_mutex);
kfree(cl);
}
EXPORT_SYMBOL(clkdev_drop);

No, of course, I'm imagining all the above!

Now, today, the IM-PD/1 support got broken in respect of ensuring that
things are properly refcounted in the rush to convert things to this
wonderful new common clk API - because it's oh soo much better.  Yes,
right, I'd forgotten the number one rule of kernel programming - always
sacrifice correctness when we get a new fantasic hip idea!  Silly me.

> This is on Mike's list and IIRC there already has been
> a proposal for unregister. Si5351 was the first clkdev driver ever
> that could possibly be unloaded, so there may be still some features
> missing.

Utter rubbish - it's not the first as you can see from the above.
Integrator had this support since the clk and clkdev APIs came along,
because the IM-PD/1 module was implemented as a module, and it has to
create and register clocks for its peripherals.

What you've found is a backwards step with the common clk API, nothing
more.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: DT binding review for Armada display subsystem

2013-07-14 Thread Russell King - ARM Linux
On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
> I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
> they're working on v4 with clock object reference counting. Presumably we
> need both clk_get() to be taking reference on the module and reference
> counted clk free, e.g. in cases where clock provider is a hot-pluggable
> device. It might be too paranoid though, I haven't seen hardware
> configurations where a clock source could be unplugged safely when whole
> system is running.

I'm not going to accept refcounting being thrown into clk_get().  The
clkdev API already has refcounting, as much as it needs to.  It just
needs people to use the hooks that I provided back in 2008 when I
created the clkdev API for doing _precisely_ this job.

Have a read through these commits, which backup my statement above:

0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the clkdev API
d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting Integrator to clkdev API

and it will show you how to do refcounting.  The common clk API just
needs to stop defining __clk_get() and __clk_put() to be an empty
function and implement them appropriately for it's clk implementation,
like they were always meant to be.

__clk_get() and __clk_put() are the clk-implementation specific parts
of the clkdev API, because the clkdev API is utterly divorsed from the
internals of what a 'struct clk' actually is.  clkdev just treats a
'struct clk' as a completely opaque type and never bothers poking
about inside it.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: DT binding review for Armada display subsystem

2013-07-14 Thread Sylwester Nawrocki

On 07/13/2013 11:02 PM, Russell King - ARM Linux wrote:

On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:

I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
they're working on v4 with clock object reference counting. Presumably we
need both clk_get() to be taking reference on the module and reference
counted clk free, e.g. in cases where clock provider is a hot-pluggable
device. It might be too paranoid though, I haven't seen hardware
configurations where a clock source could be unplugged safely when whole
system is running.


I'm not going to accept refcounting being thrown into clk_get().  The
clkdev API already has refcounting, as much as it needs to.  It just
needs people to use the hooks that I provided back in 2008 when I
created the clkdev API for doing _precisely_ this job.

Have a read through these commits, which backup my statement above:

0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the clkdev API
d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting Integrator to clkdev API

and it will show you how to do refcounting.  The common clk API just
needs to stop defining __clk_get() and __clk_put() to be an empty
function and implement them appropriately for it's clk implementation,
like they were always meant to be.


Sure, I've already seen the above commits. This is how I understood it
as well - __clk_get(), __clk_put() need to be defined by the common clk
API, since it is going to replace all the arch specific implementations.


__clk_get() and __clk_put() are the clk-implementation specific parts
of the clkdev API, because the clkdev API is utterly divorsed from the
internals of what a 'struct clk' actually is.  clkdev just treats a
'struct clk' as a completely opaque type and never bothers poking
about inside it.


OK, but at the clock's implementation side we may need, e.g. to use kref
to keep track of the clock's state, and free it only when it is unprepared,
all its children are unregistered, etc. ? Is it not what you stated in
your comment to patch [1] ?

[1] https://patchwork.kernel.org/patch/2651171/
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: DT binding review for Armada display subsystem

2013-07-14 Thread Russell King - ARM Linux
On Sun, Jul 14, 2013 at 12:16:58AM +0200, Sylwester Nawrocki wrote:
> On 07/13/2013 11:02 PM, Russell King - ARM Linux wrote:
>> On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
>>> I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
>>> they're working on v4 with clock object reference counting. Presumably we
>>> need both clk_get() to be taking reference on the module and reference
>>> counted clk free, e.g. in cases where clock provider is a hot-pluggable
>>> device. It might be too paranoid though, I haven't seen hardware
>>> configurations where a clock source could be unplugged safely when whole
>>> system is running.
>>
>> I'm not going to accept refcounting being thrown into clk_get().  The
>> clkdev API already has refcounting, as much as it needs to.  It just
>> needs people to use the hooks that I provided back in 2008 when I
>> created the clkdev API for doing _precisely_ this job.
>>
>> Have a read through these commits, which backup my statement above:
>>
>> 0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the clkdev API
>> d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting Integrator to clkdev 
>> API
>>
>> and it will show you how to do refcounting.  The common clk API just
>> needs to stop defining __clk_get() and __clk_put() to be an empty
>> function and implement them appropriately for it's clk implementation,
>> like they were always meant to be.
>
> Sure, I've already seen the above commits. This is how I understood it
> as well - __clk_get(), __clk_put() need to be defined by the common clk
> API, since it is going to replace all the arch specific implementations.
>
>> __clk_get() and __clk_put() are the clk-implementation specific parts
>> of the clkdev API, because the clkdev API is utterly divorsed from the
>> internals of what a 'struct clk' actually is.  clkdev just treats a
>> 'struct clk' as a completely opaque type and never bothers poking
>> about inside it.
>
> OK, but at the clock's implementation side we may need, e.g. to use kref
> to keep track of the clock's state, and free it only when it is unprepared,
> all its children are unregistered, etc. ? Is it not what you stated in
> your comment to patch [1] ?

If you want to do refcounting on a clock (which you run into problems
as I highlighted earlier in this thread) then yes, you need to use a
kref, and take a reference in __clk_get() and drop it in __clk_put()
to make things safe.

Whether you also take a reference on the module supplying the clock or
not depends whether you need to keep the code around to manipulate that
clock while there are users of it.

As I've already said - consider the possibilities with this scenario.
Here's one:

  A clock consumer is using a clock, but the clock supplier has been
  removed.  The clock consumer wants to change the state of the clock
  in some way - eg, system is suspending.  clk_disable() doesn't fail,
  but on resume, clk_enable() does... (and how many people assume that
  clk_enable() never fails?)  And who knows what rate the clock is now
  producing or whether it's even producing anything...

I'm not saying don't do the refcounting thing I mentioned back in June.
I'm merely pointing out the issues that there are.  There isn't a one
right answer here because each has their own advantages and disadvantages
(and problems.)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Bug 59322] r300g MSAA breaks Half-Life 2 in Wine

2013-07-14 Thread bugzilla-daemon
https://bugs.freedesktop.org/show_bug.cgi?id=59322

--- Comment #8 from Marek Olšák  ---
I have found the problem. Wine is trying to use the GL_SRGB8_ALPHA8 format,
which is not supported for rendering by the driver internally, but I agree the
format should be allowed in the API. The fix is on the way.

Is it a big deal that ARB_framebuffer_sRGB is unsupported? In such a case, the
renderbuffer format GL_SRGB8_ALPHA8 is equivalent to GL_RGBA8.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [Update][PATCH] ACPI / video / i915: Remove ACPI backlight if firmware expects Windows 8

2013-07-14 Thread Aaron Lu
On 07/13/2013 08:46 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> According to Matthew Garrett, "Windows 8 leaves backlight control up
> to individual graphics drivers rather than making ACPI calls itself.
> There's plenty of evidence to suggest that the Intel driver for
> Windows [8] doesn't use the ACPI interface, including the fact that
> it's broken on a bunch of machines when the OS claims to support
> Windows 8.  The simplest thing to do appears to be to disable the
> ACPI backlight interface on these systems".
> 
> There's a problem with that approach, however, because simply
> avoiding to register the ACPI backlight interface if the firmware
> calls _OSI for Windows 8 may not work in the following situations:
>  (1) The ACPI backlight interface actually works on the given system
>  and the i915 driver is not loaded (e.g. another graphics driver
>  is used).
>  (2) The ACPI backlight interface doesn't work on the given system,
>  but there is a vendor platform driver that will register its
>  own, equally broken, backlight interface if not prevented from
>  doing so by the ACPI subsystem.
> Therefore we need to allow the ACPI backlight interface to be
> registered until the i915 driver is loaded which then will unregister
> it if the firmware has called _OSI for Windows 8 (or will register
> the ACPI video driver without backlight support if not already
> present).
> 
> For this reason, introduce an alternative function for registering
> ACPI video, acpi_video_register_with_quirks(), that will check
> whether or not the ACPI video driver has already been registered
> and whether or not the backlight Windows 8 quirk has to be applied.
> If the quirk has to be applied, it will block the ACPI backlight
> support and either unregister the backlight interface if the ACPI
> video driver has already been registered, or register the ACPI
> video driver without the backlight interface otherwise.  Make
> the i915 driver use acpi_video_register_with_quirks() instead of
> acpi_video_register() in i915_driver_load().
> 
> This change is based on earlier patches from Matthew Garrett,
> Chun-Yi Lee and Seth Forshee and Aaron Lu's comments.
> 
> Signed-off-by: Rafael J. Wysocki 

Reviewed-by: Aaron Lu 

BTW, I also tested on a Toshiba laptop Z830 where its AML code
claims support of win8, the result is as expected: ACPI video
interface is removed, i915 Xorg driver picks intel_backlight.

Thanks for the fix.

-Aaron

> ---
>  drivers/acpi/internal.h |   11 ++
>  drivers/acpi/video.c|   65 
> 
>  drivers/acpi/video_detect.c |   21 
>  drivers/gpu/drm/i915/i915_dma.c |2 -
>  include/acpi/video.h|   11 ++
>  include/linux/acpi.h|1 
>  6 files changed, 103 insertions(+), 8 deletions(-)
> 
> Index: linux-pm/drivers/acpi/video.c
> ===
> --- linux-pm.orig/drivers/acpi/video.c
> +++ linux-pm/drivers/acpi/video.c
> @@ -44,6 +44,8 @@
>  #include 
>  #include 
>  
> +#include "internal.h"
> +
>  #define PREFIX "ACPI: "
>  
>  #define ACPI_VIDEO_BUS_NAME  "Video Bus"
> @@ -898,7 +900,7 @@ static void acpi_video_device_find_cap(s
>   device->cap._DDC = 1;
>   }
>  
> - if (acpi_video_backlight_support()) {
> + if (acpi_video_verify_backlight_support()) {
>   struct backlight_properties props;
>   struct pci_dev *pdev;
>   acpi_handle acpi_parent;
> @@ -1854,6 +1856,46 @@ static int acpi_video_bus_remove(struct
>   return 0;
>  }
>  
> +static acpi_status video_unregister_backlight(acpi_handle handle, u32 lvl,
> +   void *context, void **rv)
> +{
> + struct acpi_device *acpi_dev;
> + struct acpi_video_bus *video;
> + struct acpi_video_device *dev, *next;
> +
> + if (acpi_bus_get_device(handle, &acpi_dev))
> + return AE_OK;
> +
> + if (acpi_match_device_ids(acpi_dev, video_device_ids))
> + return AE_OK;
> +
> + video = acpi_driver_data(acpi_dev);
> + if (!video)
> + return AE_OK;
> +
> + acpi_video_bus_stop_devices(video);
> + mutex_lock(&video->device_list_lock);
> + list_for_each_entry_safe(dev, next, &video->video_device_list, entry) {
> + if (dev->backlight) {
> + backlight_device_unregister(dev->backlight);
> + dev->backlight = NULL;
> + kfree(dev->brightness->levels);
> + kfree(dev->brightness);
> + }
> + if (dev->cooling_dev) {
> + sysfs_remove_link(&dev->dev->dev.kobj,
> +   "thermal_cooling");
> + sysfs_remove_link(&dev->cooling_dev->device.kobj,
> +   "device");
> + thermal_cooling_

Re: [PATCH] drm/nouveau: kill nouveau_ttm_fault_reserve_notify handler to prevent useless buffer moves

2013-07-14 Thread Ben Skeggs
On Fri, Jul 12, 2013 at 10:45 PM, Maarten Lankhorst
 wrote:
> I have no idea what this bogus restriction on placement is, but it breaks 
> decoding 1080p
> VDPAU at boot speed. With this patch applied I only need to bump the vdec 
> clock to
> get real-time 1080p decoding. It prevents a lot of VRAM <-> VRAM buffer moves.
It's not bogus, and is required for pre-GF8 boards with VRAM > BAR size.

What configuration does the buffer that's getting moved here have
exactly?  The placement restriction isn't necessary on GF8, the rest
of the restrictions may currently be required still however.

Ben.


>
> Signed-off-by: Maarten Lankhorst 
> ---
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c 
> b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index d506da5..86eb321 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -1339,34 +1339,6 @@ nouveau_ttm_io_mem_free(struct ttm_bo_device *bdev, 
> struct ttm_mem_reg *mem)
>  }
>
>  static int
> -nouveau_ttm_fault_reserve_notify(struct ttm_buffer_object *bo)
> -{
> -   struct nouveau_drm *drm = nouveau_bdev(bo->bdev);
> -   struct nouveau_bo *nvbo = nouveau_bo(bo);
> -   struct nouveau_device *device = nv_device(drm->device);
> -   u32 mappable = pci_resource_len(device->pdev, 1) >> PAGE_SHIFT;
> -
> -   /* as long as the bo isn't in vram, and isn't tiled, we've got
> -* nothing to do here.
> -*/
> -   if (bo->mem.mem_type != TTM_PL_VRAM) {
> -   if (nv_device(drm->device)->card_type < NV_50 ||
> -   !nouveau_bo_tile_layout(nvbo))
> -   return 0;
> -   }
> -
> -   /* make sure bo is in mappable vram */
> -   if (bo->mem.start + bo->mem.num_pages < mappable)
> -   return 0;
> -
> -
> -   nvbo->placement.fpfn = 0;
> -   nvbo->placement.lpfn = mappable;
> -   nouveau_bo_placement_set(nvbo, TTM_PL_FLAG_VRAM, 0);
> -   return nouveau_bo_validate(nvbo, false, false);
> -}
> -
> -static int
>  nouveau_ttm_tt_populate(struct ttm_tt *ttm)
>  {
> struct ttm_dma_tt *ttm_dma = (void *)ttm;
> @@ -1524,7 +1496,6 @@ struct ttm_bo_driver nouveau_bo_driver = {
> .sync_obj_flush = nouveau_bo_fence_flush,
> .sync_obj_unref = nouveau_bo_fence_unref,
> .sync_obj_ref = nouveau_bo_fence_ref,
> -   .fault_reserve_notify = &nouveau_ttm_fault_reserve_notify,
> .io_mem_reserve = &nouveau_ttm_io_mem_reserve,
> .io_mem_free = &nouveau_ttm_io_mem_free,
>  };
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel


[Bug 66883] New: [r600 llvm backend] Assert with 32-bit lightsmark, TF2 when shader dump is enabled

2013-07-14 Thread bugzilla-dae...@freedesktop.org
https://bugs.freedesktop.org/show_bug.cgi?id=66883

  Priority: medium
Bug ID: 66883
  Assignee: dri-devel at lists.freedesktop.org
   Summary: [r600 llvm backend] Assert with 32-bit lightsmark, TF2
when shader dump is enabled
  Severity: normal
Classification: Unclassified
OS: All
  Reporter: ptpzz at yandex.ru
  Hardware: Other
Status: NEW
   Version: git
 Component: Drivers/Gallium/r600
   Product: Mesa

I have asserts in the middle of shader dump in llvm backend with 32-bit Team
Fortress 2 and Lightsmark, R600_DEBUG=ps,vs. Here is message with Lightsmark:

...
ALU_CLAUSE 8; dbg:backend: DebugLoc.cpp:27: llvm::MDNode*
llvm::DebugLoc::getScope(const llvm::LLVMContext&) const: Assertion
`unsigned(ScopeIdx) <= Ctx.pImpl->ScopeRecords.size() && "Invalid ScopeIdx!"'
failed.

TF2 message:

...
ALU_CLAUSE 32; dbg:hl2_linux: DebugLoc.cpp:33: llvm::MDNode*
llvm::DebugLoc::getScope(const llvm::LLVMContext&) const: Assertion
`unsigned(-ScopeIdx) <= Ctx.pImpl->ScopeInlinedAtRecords.size() && "Invalid
ScopeIdx"' failed.

It seems in both cases it happens on ALU clause start in the dump.
So far I can't reproduce this issue with 64-bit Lightsmark/mesa/llvm.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130714/96f6e511/attachment.html>


[Bug 62959] r600g (HD 6950 Cayman) fails piglit tests and hangs system

2013-07-14 Thread bugzilla-dae...@freedesktop.org
https://bugs.freedesktop.org/show_bug.cgi?id=62959

--- Comment #66 from Alexandre Demers  ---
While I'm the one who opened this bug, on my side I'm able to run all piglit
tests without any hangs since awhile now. I don't even need to run one test at
a time anymore. But I'm using latest git versions of mesa, drm, ddx with a 3.10
kernel if this can be of any help to those who are adding themselves to the CC
list.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130714/b256d6fc/attachment.html>


[Bug 66713] Team Fortress 2 crashes with r600-sb on HD4850

2013-07-14 Thread bugzilla-dae...@freedesktop.org
https://bugs.freedesktop.org/show_bug.cgi?id=66713

--- Comment #6 from Vadim Girlin  ---
Created attachment 82400
  --> https://bugs.freedesktop.org/attachment.cgi?id=82400&action=edit
[PATCH 1/2] r600g/sb: fix register allocation

Does this patch help?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130714/b0fb9f09/attachment.html>


[Bug 66888] New: [radeonsi] Need support GL_EXT_framebuffer_multisample

2013-07-14 Thread bugzilla-dae...@freedesktop.org
https://bugs.freedesktop.org/show_bug.cgi?id=66888

  Priority: medium
Bug ID: 66888
  Assignee: dri-devel at lists.freedesktop.org
   Summary: [radeonsi] Need support GL_EXT_framebuffer_multisample
  Severity: enhancement
Classification: Unclassified
OS: Linux (All)
  Reporter: grantipak at gmail.com
  Hardware: x86 (IA32)
Status: NEW
   Version: git
 Component: Drivers/Gallium/radeonsi
   Product: Mesa

Created attachment 82401
  --> https://bugs.freedesktop.org/attachment.cgi?id=82401&action=edit
graphical glitch

unigine-tropics and unigine-sanctuary don't runs. Required extension
GL_EXT_framebuffer_multisample is not supported.

ArchLinux x86; kernel 3.10; mesa - git; xf86-video-ati - git; llvm - 3.3

Radeon HD 7950

OpenGL renderer string: Gallium 0.4 on AMD TAHITI
OpenGL version string: 2.1 Mesa 9.2.0-devel (git-796b73d)
OpenGL shading language version string: 1.30

If run MESA_EXTENSION_OVERRIDE="GL_EXT_framebuffer_multisample"
unigine-tropics, test runs but have graphical glitch.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130714/34c86900/attachment.html>


[Bug 66888] [radeonsi] Need support GL_EXT_framebuffer_multisample

2013-07-14 Thread bugzilla-dae...@freedesktop.org
https://bugs.freedesktop.org/show_bug.cgi?id=66888

Vladimir Ysikov  changed:

   What|Removed |Added

  Attachment #82401|text/plain  |image/png
  mime type||

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130714/2856b6a4/attachment.html>


[Bug 66425] "failed testing IB on ring 5" when suspending to disk

2013-07-14 Thread bugzilla-dae...@freedesktop.org
https://bugs.freedesktop.org/show_bug.cgi?id=66425

--- Comment #24 from Austin Lund  ---
Patch tested and works on my machine.  I now have problems for "processors"
when doing pm_test, so I still cannot actually test this on a full resume from
disk, but at least pm_test with "devices" works.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130714/eb046c6d/attachment-0001.html>


[Bug 66425] "failed testing IB on ring 5" when suspending to disk

2013-07-14 Thread bugzilla-dae...@freedesktop.org
https://bugs.freedesktop.org/show_bug.cgi?id=66425

Christian K?nig  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED

--- Comment #25 from Christian K?nig  ---
Sounds like we can merk this as resolved now.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130714/a6e50f35/attachment.html>


[Bug 66714] Mobility Radeon HD 5650 doesn't boot with kernel 3.10 (and newer) when using radeon.audio=1

2013-07-14 Thread bugzilla-dae...@freedesktop.org
https://bugs.freedesktop.org/show_bug.cgi?id=66714

Marco Trevisan (Trevi?o)  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED

--- Comment #4 from Marco Trevisan (Trevi?o)  ---
So, I've tried the patch on a 3.10 kernel that was affected and... Great, it
Works! :)
FYI, I've also added a couple of error logs in the code to see where exactly
the failure was and this was the result:

[2.073137] [drm] fb mappable at 0xC035F000
[2.073139] [drm] vram apper at 0xC000
[2.073140] [drm] size 8294400
[2.073141] [drm] fb depth is 24
[2.073142] [drm]pitch is 7680
[2.073252] fbcon: radeondrmfb (fb0) is primary device
[2.073323] [drm:evergreen_hdmi_enable] *ERROR* Invalid DIG (dig:
880229d0fc00, dig->afmt: (null))
/home/marco/Dev/debs/linux-3.10.0/drivers/gpu/drm/radeon/evergreen_hdmi.c:298
[2.573589] [drm:evergreen_hdmi_enable] *ERROR* Invalid DIG (dig:
880229d0fc00, dig->afmt: (null))
/home/marco/Dev/debs/linux-3.10.0/drivers/gpu/drm/radeon/evergreen_hdmi.c:298
[2.607449] Console: switching to colour frame buffer device 170x48
[2.610160] radeon :01:00.0: fb0: radeondrmfb frame buffer device
[2.610162] radeon :01:00.0: registered panic notifier
[2.610237] [drm] Initialized radeon 2.33.0 20080528 for :01:00.0 on
minor 0

Full dmesg at http://pastebin.ubuntu.com/5873667/
I don't know if this may be only the side effect of another issue (wrong
initialization order?).

One more thing I discovered in the past days, is that the kernel crash was not
happening in case that the HDMI was plugged after that ligthtdm was running
(attaching it anytime before was leading to a freeze).

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130714/89fb758e/attachment.html>


[Bug 66714] Mobility Radeon HD 5650 doesn't boot with kernel 3.10 (and newer) when using radeon.audio=1

2013-07-14 Thread bugzilla-dae...@freedesktop.org
https://bugs.freedesktop.org/show_bug.cgi?id=66714

--- Comment #5 from Marco Trevisan (Trevi?o)  ---
Comment on attachment 82200
  --> https://bugs.freedesktop.org/attachment.cgi?id=82200
possible fix

Review of attachment 82200:
-

Why not including some DRM_ERROR logs as well?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130714/24e4a281/attachment.html>


[PATCH] radeon kms: do not flush uninitialized hotplug work

2013-07-14 Thread Sergey Senozhatsky
Fix a warning from lockdep caused by calling flush_work() for
uninitialized hotplug work. Initialize hotplug_work, audio_work
and reset_work upon successful radeon_irq_kms_init() completion
and thus perform hotplug flush_work only when rdev->irq.installed
is true.

[4.790019] [drm] Loading CEDAR Microcode
[4.790943] r600_cp: Failed to load firmware "radeon/CEDAR_smc.bin"
[4.791152] [drm:evergreen_startup] *ERROR* Failed to load firmware!
[4.791330] radeon :01:00.0: disabling GPU acceleration

[4.792633] INFO: trying to register non-static key.
[4.792792] the code is fine but needs lockdep annotation.
[4.792953] turning off the locking correctness validator.

[4.793114] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 
3.11.0-rc0-dbg-10676-gfe56456-dirty #1816
[4.793314] Hardware name: Acer Aspire 5741G/Aspire 5741G
, BIOS V1.20 02/08/2011
[4.793507]  821fd810 8801530b9a18 8160434e 
0002
[4.794155]  8801530b9ad8 810b8404 8801530b0798 
8801530b
[4.794789]  8801530b9b00 0046 04c0 

[4.795418] Call Trace:
[4.795573]  [] dump_stack+0x4e/0x82
[4.795731]  [] __lock_acquire+0x1a64/0x1d30
[4.795893]  [] ? dev_vprintk_emit+0x50/0x60
[4.796034]  [] lock_acquire+0xa4/0x200
[4.796216]  [] ? flush_work+0x5/0x280
[4.796375]  [] flush_work+0x3d/0x280
[4.796520]  [] ? flush_work+0x5/0x280
[4.796682]  [] ? trace_hardirqs_on_caller+0xfd/0x1c0
[4.796862]  [] ? delay_tsc+0x95/0xf0
[4.797024]  [] radeon_irq_kms_fini+0x2b/0x70
[4.797186]  [] evergreen_init+0x2a9/0x2e0
[4.797347]  [] radeon_device_init+0x5ef/0x700
[4.797511]  [] ? pci_find_capability+0x47/0x50
[4.797672]  [] radeon_driver_load_kms+0x8d/0x150
[4.797843]  [] drm_get_pci_dev+0x166/0x280
[4.798007]  [] ? kfree+0xf5/0x2e0
[4.798168]  [] ? radeon_pci_probe+0x98/0xd0
[4.798329]  [] radeon_pci_probe+0xaa/0xd0
[4.798489]  [] pci_device_probe+0x84/0xe0
[4.798644]  [] driver_probe_device+0x76/0x240
[4.798805]  [] __driver_attach+0x93/0xa0
[4.798948]  [] ? __device_attach+0x40/0x40
[4.799126]  [] bus_for_each_dev+0x6b/0xb0
[4.799272]  [] driver_attach+0x1e/0x20
[4.799434]  [] bus_add_driver+0x1f0/0x280
[4.799596]  [] driver_register+0x74/0x150
[4.799758]  [] __pci_register_driver+0x5d/0x60
[4.799936]  [] ? ttm_init+0x67/0x67
[4.800081]  [] drm_pci_init+0x115/0x130
[4.800243]  [] ? ttm_init+0x67/0x67
[4.800405]  [] radeon_init+0x9c/0xba
[4.800586]  [] do_one_initcall+0xfa/0x150
[4.800746]  [] ? parse_args+0x120/0x330
[4.800909]  [] kernel_init_freeable+0x111/0x191
[4.801052]  [] ? do_early_param+0x88/0x88
[4.801233]  [] ? rest_init+0x140/0x140
[4.801393]  [] kernel_init+0xe/0x180
[4.801556]  [] ret_from_fork+0x7c/0xb0
[4.801718]  [] ? rest_init+0x140/0x140


Signed-off-by: Sergey Senozhatsky 

---

 drivers/gpu/drm/radeon/radeon_irq_kms.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c 
b/drivers/gpu/drm/radeon/radeon_irq_kms.c
index bcdefd1..081886b 100644
--- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
@@ -260,10 +260,6 @@ int radeon_irq_kms_init(struct radeon_device *rdev)
 {
int r = 0;

-   INIT_WORK(&rdev->hotplug_work, radeon_hotplug_work_func);
-   INIT_WORK(&rdev->audio_work, r600_audio_update_hdmi);
-   INIT_WORK(&rdev->reset_work, radeon_irq_reset_work_func);
-
spin_lock_init(&rdev->irq.lock);
r = drm_vblank_init(rdev->ddev, rdev->num_crtc);
if (r) {
@@ -285,6 +281,11 @@ int radeon_irq_kms_init(struct radeon_device *rdev)
rdev->irq.installed = false;
return r;
}
+
+   INIT_WORK(&rdev->hotplug_work, radeon_hotplug_work_func);
+   INIT_WORK(&rdev->audio_work, r600_audio_update_hdmi);
+   INIT_WORK(&rdev->reset_work, radeon_irq_reset_work_func);
+
DRM_INFO("radeon: irq initialized.\n");
return 0;
 }
@@ -304,8 +305,8 @@ void radeon_irq_kms_fini(struct radeon_device *rdev)
rdev->irq.installed = false;
if (rdev->msi_enabled)
pci_disable_msi(rdev->pdev);
+   flush_work(&rdev->hotplug_work);
}
-   flush_work(&rdev->hotplug_work);
 }

 /**



[Bug 63935] TURKS [drm:r600_uvd_init] *ERROR* UVD not responding, trying to reset the VCPU!!!

2013-07-14 Thread bugzilla-dae...@freedesktop.org
https://bugs.freedesktop.org/show_bug.cgi?id=63935

Christian K?nig  changed:

   What|Removed |Added

 Status|REOPENED|RESOLVED
 Resolution|--- |FIXED

--- Comment #57 from Christian K?nig  ---
The suspend/resume issue was something completely different, so we probably can
now close this bugreport.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130714/d2ca9c28/attachment-0001.html>


[Bug 66714] Mobility Radeon HD 5650 doesn't boot with kernel 3.10 (and newer) when using radeon.audio=1

2013-07-14 Thread bugzilla-dae...@freedesktop.org
https://bugs.freedesktop.org/show_bug.cgi?id=66714

Christian K?nig  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED

--- Comment #6 from Christian K?nig  ---
(In reply to comment #5)
> Comment on attachment 82200 [details] [review]
> possible fix
> 
> Review of attachment 82200 [details] [review]:
> -
> 
> Why not including some DRM_ERROR logs as well?

Because that isn't a bug.

You just have an LVDS pannel connected to a DIG encoder and so this encoder
doesn't have an audio block.

Alex patch is already quite right.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130714/6f7a56b3/attachment.html>


[PATCH] radeon kms: do not flush uninitialized hotplug work

2013-07-14 Thread Alex Deucher
On Sun, Jul 14, 2013 at 7:03 AM, Sergey Senozhatsky
 wrote:
> Fix a warning from lockdep caused by calling flush_work() for
> uninitialized hotplug work. Initialize hotplug_work, audio_work
> and reset_work upon successful radeon_irq_kms_init() completion
> and thus perform hotplug flush_work only when rdev->irq.installed
> is true.
>
> [4.790019] [drm] Loading CEDAR Microcode
> [4.790943] r600_cp: Failed to load firmware "radeon/CEDAR_smc.bin"
> [4.791152] [drm:evergreen_startup] *ERROR* Failed to load firmware!
> [4.791330] radeon :01:00.0: disabling GPU acceleration
>
> [4.792633] INFO: trying to register non-static key.
> [4.792792] the code is fine but needs lockdep annotation.
> [4.792953] turning off the locking correctness validator.
>
> [4.793114] CPU: 2 PID: 1 Comm: swapper/0 Not tainted 
> 3.11.0-rc0-dbg-10676-gfe56456-dirty #1816
> [4.793314] Hardware name: Acer Aspire 5741G/Aspire 5741G  
>   , BIOS V1.20 02/08/2011
> [4.793507]  821fd810 8801530b9a18 8160434e 
> 0002
> [4.794155]  8801530b9ad8 810b8404 8801530b0798 
> 8801530b
> [4.794789]  8801530b9b00 0046 04c0 
> 
> [4.795418] Call Trace:
> [4.795573]  [] dump_stack+0x4e/0x82
> [4.795731]  [] __lock_acquire+0x1a64/0x1d30
> [4.795893]  [] ? dev_vprintk_emit+0x50/0x60
> [4.796034]  [] lock_acquire+0xa4/0x200
> [4.796216]  [] ? flush_work+0x5/0x280
> [4.796375]  [] flush_work+0x3d/0x280
> [4.796520]  [] ? flush_work+0x5/0x280
> [4.796682]  [] ? trace_hardirqs_on_caller+0xfd/0x1c0
> [4.796862]  [] ? delay_tsc+0x95/0xf0
> [4.797024]  [] radeon_irq_kms_fini+0x2b/0x70
> [4.797186]  [] evergreen_init+0x2a9/0x2e0
> [4.797347]  [] radeon_device_init+0x5ef/0x700
> [4.797511]  [] ? pci_find_capability+0x47/0x50
> [4.797672]  [] radeon_driver_load_kms+0x8d/0x150
> [4.797843]  [] drm_get_pci_dev+0x166/0x280
> [4.798007]  [] ? kfree+0xf5/0x2e0
> [4.798168]  [] ? radeon_pci_probe+0x98/0xd0
> [4.798329]  [] radeon_pci_probe+0xaa/0xd0
> [4.798489]  [] pci_device_probe+0x84/0xe0
> [4.798644]  [] driver_probe_device+0x76/0x240
> [4.798805]  [] __driver_attach+0x93/0xa0
> [4.798948]  [] ? __device_attach+0x40/0x40
> [4.799126]  [] bus_for_each_dev+0x6b/0xb0
> [4.799272]  [] driver_attach+0x1e/0x20
> [4.799434]  [] bus_add_driver+0x1f0/0x280
> [4.799596]  [] driver_register+0x74/0x150
> [4.799758]  [] __pci_register_driver+0x5d/0x60
> [4.799936]  [] ? ttm_init+0x67/0x67
> [4.800081]  [] drm_pci_init+0x115/0x130
> [4.800243]  [] ? ttm_init+0x67/0x67
> [4.800405]  [] radeon_init+0x9c/0xba
> [4.800586]  [] do_one_initcall+0xfa/0x150
> [4.800746]  [] ? parse_args+0x120/0x330
> [4.800909]  [] kernel_init_freeable+0x111/0x191
> [4.801052]  [] ? do_early_param+0x88/0x88
> [4.801233]  [] ? rest_init+0x140/0x140
> [4.801393]  [] kernel_init+0xe/0x180
> [4.801556]  [] ret_from_fork+0x7c/0xb0
> [4.801718]  [] ? rest_init+0x140/0x140
>
>
> Signed-off-by: Sergey Senozhatsky 
>

Applied to by fixes tree.  thanks!

Alex

> ---
>
>  drivers/gpu/drm/radeon/radeon_irq_kms.c | 11 ++-
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_irq_kms.c 
> b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> index bcdefd1..081886b 100644
> --- a/drivers/gpu/drm/radeon/radeon_irq_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_irq_kms.c
> @@ -260,10 +260,6 @@ int radeon_irq_kms_init(struct radeon_device *rdev)
>  {
> int r = 0;
>
> -   INIT_WORK(&rdev->hotplug_work, radeon_hotplug_work_func);
> -   INIT_WORK(&rdev->audio_work, r600_audio_update_hdmi);
> -   INIT_WORK(&rdev->reset_work, radeon_irq_reset_work_func);
> -
> spin_lock_init(&rdev->irq.lock);
> r = drm_vblank_init(rdev->ddev, rdev->num_crtc);
> if (r) {
> @@ -285,6 +281,11 @@ int radeon_irq_kms_init(struct radeon_device *rdev)
> rdev->irq.installed = false;
> return r;
> }
> +
> +   INIT_WORK(&rdev->hotplug_work, radeon_hotplug_work_func);
> +   INIT_WORK(&rdev->audio_work, r600_audio_update_hdmi);
> +   INIT_WORK(&rdev->reset_work, radeon_irq_reset_work_func);
> +
> DRM_INFO("radeon: irq initialized.\n");
> return 0;
>  }
> @@ -304,8 +305,8 @@ void radeon_irq_kms_fini(struct radeon_device *rdev)
> rdev->irq.installed = false;
> if (rdev->msi_enabled)
> pci_disable_msi(rdev->pdev);
> +   flush_work(&rdev->hotplug_work);
> }
> -   flush_work(&rdev->hotplug_work);
>  }
>
>  /**
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume

2013-07-14 Thread Konstantin Khlebnikov
This patch fixes regression in power consumtion of sandy bridge gpu, which
exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that
it's extremely busy. After that it never reaches rc6 state.

Bug was introduce by commit b4ae3f22d238617ca11610b29fde16cf8c0bc6e0
("drm/i915: load boot context at driver init time"). Without documentation
it's not clear what is happening here, probably this breaks internal state of
hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake
during whole initialization sequence in gen6_init_clock_gating() fixes this bug.

References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
Signed-off-by: Konstantin Khlebnikov 
---
 drivers/gpu/drm/i915/intel_pm.c |4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index aa01128..839a43f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3640,6 +3640,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)
int pipe;
uint32_t dspclk_gate = ILK_VRHUNIT_CLOCK_GATE_DISABLE;

+   gen6_gt_force_wake_get(dev_priv);
+
I915_WRITE(ILK_DSPCLK_GATE_D, dspclk_gate);

I915_WRITE(ILK_DISPLAY_CHICKEN2,
@@ -3728,6 +3730,8 @@ static void gen6_init_clock_gating(struct drm_device *dev)
cpt_init_clock_gating(dev);

gen6_check_mch_setup(dev);
+
+   gen6_gt_force_wake_put(dev_priv);
 }

 static void gen7_setup_fixed_func_scheduler(struct drm_i915_private *dev_priv)



[PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume

2013-07-14 Thread Daniel Vetter
On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov
 wrote:
> This patch fixes regression in power consumtion of sandy bridge gpu, which
> exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking that
> it's extremely busy. After that it never reaches rc6 state.
>
> Bug was introduce by commit b4ae3f22d238617ca11610b29fde16cf8c0bc6e0
> ("drm/i915: load boot context at driver init time"). Without documentation
> it's not clear what is happening here, probably this breaks internal state of
> hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake
> during whole initialization sequence in gen6_init_clock_gating() fixes this 
> bug.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
> Signed-off-by: Konstantin Khlebnikov 

We already hold an forcewake reference while setting up the rps stuff,
should we maybe hold the forcewake for the entire duration, i.e. grab
it here in clock_gating and release it only in gen6/vlv_enable_rps?
Can you please test that version, too?

In any case the forcewake grabbing here in the clock gating function
needs a big comment that otherwise setting the MCTL register might
break rc6 entry.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_pm.c |4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index aa01128..839a43f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3640,6 +3640,8 @@ static void gen6_init_clock_gating(struct drm_device 
> *dev)
> int pipe;
> uint32_t dspclk_gate = ILK_VRHUNIT_CLOCK_GATE_DISABLE;
>
> +   gen6_gt_force_wake_get(dev_priv);
> +
> I915_WRITE(ILK_DSPCLK_GATE_D, dspclk_gate);
>
> I915_WRITE(ILK_DISPLAY_CHICKEN2,
> @@ -3728,6 +3730,8 @@ static void gen6_init_clock_gating(struct drm_device 
> *dev)
> cpt_init_clock_gating(dev);
>
> gen6_check_mch_setup(dev);
> +
> +   gen6_gt_force_wake_put(dev_priv);
>  }
>
>  static void gen7_setup_fixed_func_scheduler(struct drm_i915_private 
> *dev_priv)
>



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch


[Bug 64850] Second screen black on Pitcairn PRO

2013-07-14 Thread bugzilla-dae...@freedesktop.org
https://bugs.freedesktop.org/show_bug.cgi?id=64850

--- Comment #23 from Jakob Nixdorf  ---
Just tested the first kernel with RadeonSI support (3.4.0 to my knowledge) with
the same result.
Second screen stays black in TTY and in Xorg.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20130714/c550d24a/attachment.html>


[PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume

2013-07-14 Thread Konstantin Khlebnikov
Daniel Vetter wrote:
> On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov
>   wrote:
>> This patch fixes regression in power consumtion of sandy bridge gpu, which
>> exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking 
>> that
>> it's extremely busy. After that it never reaches rc6 state.
>>
>> Bug was introduce by commit b4ae3f22d238617ca11610b29fde16cf8c0bc6e0
>> ("drm/i915: load boot context at driver init time"). Without documentation
>> it's not clear what is happening here, probably this breaks internal state of
>> hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake
>> during whole initialization sequence in gen6_init_clock_gating() fixes this 
>> bug.
>>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
>> Signed-off-by: Konstantin Khlebnikov
>
> We already hold an forcewake reference while setting up the rps stuff,
> should we maybe hold the forcewake for the entire duration, i.e. grab
> it here in clock_gating and release it only in gen6/vlv_enable_rps?
> Can you please test that version, too?

This will be racy because rps stuff is done in separate work which might be 
canceled
if intel_disable_gt_powersave() happens before its completion.

>
> In any case the forcewake grabbing here in the clock gating function
> needs a big comment that otherwise setting the MCTL register might
> break rc6 entry.
> -Daniel
>
>> ---
>>   drivers/gpu/drm/i915/intel_pm.c |4 
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index aa01128..839a43f 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3640,6 +3640,8 @@ static void gen6_init_clock_gating(struct drm_device 
>> *dev)
>>  int pipe;
>>  uint32_t dspclk_gate = ILK_VRHUNIT_CLOCK_GATE_DISABLE;
>>
>> +   gen6_gt_force_wake_get(dev_priv);
>> +
>>  I915_WRITE(ILK_DSPCLK_GATE_D, dspclk_gate);
>>
>>  I915_WRITE(ILK_DISPLAY_CHICKEN2,
>> @@ -3728,6 +3730,8 @@ static void gen6_init_clock_gating(struct drm_device 
>> *dev)
>>  cpt_init_clock_gating(dev);
>>
>>  gen6_check_mch_setup(dev);
>> +
>> +   gen6_gt_force_wake_put(dev_priv);
>>   }
>>
>>   static void gen7_setup_fixed_func_scheduler(struct drm_i915_private 
>> *dev_priv)
>>
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



[PATCH] drm/i915: fix long-standing SNB regression in power consumption after resume

2013-07-14 Thread Chris Wilson
On Sun, Jul 14, 2013 at 06:52:39PM +0200, Daniel Vetter wrote:
> On Sun, Jul 14, 2013 at 6:30 PM, Konstantin Khlebnikov
>  wrote:
> > This patch fixes regression in power consumtion of sandy bridge gpu, which
> > exists since v3.6 Sometimes after resuming from s2ram gpu starts thinking 
> > that
> > it's extremely busy. After that it never reaches rc6 state.
> >
> > Bug was introduce by commit b4ae3f22d238617ca11610b29fde16cf8c0bc6e0
> > ("drm/i915: load boot context at driver init time"). Without documentation
> > it's not clear what is happening here, probably this breaks internal state 
> > of
> > hardware ring buffers and confuses RPS engine. Fortunately keeping forcewake
> > during whole initialization sequence in gen6_init_clock_gating() fixes this 
> > bug.
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=54089
> > Signed-off-by: Konstantin Khlebnikov 
> 
> We already hold an forcewake reference while setting up the rps stuff,
> should we maybe hold the forcewake for the entire duration, i.e. grab
> it here in clock_gating and release it only in gen6/vlv_enable_rps?
> Can you please test that version, too?
> 
> In any case the forcewake grabbing here in the clock gating function
> needs a big comment that otherwise setting the MCTL register might
> break rc6 entry.

It is not clear why the forcewake works, but is easy to imagine one of
the operations in that sequence requires the GPU to be awake at the time
of programming for it to succeed. MBCTL:EnableBootFetch does seem the most
suspicious from its wording in the bspec. I guess all instances of
poking this bit should be protected similary (snb, ivb, vlv, hsw).

Based on that reasoning and that waking the GPU up here has no negative
consequences, and so long as all paths are fixed, I am happy to give this
an Acked-by: Chris Wilson 

Also, we need to reapply the w/a after a Function Level Reset, in other
words we do need to repeat the init_clock_gating after
intel_gpu_reset().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre


DT binding review for Armada display subsystem

2013-07-14 Thread Sylwester Nawrocki
On 07/13/2013 11:02 PM, Russell King - ARM Linux wrote:
> On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
>> I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
>> they're working on v4 with clock object reference counting. Presumably we
>> need both clk_get() to be taking reference on the module and reference
>> counted clk free, e.g. in cases where clock provider is a hot-pluggable
>> device. It might be too paranoid though, I haven't seen hardware
>> configurations where a clock source could be unplugged safely when whole
>> system is running.
>
> I'm not going to accept refcounting being thrown into clk_get().  The
> clkdev API already has refcounting, as much as it needs to.  It just
> needs people to use the hooks that I provided back in 2008 when I
> created the clkdev API for doing _precisely_ this job.
>
> Have a read through these commits, which backup my statement above:
>
> 0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the clkdev API
> d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting Integrator to clkdev API
>
> and it will show you how to do refcounting.  The common clk API just
> needs to stop defining __clk_get() and __clk_put() to be an empty
> function and implement them appropriately for it's clk implementation,
> like they were always meant to be.

Sure, I've already seen the above commits. This is how I understood it
as well - __clk_get(), __clk_put() need to be defined by the common clk
API, since it is going to replace all the arch specific implementations.

> __clk_get() and __clk_put() are the clk-implementation specific parts
> of the clkdev API, because the clkdev API is utterly divorsed from the
> internals of what a 'struct clk' actually is.  clkdev just treats a
> 'struct clk' as a completely opaque type and never bothers poking
> about inside it.

OK, but at the clock's implementation side we may need, e.g. to use kref
to keep track of the clock's state, and free it only when it is unprepared,
all its children are unregistered, etc. ? Is it not what you stated in
your comment to patch [1] ?

[1] https://patchwork.kernel.org/patch/2651171/


DT binding review for Armada display subsystem

2013-07-14 Thread Russell King - ARM Linux
On Sun, Jul 14, 2013 at 12:16:58AM +0200, Sylwester Nawrocki wrote:
> On 07/13/2013 11:02 PM, Russell King - ARM Linux wrote:
>> On Sat, Jul 13, 2013 at 10:43:29PM +0200, Sylwester Nawrocki wrote:
>>> I wasn't aware of it, thanks. I've seen a patch from Jiada Wang, it seems
>>> they're working on v4 with clock object reference counting. Presumably we
>>> need both clk_get() to be taking reference on the module and reference
>>> counted clk free, e.g. in cases where clock provider is a hot-pluggable
>>> device. It might be too paranoid though, I haven't seen hardware
>>> configurations where a clock source could be unplugged safely when whole
>>> system is running.
>>
>> I'm not going to accept refcounting being thrown into clk_get().  The
>> clkdev API already has refcounting, as much as it needs to.  It just
>> needs people to use the hooks that I provided back in 2008 when I
>> created the clkdev API for doing _precisely_ this job.
>>
>> Have a read through these commits, which backup my statement above:
>>
>> 0318e693d3a56836632bf1a2cfdafb7f34bcc703 - initial commit of the clkdev API
>> d72fbdf01fc77628c0b837d0dd2fd564fa26ede6 - converting Integrator to clkdev 
>> API
>>
>> and it will show you how to do refcounting.  The common clk API just
>> needs to stop defining __clk_get() and __clk_put() to be an empty
>> function and implement them appropriately for it's clk implementation,
>> like they were always meant to be.
>
> Sure, I've already seen the above commits. This is how I understood it
> as well - __clk_get(), __clk_put() need to be defined by the common clk
> API, since it is going to replace all the arch specific implementations.
>
>> __clk_get() and __clk_put() are the clk-implementation specific parts
>> of the clkdev API, because the clkdev API is utterly divorsed from the
>> internals of what a 'struct clk' actually is.  clkdev just treats a
>> 'struct clk' as a completely opaque type and never bothers poking
>> about inside it.
>
> OK, but at the clock's implementation side we may need, e.g. to use kref
> to keep track of the clock's state, and free it only when it is unprepared,
> all its children are unregistered, etc. ? Is it not what you stated in
> your comment to patch [1] ?

If you want to do refcounting on a clock (which you run into problems
as I highlighted earlier in this thread) then yes, you need to use a
kref, and take a reference in __clk_get() and drop it in __clk_put()
to make things safe.

Whether you also take a reference on the module supplying the clock or
not depends whether you need to keep the code around to manipulate that
clock while there are users of it.

As I've already said - consider the possibilities with this scenario.
Here's one:

  A clock consumer is using a clock, but the clock supplier has been
  removed.  The clock consumer wants to change the state of the clock
  in some way - eg, system is suspending.  clk_disable() doesn't fail,
  but on resume, clk_enable() does... (and how many people assume that
  clk_enable() never fails?)  And who knows what rate the clock is now
  producing or whether it's even producing anything...

I'm not saying don't do the refcounting thing I mentioned back in June.
I'm merely pointing out the issues that there are.  There isn't a one
right answer here because each has their own advantages and disadvantages
(and problems.)