Re: [Intel-gfx] [RFC 2/3] drm/i915: create intel_update_pipe_size()
On Tue, Sep 09, 2014 at 02:43:14PM -0300, Gustavo Padovan wrote: 2014-09-09 Ville Syrjälä ville.syrj...@linux.intel.com: On Tue, Sep 09, 2014 at 11:43:20AM -0300, Gustavo Padovan wrote: From: Gustavo Padovan gustavo.pado...@collabora.co.uk Factor out a piece of code from intel_pipe_set_base() that updates the pipe size and adjust fitter. This will help refactor the update primary plane path. Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk --- drivers/gpu/drm/i915/intel_display.c | 71 +--- 1 file changed, 41 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ccf7c0..e7e7184 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2779,6 +2779,46 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc) return pending; } +static void intel_update_pipe_size(struct drm_crtc *crtc) These days we usually prefer to pass intel_crtc instead of drm_crtc. You can still call it 'crtc' since that's shorter and because we don't need anything from drm_crtc in this function there won't be any confusion between the two. Actually we need the drm_crtc 3 times in this function, that is why I left it as an argument. We could just do the other way around and get it from intel_crtc-base. Yeah I prefer we use the intel_foo types internally without an intel_ prefix in the local variable name. Adding the foo-base. prefix isn't really much longer than just foo. Imo upcasting should only be done in interface hooks where we have a reason for the paramater to have the more generic type, everywhere else it just looks a bit brittle. And yes I know that we're not there at all. -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] [RFC 1/3] drm/i915: remove !enabled handling from commit primary plane step
On Tue, Sep 09, 2014 at 06:58:47PM +0300, Ville Syrjälä wrote:
On Tue, Sep 09, 2014 at 11:43:19AM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.pado...@collabora.co.uk
The !crtc-enabled case will now be handled by the !visible code,
since the handling is basically the same.
Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
---
drivers/gpu/drm/i915/intel_display.c | 26 --
1 file changed, 26 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 5279b99..2ccf7c0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11837,32 +11837,6 @@ intel_commit_primary_plane(struct drm_plane *plane,
struct drm_rect *src = state-src;
int ret;
- /*
-* If the CRTC isn't enabled, we're just pinning the framebuffer,
-* updating the fb pointer, and returning without touching the
-* hardware. This allows us to later do a drmModeSetCrtc with fb=-1 to
-* turn on the display with all planes setup as desired.
-*/
- if (!crtc-enabled) {
- mutex_lock(dev-struct_mutex);
-
- /*
-* If we already called setplane while the crtc was disabled,
-* we may have an fb pinned; unpin it.
-*/
- if (plane-fb)
- intel_unpin_fb_obj(old_obj);
-
- i915_gem_track_fb(old_obj, obj,
- INTEL_FRONTBUFFER_PRIMARY(intel_crtc-pipe));
-
- /* Pin and return without programming hardware */
- ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
- mutex_unlock(dev-struct_mutex);
-
- return ret;
- }
-
intel_crtc_wait_for_pending_flips(crtc);
Yeah this should work just fine.
One difference between the code paths is the
intel_crtc_wait_for_pending_flips()
call, but since the crtc isn't enabled there can't be any pending flip.
The other difference is the pin vs. unpin order, but that shouldn't matter
unless there's tons of other stuff pinned as well. It's not worth optimizing
setplane calls on disabled CRTCs too much IMO.
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
Queued for -next, thanks for the patch.
-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 3/4] drm/i915: CSC color correction
On Tue, Sep 09, 2014 at 06:30:09PM -0700, Matt Roper wrote:
On Tue, Sep 09, 2014 at 11:53:15AM +0530, shashank.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@intel.com
This patch adds support for CSC correction color property.
It does the following:
1. Creates a new DRM property for CSC correction. Adds this into
mode_config.
2. Attaches this CSC property to calling CRTC. Creates a blob
to store the correction values, and attaches the blob to CRTC.
3. Adds function intel_clrmgr_set_csc: This is a wrapper function
which checks the platform type, and calls the valleyview
specific set_csc function. As different platforms may have different
methods of setting CSC, wrapper function is required to route to proper
core CSC set function. In future, the support for other platfroms can be
plugged-in here. Adding this function as .set_property CSC color
property.
4. Adds function vlv_set_csc: core function to program CSC coefficients as
per
vlv specs, and then enable CSC.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
I haven't read up on the hardware programming side of this code to give
any comments there, but I got a bit lost trying to follow your blob
upload handling here. Like Bob noted, it kind of looks like you're
trying to add userspace blob property upload functionality that would
really belong in the DRM core. However, in the intermediate/long term
there probably isn't really a need for this kind of blob upload support
because the atomic propertysets will provide the functionality you need;
once we have atomic support, I think it would be better to just make
each of these values an independent property and upload all of the
values together as part of a single property set. But I realize you're
specifically trying to add add this support in a pre-atomic world which
makes things more challenging. Atomic is definitely coming, but I think
the timeframe is kind of uncertain still, so it's really going to be up
to the upstream maintainers on how to proceed. Maybe Daniel can give
you some direction?
I've thought the csc stuff here is just a matrix of register values, and
highly intel specific at that. So might as well keep it as a blob property
for now until either the specific layout changes or some standard for
generic csc emerges.
Overall I still think there's a bit too much indirection - I don't see why
the color manager needs to sit in a separate file with separate
registration functions. Doing it like that rips apart the per-crtc
setup/teardown quite a lot and isn't how properties are handled anywhere
else.
-Daniel
Matt
---
drivers/gpu/drm/drm_crtc.c | 4 +-
drivers/gpu/drm/i915/i915_reg.h | 11 ++
drivers/gpu/drm/i915/intel_clrmgr.c | 208
+++-
drivers/gpu/drm/i915/intel_clrmgr.h | 16 +++
drivers/gpu/drm/i915/intel_drv.h| 1 +
include/drm/drm_crtc.h | 7 ++
6 files changed, 244 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 272b66f..be9d110 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3917,7 +3917,7 @@ done:
return ret;
}
-static struct drm_property_blob *drm_property_create_blob(struct
drm_device *dev, int length,
+struct drm_property_blob *drm_property_create_blob(struct drm_device *dev,
int length,
void *data)
{
struct drm_property_blob *blob;
@@ -3944,7 +3944,7 @@ static struct drm_property_blob
*drm_property_create_blob(struct drm_device *dev
return blob;
}
-static void drm_property_destroy_blob(struct drm_device *dev,
+void drm_property_destroy_blob(struct drm_device *dev,
struct drm_property_blob *blob)
{
drm_mode_object_put(dev, blob-base);
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index 20673cc..e3010b3 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6183,6 +6183,17 @@ enum punit_power_well {
#define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME,
_PIPE_B_CSC_POSTOFF_ME)
#define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO,
_PIPE_B_CSC_POSTOFF_LO)
+/* VLV color correction registers */
+/* CSC */
+#define PIPECONF_CSC_ENABLE(1 15)
+#define _PIPEACSC (dev_priv-info.display_mmio_offset + \
+ 0x600b0)
+#define _PIPEBCSC (dev_priv-info.display_mmio_offset + \
+ 0x610b0)
+#define PIPECSC(pipe) (_PIPEACSC + (pipe * CSC_OFFSET))
+#define CSC_OFFSET (_PIPEBCSC - _PIPEACSC)
+#define PIPECSC(pipe) (_PIPEACSC +
Re: [Intel-gfx] [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write
On Tue, Sep 09, 2014 at 02:25:50PM -0700, Jesse Barnes wrote:
On Tue, 09 Sep 2014 21:45:08 +0530
Deepak S deepa...@intel.com wrote:
On Monday 08 September 2014 08:10 PM, Daniel Vetter wrote:
On Mon, Sep 08, 2014 at 05:14:23PM +0300, Ville Syrjälä wrote:
On Mon, Sep 08, 2014 at 05:02:43PM +0300, Ville Syrjälä wrote:
On Tue, Sep 09, 2014 at 07:14:16PM +0530,
deepa...@linux.intel.com wrote:
From: Deepak S deepa...@linux.intel.com
In chv, we have two power wells Render Media. We need to use
corresponsing forcewake count. If we dont follow this we are
getting error *ERROR*: Timed out waiting for forcewake old ack
to clear due to multiple entry into __vlv_force_wake_get.
Signed-off-by: Deepak S deepa...@linux.intel.com
---
drivers/gpu/drm/i915/intel_lrc.c | 29
+ 1 file changed, 25 insertions(+),
4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c
b/drivers/gpu/drm/i915/intel_lrc.c index bd1b28d..bafd38b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -300,8 +300,18 @@ static void execlists_elsp_write(struct
intel_engine_cs *ring,
* Instead, we do the runtime_pm_get/put when
creating/destroying requests. */
spin_lock_irqsave(dev_priv-uncore.lock, flags);
- if (dev_priv-uncore.forcewake_count++ == 0)
- dev_priv-uncore.funcs.force_wake_get(dev_priv,
FORCEWAKE_ALL);
+ if (IS_CHERRYVIEW(dev_priv-dev)) {
+ if (dev_priv-uncore.fw_rendercount++ == 0)
+
dev_priv-uncore.funcs.force_wake_get(dev_priv,
+
FORCEWAKE_RENDER);
+ if (dev_priv-uncore.fw_mediacount++ == 0)
+
dev_priv-uncore.funcs.force_wake_get(dev_priv,
+
FORCEWAKE_MEDIA);
This will wake both wells. Is that needed or should we just pick
one based on the ring?
Also unlike the comment says runtime_pm_get() can't sleep since
someone must already be holding a reference, othwewise we surely
can't go writing any registers. So in theory we should be able to
call gen6_gt_force_wake_get() here, but maybe that would trigger a
might_sleep() warning. the current force wake code duplication
(esp. outside intel_uncore.c) is rather unfortunate and I'd like
to see it killed off. Maybe we just need to pull the rpm get/put
outside gen6_gt_force_wake_get()? I never really liked hiding it
there anyway.
Yeah this is just broken design. And if you look at the other wheel
to track outstanding gpu work (requests) then it's not needed at
all.
But I'm not sure what's the priority of the rework execlists to use
requests task is and when (if ever that will happen). Jesse is the
arbiter for this stuff anyway, so adding him.
-Daniel
hmm , agreed do we have a reworked execlist? The reason why added
this, on chv when i enable execlist, due to incorrect forcewake count
is causing multiple entries to forcewake_get resulting in *ERROR*:
Timed out waiting for forcewake old ack to clear and Hang.
I'm hoping we can get execlists reworked on top of the request/seqno
stuff shortly after it lands, but I don't think that's a reason to
block this fix, since Chris is still busy fixing up the request
changes.
Queued for -next, thanks for the patch.
-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] drm: Move vblank related module options into drm_irq.c
This allows us to drop 2 header declarations from drmP.h. The 3rd one is also used in drm_ioctl.c, so for that create a new drm_internal.h header for non-legacy non-kms (since we have internal headers for those parts already) declarations private to drm.ko. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/drm_drv.c | 13 - drivers/gpu/drm/drm_internal.h | 24 drivers/gpu/drm/drm_ioctl.c| 1 + drivers/gpu/drm/drm_irq.c | 15 +++ include/drm/drmP.h | 4 5 files changed, 40 insertions(+), 17 deletions(-) create mode 100644 drivers/gpu/drm/drm_internal.h diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 184a2f07eee0..a1cb795bae76 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -39,16 +39,6 @@ unsigned int drm_debug = 0;/* 1 to enable debug output */ EXPORT_SYMBOL(drm_debug); -int drm_vblank_offdelay = 5000;/* Default to 5000 msecs. */ - -unsigned int drm_timestamp_precision = 20; /* Default to 20 usecs. */ - -/* - * Default to use monotonic timestamps for wait-for-vblank and page-flip - * complete events. - */ -unsigned int drm_timestamp_monotonic = 1; - MODULE_AUTHOR(CORE_AUTHOR); MODULE_DESCRIPTION(CORE_DESC); MODULE_LICENSE(GPL and additional rights); @@ -58,9 +48,6 @@ MODULE_PARM_DESC(timestamp_precision_usec, Max. error on timestamps [usecs]); MODULE_PARM_DESC(timestamp_monotonic, Use monotonic timestamps); module_param_named(debug, drm_debug, int, 0600); -module_param_named(vblankoffdelay, drm_vblank_offdelay, int, 0600); -module_param_named(timestamp_precision_usec, drm_timestamp_precision, int, 0600); -module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600); static DEFINE_SPINLOCK(drm_minor_lock); static struct idr drm_minors_idr; diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h new file mode 100644 index ..5fbf36bcc0b0 --- /dev/null +++ b/drivers/gpu/drm/drm_internal.h @@ -0,0 +1,24 @@ +/* + * Copyright © 2014 Intel Corporation + * Daniel Vetter daniel.vet...@ffwll.ch + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the Software), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + */ + +extern unsigned int drm_timestamp_monotonic; diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 40be746b7e68..257cf137e6b7 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -31,6 +31,7 @@ #include drm/drmP.h #include drm/drm_core.h #include drm_legacy.h +#include drm_internal.h #include linux/pci.h #include linux/export.h diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index ea547877dee4..a75da075927c 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -34,6 +34,7 @@ #include drm/drmP.h #include drm_trace.h +#include drm_internal.h #include linux/interrupt.h /* For task queue support */ #include linux/slab.h @@ -55,6 +56,20 @@ */ #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 100 +static unsigned int drm_timestamp_precision = 20; /* Default to 20 usecs. */ + +/* + * Default to use monotonic timestamps for wait-for-vblank and page-flip + * complete events. + */ +unsigned int drm_timestamp_monotonic = 1; + +static int drm_vblank_offdelay = 5000;/* Default to 5000 msecs. */ + +module_param_named(vblankoffdelay, drm_vblank_offdelay, int, 0600); +module_param_named(timestamp_precision_usec, drm_timestamp_precision, int, 0600); +module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600); + /** * drm_update_vblank_count - update the master vblank counter * @dev: DRM device diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 2d64e08b22dd..595fde39e406 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1358,10 +1358,6 @@ extern void drm_put_dev(struct drm_device *dev); extern void drm_unplug_dev(struct drm_device *dev); extern unsigned int drm_debug; -extern int
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview
Hello Bob,
Thanks for your time and review comments.
Please find my comments inline.
Regards
Shashank
On 9/10/2014 4:21 AM, Bob Paauwe wrote:
On Tue, 9 Sep 2014 11:53:13 +0530
shashank.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@intel.com
Color manager is a framework which adds drm properties for
color correction in I915 driver. This framework creates DRM
properties for each color correction feature, and attaches
it to appropriate CRTC/plane based on the property type.
This allows userspace to fine tune the display as per the panel.
This is the first patch of the series, what this patch does is:
1. Create 2 new files
intel_clrmgr.c
intel_clrmgr.h
2. Add color manager init function, This function will create
DRM properties for color correction.
3. Add color manager exit function. This function will destroy
registered DRM color properties.
4. Adds a enum for currently listed color correction properties:
they are:
CSC correction (wide gamut), Gamma correction, Contrast,
Brightness, Hue and Saturation
This enum will be further used to index color properties
from array of elements.
5. Add names for vlv color properties.
I'd suggest maybe dropping the name color manager and clrmgr in
favor of color properties and maybe color props as a shorter form.
I will try to comply with this suggestion.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/Makefile| 3 +-
drivers/gpu/drm/i915/intel_clrmgr.c | 80
drivers/gpu/drm/i915/intel_clrmgr.h | 70 +++
drivers/gpu/drm/i915/intel_display.c | 5 +++
4 files changed, 157 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.c
create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index c1dd485..6361c9b 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -46,7 +46,8 @@ i915-y += intel_bios.o \
intel_modes.o \
intel_overlay.o \
intel_sideband.o \
- intel_sprite.o
+ intel_sprite.o \
+ intel_clrmgr.o
i915-$(CONFIG_ACPI) += intel_acpi.o intel_opregion.o
i915-$(CONFIG_DRM_I915_FBDEV) += intel_fbdev.o
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
new file mode 100644
index 000..0def917
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -0,0 +1,80 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the Software),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ * ==
+ * Shashank Sharma shashank.sha...@intel.com
+ * Uma Shankar uma.shan...@intel.com
+ * Sonika Jindal sonika.jin...@intel.com
+ */
+
+#include i915_drm.h
+#include i915_drv.h
+#include i915_reg.h
+#include intel_clrmgr.h
+
+/* Names to register with color properties */
+const char *clrmgr_property_names[] = {
+ /* csc */
+ csc-correction,
+ /* gamma */
+ gamma-correction,
+ /* contrast */
+ contrast,
+ /* brightness */
+ brightness,
+ /* hue_saturation */
+ hue_saturation
+ /* add a new prop name here */
+};
I don't think you need the comments for each of the names. The names
themselves are descriptive.
I haven't looked at the rest of the series yet, but in embedded we've
considered hue and saturation separate properties.
Actually they are, but in VLV hardware they are getting
programmed/adjusted via the same register (single), so I am force to
combine them together.
It looks like you drop this array in the second patch. You should
simply not introduce it at all. Oh an then you re-introduce this array
back in patch 3 but only the first part of it.
Actually my plan was to add an empty array, and then add names, sizes
along with the corresponding
Re: [Intel-gfx] [PATCH] drm/i915: pin sprite fb only if it changed
On Tue, Sep 09, 2014 at 05:49:46PM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.pado...@collabora.co.uk
Optimize code avoiding helding dev mutex if old fb and current fb
are the same.
Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
---
drivers/gpu/drm/i915/intel_sprite.c | 23 ++-
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index a4306cf..a301838 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1038,21 +1038,23 @@ intel_commit_sprite_plane(struct drm_plane *plane,
primary_enabled = !drm_rect_equals(dst, clip) ||
colorkey_enabled(intel_plane);
WARN_ON(!primary_enabled !state-visible intel_crtc-active);
- mutex_lock(dev-struct_mutex);
/* Note that this will apply the VT-d workaround for scanouts,
* which is more restrictive than required for sprites. (The
* primary plane requires 256KiB alignment with 64 PTE padding,
* the sprite planes only require 128KiB alignment and 32 PTE padding.
*/
The comment should probably remain just before the
intel_pin_and_fence_fb_obj() call.
- ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+ if (old_obj != obj) {
+ mutex_lock(dev-struct_mutex);
+ ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
- i915_gem_track_fb(old_obj, obj,
- INTEL_FRONTBUFFER_SPRITE(pipe));
- mutex_unlock(dev-struct_mutex);
+ i915_gem_track_fb(old_obj, obj,
+ INTEL_FRONTBUFFER_SPRITE(pipe));
I don't think we should be calling this when things failed. This bug was
already present in the code, but might as well fix it while you're at
it.
+ mutex_unlock(dev-struct_mutex);
- if (ret)
- return ret;
+ if (ret)
+ return ret;
+ }
intel_plane-crtc_x = state-orig_dst.x1;
intel_plane-crtc_y = state-orig_dst.y1;
@@ -1098,15 +1100,18 @@ intel_commit_sprite_plane(struct drm_plane *plane,
intel_post_enable_primary(crtc);
}
+ if (!old_obj)
+ return 0;
+
/* Unpin old obj after new one is active to avoid ugliness */
- if (old_obj) {
+ if (old_obj != obj) {
I'd prefer
if (old_obj old_obj != obj) {
in this case.
/*
* It's fairly common to simply update the position of
* an existing object. In that case, we don't need to
* wait for vblank to avoid ugliness, we only need to
* do the pin ref bookkeeping.
*/
- if (old_obj != obj intel_crtc-active)
+ if (intel_crtc-active)
intel_wait_for_vblank(dev, intel_crtc-pipe);
mutex_lock(dev-struct_mutex);
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: CSC color correction
Thanks for your time and review.
Please find my comments inline.
Regards
Shashank
On 9/10/2014 4:21 AM, Bob Paauwe wrote:
On Tue, 9 Sep 2014 11:53:15 +0530
shashank.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@intel.com
This patch adds support for CSC correction color property.
It does the following:
1. Creates a new DRM property for CSC correction. Adds this into
mode_config.
2. Attaches this CSC property to calling CRTC. Creates a blob
to store the correction values, and attaches the blob to CRTC.
3. Adds function intel_clrmgr_set_csc: This is a wrapper function
which checks the platform type, and calls the valleyview
specific set_csc function. As different platforms may have different
methods of setting CSC, wrapper function is required to route to proper
core CSC set function. In future, the support for other platfroms can be
plugged-in here. Adding this function as .set_property CSC color property.
4. Adds function vlv_set_csc: core function to program CSC coefficients as per
vlv specs, and then enable CSC.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/drm_crtc.c | 4 +-
drivers/gpu/drm/i915/i915_reg.h | 11 ++
drivers/gpu/drm/i915/intel_clrmgr.c | 208 +++-
drivers/gpu/drm/i915/intel_clrmgr.h | 16 +++
drivers/gpu/drm/i915/intel_drv.h| 1 +
include/drm/drm_crtc.h | 7 ++
6 files changed, 244 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 272b66f..be9d110 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3917,7 +3917,7 @@ done:
return ret;
}
-static struct drm_property_blob *drm_property_create_blob(struct drm_device
*dev, int length,
+struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int
length,
void *data)
{
struct drm_property_blob *blob;
@@ -3944,7 +3944,7 @@ static struct drm_property_blob
*drm_property_create_blob(struct drm_device *dev
return blob;
}
-static void drm_property_destroy_blob(struct drm_device *dev,
+void drm_property_destroy_blob(struct drm_device *dev,
struct drm_property_blob *blob)
{
drm_mode_object_put(dev, blob-base);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 20673cc..e3010b3 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6183,6 +6183,17 @@ enum punit_power_well {
#define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME,
_PIPE_B_CSC_POSTOFF_ME)
#define PIPE_CSC_POSTOFF_LO(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO,
_PIPE_B_CSC_POSTOFF_LO)
+/* VLV color correction registers */
+/* CSC */
+#define PIPECONF_CSC_ENABLE(1 15)
+#define _PIPEACSC (dev_priv-info.display_mmio_offset + \
+ 0x600b0)
+#define _PIPEBCSC (dev_priv-info.display_mmio_offset + \
+ 0x610b0)
+#define PIPECSC(pipe) (_PIPEACSC + (pipe * CSC_OFFSET))
+#define CSC_OFFSET (_PIPEBCSC - _PIPEACSC)
+#define PIPECSC(pipe) (_PIPEACSC + (pipe * CSC_OFFSET))
+
/* VLV MIPI registers */
#define _MIPIA_PORT_CTRL (VLV_DISPLAY_BASE + 0x61190)
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
index ac0a890..36c64c1 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -32,6 +32,138 @@
#include i915_reg.h
#include intel_clrmgr.h
+/*
+* Names to register with color properties.
+* Sequence must be the same as the order
+* of enum clrmgr_tweaks
+*/
+const char *clrmgr_property_names[] = {
+ /* csc */
+ csc-correction,
+ /* add a new prop name here */
+};
+
+
+/*
+* Sizes for color properties. This can differ
+* platform by platform, hence 'vlv' prefix
+* The sequence must be same as the order of
+* enum clrmgr_tweaks
+*/
+u32 vlv_color_property_sizes[] = {
+ VLV_CSC_MATRIX_MAX_VALS,
+ /* Add new property size here */
+};
+
+/* Default CSC values to create property with */
+uint64_t vlv_csc_default[VLV_CSC_MATRIX_MAX_VALS] = {
+ 0x400, 0, 0, 0, 0x400, 0, 0, 0, 0x400
+};
+
+/*
+* vlv_set_csc
+* Valleyview specific csc correction method.
+* Programs the 6 csc registers with 3x3 correction matrix
+* values.
+* inputs:
+* - intel_crtc*
+* - color manager registered property for csc correction
+* - data: pointer to correction values to be applied
The comment isn't matching the function. data is not a pointer, it's a
single 64bit value. Also, the boolean enable isn't described in the
comment block.
Yes,Thanks for pointing out. Missed updating comments after design
change. I will fix this.
+*/
+/*
[Intel-gfx] [PATCH] drm/i915: Use GGTT batchbuffer selector
gen6 and earlier conflate address space selection (ppgtt vs ggtt) with
the security bit (i.e. only privileged batches were allowed to run from
ggtt). From Haswell onwards, you are able to select the security bit
separate from the address space - and we always requested to use ppgtt.
This breaks the golden render state batch execution as that is only
present in the global GTT and so we need to disable the use of the ppgtt
selector bit, or else we hang immediately upon boot and thence after
every GPU reset...
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
drivers/gpu/drm/i915/i915_gem_render_state.c | 2 +-
drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++--
drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +++--
4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1a0611bb576b..8ff448dc8be4 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1260,7 +1260,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
if (!file-is_master || !capable(CAP_SYS_ADMIN))
return -EPERM;
- flags |= I915_DISPATCH_SECURE;
+ flags |= I915_DISPATCH_SECURE | I915_DISPATCH_GGTT;
}
if (args-flags I915_EXEC_IS_PINNED)
flags |= I915_DISPATCH_PINNED;
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c
b/drivers/gpu/drm/i915/i915_gem_render_state.c
index a9a62d75aa57..a158d610720b 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -165,7 +165,7 @@ int i915_gem_render_state_init(struct intel_engine_cs *ring)
ret = ring-dispatch_execbuffer(ring,
so.ggtt_offset,
so.rodata-batch_items * 4,
- I915_DISPATCH_SECURE);
+ I915_DISPATCH_GGTT);
if (ret)
goto out;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 109de2eeb9a8..d053819407da 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2174,7 +2174,7 @@ gen8_ring_dispatch_execbuffer(struct intel_engine_cs
*ring,
u64 offset, u32 len,
unsigned flags)
{
- bool ppgtt = USES_PPGTT(ring-dev) !(flags I915_DISPATCH_SECURE);
+ bool ppgtt = USES_PPGTT(ring-dev) !(flags I915_DISPATCH_GGTT);
int ret;
ret = intel_ring_begin(ring, 4);
@@ -2203,7 +2203,8 @@ hsw_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
return ret;
intel_ring_emit(ring,
- MI_BATCH_BUFFER_START | MI_BATCH_PPGTT_HSW |
+ MI_BATCH_BUFFER_START |
+ (flags I915_DISPATCH_GGTT ? 0 : MI_BATCH_PPGTT_HSW) |
(flags I915_DISPATCH_SECURE ? 0 :
MI_BATCH_NON_SECURE_HSW));
/* bit0-7 is the length on GEN6+ */
intel_ring_emit(ring, offset);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 96479c89f4bd..755585a6fcc7 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -169,8 +169,9 @@ struct intel_engine_cs {
int (*dispatch_execbuffer)(struct intel_engine_cs *ring,
u64 offset, u32 length,
unsigned flags);
-#define I915_DISPATCH_SECURE 0x1
-#define I915_DISPATCH_PINNED 0x2
+#define I915_DISPATCH_GGTT 0x1
+#define I915_DISPATCH_SECURE 0x2
+#define I915_DISPATCH_PINNED 0x4
void(*cleanup)(struct intel_engine_cs *ring);
/* GEN8 signal/wait table - never trust comments!
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Use GGTT batchbuffer selector
On Wed, Sep 10, 2014 at 10:26:54AM +0100, Chris Wilson wrote:
gen6 and earlier conflate address space selection (ppgtt vs ggtt) with
the security bit (i.e. only privileged batches were allowed to run from
ggtt). From Haswell onwards, you are able to select the security bit
separate from the address space - and we always requested to use ppgtt.
This breaks the golden render state batch execution as that is only
present in the global GTT and so we need to disable the use of the ppgtt
selector bit, or else we hang immediately upon boot and thence after
every GPU reset...
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
drivers/gpu/drm/i915/i915_gem_render_state.c | 2 +-
drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++--
drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +++--
4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1a0611bb576b..8ff448dc8be4 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1260,7 +1260,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void
*data,
if (!file-is_master || !capable(CAP_SYS_ADMIN))
return -EPERM;
- flags |= I915_DISPATCH_SECURE;
+ flags |= I915_DISPATCH_SECURE | I915_DISPATCH_GGTT;
}
if (args-flags I915_EXEC_IS_PINNED)
flags |= I915_DISPATCH_PINNED;
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c
b/drivers/gpu/drm/i915/i915_gem_render_state.c
index a9a62d75aa57..a158d610720b 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -165,7 +165,7 @@ int i915_gem_render_state_init(struct intel_engine_cs
*ring)
ret = ring-dispatch_execbuffer(ring,
so.ggtt_offset,
so.rodata-batch_items * 4,
- I915_DISPATCH_SECURE);
+ I915_DISPATCH_GGTT);
if (ret)
goto out;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 109de2eeb9a8..d053819407da 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2174,7 +2174,7 @@ gen8_ring_dispatch_execbuffer(struct intel_engine_cs
*ring,
u64 offset, u32 len,
unsigned flags)
{
- bool ppgtt = USES_PPGTT(ring-dev) !(flags I915_DISPATCH_SECURE);
+ bool ppgtt = USES_PPGTT(ring-dev) !(flags I915_DISPATCH_GGTT);
int ret;
ret = intel_ring_begin(ring, 4);
@@ -2203,7 +2203,8 @@ hsw_ring_dispatch_execbuffer(struct intel_engine_cs
*ring,
return ret;
intel_ring_emit(ring,
- MI_BATCH_BUFFER_START | MI_BATCH_PPGTT_HSW |
+ MI_BATCH_BUFFER_START |
+ (flags I915_DISPATCH_GGTT ? 0 : MI_BATCH_PPGTT_HSW) |
(flags I915_DISPATCH_SECURE ? 0 :
MI_BATCH_NON_SECURE_HSW));
/* bit0-7 is the length on GEN6+ */
intel_ring_emit(ring, offset);
I thought I had also fixed up gen6 like:
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 0231144d5ef1..ce90f43cecd0 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1761,7 +1761,7 @@ gen6_emit_batchbuffer(struct i915_gem_request *rq,
intel_ring_emit(ring,
MI_BATCH_BUFFER_START |
- (flags I915_DISPATCH_SECURE ? 0 :
MI_BATCH_NON_SECURE_I965));
+ ((flags I915_DISPATCH_SECURE | I915_DISPATCH_GTT) ? 0
: MI_BATCH_NON_SECURE_I965));
/* bit0-7 is the length on GEN6+ */
intel_ring_emit(ring, offset);
intel_ring_advance(ring);
-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: Use GGTT batchbuffer selector
On Wed, Sep 10, 2014 at 10:26:54AM +0100, Chris Wilson wrote:
gen6 and earlier conflate address space selection (ppgtt vs ggtt) with
the security bit (i.e. only privileged batches were allowed to run from
ggtt). From Haswell onwards,
Not onwards unfortunately. BDW went back to the single bit approach.
you are able to select the security bit
separate from the address space - and we always requested to use ppgtt.
This breaks the golden render state batch execution as that is only
present in the global GTT and so we need to disable the use of the ppgtt
selector bit, or else we hang immediately upon boot and thence after
every GPU reset...
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
drivers/gpu/drm/i915/i915_gem_render_state.c | 2 +-
drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++--
drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +++--
4 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1a0611bb576b..8ff448dc8be4 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1260,7 +1260,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void
*data,
if (!file-is_master || !capable(CAP_SYS_ADMIN))
return -EPERM;
- flags |= I915_DISPATCH_SECURE;
+ flags |= I915_DISPATCH_SECURE | I915_DISPATCH_GGTT;
}
if (args-flags I915_EXEC_IS_PINNED)
flags |= I915_DISPATCH_PINNED;
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c
b/drivers/gpu/drm/i915/i915_gem_render_state.c
index a9a62d75aa57..a158d610720b 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -165,7 +165,7 @@ int i915_gem_render_state_init(struct intel_engine_cs
*ring)
ret = ring-dispatch_execbuffer(ring,
so.ggtt_offset,
so.rodata-batch_items * 4,
- I915_DISPATCH_SECURE);
+ I915_DISPATCH_GGTT);
if (ret)
goto out;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 109de2eeb9a8..d053819407da 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2174,7 +2174,7 @@ gen8_ring_dispatch_execbuffer(struct intel_engine_cs
*ring,
u64 offset, u32 len,
unsigned flags)
{
- bool ppgtt = USES_PPGTT(ring-dev) !(flags I915_DISPATCH_SECURE);
+ bool ppgtt = USES_PPGTT(ring-dev) !(flags I915_DISPATCH_GGTT);
int ret;
ret = intel_ring_begin(ring, 4);
@@ -2203,7 +2203,8 @@ hsw_ring_dispatch_execbuffer(struct intel_engine_cs
*ring,
return ret;
intel_ring_emit(ring,
- MI_BATCH_BUFFER_START | MI_BATCH_PPGTT_HSW |
+ MI_BATCH_BUFFER_START |
+ (flags I915_DISPATCH_GGTT ? 0 : MI_BATCH_PPGTT_HSW) |
(flags I915_DISPATCH_SECURE ? 0 :
MI_BATCH_NON_SECURE_HSW));
Do we actually want to make a distinction between GGTT and secure given
that HSW is the only one where it makes any difference? Why not just
set both GGGT and secure bits on HSW when I915_DISPATCH_SECURE is set?
/* bit0-7 is the length on GEN6+ */
intel_ring_emit(ring, offset);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 96479c89f4bd..755585a6fcc7 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -169,8 +169,9 @@ struct intel_engine_cs {
int (*dispatch_execbuffer)(struct intel_engine_cs *ring,
u64 offset, u32 length,
unsigned flags);
-#define I915_DISPATCH_SECURE 0x1
-#define I915_DISPATCH_PINNED 0x2
+#define I915_DISPATCH_GGTT 0x1
+#define I915_DISPATCH_SECURE 0x2
+#define I915_DISPATCH_PINNED 0x4
void(*cleanup)(struct intel_engine_cs *ring);
/* GEN8 signal/wait table - never trust comments!
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Use GGTT batchbuffer selector
On Wed, Sep 10, 2014 at 01:30:45PM +0300, Ville Syrjälä wrote: Do we actually want to make a distinction between GGTT and secure given that HSW is the only one where it makes any difference? Why not just set both GGGT and secure bits on HSW when I915_DISPATCH_SECURE is set? If it is only HSW, then the distinction is moot indeed. -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 0/4] Color manager framework
Hello Matt, Thanks for your detailed descriptions, reviews comments and time. Please find my comments inline. Regards Shashank On 9/10/2014 6:58 AM, Matt Roper wrote: On Tue, Sep 09, 2014 at 11:53:12AM +0530, shashank.sha...@intel.com wrote: From: Shashank Sharma shashank.sha...@intel.com Color manager is an extention to i915 driver which provides display tuning and color-correction properties to user space, via DRM propery interface.Different Intel platforms support different color tuning capabilities which can be exploited using this framework. This patch set adds color correction for VLV, and the code is written in such a way that new color properties and support for other platforms can be pluggen in, using the same architecture. For the design discussion, right now only one color correction property (CSC) has been added. Once we agree on the design, gamma-correction, contrast, brightness, and hue/saturation would be followed by the same. What this patch set does, is: 1. Color manager framework for valleyview: Add basic functions of framework, to create and destroy DRM properties for color correction. It also adds header, enums and defines. 2. Plug-in color manager attach Attach created color properties, with the objects. Basically properties get attached to two type of objects, crtc (pipe level correction) and plane (plane level correction). 3. CSC color correction First color correction method. - Add color property for CSC during init. - Add blob to keep correction values - Attach DRM CSC propery with each CRTC object - Core CSC correction for VLV 4. Add set_property function for intel CRTC. This function checks the type of the property, and calls the appropriate high level function (intel_clrmgr_set_csc), which does platform check and calls the core platform color correction function (vlv_set_csc) V2: Re-designed based on review comments and suggestions from Matt Roper and Daniel. - Creating only one property per color correction, and attaching it to every DRM object. - No additional data structures on top of DRM property. - No registeration of color property, just create and attach. - Added properties in dev-mode_config Hi Shashank, thanks for incorporating the feedback from the last review round. This patchset is definitely moving in the right direction, although I still feel that there's still a little bit of unnecessary work/complexity in creating a framework here where we probably don't need it. I'll give some more detailed responses on your individual patches, but at a high level I don't really see the need to treat color correction properties (csc, gamma, etc.) in a special way with their own registration framework. There are really three things to consider here: * How/where to create the properties * How/where to attach properties to CRTC's and/or planes * How to handle property changes For creating properties, at the moment you're doing that via intel_modeset_init() - intel_clrmgr_init() - intel_clrmgr_create_color_properties(). Presumably we'll add other (non-color correction) properties to the driver in the future, so it seems like it would be simpler if we just renamed your intel_clrmgr_create_color_properties() to something like intel_modeset_create_properties() and called it directly from intel_modeset_init(). I don't see the intermediate intel_clrmgr_init() adding any value at the moment, so I think it can be removed. I got this point Matt, and we can do this. The only confusion I have is, we already have places where we create properties, and if we are intending to create different type of properties in this function, would that align with the previous property creations places ? At least we have a common ground among the color properties, that's why I am sticking them together. If you dont like this, the other common ground is CRTC/plane properties. These are the first of CRTC or plane properties. So I can create a intel_create_crtc_properties() For attaching properties, I'm not sure I see where that is happening in your current patchset. You have an intel_attach_pipe_color_correction() function that sounds promising, but the implementation doesn't seem to actually be calling drm_object_attach_property() that I can see; instead it seems to be creating a blob value and trying to set it on the object. Honestly I think all you really need is a single call to: drm_object_attach_property(intel_crtc-base.base, dev-mode_config.csc_property, 0); Yes, I was suppose to do this, and then create a blob. I will change this. at the bottom of intel_crtc_init() where you have have your call to intel_attach_pipe_color_correction() right now. I'm not sure if this code is expected to stay VLV-specific or whether you've only provided a single platform for RFC/POC purposes, but if it is expected to stay limited to VLV you'll probably also want to do an 'if (IS_VALLEYVIEW(dev_priv))' before doing the attach so that the property
[Intel-gfx] [PATCH] drm/i915: HSW always use GGTT selector for secure batches
gen6 and earlier conflate address space selection (ppgtt vs ggtt) with the security bit (i.e. only privileged batches were allowed to run from ggtt). From Haswell onwards, you are able to select the security bit separate from the address space - and we always requested to use ppgtt. This breaks the golden render state batch execution with full-ppgtt as that is only present in the global GTT and more generally any secure batch that is not colocated in the ppgtt and ggtt. So we need to disable the use of the ppgtt selector bit for secure batches, or else we hang immediately upon boot and thence after every GPU reset... v2: Only HSW differentiates between secure dispatch and ggtt, so simply ignore the differentiation and always use secure==ggtt. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 109de2eeb9a8..25795f2efdcb 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2203,8 +2203,9 @@ hsw_ring_dispatch_execbuffer(struct intel_engine_cs *ring, return ret; intel_ring_emit(ring, - MI_BATCH_BUFFER_START | MI_BATCH_PPGTT_HSW | - (flags I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE_HSW)); + MI_BATCH_BUFFER_START | + (flags I915_DISPATCH_SECURE ? +0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW)); /* bit0-7 is the length on GEN6+ */ intel_ring_emit(ring, offset); intel_ring_advance(ring); -- 2.1.0 ___ 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: Color manager framework for valleyview
Matt,
Thanks for your time and comments.
Please find mine inline.
Regards
Shashank
On 9/10/2014 6:59 AM, Matt Roper wrote:
On Tue, Sep 09, 2014 at 11:53:13AM +0530, shashank.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@intel.com
Color manager is a framework which adds drm properties for
color correction in I915 driver. This framework creates DRM
properties for each color correction feature, and attaches
it to appropriate CRTC/plane based on the property type.
This allows userspace to fine tune the display as per the panel.
This is the first patch of the series, what this patch does is:
1. Create 2 new files
intel_clrmgr.c
intel_clrmgr.h
2. Add color manager init function, This function will create
DRM properties for color correction.
3. Add color manager exit function. This function will destroy
registered DRM color properties.
4. Adds a enum for currently listed color correction properties:
they are:
CSC correction (wide gamut), Gamma correction, Contrast,
Brightness, Hue and Saturation
This enum will be further used to index color properties
from array of elements.
5. Add names for vlv color properties.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/Makefile| 3 +-
drivers/gpu/drm/i915/intel_clrmgr.c | 80
drivers/gpu/drm/i915/intel_clrmgr.h | 70 +++
drivers/gpu/drm/i915/intel_display.c | 5 +++
4 files changed, 157 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.c
create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index c1dd485..6361c9b 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -46,7 +46,8 @@ i915-y += intel_bios.o \
intel_modes.o \
intel_overlay.o \
intel_sideband.o \
- intel_sprite.o
+ intel_sprite.o \
+ intel_clrmgr.o
i915-$(CONFIG_ACPI) += intel_acpi.o intel_opregion.o
i915-$(CONFIG_DRM_I915_FBDEV) += intel_fbdev.o
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
new file mode 100644
index 000..0def917
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -0,0 +1,80 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the Software),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ * ==
+ * Shashank Sharma shashank.sha...@intel.com
+ * Uma Shankar uma.shan...@intel.com
+ * Sonika Jindal sonika.jin...@intel.com
+ */
+
+#include i915_drm.h
+#include i915_drv.h
+#include i915_reg.h
+#include intel_clrmgr.h
+
+/* Names to register with color properties */
+const char *clrmgr_property_names[] = {
+ /* csc */
+ csc-correction,
+ /* gamma */
+ gamma-correction,
+ /* contrast */
+ contrast,
+ /* brightness */
+ brightness,
+ /* hue_saturation */
+ hue_saturation
+ /* add a new prop name here */
+};
I don't think you really need this array. A bunch of calls to
drm_property_create() with string literals for the property names seems
plenty clear to me.
Ok, got it.
+
+int intel_clrmgr_create_color_properties(struct drm_device *dev)
+{
+ DRM_DEBUG_DRIVER(\n);
+ return 0;
+}
+
+void intel_clrmgr_destroy_color_properties(struct drm_device *dev)
+{
+ DRM_DEBUG_DRIVER(\n);
+}
+
+void intel_clrmgr_init(struct drm_device *dev)
+{
+ int ret;
+
+ /* Create color properties */
+ ret = intel_clrmgr_create_color_properties(dev);
+ if (ret) {
+ DRM_ERROR(Unable to create %d propert%s\n,
+ ret, ret 1 ? ies : y);
+ return;
+ }
+ DRM_DEBUG_DRIVER(Successfully created color properties\n);
+}
As I noted in reply to your cover letter, I don't
Re: [Intel-gfx] [PATCH] drm/i915: HSW always use GGTT selector for secure batches
On Wed, Sep 10, 2014 at 12:18:27PM +0100, Chris Wilson wrote: gen6 and earlier conflate address space selection (ppgtt vs ggtt) with the security bit (i.e. only privileged batches were allowed to run from ggtt). From Haswell onwards, you are able to select the security bit ggtt). For Haswell only, you are able to select the security bit separate from the address space - and we always requested to use ppgtt. This breaks the golden render state batch execution with full-ppgtt as that is only present in the global GTT and more generally any secure batch that is not colocated in the ppgtt and ggtt. So we need to disable the use of the ppgtt selector bit for secure batches, or else we hang immediately upon boot and thence after every GPU reset... v2: Only HSW differentiates between secure dispatch and ggtt, so simply ignore the differentiation and always use secure==ggtt. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 109de2eeb9a8..25795f2efdcb 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2203,8 +2203,9 @@ hsw_ring_dispatch_execbuffer(struct intel_engine_cs *ring, return ret; intel_ring_emit(ring, - MI_BATCH_BUFFER_START | MI_BATCH_PPGTT_HSW | - (flags I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE_HSW)); + MI_BATCH_BUFFER_START | + (flags I915_DISPATCH_SECURE ? + 0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW)); /* bit0-7 is the length on GEN6+ */ intel_ring_emit(ring, offset); intel_ring_advance(ring); -- 2.1.0 -- 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/4] drm/i915: Plug-in color manager attach
Matt,
Thanks for your time and review comments.
Please find mine inline.
Regards
Shashank
On 9/10/2014 6:59 AM, Matt Roper wrote:
On Tue, Sep 09, 2014 at 11:53:14AM +0530, shashank.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@intel.com
This patch does following things:
1. Adds new function to attach color proprties with
corresponsing crtc / plane objects.
2. Call these attach functions, from corresponding crtc/plane
init functions.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/intel_clrmgr.c | 25 +++--
drivers/gpu/drm/i915/intel_clrmgr.h | 22 ++
drivers/gpu/drm/i915/intel_display.c | 2 ++
drivers/gpu/drm/i915/intel_sprite.c | 3 +++
4 files changed, 38 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
index 0def917..ac0a890 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.c
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -32,20 +32,17 @@
#include i915_reg.h
#include intel_clrmgr.h
-/* Names to register with color properties */
-const char *clrmgr_property_names[] = {
- /* csc */
- csc-correction,
- /* gamma */
- gamma-correction,
- /* contrast */
- contrast,
- /* brightness */
- brightness,
- /* hue_saturation */
- hue_saturation
- /* add a new prop name here */
-};
It looks like you just added this array in the previous patch, remove it
here, and then you add it back (with only a single element) in the next
patch. I suspect this is just an artifact of your rebasing and
respinning the patch series, but if you agree my suggestion on the
previous patch to drop the array completely, please make sure that it's
dropped throughout the series to keep the review simple. :-)
Yes, this is a mistake.
My intention was to add an empty array first, and then with each
property specific patch, add one name in this array. But looks like
its just creating confusions. I will change this.
+void
+intel_attach_plane_color_correction(struct intel_plane *intel_plane)
+{
+ DRM_DEBUG_DRIVER(\n);
+}
+
+void
+intel_attach_pipe_color_correction(struct intel_crtc *intel_crtc)
+{
+ DRM_DEBUG_DRIVER(\n);
+}
I think it's generally a bit easier to review if you just add the
functions with their bodies when you actually have the implementation.
I can see where you call these functions in the code below, but without
the bodies present, it makes it a little harder for reviewers to see
whether those are correct. Since this patch doesn't do anything by
itself, I'd suggest dropping it and just squashing the changes here into
your future patches.
Yes, the same case here. I thought the CSC patch will come and fill all
the functions at a time, and it would be easy to understand, but looks
like it was a bad idea :)
Also, it looks like the plane function remains a noop throughout your
patch series, so I'd just leave it out completely until you start
introducing the plane properties that will use it.
Yes, plane properties are contrast, brightness and hue/sat.
Matt
int intel_clrmgr_create_color_properties(struct drm_device *dev)
{
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.h
b/drivers/gpu/drm/i915/intel_clrmgr.h
index 1b7e906..8cff487 100644
--- a/drivers/gpu/drm/i915/intel_clrmgr.h
+++ b/drivers/gpu/drm/i915/intel_clrmgr.h
@@ -50,6 +50,28 @@ enum clrmgr_tweaks {
};
/*
+* intel_attach_plane_color_correction:
+* Attach plane level color correction DRM properties to
+* corresponding plane objects.
+* This function should be called from plane initialization function
+* for each plane
+* input: intel_plane *
+*/
+void
+intel_attach_plane_color_correction(struct intel_plane *intel_plane);
+
+/*
+* intel_attach_pipe_color_correction:
+* Attach pipe level color correction DRM properties to
+* corresponding CRTC objects.
+* This function should be called from CRTC initialization function
+* for each CRTC
+* input: intel_crtc *
+*/
+void
+intel_attach_pipe_color_correction(struct intel_crtc *intel_crtc);
+
+/*
* intel_clrmgr_init:
* Create drm properties for color correction
* Allocate memory to store current color correction
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index df2dcbd..a289b44 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12017,6 +12017,8 @@ static void intel_crtc_init(struct drm_device *dev, int
pipe)
drm_crtc_helper_add(intel_crtc-base, intel_helper_funcs);
+ intel_attach_pipe_color_correction(intel_crtc);
+
WARN_ON(drm_crtc_index(intel_crtc-base) != intel_crtc-pipe);
return;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index fd5f271..cc061de 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -36,6 +36,7 @@
#include
Re: [Intel-gfx] [PATCH] drm/i915: HSW always use GGTT selector for secure batches
On Wed, Sep 10, 2014 at 12:21:43PM +0100, Chris Wilson wrote: On Wed, Sep 10, 2014 at 12:18:27PM +0100, Chris Wilson wrote: gen6 and earlier conflate address space selection (ppgtt vs ggtt) with the security bit (i.e. only privileged batches were allowed to run from ggtt). From Haswell onwards, you are able to select the security bit ggtt). For Haswell only, you are able to select the security bit separate from the address space - and we always requested to use ppgtt. This breaks the golden render state batch execution with full-ppgtt as that is only present in the global GTT and more generally any secure batch that is not colocated in the ppgtt and ggtt. So we need to disable the use of the ppgtt selector bit for secure batches, or else we hang immediately upon boot and thence after every GPU reset... v2: Only HSW differentiates between secure dispatch and ggtt, so simply ignore the differentiation and always use secure==ggtt. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_ringbuffer.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 109de2eeb9a8..25795f2efdcb 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2203,8 +2203,9 @@ hsw_ring_dispatch_execbuffer(struct intel_engine_cs *ring, return ret; intel_ring_emit(ring, - MI_BATCH_BUFFER_START | MI_BATCH_PPGTT_HSW | - (flags I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE_HSW)); + MI_BATCH_BUFFER_START | + (flags I915_DISPATCH_SECURE ? +0 : MI_BATCH_PPGTT_HSW | MI_BATCH_NON_SECURE_HSW)); /* bit0-7 is the length on GEN6+ */ intel_ring_emit(ring, offset); intel_ring_advance(ring); -- 2.1.0 -- Chris Wilson, Intel Open Source Technology Centre -- 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] drm/i915: Reset RPS events when enabling RPS
After a GPU reset, we reinitialize RPS and RC6 state. (This may be unnecessary, they be preserved across the reset anyway...) Given that the GPU was active before the reset, it is likely that we do have a pending RPS work item and so we should simply disable it rather than emit a warn. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/intel_pm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 9afdeed..3dea174 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3694,7 +3694,7 @@ static void gen8_enable_rps_interrupts(struct drm_device *dev) struct drm_i915_private *dev_priv = dev-dev_private; spin_lock_irq(dev_priv-irq_lock); - WARN_ON(dev_priv-rps.pm_iir); + dev_priv-rps.pm_iir = 0; gen8_enable_pm_irq(dev_priv, dev_priv-rps.pm_events); I915_WRITE(GEN8_GT_IIR(2), dev_priv-rps.pm_events); spin_unlock_irq(dev_priv-irq_lock); @@ -3705,7 +3705,7 @@ static void gen6_enable_rps_interrupts(struct drm_device *dev) struct drm_i915_private *dev_priv = dev-dev_private; spin_lock_irq(dev_priv-irq_lock); - WARN_ON(dev_priv-rps.pm_iir); + dev_priv-rps.pm_iir = 0; gen6_enable_pm_irq(dev_priv, dev_priv-rps.pm_events); I915_WRITE(GEN6_PMIIR, dev_priv-rps.pm_events); spin_unlock_irq(dev_priv-irq_lock); -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: CSC color correction
Hi Daniel,
Overall I still think there's a bit too much indirection - I don't
see why
the color manager needs to sit in a separate file with separate
registration functions. Doing it like that rips apart the per-crtc
setup/teardown quite a lot and isn't how properties are handled anywhere
else.
I dont see any issue creating a new file, for 2 reasons:
- I dont see any value in overpopulating the 11K lines file intel-
display.c any further.
- All these properties are doing color correction, and hence there is
no harm in keeping them in a functionally separate file.
- How does it tear down per CRTC setup creating a separate file :) ?
I can see all standard DRM connector property getting created in
drm_crtc.c, other properties like pfit, force-audio etc are getting
created in intel_modes.c. They are already spread across functional
files, so I find it best to create in color correction file.
Regards
Shashank
On 9/10/2014 12:10 PM, Daniel Vetter wrote:
On Tue, Sep 09, 2014 at 06:30:09PM -0700, Matt Roper wrote:
On Tue, Sep 09, 2014 at 11:53:15AM +0530, shashank.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@intel.com
This patch adds support for CSC correction color property.
It does the following:
1. Creates a new DRM property for CSC correction. Adds this into
mode_config.
2. Attaches this CSC property to calling CRTC. Creates a blob
to store the correction values, and attaches the blob to CRTC.
3. Adds function intel_clrmgr_set_csc: This is a wrapper function
which checks the platform type, and calls the valleyview
specific set_csc function. As different platforms may have different
methods of setting CSC, wrapper function is required to route to proper
core CSC set function. In future, the support for other platfroms can be
plugged-in here. Adding this function as .set_property CSC color property.
4. Adds function vlv_set_csc: core function to program CSC coefficients as per
vlv specs, and then enable CSC.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
I haven't read up on the hardware programming side of this code to give
any comments there, but I got a bit lost trying to follow your blob
upload handling here. Like Bob noted, it kind of looks like you're
trying to add userspace blob property upload functionality that would
really belong in the DRM core. However, in the intermediate/long term
there probably isn't really a need for this kind of blob upload support
because the atomic propertysets will provide the functionality you need;
once we have atomic support, I think it would be better to just make
each of these values an independent property and upload all of the
values together as part of a single property set. But I realize you're
specifically trying to add add this support in a pre-atomic world which
makes things more challenging. Atomic is definitely coming, but I think
the timeframe is kind of uncertain still, so it's really going to be up
to the upstream maintainers on how to proceed. Maybe Daniel can give
you some direction?
I've thought the csc stuff here is just a matrix of register values, and
highly intel specific at that. So might as well keep it as a blob property
for now until either the specific layout changes or some standard for
generic csc emerges.
Overall I still think there's a bit too much indirection - I don't see why
the color manager needs to sit in a separate file with separate
registration functions. Doing it like that rips apart the per-crtc
setup/teardown quite a lot and isn't how properties are handled anywhere
else.
-Daniel
Matt
---
drivers/gpu/drm/drm_crtc.c | 4 +-
drivers/gpu/drm/i915/i915_reg.h | 11 ++
drivers/gpu/drm/i915/intel_clrmgr.c | 208 +++-
drivers/gpu/drm/i915/intel_clrmgr.h | 16 +++
drivers/gpu/drm/i915/intel_drv.h| 1 +
include/drm/drm_crtc.h | 7 ++
6 files changed, 244 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 272b66f..be9d110 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3917,7 +3917,7 @@ done:
return ret;
}
-static struct drm_property_blob *drm_property_create_blob(struct drm_device
*dev, int length,
+struct drm_property_blob *drm_property_create_blob(struct drm_device *dev, int
length,
void *data)
{
struct drm_property_blob *blob;
@@ -3944,7 +3944,7 @@ static struct drm_property_blob
*drm_property_create_blob(struct drm_device *dev
return blob;
}
-static void drm_property_destroy_blob(struct drm_device *dev,
+void drm_property_destroy_blob(struct drm_device *dev,
struct drm_property_blob *blob)
{
drm_mode_object_put(dev, blob-base);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 20673cc..e3010b3 100644
---
[Intel-gfx] [PULL] topic/vblank-rework
Hi Dave, So here's the final bits of Ville's vblank rework with a bit of cleanup from Mario on top. The neat thing this finally allows is to immediately disable the vblank interrupt on the last drm_vblank_put if the hardware has perfectly accurate vblank counter and timestamp readout support. On i915 that required piles of small adjustements from Ville since depending upon the platform and port the vblank happens at different scanout lines. Of course this is fully opt-in and per-device (we need that since gen2 doesn't have a hw vblank counter). Mario reviewed the entire pile too and after some initial hesitation (about drivers without accurate timestampt support) acked it. Cheers, Daniel The following changes since commit 21d70354bba9965a098382fc4d7fb17e138111f3: drm: move drm_stub.c to drm_drv.c (2014-08-06 19:10:44 +1000) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/topic/vblank-rework-2014-09-10 for you to fetch changes up to 2368ffb18b1d2b04eb80478d225676caa7a3c4c8: drm: Use vblank_disable_and_save in drm_vblank_cleanup() (2014-09-10 09:41:29 +0200) Mario Kleiner (2): drm: Remove drm_vblank_cleanup from drm_vblank_init error path. drm: Use vblank_disable_and_save in drm_vblank_cleanup() Ville Syrjälä (16): drm: Always reject drm_vblank_get() after drm_vblank_off() drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off() drm: Don't clear vblank timestamps when vblank interrupt is disabled drm: Move drm_update_vblank_count() drm: Have the vblank counter account for the time between vblank irq disable and drm_vblank_off() drm: Avoid random vblank counter jumps if the hardware counter has been reset drm: Reduce the amount of dev-vblank[crtc] in the code drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock drm: Fix race between drm_vblank_off() and drm_queue_vblank_event() drm: Disable vblank interrupt immediately when drm_vblank_offdelay0 drm: Add dev-vblank_disable_immediate flag drm/i915: Opt out of vblank disable timer on gen2 drm: Kick start vblank interrupts at drm_vblank_on() drm/i915: Update scanline_offset only for active crtcs drm: Fix confusing debug message in drm_update_vblank_count() drm: Store the vblank timestamp when adjusting the counter during disable Documentation/DocBook/drm.tmpl | 7 + drivers/gpu/drm/drm_drv.c| 4 +- drivers/gpu/drm/drm_irq.c| 345 ++- drivers/gpu/drm/i915/i915_irq.c | 8 + drivers/gpu/drm/i915/intel_display.c | 17 +- include/drm/drmP.h | 12 +- 6 files changed, 256 insertions(+), 137 deletions(-) -- 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 3/4] drm/i915: CSC color correction
On Wed, Sep 10, 2014 at 2:05 PM, Sharma, Shashank shashank.sha...@intel.com wrote: - How does it tear down per CRTC setup creating a separate file :) ? I can see all standard DRM connector property getting created in drm_crtc.c, other properties like pfit, force-audio etc are getting created in intel_modes.c. They are already spread across functional files, so I find it best to create in color correction file. I don't mean creating/destroying the properties, but attaching/detaching them. I agree with Matt that that should happen directly in the crtc setup/teardown functions. -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: HSW always use GGTT selector for secure batches
On Wed, Sep 10, 2014 at 03:00:03PM +0300, Ville Syrjälä wrote: On Wed, Sep 10, 2014 at 12:21:43PM +0100, Chris Wilson wrote: On Wed, Sep 10, 2014 at 12:18:27PM +0100, Chris Wilson wrote: gen6 and earlier conflate address space selection (ppgtt vs ggtt) with the security bit (i.e. only privileged batches were allowed to run from ggtt). From Haswell onwards, you are able to select the security bit ggtt). For Haswell only, you are able to select the security bit Rectified. separate from the address space - and we always requested to use ppgtt. This breaks the golden render state batch execution with full-ppgtt as that is only present in the global GTT and more generally any secure batch that is not colocated in the ppgtt and ggtt. So we need to disable the use of the ppgtt selector bit for secure batches, or else we hang immediately upon boot and thence after every GPU reset... v2: Only HSW differentiates between secure dispatch and ggtt, so simply ignore the differentiation and always use secure==ggtt. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com Queued for -next, thanks for the patch. -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] GPU-hang on i830
Hi Daniel, hi Ville, just tried the new 3.17.0+rc4 kernel, though with old userspace (i.e. xserver-xorg-video-intel is *old*, libdrm is old, mesa is old). If I do, I get a GPU hung from xorg.conf. The same userspace works fine on 3.15.0 with patches from Ville. Is this expected behavior or should I open up a bug report (I have dmesg output and debugging output from DRI ready on this, but it's a bith lengthy.) Greetings, Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Reset RPS events when enabling RPS
On Wed, Sep 10, 2014 at 01:01:58PM +0100, Chris Wilson wrote: After a GPU reset, we reinitialize RPS and RC6 state. (This may be unnecessary, they be preserved across the reset anyway...) Given that the GPU was active before the reset, it is likely that we do have a pending RPS work item and so we should simply disable it rather than emit a warn. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk This regression has been introduced in commit dd0a1aa19bd3d7203e58157b84cea78bbac605ac Author: Jeff McGee jeff.mc...@intel.com Date: Tue Feb 4 11:32:31 2014 -0600 drm/i915: Restore rps/rc6 on reset Cc: Jeff McGee jeff.mc...@intel.com Cc: sta...@vger.kernel.org (under the assumption that it blew up in reality and this isn't just a code audit exercise). Adding Jeff. -Daniel --- drivers/gpu/drm/i915/intel_pm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 9afdeed..3dea174 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3694,7 +3694,7 @@ static void gen8_enable_rps_interrupts(struct drm_device *dev) struct drm_i915_private *dev_priv = dev-dev_private; spin_lock_irq(dev_priv-irq_lock); - WARN_ON(dev_priv-rps.pm_iir); + dev_priv-rps.pm_iir = 0; gen8_enable_pm_irq(dev_priv, dev_priv-rps.pm_events); I915_WRITE(GEN8_GT_IIR(2), dev_priv-rps.pm_events); spin_unlock_irq(dev_priv-irq_lock); @@ -3705,7 +3705,7 @@ static void gen6_enable_rps_interrupts(struct drm_device *dev) struct drm_i915_private *dev_priv = dev-dev_private; spin_lock_irq(dev_priv-irq_lock); - WARN_ON(dev_priv-rps.pm_iir); + dev_priv-rps.pm_iir = 0; gen6_enable_pm_irq(dev_priv, dev_priv-rps.pm_events); I915_WRITE(GEN6_PMIIR, dev_priv-rps.pm_events); spin_unlock_irq(dev_priv-irq_lock); -- 1.9.1 ___ 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] GPU-hang on i830
On Wed, Sep 10, 2014 at 02:17:30PM +0200, Thomas Richter wrote: Hi Daniel, hi Ville, just tried the new 3.17.0+rc4 kernel, though with old userspace (i.e. xserver-xorg-video-intel is *old*, libdrm is old, mesa is old). If I do, I get a GPU hung from xorg.conf. The same userspace works fine on 3.15.0 with patches from Ville. Is this expected behavior or should I open up a bug report (I have dmesg output and debugging output from DRI ready on this, but it's a bith lengthy.) Please retest with latest drm-intel-nightly, if just merged a patch from Chris to prevent gpu hangs on i830/i845. If it still blows up please attach and error state captured from that kernel. Thanks, 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] drm/i915: De-magic the PSR AUX message
From: Ville Syrjälä ville.syrj...@linux.intel.com
Use pack_aux() to construct the PSR exit DPMS D0 AUX message,
and use the defines from dp_dp_helper.h to populate the message
contents.
Cc: Rodrigo Vivi rodrigo.v...@gmail.com
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_reg.h | 2 --
drivers/gpu/drm/i915/intel_dp.c | 16 +++-
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e4d7607..fb60493 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2453,9 +2453,7 @@ enum punit_power_well {
#define EDP_PSR_AUX_CTL(dev) (EDP_PSR_BASE(dev) + 0x10)
#define EDP_PSR_AUX_DATA1(dev) (EDP_PSR_BASE(dev) + 0x14)
-#define EDP_PSR_DPCD_COMMAND 0x8006
#define EDP_PSR_AUX_DATA2(dev) (EDP_PSR_BASE(dev) + 0x18)
-#define EDP_PSR_DPCD_NORMAL_OPERATION(124)
#define EDP_PSR_AUX_DATA3(dev) (EDP_PSR_BASE(dev) + 0x1c)
#define EDP_PSR_AUX_DATA4(dev) (EDP_PSR_BASE(dev) + 0x20)
#define EDP_PSR_AUX_DATA5(dev) (EDP_PSR_BASE(dev) + 0x24)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 81d7681..05bab25 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -225,7 +225,7 @@ intel_dp_mode_valid(struct drm_connector *connector,
}
static uint32_t
-pack_aux(uint8_t *src, int src_bytes)
+pack_aux(const uint8_t *src, int src_bytes)
{
int i;
uint32_t v = 0;
@@ -1741,8 +1741,14 @@ static void intel_edp_psr_enable_sink(struct intel_dp
*intel_dp)
struct drm_i915_private *dev_priv = dev-dev_private;
uint32_t aux_clock_divider;
int precharge = 0x3;
- int msg_size = 5; /* Header(4) + Message(1) */
bool only_standby = false;
+ static const uint8_t aux_msg[] = {
+ [0] = DP_AUX_NATIVE_WRITE 4,
+ [1] = DP_SET_POWER 8,
+ [2] = DP_SET_POWER 0xff,
+ [3] = 1 - 1,
+ [4] = DP_SET_POWER_D0,
+ };
aux_clock_divider = intel_dp-get_aux_clock_divider(intel_dp, 0);
@@ -1758,11 +1764,11 @@ static void intel_edp_psr_enable_sink(struct intel_dp
*intel_dp)
DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
/* Setup AUX registers */
- I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
- I915_WRITE(EDP_PSR_AUX_DATA2(dev), EDP_PSR_DPCD_NORMAL_OPERATION);
+ I915_WRITE(EDP_PSR_AUX_DATA1(dev), pack_aux(aux_msg[0], 4));
+ I915_WRITE(EDP_PSR_AUX_DATA2(dev), pack_aux(aux_msg[4], 1));
I915_WRITE(EDP_PSR_AUX_CTL(dev),
DP_AUX_CH_CTL_TIME_OUT_400us |
- (msg_size DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
+ (5 DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
(precharge DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
(aux_clock_divider DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
}
--
1.8.5.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PULL] drm-intel-fixes
Hi Dave, more fixes for 3.17, almost all Cc: stable material. BR, Jani. The following changes since commit 2ce7598c9a453e0acd0e07be7be3f5eb39608ebd: Linux 3.17-rc4 (2014-09-07 16:09:43 -0700) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/drm-intel-fixes-2014-09-10 for you to fetch changes up to 7a98948f3b536ca9a077e84966ddc0e9f53726df: drm/i915: Wait for vblank before enabling the TV encoder (2014-09-08 18:07:08 +0300) Chris Wilson (2): drm/i915: Prevent recursive deadlock on releasing a busy userptr drm/i915: Evict CS TLBs between batches Daniel Vetter (2): drm/i915: Fix EIO/wedged handling in gem fault handler drm/i915: Fix irq enable tracking in driver load Ville Syrjälä (1): drm/i915: Wait for vblank before enabling the TV encoder drivers/gpu/drm/i915/i915_dma.c | 9 +- drivers/gpu/drm/i915/i915_drv.h | 10 +- drivers/gpu/drm/i915/i915_gem.c | 11 +- drivers/gpu/drm/i915/i915_gem_userptr.c | 409 ++-- drivers/gpu/drm/i915/i915_reg.h | 12 +- drivers/gpu/drm/i915/intel_ringbuffer.c | 66 +++--- drivers/gpu/drm/i915/intel_tv.c | 4 + 7 files changed, 300 insertions(+), 221 deletions(-) -- 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 53/89] drm/i915/skl: Gen9 Forcewake
Damien Lespiau damien.lesp...@intel.com writes:
From: Zhe Wang zhe1.w...@intel.com
Implement common forcewake functions shared by Gen9 features.
Signed-off-by: Zhe Wang zhe1.w...@intel.com
Signed-off-by: Damien Lespiau damien.lesp...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 5 +-
drivers/gpu/drm/i915/i915_reg.h | 6 ++
drivers/gpu/drm/i915/intel_uncore.c | 175
+++-
3 files changed, 184 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9b0e398..84defa4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -535,6 +535,7 @@ struct intel_uncore {
unsigned fw_rendercount;
unsigned fw_mediacount;
+ unsigned fw_blittercount;
struct timer_list force_wake_timer;
};
@@ -2950,7 +2951,9 @@ int vlv_freq_opcode(struct drm_i915_private *dev_priv,
int val);
#define FORCEWAKE_RENDER (1 0)
#define FORCEWAKE_MEDIA (1 1)
-#define FORCEWAKE_ALL(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
+#define FORCEWAKE_BLITTER(1 2)
+#define FORCEWAKE_ALL(FORCEWAKE_RENDER | FORCEWAKE_MEDIA | \
+ FORCEWAKE_BLITTER)
#define I915_READ8(reg)
dev_priv-uncore.funcs.mmio_readb(dev_priv, (reg), true)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 414c2a5..417075d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5738,6 +5738,12 @@ enum punit_power_well {
#define VLV_GTLC_PW_MEDIA_STATUS_MASK (1 5)
#define VLV_GTLC_PW_RENDER_STATUS_MASK (1 7)
#define FORCEWAKE_MT0xa188 /*
multi-threaded */
+#define FORCEWAKE_MEDIA_GEN90xa270
+#define FORCEWAKE_RENDER_GEN9 0xa278
+#define FORCEWAKE_BLITTER_GEN9 0xa188
+#define FORCEWAKE_ACK_MEDIA_GEN90x0D88
+#define FORCEWAKE_ACK_RENDER_GEN9 0x0D84
+#define FORCEWAKE_ACK_BLITTER_GEN9 0x130044
#define FORCEWAKE_KERNEL 0x1
#define FORCEWAKE_USER 0x2
#define FORCEWAKE_MT_ACK0x130040
diff --git a/drivers/gpu/drm/i915/intel_uncore.c
b/drivers/gpu/drm/i915/intel_uncore.c
index 3b27fb0..7b7fc9e 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -299,6 +299,154 @@ static void vlv_force_wake_put(struct drm_i915_private
*dev_priv, int fw_engine)
spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags);
}
+static void __gen9_gt_force_wake_mt_reset(struct drm_i915_private *dev_priv)
+{
+ __raw_i915_write32(dev_priv, FORCEWAKE_RENDER_GEN9,
+ _MASKED_BIT_DISABLE(0x));
+
+ __raw_i915_write32(dev_priv, FORCEWAKE_MEDIA_GEN9,
+ _MASKED_BIT_DISABLE(0x));
+
+ __raw_i915_write32(dev_priv, FORCEWAKE_BLITTER_GEN9,
+ _MASKED_BIT_DISABLE(0x));
+}
+
+static void __gen9_force_wake_get(struct drm_i915_private *dev_priv,
+ int fw_engine)
+{
+ /* Check for Render Engine */
+ if (FORCEWAKE_RENDER fw_engine) {
+ if (wait_for_atomic((__raw_i915_read32(dev_priv,
+ FORCEWAKE_ACK_RENDER_GEN9)
+ FORCEWAKE_KERNEL) == 0,
+ FORCEWAKE_ACK_TIMEOUT_MS))
+ DRM_ERROR(Timed out: Render forcewake old ack to
clear.\n);
+
+ __raw_i915_write32(dev_priv, FORCEWAKE_RENDER_GEN9,
+_MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
+
+ if (wait_for_atomic((__raw_i915_read32(dev_priv,
+ FORCEWAKE_ACK_RENDER_GEN9)
+ FORCEWAKE_KERNEL),
+ FORCEWAKE_ACK_TIMEOUT_MS))
+ DRM_ERROR(Timed out: waiting for Render to ack.\n);
+ }
+
+ /* Check for Media Engine */
+ if (FORCEWAKE_MEDIA fw_engine) {
+ if (wait_for_atomic((__raw_i915_read32(dev_priv,
+ FORCEWAKE_ACK_MEDIA_GEN9)
+ FORCEWAKE_KERNEL) == 0,
+ FORCEWAKE_ACK_TIMEOUT_MS))
+ DRM_ERROR(Timed out: Media forcewake old ack to
clear.\n);
+
+ __raw_i915_write32(dev_priv, FORCEWAKE_MEDIA_GEN9,
+_MASKED_BIT_ENABLE(FORCEWAKE_KERNEL));
+
+ if (wait_for_atomic((__raw_i915_read32(dev_priv,
+ FORCEWAKE_ACK_MEDIA_GEN9)
+
Re: [Intel-gfx] [PATCH] drm/i915: De-magic the PSR AUX message
On Wed, 10 Sep 2014, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Use pack_aux() to construct the PSR exit DPMS D0 AUX message,
and use the defines from dp_dp_helper.h to populate the message
contents.
Cc: Rodrigo Vivi rodrigo.v...@gmail.com
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_reg.h | 2 --
drivers/gpu/drm/i915/intel_dp.c | 16 +++-
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e4d7607..fb60493 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2453,9 +2453,7 @@ enum punit_power_well {
#define EDP_PSR_AUX_CTL(dev) (EDP_PSR_BASE(dev) + 0x10)
#define EDP_PSR_AUX_DATA1(dev) (EDP_PSR_BASE(dev) +
0x14)
-#define EDP_PSR_DPCD_COMMAND 0x8006
#define EDP_PSR_AUX_DATA2(dev) (EDP_PSR_BASE(dev) +
0x18)
-#define EDP_PSR_DPCD_NORMAL_OPERATION (124)
#define EDP_PSR_AUX_DATA3(dev) (EDP_PSR_BASE(dev) +
0x1c)
#define EDP_PSR_AUX_DATA4(dev) (EDP_PSR_BASE(dev) +
0x20)
#define EDP_PSR_AUX_DATA5(dev) (EDP_PSR_BASE(dev) +
0x24)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 81d7681..05bab25 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -225,7 +225,7 @@ intel_dp_mode_valid(struct drm_connector *connector,
}
static uint32_t
-pack_aux(uint8_t *src, int src_bytes)
+pack_aux(const uint8_t *src, int src_bytes)
{
int i;
uint32_t v = 0;
@@ -1741,8 +1741,14 @@ static void intel_edp_psr_enable_sink(struct intel_dp
*intel_dp)
struct drm_i915_private *dev_priv = dev-dev_private;
uint32_t aux_clock_divider;
int precharge = 0x3;
- int msg_size = 5; /* Header(4) + Message(1) */
bool only_standby = false;
+ static const uint8_t aux_msg[] = {
+ [0] = DP_AUX_NATIVE_WRITE 4,
+ [1] = DP_SET_POWER 8,
+ [2] = DP_SET_POWER 0xff,
+ [3] = 1 - 1,
+ [4] = DP_SET_POWER_D0,
+ };
aux_clock_divider = intel_dp-get_aux_clock_divider(intel_dp, 0);
@@ -1758,11 +1764,11 @@ static void intel_edp_psr_enable_sink(struct intel_dp
*intel_dp)
DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
/* Setup AUX registers */
- I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
- I915_WRITE(EDP_PSR_AUX_DATA2(dev), EDP_PSR_DPCD_NORMAL_OPERATION);
+ I915_WRITE(EDP_PSR_AUX_DATA1(dev), pack_aux(aux_msg[0], 4));
+ I915_WRITE(EDP_PSR_AUX_DATA2(dev), pack_aux(aux_msg[4], 1));
I915_WRITE(EDP_PSR_AUX_CTL(dev),
DP_AUX_CH_CTL_TIME_OUT_400us |
-(msg_size DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
+(5 DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
s/5/ARRAY_SIZE(aux_msg)/ ?
(precharge DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
(aux_clock_divider DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
}
--
1.8.5.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
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] GPU-hang on i830
On Wed, Sep 10, 2014 at 03:25:31PM +0200, Thomas Richter wrote: Am 10.09.2014 14:22, schrieb Daniel Vetter: On Wed, Sep 10, 2014 at 02:17:30PM +0200, Thomas Richter wrote: Hi Daniel, hi Ville, just tried the new 3.17.0+rc4 kernel, though with old userspace (i.e. xserver-xorg-video-intel is *old*, libdrm is old, mesa is old). If I do, I get a GPU hung from xorg.conf. The same userspace works fine on 3.15.0 with patches from Ville. Is this expected behavior or should I open up a bug report (I have dmesg output and debugging output from DRI ready on this, but it's a bith lengthy.) Please retest with latest drm-intel-nightly, if just merged a patch from Chris to prevent gpu hangs on i830/i845. If it still blows up please attach and error state captured from that kernel. No, not merged from a patch. This is a clean checkout of master. drm-intel-nightly did not contain the watermark fixes the last time I checked. Error state is attached. I put Chris into CC. The w/a buffer is missing a good chunk of that dying batch. So seems related to the issue I had with corruption in the w/a buffer. But you already have the deadbeef patch, so it is not going to be so easy to fix. What would also be interesting is determining what triggered the regression, but that can be pretty painful on those machines. Does your machine support clflush? -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] GPU-hang on i830
On Wed, Sep 10, 2014 at 03:25:31PM +0200, Thomas Richter wrote: Am 10.09.2014 14:22, schrieb Daniel Vetter: On Wed, Sep 10, 2014 at 02:17:30PM +0200, Thomas Richter wrote: Hi Daniel, hi Ville, just tried the new 3.17.0+rc4 kernel, though with old userspace (i.e. xserver-xorg-video-intel is *old*, libdrm is old, mesa is old). If I do, I get a GPU hung from xorg.conf. The same userspace works fine on 3.15.0 with patches from Ville. Is this expected behavior or should I open up a bug report (I have dmesg output and debugging output from DRI ready on this, but it's a bith lengthy.) Please retest with latest drm-intel-nightly, if just merged a patch from Chris to prevent gpu hangs on i830/i845. If it still blows up please attach and error state captured from that kernel. No, not merged from a patch. This is a clean checkout of master. drm-intel-nightly did not contain the watermark fixes the last time I checked. Error state is attached. I put Chris into CC. The w/a batch is corrupted. 0x400-0x1000 somehow got turned into zeroes. Both are page boundaries, so I guess trying out Chris's TLB fix would be worth a shot. This is the commit you want: commit c4d69da167fa967749aeb70bc0e94a457e5d00c1 Author: Chris Wilson ch...@chris-wilson.co.uk Date: Mon Sep 8 14:25:41 2014 +0100 drm/i915: Evict CS TLBs between batches I just trawled through BSpec a bit and I see a clear note there that BLT TLBs are hosed on 830/845 and we need to flush after touching PTEs so that BLT will see the correct stuff. There's also a note that touching PGETBL_CTL enable bit would also flush all TLBs. So I wonder if just I915_WRITE(PGETBL_CTL, I915_READ(PGETBL_CTL)) after touching the PTEs would be enough to eliminate this problem? -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: De-magic the PSR AUX message
On Wed, Sep 10, 2014 at 04:45:09PM +0300, Jani Nikula wrote:
On Wed, 10 Sep 2014, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Use pack_aux() to construct the PSR exit DPMS D0 AUX message,
and use the defines from dp_dp_helper.h to populate the message
contents.
Cc: Rodrigo Vivi rodrigo.v...@gmail.com
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_reg.h | 2 --
drivers/gpu/drm/i915/intel_dp.c | 16 +++-
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index e4d7607..fb60493 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2453,9 +2453,7 @@ enum punit_power_well {
#define EDP_PSR_AUX_CTL(dev) (EDP_PSR_BASE(dev) +
0x10)
#define EDP_PSR_AUX_DATA1(dev) (EDP_PSR_BASE(dev) +
0x14)
-#define EDP_PSR_DPCD_COMMAND 0x8006
#define EDP_PSR_AUX_DATA2(dev) (EDP_PSR_BASE(dev) +
0x18)
-#define EDP_PSR_DPCD_NORMAL_OPERATION(124)
#define EDP_PSR_AUX_DATA3(dev) (EDP_PSR_BASE(dev) +
0x1c)
#define EDP_PSR_AUX_DATA4(dev) (EDP_PSR_BASE(dev) +
0x20)
#define EDP_PSR_AUX_DATA5(dev) (EDP_PSR_BASE(dev) +
0x24)
diff --git a/drivers/gpu/drm/i915/intel_dp.c
b/drivers/gpu/drm/i915/intel_dp.c
index 81d7681..05bab25 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -225,7 +225,7 @@ intel_dp_mode_valid(struct drm_connector *connector,
}
static uint32_t
-pack_aux(uint8_t *src, int src_bytes)
+pack_aux(const uint8_t *src, int src_bytes)
{
int i;
uint32_t v = 0;
@@ -1741,8 +1741,14 @@ static void intel_edp_psr_enable_sink(struct
intel_dp *intel_dp)
struct drm_i915_private *dev_priv = dev-dev_private;
uint32_t aux_clock_divider;
int precharge = 0x3;
- int msg_size = 5; /* Header(4) + Message(1) */
bool only_standby = false;
+ static const uint8_t aux_msg[] = {
+ [0] = DP_AUX_NATIVE_WRITE 4,
+ [1] = DP_SET_POWER 8,
+ [2] = DP_SET_POWER 0xff,
+ [3] = 1 - 1,
+ [4] = DP_SET_POWER_D0,
+ };
aux_clock_divider = intel_dp-get_aux_clock_divider(intel_dp, 0);
@@ -1758,11 +1764,11 @@ static void intel_edp_psr_enable_sink(struct
intel_dp *intel_dp)
DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
/* Setup AUX registers */
- I915_WRITE(EDP_PSR_AUX_DATA1(dev), EDP_PSR_DPCD_COMMAND);
- I915_WRITE(EDP_PSR_AUX_DATA2(dev), EDP_PSR_DPCD_NORMAL_OPERATION);
+ I915_WRITE(EDP_PSR_AUX_DATA1(dev), pack_aux(aux_msg[0], 4));
+ I915_WRITE(EDP_PSR_AUX_DATA2(dev), pack_aux(aux_msg[4], 1));
I915_WRITE(EDP_PSR_AUX_CTL(dev),
DP_AUX_CH_CTL_TIME_OUT_400us |
- (msg_size DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
+ (5 DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
s/5/ARRAY_SIZE(aux_msg)/ ?
Or just sizeof(). Either is fine by me.
(precharge DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
(aux_clock_divider DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
}
--
1.8.5.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] GPU-hang on i830
Am 10.09.2014 16:00, schrieb Ville Syrjälä: The w/a batch is corrupted. 0x400-0x1000 somehow got turned into zeroes. Both are page boundaries, so I guess trying out Chris's TLB fix would be worth a shot. According to patch, it is already applied, which is no miracle since I pulled yesterday. Should I revert it? So long, Thomas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PULL] topic/vblank-rework
Hmm, not quite an ack from my side for the pull in its current form. I
said if the two remaining issues i mentioned are addressed, then i'm
happy with it and can have my reviewed/acked-by. Looking at the code
they haven't been adressed.
However, this is easily fixable on top of the current patches:
1. A vblank_disable_timeout module parameter of zero should always
leave vblank irq's enabled and also override the drivers choice,
otherwise a user can't override the driver on a broken driver/gpu
combo, which is the only use case for having that module parameter.
Currenty the disable_immediately flag overrides the users override -
Ouch.
So in drm_vblank_put():
...
/* Last user schedules interrupt disable */
if (atomic_dec_and_test(vblank-refcount)) {
Insert zero - opt-out check
if (drm_vblank_offdelay == 0)
return;
Remaining code continues
if (dev-vblank_disable_immediate || drm_vblank_offdelay 0)
vblank_disable_fn((unsigned long)vblank);
else if (drm_vblank_offdelay 0)
mod_timer(vblank-disable_timer, jiffies +
((drm_vblank_offdelay * HZ)/1000));
...
2. For the drm: Have the vblank counter account for the time ...
patch, we must opt-out of that last timestamp/counter update/bump if
the driver doesn't support high-precision vblank timestamping,
otherwise the vblank count and timestamp will be inconsistent with
each other - or outright wrong in case of the timestamp. Rather
deliver a slightly outdated, but correct count+timestamp pair to
userspace, which is still useable for practical purposes, than a pair
that's outright wrong and will definitely confuse clients.
A simple fix in static void vblank_disable_and_save() would be to
replace the new...
if (!vblank-enabled) {
... check by ...
if (!vblank-enabled
) {
On Wed, Sep 10, 2014 at 2:05 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
Hi Dave,
So here's the final bits of Ville's vblank rework with a bit of cleanup
from Mario on top.
The neat thing this finally allows is to immediately disable the vblank
interrupt on the last drm_vblank_put if the hardware has perfectly
accurate vblank counter and timestamp readout support. On i915 that
required piles of small adjustements from Ville since depending upon the
platform and port the vblank happens at different scanout lines.
Of course this is fully opt-in and per-device (we need that since gen2
doesn't have a hw vblank counter).
Mario reviewed the entire pile too and after some initial hesitation
(about drivers without accurate timestampt support) acked it.
Cheers, Daniel
The following changes since commit 21d70354bba9965a098382fc4d7fb17e138111f3:
drm: move drm_stub.c to drm_drv.c (2014-08-06 19:10:44 +1000)
are available in the git repository at:
git://anongit.freedesktop.org/drm-intel tags/topic/vblank-rework-2014-09-10
for you to fetch changes up to 2368ffb18b1d2b04eb80478d225676caa7a3c4c8:
drm: Use vblank_disable_and_save in drm_vblank_cleanup() (2014-09-10
09:41:29 +0200)
Mario Kleiner (2):
drm: Remove drm_vblank_cleanup from drm_vblank_init error path.
drm: Use vblank_disable_and_save in drm_vblank_cleanup()
Ville Syrjälä (16):
drm: Always reject drm_vblank_get() after drm_vblank_off()
drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()
drm: Don't clear vblank timestamps when vblank interrupt is disabled
drm: Move drm_update_vblank_count()
drm: Have the vblank counter account for the time between vblank irq
disable and drm_vblank_off()
drm: Avoid random vblank counter jumps if the hardware counter has been
reset
drm: Reduce the amount of dev-vblank[crtc] in the code
drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock
drm: Fix race between drm_vblank_off() and drm_queue_vblank_event()
drm: Disable vblank interrupt immediately when drm_vblank_offdelay0
drm: Add dev-vblank_disable_immediate flag
drm/i915: Opt out of vblank disable timer on gen2
drm: Kick start vblank interrupts at drm_vblank_on()
drm/i915: Update scanline_offset only for active crtcs
drm: Fix confusing debug message in drm_update_vblank_count()
drm: Store the vblank timestamp when adjusting the counter during
disable
Documentation/DocBook/drm.tmpl | 7 +
drivers/gpu/drm/drm_drv.c| 4 +-
drivers/gpu/drm/drm_irq.c| 345
++-
drivers/gpu/drm/i915/i915_irq.c | 8 +
drivers/gpu/drm/i915/intel_display.c | 17 +-
include/drm/drmP.h | 12 +-
6 files changed, 256 insertions(+), 137 deletions(-)
--
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
Re: [Intel-gfx] [PULL] topic/vblank-rework
e-mail snafu, sent it too early by accident, and from a gmail web
interface which i'm apparently incapable of using properly...
The second fix should look like this:
A simple fix in static void vblank_disable_and_save() would be to
replace the new...
if (!vblank-enabled) {
... check by ...
if (!vblank-enabled
drm_get_last_vbltimestamp(dev, crtc, tvblank, 0)) {
... We need to make sure timestamp queries work and are actually
locked to the vblank, otherwise we can't do that last update there in
vblank_disable_and_save().
With these two fixes or similar applied i'd be happy, otherwise it
will inflict pain and real bugs on real users.
thanks,
-mario
On Wed, Sep 10, 2014 at 4:19 PM, Mario Kleiner
mario.kleiner...@gmail.com wrote:
Hmm, not quite an ack from my side for the pull in its current form. I
said if the two remaining issues i mentioned are addressed, then i'm
happy with it and can have my reviewed/acked-by. Looking at the code
they haven't been adressed.
However, this is easily fixable on top of the current patches:
1. A vblank_disable_timeout module parameter of zero should always
leave vblank irq's enabled and also override the drivers choice,
otherwise a user can't override the driver on a broken driver/gpu
combo, which is the only use case for having that module parameter.
Currenty the disable_immediately flag overrides the users override -
Ouch.
So in drm_vblank_put():
...
/* Last user schedules interrupt disable */
if (atomic_dec_and_test(vblank-refcount)) {
Insert zero - opt-out check
if (drm_vblank_offdelay == 0)
return;
Remaining code continues
if (dev-vblank_disable_immediate || drm_vblank_offdelay 0)
vblank_disable_fn((unsigned long)vblank);
else if (drm_vblank_offdelay 0)
mod_timer(vblank-disable_timer, jiffies +
((drm_vblank_offdelay * HZ)/1000));
...
2. For the drm: Have the vblank counter account for the time ...
patch, we must opt-out of that last timestamp/counter update/bump if
the driver doesn't support high-precision vblank timestamping,
otherwise the vblank count and timestamp will be inconsistent with
each other - or outright wrong in case of the timestamp. Rather
deliver a slightly outdated, but correct count+timestamp pair to
userspace, which is still useable for practical purposes, than a pair
that's outright wrong and will definitely confuse clients.
A simple fix in static void vblank_disable_and_save() would be to
replace the new...
if (!vblank-enabled) {
... check by ...
if (!vblank-enabled
) {
On Wed, Sep 10, 2014 at 2:05 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
Hi Dave,
So here's the final bits of Ville's vblank rework with a bit of cleanup
from Mario on top.
The neat thing this finally allows is to immediately disable the vblank
interrupt on the last drm_vblank_put if the hardware has perfectly
accurate vblank counter and timestamp readout support. On i915 that
required piles of small adjustements from Ville since depending upon the
platform and port the vblank happens at different scanout lines.
Of course this is fully opt-in and per-device (we need that since gen2
doesn't have a hw vblank counter).
Mario reviewed the entire pile too and after some initial hesitation
(about drivers without accurate timestampt support) acked it.
Cheers, Daniel
The following changes since commit 21d70354bba9965a098382fc4d7fb17e138111f3:
drm: move drm_stub.c to drm_drv.c (2014-08-06 19:10:44 +1000)
are available in the git repository at:
git://anongit.freedesktop.org/drm-intel tags/topic/vblank-rework-2014-09-10
for you to fetch changes up to 2368ffb18b1d2b04eb80478d225676caa7a3c4c8:
drm: Use vblank_disable_and_save in drm_vblank_cleanup() (2014-09-10
09:41:29 +0200)
Mario Kleiner (2):
drm: Remove drm_vblank_cleanup from drm_vblank_init error path.
drm: Use vblank_disable_and_save in drm_vblank_cleanup()
Ville Syrjälä (16):
drm: Always reject drm_vblank_get() after drm_vblank_off()
drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()
drm: Don't clear vblank timestamps when vblank interrupt is disabled
drm: Move drm_update_vblank_count()
drm: Have the vblank counter account for the time between vblank irq
disable and drm_vblank_off()
drm: Avoid random vblank counter jumps if the hardware counter has
been reset
drm: Reduce the amount of dev-vblank[crtc] in the code
drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock
drm: Fix race between drm_vblank_off() and drm_queue_vblank_event()
drm: Disable vblank interrupt immediately when drm_vblank_offdelay0
drm: Add dev-vblank_disable_immediate flag
drm/i915: Opt out of vblank disable timer on gen2
drm: Kick start vblank interrupts at drm_vblank_on()
drm/i915: Update scanline_offset
[Intel-gfx] [PATCH v2] drm/i915: pin sprite fb only if it changed
From: Gustavo Padovan gustavo.pado...@collabora.co.uk
Optimize code avoiding helding dev mutex if old fb and current fb
are the same.
v2: take Ville's comments
- move comment along with the pin_and_fence call
- check for error before calling i915_gem_track_fb
- move old_obj != obj to an upper if condition
Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
---
drivers/gpu/drm/i915/intel_sprite.c | 34 +++---
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index a4306cf..90bb45f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1038,21 +1038,24 @@ intel_commit_sprite_plane(struct drm_plane *plane,
primary_enabled = !drm_rect_equals(dst, clip) ||
colorkey_enabled(intel_plane);
WARN_ON(!primary_enabled !state-visible intel_crtc-active);
- mutex_lock(dev-struct_mutex);
-
- /* Note that this will apply the VT-d workaround for scanouts,
-* which is more restrictive than required for sprites. (The
-* primary plane requires 256KiB alignment with 64 PTE padding,
-* the sprite planes only require 128KiB alignment and 32 PTE padding.
-*/
- ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
- i915_gem_track_fb(old_obj, obj,
- INTEL_FRONTBUFFER_SPRITE(pipe));
- mutex_unlock(dev-struct_mutex);
+ if (old_obj != obj) {
+ mutex_lock(dev-struct_mutex);
- if (ret)
- return ret;
+ /* Note that this will apply the VT-d workaround for scanouts,
+* which is more restrictive than required for sprites. (The
+* primary plane requires 256KiB alignment with 64 PTE padding,
+* the sprite planes only require 128KiB alignment and 32 PTE
+* padding.
+*/
+ ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+ if (ret == 0)
+ i915_gem_track_fb(old_obj, obj,
+ INTEL_FRONTBUFFER_SPRITE(pipe));
+ mutex_unlock(dev-struct_mutex);
+ if (ret)
+ return ret;
+ }
intel_plane-crtc_x = state-orig_dst.x1;
intel_plane-crtc_y = state-orig_dst.y1;
@@ -1099,14 +1102,15 @@ intel_commit_sprite_plane(struct drm_plane *plane,
}
/* Unpin old obj after new one is active to avoid ugliness */
- if (old_obj) {
+ if (old_obj old_obj != obj) {
+
/*
* It's fairly common to simply update the position of
* an existing object. In that case, we don't need to
* wait for vblank to avoid ugliness, we only need to
* do the pin ref bookkeeping.
*/
- if (old_obj != obj intel_crtc-active)
+ if (intel_crtc-active)
intel_wait_for_vblank(dev, intel_crtc-pipe);
mutex_lock(dev-struct_mutex);
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 2/2] drm/i915: Merge of visible and !visible paths for primary planes
From: Gustavo Padovan gustavo.pado...@collabora.co.uk
Fold intel_pipe_set_base() in the update primary plane path merging
pieces of code that are common to both paths.
Basically the the pin/unpin procedures are the same for both paths
and some checks can also be shared (some of the were moved to the
check() stage)
v2: take Ville's comments:
- remove unecessary plane check
- move mutex lock to inside the conditional
- make the pin fail message a debug one
- add a fixme for the fastboot hack
- call intel_frontbuffer_flip() after FBC update
Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
---
drivers/gpu/drm/i915/intel_display.c | 93
1 file changed, 62 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index b78f00a..1a001bf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11824,12 +11824,23 @@ intel_check_primary_plane(struct drm_plane *plane,
struct drm_rect *dest = state-dst;
struct drm_rect *src = state-src;
const struct drm_rect *clip = state-clip;
+ int ret;
- return drm_plane_helper_check_update(plane, crtc, fb,
+ ret = drm_plane_helper_check_update(plane, crtc, fb,
src, dest, clip,
DRM_PLANE_HELPER_NO_SCALING,
DRM_PLANE_HELPER_NO_SCALING,
false, true, state-visible);
+ if (ret)
+ return ret;
+
+ /* no fb bound */
+ if (state-visible !fb) {
+ DRM_ERROR(No FB bound\n);
+ return -EINVAL;
+ }
+
+ return 0;
}
static int
@@ -11841,14 +11852,34 @@ intel_commit_primary_plane(struct drm_plane *plane,
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ enum pipe pipe = intel_crtc-pipe;
+ struct drm_framebuffer *old_fb = plane-fb;
struct drm_i915_gem_object *obj = intel_fb_obj(fb);
struct drm_i915_gem_object *old_obj = intel_fb_obj(plane-fb);
struct intel_plane *intel_plane = to_intel_plane(plane);
struct drm_rect *src = state-src;
- int ret;
+ int ret = 0;
intel_crtc_wait_for_pending_flips(crtc);
+ if (intel_crtc_has_pending_flip(crtc)) {
+ DRM_ERROR(pipe is still busy with an old pageflip\n);
+ return -EBUSY;
+ }
+
+ if (plane-fb != fb) {
+ mutex_lock(dev-struct_mutex);
+ ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+ if (ret == 0)
+ i915_gem_track_fb(old_obj, obj,
+ INTEL_FRONTBUFFER_PRIMARY(pipe));
+ mutex_unlock(dev-struct_mutex);
+ }
+ if (ret != 0) {
+ DRM_DEBUG_KMS(pin fence failed\n);
+ return ret;
+ }
+
/*
* If clipping results in a non-visible primary plane, we'll disable
* the primary plane. Note that this is a bit different than what
@@ -11856,33 +11887,9 @@ intel_commit_primary_plane(struct drm_plane *plane,
* because plane-fb still gets set and pinned.
*/
if (!state-visible) {
- mutex_lock(dev-struct_mutex);
-
- /*
-* Try to pin the new fb first so that we can bail out if we
-* fail.
-*/
- if (plane-fb != fb) {
- ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
- if (ret) {
- mutex_unlock(dev-struct_mutex);
- return ret;
- }
- }
-
- i915_gem_track_fb(old_obj, obj,
- INTEL_FRONTBUFFER_PRIMARY(intel_crtc-pipe));
-
if (intel_crtc-primary_enabled)
intel_disable_primary_hw_plane(plane, crtc);
-
- if (plane-fb != fb)
- if (plane-fb)
- intel_unpin_fb_obj(old_obj);
-
- mutex_unlock(dev-struct_mutex);
-
} else {
if (intel_crtc intel_crtc-active
intel_crtc-primary_enabled) {
@@ -11902,12 +11909,36 @@ intel_commit_primary_plane(struct drm_plane *plane,
intel_disable_fbc(dev);
}
}
- ret = intel_pipe_set_base(crtc, src-x1, src-y1, fb);
- if (ret)
- return ret;
- if (!intel_crtc-primary_enabled)
- intel_enable_primary_hw_plane(plane, crtc);
+ /* FIXME: kill
[Intel-gfx] [PATCH v2 1/2] drm/i915: create intel_update_pipe_size()
From: Gustavo Padovan gustavo.pado...@collabora.co.uk
Factor out a piece of code from intel_pipe_set_base() that updates
the pipe size and adjust fitter.
This will help refactor the update primary plane path.
v2: use struct intel_crtc as argument to intel_update_pipe_size()
v3: use 'crtc' as argument name
Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
---
drivers/gpu/drm/i915/intel_display.c | 70
1 file changed, 40 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 2ccf7c0..b78f00a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2779,6 +2779,45 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc
*crtc)
return pending;
}
+static void intel_update_pipe_size(struct intel_crtc *crtc)
+{
+ struct drm_device *dev = crtc-base.dev;
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ const struct drm_display_mode *adjusted_mode;
+
+ if (!i915.fastboot)
+ return;
+
+ /*
+* Update pipe size and adjust fitter if needed: the reason for this is
+* that in compute_mode_changes we check the native mode (not the pfit
+* mode) to see if we can flip rather than do a full mode set. In the
+* fastboot case, we'll flip, but if we don't update the pipesrc and
+* pfit state, we'll end up with a big fb scanned out into the wrong
+* sized surface.
+*
+* To fix this properly, we need to hoist the checks up into
+* compute_mode_changes (or above), check the actual pfit state and
+* whether the platform allows pfit disable with pipe active, and only
+* then update the pipesrc and pfit state, even on the flip path.
+*/
+
+ adjusted_mode = crtc-config.adjusted_mode;
+
+ I915_WRITE(PIPESRC(crtc-pipe),
+ ((adjusted_mode-crtc_hdisplay - 1) 16) |
+ (adjusted_mode-crtc_vdisplay - 1));
+ if (!crtc-config.pch_pfit.enabled
+ (intel_pipe_has_type(crtc-base, INTEL_OUTPUT_LVDS) ||
+intel_pipe_has_type(crtc-base, INTEL_OUTPUT_EDP))) {
+ I915_WRITE(PF_CTL(crtc-pipe), 0);
+ I915_WRITE(PF_WIN_POS(crtc-pipe), 0);
+ I915_WRITE(PF_WIN_SZ(crtc-pipe), 0);
+ }
+ crtc-config.pipe_src_w = adjusted_mode-crtc_hdisplay;
+ crtc-config.pipe_src_h = adjusted_mode-crtc_vdisplay;
+}
+
static int
intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
struct drm_framebuffer *fb)
@@ -2821,36 +2860,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
return ret;
}
- /*
-* Update pipe size and adjust fitter if needed: the reason for this is
-* that in compute_mode_changes we check the native mode (not the pfit
-* mode) to see if we can flip rather than do a full mode set. In the
-* fastboot case, we'll flip, but if we don't update the pipesrc and
-* pfit state, we'll end up with a big fb scanned out into the wrong
-* sized surface.
-*
-* To fix this properly, we need to hoist the checks up into
-* compute_mode_changes (or above), check the actual pfit state and
-* whether the platform allows pfit disable with pipe active, and only
-* then update the pipesrc and pfit state, even on the flip path.
-*/
- if (i915.fastboot) {
- const struct drm_display_mode *adjusted_mode =
- intel_crtc-config.adjusted_mode;
-
- I915_WRITE(PIPESRC(intel_crtc-pipe),
- ((adjusted_mode-crtc_hdisplay - 1) 16) |
- (adjusted_mode-crtc_vdisplay - 1));
- if (!intel_crtc-config.pch_pfit.enabled
- (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
-intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
- I915_WRITE(PF_CTL(intel_crtc-pipe), 0);
- I915_WRITE(PF_WIN_POS(intel_crtc-pipe), 0);
- I915_WRITE(PF_WIN_SZ(intel_crtc-pipe), 0);
- }
- intel_crtc-config.pipe_src_w = adjusted_mode-crtc_hdisplay;
- intel_crtc-config.pipe_src_h = adjusted_mode-crtc_vdisplay;
- }
+ intel_update_pipe_size(intel_crtc);
dev_priv-display.update_primary_plane(crtc, fb, x, y);
--
1.9.3
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/16] drm/i915: unify legacy S3 suspend and S4 freeze handlers
i915_suspend() is called from the DRM legacy S3 suspend/S4 freeze paths
and the switcheroo suspend path. For switcheroo we only ever need to
perform a full suspend (PM_EVENT_SUSPEND) and for the DRM legacy path
we can handle the S4 freeze (PM_EVENT_FREEZE) the same way as S3
suspend. The only difference atm between suspend and freeze is that
during freeze we don't disable the PCI device, but there is no reason
why we can't do so. So unify the two cases to reduce complexity.
Note that for the DRM legacy case the thaw event is not handled, so
we disable the display before creating the hibernation image and it
won't get re-enabled until reboot. We could fix this leaving the
display enabled for the image creation/writing (if we care enough
about UMS), but this can be done as a follow-up.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 7fa34bd..bf4ff5e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -648,11 +648,9 @@ int i915_suspend(struct drm_device *dev, pm_message_t
state)
if (error)
return error;
- if (state.event == PM_EVENT_SUSPEND) {
- /* Shut down the device */
- pci_disable_device(dev-pdev);
- pci_set_power_state(dev-pdev, PCI_D3hot);
- }
+ /* Shut down the device */
+ pci_disable_device(dev-pdev);
+ pci_set_power_state(dev-pdev, PCI_D3hot);
return 0;
}
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 00/16] fix VLV S4 suspend/resume, unify S3/S4 handlers
The first part of the patchset (1-6) fixes an S4 bug on VLV introduced recently. The rest unifies the various S3/S4 handlers, which are in practice the same. The only real difference - besides some unused code - is that during S3 suspend we disable the PCI device whereas across an S4 freeze/thaw we keep it enabled. Given that we disable everything else anyway, we can just as well disable the PCI device there too. Doing so allows us to handle S3 suspend/resume and S4 freeze/thaw/restore/ power-off the same way, simplifying/clarifying things quite a bit. I smoke tested this on BDW, HSW, VLV (BYT-M), IVB, GEN45. Imre Deak (16): drm/i915: vlv: fix gunit HW state corruption during S4 suspend drm/i915: remove dead code from legacy suspend handler drm/i915: factor out i915_drm_suspend_late drm/i915: unify legacy S3 suspend and S4 freeze handlers drm/i915: propagate error from legacy resume handler drm/i915: vlv: fix switcheroo/legacy suspend/resume drm/i915: fix S4 suspend while switcheroo state is off drm/i915: remove unused restore_gtt_mappings optimization during suspend drm/i915: check for GT faults during S3 resume and S4 restore too drm/i915: enable output polling during S4 thaw drm/i915: disable/re-enable PCI device around S4 freeze/thaw drm/i915: unify S3 and S4 suspend/resume handlers drm/i915: sanitize suspend/resume helper function names drm/i915: add poweroff_late handler drm/i915: unify switcheroo and legacy suspend/resume handlers drm/i915: add comments on what stage a given PM handler is called drivers/gpu/drm/i915/i915_dma.c | 4 +- drivers/gpu/drm/i915/i915_drv.c | 185 +++- drivers/gpu/drm/i915/i915_drv.h | 4 +- 3 files changed, 74 insertions(+), 119 deletions(-) -- 1.8.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 01/16] drm/i915: vlv: fix gunit HW state corruption during S4 suspend
During S4 freeze we don't call intel_suspend_complete(), which would
save the gunit HW state, but during S4 thaw/restore events we call
intel_resume_prepare() which restores it, thus ending up in a corrupted
HW state.
Fix this by calling intel_suspend_complete() from the corresponding
freeze_late event handler.
The issue was introduced in
commit 016970beb05da6285c2f3ed2bee1c676cb75972e
Author: Sagar Kamble sagar.a.kam...@intel.com
Date: Wed Aug 13 23:07:06 2014 +0530
CC: Sagar Kamble sagar.a.kam...@intel.com
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b8bd008..2365875 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -987,6 +987,15 @@ static int i915_pm_freeze(struct device *dev)
return i915_drm_freeze(drm_dev);
}
+static int i915_pm_freeze_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;
+
+ return intel_suspend_complete(dev_priv);
+}
+
static int i915_pm_thaw_early(struct device *dev)
{
struct pci_dev *pdev = to_pci_dev(dev);
@@ -1571,6 +1580,7 @@ static const struct dev_pm_ops i915_pm_ops = {
.resume_early = i915_pm_resume_early,
.resume = i915_pm_resume,
.freeze = i915_pm_freeze,
+ .freeze_late = i915_pm_freeze_late,
.thaw_early = i915_pm_thaw_early,
.thaw = i915_pm_thaw,
.poweroff = i915_pm_poweroff,
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 07/16] drm/i915: fix S4 suspend while switcheroo state is off
If the device is suspended already through the switcheroo interface we shouldn't suspend it again. We have the corresponding check for S3 suspend already, add it for S4 freeze and poweroff too. Note that there is still the problem that the resume path should also check for the switcheroo-off state and keep the device disabled or in case of S4 disable it. That is a separate issue, which is not addressed in this patchset. Signed-off-by: Imre Deak imre.d...@intel.com --- drivers/gpu/drm/i915/i915_drv.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index ca74d6d..a3addc2 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -994,6 +994,9 @@ static int i915_pm_freeze(struct device *dev) return -ENODEV; } + if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF) + return 0; + return i915_drm_freeze(drm_dev); } @@ -1003,6 +1006,9 @@ static int i915_pm_freeze_late(struct device *dev) struct drm_device *drm_dev = pci_get_drvdata(pdev); struct drm_i915_private *dev_priv = drm_dev-dev_private; + if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF) + return 0; + return intel_suspend_complete(dev_priv); } @@ -1027,6 +1033,9 @@ static int i915_pm_poweroff(struct device *dev) struct pci_dev *pdev = to_pci_dev(dev); struct drm_device *drm_dev = pci_get_drvdata(pdev); + if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF) + return 0; + return i915_drm_freeze(drm_dev); } -- 1.8.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 09/16] drm/i915: check for GT faults during S3 resume and S4 restore too
Checking for GT faults is not specific in any way to S4 thaw, so do it
also during S3 resume and S4 restore. This allows us to unify the
handlers for these events in an upcoming patch.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5e25283..d0fee60 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -672,6 +672,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
struct drm_i915_private *dev_priv = dev-dev_private;
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+ i915_check_and_clear_faults(dev);
+
mutex_lock(dev-struct_mutex);
i915_gem_restore_gtt_mappings(dev);
mutex_unlock(dev-struct_mutex);
@@ -736,9 +738,6 @@ static int __i915_drm_thaw(struct drm_device *dev)
static int i915_drm_thaw(struct drm_device *dev)
{
- if (drm_core_check_feature(dev, DRIVER_MODESET))
- i915_check_and_clear_faults(dev);
-
return __i915_drm_thaw(dev);
}
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 02/16] drm/i915: remove dead code from legacy suspend handler
The legacy DRM suspend logic (effective in UMS) doesn't handle any S4 thaw events so we don't need to care about it either. Only S3 suspend and S4 freeze events are handled. Leave an assert behind to be sure. Signed-off-by: Imre Deak imre.d...@intel.com --- drivers/gpu/drm/i915/i915_drv.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 2365875..f36e2d2 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -618,9 +618,9 @@ int i915_suspend(struct drm_device *dev, pm_message_t state) return -ENODEV; } - if (state.event == PM_EVENT_PRETHAW) - return 0; - + if (WARN_ON_ONCE(state.event != PM_EVENT_SUSPEND +state.event != PM_EVENT_FREEZE)) + return -EINVAL; if (dev-switch_power_state == DRM_SWITCH_POWER_OFF) return 0; -- 1.8.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 16/16] drm/i915: add comments on what stage a given PM handler is called
This will hopefully make it easier to navigate the code without the need
to consult the full PM documentation.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 55b04e6..44c9317 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1506,10 +1506,21 @@ static int intel_resume_prepare(struct drm_i915_private
*dev_priv,
}
static const struct dev_pm_ops i915_pm_ops = {
+ /* S0ix, S3 event handlers */
.suspend = i915_pm_suspend,
.suspend_late = i915_pm_suspend_late,
.resume_early = i915_pm_resume_early,
.resume = i915_pm_resume,
+
+ /*
+* S4 event handlers
+* @freeze, @freeze_late: called before creating hibernation image
+* @thaw, @thaw_early : called after creating hibernation image,
+*before writing it
+* @poweroff, @poweroff_late: called after writing hibernation image,
+*before rebooting
+* @restore, @restore_early : called after rebooting
+*/
.freeze = i915_pm_suspend,
.freeze_late = i915_pm_suspend_late,
.thaw_early = i915_pm_resume_early,
@@ -1518,6 +1529,8 @@ static const struct dev_pm_ops i915_pm_ops = {
.poweroff_late = i915_pm_suspend_late,
.restore_early = i915_pm_resume_early,
.restore = i915_pm_resume,
+
+ /* D0-D3 (runtime PM) event handlers */
.runtime_suspend = intel_runtime_suspend,
.runtime_resume = intel_runtime_resume,
};
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/16] drm/i915: propagate error from legacy resume handler
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index bf4ff5e..89b63fc 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -789,10 +789,13 @@ int i915_resume(struct drm_device *dev)
static int i915_resume_legacy(struct drm_device *dev)
{
- i915_resume_early(dev);
- i915_resume(dev);
+ int ret;
- return 0;
+ ret = i915_resume_early(dev);
+ if (ret)
+ return ret;
+
+ return i915_resume(dev);
}
/**
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 03/16] drm/i915: factor out i915_drm_suspend_late
This is needed by an upcoming patch fixing the switcheroo/legacy suspend
paths.
No functional change.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 32
1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f36e2d2..7fa34bd 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -608,6 +608,25 @@ static int i915_drm_freeze(struct drm_device *dev)
return 0;
}
+static int i915_drm_suspend_late(struct drm_device *drm_dev)
+{
+ struct drm_i915_private *dev_priv = drm_dev-dev_private;
+ int ret;
+
+ ret = intel_suspend_complete(dev_priv);
+
+ if (ret) {
+ DRM_ERROR(Suspend complete failed: %d\n, ret);
+
+ return ret;
+ }
+
+ pci_disable_device(drm_dev-pdev);
+ pci_set_power_state(drm_dev-pdev, PCI_D3hot);
+
+ return 0;
+}
+
int i915_suspend(struct drm_device *dev, pm_message_t state)
{
int error;
@@ -931,8 +950,6 @@ 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
@@ -946,16 +963,7 @@ static int i915_pm_suspend_late(struct device *dev)
if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
- ret = intel_suspend_complete(dev_priv);
-
- if (ret)
- DRM_ERROR(Suspend complete failed: %d\n, ret);
- else {
- pci_disable_device(pdev);
- pci_set_power_state(pdev, PCI_D3hot);
- }
-
- return ret;
+ return i915_drm_suspend_late(drm_dev);
}
static int i915_pm_resume_early(struct device *dev)
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PULL] topic/vblank-rework
On Wed, Sep 10, 2014 at 4:19 PM, Mario Kleiner
mario.kleiner...@gmail.com wrote:
Hmm, not quite an ack from my side for the pull in its current form. I
said if the two remaining issues i mentioned are addressed, then i'm
happy with it and can have my reviewed/acked-by. Looking at the code
they haven't been adressed.
Sorry about the confusion, I've somehow thought that you've retracted
those comments in Message-ID:
caesyxygk4foqhky1wcerak_hybex2ogpftjyhu_zfhlbx46...@mail.gmail.com
But I've missed that that was about just one of the issues.
However, this is easily fixable on top of the current patches:
1. A vblank_disable_timeout module parameter of zero should always
leave vblank irq's enabled and also override the drivers choice,
otherwise a user can't override the driver on a broken driver/gpu
combo, which is the only use case for having that module parameter.
Currenty the disable_immediately flag overrides the users override -
Ouch.
So in drm_vblank_put():
...
/* Last user schedules interrupt disable */
if (atomic_dec_and_test(vblank-refcount)) {
Insert zero - opt-out check
if (drm_vblank_offdelay == 0)
return;
Remaining code continues
if (dev-vblank_disable_immediate || drm_vblank_offdelay 0)
vblank_disable_fn((unsigned long)vblank);
else if (drm_vblank_offdelay 0)
mod_timer(vblank-disable_timer, jiffies +
((drm_vblank_offdelay * HZ)/1000));
Yeah, I guess that makes sense. I'm not really a fan of giving users
too powerful module options to hack around driver bugs since often
that means they'll never report the bug :( But we have the support now
to mark certain module options as debug-only and they'll taint the
kernel if set, so this is fixable.
I'll follow up with the patch you've suggested.
...
2. For the drm: Have the vblank counter account for the time ...
patch, we must opt-out of that last timestamp/counter update/bump if
the driver doesn't support high-precision vblank timestamping,
otherwise the vblank count and timestamp will be inconsistent with
each other - or outright wrong in case of the timestamp. Rather
deliver a slightly outdated, but correct count+timestamp pair to
userspace, which is still useable for practical purposes, than a pair
that's outright wrong and will definitely confuse clients.
A simple fix in static void vblank_disable_and_save() would be to
replace the new...
if (!vblank-enabled) {
... check by ...
if (!vblank-enabled
) {
Yeah, makes sense (well the follow-up one ofc). I'll do a patch which
adds this and adds a comment. Aside I think it would be useful to add
a #define for the 0 return value, since the magic checks all over are
imo fairly hard to understand.
I'll also float a patch for rfc about that.
Thanks for your comments and again my apologies for missing that
there's still outstanding work left to do on this.
Cheers, Daniel
On Wed, Sep 10, 2014 at 2:05 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
Hi Dave,
So here's the final bits of Ville's vblank rework with a bit of cleanup
from Mario on top.
The neat thing this finally allows is to immediately disable the vblank
interrupt on the last drm_vblank_put if the hardware has perfectly
accurate vblank counter and timestamp readout support. On i915 that
required piles of small adjustements from Ville since depending upon the
platform and port the vblank happens at different scanout lines.
Of course this is fully opt-in and per-device (we need that since gen2
doesn't have a hw vblank counter).
Mario reviewed the entire pile too and after some initial hesitation
(about drivers without accurate timestampt support) acked it.
Cheers, Daniel
The following changes since commit 21d70354bba9965a098382fc4d7fb17e138111f3:
drm: move drm_stub.c to drm_drv.c (2014-08-06 19:10:44 +1000)
are available in the git repository at:
git://anongit.freedesktop.org/drm-intel tags/topic/vblank-rework-2014-09-10
for you to fetch changes up to 2368ffb18b1d2b04eb80478d225676caa7a3c4c8:
drm: Use vblank_disable_and_save in drm_vblank_cleanup() (2014-09-10
09:41:29 +0200)
Mario Kleiner (2):
drm: Remove drm_vblank_cleanup from drm_vblank_init error path.
drm: Use vblank_disable_and_save in drm_vblank_cleanup()
Ville Syrjälä (16):
drm: Always reject drm_vblank_get() after drm_vblank_off()
drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()
drm: Don't clear vblank timestamps when vblank interrupt is disabled
drm: Move drm_update_vblank_count()
drm: Have the vblank counter account for the time between vblank irq
disable and drm_vblank_off()
drm: Avoid random vblank counter jumps if the hardware counter has
been reset
drm: Reduce the amount of dev-vblank[crtc] in the code
drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock
drm: Fix race
[Intel-gfx] [PATCH 11/16] drm/i915: disable/re-enable PCI device around S4 freeze/thaw
We already disable everything during S4 freeze, except the PCI device
itself. There is no reason why we couldn't disable that too and doing
so allows us to unify these handlers in the next patch with the
corresponding S3 suspend/resume handlers.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ea8dda7..0ff5e92 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -991,12 +991,11 @@ static int i915_pm_freeze_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;
if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
- return intel_suspend_complete(dev_priv);
+ return i915_drm_suspend_late(drm_dev);
}
static int i915_pm_thaw_early(struct device *dev)
@@ -1004,7 +1003,7 @@ static int i915_pm_thaw_early(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
struct drm_device *drm_dev = pci_get_drvdata(pdev);
- return i915_drm_thaw_early(drm_dev);
+ return i915_resume_early(drm_dev);
}
static int i915_pm_thaw(struct device *dev)
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 10/16] drm/i915: enable output polling during S4 thaw
To avoid processing hotplug events we disable connector polling for the
duration of S3 suspend. We also disable it for S4 freeze, and keep it
disabled after S4 thaw. This won't prevent though hotplug processing,
since we re-enable interrupts anyway. There is also no need to prevent
it at that time, since we reinitialize everything during thaw, so the
device is in a consistent state. So to simplify things enable polling
during thaw, which will allow us to handle S4 thaw the same way as S3
resume in an upcoming patch.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d0fee60..ea8dda7 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -733,6 +733,8 @@ static int __i915_drm_thaw(struct drm_device *dev)
intel_opregion_notify_adapter(dev, PCI_D0);
+ drm_kms_helper_poll_enable(dev);
+
return 0;
}
@@ -765,14 +767,7 @@ static int i915_resume_early(struct drm_device *dev)
static int i915_drm_resume(struct drm_device *dev)
{
- int ret;
-
- ret = __i915_drm_thaw(dev);
- if (ret)
- return ret;
-
- drm_kms_helper_poll_enable(dev);
- return 0;
+ return __i915_drm_thaw(dev);
}
static int i915_resume_legacy(struct drm_device *dev)
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/16] drm/i915: remove unused restore_gtt_mappings optimization during suspend
The logic to skip restoring GTT mappings was added to speed up
suspend/resume, but not on old GENs where not restoring them caused
problems. The check for old GENs is based on the existance of OpRegion,
but this doesn't work since opregion is initialized only after
the check. So we end up always restoring the mappings.
On my BYT - which has OpRegion - skipping restoring the mappings during
suspend doesn't work, I get a GPU hang after resume. Also the logic of
when to allow the optimization during S4 is reversed: we should allow it
during S4 thaw but not during S4 restore, but atm we have it the other
way around in the code.
Since correctness wins over optimal code and since the optimization
wasn't used anyway I decided not to try to fix it at this point, but
just remove it. This allows us to unify the S3 and S4 handlers in the
following patches.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a3addc2..5e25283 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -667,12 +667,11 @@ static int i915_drm_thaw_early(struct drm_device *dev)
return ret;
}
-static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
+static int __i915_drm_thaw(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
- if (drm_core_check_feature(dev, DRIVER_MODESET)
- restore_gtt_mappings) {
+ if (drm_core_check_feature(dev, DRIVER_MODESET)) {
mutex_lock(dev-struct_mutex);
i915_gem_restore_gtt_mappings(dev);
mutex_unlock(dev-struct_mutex);
@@ -740,7 +739,7 @@ static int i915_drm_thaw(struct drm_device *dev)
if (drm_core_check_feature(dev, DRIVER_MODESET))
i915_check_and_clear_faults(dev);
- return __i915_drm_thaw(dev, true);
+ return __i915_drm_thaw(dev);
}
static int i915_resume_early(struct drm_device *dev)
@@ -767,15 +766,9 @@ static int i915_resume_early(struct drm_device *dev)
static int i915_drm_resume(struct drm_device *dev)
{
- struct drm_i915_private *dev_priv = dev-dev_private;
int ret;
- /*
-* Platforms with opregion should have sane BIOS, older ones (gen3 and
-* earlier) need to restore the GTT mappings since the BIOS might clear
-* all our scratch PTEs.
-*/
- ret = __i915_drm_thaw(dev, !dev_priv-opregion.header);
+ ret = __i915_drm_thaw(dev);
if (ret)
return ret;
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 14/16] drm/i915: add poweroff_late handler
The suspend_late handler saves some registers and powers off the device,
so it doesn't have a big overhead. Calling it at S4 poweroff_late time
makes the power off handling identical to the S3 suspend and S4 freeze
handling, so do this for consistency.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f6157a1..907a027 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1520,6 +1520,7 @@ static const struct dev_pm_ops i915_pm_ops = {
.thaw_early = i915_pm_resume_early,
.thaw = i915_pm_resume,
.poweroff = i915_pm_suspend,
+ .poweroff_late = i915_pm_suspend_late,
.restore_early = i915_pm_resume_early,
.restore = i915_pm_resume,
.runtime_suspend = intel_runtime_suspend,
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 15/16] drm/i915: unify switcheroo and legacy suspend/resume handlers
By now we handle switcheroo and legacy suspend/resume the same way, so
no need to keep separate functions for them.
No functional change.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_dma.c | 4 ++--
drivers/gpu/drm/i915/i915_drv.c | 11 +++
drivers/gpu/drm/i915/i915_drv.h | 4 ++--
3 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 09dc0d0..6c86e88 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1274,12 +1274,12 @@ static void i915_switcheroo_set_state(struct pci_dev
*pdev, enum vga_switcheroo_
dev-switch_power_state = DRM_SWITCH_POWER_CHANGING;
/* i915 resume handler doesn't set to D0 */
pci_set_power_state(dev-pdev, PCI_D0);
- i915_resume(dev);
+ i915_resume_legacy(dev);
dev-switch_power_state = DRM_SWITCH_POWER_ON;
} else {
pr_err(switched off\n);
dev-switch_power_state = DRM_SWITCH_POWER_CHANGING;
- i915_suspend(dev, pmm);
+ i915_suspend_legacy(dev, pmm);
dev-switch_power_state = DRM_SWITCH_POWER_OFF;
}
}
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 907a027..55b04e6 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -627,7 +627,7 @@ static int i915_drm_suspend_late(struct drm_device *drm_dev)
return 0;
}
-int i915_suspend(struct drm_device *dev, pm_message_t state)
+int i915_suspend_legacy(struct drm_device *dev, pm_message_t state)
{
int error;
@@ -755,7 +755,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
return ret;
}
-static int i915_resume_legacy(struct drm_device *dev)
+int i915_resume_legacy(struct drm_device *dev)
{
int ret;
@@ -766,11 +766,6 @@ static int i915_resume_legacy(struct drm_device *dev)
return i915_drm_resume(dev);
}
-int i915_resume(struct drm_device *dev)
-{
- return i915_resume_legacy(dev);
-}
-
/**
* i915_reset - reset chip after a hang
* @dev: drm device to reset
@@ -1564,7 +1559,7 @@ static struct drm_driver driver = {
.set_busid = drm_pci_set_busid,
/* Used in place of i915_pm_ops for non-DRIVER_MODESET */
- .suspend = i915_suspend,
+ .suspend = i915_suspend_legacy,
.resume = i915_resume_legacy,
.device_is_agp = i915_driver_device_is_agp,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e3ca8df..1396005 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2205,8 +2205,8 @@ struct drm_i915_cmd_table {
extern const struct drm_ioctl_desc i915_ioctls[];
extern int i915_max_ioctl;
-extern int i915_suspend(struct drm_device *dev, pm_message_t state);
-extern int i915_resume(struct drm_device *dev);
+extern int i915_suspend_legacy(struct drm_device *dev, pm_message_t state);
+extern int i915_resume_legacy(struct drm_device *dev);
extern int i915_master_create(struct drm_device *dev, struct drm_master
*master);
extern void i915_master_destroy(struct drm_device *dev, struct drm_master
*master);
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 12/16] drm/i915: unify S3 and S4 suspend/resume handlers
The S3 and S4 events are now handled the same way internally, there is no
need to keep separate wrapper functions around them. Simply reuse the
suspend/resume versions everywhere.
No functional change.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 98 +++--
1 file changed, 17 insertions(+), 81 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0ff5e92..44c1c03 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -651,22 +651,6 @@ int i915_suspend(struct drm_device *dev, pm_message_t
state)
return i915_drm_suspend_late(dev);
}
-static int i915_drm_thaw_early(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = dev-dev_private;
- int ret;
-
- 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 ret;
-}
-
static int __i915_drm_thaw(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -738,13 +722,11 @@ static int __i915_drm_thaw(struct drm_device *dev)
return 0;
}
-static int i915_drm_thaw(struct drm_device *dev)
-{
- return __i915_drm_thaw(dev);
-}
-
static int i915_resume_early(struct drm_device *dev)
{
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ int ret;
+
if (dev-switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
@@ -762,7 +744,15 @@ static int i915_resume_early(struct drm_device *dev)
pci_set_master(dev-pdev);
- return i915_drm_thaw_early(dev);
+ 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 ret;
}
static int i915_drm_resume(struct drm_device *dev)
@@ -971,60 +961,6 @@ static int i915_pm_resume(struct device *dev)
return i915_drm_resume(drm_dev);
}
-static int i915_pm_freeze(struct device *dev)
-{
- struct pci_dev *pdev = to_pci_dev(dev);
- struct drm_device *drm_dev = pci_get_drvdata(pdev);
-
- if (!drm_dev || !drm_dev-dev_private) {
- dev_err(dev, DRM not initialized, aborting suspend.\n);
- return -ENODEV;
- }
-
- if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF)
- return 0;
-
- return i915_drm_freeze(drm_dev);
-}
-
-static int i915_pm_freeze_late(struct device *dev)
-{
- struct pci_dev *pdev = to_pci_dev(dev);
- struct drm_device *drm_dev = pci_get_drvdata(pdev);
-
- if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF)
- return 0;
-
- return i915_drm_suspend_late(drm_dev);
-}
-
-static int i915_pm_thaw_early(struct device *dev)
-{
- struct pci_dev *pdev = to_pci_dev(dev);
- struct drm_device *drm_dev = pci_get_drvdata(pdev);
-
- return i915_resume_early(drm_dev);
-}
-
-static int i915_pm_thaw(struct device *dev)
-{
- struct pci_dev *pdev = to_pci_dev(dev);
- struct drm_device *drm_dev = pci_get_drvdata(pdev);
-
- return i915_drm_thaw(drm_dev);
-}
-
-static int i915_pm_poweroff(struct device *dev)
-{
- struct pci_dev *pdev = to_pci_dev(dev);
- struct drm_device *drm_dev = pci_get_drvdata(pdev);
-
- if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF)
- return 0;
-
- return i915_drm_freeze(drm_dev);
-}
-
static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
{
hsw_enable_pc8(dev_priv);
@@ -1584,11 +1520,11 @@ static const struct dev_pm_ops i915_pm_ops = {
.suspend_late = i915_pm_suspend_late,
.resume_early = i915_pm_resume_early,
.resume = i915_pm_resume,
- .freeze = i915_pm_freeze,
- .freeze_late = i915_pm_freeze_late,
- .thaw_early = i915_pm_thaw_early,
- .thaw = i915_pm_thaw,
- .poweroff = i915_pm_poweroff,
+ .freeze = i915_pm_suspend,
+ .freeze_late = i915_pm_suspend_late,
+ .thaw_early = i915_pm_resume_early,
+ .thaw = i915_pm_resume,
+ .poweroff = i915_pm_suspend,
.restore_early = i915_pm_resume_early,
.restore = i915_pm_resume,
.runtime_suspend = intel_runtime_suspend,
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 13/16] drm/i915: sanitize suspend/resume helper function names
By now the S4 freeze/thaw and S3 suspend/resume events are handled the
same way, so we can rename the freeze/thaw internal helpers to
suspend/resume accordingly to make clearer what the helpers do. Also
rename i915_resume_early to i915_drm_resume_early aligning it with the
rest of the helper names.
No functional change.
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 19 +++
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 44c1c03..f6157a1 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -532,7 +532,7 @@ static int intel_suspend_complete(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)
+static int i915_drm_suspend(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
struct drm_crtc *crtc;
@@ -644,14 +644,14 @@ int i915_suspend(struct drm_device *dev, pm_message_t
state)
if (dev-switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
- error = i915_drm_freeze(dev);
+ error = i915_drm_suspend(dev);
if (error)
return error;
return i915_drm_suspend_late(dev);
}
-static int __i915_drm_thaw(struct drm_device *dev)
+static int i915_drm_resume(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -722,7 +722,7 @@ static int __i915_drm_thaw(struct drm_device *dev)
return 0;
}
-static int i915_resume_early(struct drm_device *dev)
+static int i915_drm_resume_early(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
int ret;
@@ -755,16 +755,11 @@ static int i915_resume_early(struct drm_device *dev)
return ret;
}
-static int i915_drm_resume(struct drm_device *dev)
-{
- return __i915_drm_thaw(dev);
-}
-
static int i915_resume_legacy(struct drm_device *dev)
{
int ret;
- ret = i915_resume_early(dev);
+ ret = i915_drm_resume_early(dev);
if (ret)
return ret;
@@ -922,7 +917,7 @@ static int i915_pm_suspend(struct device *dev)
if (drm_dev-switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
- return i915_drm_freeze(drm_dev);
+ return i915_drm_suspend(drm_dev);
}
static int i915_pm_suspend_late(struct device *dev)
@@ -950,7 +945,7 @@ static int i915_pm_resume_early(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
struct drm_device *drm_dev = pci_get_drvdata(pdev);
- return i915_resume_early(drm_dev);
+ return i915_drm_resume_early(drm_dev);
}
static int i915_pm_resume(struct device *dev)
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/16] drm/i915: vlv: fix switcheroo/legacy suspend/resume
During switcheroo/legacy suspend we don't call the suspend_late handler but
when resuming afterwards we call resume_early. This happened to work so far,
since suspend_late only disabled the PCI device. This changed in
commit 016970beb05da6285c2f3ed2bee1c676cb75972e
Author: Sagar Kamble sagar.a.kam...@intel.com
Date: Wed Aug 13 23:07:06 2014 +0530
drm/i915: Sharing platform specific sequence between runtime and system susp
after which we also saved/restored the VLV Gunit HW state in
suspend_late/resume_early. So now since we don't save the state during
suspend a following resume will restore a corrupted state.
Fix this by calling the suspend_late handler during both switcheroo and
legacy suspend.
CC: Sagar Kamble sagar.a.kam...@intel.com
Signed-off-by: Imre Deak imre.d...@intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 17 +
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 89b63fc..ca74d6d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -648,11 +648,7 @@ int i915_suspend(struct drm_device *dev, pm_message_t
state)
if (error)
return error;
- /* Shut down the device */
- pci_disable_device(dev-pdev);
- pci_set_power_state(dev-pdev, PCI_D3hot);
-
- return 0;
+ return i915_drm_suspend_late(dev);
}
static int i915_drm_thaw_early(struct drm_device *dev)
@@ -769,7 +765,7 @@ static int i915_resume_early(struct drm_device *dev)
return i915_drm_thaw_early(dev);
}
-int i915_resume(struct drm_device *dev)
+static int i915_drm_resume(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev-dev_private;
int ret;
@@ -795,7 +791,12 @@ static int i915_resume_legacy(struct drm_device *dev)
if (ret)
return ret;
- return i915_resume(dev);
+ return i915_drm_resume(dev);
+}
+
+int i915_resume(struct drm_device *dev)
+{
+ return i915_resume_legacy(dev);
}
/**
@@ -980,7 +981,7 @@ static int i915_pm_resume(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
struct drm_device *drm_dev = pci_get_drvdata(pdev);
- return i915_resume(drm_dev);
+ return i915_drm_resume(drm_dev);
}
static int i915_pm_freeze(struct device *dev)
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PULL] topic/vblank-rework
On Wed, Sep 10, 2014 at 5:29 PM, Daniel Vetter daniel.vet...@ffwll.ch wrote:
On Wed, Sep 10, 2014 at 4:19 PM, Mario Kleiner
mario.kleiner...@gmail.com wrote:
Hmm, not quite an ack from my side for the pull in its current form. I
said if the two remaining issues i mentioned are addressed, then i'm
happy with it and can have my reviewed/acked-by. Looking at the code
they haven't been adressed.
Sorry about the confusion, I've somehow thought that you've retracted
those comments in Message-ID:
caesyxygk4foqhky1wcerak_hybex2ogpftjyhu_zfhlbx46...@mail.gmail.com
But I've missed that that was about just one of the issues.
Thought so. That one patch turns out to be crucial. My own software
immediately complained loudly about broken vblank irqs and switched to
lower performance fallbacks when that patch was missing.
I'll test the patches on a few more cards in the next days - but so
far things look good at least as far as my special test cases go.
However, this is easily fixable on top of the current patches:
1. A vblank_disable_timeout module parameter of zero should always
leave vblank irq's enabled and also override the drivers choice,
otherwise a user can't override the driver on a broken driver/gpu
combo, which is the only use case for having that module parameter.
Currenty the disable_immediately flag overrides the users override -
Ouch.
So in drm_vblank_put():
...
/* Last user schedules interrupt disable */
if (atomic_dec_and_test(vblank-refcount)) {
Insert zero - opt-out check
if (drm_vblank_offdelay == 0)
return;
Remaining code continues
if (dev-vblank_disable_immediate || drm_vblank_offdelay 0)
vblank_disable_fn((unsigned long)vblank);
else if (drm_vblank_offdelay 0)
mod_timer(vblank-disable_timer, jiffies +
((drm_vblank_offdelay * HZ)/1000));
Yeah, I guess that makes sense. I'm not really a fan of giving users
too powerful module options to hack around driver bugs since often
that means they'll never report the bug :( But we have the support now
to mark certain module options as debug-only and they'll taint the
kernel if set, so this is fixable.
I'll follow up with the patch you've suggested.
Thanks. I think the modules parameters i usually care about will get
proper testing and reporting, because while my software and users are
good at detecting such problems, they wouldn't know how to fix them
themselves, and at the same time they crucially depend on this stuff
working, so this gets reported to me quickly and i can give them the
module param workaround in private e-mail and take it from there with
proper bug reports or patches.
...
2. For the drm: Have the vblank counter account for the time ...
patch, we must opt-out of that last timestamp/counter update/bump if
the driver doesn't support high-precision vblank timestamping,
otherwise the vblank count and timestamp will be inconsistent with
each other - or outright wrong in case of the timestamp. Rather
deliver a slightly outdated, but correct count+timestamp pair to
userspace, which is still useable for practical purposes, than a pair
that's outright wrong and will definitely confuse clients.
A simple fix in static void vblank_disable_and_save() would be to
replace the new...
if (!vblank-enabled) {
... check by ...
if (!vblank-enabled
) {
Yeah, makes sense (well the follow-up one ofc). I'll do a patch which
adds this and adds a comment. Aside I think it would be useful to add
a #define for the 0 return value, since the magic checks all over are
imo fairly hard to understand.
I'll also float a patch for rfc about that.
Good!
thanks,
-mario
Thanks for your comments and again my apologies for missing that
there's still outstanding work left to do on this.
Cheers, Daniel
On Wed, Sep 10, 2014 at 2:05 PM, Daniel Vetter daniel.vet...@ffwll.ch
wrote:
Hi Dave,
So here's the final bits of Ville's vblank rework with a bit of cleanup
from Mario on top.
The neat thing this finally allows is to immediately disable the vblank
interrupt on the last drm_vblank_put if the hardware has perfectly
accurate vblank counter and timestamp readout support. On i915 that
required piles of small adjustements from Ville since depending upon the
platform and port the vblank happens at different scanout lines.
Of course this is fully opt-in and per-device (we need that since gen2
doesn't have a hw vblank counter).
Mario reviewed the entire pile too and after some initial hesitation
(about drivers without accurate timestampt support) acked it.
Cheers, Daniel
The following changes since commit 21d70354bba9965a098382fc4d7fb17e138111f3:
drm: move drm_stub.c to drm_drv.c (2014-08-06 19:10:44 +1000)
are available in the git repository at:
git://anongit.freedesktop.org/drm-intel
tags/topic/vblank-rework-2014-09-10
for you to fetch changes up to 2368ffb18b1d2b04eb80478d225676caa7a3c4c8:
drm: Use vblank_disable_and_save in
Re: [Intel-gfx] [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write
On Wed, Sep 10, 2014 at 09:16:45AM +0200, Daniel Vetter wrote:
On Tue, Sep 09, 2014 at 02:25:50PM -0700, Jesse Barnes wrote:
On Tue, 09 Sep 2014 21:45:08 +0530
Deepak S deepa...@intel.com wrote:
On Monday 08 September 2014 08:10 PM, Daniel Vetter wrote:
On Mon, Sep 08, 2014 at 05:14:23PM +0300, Ville Syrjälä wrote:
On Mon, Sep 08, 2014 at 05:02:43PM +0300, Ville Syrjälä wrote:
On Tue, Sep 09, 2014 at 07:14:16PM +0530,
deepa...@linux.intel.com wrote:
From: Deepak S deepa...@linux.intel.com
In chv, we have two power wells Render Media. We need to use
corresponsing forcewake count. If we dont follow this we are
getting error *ERROR*: Timed out waiting for forcewake old ack
to clear due to multiple entry into __vlv_force_wake_get.
Signed-off-by: Deepak S deepa...@linux.intel.com
---
drivers/gpu/drm/i915/intel_lrc.c | 29
+ 1 file changed, 25 insertions(+),
4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c
b/drivers/gpu/drm/i915/intel_lrc.c index bd1b28d..bafd38b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -300,8 +300,18 @@ static void execlists_elsp_write(struct
intel_engine_cs *ring,
* Instead, we do the runtime_pm_get/put when
creating/destroying requests. */
spin_lock_irqsave(dev_priv-uncore.lock, flags);
-if (dev_priv-uncore.forcewake_count++ == 0)
-dev_priv-uncore.funcs.force_wake_get(dev_priv,
FORCEWAKE_ALL);
+if (IS_CHERRYVIEW(dev_priv-dev)) {
+if (dev_priv-uncore.fw_rendercount++ == 0)
+
dev_priv-uncore.funcs.force_wake_get(dev_priv,
+
FORCEWAKE_RENDER);
+if (dev_priv-uncore.fw_mediacount++ == 0)
+
dev_priv-uncore.funcs.force_wake_get(dev_priv,
+
FORCEWAKE_MEDIA);
This will wake both wells. Is that needed or should we just pick
one based on the ring?
Also unlike the comment says runtime_pm_get() can't sleep since
someone must already be holding a reference, othwewise we surely
can't go writing any registers. So in theory we should be able to
call gen6_gt_force_wake_get() here, but maybe that would trigger a
might_sleep() warning. the current force wake code duplication
(esp. outside intel_uncore.c) is rather unfortunate and I'd like
to see it killed off. Maybe we just need to pull the rpm get/put
outside gen6_gt_force_wake_get()? I never really liked hiding it
there anyway.
Yeah this is just broken design. And if you look at the other wheel
to track outstanding gpu work (requests) then it's not needed at
all.
But I'm not sure what's the priority of the rework execlists to use
requests task is and when (if ever that will happen). Jesse is the
arbiter for this stuff anyway, so adding him.
-Daniel
hmm , agreed do we have a reworked execlist? The reason why added
this, on chv when i enable execlist, due to incorrect forcewake count
is causing multiple entries to forcewake_get resulting in *ERROR*:
Timed out waiting for forcewake old ack to clear and Hang.
I'm hoping we can get execlists reworked on top of the request/seqno
stuff shortly after it lands, but I don't think that's a reason to
block this fix, since Chris is still busy fixing up the request
changes.
Queued for -next, thanks for the patch.
Aside if someone gets bored and wants to apply some polish: And __ variant
of force_wake_get/put which only asserts that the device isn't runtime
suspended instead of doing the runtime_pm_get/put would be cute - we have
one other user in the hsw/bdw rpm code already.
The current force_wake_get/put functions would then just wrap those raw
versions.
-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 00/16] fix VLV S4 suspend/resume, unify S3/S4 handlers
On Wed, Sep 10, 2014 at 06:16:53PM +0300, Imre Deak wrote: The first part of the patchset (1-6) fixes an S4 bug on VLV introduced recently. The rest unifies the various S3/S4 handlers, which are in practice the same. The only real difference - besides some unused code - is that during S3 suspend we disable the PCI device whereas across an S4 freeze/thaw we keep it enabled. Given that we disable everything else anyway, we can just as well disable the PCI device there too. Doing so allows us to handle S3 suspend/resume and S4 freeze/thaw/restore/ power-off the same way, simplifying/clarifying things quite a bit. Hm, this might explain why we've seen random corruption on S4 on recent platforms. https://bugzilla.kernel.org/show_bug.cgi?id=59321 Can you please ask for retesting from reporters? Thanks, Daniel I smoke tested this on BDW, HSW, VLV (BYT-M), IVB, GEN45. Imre Deak (16): drm/i915: vlv: fix gunit HW state corruption during S4 suspend drm/i915: remove dead code from legacy suspend handler drm/i915: factor out i915_drm_suspend_late drm/i915: unify legacy S3 suspend and S4 freeze handlers drm/i915: propagate error from legacy resume handler drm/i915: vlv: fix switcheroo/legacy suspend/resume drm/i915: fix S4 suspend while switcheroo state is off drm/i915: remove unused restore_gtt_mappings optimization during suspend drm/i915: check for GT faults during S3 resume and S4 restore too drm/i915: enable output polling during S4 thaw drm/i915: disable/re-enable PCI device around S4 freeze/thaw drm/i915: unify S3 and S4 suspend/resume handlers drm/i915: sanitize suspend/resume helper function names drm/i915: add poweroff_late handler drm/i915: unify switcheroo and legacy suspend/resume handlers drm/i915: add comments on what stage a given PM handler is called drivers/gpu/drm/i915/i915_dma.c | 4 +- drivers/gpu/drm/i915/i915_drv.c | 185 +++- drivers/gpu/drm/i915/i915_drv.h | 4 +- 3 files changed, 74 insertions(+), 119 deletions(-) -- 1.8.4 ___ 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: add cherryview specfic forcewake in execlists_elsp_write
On Wed, Sep 10, 2014 at 05:47:39PM +0200, Daniel Vetter wrote: Aside if someone gets bored and wants to apply some polish: And __ variant of force_wake_get/put which only asserts that the device isn't runtime suspended instead of doing the runtime_pm_get/put would be cute - we have one other user in the hsw/bdw rpm code already. The current force_wake_get/put functions would then just wrap those raw versions. I fail to see the reason why they take it at all. Conceptually, gen6_gt_force_wake_get() is just as lowlevel as the register access they wrap. The only one that breaks the rule is i915_debugfs.c and that seems easy enough to wrap with its own intel_runtime_pm_get/_put. And once you've finish with that you can then sort out the mess that is the vlv variants. And now skl continues in the cut'n'paste'n'paste vein. -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 3/4] drm/i915: CSC color correction
On Wed, 10 Sep 2014 14:25:00 +0530
Sharma, Shashank shashank.sha...@intel.com wrote:
Thanks for your time and review.
Thanks for working on this. This is a feature that the IOTG group is
interested in.
Please find my comments inline.
Regards
Shashank
On 9/10/2014 4:21 AM, Bob Paauwe wrote:
On Tue, 9 Sep 2014 11:53:15 +0530
shashank.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@intel.com
This patch adds support for CSC correction color property.
It does the following:
1. Creates a new DRM property for CSC correction. Adds this into
mode_config.
2. Attaches this CSC property to calling CRTC. Creates a blob
to store the correction values, and attaches the blob to CRTC.
3. Adds function intel_clrmgr_set_csc: This is a wrapper function
which checks the platform type, and calls the valleyview
specific set_csc function. As different platforms may have different
methods of setting CSC, wrapper function is required to route to proper
core CSC set function. In future, the support for other platfroms can
be
plugged-in here. Adding this function as .set_property CSC color
property.
4. Adds function vlv_set_csc: core function to program CSC coefficients as
per
vlv specs, and then enable CSC.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/drm_crtc.c | 4 +-
drivers/gpu/drm/i915/i915_reg.h | 11 ++
drivers/gpu/drm/i915/intel_clrmgr.c | 208
+++-
drivers/gpu/drm/i915/intel_clrmgr.h | 16 +++
drivers/gpu/drm/i915/intel_drv.h| 1 +
include/drm/drm_crtc.h | 7 ++
6 files changed, 244 insertions(+), 3 deletions(-)
+*/
+/* Enable color space conversion on PIPE */
+bool vlv_set_csc(struct intel_crtc *intel_crtc,
+ struct drm_property *prop, uint64_t values, bool enable)
+{
+ u32 count = 0;
+ u32 c0, c1, c2;
+ u32 pipeconf, csc_reg, data_size;
+ uint64_t *blob_data;
+ uint64_t *data = NULL;
+ struct drm_device *dev = intel_crtc-base.dev;
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ struct drm_property_blob *blob = intel_crtc-csc_blob;
+
+ /* Sanity */
+ if (!blob || (blob-length != VLV_CSC_MATRIX_MAX_VALS)) {
+ DRM_ERROR(Invalid/NULL CSC blob\n);
+ return false;
+ }
+ blob_data = (uint64_t *)blob-data;
+
+ /* No need of values to disable property */
+ if (!enable)
+ goto configure;
+
+ /* Enabling property needs correction values */
+ data_size = VLV_CSC_MATRIX_MAX_VALS;
+ data = kmalloc(sizeof(uint64_t) * (data_size), GFP_KERNEL);
+ if (!data) {
+ DRM_ERROR(Out of memory\n);
+ return false;
+ }
+
+ if (copy_from_user((void *)data, (const void __user *)values,
+ data_size * sizeof(uint64_t))) {
+ DRM_ERROR(Failed to copy all data\n);
+ kfree(data);
+ return false;
+ }
I don't think this should be doing a copy_from_user. It should be the
drm_property code that handles the IOCTL that does that.
The whole handling of the blob property looks suspect to me. I believe
that the DRM code currently doesn't allow blob type properties to be
set. So you should first have a patch that adds that capability to the
DRM property code. But I'm not sure that's even the right way to
handle this.
Humm. This adds another dilemma of how to do this.
I believe blob property type is the only one which suits the CSC
property, (also gamma correction, with 129 correction values)
Even if we create a set_blob, that will be again doing a
copy_from_user() only, won't it ?
I can add a patch to do a set_blob in the next patch set. Please have a
look and let me know if that's what you expect.
Yeah, any set_blob function will need to do the copy_from_user but I
think it would be better if this was in the core drm code. I thought I
saw a patch a while back that added writable property blobs but I don't
see it in the code so maybe it never got in. Might be worth searching
the archive for that.
+
+ DRM_DEBUG_DRIVER(Setting CSC on pipe = %d\n, intel_crtc-pipe);
+ csc_reg = PIPECSC(intel_crtc-pipe);
+
+ /* Read CSC matrix, one row at a time */
+ while (count VLV_CSC_MATRIX_MAX_VALS) {
+ c0 = data[count] VLV_CSC_VALUE_MASK;
+ *blob_data++ = c0;
+ c1 = data[count] VLV_CSC_VALUE_MASK;
+ *blob_data++ = c1;
+ c2 = data[count] VLV_CSC_VALUE_MASK;
+ *blob_data++ = c2;
You aren't incrementing count after each assignment above, that means
that c0, c1, and c2 are all getting set to the same value. That doesn't
seem right.
I am sorry, this is bad. While splitting the patches, I messed here, and
looks like the unit CSC values were getting applied properly, so dint
catch this during ULT.
Thanks for
Re: [Intel-gfx] [PATCH 1/4] drm/i915: Color manager framework for valleyview
On Wed, 10 Sep 2014 14:10:01 +0530
Sharma, Shashank shashank.sha...@intel.com wrote:
Hello Bob,
Thanks for your time and review comments.
Please find my comments inline.
Regards
Shashank
On 9/10/2014 4:21 AM, Bob Paauwe wrote:
On Tue, 9 Sep 2014 11:53:13 +0530
shashank.sha...@intel.com wrote:
From: Shashank Sharma shashank.sha...@intel.com
Color manager is a framework which adds drm properties for
color correction in I915 driver. This framework creates DRM
properties for each color correction feature, and attaches
it to appropriate CRTC/plane based on the property type.
This allows userspace to fine tune the display as per the panel.
This is the first patch of the series, what this patch does is:
1. Create 2 new files
intel_clrmgr.c
intel_clrmgr.h
2. Add color manager init function, This function will create
DRM properties for color correction.
3. Add color manager exit function. This function will destroy
registered DRM color properties.
4. Adds a enum for currently listed color correction properties:
they are:
CSC correction (wide gamut), Gamma correction, Contrast,
Brightness, Hue and Saturation
This enum will be further used to index color properties
from array of elements.
5. Add names for vlv color properties.
I'd suggest maybe dropping the name color manager and clrmgr in
favor of color properties and maybe color props as a shorter form.
I will try to comply with this suggestion.
Signed-off-by: Shashank Sharma shashank.sha...@intel.com
---
drivers/gpu/drm/i915/Makefile| 3 +-
drivers/gpu/drm/i915/intel_clrmgr.c | 80
drivers/gpu/drm/i915/intel_clrmgr.h | 70 +++
drivers/gpu/drm/i915/intel_display.c | 5 +++
4 files changed, 157 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.c
create mode 100644 drivers/gpu/drm/i915/intel_clrmgr.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index c1dd485..6361c9b 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -46,7 +46,8 @@ i915-y += intel_bios.o \
intel_modes.o \
intel_overlay.o \
intel_sideband.o \
-intel_sprite.o
+intel_sprite.o \
+intel_clrmgr.o
i915-$(CONFIG_ACPI) += intel_acpi.o intel_opregion.o
i915-$(CONFIG_DRM_I915_FBDEV)+= intel_fbdev.o
diff --git a/drivers/gpu/drm/i915/intel_clrmgr.c
b/drivers/gpu/drm/i915/intel_clrmgr.c
new file mode 100644
index 000..0def917
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_clrmgr.c
@@ -0,0 +1,80 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
Software),
+ * to deal in the Software without restriction, including without
limitation
+ * the rights to use, copy, modify, merge, publish, distribute,
sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
next
+ * paragraph) shall be included in all copies or substantial portions of
the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ * ==
+ * Shashank Sharma shashank.sha...@intel.com
+ * Uma Shankar uma.shan...@intel.com
+ * Sonika Jindal sonika.jin...@intel.com
+ */
+
+#include i915_drm.h
+#include i915_drv.h
+#include i915_reg.h
+#include intel_clrmgr.h
+
+/* Names to register with color properties */
+const char *clrmgr_property_names[] = {
+ /* csc */
+ csc-correction,
+ /* gamma */
+ gamma-correction,
+ /* contrast */
+ contrast,
+ /* brightness */
+ brightness,
+ /* hue_saturation */
+ hue_saturation
+ /* add a new prop name here */
+};
I don't think you need the comments for each of the names. The names
themselves are descriptive.
I haven't looked at the rest of the series yet, but in embedded we've
considered hue and saturation separate properties.
Actually they are, but in VLV hardware they are getting
programmed/adjusted via the same register (single), so I am force to
combine them together.
But the interface to them
Re: [Intel-gfx] [PATCH v2] drm/i915: pin sprite fb only if it changed
On Wed, Sep 10, 2014 at 12:03:17PM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.pado...@collabora.co.uk
Optimize code avoiding helding dev mutex if old fb and current fb
are the same.
v2: take Ville's comments
- move comment along with the pin_and_fence call
- check for error before calling i915_gem_track_fb
- move old_obj != obj to an upper if condition
Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
lgtm
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_sprite.c | 34 +++---
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index a4306cf..90bb45f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1038,21 +1038,24 @@ intel_commit_sprite_plane(struct drm_plane *plane,
primary_enabled = !drm_rect_equals(dst, clip) ||
colorkey_enabled(intel_plane);
WARN_ON(!primary_enabled !state-visible intel_crtc-active);
- mutex_lock(dev-struct_mutex);
-
- /* Note that this will apply the VT-d workaround for scanouts,
- * which is more restrictive than required for sprites. (The
- * primary plane requires 256KiB alignment with 64 PTE padding,
- * the sprite planes only require 128KiB alignment and 32 PTE padding.
- */
- ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
- i915_gem_track_fb(old_obj, obj,
- INTEL_FRONTBUFFER_SPRITE(pipe));
- mutex_unlock(dev-struct_mutex);
+ if (old_obj != obj) {
+ mutex_lock(dev-struct_mutex);
- if (ret)
- return ret;
+ /* Note that this will apply the VT-d workaround for scanouts,
+ * which is more restrictive than required for sprites. (The
+ * primary plane requires 256KiB alignment with 64 PTE padding,
+ * the sprite planes only require 128KiB alignment and 32 PTE
+ * padding.
+ */
+ ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+ if (ret == 0)
+ i915_gem_track_fb(old_obj, obj,
+ INTEL_FRONTBUFFER_SPRITE(pipe));
+ mutex_unlock(dev-struct_mutex);
+ if (ret)
+ return ret;
+ }
intel_plane-crtc_x = state-orig_dst.x1;
intel_plane-crtc_y = state-orig_dst.y1;
@@ -1099,14 +1102,15 @@ intel_commit_sprite_plane(struct drm_plane *plane,
}
/* Unpin old obj after new one is active to avoid ugliness */
- if (old_obj) {
+ if (old_obj old_obj != obj) {
+
/*
* It's fairly common to simply update the position of
* an existing object. In that case, we don't need to
* wait for vblank to avoid ugliness, we only need to
* do the pin ref bookkeeping.
*/
- if (old_obj != obj intel_crtc-active)
+ if (intel_crtc-active)
intel_wait_for_vblank(dev, intel_crtc-pipe);
mutex_lock(dev-struct_mutex);
--
1.9.3
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: create intel_update_pipe_size()
On Wed, Sep 10, 2014 at 12:04:17PM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.pado...@collabora.co.uk
Factor out a piece of code from intel_pipe_set_base() that updates
the pipe size and adjust fitter.
This will help refactor the update primary plane path.
v2: use struct intel_crtc as argument to intel_update_pipe_size()
v3: use 'crtc' as argument name
Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_display.c | 70
1 file changed, 40 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 2ccf7c0..b78f00a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2779,6 +2779,45 @@ static bool intel_crtc_has_pending_flip(struct
drm_crtc *crtc)
return pending;
}
+static void intel_update_pipe_size(struct intel_crtc *crtc)
+{
+ struct drm_device *dev = crtc-base.dev;
+ struct drm_i915_private *dev_priv = dev-dev_private;
+ const struct drm_display_mode *adjusted_mode;
+
+ if (!i915.fastboot)
+ return;
+
+ /*
+ * Update pipe size and adjust fitter if needed: the reason for this is
+ * that in compute_mode_changes we check the native mode (not the pfit
+ * mode) to see if we can flip rather than do a full mode set. In the
+ * fastboot case, we'll flip, but if we don't update the pipesrc and
+ * pfit state, we'll end up with a big fb scanned out into the wrong
+ * sized surface.
+ *
+ * To fix this properly, we need to hoist the checks up into
+ * compute_mode_changes (or above), check the actual pfit state and
+ * whether the platform allows pfit disable with pipe active, and only
+ * then update the pipesrc and pfit state, even on the flip path.
+ */
+
+ adjusted_mode = crtc-config.adjusted_mode;
+
+ I915_WRITE(PIPESRC(crtc-pipe),
+((adjusted_mode-crtc_hdisplay - 1) 16) |
+(adjusted_mode-crtc_vdisplay - 1));
+ if (!crtc-config.pch_pfit.enabled
+ (intel_pipe_has_type(crtc-base, INTEL_OUTPUT_LVDS) ||
+ intel_pipe_has_type(crtc-base, INTEL_OUTPUT_EDP))) {
+ I915_WRITE(PF_CTL(crtc-pipe), 0);
+ I915_WRITE(PF_WIN_POS(crtc-pipe), 0);
+ I915_WRITE(PF_WIN_SZ(crtc-pipe), 0);
+ }
+ crtc-config.pipe_src_w = adjusted_mode-crtc_hdisplay;
+ crtc-config.pipe_src_h = adjusted_mode-crtc_vdisplay;
+}
+
static int
intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
struct drm_framebuffer *fb)
@@ -2821,36 +2860,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int
y,
return ret;
}
- /*
- * Update pipe size and adjust fitter if needed: the reason for this is
- * that in compute_mode_changes we check the native mode (not the pfit
- * mode) to see if we can flip rather than do a full mode set. In the
- * fastboot case, we'll flip, but if we don't update the pipesrc and
- * pfit state, we'll end up with a big fb scanned out into the wrong
- * sized surface.
- *
- * To fix this properly, we need to hoist the checks up into
- * compute_mode_changes (or above), check the actual pfit state and
- * whether the platform allows pfit disable with pipe active, and only
- * then update the pipesrc and pfit state, even on the flip path.
- */
- if (i915.fastboot) {
- const struct drm_display_mode *adjusted_mode =
- intel_crtc-config.adjusted_mode;
-
- I915_WRITE(PIPESRC(intel_crtc-pipe),
-((adjusted_mode-crtc_hdisplay - 1) 16) |
-(adjusted_mode-crtc_vdisplay - 1));
- if (!intel_crtc-config.pch_pfit.enabled
- (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) ||
- intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))) {
- I915_WRITE(PF_CTL(intel_crtc-pipe), 0);
- I915_WRITE(PF_WIN_POS(intel_crtc-pipe), 0);
- I915_WRITE(PF_WIN_SZ(intel_crtc-pipe), 0);
- }
- intel_crtc-config.pipe_src_w = adjusted_mode-crtc_hdisplay;
- intel_crtc-config.pipe_src_h = adjusted_mode-crtc_vdisplay;
- }
+ intel_update_pipe_size(intel_crtc);
dev_priv-display.update_primary_plane(crtc, fb, x, y);
--
1.9.3
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
Re: [Intel-gfx] [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write
Hmm Ok Daniel. Let me try and clean up the forcewake. :) Chris, most of the debugfs is already covered with runtime_get/put. If anything is missed we can add that Thanks Deepak -Original Message- From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] Sent: Wednesday, September 10, 2014 9:21 PM To: Daniel Vetter Cc: Jesse Barnes; S, Deepak; intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH] drm/i915: add cherryview specfic forcewake in execlists_elsp_write On Wed, Sep 10, 2014 at 05:47:39PM +0200, Daniel Vetter wrote: Aside if someone gets bored and wants to apply some polish: And __ variant of force_wake_get/put which only asserts that the device isn't runtime suspended instead of doing the runtime_pm_get/put would be cute - we have one other user in the hsw/bdw rpm code already. The current force_wake_get/put functions would then just wrap those raw versions. I fail to see the reason why they take it at all. Conceptually, gen6_gt_force_wake_get() is just as lowlevel as the register access they wrap. The only one that breaks the rule is i915_debugfs.c and that seems easy enough to wrap with its own intel_runtime_pm_get/_put. And once you've finish with that you can then sort out the mess that is the vlv variants. And now skl continues in the cut'n'paste'n'paste vein. -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] rpm
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 ++
drivers/gpu/drm/i915/intel_display.c | 20 ++
drivers/gpu/drm/i915/intel_lrc.c | 21 ++-
drivers/gpu/drm/i915/intel_uncore.c | 52 +++-
4 files changed, 27 insertions(+), 68 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 5f35048..a72d8b8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inode,
struct file *file)
if (INTEL_INFO(dev)-gen 6)
return 0;
+ intel_runtime_pm_get(dev_priv);
gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
return 0;
@@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode *inode,
struct file *file)
return 0;
gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
+ intel_runtime_pm_put(dev_priv);
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 794ad8f..fafd202 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7596,7 +7596,6 @@ static void hsw_disable_lcpll(struct drm_i915_private
*dev_priv,
static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
{
uint32_t val;
- unsigned long irqflags;
val = I915_READ(LCPLL_CTL);
@@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct drm_i915_private
*dev_priv)
/*
* Make sure we're not on PC8 state before disabling PC8, otherwise
* we'll hang the machine. To prevent PC8 state, just enable force_wake.
-*
-* The other problem is that hsw_restore_lcpll() is called as part of
-* the runtime PM resume sequence, so we can't just call
-* gen6_gt_force_wake_get() because that function calls
-* intel_runtime_pm_get(), and we can't change the runtime PM refcount
-* while we are on the resume sequence. So to solve this problem we have
-* to call special forcewake code that doesn't touch runtime PM and
-* doesn't enable the forcewake delayed work.
*/
- spin_lock_irqsave(dev_priv-uncore.lock, irqflags);
- if (dev_priv-uncore.forcewake_count++ == 0)
- dev_priv-uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
- spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags);
+ gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
if (val LCPLL_POWER_DOWN_ALLOW) {
val = ~LCPLL_POWER_DOWN_ALLOW;
@@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct drm_i915_private
*dev_priv)
DRM_ERROR(Switching back to LCPLL failed\n);
}
- /* See the big comment above. */
- spin_lock_irqsave(dev_priv-uncore.lock, irqflags);
- if (--dev_priv-uncore.forcewake_count == 0)
- dev_priv-uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
- spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags);
+ gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
}
/*
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 6f1dd00..aeaa1bc 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -243,7 +243,6 @@ static void execlists_submit_pair(struct intel_engine_cs
*engine,
struct drm_i915_private *dev_priv = engine-i915;
uint64_t tmp;
uint32_t desc[4];
- unsigned long flags;
/* XXX: You must always write both descriptors in the order below. */
@@ -260,18 +259,7 @@ static void execlists_submit_pair(struct intel_engine_cs
*engine,
desc[1] = upper_32_bits(tmp);
desc[0] = lower_32_bits(tmp);
- /* Set Force Wakeup bit to prevent GT from entering C6 while ELSP writes
-* are in progress.
-*
-* The other problem is that we can't just call gen6_gt_force_wake_get()
-* because that function calls intel_runtime_pm_get(), which might
sleep.
-* Instead, we do the runtime_pm_get/put when creating/destroying
requests.
-*/
- spin_lock_irqsave(dev_priv-uncore.lock, flags);
- if (dev_priv-uncore.forcewake_count++ == 0)
- dev_priv-uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
- spin_unlock_irqrestore(dev_priv-uncore.lock, flags);
-
+ gen6_gt_force_wake_get(dev_priv, engine-power_domains);
I915_WRITE(RING_ELSP(engine), desc[1]);
I915_WRITE(RING_ELSP(engine), desc[0]);
I915_WRITE(RING_ELSP(engine), desc[3]);
@@ -280,12 +268,7 @@ static void execlists_submit_pair(struct intel_engine_cs
*engine,
/* ELSP is a wo register, so use another nearby reg for posting instead
*/
POSTING_READ(RING_EXECLIST_STATUS(engine));
-
- /* Release Force Wakeup (see the big comment above). */
-
Re: [Intel-gfx] [PATCH v2] drm/i915: pin sprite fb only if it changed
On Wed, Sep 10, 2014 at 07:26:53PM +0300, Ville Syrjälä wrote:
On Wed, Sep 10, 2014 at 12:03:17PM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.pado...@collabora.co.uk
Optimize code avoiding helding dev mutex if old fb and current fb
are the same.
v2: take Ville's comments
- move comment along with the pin_and_fence call
- check for error before calling i915_gem_track_fb
- move old_obj != obj to an upper if condition
Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
lgtm
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
Queued for -next, thanks for the patch.
-Daniel
---
drivers/gpu/drm/i915/intel_sprite.c | 34 +++---
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index a4306cf..90bb45f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1038,21 +1038,24 @@ intel_commit_sprite_plane(struct drm_plane *plane,
primary_enabled = !drm_rect_equals(dst, clip) ||
colorkey_enabled(intel_plane);
WARN_ON(!primary_enabled !state-visible intel_crtc-active);
- mutex_lock(dev-struct_mutex);
-
- /* Note that this will apply the VT-d workaround for scanouts,
-* which is more restrictive than required for sprites. (The
-* primary plane requires 256KiB alignment with 64 PTE padding,
-* the sprite planes only require 128KiB alignment and 32 PTE padding.
-*/
- ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
- i915_gem_track_fb(old_obj, obj,
- INTEL_FRONTBUFFER_SPRITE(pipe));
- mutex_unlock(dev-struct_mutex);
+ if (old_obj != obj) {
+ mutex_lock(dev-struct_mutex);
- if (ret)
- return ret;
+ /* Note that this will apply the VT-d workaround for scanouts,
+* which is more restrictive than required for sprites. (The
+* primary plane requires 256KiB alignment with 64 PTE padding,
+* the sprite planes only require 128KiB alignment and 32 PTE
+* padding.
+*/
+ ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+ if (ret == 0)
+ i915_gem_track_fb(old_obj, obj,
+ INTEL_FRONTBUFFER_SPRITE(pipe));
+ mutex_unlock(dev-struct_mutex);
+ if (ret)
+ return ret;
+ }
intel_plane-crtc_x = state-orig_dst.x1;
intel_plane-crtc_y = state-orig_dst.y1;
@@ -1099,14 +1102,15 @@ intel_commit_sprite_plane(struct drm_plane *plane,
}
/* Unpin old obj after new one is active to avoid ugliness */
- if (old_obj) {
+ if (old_obj old_obj != obj) {
+
/*
* It's fairly common to simply update the position of
* an existing object. In that case, we don't need to
* wait for vblank to avoid ugliness, we only need to
* do the pin ref bookkeeping.
*/
- if (old_obj != obj intel_crtc-active)
+ if (intel_crtc-active)
intel_wait_for_vblank(dev, intel_crtc-pipe);
mutex_lock(dev-struct_mutex);
--
1.9.3
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-de...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
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] rpm
2014-09-10 13:43 GMT-03:00 Chris Wilson ch...@chris-wilson.co.uk:
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 ++
drivers/gpu/drm/i915/intel_display.c | 20 ++
drivers/gpu/drm/i915/intel_lrc.c | 21 ++-
drivers/gpu/drm/i915/intel_uncore.c | 52
+++-
4 files changed, 27 insertions(+), 68 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 5f35048..a72d8b8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inode,
struct file *file)
if (INTEL_INFO(dev)-gen 6)
return 0;
+ intel_runtime_pm_get(dev_priv);
gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
return 0;
@@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode *inode,
struct file *file)
return 0;
gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
+ intel_runtime_pm_put(dev_priv);
return 0;
}
Just a minor comment on the chunk above. I didn't look at the rest of the patch.
We used to have exactly the code that you rewrote, but we decided to
remove it because, without it, we can test that gen6_gt_force_wake_get
actually wakes up the HW and keeps it awake until someone calls
gen6_gt_force_wake_put.
If we add these get/put calls above, we won't really detect any
problems related with the actual gen6_gt_force_wake_get/put functions,
so we may hide problems.
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 794ad8f..fafd202 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7596,7 +7596,6 @@ static void hsw_disable_lcpll(struct drm_i915_private
*dev_priv,
static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
{
uint32_t val;
- unsigned long irqflags;
val = I915_READ(LCPLL_CTL);
@@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct drm_i915_private
*dev_priv)
/*
* Make sure we're not on PC8 state before disabling PC8, otherwise
* we'll hang the machine. To prevent PC8 state, just enable
force_wake.
-*
-* The other problem is that hsw_restore_lcpll() is called as part of
-* the runtime PM resume sequence, so we can't just call
-* gen6_gt_force_wake_get() because that function calls
-* intel_runtime_pm_get(), and we can't change the runtime PM refcount
-* while we are on the resume sequence. So to solve this problem we
have
-* to call special forcewake code that doesn't touch runtime PM and
-* doesn't enable the forcewake delayed work.
*/
- spin_lock_irqsave(dev_priv-uncore.lock, irqflags);
- if (dev_priv-uncore.forcewake_count++ == 0)
- dev_priv-uncore.funcs.force_wake_get(dev_priv,
FORCEWAKE_ALL);
- spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags);
+ gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
if (val LCPLL_POWER_DOWN_ALLOW) {
val = ~LCPLL_POWER_DOWN_ALLOW;
@@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct drm_i915_private
*dev_priv)
DRM_ERROR(Switching back to LCPLL failed\n);
}
- /* See the big comment above. */
- spin_lock_irqsave(dev_priv-uncore.lock, irqflags);
- if (--dev_priv-uncore.forcewake_count == 0)
- dev_priv-uncore.funcs.force_wake_put(dev_priv,
FORCEWAKE_ALL);
- spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags);
+ gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
}
/*
diff --git a/drivers/gpu/drm/i915/intel_lrc.c
b/drivers/gpu/drm/i915/intel_lrc.c
index 6f1dd00..aeaa1bc 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -243,7 +243,6 @@ static void execlists_submit_pair(struct intel_engine_cs
*engine,
struct drm_i915_private *dev_priv = engine-i915;
uint64_t tmp;
uint32_t desc[4];
- unsigned long flags;
/* XXX: You must always write both descriptors in the order below. */
@@ -260,18 +259,7 @@ static void execlists_submit_pair(struct intel_engine_cs
*engine,
desc[1] = upper_32_bits(tmp);
desc[0] = lower_32_bits(tmp);
- /* Set Force Wakeup bit to prevent GT from entering C6 while ELSP
writes
-* are in progress.
-*
-* The other problem is that we can't just call
gen6_gt_force_wake_get()
-* because that function calls intel_runtime_pm_get(), which might
sleep.
-* Instead, we do the runtime_pm_get/put when creating/destroying
requests.
-*/
- spin_lock_irqsave(dev_priv-uncore.lock, flags);
- if (dev_priv-uncore.forcewake_count++ == 0)
-
Re: [Intel-gfx] [PATCH] rpm
On Wed, Sep 10, 2014 at 01:57:16PM -0300, Paulo Zanoni wrote: 2014-09-10 13:43 GMT-03:00 Chris Wilson ch...@chris-wilson.co.uk: --- drivers/gpu/drm/i915/i915_debugfs.c | 2 ++ drivers/gpu/drm/i915/intel_display.c | 20 ++ drivers/gpu/drm/i915/intel_lrc.c | 21 ++- drivers/gpu/drm/i915/intel_uncore.c | 52 +++- 4 files changed, 27 insertions(+), 68 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 5f35048..a72d8b8 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inode, struct file *file) if (INTEL_INFO(dev)-gen 6) return 0; + intel_runtime_pm_get(dev_priv); gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); return 0; @@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode *inode, struct file *file) return 0; gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL); + intel_runtime_pm_put(dev_priv); return 0; } Just a minor comment on the chunk above. I didn't look at the rest of the patch. We used to have exactly the code that you rewrote, but we decided to remove it because, without it, we can test that gen6_gt_force_wake_get actually wakes up the HW and keeps it awake until someone calls gen6_gt_force_wake_put. But why? gen6_gt_force_wake_get() should never be called outside of a rpm context - it is lowlevel register access. If we add these get/put calls above, we won't really detect any problems related with the actual gen6_gt_force_wake_get/put functions, so we may hide problems. See above. That's how the design got broken in the first place and we have very lowlevel code being copied and pasted throughout the driver. -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] rpm
On Wed, Sep 10, 2014 at 01:57:16PM -0300, Paulo Zanoni wrote:
2014-09-10 13:43 GMT-03:00 Chris Wilson ch...@chris-wilson.co.uk:
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 ++
drivers/gpu/drm/i915/intel_display.c | 20 ++
drivers/gpu/drm/i915/intel_lrc.c | 21 ++-
drivers/gpu/drm/i915/intel_uncore.c | 52
+++-
4 files changed, 27 insertions(+), 68 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 5f35048..a72d8b8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inode,
struct file *file)
if (INTEL_INFO(dev)-gen 6)
return 0;
+ intel_runtime_pm_get(dev_priv);
gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
return 0;
@@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode
*inode, struct file *file)
return 0;
gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
+ intel_runtime_pm_put(dev_priv);
return 0;
}
Just a minor comment on the chunk above. I didn't look at the rest of the
patch.
We used to have exactly the code that you rewrote, but we decided to
remove it because, without it, we can test that gen6_gt_force_wake_get
actually wakes up the HW and keeps it awake until someone calls
gen6_gt_force_wake_put.
If we add these get/put calls above, we won't really detect any
problems related with the actual gen6_gt_force_wake_get/put functions,
so we may hide problems.
Well, I think the patch is a great. With the rpm stuff killed
from forcewake handling we could actually use the delayed forcewake put
in the register access macros, which is where it might actually do some
good. IIRC that was even the original intention when the timer got added.
In the current form I'm pretty sure the timer is practically useless.
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 794ad8f..fafd202 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7596,7 +7596,6 @@ static void hsw_disable_lcpll(struct drm_i915_private
*dev_priv,
static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
{
uint32_t val;
- unsigned long irqflags;
val = I915_READ(LCPLL_CTL);
@@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct
drm_i915_private *dev_priv)
/*
* Make sure we're not on PC8 state before disabling PC8, otherwise
* we'll hang the machine. To prevent PC8 state, just enable
force_wake.
-*
-* The other problem is that hsw_restore_lcpll() is called as part
of
-* the runtime PM resume sequence, so we can't just call
-* gen6_gt_force_wake_get() because that function calls
-* intel_runtime_pm_get(), and we can't change the runtime PM
refcount
-* while we are on the resume sequence. So to solve this problem we
have
-* to call special forcewake code that doesn't touch runtime PM and
-* doesn't enable the forcewake delayed work.
*/
- spin_lock_irqsave(dev_priv-uncore.lock, irqflags);
- if (dev_priv-uncore.forcewake_count++ == 0)
- dev_priv-uncore.funcs.force_wake_get(dev_priv,
FORCEWAKE_ALL);
- spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags);
+ gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
if (val LCPLL_POWER_DOWN_ALLOW) {
val = ~LCPLL_POWER_DOWN_ALLOW;
@@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct
drm_i915_private *dev_priv)
DRM_ERROR(Switching back to LCPLL failed\n);
}
- /* See the big comment above. */
- spin_lock_irqsave(dev_priv-uncore.lock, irqflags);
- if (--dev_priv-uncore.forcewake_count == 0)
- dev_priv-uncore.funcs.force_wake_put(dev_priv,
FORCEWAKE_ALL);
- spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags);
+ gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
}
/*
diff --git a/drivers/gpu/drm/i915/intel_lrc.c
b/drivers/gpu/drm/i915/intel_lrc.c
index 6f1dd00..aeaa1bc 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -243,7 +243,6 @@ static void execlists_submit_pair(struct
intel_engine_cs *engine,
struct drm_i915_private *dev_priv = engine-i915;
uint64_t tmp;
uint32_t desc[4];
- unsigned long flags;
/* XXX: You must always write both descriptors in the order below.
*/
@@ -260,18 +259,7 @@ static void execlists_submit_pair(struct
intel_engine_cs *engine,
desc[1] = upper_32_bits(tmp);
desc[0] =
Re: [Intel-gfx] [PATCH] rpm
On Wed, Sep 10, 2014 at 05:43:15PM +0100, Chris Wilson wrote:
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 ++
drivers/gpu/drm/i915/intel_display.c | 20 ++
drivers/gpu/drm/i915/intel_lrc.c | 21 ++-
drivers/gpu/drm/i915/intel_uncore.c | 52
+++-
4 files changed, 27 insertions(+), 68 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 5f35048..a72d8b8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4148,6 +4148,7 @@ static int i915_forcewake_open(struct inode *inode,
struct file *file)
if (INTEL_INFO(dev)-gen 6)
return 0;
+ intel_runtime_pm_get(dev_priv);
gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
return 0;
@@ -4162,6 +4163,7 @@ static int i915_forcewake_release(struct inode *inode,
struct file *file)
return 0;
gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
+ intel_runtime_pm_put(dev_priv);
return 0;
}
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 794ad8f..fafd202 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7596,7 +7596,6 @@ static void hsw_disable_lcpll(struct drm_i915_private
*dev_priv,
static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
{
uint32_t val;
- unsigned long irqflags;
val = I915_READ(LCPLL_CTL);
@@ -7607,19 +7606,8 @@ static void hsw_restore_lcpll(struct drm_i915_private
*dev_priv)
/*
* Make sure we're not on PC8 state before disabling PC8, otherwise
* we'll hang the machine. To prevent PC8 state, just enable force_wake.
- *
- * The other problem is that hsw_restore_lcpll() is called as part of
- * the runtime PM resume sequence, so we can't just call
- * gen6_gt_force_wake_get() because that function calls
- * intel_runtime_pm_get(), and we can't change the runtime PM refcount
- * while we are on the resume sequence. So to solve this problem we have
- * to call special forcewake code that doesn't touch runtime PM and
- * doesn't enable the forcewake delayed work.
*/
- spin_lock_irqsave(dev_priv-uncore.lock, irqflags);
- if (dev_priv-uncore.forcewake_count++ == 0)
- dev_priv-uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
- spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags);
+ gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
if (val LCPLL_POWER_DOWN_ALLOW) {
val = ~LCPLL_POWER_DOWN_ALLOW;
@@ -7649,11 +7637,7 @@ static void hsw_restore_lcpll(struct drm_i915_private
*dev_priv)
DRM_ERROR(Switching back to LCPLL failed\n);
}
- /* See the big comment above. */
- spin_lock_irqsave(dev_priv-uncore.lock, irqflags);
- if (--dev_priv-uncore.forcewake_count == 0)
- dev_priv-uncore.funcs.force_wake_put(dev_priv, FORCEWAKE_ALL);
- spin_unlock_irqrestore(dev_priv-uncore.lock, irqflags);
+ gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
}
/*
diff --git a/drivers/gpu/drm/i915/intel_lrc.c
b/drivers/gpu/drm/i915/intel_lrc.c
index 6f1dd00..aeaa1bc 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -243,7 +243,6 @@ static void execlists_submit_pair(struct intel_engine_cs
*engine,
struct drm_i915_private *dev_priv = engine-i915;
uint64_t tmp;
uint32_t desc[4];
- unsigned long flags;
/* XXX: You must always write both descriptors in the order below. */
@@ -260,18 +259,7 @@ static void execlists_submit_pair(struct intel_engine_cs
*engine,
desc[1] = upper_32_bits(tmp);
desc[0] = lower_32_bits(tmp);
- /* Set Force Wakeup bit to prevent GT from entering C6 while ELSP writes
- * are in progress.
- *
- * The other problem is that we can't just call gen6_gt_force_wake_get()
- * because that function calls intel_runtime_pm_get(), which might
sleep.
- * Instead, we do the runtime_pm_get/put when creating/destroying
requests.
- */
- spin_lock_irqsave(dev_priv-uncore.lock, flags);
- if (dev_priv-uncore.forcewake_count++ == 0)
- dev_priv-uncore.funcs.force_wake_get(dev_priv, FORCEWAKE_ALL);
- spin_unlock_irqrestore(dev_priv-uncore.lock, flags);
-
+ gen6_gt_force_wake_get(dev_priv, engine-power_domains);
I915_WRITE(RING_ELSP(engine), desc[1]);
I915_WRITE(RING_ELSP(engine), desc[0]);
I915_WRITE(RING_ELSP(engine), desc[3]);
@@ -280,12 +268,7 @@ static void execlists_submit_pair(struct intel_engine_cs
*engine,
/* ELSP is a wo register, so use another nearby reg for posting instead
*/
POSTING_READ(RING_EXECLIST_STATUS(engine));
-
- /* Release
Re: [Intel-gfx] [PATCH] rpm
On Wed, Sep 10, 2014 at 08:15:47PM +0300, Ville Syrjälä wrote: On Wed, Sep 10, 2014 at 05:43:15PM +0100, Chris Wilson wrote: static void gen6_force_wake_timer(unsigned long arg) Looks like you forgot to kill the rpm_put() from the timer. And then we also need to make sure the timer is approriately cancelled when runtime suspending the device. Or maybe I have that already fixed in my tree so you don't see it in this patch ;-) -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: Clear PCODE_DATA1 on SNB+
On Fri, Sep 05, 2014 at 01:53:12PM +0100, Damien Lespiau wrote:
Ville found out that the DATA1 register exists since SNB with some
scarce apparitions in the specs throughout the times. In his own words:
Also according to Bspec the mailbox data1 register already existed
since snb. The hsw cdclk change sequence also mentions that it should
be set to 0, but eg. the bdw IPS sequence doesn't mention it. I guess
in theory some pcode command might cause it to be clobbered, so I'm
thinking we should just explicitly set it to 0 for all platforms in
the pcode read/write functions
Suggested-by: Ville Syrjälä ville.syrj...@linux.intel.com
Signed-off-by: Damien Lespiau damien.lesp...@intel.com
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_reg.h | 2 +-
drivers/gpu/drm/i915/intel_pm.c | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5a7adb1..56cccde 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5952,8 +5952,8 @@ enum skl_disp_power_wells {
#define GEN6_PCODE_DATA 0x138128
#define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8
#define GEN6_PCODE_FREQ_RING_RATIO_SHIFT 16
+#define GEN6_PCODE_DATA1 0x13812C
-#define GEN9_PCODE_DATA1 0x13812C
#define GEN9_PCODE_READ_MEM_LATENCY0x6
#define GEN9_MEM_LATENCY_LEVEL_MASK0xFF
#define GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT 8
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3f69f9a..7bc8f73 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -8716,8 +8716,7 @@ int sandybridge_pcode_read(struct drm_i915_private
*dev_priv, u8 mbox, u32 *val)
}
I915_WRITE(GEN6_PCODE_DATA, *val);
- if (INTEL_INFO(dev_priv)-gen = 9)
- I915_WRITE(GEN9_PCODE_DATA1, 0);
+ I915_WRITE(GEN6_PCODE_DATA1, 0);
I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
if (wait_for((I915_READ(GEN6_PCODE_MAILBOX) GEN6_PCODE_READY) == 0,
--
1.8.3.1
--
Ville Syrjälä
Intel OTC
___
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: Use anonymous union/struct to save space taken by latency values
On Fri, Sep 05, 2014 at 01:53:11PM +0100, Damien Lespiau wrote:
Suggested-by: Ville Syrjälä ville.syrj...@linux.intel.com
Signed-off-by: Damien Lespiau damien.lesp...@intel.com
I think I'll punt on reviewing this for now. I need to go through
the other skl wm stuff first, and then I probaly need to stare at the
result a bit before I can think about unification.
---
drivers/gpu/drm/i915/i915_drv.h | 38 +-
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0baf7f3..8471d12 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1730,23 +1730,27 @@ struct drm_i915_private {
struct vlv_s0ix_state vlv_s0ix_state;
struct {
- /*
- * Raw watermark latency values:
- * in 0.1us units for WM0,
- * in 0.5us units for WM1+.
- */
- /* primary */
- uint16_t pri_latency[5];
- /* sprite */
- uint16_t spr_latency[5];
- /* cursor */
- uint16_t cur_latency[5];
- /*
- * Raw watermark memory latency values
- * for SKL for all 8 levels
- * in 1us units.
- */
- uint16_t skl_latency[8];
+ union {
+ /*
+ * Raw watermark latency values:
+ * in 0.1us units for WM0,
+ * in 0.5us units for WM1+.
+ */
+ struct {
+ /* primary */
+ uint16_t pri_latency[5];
+ /* sprite */
+ uint16_t spr_latency[5];
+ /* cursor */
+ uint16_t cur_latency[5];
+ };
+
+ /*
+ * Raw watermark memory latency values for SKL for all
+ * 8 levels. In 1us units.
+ */
+ uint16_t skl_latency[8];
+ };
/*
* The skl_wm_values structure is a bit too big for stack
--
1.8.3.1
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 43/89 v6] drm/i915/skl: Read the Memory Latency Values for WM computation
On Thu, Sep 04, 2014 at 07:49:43PM +0100, Damien Lespiau wrote:
From: Pradeep Bhat pradeep.b...@intel.com
This patch reads the memory latency values for all the 8 levels for
SKL. These values are needed for the Watermark computation.
v2: Incorporated the review comments from Damien on register
indentation.
v3: Updated the code to use the sandybridge_pcode_read for reading
memory latencies for GEN9.
v4: Don't put gen 9 in the middle of an ordered list of ifs
(Damien)
v5: take the rps.hw_lock around sandybridge_pcode_read() (Damien)
v6: Use gen = 9 in the pcode_read() function for data1.
Move the defines near the gen6 ones and prefix them with PCODE.
Remove unused timeout define (the pcode_read() code has a larger
timeout already).
Signed-off-by: Pradeep Bhat pradeep.b...@intel.com
Signed-off-by: Damien Lespiau damien.lesp...@intel.com
With the pcode gen6 stuff handled by the followup patch this is:
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 6
drivers/gpu/drm/i915/i915_reg.h | 7
drivers/gpu/drm/i915/intel_pm.c | 76
+
3 files changed, 83 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dcd1c72..32be299 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1665,6 +1665,12 @@ struct drm_i915_private {
uint16_t spr_latency[5];
/* cursor */
uint16_t cur_latency[5];
+ /*
+ * Raw watermark memory latency values
+ * for SKL for all 8 levels
+ * in 1us units.
+ */
+ uint16_t skl_latency[8];
/* current hardware state */
struct ilk_wm_values hw;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0159f2d..6785d51 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5855,6 +5855,13 @@ enum punit_power_well {
#define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8
#define GEN6_PCODE_FREQ_RING_RATIO_SHIFT 16
+#define GEN9_PCODE_DATA1 0x13812C
+#define GEN9_PCODE_READ_MEM_LATENCY0x6
+#define GEN9_MEM_LATENCY_LEVEL_MASK0xFF
+#define GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT 8
+#define GEN9_MEM_LATENCY_LEVEL_2_6_SHIFT 16
+#define GEN9_MEM_LATENCY_LEVEL_3_7_SHIFT 24
+
#define GEN6_GT_CORE_STATUS 0x138060
#define GEN6_CORE_CPD_STATE_MASK (74)
#define GEN6_RCn_MASK 7
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a236e77..2774db1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2239,11 +2239,56 @@ hsw_compute_linetime_wm(struct drm_device *dev,
struct drm_crtc *crtc)
PIPE_WM_LINETIME_TIME(linetime);
}
-static void intel_read_wm_latency(struct drm_device *dev, uint16_t wm[5])
+static void intel_read_wm_latency(struct drm_device *dev, uint16_t wm[8])
{
struct drm_i915_private *dev_priv = dev-dev_private;
- if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+ if (IS_GEN9(dev)) {
+ uint32_t val;
+ int ret;
+
+ /* read the first set of memory latencies[0:3] */
+ val = 0; /* data0 to be programmed to 0 for first set */
+ mutex_lock(dev_priv-rps.hw_lock);
+ ret = sandybridge_pcode_read(dev_priv,
+ GEN9_PCODE_READ_MEM_LATENCY,
+ val);
+ mutex_unlock(dev_priv-rps.hw_lock);
+
+ if (ret) {
+ DRM_ERROR(SKL Mailbox read error = %d\n, ret);
+ return;
+ }
+
+ wm[0] = val GEN9_MEM_LATENCY_LEVEL_MASK;
+ wm[1] = (val GEN9_MEM_LATENCY_LEVEL_1_5_SHIFT)
+ GEN9_MEM_LATENCY_LEVEL_MASK;
+ wm[2] = (val GEN9_MEM_LATENCY_LEVEL_2_6_SHIFT)
+ GEN9_MEM_LATENCY_LEVEL_MASK;
+ wm[3] = (val GEN9_MEM_LATENCY_LEVEL_3_7_SHIFT)
+ GEN9_MEM_LATENCY_LEVEL_MASK;
+
+ /* read the second set of memory latencies[4:7] */
+ val = 1; /* data0 to be programmed to 1 for second set */
+ mutex_lock(dev_priv-rps.hw_lock);
+ ret = sandybridge_pcode_read(dev_priv,
+ GEN9_PCODE_READ_MEM_LATENCY,
+ val);
+ mutex_unlock(dev_priv-rps.hw_lock);
+ if (ret) {
+ DRM_ERROR(SKL Mailbox read error = %d\n, ret);
+ return;
+ }
+
+ wm[4] = val
Re: [Intel-gfx] [PATCH 44/89] drm/i915/skl: Register definitions and macros for SKL Watermark regs
On Thu, Sep 04, 2014 at 12:27:10PM +0100, Damien Lespiau wrote:
From: Pradeep Bhat pradeep.b...@intel.com
This patch defines SKL specific PLANE_WM Watermark registers. It also
defines macros to get the addresses of different LP levels within a pipe.
v2: Reworked the register definitions and associated macros to make it more
generic and be able to use for_each_pipe in values computation.
Incorporated Damien's review comments and indentation.
v3: Added default values for lines and blocks. Provided mask for blocks.
Signed-off-by: Pradeep Bhat pradeep.b...@intel.com
Signed-off-by: Damien Lespiau damien.lesp...@intel.com
---
drivers/gpu/drm/i915/i915_reg.h | 37 +
1 file changed, 37 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index bc55990..9fbce2c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4066,6 +4066,43 @@ enum punit_power_well {
#define I965_CURSOR_MAX_WM 32
#define I965_CURSOR_DFT_WM 8
+/* Watermark register definitions for SKL */
+#define CUR_WM_A_0 0x70140
+#define CUR_WM_B_0 0x71140
+#define PLANE_WM_1_A_0 0x70240
+#define PLANE_WM_1_B_0 0x71240
+#define PLANE_WM_2_A_0 0x70340
+#define PLANE_WM_2_B_0 0x71340
+#define PLANE_WM_TRANS_1_A_0 0x70268
+#define PLANE_WM_TRANS_1_B_0 0x71268
+#define PLANE_WM_TRANS_2_A_0 0x70368
+#define PLANE_WM_TRANS_2_B_0 0x71368
+#define CUR_WM_TRANS_A_0 0x70168
+#define CUR_WM_TRANS_B_0 0x71168
+#define PLANE_WM_EN(1 31)
+#define PLANE_WM_LINES_SHIFT 14
+#define PLANE_WM_LINES_MASK0x1f
+#define PLANE_WM_BLOCKS_MASK 0x3ff
+#define PLANE_WM_LINES_DEFAULT 0x1
+#define PLANE_WM_BLOCKS_DEFAULT0x7
+
These numbers seemed to make sense when I tried to compare with the
spec.
+#define CUR_WM_0(pipe) _PIPE(pipe, CUR_WM_A_0, CUR_WM_B_0)
+#define CUR_WM(pipe, level) (CUR_WM_0(pipe) + ((4) * (level)))
+#define CUR_WM_TRANS(pipe) _PIPE(pipe, CUR_WM_TRANS_A_0, CUR_WM_TRANS_B_0)
+
+#define PLANE_WM_1(pipe) _PIPE(pipe, PLANE_WM_1_A_0, PLANE_WM_1_B_0)
+#define PLANE_WM_2(pipe) _PIPE(pipe, PLANE_WM_2_A_0, PLANE_WM_2_B_0)
+#define PLANE_WM_BASE(pipe, plane) \
+ _PLANE(plane, PLANE_WM_1(pipe), PLANE_WM_2(pipe))
+#define PLANE_WM(pipe, plane, level) \
+ (PLANE_WM_BASE(pipe, plane) + ((4) * (level)))
+#define PLANE_WM_TRANS_1(pipe) \
+ _PIPE(pipe, PLANE_WM_TRANS_1_A_0, PLANE_WM_TRANS_1_B_0)
+#define PLANE_WM_TRANS_2(pipe) \
+ _PIPE(pipe, PLANE_WM_TRANS_2_A_0, PLANE_WM_TRANS_2_B_0)
+#define PLANE_WM_TRANS(pipe, plane) \
+ _PLANE(plane, PLANE_WM_TRANS_1(pipe), PLANE_WM_TRANS_2(pipe))
I must admit to my eyes glazing over when trying to parse these macros.
Not sure if a bit less redirection might help here. Anyway I plugged them
into a small test program and the resulting register offsets were all good.
Just one small nit: if the intermediate macros aren't supposed to be used
by anywhere outside this file then they might be prefixed with _ to make
it clear they're internal.
Either way:
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
+
/* define the Watermark register on Ironlake */
#define WM0_PIPEA_ILK0x45100
#define WM0_PIPE_PLANE_MASK (0x16)
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0/4] Color manager framework
On Wed, Sep 10, 2014 at 04:38:43PM +0530, Sharma, Shashank wrote: Hello Matt, Thanks for your detailed descriptions, reviews comments and time. Please find my comments inline. Regards Shashank On 9/10/2014 6:58 AM, Matt Roper wrote: On Tue, Sep 09, 2014 at 11:53:12AM +0530, shashank.sha...@intel.com wrote: From: Shashank Sharma shashank.sha...@intel.com Color manager is an extention to i915 driver which provides display tuning and color-correction properties to user space, via DRM propery interface.Different Intel platforms support different color tuning capabilities which can be exploited using this framework. This patch set adds color correction for VLV, and the code is written in such a way that new color properties and support for other platforms can be pluggen in, using the same architecture. For the design discussion, right now only one color correction property (CSC) has been added. Once we agree on the design, gamma-correction, contrast, brightness, and hue/saturation would be followed by the same. What this patch set does, is: 1. Color manager framework for valleyview: Add basic functions of framework, to create and destroy DRM properties for color correction. It also adds header, enums and defines. 2. Plug-in color manager attach Attach created color properties, with the objects. Basically properties get attached to two type of objects, crtc (pipe level correction) and plane (plane level correction). 3. CSC color correction First color correction method. - Add color property for CSC during init. - Add blob to keep correction values - Attach DRM CSC propery with each CRTC object - Core CSC correction for VLV 4. Add set_property function for intel CRTC. This function checks the type of the property, and calls the appropriate high level function (intel_clrmgr_set_csc), which does platform check and calls the core platform color correction function (vlv_set_csc) V2: Re-designed based on review comments and suggestions from Matt Roper and Daniel. - Creating only one property per color correction, and attaching it to every DRM object. - No additional data structures on top of DRM property. - No registeration of color property, just create and attach. - Added properties in dev-mode_config Hi Shashank, thanks for incorporating the feedback from the last review round. This patchset is definitely moving in the right direction, although I still feel that there's still a little bit of unnecessary work/complexity in creating a framework here where we probably don't need it. I'll give some more detailed responses on your individual patches, but at a high level I don't really see the need to treat color correction properties (csc, gamma, etc.) in a special way with their own registration framework. There are really three things to consider here: * How/where to create the properties * How/where to attach properties to CRTC's and/or planes * How to handle property changes For creating properties, at the moment you're doing that via intel_modeset_init() - intel_clrmgr_init() - intel_clrmgr_create_color_properties(). Presumably we'll add other (non-color correction) properties to the driver in the future, so it seems like it would be simpler if we just renamed your intel_clrmgr_create_color_properties() to something like intel_modeset_create_properties() and called it directly from intel_modeset_init(). I don't see the intermediate intel_clrmgr_init() adding any value at the moment, so I think it can be removed. I got this point Matt, and we can do this. The only confusion I have is, we already have places where we create properties, and if we are intending to create different type of properties in this function, would that align with the previous property creations places ? At least we have a common ground among the color properties, that's why I am sticking them together. If you dont like this, the other common ground is CRTC/plane properties. These are the first of CRTC or plane properties. So I can create a intel_create_crtc_properties() We do actually have one plane property (rotation) now that just went into the driver. But you're right that we also have connector properties that are handled a bit differently at the moment. There are 'audio' and 'Broadcast RGB' properties that are stored in dev_priv and get created the first time they are attached to a connector. Personally I don't see a problem with moving the creation of those two properties into your new intel_modeset_create_properties() function; it's not like we're using up a ton of memory in cases where we don't have any relevant connectors to attach them to. We also have a whole bunch of SDVO/TV connector properties that are actually stored in the connector itself, which seems a bit off to me. Generally properties get stored in a central location like dev or dev_priv so that they can be shared between multiple objects.
Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Merge of visible and !visible paths for primary planes
On Wed, Sep 10, 2014 at 12:04:18PM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.pado...@collabora.co.uk
Fold intel_pipe_set_base() in the update primary plane path merging
pieces of code that are common to both paths.
Basically the the pin/unpin procedures are the same for both paths
and some checks can also be shared (some of the were moved to the
check() stage)
v2: take Ville's comments:
- remove unecessary plane check
- move mutex lock to inside the conditional
- make the pin fail message a debug one
- add a fixme for the fastboot hack
- call intel_frontbuffer_flip() after FBC update
Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
---
drivers/gpu/drm/i915/intel_display.c | 93
1 file changed, 62 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index b78f00a..1a001bf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11824,12 +11824,23 @@ intel_check_primary_plane(struct drm_plane *plane,
struct drm_rect *dest = state-dst;
struct drm_rect *src = state-src;
const struct drm_rect *clip = state-clip;
+ int ret;
- return drm_plane_helper_check_update(plane, crtc, fb,
+ ret = drm_plane_helper_check_update(plane, crtc, fb,
src, dest, clip,
DRM_PLANE_HELPER_NO_SCALING,
DRM_PLANE_HELPER_NO_SCALING,
false, true, state-visible);
+ if (ret)
+ return ret;
+
+ /* no fb bound */
+ if (state-visible !fb) {
+ DRM_ERROR(No FB bound\n);
+ return -EINVAL;
+ }
+
+ return 0;
}
static int
@@ -11841,14 +11852,34 @@ intel_commit_primary_plane(struct drm_plane *plane,
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ enum pipe pipe = intel_crtc-pipe;
+ struct drm_framebuffer *old_fb = plane-fb;
struct drm_i915_gem_object *obj = intel_fb_obj(fb);
struct drm_i915_gem_object *old_obj = intel_fb_obj(plane-fb);
struct intel_plane *intel_plane = to_intel_plane(plane);
struct drm_rect *src = state-src;
- int ret;
+ int ret = 0;
intel_crtc_wait_for_pending_flips(crtc);
+ if (intel_crtc_has_pending_flip(crtc)) {
+ DRM_ERROR(pipe is still busy with an old pageflip\n);
+ return -EBUSY;
+ }
+
+ if (plane-fb != fb) {
+ mutex_lock(dev-struct_mutex);
+ ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+ if (ret == 0)
+ i915_gem_track_fb(old_obj, obj,
+ INTEL_FRONTBUFFER_PRIMARY(pipe));
+ mutex_unlock(dev-struct_mutex);
+ }
+ if (ret != 0) {
+ DRM_DEBUG_KMS(pin fence failed\n);
+ return ret;
+ }
I'd move this error handling also inside the 'plane-fb != fb' block.
That avoids the ret=0 initialization, and more importantly it will
match what you did in the sprite code. Thanks to your efforts I'm
already dreaming about sharing buffer pinning codes across
different plane types...
+
/*
* If clipping results in a non-visible primary plane, we'll disable
* the primary plane. Note that this is a bit different than what
@@ -11856,33 +11887,9 @@ intel_commit_primary_plane(struct drm_plane *plane,
* because plane-fb still gets set and pinned.
*/
if (!state-visible) {
- mutex_lock(dev-struct_mutex);
-
- /*
- * Try to pin the new fb first so that we can bail out if we
- * fail.
- */
- if (plane-fb != fb) {
- ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
- if (ret) {
- mutex_unlock(dev-struct_mutex);
- return ret;
- }
- }
-
- i915_gem_track_fb(old_obj, obj,
- INTEL_FRONTBUFFER_PRIMARY(intel_crtc-pipe));
-
if (intel_crtc-primary_enabled)
intel_disable_primary_hw_plane(plane, crtc);
-
- if (plane-fb != fb)
- if (plane-fb)
- intel_unpin_fb_obj(old_obj);
-
- mutex_unlock(dev-struct_mutex);
-
} else {
if (intel_crtc intel_crtc-active
intel_crtc-primary_enabled) {
@@ -11902,12 +11909,36 @@ intel_commit_primary_plane(struct drm_plane *plane,
intel_disable_fbc(dev);
[Intel-gfx] [PATCH] drm/i915: Reduce duplicated forcewake logic
Introduce a structure to track the individual forcewake domains and use
that to eliminated duplicate logic.
---
drivers/gpu/drm/i915/i915_debugfs.c | 41 +++---
drivers/gpu/drm/i915/i915_drv.h | 32 +++--
drivers/gpu/drm/i915/intel_uncore.c | 265
3 files changed, 157 insertions(+), 181 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index a72d8b8..c3176f1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1317,7 +1317,6 @@ static int vlv_drpc_info(struct seq_file *m)
struct drm_device *dev = node-minor-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
u32 rpmodectl1, rcctl1;
- unsigned fw_rendercount = 0, fw_mediacount = 0;
intel_runtime_pm_get(dev_priv);
@@ -1350,22 +1349,12 @@ static int vlv_drpc_info(struct seq_file *m)
seq_printf(m, Media RC6 residency since boot: %u\n,
I915_READ(VLV_GT_MEDIA_RC6));
- spin_lock_irq(dev_priv-uncore.lock);
- fw_rendercount = dev_priv-uncore.fw_rendercount;
- fw_mediacount = dev_priv-uncore.fw_mediacount;
- spin_unlock_irq(dev_priv-uncore.lock);
-
- seq_printf(m, Forcewake Render Count = %u\n, fw_rendercount);
- seq_printf(m, Forcewake Media Count = %u\n, fw_mediacount);
-
-
- return 0;
+ return i915_gen6_forcewake_count_info(m, data);
}
static int gen6_drpc_info(struct seq_file *m)
{
-
struct drm_info_node *node = m-private;
struct drm_device *dev = node-minor-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -1379,7 +1368,7 @@ static int gen6_drpc_info(struct seq_file *m)
intel_runtime_pm_get(dev_priv);
spin_lock_irq(dev_priv-uncore.lock);
- forcewake_count = dev_priv-uncore.forcewake_count;
+ forcewake_count =
dev_priv-uncore.fw_domain[FW_DOMAIN_RENDER].wake_count;
spin_unlock_irq(dev_priv-uncore.lock);
if (forcewake_count) {
@@ -1976,21 +1965,23 @@ static int i915_gen6_forcewake_count_info(struct
seq_file *m, void *data)
struct drm_info_node *node = m-private;
struct drm_device *dev = node-minor-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
- unsigned forcewake_count = 0, fw_rendercount = 0, fw_mediacount = 0;
+ const char *domain_names[] = {
+ render,
+ media,
+ };
+ int i;
spin_lock_irq(dev_priv-uncore.lock);
- if (IS_VALLEYVIEW(dev)) {
- fw_rendercount = dev_priv-uncore.fw_rendercount;
- fw_mediacount = dev_priv-uncore.fw_mediacount;
- } else
- forcewake_count = dev_priv-uncore.forcewake_count;
- spin_unlock_irq(dev_priv-uncore.lock);
+ for (i = 0; i FW_DOMAIN_COUNT; i++) {
+ if ((dev_priv-uncore.fw_domains (1 i)) == 0)
+ continue;
- if (IS_VALLEYVIEW(dev)) {
- seq_printf(m, fw_rendercount = %u\n, fw_rendercount);
- seq_printf(m, fw_mediacount = %u\n, fw_mediacount);
- } else
- seq_printf(m, forcewake count = %u\n, forcewake_count);
+ seq_printf(m, %s.wake_count = %u\n,
+ domain_names[i],
+ dev_priv-uncore.fw_domain[i].wake_count);
+ }
+
+ spin_unlock_irq(dev_priv-uncore.lock);
return 0;
}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5d0b38c..6424191 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -528,18 +528,30 @@ struct intel_uncore_funcs {
uint64_t val, bool trace);
};
+enum {
+ FW_DOMAIN_RENDER = 0,
+ FW_DOMAIN_MEDIA,
+
+ FW_DOMAIN_COUNT
+};
+
struct intel_uncore {
spinlock_t lock; /** lock is also taken in irq contexts. */
struct intel_uncore_funcs funcs;
unsigned fifo_count;
- unsigned forcewake_count;
-
- unsigned fw_rendercount;
- unsigned fw_mediacount;
+ unsigned fw_domains;
- struct timer_list force_wake_timer;
+ struct intel_uncore_forcewake_domain {
+ struct drm_i915_private *i915;
+ int id;
+ unsigned wake_count;
+ struct timer_list timer;
+ } fw_domain[FW_DOMAIN_COUNT];
+#define FORCEWAKE_RENDER (1 FW_DOMAIN_RENDER)
+#define FORCEWAKE_MEDIA(1 FW_DOMAIN_MEDIA)
+#define FORCEWAKE_ALL (FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
};
#define DEV_INFO_FOR_EACH_FLAG(func, sep) \
@@ -2948,8 +2960,10 @@ extern void intel_display_print_error_state(struct
drm_i915_error_state_buf *e,
* must be set to prevent GT core from power down and stale values being
* returned.
*/
-void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv, int fw_engine);
-void
Re: [Intel-gfx] [PATCH 00/16] fix VLV S4 suspend/resume, unify S3/S4 handlers
On Wed, 2014-09-10 at 17:52 +0200, Daniel Vetter wrote: On Wed, Sep 10, 2014 at 06:16:53PM +0300, Imre Deak wrote: The first part of the patchset (1-6) fixes an S4 bug on VLV introduced recently. The rest unifies the various S3/S4 handlers, which are in practice the same. The only real difference - besides some unused code - is that during S3 suspend we disable the PCI device whereas across an S4 freeze/thaw we keep it enabled. Given that we disable everything else anyway, we can just as well disable the PCI device there too. Doing so allows us to handle S3 suspend/resume and S4 freeze/thaw/restore/ power-off the same way, simplifying/clarifying things quite a bit. Hm, this might explain why we've seen random corruption on S4 on recent platforms. https://bugzilla.kernel.org/show_bug.cgi?id=59321 Can you please ask for retesting from reporters? Ok, can do, I also forgot to add https://bugs.freedesktop.org/show_bug.cgi?id=82842 which it fixes. I can't see immediately how platforms other than VLV would be fixed with these, but maybe I missed something. --Imre Thanks, Daniel I smoke tested this on BDW, HSW, VLV (BYT-M), IVB, GEN45. Imre Deak (16): drm/i915: vlv: fix gunit HW state corruption during S4 suspend drm/i915: remove dead code from legacy suspend handler drm/i915: factor out i915_drm_suspend_late drm/i915: unify legacy S3 suspend and S4 freeze handlers drm/i915: propagate error from legacy resume handler drm/i915: vlv: fix switcheroo/legacy suspend/resume drm/i915: fix S4 suspend while switcheroo state is off drm/i915: remove unused restore_gtt_mappings optimization during suspend drm/i915: check for GT faults during S3 resume and S4 restore too drm/i915: enable output polling during S4 thaw drm/i915: disable/re-enable PCI device around S4 freeze/thaw drm/i915: unify S3 and S4 suspend/resume handlers drm/i915: sanitize suspend/resume helper function names drm/i915: add poweroff_late handler drm/i915: unify switcheroo and legacy suspend/resume handlers drm/i915: add comments on what stage a given PM handler is called drivers/gpu/drm/i915/i915_dma.c | 4 +- drivers/gpu/drm/i915/i915_drv.c | 185 +++- drivers/gpu/drm/i915/i915_drv.h | 4 +- 3 files changed, 74 insertions(+), 119 deletions(-) -- 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
Re: [Intel-gfx] [PATCH 45/89] drm/i915/skl: Definition of SKL WM param structs for pipe/plane
On Thu, Sep 04, 2014 at 12:27:11PM +0100, Damien Lespiau wrote:
From: Pradeep Bhat pradeep.b...@intel.com
This patch defines the structures needed for computation of
watermarks of pipes and planes for SKL.
v2: Incorporated Damien's review comments and removed unused fields
in structs for future features like rotation, drrs and scaling.
The skl_wm_values struct is now made more generic across planes
and cursor planes for all pipes.
v3: implemented the plane/cursor split.
Signed-off-by: Pradeep Bhat pradeep.b...@intel.com
Signed-off-by: Damien Lespiau damien.lesp...@intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 18 ++
drivers/gpu/drm/i915/intel_drv.h | 10 +-
drivers/gpu/drm/i915/intel_pm.c | 8
3 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 32be299..3764ad5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1381,6 +1381,24 @@ struct ilk_wm_values {
enum intel_ddb_partitioning partitioning;
};
+struct skl_wm_values {
+ bool dirty[I915_MAX_PIPES];
+ uint32_t wm_linetime[I915_MAX_PIPES];
+ uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
+ uint32_t cursor[I915_MAX_PIPES][8];
+ uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
+ uint32_t cursor_trans[I915_MAX_PIPES];
+};
These multi dimensional arrays hurt my eyes. Maybe we should restructure
this a bit to eg:
struct skl_wm_values {
struct {
wm_linetime;
plane[MAX_PLANES][8];
...
} pipe[MAX_PIPES];
};
The two dimensional plane[][] array is still a bit nasty, but maybe we
can live with it.
We could also do the same operatiob for the ilk version to keep stuff
similar.
+
+struct skl_wm_level {
+ bool plane_en[I915_MAX_PLANES];
+ uint16_t plane_res_b[I915_MAX_PLANES];
+ uint8_t plane_res_l[I915_MAX_PLANES];
This stuff could also look better as an array of struct of some sort.
Also should probably put the bool and uint8_t next to each other in case
gcc is smart enough to pack things more tightly.
+ bool cursor_en;
+ uint16_t cursor_res_b;
+ uint8_t cursor_res_l;
And this could also be an instance of the same struct use for the proper
planes.
I'm actually wondering if we want this cursor stuff around at all.
I was under the impression using the cursor eats one of the planes,
so we could just as well use the plane always. But if the current
code actually implements stuff using the legacy cursor thing I guess we
need it for now at least.
+};
+
/*
* This struct helps tracking the state needed for runtime PM, which puts the
* device in PCI D3 state. Notice that when this happens, nothing on the
diff --git a/drivers/gpu/drm/i915/intel_drv.h
b/drivers/gpu/drm/i915/intel_drv.h
index 3239e58..268087f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -387,6 +387,12 @@ struct intel_mmio_flip {
u32 ring_id;
};
+struct skl_pipe_wm {
+ struct skl_wm_level wm[8];
+ struct skl_wm_level trans_wm;
+ uint32_t linetime;
+};
+
struct intel_crtc {
struct drm_crtc base;
enum pipe pipe;
@@ -431,9 +437,11 @@ struct intel_crtc {
bool pch_fifo_underrun_disabled;
/* per-pipe watermark state */
- struct {
+ union {
/* watermarks currently being used */
struct intel_pipe_wm active;
+ /* SKL wm values currently in use */
+ struct skl_pipe_wm skl_active;
} wm;
int scanline_offset;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d8c8531..2503ab9 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1928,6 +1928,14 @@ static uint32_t ilk_wm_fbc(uint32_t pri_val, uint32_t
horiz_pixels,
return DIV_ROUND_UP(pri_val * 64, horiz_pixels * bytes_per_pixel) + 2;
}
+struct skl_pipe_wm_parameters {
+ bool active;
+ uint32_t pipe_htotal;
+ uint32_t pixel_rate; /* in KHz */
+ struct intel_plane_wm_parameters plane[I915_MAX_PLANES];
+ struct intel_plane_wm_parameters cursor;
+};
I suppose we just need to start using some kind of named indexes for the
planes on all platforms so we can unify all this stuff. But that can be
done when we have all the code merged so we can better see how to unify
things.
+
struct ilk_pipe_wm_parameters {
bool active;
uint32_t pipe_htotal;
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
Re: [Intel-gfx] [PATCH v2 2/2] drm/i915: Merge of visible and !visible paths for primary planes
On Wed, Sep 10, 2014 at 09:25:29PM +0300, Ville Syrjälä wrote:
On Wed, Sep 10, 2014 at 12:04:18PM -0300, Gustavo Padovan wrote:
From: Gustavo Padovan gustavo.pado...@collabora.co.uk
Fold intel_pipe_set_base() in the update primary plane path merging
pieces of code that are common to both paths.
Basically the the pin/unpin procedures are the same for both paths
and some checks can also be shared (some of the were moved to the
check() stage)
v2: take Ville's comments:
- remove unecessary plane check
- move mutex lock to inside the conditional
- make the pin fail message a debug one
- add a fixme for the fastboot hack
- call intel_frontbuffer_flip() after FBC update
Signed-off-by: Gustavo Padovan gustavo.pado...@collabora.co.uk
---
drivers/gpu/drm/i915/intel_display.c | 93
1 file changed, 62 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index b78f00a..1a001bf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11824,12 +11824,23 @@ intel_check_primary_plane(struct drm_plane *plane,
struct drm_rect *dest = state-dst;
struct drm_rect *src = state-src;
const struct drm_rect *clip = state-clip;
+ int ret;
- return drm_plane_helper_check_update(plane, crtc, fb,
+ ret = drm_plane_helper_check_update(plane, crtc, fb,
src, dest, clip,
DRM_PLANE_HELPER_NO_SCALING,
DRM_PLANE_HELPER_NO_SCALING,
false, true, state-visible);
+ if (ret)
+ return ret;
+
+ /* no fb bound */
+ if (state-visible !fb) {
+ DRM_ERROR(No FB bound\n);
+ return -EINVAL;
+ }
+
+ return 0;
}
static int
@@ -11841,14 +11852,34 @@ intel_commit_primary_plane(struct drm_plane
*plane,
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ enum pipe pipe = intel_crtc-pipe;
+ struct drm_framebuffer *old_fb = plane-fb;
struct drm_i915_gem_object *obj = intel_fb_obj(fb);
struct drm_i915_gem_object *old_obj = intel_fb_obj(plane-fb);
struct intel_plane *intel_plane = to_intel_plane(plane);
struct drm_rect *src = state-src;
- int ret;
+ int ret = 0;
intel_crtc_wait_for_pending_flips(crtc);
+ if (intel_crtc_has_pending_flip(crtc)) {
+ DRM_ERROR(pipe is still busy with an old pageflip\n);
+ return -EBUSY;
+ }
+
+ if (plane-fb != fb) {
+ mutex_lock(dev-struct_mutex);
+ ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+ if (ret == 0)
+ i915_gem_track_fb(old_obj, obj,
+ INTEL_FRONTBUFFER_PRIMARY(pipe));
+ mutex_unlock(dev-struct_mutex);
+ }
+ if (ret != 0) {
+ DRM_DEBUG_KMS(pin fence failed\n);
+ return ret;
+ }
I'd move this error handling also inside the 'plane-fb != fb' block.
That avoids the ret=0 initialization, and more importantly it will
match what you did in the sprite code. Thanks to your efforts I'm
already dreaming about sharing buffer pinning codes across
different plane types...
+
/*
* If clipping results in a non-visible primary plane, we'll disable
* the primary plane. Note that this is a bit different than what
@@ -11856,33 +11887,9 @@ intel_commit_primary_plane(struct drm_plane *plane,
* because plane-fb still gets set and pinned.
*/
if (!state-visible) {
- mutex_lock(dev-struct_mutex);
-
- /*
-* Try to pin the new fb first so that we can bail out if we
-* fail.
-*/
- if (plane-fb != fb) {
- ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
- if (ret) {
- mutex_unlock(dev-struct_mutex);
- return ret;
- }
- }
-
- i915_gem_track_fb(old_obj, obj,
- INTEL_FRONTBUFFER_PRIMARY(intel_crtc-pipe));
-
if (intel_crtc-primary_enabled)
intel_disable_primary_hw_plane(plane, crtc);
-
- if (plane-fb != fb)
- if (plane-fb)
- intel_unpin_fb_obj(old_obj);
-
- mutex_unlock(dev-struct_mutex);
-
} else {
if (intel_crtc intel_crtc-active
intel_crtc-primary_enabled) {
@@ -11902,12 +11909,36 @@ intel_commit_primary_plane(struct drm_plane
*plane,
[Intel-gfx] [PATCH 1/2] drm/i915: Remove dead code, i915_gem_verify_gtt
The data structure it was supposed to be sanity checking has long gone.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
drivers/gpu/drm/i915/i915_gem.c | 42 -
1 file changed, 42 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 06866bd205df..abb2b2ed9c4c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3040,46 +3040,6 @@ static bool i915_gem_valid_gtt_space(struct drm_device
*dev,
return true;
}
-static void i915_gem_verify_gtt(struct drm_device *dev)
-{
-#if WATCH_GTT
- struct drm_i915_private *dev_priv = dev-dev_private;
- struct drm_i915_gem_object *obj;
- int err = 0;
-
- list_for_each_entry(obj, dev_priv-mm.gtt_list, global_list) {
- if (obj-gtt_space == NULL) {
- printk(KERN_ERR object found on GTT list with no space
reserved\n);
- err++;
- continue;
- }
-
- if (obj-cache_level != obj-gtt_space-color) {
- printk(KERN_ERR object reserved space [%08lx, %08lx]
with wrong color, cache_level=%x, color=%lx\n,
- i915_gem_obj_ggtt_offset(obj),
- i915_gem_obj_ggtt_offset(obj) +
i915_gem_obj_ggtt_size(obj),
- obj-cache_level,
- obj-gtt_space-color);
- err++;
- continue;
- }
-
- if (!i915_gem_valid_gtt_space(dev,
- obj-gtt_space,
- obj-cache_level)) {
- printk(KERN_ERR invalid GTT space found at [%08lx,
%08lx] - color=%x\n,
- i915_gem_obj_ggtt_offset(obj),
- i915_gem_obj_ggtt_offset(obj) +
i915_gem_obj_ggtt_size(obj),
- obj-cache_level);
- err++;
- continue;
- }
- }
-
- WARN_ON(err);
-#endif
-}
-
/**
* Finds free space in the GTT aperture and binds the object there.
*/
@@ -3217,7 +3177,6 @@ search_free:
vma-bind_vma(vma, obj-cache_level,
flags (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
- i915_gem_verify_gtt(dev);
return vma;
err_remove_node:
@@ -3450,7 +3409,6 @@ int i915_gem_object_set_cache_level(struct
drm_i915_gem_object *obj,
old_write_domain);
}
- i915_gem_verify_gtt(dev);
return 0;
}
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Match GTT space sanity checker with implementation
If we believe that the device can cross cache domains in its prefetcher
(i.e. we allow neighbouring pages in different domains), we don't supply
a color_adjust callback. Use the presence of this callback to better
determine when we should be verifying that the GTT space we just
used is valid.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
---
drivers/gpu/drm/i915/i915_gem.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index abb2b2ed9c4c..55e3959a0b76 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3011,16 +3011,17 @@ i915_gem_object_get_fence(struct drm_i915_gem_object
*obj)
}
static bool i915_gem_valid_gtt_space(struct drm_device *dev,
-struct drm_mm_node *gtt_space,
+struct i915_vma *vma,
unsigned long cache_level)
{
+ struct drm_mm_node *gtt_space = vma-node;
struct drm_mm_node *other;
/* On non-LLC machines we have to be careful when putting differing
* types of snoopable memory together to avoid the prefetcher
* crossing memory domains and dying.
*/
- if (HAS_LLC(dev))
+ if (vma-vm-mm.color_adjust == NULL)
return true;
if (!drm_mm_node_allocated(gtt_space))
@@ -3146,8 +3147,7 @@ search_free:
goto err_free_vma;
}
}
- if (WARN_ON(!i915_gem_valid_gtt_space(dev, vma-node,
- obj-cache_level))) {
+ if (WARN_ON(!i915_gem_valid_gtt_space(dev, vma, obj-cache_level))) {
ret = -EINVAL;
goto err_remove_node;
}
@@ -3353,7 +3353,7 @@ int i915_gem_object_set_cache_level(struct
drm_i915_gem_object *obj,
}
list_for_each_entry_safe(vma, next, obj-vma_list, vma_link) {
- if (!i915_gem_valid_gtt_space(dev, vma-node, cache_level)) {
+ if (!i915_gem_valid_gtt_space(dev, vma, cache_level)) {
ret = i915_vma_unbind(vma);
if (ret)
return ret;
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/2 xf86-video-intel] Two DRI3/Present bug fixes for UXA
Here are a couple of small bug fixes which make DRI3/Present work better with UXA. [PATCH 1/2] Do not clear pending kernel events on mode switch This patch prevents GL-based compositing managers from wedging when performing video mode setting. The problem was that DIX was never receiving notification about page flips being completed when one was pending across a mode switch. [PATCH 2/2] Correct BO allocation alignment This patch makes UXA and Mesa agree about how buffers are allocated for images. Without this, UXA was requiring larger padding, which meant that converting some textures into pixmaps using DRI3 would fail. -keith ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] Correct BO allocation alignment
BO allocations for pixmaps must be aligned to the tile height, but at some point the code was changed to align them to twice the tile height. This overallocates pixmaps, wasting memory, but more importantly, for buffers allocated by DRM and shared through DRI3, the stricter alignment check causes sharing to fail. From reading through the history of the code and related bugs, it seems like this change was part of a set of changes trying to address what turned out to be a kernel regression. Reverting this change solves the DRI3 problem and saves a bit of memory for pixmap allocations. Signed-off-by: Keith Packard kei...@keithp.com --- src/uxa/intel_uxa.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/uxa/intel_uxa.c b/src/uxa/intel_uxa.c index d33eca5..4ce6eae 100644 --- a/src/uxa/intel_uxa.c +++ b/src/uxa/intel_uxa.c @@ -206,7 +206,7 @@ intel_uxa_compute_size(struct intel_screen_private *intel, tile_height = 8; else tile_height = 32; - aligned_h = ALIGN(h, 2*tile_height); + aligned_h = ALIGN(h, tile_height); *stride = intel_get_fence_pitch(intel, ALIGN(pitch, 512), @@ -768,7 +768,7 @@ free_priv: else height = 32; - height = ALIGN(pixmap-drawable.height, 2*height); + height = ALIGN(pixmap-drawable.height, height); size = intel_get_fence_size(intel, priv-stride * height); } else size = priv-stride * pixmap-drawable.height; -- 2.0.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] Do not clear pending kernel events on mode switch
Let the kernel send these back to us so that DIX hears about them in
the usual way.
Mode setting while Present has a flip active will trigger an unflip
before the mode is changed. The event from that unflip will not get
processed before the mode switch is executed. Clearing the driver
queue at mode switch time will discard the connection between the
kernel event and the present callback so that DIX will never know that
the flip pixmap is idle.
Signed-off-by: Keith Packard kei...@keithp.com
---
src/uxa/intel_display.c | 4
1 file changed, 4 deletions(-)
diff --git a/src/uxa/intel_display.c b/src/uxa/intel_display.c
index 97af76d..8c43ae9 100644
--- a/src/uxa/intel_display.c
+++ b/src/uxa/intel_display.c
@@ -71,9 +71,6 @@ struct intel_drm_queue {
intel_drm_abort_proc abort;
};
-static void
-intel_drm_abort_scrn(ScrnInfoPtr scrn);
-
static uint32_t intel_drm_seq;
static struct list intel_drm_queue;
@@ -398,7 +395,6 @@ intel_crtc_apply(xf86CrtcPtr crtc)
if (scrn-pScreen)
xf86_reload_cursors(scrn-pScreen);
-intel_drm_abort_scrn(scrn);
done:
free(output_ids);
--
2.0.1
___
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: Color manager framework for valleyview
On Wed, Sep 10, 2014 at 04:50:56PM +0530, Sharma, Shashank wrote:
...
+
+/* Properties */
+enum clrmgr_tweaks {
+ csc = 0,
+ gamma,
+ contrast,
+ brightness,
+ hue_saturation,
+ clrmgr_tweak_invalid
+};
These are just enums into your property name array, right? I'm not
sure if we need these either.
Actually my plan was like this:
One enum(like this), which assigns color property id to each property.
Arrays, arranged in order of this enum for sizes, name and
init_values of these properties. So it would be easy to access, easy
to plug in new property, and less hard coding.
+/* Properties */
enum clrmgr_tweaks {
csc = 0,
gamma,
contrast,
brightness,
hue_saturation,
clrmgr_tweak_invalid
};
array color_prop_sizes{size_csc, size_gamma, size_cont, size_bright};
array color_prop_name{csc, gamma, cont, bright};
array init_values{{9 init values for CSC} {129 init values for
gamma} {1 default value of contrast} {1 default val for brightness}}
Does it look that bad :) ?
Okay, I think I see what you were going for, but I'm still not convinced
that pulling these values out into separate arrays is more clear at the
moment. You still have to make sure your arrays stay in sync and if
there's any control flow (e.g., some properties are only relevant on
certain platforms), then that gets more complicated as well.
I'd suggest not using the arrays for now while we only have a handful of
properties. Once we have a large list of properties in the driver maybe
we can revisit whether there's a nicer way to stick them in a table and
simplify the code.
Matt
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: CSC color correction
On Tue, Sep 09, 2014 at 11:53:15AM +0530, shashank.sha...@intel.com wrote: From: Shashank Sharma shashank.sha...@intel.com This patch adds support for CSC correction color property. It does the following: 1. Creates a new DRM property for CSC correction. Adds this into mode_config. ... One thing I forgot to ask in my response yesterday...does it make sense to create this property in dev-mode_config rather than in dev_priv? Some properties (like the plane rotation property that went in a little while ago) do seem like a good match for dev-mode_config because they're generic concepts that a lot of different drivers will want to reuse (and in some cases the common core/helper library code might want to make use of them as well, as we do in restore_fbdev_mode()). But it's unclear to me whether non-Intel hardware handles color correction similarly enough that the CSC property you're creating here is something other drivers will ever use, or whether it will always be an Intel-specific thing. My understanding is that the values here are very Intel-specific, so there's never going to be an opportunity for core/helper library code to make use of the property, right? If this does turn out to be an Intel-only property, then it's probably better to move it to dev_priv (where we have a couple of our connector properties today). If you do decide that there's potential for reuse by other drivers and that you want to keep this in dev-mode_config you'll want to make sure you Cc the dri-devel mailing list on the patches that touch the DRM core files. Matt -- Matt Roper Graphics Software Engineer IoTG Platform Enabling Development Intel Corporation (916) 356-2795 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
