Re: [Intel-gfx] [PATCH 2/2] drm/i915: Support for HDMI complaince HPD
I know. That is orthogonal to the tweaks I was suggesting. Also if you feel you need to add details to your rationale, then your changelog is incomplete. -Chris Thanks Chris, Please find my comments inline to your previous mail, with suggestions. On 8/12/2014 6:17 PM, Chris Wilson wrote: On Tue, Aug 12, 2014 at 06:08:21PM +0530, shashank.sha...@intel.com wrote: From: Shashank Sharma shashank.sha...@intel.com During the HDMI complaince tests, most of the HDMI analyzers issue a soft HPD, to automate the tests. This process keeps the HDMI cable connected, and DDC chhanel alive. HDMI detect() logic relies on EDID readability, to decide if its a HDMI connect or disconnect event. But in this case, the EDID is always readable, as HDMI cable is physically connected and DDC channel is UP, so detect() always reports a HDMI connect even if its intended to be a HDMI disconnect call. So if HDMI compliance is enabled, we should rely on the live status register, than EDID availability. This patch adds: 1. One kernel command line parameter for i915 module, which indicates if we want to support HDMI compliance, for this platform. I would rather have this as an output property. In fact, I would like the hotplug detection method exposed (and even selectable, but other than this compliance testing, I can't think of a scenario where the kernel shouldn't be able to figure things out for itself). Considering the history of the case, can you please elaborate this suggestion ? I dont think I am getting it right. 2. A new function to read EDID, which gets called only in case of HDMI compliance support is required. This function reads EDID only if live status register is up. The normal code flow doesn't get effected if kernel command line parameter is not enabled. 3. After various experiments on VLV2 board, with various HDMI monitors we have seen that, with few monitors, the live status register gets set after a slight delay, and then stays reliably. To support such monitors, there is a busy-loop added, with a max delay upto 50ms, with a status check after every 10ms. Please see the comment in intel_hdmi_get_edid. Wouldn't a tidier solution be to delay the hpd by 50-100ms after the hotplug interrupt? That may overcome the issue with the live status for all connectors... -Chris There would be few problems in this case: 1. We dont want this scenario to come into picture for DP, as DP HPD pulse can be as small as 2ms. 2. Not all the HDMI monitors show this problem, but a significant subset of popular monitors do. 3. In HDCP compliance testing, we send a HPD pulse train of UP and Down, where down pulse can be as small as 100ms. If we increase the delay by 100ms, we will definitely miss the HPD down pulse. 4. The method what we are using is a busy waiting check, where we delay the pulse for 50ms, but take a sample of live_status per 10ms, so if the live status is up with a delay of 20ms, we needn't to waste another 30. 5. We want this code routine only to be executed for commercial (like android) platforms, whereas others get the routine code. @ Danvet: Do you want to add something here ? Regards Shashank ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Support for HDMI complaince HPD
On Wed, Aug 13, 2014 at 11:34:02AM +0530, Sharma, Shashank wrote: I know. That is orthogonal to the tweaks I was suggesting. Also if you feel you need to add details to your rationale, then your changelog is incomplete. -Chris Thanks Chris, Please find my comments inline to your previous mail, with suggestions. On 8/12/2014 6:17 PM, Chris Wilson wrote: On Tue, Aug 12, 2014 at 06:08:21PM +0530, shashank.sha...@intel.com wrote: From: Shashank Sharma shashank.sha...@intel.com During the HDMI complaince tests, most of the HDMI analyzers issue a soft HPD, to automate the tests. This process keeps the HDMI cable connected, and DDC chhanel alive. HDMI detect() logic relies on EDID readability, to decide if its a HDMI connect or disconnect event. But in this case, the EDID is always readable, as HDMI cable is physically connected and DDC channel is UP, so detect() always reports a HDMI connect even if its intended to be a HDMI disconnect call. So if HDMI compliance is enabled, we should rely on the live status register, than EDID availability. This patch adds: 1. One kernel command line parameter for i915 module, which indicates if we want to support HDMI compliance, for this platform. I would rather have this as an output property. In fact, I would like the hotplug detection method exposed (and even selectable, but other than this compliance testing, I can't think of a scenario where the kernel shouldn't be able to figure things out for itself). Considering the history of the case, can you please elaborate this suggestion ? I dont think I am getting it right. Instead of (or in addition to) adding a kernel parameter, you add an output property so that it can be adjusted on the fly for individual monitors. 2. A new function to read EDID, which gets called only in case of HDMI compliance support is required. This function reads EDID only if live status register is up. The normal code flow doesn't get effected if kernel command line parameter is not enabled. 3. After various experiments on VLV2 board, with various HDMI monitors we have seen that, with few monitors, the live status register gets set after a slight delay, and then stays reliably. To support such monitors, there is a busy-loop added, with a max delay upto 50ms, with a status check after every 10ms. Please see the comment in intel_hdmi_get_edid. Wouldn't a tidier solution be to delay the hpd by 50-100ms after the hotplug interrupt? That may overcome the issue with the live status for all connectors... -Chris There would be few problems in this case: 1. We dont want this scenario to come into picture for DP, as DP HPD pulse can be as small as 2ms. If the live status is asserted and deasserted within 2ms, do you care? Or perhaps you are talking about something else entirely. 2. Not all the HDMI monitors show this problem, but a significant subset of popular monitors do. 3. In HDCP compliance testing, we send a HPD pulse train of UP and Down, where down pulse can be as small as 100ms. If we increase the delay by 100ms, we will definitely miss the HPD down pulse. And? If the monitor is only plugged in for less than 0.1s do I really want to waste 1-2s of user time reconfiguring the desktop and applications before undoing all the changes? There is no point in compliance testing if it does not actually test the code going to be used. 4. The method what we are using is a busy waiting check, where we delay the pulse for 50ms, but take a sample of live_status per 10ms, so if the live status is up with a delay of 20ms, we needn't to waste another 30. Yes. You block using 100% of the cpu in an uninterruptable context for a significant period of time. DO NOT DO THIS. 5. We want this code routine only to be executed for commercial (like android) platforms, whereas others get the routine code. In other words, you want to ignore years of real world compatibity testing and larger user bases. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Add support for variable cursor size on 845/865
On Tue, Aug 12, 2014 at 07:39:55PM +0300, ville.syrj...@linux.intel.com wrote:
/* Check for which cursor types we support */
- if (!((width == 64 height == 64) ||
- (width == 128 height == 128 !IS_GEN2(dev)) ||
- (width == 256 height == 256 !IS_GEN2(dev {
- DRM_DEBUG(Cursor dimension not supported\n);
- return -EINVAL;
+ if (IS_845G(dev) || IS_I865G(dev)) {
+ if (width == 0 || height == 0 || (width 63) != 0 ||
+ width (IS_845G(dev) ? 64 : 512) || height 1023) {
+ DRM_DEBUG(Cursor dimension not supported\n);
+ return -EINVAL;
+ }
+ } else {
+ if (!((width == 64 height == 64) ||
+ (width == 128 height == 128 !IS_GEN2(dev)) ||
+ (width == 256 height == 256 !IS_GEN2(dev {
+ DRM_DEBUG(Cursor dimension not supported\n);
+ return -EINVAL;
Whilst changing this code, could we rewrite this sanely?
switch (width | height) { // fails width==0 xor height==0
case 128:
case 256: if (!IS_GEN2(dev))
case 64: break;
default:
DRM_DEBUG(Cursor dimension not supported\n);
return -EINVAL;
}
Ok, maybe I was having too much fun, but there are simpler ways of
writing that predicate.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Add support for variable cursor size on 845/865
On Tue, Aug 12, 2014 at 07:39:55PM +0300, ville.syrj...@linux.intel.com wrote:
/* Check for which cursor types we support */
- if (!((width == 64 height == 64) ||
- (width == 128 height == 128 !IS_GEN2(dev)) ||
- (width == 256 height == 256 !IS_GEN2(dev {
- DRM_DEBUG(Cursor dimension not supported\n);
- return -EINVAL;
+ if (IS_845G(dev) || IS_I865G(dev)) {
+ if (width == 0 || height == 0 || (width 63) != 0 ||
+ width (IS_845G(dev) ? 64 : 512) || height 1023) {
+ DRM_DEBUG(Cursor dimension not supported\n);
+ return -EINVAL;
+ }
This is the cursor size in pixels and cannot be greater than the stride value
being used.
[DevBDG]: In all modes the size is fixed to be 64.
[DevSDG]: In all modes the size must be an integer mutliple of 64.
That does the right thing, though I had to read it through a couple of times.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: Add support for variable cursor size on 845/865
On Tue, Aug 12, 2014 at 07:39:55PM +0300, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
845/865 support different cursor sizes as well, albeit a bit differently
than later platforms. Add the necessary code to make them work.
Untested due to lack of hardware.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_reg.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 86
+---
drivers/gpu/drm/i915/intel_drv.h | 1 +
3 files changed, 64 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f79c20d..203062e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4128,7 +4128,8 @@ enum punit_power_well {
/* Old style CUR*CNTR flags (desktop 8xx) */
#define CURSOR_ENABLE 0x8000
#define CURSOR_GAMMA_ENABLE0x4000
-#define CURSOR_STRIDE_MASK 0x3000
+#define CURSOR_STRIDE_SHIFT28
+#define CURSOR_STRIDE(x) ((ffs(x)-9) CURSOR_STRIDE_SHIFT) /*
256,512,1k,2k */
#define CURSOR_PIPE_CSC_ENABLE (124)
#define CURSOR_FORMAT_SHIFT24
#define CURSOR_FORMAT_MASK (0x07 CURSOR_FORMAT_SHIFT)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index a1cf052..db10870 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8005,30 +8005,53 @@ static void i845_update_cursor(struct drm_crtc *crtc,
u32 base)
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- uint32_t cntl;
+ uint32_t cntl = 0, size = 0;
- if (base != intel_crtc-cursor_base) {
- /* On these chipsets we can only modify the base whilst
- * the cursor is disabled.
- */
- if (intel_crtc-cursor_cntl) {
- I915_WRITE(_CURACNTR, 0);
- POSTING_READ(_CURACNTR);
- intel_crtc-cursor_cntl = 0;
+ if (base) {
+ unsigned int width = intel_crtc-cursor_width;
+ unsigned int height = intel_crtc-cursor_height;
+ unsigned int stride = roundup_pow_of_two(width) * 4;
+
+ switch (stride) {
+ case 256:
+ case 512:
+ case 1024:
+ case 2048:
+ cntl |= CURSOR_STRIDE(stride);
+ break;
+ default:
+ WARN_ON(1);
+ return;
Hmm, this is just a programming error. I would rather
switch (stride) {
default:
WARN_ONCE(1,
Invalid cursor width/stride, width=%d, stride=%d\n,
width, stride));
stride = 256;
/* fallthrough */
case 256:
case 512:
case 1024:
case 2048:
break;
}
You get the warning about the programming bug, the user keeps his cursor
(even a corrupt cursor is better than none, and is much easier to
spot!).
Sadly, I only have 845g and so since I only use square cursors in the
ddx I don't actually have the test case I thought I did.
Still,
Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk
(preferrably with the minor suggestions taken into account, if not acted
upon :)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Support for HDMI complaince HPD
On 8/13/2014 11:46 AM, Chris Wilson wrote: On Wed, Aug 13, 2014 at 11:34:02AM +0530, Sharma, Shashank wrote: I know. That is orthogonal to the tweaks I was suggesting. Also if you feel you need to add details to your rationale, then your changelog is incomplete. -Chris Thanks Chris, Please find my comments inline to your previous mail, with suggestions. On 8/12/2014 6:17 PM, Chris Wilson wrote: On Tue, Aug 12, 2014 at 06:08:21PM +0530, shashank.sha...@intel.com wrote: From: Shashank Sharma shashank.sha...@intel.com During the HDMI complaince tests, most of the HDMI analyzers issue a soft HPD, to automate the tests. This process keeps the HDMI cable connected, and DDC chhanel alive. HDMI detect() logic relies on EDID readability, to decide if its a HDMI connect or disconnect event. But in this case, the EDID is always readable, as HDMI cable is physically connected and DDC channel is UP, so detect() always reports a HDMI connect even if its intended to be a HDMI disconnect call. So if HDMI compliance is enabled, we should rely on the live status register, than EDID availability. This patch adds: 1. One kernel command line parameter for i915 module, which indicates if we want to support HDMI compliance, for this platform. I would rather have this as an output property. In fact, I would like the hotplug detection method exposed (and even selectable, but other than this compliance testing, I can't think of a scenario where the kernel shouldn't be able to figure things out for itself). Considering the history of the case, can you please elaborate this suggestion ? I dont think I am getting it right. Instead of (or in addition to) adding a kernel parameter, you add an output property so that it can be adjusted on the fly for individual monitors. Yes, this can be done. This might cause some problems with the first modeset from driver (like fb_console), where there is no userspace to set a property yet, but I think its manageable. 2. A new function to read EDID, which gets called only in case of HDMI compliance support is required. This function reads EDID only if live status register is up. The normal code flow doesn't get effected if kernel command line parameter is not enabled. 3. After various experiments on VLV2 board, with various HDMI monitors we have seen that, with few monitors, the live status register gets set after a slight delay, and then stays reliably. To support such monitors, there is a busy-loop added, with a max delay upto 50ms, with a status check after every 10ms. Please see the comment in intel_hdmi_get_edid. Wouldn't a tidier solution be to delay the hpd by 50-100ms after the hotplug interrupt? That may overcome the issue with the live status for all connectors... -Chris There would be few problems in this case: 1. We dont want this scenario to come into picture for DP, as DP HPD pulse can be as small as 2ms. If the live status is asserted and deasserted within 2ms, do you care? Or perhaps you are talking about something else entirely. Actually what I mean was, in case of compliance, there would be an expectation for Port Up/Down/UP, but what we will get is UP/UP This can fail the test case. 2. Not all the HDMI monitors show this problem, but a significant subset of popular monitors do. 3. In HDCP compliance testing, we send a HPD pulse train of UP and Down, where down pulse can be as small as 100ms. If we increase the delay by 100ms, we will definitely miss the HPD down pulse. And? If the monitor is only plugged in for less than 0.1s do I really want to waste 1-2s of user time reconfiguring the desktop and applications before undoing all the changes? There is no point in compliance testing if it does not actually test the code going to be used. I completely agree, but unfortunately if the test case fails, this would be blocker for commercial projects. The test clearly mentions we should respond properly to the HPD pulse train. 4. The method what we are using is a busy waiting check, where we delay the pulse for 50ms, but take a sample of live_status per 10ms, so if the live status is up with a delay of 20ms, we needn't to waste another 30. Yes. You block using 100% of the cpu in an uninterruptable context for a significant period of time. DO NOT DO THIS. I can very well switch to a sleep, but was not sure if the context switching 5 times for 10ms is beneficial :), please correct me if this is not ok. 5. We want this code routine only to be executed for commercial (like android) platforms, whereas others get the routine code. In other words, you want to ignore years of real world compatibity testing and larger user bases. -Chris I do not mean this for sure, in fact we would be happy if the community accepts this for regular code flow also, but due to their previous objections and stand for not to go for a live_status based solution, made to to come via this route.
Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended
On Tue, Aug 12, 2014 at 09:51:13PM +0100, Chris Wilson wrote: On Tue, Aug 12, 2014 at 10:37:21PM +0200, Daniel Vetter wrote: On Tue, Aug 12, 2014 at 10:30 PM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Tue, Aug 12, 2014 at 10:19:20PM +0200, Daniel Vetter wrote: On Tue, Aug 12, 2014 at 9:33 PM, Paulo Zanoni przan...@gmail.com wrote: 2014-08-12 16:28 GMT-03:00 Chris Wilson ch...@chris-wilson.co.uk: On Tue, Aug 12, 2014 at 04:12:38PM -0300, Paulo Zanoni wrote: But we just get/put RPM around this function, not for the whole time while the object is pinned. Ah misread, saw pin-get, unpin-put and assumed the symmetry. But why unpin then? It doesn't touch any hardware registers. Only because Daniel asked it on a conversation we had on IRC, and I automatically assumed the patch would be rejected if I didn't include it :) Since both you and VIlle pointed that out, I should probably submit yet another version, without the unpin part, and let Daniel choose which one to merge... Hm, I've indeed forgotten about the lazy unbinding. But that poses the question about the final bo unref. For example: 1) create bo, gtt mmap it to force it into existence (and into the global gtt) 2) unmap binding 3) wait for rpm entry 4) unref bo ... causing pte writes for the global gtt unbinding while runtime suspended or not? boom or not boom? Maybe the bug is simply in a different function ;-) Yes. If you get serious about it, you will want to move the lazy stuff into its own workqueue to be run the next time the device is awake. 4b) shrinker happens and unbinds (potentially purgeable) buffer objects. In that case I don't think the core mm would be happy if we'd indefinitely delay this until someone wiggles the mouse. You underestimate just how much we can delay it ;-) But for your next trick, you could unbind the buffer without touching the ptes since the gpu is not using those pages... Diminishing returns I guess. That's actually something I've considered - on gen6+ we don't use the global gtt any more for rendering, so it's fully isolated from whatever userspace can get at. Well ignoring an icky regression from full ppgtt for the aliasing ppgtt binding rules. So we /could/ just leave the stale pte hanging in the air forever. But I'm not sure whether we want to do that for general robustness reasons. Clearing all ptes on device wake-up isn't an option since it takes too much time, and delaying the clearing doesn't look like worth it from a complexity pov. Especially if the compositor wants that memory to render the frame it needs to switch everything on again ... But's true without rpm anyway. It would need to enable the device to render, whether or not the system is thrashing. Yeah, which is also why I don't think just waking the device in the shrinker/bo_free callback is harmful - very likely we didn't wake it for nothing anyway. Oh any my scenario can be fixed with some software rendering into an Xshm or so, and if you assume something else running overnight has thrashed all the memory to swap. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v4 4/4] drm/i915: Add support for variable cursor size on 845/865
From: Ville Syrjälä ville.syrj...@linux.intel.com
845/865 support different cursor sizes as well, albeit a bit differently
than later platforms. Add the necessary code to make them work.
Untested due to lack of hardware.
v2: Warn but accept invalid stride (Chris)
Rewrite the cursor size checks for other platforms (Chris)
v3: More polish and magic to the cursor size checks (Chris)
v4: Moar polish and a comment (Chris)
Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_reg.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 111 +++
drivers/gpu/drm/i915/intel_drv.h | 1 +
3 files changed, 91 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f79c20d..203062e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4128,7 +4128,8 @@ enum punit_power_well {
/* Old style CUR*CNTR flags (desktop 8xx) */
#define CURSOR_ENABLE0x8000
#define CURSOR_GAMMA_ENABLE 0x4000
-#define CURSOR_STRIDE_MASK 0x3000
+#define CURSOR_STRIDE_SHIFT 28
+#define CURSOR_STRIDE(x) ((ffs(x)-9) CURSOR_STRIDE_SHIFT) /*
256,512,1k,2k */
#define CURSOR_PIPE_CSC_ENABLE (124)
#define CURSOR_FORMAT_SHIFT 24
#define CURSOR_FORMAT_MASK (0x07 CURSOR_FORMAT_SHIFT)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index a1cf052..0cefd15 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8005,30 +8005,55 @@ static void i845_update_cursor(struct drm_crtc *crtc,
u32 base)
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- uint32_t cntl;
+ uint32_t cntl = 0, size = 0;
- if (base != intel_crtc-cursor_base) {
- /* On these chipsets we can only modify the base whilst
-* the cursor is disabled.
-*/
- if (intel_crtc-cursor_cntl) {
- I915_WRITE(_CURACNTR, 0);
- POSTING_READ(_CURACNTR);
- intel_crtc-cursor_cntl = 0;
+ if (base) {
+ unsigned int width = intel_crtc-cursor_width;
+ unsigned int height = intel_crtc-cursor_height;
+ unsigned int stride = roundup_pow_of_two(width) * 4;
+
+ switch (stride) {
+ default:
+ WARN_ONCE(1, Invalid cursor width/stride, width=%u,
stride=%u\n,
+ width, stride);
+ stride = 256;
+ /* fallthrough */
+ case 256:
+ case 512:
+ case 1024:
+ case 2048:
+ break;
}
+ cntl |= CURSOR_ENABLE |
+ CURSOR_GAMMA_ENABLE |
+ CURSOR_FORMAT_ARGB |
+ CURSOR_STRIDE(stride);
+
+ size = (height 12) | width;
+ }
+
+ if (intel_crtc-cursor_cntl != 0
+ (intel_crtc-cursor_base != base ||
+intel_crtc-cursor_size != size ||
+intel_crtc-cursor_cntl != cntl)) {
+ /* On these chipsets we can only modify the base/size/stride
+* whilst the cursor is disabled.
+*/
+ I915_WRITE(_CURACNTR, 0);
+ POSTING_READ(_CURACNTR);
+ intel_crtc-cursor_cntl = 0;
+ }
+
+ if (intel_crtc-cursor_base != base)
I915_WRITE(_CURABASE, base);
- POSTING_READ(_CURABASE);
+
+ if (intel_crtc-cursor_size != size) {
+ I915_WRITE(CURSIZE, size);
+ intel_crtc-cursor_size = size;
}
- /* XXX width must be 64, stride 256 = 0x00 28 */
- cntl = 0;
- if (base)
- cntl = (CURSOR_ENABLE |
- CURSOR_GAMMA_ENABLE |
- CURSOR_FORMAT_ARGB);
if (intel_crtc-cursor_cntl != cntl) {
- I915_WRITE(CURSIZE, (64 12) | 64);
I915_WRITE(_CURACNTR, cntl);
POSTING_READ(_CURACNTR);
intel_crtc-cursor_cntl = cntl;
@@ -8127,6 +8152,43 @@ static void intel_crtc_update_cursor(struct drm_crtc
*crtc,
intel_crtc-cursor_base = base;
}
+static bool cursor_size_ok(struct drm_device *dev,
+ uint32_t width, uint32_t height)
+{
+ if (width == 0 || height == 0)
+ return false;
+
+ /*
+* 845g/865g are special in that they are only limited by
+* the width of their cursors, the height is arbitrary up to
+* the precision of the register. Everything else requires
+* square cursors, limited
[Intel-gfx] [PATCH v2 2/4] drm/i915: add some framework for backlight bl_power support
Make backlight class sysfs bl_power a sub-state of backlight enabled, if
a backlight power connector callback is defined. It's up to the
connector callback to handle the sub-state, typically in a way that
respects panel power sequencing.
v2: Post the version that does not oops. *facepalm*.
Signed-off-by: Jani Nikula jani.nik...@intel.com
---
drivers/gpu/drm/i915/intel_drv.h | 2 ++
drivers/gpu/drm/i915/intel_panel.c | 26 ++
2 files changed, 28 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1b3d1d7e466e..43b7b6609f0e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -173,6 +173,8 @@ struct intel_panel {
bool active_low_pwm;
struct backlight_device *device;
} backlight;
+
+ void (*backlight_power)(struct intel_connector *, bool enable);
};
struct intel_connector {
diff --git a/drivers/gpu/drm/i915/intel_panel.c
b/drivers/gpu/drm/i915/intel_panel.c
index 59b028f0b1e8..af5435634929 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -751,6 +751,8 @@ void intel_panel_disable_backlight(struct intel_connector
*connector)
spin_lock_irqsave(dev_priv-backlight_lock, flags);
+ if (panel-backlight.device)
+ panel-backlight.device-props.power = FB_BLANK_POWERDOWN;
panel-backlight.enabled = false;
dev_priv-display.disable_backlight(connector);
@@ -957,6 +959,8 @@ void intel_panel_enable_backlight(struct intel_connector
*connector)
dev_priv-display.enable_backlight(connector);
panel-backlight.enabled = true;
+ if (panel-backlight.device)
+ panel-backlight.device-props.power = FB_BLANK_UNBLANK;
spin_unlock_irqrestore(dev_priv-backlight_lock, flags);
}
@@ -965,6 +969,7 @@ void intel_panel_enable_backlight(struct intel_connector
*connector)
static int intel_backlight_device_update_status(struct backlight_device *bd)
{
struct intel_connector *connector = bl_get_data(bd);
+ struct intel_panel *panel = connector-panel;
struct drm_device *dev = connector-base.dev;
drm_modeset_lock(dev-mode_config.connection_mutex, NULL);
@@ -972,6 +977,22 @@ static int intel_backlight_device_update_status(struct
backlight_device *bd)
bd-props.brightness, bd-props.max_brightness);
intel_panel_set_backlight(connector, bd-props.brightness,
bd-props.max_brightness);
+
+ /*
+* Allow flipping bl_power as a sub-state of enabled. Sadly the
+* backlight class device does not make it easy to to differentiate
+* between callbacks for brightness and bl_power, so our backlight_power
+* callback needs to take this into account.
+*/
+ if (panel-backlight.enabled) {
+ if (panel-backlight_power) {
+ bool enable = bd-props.power == FB_BLANK_UNBLANK;
+ panel-backlight_power(connector, enable);
+ }
+ } else {
+ bd-props.power = FB_BLANK_POWERDOWN;
+ }
+
drm_modeset_unlock(dev-mode_config.connection_mutex);
return 0;
}
@@ -1023,6 +1044,11 @@ static int intel_backlight_device_register(struct
intel_connector *connector)
panel-backlight.level,
props.max_brightness);
+ if (panel-backlight.enabled)
+ props.power = FB_BLANK_UNBLANK;
+ else
+ props.power = FB_BLANK_POWERDOWN;
+
/*
* Note: using the same name independent of the connector prevents
* registration of multiple backlight devices in the driver.
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] linux: Automatically ShareVTs if the VT are already in graphics mode
If the VT we are using is already in KD_GRAPHICS mode, calling SETACTIVE
will silently fail. This leads to an indefinite hang as WAITACTIVE never
returns causing lockups on boot. This issue becomes apparent when the
kernel driver does not install a fbdev for kernel to use for consoles
and plymouth leaves the VT in graphics mode.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: Adam Jackson a...@redhat.com
---
hw/xfree86/os-support/linux/lnx_init.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/hw/xfree86/os-support/linux/lnx_init.c
b/hw/xfree86/os-support/linux/lnx_init.c
index 85709c6..b201310 100644
--- a/hw/xfree86/os-support/linux/lnx_init.c
+++ b/hw/xfree86/os-support/linux/lnx_init.c
@@ -86,6 +86,7 @@ xf86OpenConsole(void)
MessageType from = X_PROBED;
const char *tty0[] = { /dev/tty0, /dev/vc/0, NULL };
const char *vcs[] = { /dev/vc/%d, /dev/tty%d, NULL };
+int kdmode;
if (serverGeneration == 1) {
/*
@@ -188,6 +189,14 @@ xf86OpenConsole(void)
else
activeVT = vts.v_active;
+ /* If the console is already in graphics mode, e.g. if there is
+* no text console in the kernel, we cannot do any VT switching
+* and so must share the vt.
+*/
+ SYSCALL(ret = ioctl(xf86Info.consoleFd, KDGETMODE, kdmode));
+ if (ret == 0 kdmode == KD_GRAPHICS)
+ xf86Info.ShareVTs = TRUE;
+
#if 0
if (!KeepTty) {
/*
--
2.1.0.rc1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: Flush the PTEs after updating them before suspend
As we use WC updates of the PTE, we are responsible for notifying the
hardware when to flush its TLBs. Do so after we zap all the PTEs before
suspend (and the BIOS tries to read our GTT).
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/i915_gem_gtt.c | 14 +-
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d1513e6ea50d..d0222cb9b7a5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2082,7 +2082,7 @@ struct drm_i915_cmd_table {
/* Note that the (struct drm_i915_private *) cast is just to shut up gcc. */
#define __I915__(p)((sizeof(*(p)) == sizeof(struct drm_i915_private)) ? \
-(struct drm_i915_private *)(p) : to_i915(p))
+(struct drm_i915_private *)(p) : to_i915((struct
drm_device *)p))
#define INTEL_INFO(p) (__I915__(p)-info)
#define INTEL_DEVID(p) (INTEL_INFO(p)-device_id)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index f3fd448505f1..b8d5a5d67a73 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1316,6 +1316,16 @@ void i915_check_and_clear_faults(struct drm_device *dev)
POSTING_READ(RING_FAULT_REG(dev_priv-ring[RCS]));
}
+static void i915_ggtt_flush(struct drm_i915_private *dev_priv)
+{
+ if (INTEL_INFO(dev_priv)-gen 6) {
+ intel_gtt_chipset_flush();
+ } else {
+ I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
+ POSTING_READ(GFX_FLSH_CNTL_GEN6);
+ }
+}
+
void i915_gem_suspend_gtt_mappings(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -1332,6 +1342,8 @@ void i915_gem_suspend_gtt_mappings(struct drm_device *dev)
dev_priv-gtt.base.start,
dev_priv-gtt.base.total,
true);
+
+ i915_ggtt_flush(dev_priv);
}
void i915_gem_restore_gtt_mappings(struct drm_device *dev)
@@ -1384,7 +1396,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
gen6_write_pdes(container_of(vm, struct i915_hw_ppgtt, base));
}
- i915_gem_chipset_flush(dev);
+ i915_ggtt_flush(dev_priv);
}
int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
--
2.1.0.rc1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Move i915_gem_chipset_flush to where it belongs
From: Daniel Vetter daniel.vet...@ffwll.ch
That's right, back to where it was!
This effectively reverts
commit 9aa2062bb850e665b3b673b53bd90127d1215490
Author: Daniel Vetter daniel.vet...@ffwll.ch
Date: Wed Aug 6 15:04:46 2014 +0200
drm/i915: Move i915_gem_chipset_flush to where it belongs
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_drv.h | 6 ++
drivers/gpu/drm/i915/i915_gem_gtt.c | 6 --
drivers/gpu/drm/i915/i915_gem_gtt.h | 2 --
3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d0222cb9b7a5..8dbfa18c265b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3007,4 +3007,10 @@ wait_remaining_ms_from_jiffies(unsigned long
timestamp_jiffies, int to_wait_ms)
}
}
+static inline void i915_gem_chipset_flush(struct drm_device *dev)
+{
+ if (INTEL_INFO(dev)-gen 6)
+ intel_gtt_chipset_flush();
+}
+
#endif
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index b8d5a5d67a73..bbab8156b7ca 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2070,12 +2070,6 @@ static void gen6_gmch_remove(struct i915_address_space
*vm)
teardown_scratch_page(vm-dev);
}
-void i915_gem_chipset_flush(struct drm_device *dev)
-{
- if (INTEL_INFO(dev)-gen 6)
- intel_gtt_chipset_flush();
-}
-
static int i915_gmch_probe(struct drm_device *dev,
size_t *gtt_total,
size_t *stolen,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 55de8aec0a33..6280648d4805 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -299,6 +299,4 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev);
int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj);
void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj);
-void i915_gem_chipset_flush(struct drm_device *dev);
-
#endif
--
2.1.0.rc1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Localise the fbdev console lock frobbing
On Tue, Aug 12, 2014 at 03:15:17PM +0100, Chris Wilson wrote:
Rather than take and release the console_lock() around a non-existent
DRM_I915_FBDEV, move the lock acquisation into the callee where it will
be compiled out by the config option entirely. This includes moving the
deferred fb_set_suspend() dance and encapsulating it entirely within
intel_fbdev.c.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_dma.c| 5 ++---
drivers/gpu/drm/i915/i915_drv.c| 26 +-
drivers/gpu/drm/i915/i915_drv.h| 8
drivers/gpu/drm/i915/intel_fbdev.c | 33 +
4 files changed, 36 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 49149406fcd8..d0b3411fc13c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device
*dev)
if (ret)
goto cleanup_irq;
- INIT_WORK(dev_priv-console_resume_work, intel_console_resume);
-
intel_modeset_gem_init(dev);
/* Always safe in the mode setting case. */
@@ -1835,6 +1833,8 @@ int i915_driver_unload(struct drm_device *dev)
return ret;
}
+ flush_scheduled_work();
Tbh I prefer the explicit work flushing and keeping it in dev_priv. We can
shovel it into the I915_FBDEV #ifdef that's already there though.
+
i915_perf_unregister(dev);
intel_fini_runtime_pm(dev_priv);
@@ -1859,7 +1859,6 @@ int i915_driver_unload(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
intel_fbdev_fini(dev);
intel_modeset_cleanup(dev);
- cancel_work_sync(dev_priv-console_resume_work);
/*
* free the memory space allocated for the child device
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 35b119072c11..9adafc7c5819 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -558,9 +558,7 @@ static int i915_drm_freeze(struct drm_device *dev)
intel_uncore_forcewake_reset(dev, false);
intel_opregion_fini(dev);
- console_lock();
intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
This one here must be synchronous.
- console_unlock();
dev_priv-suspend_count++;
@@ -599,18 +597,6 @@ int i915_suspend(struct drm_device *dev, pm_message_t
state)
return 0;
}
-void intel_console_resume(struct work_struct *work)
-{
- struct drm_i915_private *dev_priv =
- container_of(work, struct drm_i915_private,
- console_resume_work);
- struct drm_device *dev = dev_priv-dev;
-
- console_lock();
- intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
- console_unlock();
-}
-
static int i915_drm_thaw_early(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -683,17 +669,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool
restore_gtt_mappings)
intel_opregion_init(dev);
- /*
- * The console lock can be pretty contented on resume due
- * to all the printk activity. Try to keep it out of the hot
- * path of resume if possible.
- */
- if (console_trylock()) {
- intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
- console_unlock();
- } else {
- schedule_work(dev_priv-console_resume_work);
- }
+ intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
mutex_lock(dev_priv-modeset_restore_lock);
dev_priv-modeset_restore = MODESET_DONE;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 517a78e27bbb..e38dd39d84ad 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1603,12 +1603,6 @@ struct drm_i915_private {
struct intel_fbdev *fbdev;
#endif
- /*
- * The console may be contended at resume, but we don't
- * want it to block on it.
- */
- struct work_struct console_resume_work;
-
struct drm_property *broadcast_rgb_property;
struct drm_property *force_audio_property;
@@ -2310,8 +2304,6 @@ extern unsigned long i915_gfx_val(struct
drm_i915_private *dev_priv);
extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
-extern void intel_console_resume(struct work_struct *work);
-
/* i915_irq.c */
void i915_queue_hangcheck(struct drm_device *dev);
__printf(3, 4)
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
b/drivers/gpu/drm/i915/intel_fbdev.c
index 5d879d185fe1..4eecb34a0e32 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Flush the PTEs after updating them before suspend
On Wed, Aug 13, 2014 at 11:51:54AM +0100, Chris Wilson wrote:
As we use WC updates of the PTE, we are responsible for notifying the
hardware when to flush its TLBs. Do so after we zap all the PTEs before
suspend (and the BIOS tries to read our GTT).
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Imo would make more sense to add them to the clear_range functions.
-Daniel
---
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/i915_gem_gtt.c | 14 +-
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d1513e6ea50d..d0222cb9b7a5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2082,7 +2082,7 @@ struct drm_i915_cmd_table {
/* Note that the (struct drm_i915_private *) cast is just to shut up gcc. */
#define __I915__(p) ((sizeof(*(p)) == sizeof(struct drm_i915_private)) ? \
- (struct drm_i915_private *)(p) : to_i915(p))
+ (struct drm_i915_private *)(p) : to_i915((struct
drm_device *)p))
#define INTEL_INFO(p)(__I915__(p)-info)
#define INTEL_DEVID(p) (INTEL_INFO(p)-device_id)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index f3fd448505f1..b8d5a5d67a73 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1316,6 +1316,16 @@ void i915_check_and_clear_faults(struct drm_device
*dev)
POSTING_READ(RING_FAULT_REG(dev_priv-ring[RCS]));
}
+static void i915_ggtt_flush(struct drm_i915_private *dev_priv)
+{
+ if (INTEL_INFO(dev_priv)-gen 6) {
+ intel_gtt_chipset_flush();
+ } else {
+ I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
+ POSTING_READ(GFX_FLSH_CNTL_GEN6);
+ }
+}
+
void i915_gem_suspend_gtt_mappings(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -1332,6 +1342,8 @@ void i915_gem_suspend_gtt_mappings(struct drm_device
*dev)
dev_priv-gtt.base.start,
dev_priv-gtt.base.total,
true);
+
+ i915_ggtt_flush(dev_priv);
}
void i915_gem_restore_gtt_mappings(struct drm_device *dev)
@@ -1384,7 +1396,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device
*dev)
gen6_write_pdes(container_of(vm, struct i915_hw_ppgtt, base));
}
- i915_gem_chipset_flush(dev);
+ i915_ggtt_flush(dev_priv);
}
int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
--
2.1.0.rc1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Localise the fbdev console lock frobbing
On Wed, Aug 13, 2014 at 01:26:40PM +0200, Daniel Vetter wrote:
On Tue, Aug 12, 2014 at 03:15:17PM +0100, Chris Wilson wrote:
Rather than take and release the console_lock() around a non-existent
DRM_I915_FBDEV, move the lock acquisation into the callee where it will
be compiled out by the config option entirely. This includes moving the
deferred fb_set_suspend() dance and encapsulating it entirely within
intel_fbdev.c.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_dma.c| 5 ++---
drivers/gpu/drm/i915/i915_drv.c| 26 +-
drivers/gpu/drm/i915/i915_drv.h| 8
drivers/gpu/drm/i915/intel_fbdev.c | 33 +
4 files changed, 36 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 49149406fcd8..d0b3411fc13c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device
*dev)
if (ret)
goto cleanup_irq;
- INIT_WORK(dev_priv-console_resume_work, intel_console_resume);
-
intel_modeset_gem_init(dev);
/* Always safe in the mode setting case. */
@@ -1835,6 +1833,8 @@ int i915_driver_unload(struct drm_device *dev)
return ret;
}
+ flush_scheduled_work();
Tbh I prefer the explicit work flushing and keeping it in dev_priv. We can
shovel it into the I915_FBDEV #ifdef that's already there though.
+
i915_perf_unregister(dev);
intel_fini_runtime_pm(dev_priv);
@@ -1859,7 +1859,6 @@ int i915_driver_unload(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
intel_fbdev_fini(dev);
intel_modeset_cleanup(dev);
- cancel_work_sync(dev_priv-console_resume_work);
/*
* free the memory space allocated for the child device
diff --git a/drivers/gpu/drm/i915/i915_drv.c
b/drivers/gpu/drm/i915/i915_drv.c
index 35b119072c11..9adafc7c5819 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -558,9 +558,7 @@ static int i915_drm_freeze(struct drm_device *dev)
intel_uncore_forcewake_reset(dev, false);
intel_opregion_fini(dev);
- console_lock();
intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
This one here must be synchronous.
Right, but notice that we synchronize the work afterwards. I thought if
we were careful enough to call the waiter that waited for additional
work items to be completed, it would be sufficient.
+static void intel_fbdev_set_suspend_worker(struct work_struct *work)
+{
+ struct set_suspend_work *ss = container_of(work, typeof(*ss), work);
+ intel_fbdev_set_suspend(ss-dev, ss-state);
This still trylocks so ends up busy-looping through launching more work
items. Probably better also do this synchronously.
Considered that, but I decided that trying to keep the locking
straightforward was better.
I'll look at putting the work item back onto the dev_priv so that we can
cleanly flush it.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Move i915_gem_chipset_flush to where it belongs
On Wed, Aug 13, 2014 at 11:51:55AM +0100, Chris Wilson wrote:
From: Daniel Vetter daniel.vet...@ffwll.ch
That's right, back to where it was!
This effectively reverts
Hm, why?
-Daniel
commit 9aa2062bb850e665b3b673b53bd90127d1215490
Author: Daniel Vetter daniel.vet...@ffwll.ch
Date: Wed Aug 6 15:04:46 2014 +0200
drm/i915: Move i915_gem_chipset_flush to where it belongs
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_drv.h | 6 ++
drivers/gpu/drm/i915/i915_gem_gtt.c | 6 --
drivers/gpu/drm/i915/i915_gem_gtt.h | 2 --
3 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d0222cb9b7a5..8dbfa18c265b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3007,4 +3007,10 @@ wait_remaining_ms_from_jiffies(unsigned long
timestamp_jiffies, int to_wait_ms)
}
}
+static inline void i915_gem_chipset_flush(struct drm_device *dev)
+{
+ if (INTEL_INFO(dev)-gen 6)
+ intel_gtt_chipset_flush();
+}
+
#endif
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index b8d5a5d67a73..bbab8156b7ca 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2070,12 +2070,6 @@ static void gen6_gmch_remove(struct i915_address_space
*vm)
teardown_scratch_page(vm-dev);
}
-void i915_gem_chipset_flush(struct drm_device *dev)
-{
- if (INTEL_INFO(dev)-gen 6)
- intel_gtt_chipset_flush();
-}
-
static int i915_gmch_probe(struct drm_device *dev,
size_t *gtt_total,
size_t *stolen,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 55de8aec0a33..6280648d4805 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -299,6 +299,4 @@ void i915_gem_restore_gtt_mappings(struct drm_device
*dev);
int __must_check i915_gem_gtt_prepare_object(struct drm_i915_gem_object
*obj);
void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj);
-void i915_gem_chipset_flush(struct drm_device *dev);
-
#endif
--
2.1.0.rc1
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 4/4] drm/i915: Add support for variable cursor size on 845/865
On Wed, Aug 13, 2014 at 11:57:05AM +0300, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
845/865 support different cursor sizes as well, albeit a bit differently
than later platforms. Add the necessary code to make them work.
Untested due to lack of hardware.
v2: Warn but accept invalid stride (Chris)
Rewrite the cursor size checks for other platforms (Chris)
v3: More polish and magic to the cursor size checks (Chris)
v4: Moar polish and a comment (Chris)
Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
All merged to dinq, thanks.
-Daniel
---
drivers/gpu/drm/i915/i915_reg.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 111
+++
drivers/gpu/drm/i915/intel_drv.h | 1 +
3 files changed, 91 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f79c20d..203062e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4128,7 +4128,8 @@ enum punit_power_well {
/* Old style CUR*CNTR flags (desktop 8xx) */
#define CURSOR_ENABLE 0x8000
#define CURSOR_GAMMA_ENABLE0x4000
-#define CURSOR_STRIDE_MASK 0x3000
+#define CURSOR_STRIDE_SHIFT28
+#define CURSOR_STRIDE(x) ((ffs(x)-9) CURSOR_STRIDE_SHIFT) /*
256,512,1k,2k */
#define CURSOR_PIPE_CSC_ENABLE (124)
#define CURSOR_FORMAT_SHIFT24
#define CURSOR_FORMAT_MASK (0x07 CURSOR_FORMAT_SHIFT)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index a1cf052..0cefd15 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8005,30 +8005,55 @@ static void i845_update_cursor(struct drm_crtc *crtc,
u32 base)
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- uint32_t cntl;
+ uint32_t cntl = 0, size = 0;
- if (base != intel_crtc-cursor_base) {
- /* On these chipsets we can only modify the base whilst
- * the cursor is disabled.
- */
- if (intel_crtc-cursor_cntl) {
- I915_WRITE(_CURACNTR, 0);
- POSTING_READ(_CURACNTR);
- intel_crtc-cursor_cntl = 0;
+ if (base) {
+ unsigned int width = intel_crtc-cursor_width;
+ unsigned int height = intel_crtc-cursor_height;
+ unsigned int stride = roundup_pow_of_two(width) * 4;
+
+ switch (stride) {
+ default:
+ WARN_ONCE(1, Invalid cursor width/stride, width=%u,
stride=%u\n,
+ width, stride);
+ stride = 256;
+ /* fallthrough */
+ case 256:
+ case 512:
+ case 1024:
+ case 2048:
+ break;
}
+ cntl |= CURSOR_ENABLE |
+ CURSOR_GAMMA_ENABLE |
+ CURSOR_FORMAT_ARGB |
+ CURSOR_STRIDE(stride);
+
+ size = (height 12) | width;
+ }
+
+ if (intel_crtc-cursor_cntl != 0
+ (intel_crtc-cursor_base != base ||
+ intel_crtc-cursor_size != size ||
+ intel_crtc-cursor_cntl != cntl)) {
+ /* On these chipsets we can only modify the base/size/stride
+ * whilst the cursor is disabled.
+ */
+ I915_WRITE(_CURACNTR, 0);
+ POSTING_READ(_CURACNTR);
+ intel_crtc-cursor_cntl = 0;
+ }
+
+ if (intel_crtc-cursor_base != base)
I915_WRITE(_CURABASE, base);
- POSTING_READ(_CURABASE);
+
+ if (intel_crtc-cursor_size != size) {
+ I915_WRITE(CURSIZE, size);
+ intel_crtc-cursor_size = size;
}
- /* XXX width must be 64, stride 256 = 0x00 28 */
- cntl = 0;
- if (base)
- cntl = (CURSOR_ENABLE |
- CURSOR_GAMMA_ENABLE |
- CURSOR_FORMAT_ARGB);
if (intel_crtc-cursor_cntl != cntl) {
- I915_WRITE(CURSIZE, (64 12) | 64);
I915_WRITE(_CURACNTR, cntl);
POSTING_READ(_CURACNTR);
intel_crtc-cursor_cntl = cntl;
@@ -8127,6 +8152,43 @@ static void intel_crtc_update_cursor(struct drm_crtc
*crtc,
intel_crtc-cursor_base = base;
}
+static bool cursor_size_ok(struct drm_device *dev,
+uint32_t width, uint32_t height)
+{
+ if (width == 0 || height == 0)
+ return false;
+
+ /*
+ * 845g/865g are special in that they are only limited by
+ * the width of their cursors, the height is
Re: [Intel-gfx] [PATCH] drm/i915: Localise the fbdev console lock frobbing
On Wed, Aug 13, 2014 at 12:32:33PM +0100, Chris Wilson wrote:
On Wed, Aug 13, 2014 at 01:26:40PM +0200, Daniel Vetter wrote:
On Tue, Aug 12, 2014 at 03:15:17PM +0100, Chris Wilson wrote:
Rather than take and release the console_lock() around a non-existent
DRM_I915_FBDEV, move the lock acquisation into the callee where it will
be compiled out by the config option entirely. This includes moving the
deferred fb_set_suspend() dance and encapsulating it entirely within
intel_fbdev.c.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_dma.c| 5 ++---
drivers/gpu/drm/i915/i915_drv.c| 26 +-
drivers/gpu/drm/i915/i915_drv.h| 8
drivers/gpu/drm/i915/intel_fbdev.c | 33 +
4 files changed, 36 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 49149406fcd8..d0b3411fc13c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device
*dev)
if (ret)
goto cleanup_irq;
- INIT_WORK(dev_priv-console_resume_work, intel_console_resume);
-
intel_modeset_gem_init(dev);
/* Always safe in the mode setting case. */
@@ -1835,6 +1833,8 @@ int i915_driver_unload(struct drm_device *dev)
return ret;
}
+ flush_scheduled_work();
Tbh I prefer the explicit work flushing and keeping it in dev_priv. We can
shovel it into the I915_FBDEV #ifdef that's already there though.
+
i915_perf_unregister(dev);
intel_fini_runtime_pm(dev_priv);
@@ -1859,7 +1859,6 @@ int i915_driver_unload(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
intel_fbdev_fini(dev);
intel_modeset_cleanup(dev);
- cancel_work_sync(dev_priv-console_resume_work);
/*
* free the memory space allocated for the child device
diff --git a/drivers/gpu/drm/i915/i915_drv.c
b/drivers/gpu/drm/i915/i915_drv.c
index 35b119072c11..9adafc7c5819 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -558,9 +558,7 @@ static int i915_drm_freeze(struct drm_device *dev)
intel_uncore_forcewake_reset(dev, false);
intel_opregion_fini(dev);
- console_lock();
intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
This one here must be synchronous.
Right, but notice that we synchronize the work afterwards. I thought if
we were careful enough to call the waiter that waited for additional
work items to be completed, it would be sufficient.
Given that the work creates new work when it can't do it's job I don't
think that flush is sufficient. Iirc that only flush out pending stuff,
not newly queued work.
+static void intel_fbdev_set_suspend_worker(struct work_struct *work)
+{
+ struct set_suspend_work *ss = container_of(work, typeof(*ss), work);
+ intel_fbdev_set_suspend(ss-dev, ss-state);
This still trylocks so ends up busy-looping through launching more work
items. Probably better also do this synchronously.
Considered that, but I decided that trying to keep the locking
straightforward was better.
Open-coding console_lock(); fb_set_suspend(); console_unlock(); doesn't
really look onerous to me.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Replace __I915__ with typesafe variant
On Wed, Aug 13, 2014 at 12:14:12PM +0100, Chris Wilson wrote:
Ville pointed out the GCCism __builtin_types_compatible_p() that we
could use to replace our heavily casted presumption __I915__ macro that
was based on comparing struct sizes.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Yeah, that looks a bit less crazy. Queued for -next, thanks for the patch.
-Daniel
---
drivers/gpu/drm/i915/i915_dma.c | 3 ---
drivers/gpu/drm/i915/i915_drv.h | 12 ++--
2 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 5e3b1390ac5c..b2e4dbcb77ae 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1582,9 +1582,6 @@ int i915_driver_load(struct drm_device *dev, unsigned
long flags)
if (!drm_core_check_feature(dev, DRIVER_MODESET) !dev-agp)
return -EINVAL;
- /* For the ugly agnostic INTEL_INFO macro */
- BUILD_BUG_ON(sizeof(*dev_priv) == sizeof(*dev));
-
BUILD_BUG_ON(I915_NUM_RINGS = (1 I915_NUM_RING_BITS));
dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d1513e6ea50d..b6dcbf79b31c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2081,8 +2081,16 @@ struct drm_i915_cmd_table {
};
/* Note that the (struct drm_i915_private *) cast is just to shut up gcc. */
-#define __I915__(p) ((sizeof(*(p)) == sizeof(struct drm_i915_private)) ? \
- (struct drm_i915_private *)(p) : to_i915(p))
+#define __I915__(p) ({ \
+ struct drm_i915_private *__p; \
+ if (__builtin_types_compatible_p(typeof(*p), struct drm_i915_private)) \
+ __p = (struct drm_i915_private *)p; \
+ else if (__builtin_types_compatible_p(typeof(*p), struct drm_device)) \
+ __p = to_i915((struct drm_device *)p); \
+ else \
+ BUILD_BUG(); \
+ __p; \
+})
#define INTEL_INFO(p)(__I915__(p)-info)
#define INTEL_DEVID(p) (INTEL_INFO(p)-device_id)
--
2.1.0.rc1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Move i915_gem_chipset_flush to where it belongs
On Wed, Aug 13, 2014 at 01:32:31PM +0200, Daniel Vetter wrote: On Wed, Aug 13, 2014 at 11:51:55AM +0100, Chris Wilson wrote: From: Daniel Vetter daniel.vet...@ffwll.ch That's right, back to where it was! This effectively reverts Hm, why? Becuase it doesn't belong in i915_gem_gtt.c since the flush is not about flushing the GGTT and it is not used in i915_gem_gtt.c -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Flush the PTEs after updating them before suspend
On Wed, Aug 13, 2014 at 01:32:08PM +0200, Daniel Vetter wrote: On Wed, Aug 13, 2014 at 11:51:54AM +0100, Chris Wilson wrote: As we use WC updates of the PTE, we are responsible for notifying the hardware when to flush its TLBs. Do so after we zap all the PTEs before suspend (and the BIOS tries to read our GTT). Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Imo would make more sense to add them to the clear_range functions. They are not required in the clear_range function. Here it is only required in the i915_gem_suspend_gtt_mapping() because a third party is involved. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Localise the fbdev console lock frobbing
On Wed, Aug 13, 2014 at 01:39:22PM +0200, Daniel Vetter wrote: On Wed, Aug 13, 2014 at 12:32:33PM +0100, Chris Wilson wrote: On Wed, Aug 13, 2014 at 01:26:40PM +0200, Daniel Vetter wrote: This one here must be synchronous. Right, but notice that we synchronize the work afterwards. I thought if we were careful enough to call the waiter that waited for additional work items to be completed, it would be sufficient. Given that the work creates new work when it can't do it's job I don't think that flush is sufficient. Iirc that only flush out pending stuff, not newly queued work. I had a fancy while(flush_work()) ; all planned out. But that is effectively a busy-wait during suspend and the console_lock() is contended, so that got binned. :( -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Localise the fbdev console lock frobbing
On Wed, Aug 13, 2014 at 01:39:22PM +0200, Daniel Vetter wrote:
On Wed, Aug 13, 2014 at 12:32:33PM +0100, Chris Wilson wrote:
On Wed, Aug 13, 2014 at 01:26:40PM +0200, Daniel Vetter wrote:
On Tue, Aug 12, 2014 at 03:15:17PM +0100, Chris Wilson wrote:
+static void intel_fbdev_set_suspend_worker(struct work_struct *work)
+{
+ struct set_suspend_work *ss = container_of(work, typeof(*ss),
work);
+ intel_fbdev_set_suspend(ss-dev, ss-state);
This still trylocks so ends up busy-looping through launching more work
items. Probably better also do this synchronously.
Considered that, but I decided that trying to keep the locking
straightforward was better.
Open-coding console_lock(); fb_set_suspend(); console_unlock(); doesn't
really look onerous to me.
Apart from intel_fbdev_set_suspend does a little bit more on resume.
It's simple enough to add an extra parameter to tell
intel_fbdev_set_suspend to wait.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Localise the fbdev console lock frobbing
Rather than take and release the console_lock() around a non-existent
DRM_I915_FBDEV, move the lock acquisation into the callee where it will
be compiled out by the config option entirely. This includes moving the
deferred fb_set_suspend() dance and encapsulating it entirely within
intel_fbdev.c.
v2: Use an integral work item so that we can explicitly flush the work
upon suspend/unload.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_dma.c| 3 ---
drivers/gpu/drm/i915/i915_drv.c| 28 ++---
drivers/gpu/drm/i915/i915_drv.h| 9 +---
drivers/gpu/drm/i915/intel_drv.h | 4 ++--
drivers/gpu/drm/i915/intel_fbdev.c | 42 +-
5 files changed, 46 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 860da759ae34..fbab9370cb5c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device *dev)
if (ret)
goto cleanup_irq;
- INIT_WORK(dev_priv-console_resume_work, intel_console_resume);
-
intel_modeset_gem_init(dev);
/* Always safe in the mode setting case. */
@@ -1857,7 +1855,6 @@ int i915_driver_unload(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
intel_fbdev_fini(dev);
intel_modeset_cleanup(dev);
- cancel_work_sync(dev_priv-console_resume_work);
/*
* free the memory space allocated for the child device
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 35b119072c11..7c7bb94d7654 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -558,9 +558,7 @@ static int i915_drm_freeze(struct drm_device *dev)
intel_uncore_forcewake_reset(dev, false);
intel_opregion_fini(dev);
- console_lock();
- intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
- console_unlock();
+ intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
dev_priv-suspend_count++;
@@ -599,18 +597,6 @@ int i915_suspend(struct drm_device *dev, pm_message_t
state)
return 0;
}
-void intel_console_resume(struct work_struct *work)
-{
- struct drm_i915_private *dev_priv =
- container_of(work, struct drm_i915_private,
-console_resume_work);
- struct drm_device *dev = dev_priv-dev;
-
- console_lock();
- intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
- console_unlock();
-}
-
static int i915_drm_thaw_early(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -683,17 +669,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool
restore_gtt_mappings)
intel_opregion_init(dev);
- /*
-* The console lock can be pretty contented on resume due
-* to all the printk activity. Try to keep it out of the hot
-* path of resume if possible.
-*/
- if (console_trylock()) {
- intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
- console_unlock();
- } else {
- schedule_work(dev_priv-console_resume_work);
- }
+ intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false);
mutex_lock(dev_priv-modeset_restore_lock);
dev_priv-modeset_restore = MODESET_DONE;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 19aa07b8d5dd..00f343116aad 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1601,14 +1601,9 @@ struct drm_i915_private {
#ifdef CONFIG_DRM_I915_FBDEV
/* list of fbdev register on this device */
struct intel_fbdev *fbdev;
+ struct work_struct fbdev_suspend_work;
#endif
- /*
-* The console may be contended at resume, but we don't
-* want it to block on it.
-*/
- struct work_struct console_resume_work;
-
struct drm_property *broadcast_rgb_property;
struct drm_property *force_audio_property;
@@ -2308,8 +2303,6 @@ extern unsigned long i915_gfx_val(struct drm_i915_private
*dev_priv);
extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
-extern void intel_console_resume(struct work_struct *work);
-
/* i915_irq.c */
void i915_queue_hangcheck(struct drm_device *dev);
__printf(3, 4)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index feb16c730d91..ffc9288ce3f3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -960,7 +960,7 @@ void intel_dvo_init(struct drm_device *dev);
extern int intel_fbdev_init(struct drm_device *dev);
extern
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Support for HDMI complaince HPD
On Wed, Aug 13, 2014 at 01:12:12PM +0530, Sharma, Shashank wrote: On 8/13/2014 11:46 AM, Chris Wilson wrote: On Wed, Aug 13, 2014 at 11:34:02AM +0530, Sharma, Shashank wrote: 5. We want this code routine only to be executed for commercial (like android) platforms, whereas others get the routine code. In other words, you want to ignore years of real world compatibity testing and larger user bases. I do not mean this for sure, in fact we would be happy if the community accepts this for regular code flow also, but due to their previous objections and stand for not to go for a live_status based solution, made to to come via this route. Please suggest how can we do this in better way. So I'm grumpily ok with a special validation mode to get a sticker. Occasionally there's a business need for those stickers, and it kinda makes sense to merge that code upstream. But in the end reality in the form of existing broken hdmi screens out there wins. Always. And you actually learned that already by adding hacks to your validation-only path to make it actually work in reality. Sooner or later you need to add all the other crap we've accumulated too, or the driver will simply not work. Which means we'll actually not end up with a restricted validation-only hack, but duplicated codebugs. I'm _not_ going to have 2 separate paths with real-world quirks. If there's anything actually broken with the current code in upstream, we need to fix that one, not add a new one because that's easier. Because it's not. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Optimize HDMI EDID reads
On Tue, Aug 12, 2014 at 06:08:20PM +0530, shashank.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@intel.com
The current hdmi_detect() function is getting called from
many places, few of these are:
1. HDMI hot plug interrupt bottom half
2. get_resources() IOCTL family
3. drm_helper_probe_single_connector_modes() family
4. output_poll_execute()
5. status_show() etc...
Every time this function is called, it goes and reads HDMI EDID over
DDC channel. Ideally, reading EDID is only required when there is a
real hot plug, and then for all IOCTL and userspace detect functions
can be answered using this same EDID.
The current patch adds EDID caching for a finite duration (1 minute)
This is how it works:
1. With in this caching duration, when usespace get_resource and other
modeset_detect calls get called, we can use the cached EDID.
2. Even the get_mode function can use the cached EDID.
3. A delayed work will clear the cached EDID after the timeout.
4. If there is a real HDMI hotplug, within the caching duration, the
cached EDID is updates, and a new delayed work is scheduled.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
This has a bunch of changes compared to what I've proposed, and so will
not actually work. Also, keying off the source platform (with the gen6
checks) is useless if we're dealing with random brokeness in existing hdmi
sinks here.
-Daniel
---
drivers/gpu/drm/i915/intel_drv.h | 4 ++
drivers/gpu/drm/i915/intel_hdmi.c | 92
---
2 files changed, 90 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h
b/drivers/gpu/drm/i915/intel_drv.h
index 28d185d..185a45a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -110,6 +110,8 @@
#define INTEL_DSI_VIDEO_MODE 0
#define INTEL_DSI_COMMAND_MODE 1
+#define INTEL_HDMI_EDID_CACHING_SEC 60
+
struct intel_framebuffer {
struct drm_framebuffer base;
struct drm_i915_gem_object *obj;
@@ -507,6 +509,8 @@ struct intel_hdmi {
enum hdmi_force_audio force_audio;
bool rgb_quant_range_selectable;
enum hdmi_picture_aspect aspect_ratio;
+ struct edid *edid;
+ struct delayed_work edid_work;
void (*write_infoframe)(struct drm_encoder *encoder,
enum hdmi_infoframe_type type,
const void *frame, ssize_t len);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
b/drivers/gpu/drm/i915/intel_hdmi.c
index 5f47d35..8dc3970 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -962,6 +962,22 @@ bool intel_hdmi_compute_config(struct intel_encoder
*encoder,
return true;
}
+/* Work function to invalidate EDID caching */
+static void intel_hdmi_invalidate_edid(struct work_struct *work)
+{
+ struct intel_hdmi *intel_hdmi = container_of(to_delayed_work(work),
+ struct intel_hdmi, edid_work);
+ struct drm_device *dev = intel_hdmi_to_dev(intel_hdmi);
+ struct drm_mode_config *mode_config = dev-mode_config;
+
+ mutex_lock(mode_config-mutex);
+ /* Checkpatch says kfree is NULL protected */
+ kfree(intel_hdmi-edid);
+ intel_hdmi-edid = NULL;
+ mutex_unlock(mode_config-mutex);
+ DRM_DEBUG_DRIVER(cleaned up cached EDID\n);
+}
+
static enum drm_connector_status
intel_hdmi_detect(struct drm_connector *connector, bool force)
{
@@ -978,15 +994,58 @@ intel_hdmi_detect(struct drm_connector *connector, bool
force)
DRM_DEBUG_KMS([CONNECTOR:%d:%s]\n,
connector-base.id, connector-name);
+ /*
+ * hdmi_detect() gets called from both get_resource()
+ * and HDMI hpd bottom half work function.
+ * Its not required to read EDID for every detect call until it's is
+ * from a hot plug. Lets cache the EDID as soon as we get
+ * a HPD, and then try to re-use this for all the non hpd detact calls
+ * coming with in a finite duration.
+ */
+ if (INTEL_INFO(dev)-gen 6)
+ /* Do not break old platforms */
+ goto skip_optimization;
+
+ /* If call is from HPD, do check real status by reading EDID */
+ if (!force)
+ goto skip_optimization;
+
+ /* This is a non-hpd call, lets see if we can optimize this */
+ if (intel_hdmi-edid) {
+ /*
+ * So this is a non-hpd call, within the duration when
+ * EDID caching is valid. So previous status (valid)
+ * of connector is re-usable.
+ */
+ if (connector-status != connector_status_unknown) {
+ DRM_DEBUG_DRIVER(Reporting force status\n);
+ return connector-status;
+ }
+ }
+
+skip_optimization:
power_domain = intel_display_port_power_domain(intel_encoder);
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Move i915_gem_chipset_flush to where it belongs
On Wed, Aug 13, 2014 at 12:45:11PM +0100, Chris Wilson wrote: On Wed, Aug 13, 2014 at 01:32:31PM +0200, Daniel Vetter wrote: On Wed, Aug 13, 2014 at 11:51:55AM +0100, Chris Wilson wrote: From: Daniel Vetter daniel.vet...@ffwll.ch That's right, back to where it was! This effectively reverts Hm, why? Becuase it doesn't belong in i915_gem_gtt.c since the flush is not about flushing the GGTT and it is not used in i915_gem_gtt.c Indeed, makes sense. Dropped my original patch from dinq. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Localise the fbdev console lock frobbing
On Wed, Aug 13, 2014 at 01:09:46PM +0100, Chris Wilson wrote:
Rather than take and release the console_lock() around a non-existent
DRM_I915_FBDEV, move the lock acquisation into the callee where it will
be compiled out by the config option entirely. This includes moving the
deferred fb_set_suspend() dance and encapsulating it entirely within
intel_fbdev.c.
v2: Use an integral work item so that we can explicitly flush the work
upon suspend/unload.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_dma.c| 3 ---
drivers/gpu/drm/i915/i915_drv.c| 28 ++---
drivers/gpu/drm/i915/i915_drv.h| 9 +---
drivers/gpu/drm/i915/intel_drv.h | 4 ++--
drivers/gpu/drm/i915/intel_fbdev.c | 42
+-
5 files changed, 46 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 860da759ae34..fbab9370cb5c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device
*dev)
if (ret)
goto cleanup_irq;
- INIT_WORK(dev_priv-console_resume_work, intel_console_resume);
-
intel_modeset_gem_init(dev);
/* Always safe in the mode setting case. */
@@ -1857,7 +1855,6 @@ int i915_driver_unload(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
intel_fbdev_fini(dev);
intel_modeset_cleanup(dev);
- cancel_work_sync(dev_priv-console_resume_work);
This one here seems to have been lost - shouldn't we move this to
fbdev_fini instead? Otherwise this looks good imo, so I'll fix that up
while merging if you're ok.
-Daniel
/*
* free the memory space allocated for the child device
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 35b119072c11..7c7bb94d7654 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -558,9 +558,7 @@ static int i915_drm_freeze(struct drm_device *dev)
intel_uncore_forcewake_reset(dev, false);
intel_opregion_fini(dev);
- console_lock();
- intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
- console_unlock();
+ intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
dev_priv-suspend_count++;
@@ -599,18 +597,6 @@ int i915_suspend(struct drm_device *dev, pm_message_t
state)
return 0;
}
-void intel_console_resume(struct work_struct *work)
-{
- struct drm_i915_private *dev_priv =
- container_of(work, struct drm_i915_private,
- console_resume_work);
- struct drm_device *dev = dev_priv-dev;
-
- console_lock();
- intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
- console_unlock();
-}
-
static int i915_drm_thaw_early(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -683,17 +669,7 @@ static int __i915_drm_thaw(struct drm_device *dev, bool
restore_gtt_mappings)
intel_opregion_init(dev);
- /*
- * The console lock can be pretty contented on resume due
- * to all the printk activity. Try to keep it out of the hot
- * path of resume if possible.
- */
- if (console_trylock()) {
- intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING);
- console_unlock();
- } else {
- schedule_work(dev_priv-console_resume_work);
- }
+ intel_fbdev_set_suspend(dev, FBINFO_STATE_RUNNING, false);
mutex_lock(dev_priv-modeset_restore_lock);
dev_priv-modeset_restore = MODESET_DONE;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 19aa07b8d5dd..00f343116aad 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1601,14 +1601,9 @@ struct drm_i915_private {
#ifdef CONFIG_DRM_I915_FBDEV
/* list of fbdev register on this device */
struct intel_fbdev *fbdev;
+ struct work_struct fbdev_suspend_work;
#endif
- /*
- * The console may be contended at resume, but we don't
- * want it to block on it.
- */
- struct work_struct console_resume_work;
-
struct drm_property *broadcast_rgb_property;
struct drm_property *force_audio_property;
@@ -2308,8 +2303,6 @@ extern unsigned long i915_gfx_val(struct
drm_i915_private *dev_priv);
extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
-extern void intel_console_resume(struct work_struct *work);
-
/* i915_irq.c */
void i915_queue_hangcheck(struct drm_device *dev);
__printf(3, 4)
diff --git a/drivers/gpu/drm/i915/intel_drv.h
Re: [Intel-gfx] [PATCH] drm/i915: Localise the fbdev console lock frobbing
On Wed, Aug 13, 2014 at 02:46:53PM +0200, Daniel Vetter wrote:
On Wed, Aug 13, 2014 at 01:09:46PM +0100, Chris Wilson wrote:
Rather than take and release the console_lock() around a non-existent
DRM_I915_FBDEV, move the lock acquisation into the callee where it will
be compiled out by the config option entirely. This includes moving the
deferred fb_set_suspend() dance and encapsulating it entirely within
intel_fbdev.c.
v2: Use an integral work item so that we can explicitly flush the work
upon suspend/unload.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_dma.c| 3 ---
drivers/gpu/drm/i915/i915_drv.c| 28 ++---
drivers/gpu/drm/i915/i915_drv.h| 9 +---
drivers/gpu/drm/i915/intel_drv.h | 4 ++--
drivers/gpu/drm/i915/intel_fbdev.c | 42
+-
5 files changed, 46 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 860da759ae34..fbab9370cb5c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device
*dev)
if (ret)
goto cleanup_irq;
- INIT_WORK(dev_priv-console_resume_work, intel_console_resume);
-
intel_modeset_gem_init(dev);
/* Always safe in the mode setting case. */
@@ -1857,7 +1855,6 @@ int i915_driver_unload(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
intel_fbdev_fini(dev);
intel_modeset_cleanup(dev);
- cancel_work_sync(dev_priv-console_resume_work);
This one here seems to have been lost - shouldn't we move this to
fbdev_fini instead? Otherwise this looks good imo, so I'll fix that up
while merging if you're ok.
Hmm, I counted on us calling suspend before intel_fbdev_fini() in
unload. Is this not the case? Otherwise I think we should be suspending
the console.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Localise the fbdev console lock frobbing
On Wed, Aug 13, 2014 at 01:58:30PM +0100, Chris Wilson wrote:
On Wed, Aug 13, 2014 at 02:46:53PM +0200, Daniel Vetter wrote:
On Wed, Aug 13, 2014 at 01:09:46PM +0100, Chris Wilson wrote:
Rather than take and release the console_lock() around a non-existent
DRM_I915_FBDEV, move the lock acquisation into the callee where it will
be compiled out by the config option entirely. This includes moving the
deferred fb_set_suspend() dance and encapsulating it entirely within
intel_fbdev.c.
v2: Use an integral work item so that we can explicitly flush the work
upon suspend/unload.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_dma.c| 3 ---
drivers/gpu/drm/i915/i915_drv.c| 28 ++---
drivers/gpu/drm/i915/i915_drv.h| 9 +---
drivers/gpu/drm/i915/intel_drv.h | 4 ++--
drivers/gpu/drm/i915/intel_fbdev.c | 42
+-
5 files changed, 46 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 860da759ae34..fbab9370cb5c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct drm_device
*dev)
if (ret)
goto cleanup_irq;
- INIT_WORK(dev_priv-console_resume_work, intel_console_resume);
-
intel_modeset_gem_init(dev);
/* Always safe in the mode setting case. */
@@ -1857,7 +1855,6 @@ int i915_driver_unload(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
intel_fbdev_fini(dev);
intel_modeset_cleanup(dev);
- cancel_work_sync(dev_priv-console_resume_work);
This one here seems to have been lost - shouldn't we move this to
fbdev_fini instead? Otherwise this looks good imo, so I'll fix that up
while merging if you're ok.
Hmm, I counted on us calling suspend before intel_fbdev_fini() in
unload. Is this not the case? Otherwise I think we should be suspending
the console.
We don't do any of that. Unregistering the fbdev emulation in fbdev_fini
should be enough though to stop anyone from actually using the gpu, and
we're careful to unregister other console drivers like vgacon to make sure
no one else can grow stupid ideas.
So I think the only concern really is that the work item completes before
the text section disappears, and maybe (on multi-gpu systems) that we
don't keep all fbdev devices disabled forever after a racy module unload.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Localise the fbdev console lock frobbing
On Wed, Aug 13, 2014 at 03:04:02PM +0200, Daniel Vetter wrote:
On Wed, Aug 13, 2014 at 01:58:30PM +0100, Chris Wilson wrote:
On Wed, Aug 13, 2014 at 02:46:53PM +0200, Daniel Vetter wrote:
On Wed, Aug 13, 2014 at 01:09:46PM +0100, Chris Wilson wrote:
Rather than take and release the console_lock() around a non-existent
DRM_I915_FBDEV, move the lock acquisation into the callee where it will
be compiled out by the config option entirely. This includes moving the
deferred fb_set_suspend() dance and encapsulating it entirely within
intel_fbdev.c.
v2: Use an integral work item so that we can explicitly flush the work
upon suspend/unload.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_dma.c| 3 ---
drivers/gpu/drm/i915/i915_drv.c| 28 ++---
drivers/gpu/drm/i915/i915_drv.h| 9 +---
drivers/gpu/drm/i915/intel_drv.h | 4 ++--
drivers/gpu/drm/i915/intel_fbdev.c | 42
+-
5 files changed, 46 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 860da759ae34..fbab9370cb5c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct
drm_device *dev)
if (ret)
goto cleanup_irq;
- INIT_WORK(dev_priv-console_resume_work, intel_console_resume);
-
intel_modeset_gem_init(dev);
/* Always safe in the mode setting case. */
@@ -1857,7 +1855,6 @@ int i915_driver_unload(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
intel_fbdev_fini(dev);
intel_modeset_cleanup(dev);
- cancel_work_sync(dev_priv-console_resume_work);
This one here seems to have been lost - shouldn't we move this to
fbdev_fini instead? Otherwise this looks good imo, so I'll fix that up
while merging if you're ok.
Hmm, I counted on us calling suspend before intel_fbdev_fini() in
unload. Is this not the case? Otherwise I think we should be suspending
the console.
We don't do any of that. Unregistering the fbdev emulation in fbdev_fini
should be enough though to stop anyone from actually using the gpu, and
we're careful to unregister other console drivers like vgacon to make sure
no one else can grow stupid ideas.
So I think the only concern really is that the work item completes before
the text section disappears, and maybe (on multi-gpu systems) that we
don't keep all fbdev devices disabled forever after a racy module unload.
Ok, I followed the unload-fbdev_fini-unregister_console and concur
that all you want is the flush_work() in fbdev_fini.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 21/43] drm/i915/bdw: Emission of requests with logical rings
-Original Message-
From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
Vetter
Sent: Monday, August 11, 2014 9:57 PM
To: Daniel, Thomas
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 21/43] drm/i915/bdw: Emission of requests
with logical rings
On Thu, Jul 24, 2014 at 05:04:29PM +0100, Thomas Daniel wrote:
From: Oscar Mateo oscar.ma...@intel.com
On a previous iteration of this patch, I created an Execlists version
of __i915_add_request and asbtracted it away as a vfunc. Daniel Vetter
wondered then why that was needed:
with the clean split in command submission I expect every function to
know wether it'll submit to an lrc (everything in
intel_lrc.c) or wether it'll submit to a legacy ring (existing code),
so I don't see a need for an add_request vfunc.
The honest, hairy truth is that this patch is the glue keeping the
whole logical ring puzzle together:
Oops, I didn't spot this and it's indeed not terribly pretty.
Are you saying you want to go back to a vfunc for add_request?
- i915_add_request is used by intel_ring_idle, which in turn is
used by i915_gpu_idle, which in turn is used in several places
inside the eviction and gtt codes.
This should probably be folded in with the lrc specific version of stop_rings
and so should work out.
- Also, it is used by i915_gem_check_olr, which is littered all
over i915_gem.c
We now always preallocate the request struct, so olr is officially dead.
Well almost, except for non-execbuf stuff that we emit through the rings.
Which is nothing for lrc/execlist mode.
Also there's the icky-bitty problem with ringbuf-ctx which makes this patch
not apply any more. I think we need to revise or at least discuss a bit.
Context is required when doing an advance_and_submit so that we know
which context to queue in the execlist queue.
- ...
If I were to duplicate all the code that directly or indirectly uses
__i915_add_request, I'll end up creating a separate driver.
To show the differences between the existing legacy version and the
new Execlists one, this time I have special-cased __i915_add_request
instead of adding an add_request vfunc. I hope this helps to untangle
this Gordian knot.
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_gem.c | 72
--
drivers/gpu/drm/i915/intel_lrc.c | 30 +---
drivers/gpu/drm/i915/intel_lrc.h |1 +
3 files changed, 80 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c index 9560b40..1c83b9c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2327,10 +2327,21 @@ int __i915_add_request(struct intel_engine_cs
*ring, {
struct drm_i915_private *dev_priv = ring-dev-dev_private;
struct drm_i915_gem_request *request;
+ struct intel_ringbuffer *ringbuf;
u32 request_ring_position, request_start;
int ret;
- request_start = intel_ring_get_tail(ring-buffer);
+ request = ring-preallocated_lazy_request;
+ if (WARN_ON(request == NULL))
+ return -ENOMEM;
+
+ if (i915.enable_execlists) {
+ struct intel_context *ctx = request-ctx;
+ ringbuf = ctx-engine[ring-id].ringbuf;
+ } else
+ ringbuf = ring-buffer;
+
+ request_start = intel_ring_get_tail(ringbuf);
/*
* Emit any outstanding flushes - execbuf can fail to emit the flush
* after having emitted the batchbuffer command. Hence we need to
fix @@ -2338,24 +2349,32 @@ int __i915_add_request(struct
intel_engine_cs *ring,
* is that the flush _must_ happen before the next request, no
matter
* what.
*/
- ret = intel_ring_flush_all_caches(ring);
- if (ret)
- return ret;
-
- request = ring-preallocated_lazy_request;
- if (WARN_ON(request == NULL))
- return -ENOMEM;
+ if (i915.enable_execlists) {
+ ret = logical_ring_flush_all_caches(ringbuf);
+ if (ret)
+ return ret;
+ } else {
+ ret = intel_ring_flush_all_caches(ring);
+ if (ret)
+ return ret;
+ }
/* Record the position of the start of the request so that
* should we detect the updated seqno part-way through the
* GPU processing the request, we never over-estimate the
* position of the head.
*/
- request_ring_position = intel_ring_get_tail(ring-buffer);
+ request_ring_position = intel_ring_get_tail(ringbuf);
- ret = ring-add_request(ring);
- if (ret)
- return ret;
+ if (i915.enable_execlists) {
+ ret = ring-emit_request(ringbuf);
+ if (ret)
+ return ret;
+ } else {
+ ret = ring-add_request(ring);
+ if
Re: [Intel-gfx] [PATCH 08/43] drm/i915/bdw: Add a context and an engine pointers to the ringbuffer
-Original Message-
From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
Vetter
Sent: Monday, August 11, 2014 3:21 PM
To: Daniel, Thomas
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 08/43] drm/i915/bdw: Add a context and an
engine pointers to the ringbuffer
On Mon, Aug 11, 2014 at 04:14:13PM +0200, Daniel Vetter wrote:
On Thu, Jul 24, 2014 at 05:04:16PM +0100, Thomas Daniel wrote:
From: Oscar Mateo oscar.ma...@intel.com
Any given ringbuffer is unequivocally tied to one context and one engine.
By setting the appropriate pointers to them, the ringbuffer struct
holds all the infromation you might need to submit a workload for
processing, Execlists style.
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/intel_lrc.c|2 ++
drivers/gpu/drm/i915/intel_ringbuffer.c |2 ++
drivers/gpu/drm/i915/intel_ringbuffer.h |3 +++
3 files changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c
b/drivers/gpu/drm/i915/intel_lrc.c
index 0a12b8c..2eb7db6 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -132,6 +132,8 @@ int intel_lr_context_deferred_create(struct
intel_context *ctx,
return ret;
}
+ ringbuf-ring = ring;
+ ringbuf-ctx = ctx;
ringbuf-size = 32 * PAGE_SIZE;
ringbuf-effective_size = ringbuf-size;
ringbuf-head = 0;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 01e9840..279dda4 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1570,6 +1570,8 @@ static int intel_init_ring_buffer(struct
drm_device *dev,
INIT_LIST_HEAD(ring-active_list);
INIT_LIST_HEAD(ring-request_list);
ringbuf-size = 32 * PAGE_SIZE;
+ ringbuf-ring = ring;
+ ringbuf-ctx = ring-default_context;
That doesn't make a terribly lot of sense tbh. I fear it's one of
these slight confusions which will take tons of patches to clean up.
Why exactly do we need the ring-ctx pointer?
If we only need this for lrc I want to name it accordingly, to make
sure legacy code doesn't grow stupid ideas. And also we should only
initialize this in the lrc ctx init then.
All patches up to this one merged.
Ok, I've discussed this quickly with Damien on irc. We decided to cut away
the ring-ctx part of this patch for now to be able to move on.
-Daniel
As you've seen, removing ringbuffer-ctx causes serious problems with the
plumbing later on. This can be renamed (perhaps to lrc) and removed from
legacy init.
Each ring buffer belongs to a specific context - it makes sense to me to
keep this information within the ringbuffer structure so that we don't have
to pass the context pointer around everywhere.
Thomas.
-Daniel
memset(ring-semaphore.sync_seqno, 0,
sizeof(ring-semaphore.sync_seqno));
init_waitqueue_head(ring-irq_queue);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 053d004..be40788 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -88,6 +88,9 @@ struct intel_ringbuffer {
struct drm_i915_gem_object *obj;
void __iomem *virtual_start;
+ struct intel_engine_cs *ring;
+ struct intel_context *ctx;
+
u32 head;
u32 tail;
int space;
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Localise the fbdev console lock frobbing
On Wed, Aug 13, 2014 at 02:09:22PM +0100, Chris Wilson wrote:
On Wed, Aug 13, 2014 at 03:04:02PM +0200, Daniel Vetter wrote:
On Wed, Aug 13, 2014 at 01:58:30PM +0100, Chris Wilson wrote:
On Wed, Aug 13, 2014 at 02:46:53PM +0200, Daniel Vetter wrote:
On Wed, Aug 13, 2014 at 01:09:46PM +0100, Chris Wilson wrote:
Rather than take and release the console_lock() around a non-existent
DRM_I915_FBDEV, move the lock acquisation into the callee where it
will
be compiled out by the config option entirely. This includes moving
the
deferred fb_set_suspend() dance and encapsulating it entirely within
intel_fbdev.c.
v2: Use an integral work item so that we can explicitly flush the work
upon suspend/unload.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_dma.c| 3 ---
drivers/gpu/drm/i915/i915_drv.c| 28 ++---
drivers/gpu/drm/i915/i915_drv.h| 9 +---
drivers/gpu/drm/i915/intel_drv.h | 4 ++--
drivers/gpu/drm/i915/intel_fbdev.c | 42
+-
5 files changed, 46 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c
b/drivers/gpu/drm/i915/i915_dma.c
index 860da759ae34..fbab9370cb5c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1339,8 +1339,6 @@ static int i915_load_modeset_init(struct
drm_device *dev)
if (ret)
goto cleanup_irq;
- INIT_WORK(dev_priv-console_resume_work, intel_console_resume);
-
intel_modeset_gem_init(dev);
/* Always safe in the mode setting case. */
@@ -1857,7 +1855,6 @@ int i915_driver_unload(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
intel_fbdev_fini(dev);
intel_modeset_cleanup(dev);
- cancel_work_sync(dev_priv-console_resume_work);
This one here seems to have been lost - shouldn't we move this to
fbdev_fini instead? Otherwise this looks good imo, so I'll fix that up
while merging if you're ok.
Hmm, I counted on us calling suspend before intel_fbdev_fini() in
unload. Is this not the case? Otherwise I think we should be suspending
the console.
We don't do any of that. Unregistering the fbdev emulation in fbdev_fini
should be enough though to stop anyone from actually using the gpu, and
we're careful to unregister other console drivers like vgacon to make sure
no one else can grow stupid ideas.
So I think the only concern really is that the work item completes before
the text section disappears, and maybe (on multi-gpu systems) that we
don't keep all fbdev devices disabled forever after a racy module unload.
Ok, I followed the unload-fbdev_fini-unregister_console and concur
that all you want is the flush_work() in fbdev_fini.
Donemerged, thanks a lot for the patch - looks much better than my #ifdef
hacks ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [REGRESSION BISECTED] backlight control stops workin with 3.14 and later
On Wed, 23 Jul 2014, Hans de Goede hdego...@redhat.com wrote: Hi, On 07/22/2014 08:52 AM, Daniel Vetter wrote: On Tue, Jul 22, 2014 at 8:42 AM, Hans de Goede hdego...@redhat.com wrote: Hi Jani et al, A friend of mine Bertrik Sikken (in the Cc) his backlight control stopped working for him on his Samsung N150Plus netbook. I took a quick look, and the raw intel_backlight backlight interface works under 3.14, but the firmware samsung_laptop backlight interface, which is what most userspace apps will use by default, stops working in 3.14 . I've asked him to bisect this and the bisect points out this commit as the culprit: b35684b8fa94e04f55fd38bf672b737741d2f9e2 is the first bad commit commit b35684b8fa94e04f55fd38bf672b737741d2f9e2 Author: Jani Nikula jani.nik...@intel.com Date: Thu Nov 14 12:13:41 2013 +0200 drm/i915: do full backlight setup at enable time We should now have all the information we need to do a full initialization of the backlight registers. v2: Keep QUIRK_NO_PCH_PWM_ENABLE for now (Imre). Signed-off-by: Jani Nikula jani.nik...@intel.com Reviewed-by: Imre Deak imre.d...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Note that this laptop has an acpi_video backlight interface too, but that has been broken from the start and gets disabled by samsung-laptop based on dmi matching. How does the intel backlight fare? That works fine with 3.14 . Please test both 3.14 and 3.15 and We've tested with 3.14, please let us know if you also want Bertrik to test with 3.15. also test what happens when you blacklist the samsung-laptop driver (if that's possible without wreaking the machine). Then the vendor interface won't get promoted, acpi-video will load, and things likely will not work. Bertrik, can you try blacklisting the samsung-laptop module, then check /sys/class/backlight, the samsung_laptop dir should be gone replaced by an acpi_video0 (or some such) dir. Please try if that works. If that does not work, try booting with both the module blacklisted and acpi_backlight=vendor on the kernel commandline, then you should see only the intel-backlight under /sys/class/backlight and things should work. Also please grab latest intel-gpu-tools and record a register dump with intel_reg_dump, again for both broken and working kernels. Bertrik, can you do this please (without the blacklisting or special kernel commandline options). Please attach dmesg with drm.debug=0xe module parameter set for some recent kernel. BR, Jani. Regards, Hans -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths
On Tue, 2014-08-12 at 16:21 +0530, sagar.a.kam...@intel.com wrote:
From: Sagar Kamble sagar.a.kamble at intel.com
The change is substantial enough that you should add a commit message
explaining what it fixes and how. There are further useful guidelines on
this topic in Documentation/SubmittingPatches.
In the future, you would make the review (and a possible bisection)
easier if you split this patch into a refactoring patch without
functional change and one that fixes the issue.
v1:
Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of
gunit registers need to be followed in
PM suspend and resume path similar to runtime suspend and resume.
v2:
1. Keeping GT access, wake, gunit save/restore related helpers static.
2. Moved GT access check, Wake Control, Gunit state save to end of
i915_drm_freeze.
3. Reusing the sequence in runtime_suspend/resume path at macro level.
v3:
1. Prepared common handlers for platform specific tasks to be done before HW
suspend and after HW resume from
D0i3.
2. Changed commit header.
Cc: Imre Deak imre.d...@intel.com
Cc: Paulo Zanoni paulo.r.zan...@intel.com
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: Jani Nikula jani.nik...@linux.intel.com
Cc: Goel, Akash akash.g...@intel.com
Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
Signed-off-by: Sagar Kamble sagar.a.kamble at intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 130
++--
1 file changed, 85 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ec96f9a..4440722 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
return true;
}
+
+static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv);
+static int post_hw_resume_init(struct drm_i915_private *dev_priv,
+ bool resume_from_rpm_suspend);
Nitpick: Something like intel_suspend_complete, intel_resume_prepare
would be more descriptive names.
+
static int i915_drm_freeze(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work)
static int i915_drm_thaw_early(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
+ int ret = 0;
The initialization here is redundant and could suppress complier
warnings. There are also a couple more instances below.
- if (IS_HASWELL(dev) || IS_BROADWELL(dev))
- hsw_disable_pc8(dev_priv);
+ /* Restore any platform specific registers/clk state */
+ ret = post_hw_resume_init(dev_priv, false);
You could print an error message here, noting that we continue resuming
despite the error.
intel_uncore_early_sanitize(dev, true);
intel_uncore_sanitize(dev);
intel_power_domains_init_hw(dev_priv);
- return 0;
+ return ret;
}
static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
@@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
struct drm_device *drm_dev = pci_get_drvdata(pdev);
struct drm_i915_private *dev_priv = drm_dev-dev_private;
+ int ret = 0;
/*
* We have a suspedn ordering issue with the snd-hda driver also
@@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev)
if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
- if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
- hsw_enable_pc8(dev_priv);
+ /* Save any platform specific registers/clk state needed post resume */
+ ret = pre_hw_suspend_deinit(dev_priv);
pci_disable_device(pdev);
pci_set_power_state(pdev, PCI_D3hot);
In case of error the suspend will be canceled and the resume handler
will be called, so we shouldn't disable the device.
- return 0;
+ return ret;
}
static int i915_pm_resume_early(struct device *dev)
@@ -983,23 +990,26 @@ static int i915_pm_poweroff(struct device *dev)
return i915_drm_freeze(drm_dev);
}
-static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
+static int hsw_suspend(struct drm_i915_private *dev_priv)
Based on the above nitpick something like hsw_suspend_prepare would be
better. The same goes for the other platform handlers.
{
hsw_enable_pc8(dev_priv);
return 0;
}
-static int snb_runtime_resume(struct drm_i915_private *dev_priv)
+static int snb_resume(struct drm_i915_private *dev_priv,
+ bool resume_from_rpm_suspend)
Nitpick: s/resume_from_rpm_suspend/rpm_resume/ would be a bit more
compact.
{
struct drm_device *dev = dev_priv-dev;
- intel_init_pch_refclk(dev);
+ if
Re: [Intel-gfx] [PATCH 1/5] drm/i915: Print captured bo for all VM in error state
Chris Wilson ch...@chris-wilson.co.uk writes:
The current error state harks back to the era of just a single VM. For
full-ppgtt, we capture every bo on every VM. It behoves us to then print
every bo for every VM, which we currently fail to do and so miss vital
information in the error state.
v2: Use the vma address rather than -1!
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Offsets can collide between different vm areas.
If we add vm index also to the captured batchbuffer objects,
we could print it part of the offset '%d:0x%x' that would easily
identify vm and we would immediately see what vm was active on a ring.
-Mika
---
drivers/gpu/drm/i915/i915_drv.h | 2 +
drivers/gpu/drm/i915/i915_gpu_error.c | 80
---
2 files changed, 58 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1bf2cea..e0dcd70 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -396,6 +396,7 @@ struct drm_i915_error_state {
pid_t pid;
char comm[TASK_COMM_LEN];
} ring[I915_NUM_RINGS];
+
struct drm_i915_error_buffer {
u32 size;
u32 name;
@@ -414,6 +415,7 @@ struct drm_i915_error_state {
} **active_bo, **pinned_bo;
u32 *active_bo_count, *pinned_bo_count;
+ u32 vm_count;
};
struct intel_connector;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index fc11ac6..35e70d5 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -192,10 +192,10 @@ static void print_error_buffers(struct
drm_i915_error_state_buf *m,
struct drm_i915_error_buffer *err,
int count)
{
- err_printf(m, %s [%d]:\n, name, count);
+ err_printf(m, %s [%d]:\n, name, count);
while (count--) {
- err_printf(m, %08x %8u %02x %02x %x %x,
+ err_printf(m, %08x %8u %02x %02x %x %x,
err-gtt_offset,
err-size,
err-read_domains,
@@ -393,15 +393,17 @@ int i915_error_state_to_str(struct
drm_i915_error_state_buf *m,
i915_ring_error_state(m, dev, error-ring[i]);
}
- if (error-active_bo)
+ for (i = 0; i error-vm_count; i++) {
+ err_printf(m, vm[%d]\n, i);
+
print_error_buffers(m, Active,
- error-active_bo[0],
- error-active_bo_count[0]);
+ error-active_bo[i],
+ error-active_bo_count[i]);
- if (error-pinned_bo)
print_error_buffers(m, Pinned,
- error-pinned_bo[0],
- error-pinned_bo_count[0]);
+ error-pinned_bo[i],
+ error-pinned_bo_count[i]);
+ }
for (i = 0; i ARRAY_SIZE(error-ring); i++) {
obj = error-ring[i].batchbuffer;
@@ -644,13 +646,15 @@ unwind:
(src)-base.sizePAGE_SHIFT)
static void capture_bo(struct drm_i915_error_buffer *err,
-struct drm_i915_gem_object *obj)
+struct i915_vma *vma)
{
+ struct drm_i915_gem_object *obj = vma-obj;
+
err-size = obj-base.size;
err-name = obj-base.name;
err-rseqno = obj-last_read_seqno;
err-wseqno = obj-last_write_seqno;
- err-gtt_offset = i915_gem_obj_ggtt_offset(obj);
+ err-gtt_offset = vma-node.start;
err-read_domains = obj-base.read_domains;
err-write_domain = obj-base.write_domain;
err-fence_reg = obj-fence_reg;
@@ -674,7 +678,7 @@ static u32 capture_active_bo(struct drm_i915_error_buffer
*err,
int i = 0;
list_for_each_entry(vma, head, mm_list) {
- capture_bo(err++, vma-obj);
+ capture_bo(err++, vma);
if (++i == count)
break;
}
@@ -683,21 +687,27 @@ static u32 capture_active_bo(struct
drm_i915_error_buffer *err,
}
static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
- int count, struct list_head *head)
+ int count, struct list_head *head,
+ struct i915_address_space *vm)
{
struct drm_i915_gem_object *obj;
- int i = 0;
+ struct drm_i915_error_buffer * const first = err;
+ struct drm_i915_error_buffer * const last = err + count;
list_for_each_entry(obj, head, global_list) {
- if (!i915_gem_obj_is_pinned(obj))
- continue;
+ struct i915_vma *vma;
- capture_bo(err++, obj);
- if
Re: [Intel-gfx] [PATCH v3 1/1] drm/i915: Sharing Gfx Clock, Wake and Gunit save/restore logic using common handler for runtime/system s/r paths
On Wed, 2014-08-13 at 16:47 +0300, Imre Deak wrote:
On Tue, 2014-08-12 at 16:21 +0530, sagar.a.kam...@intel.com wrote:
From: Sagar Kamble sagar.a.kamble at intel.com
The change is substantial enough that you should add a commit message
explaining what it fixes and how. There are further useful guidelines on
this topic in Documentation/SubmittingPatches.
In the future, you would make the review (and a possible bisection)
easier if you split this patch into a refactoring patch without
functional change and one that fixes the issue.
Thanks for the review Imre, Daniel.
Agree that I need to split this patch and give more description for the
changes. Will fix other reviewed code as well.
v1:
Sequence to get gfx clocks on/off, allow/disallow wake and save/restore of
gunit registers need to be followed in
PM suspend and resume path similar to runtime suspend and resume.
v2:
1. Keeping GT access, wake, gunit save/restore related helpers static.
2. Moved GT access check, Wake Control, Gunit state save to end of
i915_drm_freeze.
3. Reusing the sequence in runtime_suspend/resume path at macro level.
v3:
1. Prepared common handlers for platform specific tasks to be done before
HW suspend and after HW resume from
D0i3.
2. Changed commit header.
Cc: Imre Deak imre.d...@intel.com
Cc: Paulo Zanoni paulo.r.zan...@intel.com
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: Jani Nikula jani.nik...@linux.intel.com
Cc: Goel, Akash akash.g...@intel.com
Change-Id: I15cfdeeec9c976d9839bb281f809664f4a0c78a2
Signed-off-by: Sagar Kamble sagar.a.kamble at intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 130
++--
1 file changed, 85 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c
b/drivers/gpu/drm/i915/i915_drv.c
index ec96f9a..4440722 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -494,6 +494,11 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
return true;
}
+
+static int pre_hw_suspend_deinit(struct drm_i915_private *dev_priv);
+static int post_hw_resume_init(struct drm_i915_private *dev_priv,
+ bool resume_from_rpm_suspend);
Nitpick: Something like intel_suspend_complete, intel_resume_prepare
would be more descriptive names.
+
static int i915_drm_freeze(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -614,15 +619,16 @@ void intel_console_resume(struct work_struct *work)
static int i915_drm_thaw_early(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
+ int ret = 0;
The initialization here is redundant and could suppress complier
warnings. There are also a couple more instances below.
- if (IS_HASWELL(dev) || IS_BROADWELL(dev))
- hsw_disable_pc8(dev_priv);
+ /* Restore any platform specific registers/clk state */
+ ret = post_hw_resume_init(dev_priv, false);
You could print an error message here, noting that we continue resuming
despite the error.
intel_uncore_early_sanitize(dev, true);
intel_uncore_sanitize(dev);
intel_power_domains_init_hw(dev_priv);
- return 0;
+ return ret;
}
static int __i915_drm_thaw(struct drm_device *dev, bool
restore_gtt_mappings)
@@ -908,6 +914,7 @@ static int i915_pm_suspend_late(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
struct drm_device *drm_dev = pci_get_drvdata(pdev);
struct drm_i915_private *dev_priv = drm_dev-dev_private;
+ int ret = 0;
/*
* We have a suspedn ordering issue with the snd-hda driver also
@@ -921,13 +928,13 @@ static int i915_pm_suspend_late(struct device *dev)
if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
- if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
- hsw_enable_pc8(dev_priv);
+ /* Save any platform specific registers/clk state needed post resume */
+ ret = pre_hw_suspend_deinit(dev_priv);
pci_disable_device(pdev);
pci_set_power_state(pdev, PCI_D3hot);
In case of error the suspend will be canceled and the resume handler
will be called, so we shouldn't disable the device.
- return 0;
+ return ret;
}
static int i915_pm_resume_early(struct device *dev)
@@ -983,23 +990,26 @@ static int i915_pm_poweroff(struct device *dev)
return i915_drm_freeze(drm_dev);
}
-static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
+static int hsw_suspend(struct drm_i915_private *dev_priv)
Based on the above nitpick something like hsw_suspend_prepare would be
better. The same goes for the other platform handlers.
{
hsw_enable_pc8(dev_priv);
return 0;
}
-static int snb_runtime_resume(struct drm_i915_private *dev_priv)
+static int snb_resume(struct
Re: [Intel-gfx] Usage of _PAGE_PCD et al in i915 driver
On Fri, 8 Aug 2014 15:14:15 +0200 Daniel Vetter daniel.vet...@ffwll.ch wrote: Adding relevant mailing lists. On Fri, Aug 8, 2014 at 1:23 PM, Juergen Gross jgr...@suse.com wrote: I'm just about to create a patch for full PAT support in the Linux kernel, including Xen. For this purpose I introduce a translation between cache modes and pte bits. Scanning the kernel sources for usage of the cache mode bits in the pte I discovered drivers/gpu/drm/i915/i915_gem_gtt.h is using _PAGE_PCD, _PAGE_PWT and _PAGE_PAT. I think those defines are used to create ptes not for usage by the main processor, but for the graphics processor. Is this true? In this case I'd suggest to define i915-specific macros instead of using the x86 ones. Yeah, those are gpu specific PAT tables, but the hw engineers specifically designed this to match, and we've tried to follow the cpu side to match it. Especially in the future that will be somewhat important, since we want to fully share the entire address space between cpu and gpu on the next platform. Jesse is working on that. Right, we have an x86 compatible MMU in the GPU itself, so re-using the defines makes sense. I suppose with your work you'll move them and make them a bit more opaque? If so, we'll still want a way to get at them directly, or access your mapping functions for generating PTE bits for the GPU MMU. Thanks, -- Jesse Barnes, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 1/5] drm/i915: take display port power domain in DP HPD handler
Ville noticed that we can call ibx_digital_port_connected() which accesses
the HW without holding any power well/runtime pm reference. Fix this by
holding a display port power domain reference around the whole hpd_pulse
handler.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/intel_dp.c | 19 ++-
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e5ada4f..0ebad42 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4037,9 +4037,12 @@ bool
intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
{
struct intel_dp *intel_dp = intel_dig_port-dp;
+ struct intel_encoder *intel_encoder = intel_dig_port-base;
struct drm_device *dev = intel_dig_port-base.base.dev;
struct drm_i915_private *dev_priv = dev-dev_private;
- int ret;
+ enum intel_display_power_domain power_domain;
+ bool ret = true;
+
if (intel_dig_port-base.type != INTEL_OUTPUT_EDP)
intel_dig_port-base.type = INTEL_OUTPUT_DISPLAYPORT;
@@ -4047,6 +4050,9 @@ intel_dp_hpd_pulse(struct intel_digital_port
*intel_dig_port, bool long_hpd)
port_name(intel_dig_port-port),
long_hpd ? long : short);
+ power_domain = intel_display_port_power_domain(intel_encoder);
+ intel_display_power_get(dev_priv, power_domain);
+
if (long_hpd) {
if (!ibx_digital_port_connected(dev_priv, intel_dig_port))
goto mst_fail;
@@ -4062,8 +4068,7 @@ intel_dp_hpd_pulse(struct intel_digital_port
*intel_dig_port, bool long_hpd)
} else {
if (intel_dp-is_mst) {
- ret = intel_dp_check_mst_status(intel_dp);
- if (ret == -EINVAL)
+ if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
goto mst_fail;
}
@@ -4077,7 +4082,8 @@ intel_dp_hpd_pulse(struct intel_digital_port
*intel_dig_port, bool long_hpd)
drm_modeset_unlock(dev-mode_config.connection_mutex);
}
}
- return false;
+ ret = false;
+ goto put_power;
mst_fail:
/* if we were in MST mode, and device is not there get out of MST mode
*/
if (intel_dp-is_mst) {
@@ -4085,7 +4091,10 @@ mst_fail:
intel_dp-is_mst = false;
drm_dp_mst_topology_mgr_set_mst(intel_dp-mst_mgr,
intel_dp-is_mst);
}
- return true;
+put_power:
+ intel_display_power_put(dev_priv, power_domain);
+
+ return ret;
}
/* Return which DP Port should be selected for Transcoder DP control */
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 2/5] drm/i915: fix HPD IRQ reenable work cancelation
Atm, the HPD IRQ reenable timer can get rearmed right after it's
canceled. Also to access the HPD IRQ mask registers we need to wake up
the HW.
Solve both issues by converting the reenable timer to a delayed work and
grabbing a runtime PM reference in the work. By this we can also forgo
canceling the timer during runtime suspend, since the only important
thing there is that the HW is awake when we write the registers and
that's ensured by the RPM ref. So do the cancelation only during driver
unload time; this is also a requirement for an upcoming patch where we
want to cancel all HPD related works only during system suspend and
driver unload time, but not during runtime suspend.
Note that there is still a race between the HPD IRQ reenable work and
drm_irq_uninstall() during driver unload, where the work can reenable
the HPD IRQs disabled by drm_irq_uninstall(). This isn't a problem since
the HPD IRQs will still be effectively masked by the first level
interrupt mask.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/i915_irq.c | 33 -
drivers/gpu/drm/i915/intel_display.c | 1 +
3 files changed, 14 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 541fb6f..9d9a2e7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1483,7 +1483,7 @@ struct drm_i915_private {
} hpd_mark;
} hpd_stats[HPD_NUM_PINS];
u32 hpd_event_bits;
- struct timer_list hotplug_reenable_timer;
+ struct delayed_work hotplug_reenable_work;
struct i915_fbc fbc;
struct i915_drrs drrs;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b1bb88f..68fe425 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1189,8 +1189,8 @@ static void i915_hotplug_work_func(struct work_struct
*work)
* some connectors */
if (hpd_disabled) {
drm_kms_helper_poll_enable(dev);
- mod_timer(dev_priv-hotplug_reenable_timer,
- jiffies +
msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
+ schedule_delayed_work(dev_priv-hotplug_reenable_work,
+ msecs_to_jiffies(I915_REENABLE_HOTPLUG_DELAY));
}
spin_unlock_irqrestore(dev_priv-irq_lock, irqflags);
@@ -1213,11 +1213,6 @@ static void i915_hotplug_work_func(struct work_struct
*work)
drm_kms_helper_hotplug_event(dev);
}
-static void intel_hpd_irq_uninstall(struct drm_i915_private *dev_priv)
-{
- del_timer_sync(dev_priv-hotplug_reenable_timer);
-}
-
static void ironlake_rps_change_irq_handler(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -3901,8 +3896,6 @@ static void gen8_irq_uninstall(struct drm_device *dev)
if (!dev_priv)
return;
- intel_hpd_irq_uninstall(dev_priv);
-
gen8_irq_reset(dev);
}
@@ -3917,8 +3910,6 @@ static void valleyview_irq_uninstall(struct drm_device
*dev)
I915_WRITE(VLV_MASTER_IER, 0);
- intel_hpd_irq_uninstall(dev_priv);
-
for_each_pipe(pipe)
I915_WRITE(PIPESTAT(pipe), 0x);
@@ -3997,8 +3988,6 @@ static void ironlake_irq_uninstall(struct drm_device *dev)
if (!dev_priv)
return;
- intel_hpd_irq_uninstall(dev_priv);
-
ironlake_irq_reset(dev);
}
@@ -4369,8 +4358,6 @@ static void i915_irq_uninstall(struct drm_device * dev)
struct drm_i915_private *dev_priv = dev-dev_private;
int pipe;
- intel_hpd_irq_uninstall(dev_priv);
-
if (I915_HAS_HOTPLUG(dev)) {
I915_WRITE(PORT_HOTPLUG_EN, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
@@ -4606,8 +4593,6 @@ static void i965_irq_uninstall(struct drm_device * dev)
if (!dev_priv)
return;
- intel_hpd_irq_uninstall(dev_priv);
-
I915_WRITE(PORT_HOTPLUG_EN, 0);
I915_WRITE(PORT_HOTPLUG_STAT, I915_READ(PORT_HOTPLUG_STAT));
@@ -4623,14 +4608,18 @@ static void i965_irq_uninstall(struct drm_device * dev)
I915_WRITE(IIR, I915_READ(IIR));
}
-static void intel_hpd_irq_reenable(unsigned long data)
+static void intel_hpd_irq_reenable(struct work_struct *work)
{
- struct drm_i915_private *dev_priv = (struct drm_i915_private *)data;
+ struct drm_i915_private *dev_priv =
+ container_of(work, typeof(*dev_priv),
+hotplug_reenable_work.work);
struct drm_device *dev = dev_priv-dev;
struct drm_mode_config *mode_config = dev-mode_config;
unsigned long irqflags;
int i;
+ intel_runtime_pm_get(dev_priv);
+
spin_lock_irqsave(dev_priv-irq_lock, irqflags);
for (i = (HPD_NONE + 1); i HPD_NUM_PINS;
[Intel-gfx] [PATCH v2 4/5] drm/i915: make sure VDD is turned off during system suspend
Atm we may leave eDP VDD enabled during system suspend after the CRTCs
are disabled through an HPD-DPCD read event. So disable VDD during
suspend at a point when no HPDs can occur.
Note that runtime suspend doesn't have the same problem, since there the
RPM ref held by VDD provides already the needed serialization.
v2:
- add note to commit message about the runtime suspend path (Ville)
- use edp_panel_vdd_off_sync(), so we can keep the WARN in
edp_panel_vdd_off() (Ville)
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 15 +++
drivers/gpu/drm/i915/intel_dp.c | 11 +++
drivers/gpu/drm/i915/intel_drv.h | 6 ++
3 files changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d1a29fe..5983b14 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -509,6 +509,19 @@ void intel_hpd_cancel_work(struct drm_i915_private
*dev_priv)
cancel_delayed_work_sync(dev_priv-hotplug_reenable_work);
}
+static void intel_suspend_encoders(struct drm_i915_private *dev_priv)
+{
+ struct drm_device *dev = dev_priv-dev;
+ struct intel_encoder *intel_encoder;
+
+ drm_modeset_lock_all(dev);
+ for_each_intel_encoder(dev, intel_encoder) {
+ if (intel_encoder-suspend)
+ intel_encoder-suspend(intel_encoder);
+ }
+ drm_modeset_unlock_all(dev);
+}
+
static int i915_drm_freeze(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -555,6 +568,8 @@ static int i915_drm_freeze(struct drm_device *dev)
intel_runtime_pm_disable_interrupts(dev);
intel_hpd_cancel_work(dev_priv);
+ intel_suspend_encoders(dev_priv);
+
intel_suspend_gt_powersave(dev);
intel_modeset_suspend_hw(dev);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0ebad42..ee982fc 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4003,6 +4003,16 @@ void intel_dp_encoder_destroy(struct drm_encoder
*encoder)
kfree(intel_dig_port);
}
+void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)
+{
+ struct intel_dp *intel_dp = enc_to_intel_dp(intel_encoder-base);
+
+ if (!is_edp(intel_dp))
+ return;
+
+ edp_panel_vdd_off_sync(intel_dp);
+}
+
static void intel_dp_encoder_reset(struct drm_encoder *encoder)
{
intel_edp_panel_vdd_sanitize(to_intel_encoder(encoder));
@@ -4732,6 +4742,7 @@ intel_dp_init(struct drm_device *dev, int output_reg,
enum port port)
intel_encoder-disable = intel_disable_dp;
intel_encoder-get_hw_state = intel_dp_get_hw_state;
intel_encoder-get_config = intel_dp_get_config;
+ intel_encoder-suspend = intel_dp_encoder_suspend;
if (IS_CHERRYVIEW(dev)) {
intel_encoder-pre_pll_enable = chv_dp_pre_pll_enable;
intel_encoder-pre_enable = chv_pre_enable_dp;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3abc915..5d7b3f9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -153,6 +153,12 @@ struct intel_encoder {
* be set correctly before calling this function. */
void (*get_config)(struct intel_encoder *,
struct intel_crtc_config *pipe_config);
+ /*
+* Called during system suspend after all pending requests for the
+* encoder are flushed (for example for DP AUX transactions) and
+* device interrupts are disabled.
+*/
+ void (*suspend)(struct intel_encoder *);
int crtc_mask;
enum hpd_pin hpd_pin;
};
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 5/5] drm/i915: don't try to retrain a DP link on an inactive CRTC
Atm we may retrain the DP link even if the CRTC is inactive through
HPD work-intel_dp_check_link_status(). This in turn can lock up the PHY
(at least on BYT), since the DP port is disabled.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81948
Signed-off-by: Imre Deak imre.d...@intel.com
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_dp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ee982fc..8c1d4e1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3553,6 +3553,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
if (WARN_ON(!intel_encoder-base.crtc))
return;
+ if (!to_intel_crtc(intel_encoder-base.crtc)-active)
+ return;
+
/* Try to read receiver status if the link appears to be up */
if (!intel_dp_get_link_status(intel_dp, link_status)) {
return;
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/43] drm/i915/bdw: Add a context and an engine pointers to the ringbuffer
On Wed, Aug 13, 2014 at 01:34:15PM +, Daniel, Thomas wrote: -Original Message- From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel Vetter Sent: Monday, August 11, 2014 3:21 PM To: Daniel, Thomas Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 08/43] drm/i915/bdw: Add a context and an engine pointers to the ringbuffer On Mon, Aug 11, 2014 at 04:14:13PM +0200, Daniel Vetter wrote: On Thu, Jul 24, 2014 at 05:04:16PM +0100, Thomas Daniel wrote: From: Oscar Mateo oscar.ma...@intel.com Any given ringbuffer is unequivocally tied to one context and one engine. By setting the appropriate pointers to them, the ringbuffer struct holds all the infromation you might need to submit a workload for processing, Execlists style. Signed-off-by: Oscar Mateo oscar.ma...@intel.com --- drivers/gpu/drm/i915/intel_lrc.c|2 ++ drivers/gpu/drm/i915/intel_ringbuffer.c |2 ++ drivers/gpu/drm/i915/intel_ringbuffer.h |3 +++ 3 files changed, 7 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 0a12b8c..2eb7db6 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -132,6 +132,8 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, return ret; } + ringbuf-ring = ring; + ringbuf-ctx = ctx; ringbuf-size = 32 * PAGE_SIZE; ringbuf-effective_size = ringbuf-size; ringbuf-head = 0; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 01e9840..279dda4 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1570,6 +1570,8 @@ static int intel_init_ring_buffer(struct drm_device *dev, INIT_LIST_HEAD(ring-active_list); INIT_LIST_HEAD(ring-request_list); ringbuf-size = 32 * PAGE_SIZE; + ringbuf-ring = ring; + ringbuf-ctx = ring-default_context; That doesn't make a terribly lot of sense tbh. I fear it's one of these slight confusions which will take tons of patches to clean up. Why exactly do we need the ring-ctx pointer? If we only need this for lrc I want to name it accordingly, to make sure legacy code doesn't grow stupid ideas. And also we should only initialize this in the lrc ctx init then. All patches up to this one merged. Ok, I've discussed this quickly with Damien on irc. We decided to cut away the ring-ctx part of this patch for now to be able to move on. -Daniel As you've seen, removing ringbuffer-ctx causes serious problems with the plumbing later on. This can be renamed (perhaps to lrc) and removed from legacy init. Each ring buffer belongs to a specific context - it makes sense to me to keep this information within the ringbuffer structure so that we don't have to pass the context pointer around everywhere. I agree that it causes trouble with the follow-up patches, but I'm not sold on this being a terrible good idea. After all for ELSP we don't want to submit a ring, we want to submit the full context. So if the code that's supposed to do the execlist ctx submission only has the pointer to the ring object, the layer looks a bit wrong. Same was iirc about the add_request part. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 3/5] drm/i915: cancel hotplug and dig_port work during suspend and unload
Make sure these work handlers don't run after we system suspend or
unload the driver. Note that we don't cancel the handlers during runtime
suspend. That could lead to a lockup, since we take a runtime PM ref
from the handlers themselves. Fortunaltely canceling there is not needed
since the RPM ref itself provides for the needed serialization.
v2:
- fix the order of canceling dig_port_work wrt. hotplug_work (Ville)
- zero out {long,short}_hpd_port_mask and hpd_event_bits for speed
(Ville)
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 16
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_display.c | 3 +--
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 01de977..d1a29fe 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -494,6 +494,21 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
return true;
}
+void intel_hpd_cancel_work(struct drm_i915_private *dev_priv)
+{
+ spin_lock_irq(dev_priv-irq_lock);
+
+ dev_priv-long_hpd_port_mask = 0;
+ dev_priv-short_hpd_port_mask = 0;
+ dev_priv-hpd_event_bits = 0;
+
+ spin_unlock_irq(dev_priv-irq_lock);
+
+ cancel_work_sync(dev_priv-dig_port_work);
+ cancel_work_sync(dev_priv-hotplug_work);
+ cancel_delayed_work_sync(dev_priv-hotplug_reenable_work);
+}
+
static int i915_drm_freeze(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -538,6 +553,7 @@ static int i915_drm_freeze(struct drm_device *dev)
flush_delayed_work(dev_priv-rps.delayed_resume_work);
intel_runtime_pm_disable_interrupts(dev);
+ intel_hpd_cancel_work(dev_priv);
intel_suspend_gt_powersave(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9d9a2e7..d8b6f67 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2219,6 +2219,7 @@ extern unsigned long i915_mch_val(struct drm_i915_private
*dev_priv);
extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
+void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
/* i915_irq.c */
void i915_queue_hangcheck(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index e15b155..c30b9f8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13116,8 +13116,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
* experience fancy races otherwise.
*/
drm_irq_uninstall(dev);
- cancel_work_sync(dev_priv-hotplug_work);
- cancel_delayed_work_sync(dev_priv-hotplug_reenable_work);
+ intel_hpd_cancel_work(dev_priv);
dev_priv-pm._irqs_disabled = true;
/*
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 27/43] drm/i915/bdw: Render state init for Execlists
-Original Message-
From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
Vetter
Sent: Monday, August 11, 2014 10:25 PM
To: Daniel, Thomas
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 27/43] drm/i915/bdw: Render state init for
Execlists
On Thu, Jul 24, 2014 at 05:04:35PM +0100, Thomas Daniel wrote:
From: Oscar Mateo oscar.ma...@intel.com
The batchbuffer that sets the render context state is submitted in a
different way, and from different places.
We needed to make both the render state preparation and free functions
outside accesible, and namespace accordingly. This mess is so that all
LR, LRC and Execlists functionality can go together in intel_lrc.c: we
can fix all of this later on, once the interfaces are clear.
v2: Create a separate ctx-rcs_initialized for the Execlists case, as
suggested by Chris Wilson.
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h |4 +--
drivers/gpu/drm/i915/i915_gem_context.c | 17 +-
drivers/gpu/drm/i915/i915_gem_render_state.c | 40 ++--
--
drivers/gpu/drm/i915/i915_gem_render_state.h | 47
++
drivers/gpu/drm/i915/intel_lrc.c | 46
+
drivers/gpu/drm/i915/intel_lrc.h |2 ++
drivers/gpu/drm/i915/intel_renderstate.h |8 +
7 files changed, 139 insertions(+), 25 deletions(-) create mode
100644 drivers/gpu/drm/i915/i915_gem_render_state.h
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h index 4303e2c..b7cf0ec 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -37,6 +37,7 @@
#include intel_ringbuffer.h
#include intel_lrc.h
#include i915_gem_gtt.h
+#include i915_gem_render_state.h
#include linux/io-mapping.h
#include linux/i2c.h
#include linux/i2c-algo-bit.h
@@ -623,6 +624,7 @@ struct intel_context {
} legacy_hw_ctx;
/* Execlists */
+ bool rcs_initialized;
struct {
struct drm_i915_gem_object *state;
struct intel_ringbuffer *ringbuf;
@@ -2553,8 +2555,6 @@ int i915_gem_context_create_ioctl(struct
drm_device *dev, void *data, int i915_gem_context_destroy_ioctl(struct
drm_device *dev, void *data,
struct drm_file *file);
-/* i915_gem_render_state.c */
-int i915_gem_render_state_init(struct intel_engine_cs *ring);
/* i915_gem_evict.c */
int __must_check i915_gem_evict_something(struct drm_device *dev,
struct i915_address_space *vm, diff
--git
a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 9085ff1..0dc6992 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -513,8 +513,23 @@ int i915_gem_context_enable(struct
drm_i915_private *dev_priv)
ppgtt-enable(ppgtt);
}
- if (i915.enable_execlists)
+ if (i915.enable_execlists) {
+ struct intel_context *dctx;
+
+ ring = dev_priv-ring[RCS];
+ dctx = ring-default_context;
+
+ if (!dctx-rcs_initialized) {
+ ret = intel_lr_context_render_state_init(ring, dctx);
+ if (ret) {
+ DRM_ERROR(Init render state failed: %d\n,
ret);
+ return ret;
+ }
+ dctx-rcs_initialized = true;
+ }
+
return 0;
+ }
This looks very much like the wrong place. We should init the render state
when we create the context, or when we switch to it for the first time.
The later is what the legacy contexts currently do in do_switch.
But ctx_enable should do the switch to the default context and that's about
Well, a side-effect of switching to the default context in legacy mode is that
the render state gets initialized. I could move the lr render state init call
into an enable_execlists branch in i915_switch_context() but that doen't
seem like the right place.
How about in i915_gem_init() after calling i915_gem_init_hw()?
it. If there's some depency then I guess we should stall the creation of the
default context a bit, maybe.
In any case someone needs to explain this better and if there's not other
wey this at least needs a bit comment. So I'll punt for now.
When the default context is created the driver is not ready to execute a
batch. That is why the render state init can't be done then.
Thomas.
-Daniel
/* FIXME: We should make this work, even in reset */
if (i915_reset_in_progress(dev_priv-gpu_error))
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c
b/drivers/gpu/drm/i915/i915_gem_render_state.c
index e60be3f..a9a62d7 100644
---
Re: [Intel-gfx] [PATCH 21/43] drm/i915/bdw: Emission of requests with logical rings
On Wed, Aug 13, 2014 at 01:34:28PM +, Daniel, Thomas wrote:
-Original Message-
From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
Vetter
Sent: Monday, August 11, 2014 9:57 PM
To: Daniel, Thomas
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 21/43] drm/i915/bdw: Emission of requests
with logical rings
On Thu, Jul 24, 2014 at 05:04:29PM +0100, Thomas Daniel wrote:
From: Oscar Mateo oscar.ma...@intel.com
On a previous iteration of this patch, I created an Execlists version
of __i915_add_request and asbtracted it away as a vfunc. Daniel Vetter
wondered then why that was needed:
with the clean split in command submission I expect every function to
know wether it'll submit to an lrc (everything in
intel_lrc.c) or wether it'll submit to a legacy ring (existing code),
so I don't see a need for an add_request vfunc.
The honest, hairy truth is that this patch is the glue keeping the
whole logical ring puzzle together:
Oops, I didn't spot this and it's indeed not terribly pretty.
Are you saying you want to go back to a vfunc for add_request?
Nope, I'd have expected that there's no need for such a switch at all, and
that all these differences disappear behind the execbuf cmd submission
abstraction.
- i915_add_request is used by intel_ring_idle, which in turn is
used by i915_gpu_idle, which in turn is used in several places
inside the eviction and gtt codes.
This should probably be folded in with the lrc specific version of
stop_rings
and so should work out.
- Also, it is used by i915_gem_check_olr, which is littered all
over i915_gem.c
We now always preallocate the request struct, so olr is officially dead.
Well almost, except for non-execbuf stuff that we emit through the rings.
Which is nothing for lrc/execlist mode.
Also there's the icky-bitty problem with ringbuf-ctx which makes this patch
not apply any more. I think we need to revise or at least discuss a bit.
Context is required when doing an advance_and_submit so that we know
which context to queue in the execlist queue.
Might need to re-read, but imo it's not a good idea to do a submit in
there. If I understand this correctly there should only be a need for an
ELSP write (or queueing it up) at the end of the execlist-specific cmd
submission implementation. The ringbuffer writes we do before that for the
different pieces (flushes, seqno write, bb_start, whatever) should just
update the tail pointer in the ring.
Or do I miss something really big here?
- ...
If I were to duplicate all the code that directly or indirectly uses
__i915_add_request, I'll end up creating a separate driver.
To show the differences between the existing legacy version and the
new Execlists one, this time I have special-cased __i915_add_request
instead of adding an add_request vfunc. I hope this helps to untangle
this Gordian knot.
Signed-off-by: Oscar Mateo oscar.ma...@intel.com
---
drivers/gpu/drm/i915/i915_gem.c | 72
--
drivers/gpu/drm/i915/intel_lrc.c | 30 +---
drivers/gpu/drm/i915/intel_lrc.h |1 +
3 files changed, 80 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c
b/drivers/gpu/drm/i915/i915_gem.c index 9560b40..1c83b9c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2327,10 +2327,21 @@ int __i915_add_request(struct intel_engine_cs
*ring, {
struct drm_i915_private *dev_priv = ring-dev-dev_private;
struct drm_i915_gem_request *request;
+ struct intel_ringbuffer *ringbuf;
u32 request_ring_position, request_start;
int ret;
- request_start = intel_ring_get_tail(ring-buffer);
+ request = ring-preallocated_lazy_request;
+ if (WARN_ON(request == NULL))
+ return -ENOMEM;
+
+ if (i915.enable_execlists) {
+ struct intel_context *ctx = request-ctx;
+ ringbuf = ctx-engine[ring-id].ringbuf;
+ } else
+ ringbuf = ring-buffer;
+
+ request_start = intel_ring_get_tail(ringbuf);
/*
* Emit any outstanding flushes - execbuf can fail to emit the flush
* after having emitted the batchbuffer command. Hence we need to
fix @@ -2338,24 +2349,32 @@ int __i915_add_request(struct
intel_engine_cs *ring,
* is that the flush _must_ happen before the next request, no
matter
* what.
*/
- ret = intel_ring_flush_all_caches(ring);
- if (ret)
- return ret;
-
- request = ring-preallocated_lazy_request;
- if (WARN_ON(request == NULL))
- return -ENOMEM;
+ if (i915.enable_execlists) {
+ ret = logical_ring_flush_all_caches(ringbuf);
+ if (ret)
+ return ret;
+ } else {
+ ret = intel_ring_flush_all_caches(ring);
+
Re: [Intel-gfx] [PATCH 27/43] drm/i915/bdw: Render state init for Execlists
On Wed, Aug 13, 2014 at 03:07:29PM +, Daniel, Thomas wrote:
-Original Message-
From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel
Vetter
Sent: Monday, August 11, 2014 10:25 PM
To: Daniel, Thomas
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 27/43] drm/i915/bdw: Render state init for
Execlists
On Thu, Jul 24, 2014 at 05:04:35PM +0100, Thomas Daniel wrote:
From: Oscar Mateo oscar.ma...@intel.com
index 9085ff1..0dc6992 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -513,8 +513,23 @@ int i915_gem_context_enable(struct
drm_i915_private *dev_priv)
ppgtt-enable(ppgtt);
}
- if (i915.enable_execlists)
+ if (i915.enable_execlists) {
+ struct intel_context *dctx;
+
+ ring = dev_priv-ring[RCS];
+ dctx = ring-default_context;
+
+ if (!dctx-rcs_initialized) {
+ ret = intel_lr_context_render_state_init(ring, dctx);
+ if (ret) {
+ DRM_ERROR(Init render state failed: %d\n,
ret);
+ return ret;
+ }
+ dctx-rcs_initialized = true;
+ }
+
return 0;
+ }
This looks very much like the wrong place. We should init the render state
when we create the context, or when we switch to it for the first time.
The later is what the legacy contexts currently do in do_switch.
But ctx_enable should do the switch to the default context and that's about
Well, a side-effect of switching to the default context in legacy mode is that
the render state gets initialized. I could move the lr render state init call
into an enable_execlists branch in i915_switch_context() but that doen't
seem like the right place.
How about in i915_gem_init() after calling i915_gem_init_hw()?
it. If there's some depency then I guess we should stall the creation of the
default context a bit, maybe.
In any case someone needs to explain this better and if there's not other
wey this at least needs a bit comment. So I'll punt for now.
When the default context is created the driver is not ready to execute a
batch. That is why the render state init can't be done then.
That sounds like the default context is created too early. Essentially I
want to avoid needless divergence between the default context and normal
contexts, because sooner or later that will means someone will creep in
with a _really_ subtle bug.
What about:
- We create the default lrc contexs in context_init, but like with a
normal context we don't do any of the deferred setup.
- In context_enable (which since yesterday properly propagates errors to
callers) we force the deferred lrc ctx setup for the default contexts on
all engines.
- The render state init is done as part of the deferred ctx setup for the
render engine in all cases.
Totally off the track or do you see a workable solution somewhere in that
direction?
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: fix HPD IRQ reenable work cancelation
The series seems fine to me. Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com for the rest as well. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Sharing platform specific sequence between runtime and system suspend/ resume paths
From: Sagar Kamble sagar.a.kam...@intel.com
On VLV, post S0i3 during i915_drm_thaw following issue is observed during ring
initialization.
[ 335.604039] [drm:stop_ring] ERROR render ring :timed out trying to stop ring
[ 336.607340] [drm:stop_ring] ERROR render ring :timed out trying to stop ring
[ 336.607345] [drm:init_ring_common] ERROR failed to set render ring head to
zero ctl head tail start
[ 337.610645] [drm:stop_ring] ERROR bsd ring :timed out trying to stop ring
[ 338.613952] [drm:stop_ring] ERROR bsd ring :timed out trying to stop ring
[ 338.613956] [drm:init_ring_common] ERROR failed to set bsd ring head to zero
ctl head tail start
[ 339.617256] [drm:stop_ring] ERROR render ring :timed out trying to stop ring
[ 339.617258] ---[ cut here ]---
[ 339.617267] WARNING: CPU: 0 PID: 6 at
drivers/gpu/drm/i915/intel_ringbuffer.c:1666 intel_cleanup_ring+0xe6/0xf0()
[ 339.617396] --[ end trace 5ef5ed1a3c92e2a6 ]--
[ 339.617428] [drm:__i915_drm_thaw] ERROR failed to re-initialize GPU,
declaring wedged!
This is happening since wake is not enabled and Gunit registers are not
restored.
For this system suspend/resume paths need to follow save/restore and additional
platform specific setup in suspend_complete and resume_prepare.
suspend_complete is shared unconditionaly for VLV, HSW, BDW. resume_prepare for
HSW and BDW has pc8 disabling which is needed during thaw_early so sharing
uncondtionally. For VLV and SNB runtime resume specific sequence exists.
Cc: Imre Deak imre.d...@intel.com
Cc: Paulo Zanoni paulo.r.zan...@intel.com
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: Jani Nikula jani.nik...@linux.intel.com
Cc: Goel, Akash akash.g...@intel.com
Signed-off-by: Sagar Kamble sagar.a.kam...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 63 -
1 file changed, 43 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 88464ad..740311b 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -496,7 +496,8 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
static int intel_suspend_complete(struct drm_i915_private *dev_priv);
-static int intel_resume_prepare(struct drm_i915_private *dev_priv);
+static int intel_resume_prepare(struct drm_i915_private *dev_priv,
+ bool rpm_resume);
static int i915_drm_freeze(struct drm_device *dev)
{
@@ -618,15 +619,17 @@ void intel_console_resume(struct work_struct *work)
static int i915_drm_thaw_early(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
+ int ret;
- if (IS_HASWELL(dev) || IS_BROADWELL(dev))
- hsw_disable_pc8(dev_priv);
+ ret = intel_resume_prepare(dev_priv, false);
+ if (ret)
+ DRM_ERROR(Resume prepare failed: %d,Continuing resume\n, ret);
intel_uncore_early_sanitize(dev, true);
intel_uncore_sanitize(dev);
intel_power_domains_init_hw(dev_priv);
- return 0;
+ return ret;
}
static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
@@ -912,6 +915,7 @@ static int i915_pm_suspend_late(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
struct drm_device *drm_dev = pci_get_drvdata(pdev);
struct drm_i915_private *dev_priv = drm_dev-dev_private;
+ int ret;
/*
* We have a suspedn ordering issue with the snd-hda driver also
@@ -925,13 +929,16 @@ static int i915_pm_suspend_late(struct device *dev)
if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
- if (IS_HASWELL(drm_dev) || IS_BROADWELL(drm_dev))
- hsw_enable_pc8(dev_priv);
+ ret = intel_suspend_complete(dev_priv);
- pci_disable_device(pdev);
- pci_set_power_state(pdev, PCI_D3hot);
+ if (ret)
+ DRM_ERROR(Suspend complete failed: %d\n, ret);
+ else {
+ pci_disable_device(pdev);
+ pci_set_power_state(pdev, PCI_D3hot);
+ }
- return 0;
+ return ret;
}
static int i915_pm_resume_early(struct device *dev)
@@ -994,16 +1001,19 @@ static int hsw_suspend_complete(struct drm_i915_private
*dev_priv)
return 0;
}
-static int snb_resume_prepare(struct drm_i915_private *dev_priv)
+static int snb_resume_prepare(struct drm_i915_private *dev_priv,
+ bool rpm_resume)
{
struct drm_device *dev = dev_priv-dev;
- intel_init_pch_refclk(dev);
+ if (rpm_resume)
+ intel_init_pch_refclk(dev);
return 0;
}
-static int hsw_resume_prepare(struct drm_i915_private *dev_priv)
+static int hsw_resume_prepare(struct drm_i915_private *dev_priv,
+ bool rpm_resume)
{
hsw_disable_pc8(dev_priv);
@@ -1339,7
[Intel-gfx] [PATCH 1/2] drm/i915: Created common handler for platform specific suspend/resume
From: Sagar Kamble sagar.a.kam...@intel.com
With this change, intel_runtime_suspend and intel_runtime_resume functions
become completely platform agnostic. Platform specific suspend/resume
changes are moved to intel_suspend_complete and intel_resume_prepare.
Cc: Imre Deak imre.d...@intel.com
Cc: Paulo Zanoni paulo.r.zan...@intel.com
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: Jani Nikula jani.nik...@linux.intel.com
Cc: Goel, Akash akash.g...@intel.com
Signed-off-by: Sagar Kamble sagar.a.kam...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 76 ++---
1 file changed, 49 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ec96f9a..88464ad 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -494,6 +494,10 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
return true;
}
+
+static int intel_suspend_complete(struct drm_i915_private *dev_priv);
+static int intel_resume_prepare(struct drm_i915_private *dev_priv);
+
static int i915_drm_freeze(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -983,14 +987,14 @@ static int i915_pm_poweroff(struct device *dev)
return i915_drm_freeze(drm_dev);
}
-static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
+static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
{
hsw_enable_pc8(dev_priv);
return 0;
}
-static int snb_runtime_resume(struct drm_i915_private *dev_priv)
+static int snb_resume_prepare(struct drm_i915_private *dev_priv)
{
struct drm_device *dev = dev_priv-dev;
@@ -999,7 +1003,7 @@ static int snb_runtime_resume(struct drm_i915_private
*dev_priv)
return 0;
}
-static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
+static int hsw_resume_prepare(struct drm_i915_private *dev_priv)
{
hsw_disable_pc8(dev_priv);
@@ -1295,7 +1299,7 @@ static void vlv_check_no_gt_access(struct
drm_i915_private *dev_priv)
I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
}
-static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
+static int vlv_suspend_complete(struct drm_i915_private *dev_priv)
{
u32 mask;
int err;
@@ -1335,7 +1339,7 @@ err1:
return err;
}
-static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
+static int vlv_resume_prepare(struct drm_i915_private *dev_priv)
{
struct drm_device *dev = dev_priv-dev;
int err;
@@ -1413,17 +1417,7 @@ static int intel_runtime_suspend(struct device *device)
cancel_work_sync(dev_priv-rps.work);
intel_runtime_pm_disable_interrupts(dev);
- if (IS_GEN6(dev)) {
- ret = 0;
- } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
- ret = hsw_runtime_suspend(dev_priv);
- } else if (IS_VALLEYVIEW(dev)) {
- ret = vlv_runtime_suspend(dev_priv);
- } else {
- ret = -ENODEV;
- WARN_ON(1);
- }
-
+ ret = intel_suspend_complete(dev_priv);
if (ret) {
DRM_ERROR(Runtime suspend failed, disabling it (%d)\n, ret);
intel_runtime_pm_restore_interrupts(dev);
@@ -1461,17 +1455,7 @@ static int intel_runtime_resume(struct device *device)
intel_opregion_notify_adapter(dev, PCI_D0);
dev_priv-pm.suspended = false;
- if (IS_GEN6(dev)) {
- ret = snb_runtime_resume(dev_priv);
- } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
- ret = hsw_runtime_resume(dev_priv);
- } else if (IS_VALLEYVIEW(dev)) {
- ret = vlv_runtime_resume(dev_priv);
- } else {
- WARN_ON(1);
- ret = -ENODEV;
- }
-
+ ret = intel_resume_prepare(dev_priv);
/*
* No point of rolling back things in case of an error, as the best
* we can do is to hope that things will still work (and disable RPM).
@@ -1490,6 +1474,44 @@ static int intel_runtime_resume(struct device *device)
return ret;
}
+static int intel_suspend_complete(struct drm_i915_private *dev_priv)
+{
+ struct drm_device *dev = dev_priv-dev;
+ int ret;
+
+ if (IS_GEN6(dev)) {
+ ret = 0;
+ } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+ ret = hsw_suspend_complete(dev_priv);
+ } else if (IS_VALLEYVIEW(dev)) {
+ ret = vlv_suspend_complete(dev_priv);
+ } else {
+ ret = -ENODEV;
+ WARN_ON(1);
+ }
+
+ return ret;
+}
+
+static int intel_resume_prepare(struct drm_i915_private *dev_priv)
+{
+ struct drm_device *dev = dev_priv-dev;
+ int ret;
+
+ if (IS_GEN6(dev)) {
+ ret = snb_resume_prepare(dev_priv);
+ } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+ ret = hsw_resume_prepare(dev_priv);
+ } else
Re: [Intel-gfx] [PATCH 0/4] module: add support for unsafe, tainting parameters
Jani Nikula jani.nik...@intel.com writes:
This is a generic version of Daniel's patch [1] letting us have unsafe
module parameters (experimental, debugging, testing, etc.) that taint
the kernel when set. Quoting Daniel,
OK, I think the idea is fine, but we'll probably only want this for
a few types (eg. int and bool). So for the moment I prefer a more
naive approach.
Does this work for you?
Subject: module: add taint_int type
An int parameter which taints the kernel if set; i915 at least wants this.
Based-on-patches-by: Daniel Vetter daniel.vet...@ffwll.ch
Based-on-patches-by: Jani Nikula jani.nik...@intel.com
Signed-off-by: Rusty Russell ru...@rustcorp.com.au
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 494f99e852da..99ba68206ba4 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -408,6 +408,10 @@ extern int param_set_bint(const char *val, const struct
kernel_param *kp);
#define param_get_bint param_get_int
#define param_check_bint param_check_int
+/* An int, which taints the kernel if set. */
+extern struct kernel_param_ops param_ops_taint_int;
+#define param_check_taint_int param_check_int
+
/**
* module_param_array - a parameter which is an array of some type
* @name: the name of the array variable
diff --git a/kernel/params.c b/kernel/params.c
index 34f527023794..3128218158cf 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -375,6 +375,20 @@ struct kernel_param_ops param_ops_bint = {
};
EXPORT_SYMBOL(param_ops_bint);
+static int param_set_taint_int(const char *val, const struct kernel_param *kp)
+{
+ pr_warn(Setting dangerous option %s - tainting kernel\n, kp-name);
+ add_taint(TAINT_USER, LOCKDEP_STILL_OK);
+
+ return param_set_int(val, kp);
+}
+
+struct kernel_param_ops param_ops_taint_int = {
+ .set = param_set_taint_int,
+ .get = param_get_int,
+};
+EXPORT_SYMBOL(param_ops_taint_int);
+
/* We break the rule and mangle the string. */
static int param_array(const char *name,
const char *val,
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/5] drm/i915: take display port power domain in DP HPD handler
Ville noticed that we can call ibx_digital_port_connected() which accesses
the HW without holding any power well/runtime pm reference. Fix this by
holding a display port power domain reference around the whole hpd_pulse
handler.
Signed-off-by: Imre Deak imre.d...@intel.com
Makes sense to me,
Reviewed-by: Dave Airlie airl...@redhat.com
---
drivers/gpu/drm/i915/intel_dp.c | 19 ++-
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e5ada4f..0ebad42 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4037,9 +4037,12 @@ bool
intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
{
struct intel_dp *intel_dp = intel_dig_port-dp;
+ struct intel_encoder *intel_encoder = intel_dig_port-base;
struct drm_device *dev = intel_dig_port-base.base.dev;
struct drm_i915_private *dev_priv = dev-dev_private;
- int ret;
+ enum intel_display_power_domain power_domain;
+ bool ret = true;
+
if (intel_dig_port-base.type != INTEL_OUTPUT_EDP)
intel_dig_port-base.type = INTEL_OUTPUT_DISPLAYPORT;
@@ -4047,6 +4050,9 @@ intel_dp_hpd_pulse(struct intel_digital_port
*intel_dig_port, bool long_hpd)
port_name(intel_dig_port-port),
long_hpd ? long : short);
+ power_domain = intel_display_port_power_domain(intel_encoder);
+ intel_display_power_get(dev_priv, power_domain);
+
if (long_hpd) {
if (!ibx_digital_port_connected(dev_priv, intel_dig_port))
goto mst_fail;
@@ -4062,8 +4068,7 @@ intel_dp_hpd_pulse(struct intel_digital_port
*intel_dig_port, bool long_hpd)
} else {
if (intel_dp-is_mst) {
- ret = intel_dp_check_mst_status(intel_dp);
- if (ret == -EINVAL)
+ if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
goto mst_fail;
}
@@ -4077,7 +4082,8 @@ intel_dp_hpd_pulse(struct intel_digital_port
*intel_dig_port, bool long_hpd)
drm_modeset_unlock(dev-mode_config.connection_mutex);
}
}
- return false;
+ ret = false;
+ goto put_power;
mst_fail:
/* if we were in MST mode, and device is not there get out of MST
mode */
if (intel_dp-is_mst) {
@@ -4085,7 +4091,10 @@ mst_fail:
intel_dp-is_mst = false;
drm_dp_mst_topology_mgr_set_mst(intel_dp-mst_mgr,
intel_dp-is_mst);
}
- return true;
+put_power:
+ intel_display_power_put(dev_priv, power_domain);
+
+ return ret;
}
/* Return which DP Port should be selected for Transcoder DP control */
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] Graphics driver Init code
Hi Everyone, I am an Operating system developer and I'm working on a project for which I wanted to understand the working of Intel Graphics Driver. Being somewhat new in this field, Can I ask anyone to point me where should I start digging in the code? I mean where the graphic driver initializes contact with the GPU. I'm extremely sorry if I broke any mailing list etiquette. Please forgive me. Thanks in Advance, Dushyant Behl ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] drm/i915: backlight sysfs bl_power and brightness == 0
On 08/12/2014 07:11 AM, Jani Nikula wrote: This series adds support for backlight class sysfs bl_power attribute for eDP panels, which allows switching the backlight on/off. This is done using the eDP panel power control as a sub-state of everything else being enabled. Patch 4 also makes 0 brightness switch off the eDP backlight using the same mechanism. This is all eDP specific, with no support for LVDS. I'm not particularly fond of these patches, but there appears to be demand for them, particularly since commit 6dda730e55f412a6dfb181cae6784822ba463847 Author: Jani Nikula jani.nik...@intel.com Date: Tue Jun 24 18:27:40 2014 +0300 drm/i915: respect the VBT minimum backlight brightness All the discussions seem to just drag on and on without code to speak about, so here goes. I've tried to make this as simple and unintrusive as possible. BR, Jani. I have hand merged the first 3 patches of this series into kernel 3.10 Chromium and the bl_power sysfs entry is functional and turns off backlight power independent of the PWM as designed. The fourth patch is non-functional due to the difference between the 11 Chromium backlight patches from April 2014 and current upstream implementation. Clint Jani Nikula (4): drm/i915/dp: split up panel power control from backlight pwm control drm/i915: add some framework for backlight bl_power support drm/i915/dp: make backlight bl_power control power sequencer backlight drm/i915: switch off backlight for backlight class 0 brightness drivers/gpu/drm/i915/intel_dp.c| 61 ++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_panel.c | 27 + 3 files changed, 77 insertions(+), 13 deletions(-) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] module: add support for unsafe, tainting parameters
On Wed, Aug 13, 2014 at 10:25 PM, Rusty Russell ru...@rustcorp.com.au wrote: Jani Nikula jani.nik...@intel.com writes: This is a generic version of Daniel's patch [1] letting us have unsafe module parameters (experimental, debugging, testing, etc.) that taint the kernel when set. Quoting Daniel, OK, I think the idea is fine, but we'll probably only want this for a few types (eg. int and bool). So for the moment I prefer a more naive approach. Does this work for you? Can you please discuss this with yourself from a few months back? We've done the general version since you suggested that just doing it for int is a bit lame ;-) And I actually agreed so asked Jani to look into that. http://mid.mail-archive.com/87r46gywul.fsf@rustcorp.com.au If this is a good idea, you can write a macro module_param_unsafe_named which is a general wrapper. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
