Re: [Intel-gfx] [PATCH] drm/i915: Disable primary plane if we fail to reconstruct BIOS fb
On Mon, Nov 23, 2015 at 04:10:58PM -0800, Matt Roper wrote:
> On Mon, Nov 23, 2015 at 04:53:55PM +, Chris Wilson wrote:
> > On Mon, Nov 23, 2015 at 08:48:32AM -0800, Matt Roper wrote:
> > > If we fail to reconstruct the BIOS fb (e.g., because the FB is too
> > > large), we'll be left with plane state that indicates the primary plane
> > > is visible yet has a NULL fb. This mismatch causes problems later on
> > > (e.g., for the watermark code). Since we've failed to reconstruct the
> > > BIOS FB, the best solution is to just disable the primary plane and
> > > pretend the BIOS never had it enabled.
> > >
> > > Cc: Daniel Vetter
> > > Cc: Ville Syrjälä
> > > Signed-off-by: Matt Roper
> >
> > There are a bunch of bugzillas open about this as it is a regression
> > from about 3.2.
> > -Chris
> >
>
> Do you happen to know any of the bug ID's off the top of your head (or
> keywords to look for)? I skimmed through the bug list, but the only one
> that jumped out at me as possibly related is
>
> https://bugs.freedesktop.org/show_bug.cgi?id=91751
> ("Display still active even though primary plane is disabled and kills
> the GPU")
Errors like https://bugs.freedesktop.org/show_bug.cgi?id=89319 or GPU
hangs on takeover like
https://bugs.freedesktop.org/show_bug.cgi?id=87677
https://bugs.freedesktop.org/show_bug.cgi?id=89146
https://bugs.freedesktop.org/show_bug.cgi?id=91653
from a quick search
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/armada: Use a private mutex to protect priv->linear
Reusing the Big DRM Lock just leaks, and the few things left that
dev->struct_mutex protected are very well contained - it's just the
linear drm_mm manager.
With this armada is completely struct_mutex free!
v2: Convert things properly and also take the lock in
armage_gem_free_object, and remove the stale comment (Russell).
Cc: Russell King
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/armada/armada_debugfs.c | 4 ++--
drivers/gpu/drm/armada/armada_drm.h | 3 ++-
drivers/gpu/drm/armada/armada_drv.c | 1 +
drivers/gpu/drm/armada/armada_gem.c | 14 +-
4 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_debugfs.c
b/drivers/gpu/drm/armada/armada_debugfs.c
index 471e45627f1e..d4f7ab0a30d4 100644
--- a/drivers/gpu/drm/armada/armada_debugfs.c
+++ b/drivers/gpu/drm/armada/armada_debugfs.c
@@ -21,9 +21,9 @@ static int armada_debugfs_gem_linear_show(struct seq_file *m,
void *data)
struct armada_private *priv = dev->dev_private;
int ret;
- mutex_lock(&dev->struct_mutex);
+ mutex_lock(&priv->linear_lock);
ret = drm_mm_dump_table(m, &priv->linear);
- mutex_unlock(&dev->struct_mutex);
+ mutex_unlock(&priv->linear_lock);
return ret;
}
diff --git a/drivers/gpu/drm/armada/armada_drm.h
b/drivers/gpu/drm/armada/armada_drm.h
index 4df6f2af2b21..3b2bb6128d40 100644
--- a/drivers/gpu/drm/armada/armada_drm.h
+++ b/drivers/gpu/drm/armada/armada_drm.h
@@ -57,7 +57,8 @@ struct armada_private {
DECLARE_KFIFO(fb_unref, struct drm_framebuffer *, 8);
struct drm_fb_helper*fbdev;
struct armada_crtc *dcrtc[2];
- struct drm_mm linear;
+ struct drm_mm linear; /* protected by linear_lock */
+ struct mutexlinear_lock;
struct drm_property *csc_yuv_prop;
struct drm_property *csc_rgb_prop;
struct drm_property *colorkey_prop;
diff --git a/drivers/gpu/drm/armada/armada_drv.c
b/drivers/gpu/drm/armada/armada_drv.c
index 77ab93d60125..3bd7e1cde99e 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -102,6 +102,7 @@ static int armada_drm_load(struct drm_device *dev, unsigned
long flags)
dev->mode_config.preferred_depth = 24;
dev->mode_config.funcs = &armada_drm_mode_config_funcs;
drm_mm_init(&priv->linear, mem->start, resource_size(mem));
+ mutex_init(&priv->linear_lock);
ret = component_bind_all(dev->dev, dev);
if (ret)
diff --git a/drivers/gpu/drm/armada/armada_gem.c
b/drivers/gpu/drm/armada/armada_gem.c
index e3a86b96af2a..6e731db31aa4 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -46,22 +46,26 @@ static size_t roundup_gem_size(size_t size)
return roundup(size, PAGE_SIZE);
}
-/* dev->struct_mutex is held here */
void armada_gem_free_object(struct drm_gem_object *obj)
{
struct armada_gem_object *dobj = drm_to_armada_gem(obj);
+ struct armada_private *priv = obj->dev->dev_private;
DRM_DEBUG_DRIVER("release obj %p\n", dobj);
drm_gem_free_mmap_offset(&dobj->obj);
+ might_lock(&priv->linear_lock);
+
if (dobj->page) {
/* page backed memory */
unsigned int order = get_order(dobj->obj.size);
__free_pages(dobj->page, order);
} else if (dobj->linear) {
/* linear backed memory */
+ mutex_lock(&priv->linear_lock);
drm_mm_remove_node(dobj->linear);
+ mutex_unlock(&priv->linear_lock);
kfree(dobj->linear);
if (dobj->addr)
iounmap(dobj->addr);
@@ -144,10 +148,10 @@ armada_gem_linear_back(struct drm_device *dev, struct
armada_gem_object *obj)
if (!node)
return -ENOSPC;
- mutex_lock(&dev->struct_mutex);
+ mutex_lock(&priv->linear_lock);
ret = drm_mm_insert_node(&priv->linear, node, size, align,
DRM_MM_SEARCH_DEFAULT);
- mutex_unlock(&dev->struct_mutex);
+ mutex_unlock(&priv->linear_lock);
if (ret) {
kfree(node);
return ret;
@@ -158,9 +162,9 @@ armada_gem_linear_back(struct drm_device *dev, struct
armada_gem_object *obj)
/* Ensure that the memory we're returning is cleared. */
ptr = ioremap_wc(obj->linear->start, size);
if (!ptr) {
- mutex_lock(&dev->struct_mutex);
+ mutex_lock(&priv->linear_lock);
drm_mm_remove_node(obj->linear);
- mutex_unlock(&dev->struct_mutex);
+ mutex_unlock(&priv->linear_lock);
kfree(obj->linear);
o
[Intel-gfx] [PATCH 1/9] drm/i915: Set connector_state->connector correctly.
I can't believe we didn't trip over this sooner.. Signed-off-by: Maarten Lankhorst --- drivers/gpu/drm/i915/intel_display.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2917fef33f31..dcc7ec7665c2 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6461,6 +6461,8 @@ int intel_connector_init(struct intel_connector *connector) if (!connector_state) return -ENOMEM; + connector_state->connector = &connector->base; + connector->base.state = connector_state; 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/9] drm/tegra: Assign conn_state->connector when allocating connector state.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/tegra/dsi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index f0a138ef68ce..6b8ae3d08eeb 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -751,8 +751,10 @@ static void tegra_dsi_connector_reset(struct drm_connector
*connector)
connector->state = NULL;
state = kzalloc(sizeof(*state), GFP_KERNEL);
- if (state)
+ if (state) {
+ state->base.connector = connector;
connector->state = &state->base;
+ }
}
static struct drm_connector_state *
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/9] drm/core: Add drm_for_each_encoder_mask.
Signed-off-by: Maarten Lankhorst
---
include/drm/drm_crtc.h | 11 +++
1 file changed, 11 insertions(+)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 29cfb4f8f99d..c54da2d297ec 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1172,6 +1172,17 @@ struct drm_mode_config {
list_for_each_entry((plane), &(dev)->mode_config.plane_list, head) \
if ((plane_mask) & (1 << drm_plane_index(plane)))
+/**
+ * drm_for_each_encoder_mask - iterate over encoders specified by bitmask
+ * @encoder: the loop cursor
+ * @dev: the DRM device
+ * @encoder_mask: bitmask of encoder indices
+ *
+ * Iterate over all encoders specified by bitmask.
+ */
+#define drm_for_each_encoder_mask(encoder, dev, encoder_mask) \
+ list_for_each_entry((encoder), &(dev)->mode_config.encoder_list, head) \
+ if ((encoder_mask) & (1 << drm_encoder_index(encoder)))
#define obj_to_crtc(x) container_of(x, struct drm_crtc, base)
#define obj_to_connector(x) container_of(x, struct drm_connector, base)
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/9] drm/core: Add drm_encoder_index.
This is useful for adding encoder_mask in crtc_state.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/drm_crtc.c | 23 +++
include/drm/drm_crtc.h | 1 +
2 files changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 32dd134700bd..904a34c560f4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1121,6 +1121,29 @@ out_unlock:
EXPORT_SYMBOL(drm_encoder_init);
/**
+ * drm_encoder_index - find the index of a registered encoder
+ * @encoder: encoder to find index for
+ *
+ * Given a registered encoder, return the index of that encoder within a DRM
+ * device's list of encoders.
+ */
+unsigned int drm_encoder_index(struct drm_encoder *encoder)
+{
+ unsigned int index = 0;
+ struct drm_encoder *tmp;
+
+ drm_for_each_encoder(tmp, encoder->dev) {
+ if (tmp == encoder)
+ return index;
+
+ index++;
+ }
+
+ BUG();
+}
+EXPORT_SYMBOL(drm_encoder_index);
+
+/**
* drm_encoder_cleanup - cleans up an initialised encoder
* @encoder: encoder to cleanup
*
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 173535a35d65..29cfb4f8f99d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1241,6 +1241,7 @@ extern int drm_encoder_init(struct drm_device *dev,
struct drm_encoder *encoder,
const struct drm_encoder_funcs *funcs,
int encoder_type);
+extern unsigned int drm_encoder_index(struct drm_encoder *encoder);
/**
* drm_encoder_crtc_ok - can a given crtc drive a given encoder?
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 7/9] drm/atomic: Small documentation fix.
Use the correct function name for drm_atomic_clean_old_fb docs. Signed-off-by: Maarten Lankhorst --- drivers/gpu/drm/drm_atomic.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index f70350ca200f..5789649a4099 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1447,7 +1447,7 @@ static int atomic_set_prop(struct drm_atomic_state *state, } /** - * drm_atomic_update_old_fb -- Unset old_fb pointers and set plane->fb pointers. + * drm_atomic_clean_old_fb -- Unset old_fb pointers and set plane->fb pointers. * * @dev: drm device to check. * @plane_mask: plane mask for planes that were updated. -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/9] drm/atomic: add connector mask to drm_crtc_state.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/drm_atomic.c | 11 +++
include/drm/drm_crtc.h | 3 +++
2 files changed, 14 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index d8cc2328317a..f70350ca200f 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1042,10 +1042,21 @@ drm_atomic_set_crtc_for_connector(struct
drm_connector_state *conn_state,
{
struct drm_crtc_state *crtc_state;
+ if (conn_state->crtc && conn_state->crtc != crtc) {
+ crtc_state =
drm_atomic_get_existing_crtc_state(conn_state->state,
+
conn_state->crtc);
+
+ crtc_state->connector_mask &=
+ ~(1 << drm_connector_index(conn_state->connector));
+ }
+
if (crtc) {
crtc_state = drm_atomic_get_crtc_state(conn_state->state, crtc);
if (IS_ERR(crtc_state))
return PTR_ERR(crtc_state);
+
+ crtc_state->connector_mask |=
+ 1 << drm_connector_index(conn_state->connector);
}
conn_state->crtc = crtc;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c54da2d297ec..4999fe530f37 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -264,6 +264,7 @@ struct drm_atomic_state;
* @active_changed: crtc_state->active has been toggled.
* @connectors_changed: connectors to this crtc have been updated
* @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
+ * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of
attached connectors
* @last_vblank_count: for helpers and drivers to capture the vblank of the
* update to ensure framebuffer cleanup isn't done too early
* @adjusted_mode: for use by helpers and drivers to compute adjusted mode
timings
@@ -297,6 +298,8 @@ struct drm_crtc_state {
*/
u32 plane_mask;
+ u32 connector_mask;
+
/* last_vblank_count: for vblank waits before cleanup */
u32 last_vblank_count;
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/9] drm/i915: Update connector_mask during readout.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/i915/intel_display.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index dcc7ec7665c2..21b1984e828b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15309,6 +15309,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) <
0);
crtc->base.state->active = crtc->active;
crtc->base.enabled = crtc->active;
+ crtc->base.state->connector_mask = 0;
/* Because we only establish the connector -> encoder ->
* crtc links if something is active, this means the
@@ -15344,20 +15345,24 @@ static void intel_sanitize_encoder(struct
intel_encoder *encoder)
{
struct intel_connector *connector;
struct drm_device *dev = encoder->base.dev;
+ struct drm_crtc *crtc = encoder->base.crtc;
bool active = false;
/* We need to check both for a crtc link (meaning that the
* encoder is active and trying to read from a pipe) and the
* pipe itself being active. */
- bool has_active_crtc = encoder->base.crtc &&
- to_intel_crtc(encoder->base.crtc)->active;
+ bool has_active_crtc = crtc && crtc->state->active;
for_each_intel_connector(dev, connector) {
if (connector->base.encoder != &encoder->base)
continue;
active = true;
- break;
+ if (!has_active_crtc)
+ break;
+
+ crtc->state->connector_mask |=
+ 1 << drm_connector_index(&connector->base);
}
if (active && !has_active_crtc) {
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/9] drm/atomic: Add encoder_mask and connector_mask to crtc_state.
First 2 patches are fixes for a missing connector_state->connector. The atomic code doesn't use it currently but with these patches it causes a null pointer dereference. It's useful to know from the crtc_state what connectors and encoders are attached. This makes it possible to do iterations over connectors and encoders with just the crtc_state. The drm/tegra patch is compile tested only. Maarten Lankhorst (9): drm/i915: Set connector_state->connector correctly. drm/tegra: Assign conn_state->connector when allocating connector state. drm/core: Add drm_encoder_index. drm/core: Add drm_for_each_encoder_mask. drm/atomic: add connector mask to drm_crtc_state. drm/i915: Update connector_mask during readout. drm/atomic: Small documentation fix. drm/atomic: Remove drm_atomic_connectors_for_crtc. drm/atomic: Add encoder_mask to crtc_state. drivers/gpu/drm/drm_atomic.c | 42 +++- drivers/gpu/drm/drm_atomic_helper.c | 13 ++- drivers/gpu/drm/drm_crtc.c | 23 drivers/gpu/drm/i915/intel_display.c | 16 +++--- drivers/gpu/drm/tegra/dsi.c | 4 +++- drivers/gpu/drm/vc4/vc4_crtc.c | 2 +- include/drm/drm_atomic.h | 4 include/drm/drm_crtc.h | 17 +++ 8 files changed, 75 insertions(+), 46 deletions(-) -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 9/9] drm/atomic: Add encoder_mask to crtc_state.
This allows iteration over encoders without requiring connection_mutex.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/drm_atomic_helper.c | 4
drivers/gpu/drm/i915/intel_display.c | 3 +++
include/drm/drm_crtc.h | 2 ++
3 files changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index fb79c54b2aed..f3fd8f131f92 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -269,6 +269,8 @@ mode_fixup(struct drm_atomic_state *state)
continue;
drm_mode_copy(&crtc_state->adjusted_mode, &crtc_state->mode);
+
+ crtc_state->encoder_mask = 0;
}
for_each_connector_in_state(state, connector, conn_state, i) {
@@ -288,6 +290,8 @@ mode_fixup(struct drm_atomic_state *state)
* it away), so we won't call ->mode_fixup twice.
*/
encoder = conn_state->best_encoder;
+ crtc_state->encoder_mask |= 1 << drm_encoder_index(encoder);
+
funcs = encoder->helper_private;
if (!funcs)
continue;
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 21b1984e828b..3317db3806a8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15310,6 +15310,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
crtc->base.state->active = crtc->active;
crtc->base.enabled = crtc->active;
crtc->base.state->connector_mask = 0;
+ crtc->base.state->encoder_mask = 0;
/* Because we only establish the connector -> encoder ->
* crtc links if something is active, this means the
@@ -15363,6 +15364,8 @@ static void intel_sanitize_encoder(struct intel_encoder
*encoder)
crtc->state->connector_mask |=
1 << drm_connector_index(&connector->base);
+ crtc->state->encoder_mask |=
+ 1 << drm_encoder_index(&encoder->base);
}
if (active && !has_active_crtc) {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 4999fe530f37..7ea83b6a2864 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -265,6 +265,7 @@ struct drm_atomic_state;
* @connectors_changed: connectors to this crtc have been updated
* @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
* @connector_mask: bitmask of (1 << drm_connector_index(connector)) of
attached connectors
+ * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached
encoders
* @last_vblank_count: for helpers and drivers to capture the vblank of the
* update to ensure framebuffer cleanup isn't done too early
* @adjusted_mode: for use by helpers and drivers to compute adjusted mode
timings
@@ -299,6 +300,7 @@ struct drm_crtc_state {
u32 plane_mask;
u32 connector_mask;
+ u32 encoder_mask;
/* last_vblank_count: for vblank waits before cleanup */
u32 last_vblank_count;
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 8/9] drm/atomic: Remove drm_atomic_connectors_for_crtc.
Now that connector_mask is reliable there's no need for this
function any more.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/drm_atomic.c| 29 -
drivers/gpu/drm/drm_atomic_helper.c | 9 ++---
drivers/gpu/drm/vc4/vc4_crtc.c | 2 +-
include/drm/drm_atomic.h| 4
4 files changed, 3 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 5789649a4099..9db2941f636e 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1162,35 +1162,6 @@ drm_atomic_add_affected_planes(struct drm_atomic_state
*state,
EXPORT_SYMBOL(drm_atomic_add_affected_planes);
/**
- * drm_atomic_connectors_for_crtc - count number of connected outputs
- * @state: atomic state
- * @crtc: DRM crtc
- *
- * This function counts all connectors which will be connected to @crtc
- * according to @state. Useful to recompute the enable state for @crtc.
- */
-int
-drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
- struct drm_crtc *crtc)
-{
- struct drm_connector *connector;
- struct drm_connector_state *conn_state;
-
- int i, num_connected_connectors = 0;
-
- for_each_connector_in_state(state, connector, conn_state, i) {
- if (conn_state->crtc == crtc)
- num_connected_connectors++;
- }
-
- DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d]\n",
-state, num_connected_connectors, crtc->base.id);
-
- return num_connected_connectors;
-}
-EXPORT_SYMBOL(drm_atomic_connectors_for_crtc);
-
-/**
* drm_atomic_legacy_backoff - locking backoff for legacy ioctls
* @state: atomic state
*
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index cee31d43cd1c..fb79c54b2aed 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -420,8 +420,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
* crtc only changed its mode but has the same set of connectors.
*/
for_each_crtc_in_state(state, crtc, crtc_state, i) {
- int num_connectors;
-
/*
* We must set ->active_changed after walking connectors for
* otherwise an update that only changes active would result in
@@ -449,10 +447,7 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
if (ret != 0)
return ret;
- num_connectors = drm_atomic_connectors_for_crtc(state,
- crtc);
-
- if (crtc_state->enable != !!num_connectors) {
+ if (crtc_state->enable != !!crtc_state->connector_mask) {
DRM_DEBUG_ATOMIC("[CRTC:%d] enabled/connectors
mismatch\n",
crtc->base.id);
@@ -1668,7 +1663,7 @@ static int update_output_state(struct drm_atomic_state
*state,
if (crtc == set->crtc)
continue;
- if (!drm_atomic_connectors_for_crtc(state, crtc)) {
+ if (!crtc_state->connector_mask) {
ret = drm_atomic_set_mode_prop_for_crtc(crtc_state,
NULL);
if (ret < 0)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 7a9f4768591e..d62a541110a3 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -327,7 +327,7 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
/* The pixelvalve can only feed one encoder (and encoders are
* 1:1 with connectors.)
*/
- if (drm_atomic_connectors_for_crtc(state->state, crtc) > 1)
+ if (hweight32(state->connector_mask) > 1)
return -EINVAL;
drm_atomic_crtc_state_for_each_plane(plane, state) {
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 4b74c97d297a..cdc97589b5ce 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -130,10 +130,6 @@ int __must_check
drm_atomic_add_affected_planes(struct drm_atomic_state *state,
struct drm_crtc *crtc);
-int
-drm_atomic_connectors_for_crtc(struct drm_atomic_state *state,
- struct drm_crtc *crtc);
-
void drm_atomic_legacy_backoff(struct drm_atomic_state *state);
void
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/9] drm/tegra: Assign conn_state->connector when allocating connector state.
On Tue, Nov 24, 2015 at 10:34:29AM +0100, Maarten Lankhorst wrote: > Signed-off-by: Maarten Lankhorst > --- > drivers/gpu/drm/tegra/dsi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) Acked-by: Thierry Reding signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: Use a crtc mask instead of a active refcount for dpll functions.
This makes it easier to verify correct dpll setup with only a single crtc.
It it also useful to detect double dpll enable/disable.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/intel_display.c | 68
3 files changed, 40 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index b86feeeafc45..37d4a7f3d112 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3236,7 +3236,7 @@ static int i915_shared_dplls_info(struct seq_file *m,
void *unused)
seq_printf(m, "DPLL%i: %s, id: %i\n", i, pll->name, pll->id);
seq_printf(m, " crtc_mask: 0x%08x, active: %d, on: %s\n",
- pll->config.crtc_mask, pll->active, yesno(pll->on));
+ pll->config.crtc_mask, hweight32(pll->active_mask),
yesno(pll->on));
seq_printf(m, " tracked hardware state:\n");
seq_printf(m, " dpll:0x%08x\n", pll->config.hw_state.dpll);
seq_printf(m, " dpll_md: 0x%08x\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f3ad72abeea7..e0763c3005e8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -393,7 +393,7 @@ struct intel_shared_dpll_config {
struct intel_shared_dpll {
struct intel_shared_dpll_config config;
- int active; /* count of number of active CRTCs (i.e. DPMS on) */
+ unsigned active_mask; /* mask of active CRTCs (i.e. DPMS on) */
bool on; /* is the PLL actually active? Disabled during modeset */
const char *name;
/* should match the index in the dev_priv->shared_dplls array */
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 3317db3806a8..79bb22ef67dc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1870,7 +1870,7 @@ static void intel_prepare_shared_dpll(struct intel_crtc
*crtc)
return;
WARN_ON(!pll->config.crtc_mask);
- if (pll->active == 0) {
+ if (pll->active_mask == 0) {
DRM_DEBUG_DRIVER("setting up %s\n", pll->name);
WARN_ON(pll->on);
assert_shared_dpll_disabled(dev_priv, pll);
@@ -1892,6 +1892,7 @@ static void intel_enable_shared_dpll(struct intel_crtc
*crtc)
struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
+ unsigned crtc_mask = 1 << drm_crtc_index(&crtc->base);
if (WARN_ON(pll == NULL))
return;
@@ -1899,11 +1900,16 @@ static void intel_enable_shared_dpll(struct intel_crtc
*crtc)
if (WARN_ON(pll->config.crtc_mask == 0))
return;
+ if (WARN_ON(pll->active_mask & crtc_mask))
+ return;
+
DRM_DEBUG_KMS("enable %s (active %d, on? %d) for crtc %d\n",
- pll->name, pll->active, pll->on,
+ pll->name, hweight32(pll->active_mask), pll->on,
crtc->base.base.id);
- if (pll->active++) {
+ pll->active_mask |= crtc_mask;
+
+ if (pll->active_mask != crtc_mask) {
WARN_ON(!pll->on);
assert_shared_dpll_enabled(dev_priv, pll);
return;
@@ -1922,30 +1928,33 @@ static void intel_disable_shared_dpll(struct intel_crtc
*crtc)
struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
+ unsigned crtc_mask = 1 << drm_crtc_index(&crtc->base);
/* PCH only available on ILK+ */
- if (INTEL_INFO(dev)->gen < 5)
+ if (INTEL_INFO(dev_priv)->gen < 5)
return;
if (pll == NULL)
return;
- if (WARN_ON(!(pll->config.crtc_mask & (1 <<
drm_crtc_index(&crtc->base)
+ if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)))
return;
- DRM_DEBUG_KMS("disable %s (active %d, on? %d) for crtc %d\n",
- pll->name, pll->active, pll->on,
+ if (WARN_ON(!(pll->active_mask & crtc_mask)))
+ return;
+
+ DRM_DEBUG_KMS("disable %s (active %x, on? %d) for crtc %d\n",
+ pll->name, pll->active_mask, pll->on,
crtc->base.base.id);
- if (WARN_ON(pll->active == 0)) {
+ pll->active_mask &= ~crtc_mask;
+ if (pll->active_mask) {
assert_shared_dpll_disabled(dev_priv, pll);
return;
}
assert_shared_dpll_enabled(dev_priv, pll);
WARN_ON(!pll->on);
- if (--pll->active)
- return;
[Intel-gfx] [PATCH 2/2] drm/i915: Perform dpll commit first.
Warn for the wrong mask in enable only. Disable will have the wrong mask
now because the new state is committed before disabling the old state.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/i915/intel_display.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 79bb22ef67dc..3eb51195da72 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1897,7 +1897,7 @@ static void intel_enable_shared_dpll(struct intel_crtc
*crtc)
if (WARN_ON(pll == NULL))
return;
- if (WARN_ON(pll->config.crtc_mask == 0))
+ if (WARN_ON(!(pll->config.crtc_mask & (1 <<
drm_crtc_index(&crtc->base)
return;
if (WARN_ON(pll->active_mask & crtc_mask))
@@ -1937,9 +1937,6 @@ static void intel_disable_shared_dpll(struct intel_crtc
*crtc)
if (pll == NULL)
return;
- if (WARN_ON(!(pll->config.crtc_mask & crtc_mask)))
- return;
-
if (WARN_ON(!(pll->active_mask & crtc_mask)))
return;
@@ -13382,7 +13379,8 @@ static int intel_atomic_commit(struct drm_device *dev,
}
drm_atomic_helper_swap_state(dev, state);
- dev_priv->wm.config = to_intel_atomic_state(state)->wm_config;
+ dev_priv->wm.config = intel_state->wm_config;
+ intel_shared_dpll_commit(state);
if (intel_state->modeset) {
memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
@@ -13415,8 +13413,6 @@ static int intel_atomic_commit(struct drm_device *dev,
intel_modeset_update_crtc_state(state);
if (intel_state->modeset) {
- intel_shared_dpll_commit(state);
-
drm_atomic_helper_update_legacy_modeset_state(state->dev,
state);
modeset_update_crtc_power_domains(state);
}
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915 : Avoid superfluous invalidation of CPU cache lines
From: Akash Goel
When the object is moved out of CPU read domain, the cachelines
are not invalidated immediately. The invalidation is deferred till
next time the object is brought back into CPU read domain.
But the invalidation is done unconditionally, i.e. even for the case
where the cachelines were flushed previously, when the object moved out
of CPU write domain. This is avoidable and would lead to some optimization.
Though this is not a hypothetical case, but is unlikely to occur often.
The aim is to detect changes to the backing storage whilst the
data is potentially in the CPU cache, and only clflush in those case.
Signed-off-by: Chris Wilson
Signed-off-by: Akash Goel
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_gem.c | 9 -
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index df9316f..fedb71d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2098,6 +2098,7 @@ struct drm_i915_gem_object {
unsigned long gt_ro:1;
unsigned int cache_level:3;
unsigned int cache_dirty:1;
+ unsigned int cache_clean:1;
unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 19c282b..a13ffd4 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3552,6 +3552,7 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj,
trace_i915_gem_object_clflush(obj);
drm_clflush_sg(obj->pages);
obj->cache_dirty = false;
+ obj->cache_clean = true;
return true;
}
@@ -3982,7 +3983,13 @@ i915_gem_object_set_to_cpu_domain(struct
drm_i915_gem_object *obj, bool write)
/* Flush the CPU cache if it's still invalid. */
if ((obj->base.read_domains & I915_GEM_DOMAIN_CPU) == 0) {
- i915_gem_clflush_object(obj, false);
+ /* Invalidation not needed as there should not be any data in
+* CPU cache lines for this object, since clflush would have
+* happened when the object last moved out of CPU write domain.
+*/
+ if (!obj->cache_clean)
+ i915_gem_clflush_object(obj, false);
+ obj->cache_clean = false;
obj->base.read_domains |= I915_GEM_DOMAIN_CPU;
}
--
1.9.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915 : Avoid superfluous invalidation of CPU cache lines
On Tue, Nov 24, 2015 at 03:35:24PM +0530, akash.g...@intel.com wrote:
> From: Akash Goel
>
> When the object is moved out of CPU read domain, the cachelines
> are not invalidated immediately. The invalidation is deferred till
> next time the object is brought back into CPU read domain.
> But the invalidation is done unconditionally, i.e. even for the case
> where the cachelines were flushed previously, when the object moved out
> of CPU write domain. This is avoidable and would lead to some optimization.
> Though this is not a hypothetical case, but is unlikely to occur often.
> The aim is to detect changes to the backing storage whilst the
> data is potentially in the CPU cache, and only clflush in those case.
>
> Signed-off-by: Chris Wilson
> Signed-off-by: Akash Goel
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gem.c | 9 -
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index df9316f..fedb71d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2098,6 +2098,7 @@ struct drm_i915_gem_object {
> unsigned long gt_ro:1;
> unsigned int cache_level:3;
> unsigned int cache_dirty:1;
> + unsigned int cache_clean:1;
So now we have cache_dirty and cache_clean which seems redundant,
except somehow cache_dirty != !cache_clean?
>
> unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 19c282b..a13ffd4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3552,6 +3552,7 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj,
> trace_i915_gem_object_clflush(obj);
> drm_clflush_sg(obj->pages);
> obj->cache_dirty = false;
> + obj->cache_clean = true;
>
> return true;
> }
> @@ -3982,7 +3983,13 @@ i915_gem_object_set_to_cpu_domain(struct
> drm_i915_gem_object *obj, bool write)
>
> /* Flush the CPU cache if it's still invalid. */
> if ((obj->base.read_domains & I915_GEM_DOMAIN_CPU) == 0) {
> - i915_gem_clflush_object(obj, false);
> + /* Invalidation not needed as there should not be any data in
> + * CPU cache lines for this object, since clflush would have
> + * happened when the object last moved out of CPU write domain.
> + */
> + if (!obj->cache_clean)
> + i915_gem_clflush_object(obj, false);
> + obj->cache_clean = false;
>
> obj->base.read_domains |= I915_GEM_DOMAIN_CPU;
> }
> --
> 1.9.2
>
> ___
> 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 : Avoid superfluous invalidation of CPU cache lines
On Tue, Nov 24, 2015 at 03:35:24PM +0530, akash.g...@intel.com wrote:
> From: Akash Goel
>
> When the object is moved out of CPU read domain, the cachelines
> are not invalidated immediately. The invalidation is deferred till
> next time the object is brought back into CPU read domain.
> But the invalidation is done unconditionally, i.e. even for the case
> where the cachelines were flushed previously, when the object moved out
> of CPU write domain. This is avoidable and would lead to some optimization.
> Though this is not a hypothetical case, but is unlikely to occur often.
> The aim is to detect changes to the backing storage whilst the
> data is potentially in the CPU cache, and only clflush in those case.
Testcase: igt/gem_concurrent_blit
Testcase: igt/benchmarks/gem_set_domain
> Signed-off-by: Chris Wilson
> Signed-off-by: Akash Goel
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gem.c | 9 -
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index df9316f..fedb71d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2098,6 +2098,7 @@ struct drm_i915_gem_object {
> unsigned long gt_ro:1;
> unsigned int cache_level:3;
> unsigned int cache_dirty:1;
> + unsigned int cache_clean:1;
>
> unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 19c282b..a13ffd4 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3552,6 +3552,7 @@ i915_gem_clflush_object(struct drm_i915_gem_object *obj,
> trace_i915_gem_object_clflush(obj);
> drm_clflush_sg(obj->pages);
> obj->cache_dirty = false;
> + obj->cache_clean = true;
>
> return true;
> }
> @@ -3982,7 +3983,13 @@ i915_gem_object_set_to_cpu_domain(struct
> drm_i915_gem_object *obj, bool write)
>
> /* Flush the CPU cache if it's still invalid. */
> if ((obj->base.read_domains & I915_GEM_DOMAIN_CPU) == 0) {
> - i915_gem_clflush_object(obj, false);
> + /* Invalidation not needed as there should not be any data in
> + * CPU cache lines for this object, since clflush would have
> + * happened when the object last moved out of CPU write domain.
> + */
/* If an object is moved out of the CPU domain following a CPU write
* and before a GPU or GTT write, we will clflush it out of the CPU cache,
* and mark the cache as clean. As the object has not been accessed on the CPU
* since (i.e. the CPU cache is still clean and it is out of the CPU domain),
* we know that no CPU cache line contains stale data and so we can skip
* invalidating the CPU cache in preparing to read from the object.
*/
Marginally more verbose in stating the sequence of events for which we
can ignore the clflush invalidate.
Please Cc: Ville Syrjälä as I trust his
criticisms here.
-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 1/4] drm/i915/skl: Do not allow scaling when crtc is disabled.
This fixes a warning when the crtc is turned off. In that case fb will be NULL, and crtc_clock will be 0. Because the crtc is no longer active this is not a bug, and shouldn't trigger the WARN_ON. Also remove handling a null crtc_state, with all transitional helpers gone this can no longer happen. Signed-off-by: Maarten Lankhorst --- drivers/gpu/drm/i915/intel_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 764eeb05492d..351ecb69e5eb 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13693,7 +13693,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state struct drm_i915_private *dev_priv; int crtc_clock, cdclk; - if (!intel_crtc || !crtc_state) + if (!intel_crtc || !crtc_state->base.enable) return DRM_PLANE_HELPER_NO_SCALING; dev = intel_crtc->base.dev; -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/4] drm/i915/bxt: Use the bypass frequency if there are no active pipes.
Now that pixel clock is set to 0 when there are no active pipes it's easy to set the bypass frequency for this case. Signed-off-by: Maarten Lankhorst --- drivers/gpu/drm/i915/intel_display.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 178a042f917e..8a825f24ce51 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5972,7 +5972,6 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv, /* * FIXME: * - remove the guardband, it's not needed on BXT -* - set 19.2MHz bypass frequency if there are no active pipes */ if (max_pixclk > 576000*9/10) return 624000; @@ -5982,8 +5981,10 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv, return 384000; else if (max_pixclk > 144000*9/10) return 288000; - else + else if (max_pixclk) return 144000; + else + return 19200; } /* Compute the max pixel clock for new configuration. Uses atomic state if -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/4] drm/i915: Handle cdclk limits on broadwell.
As the comment indicates this can only fail gracefully when
called from compute_config. Fortunately this is now what's happening,
so the fixme can be removed and the DRM_ERROR downgraded.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/i915/intel_display.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 351ecb69e5eb..3c46037b6e55 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9677,14 +9677,10 @@ static int broadwell_modeset_calc_cdclk(struct
drm_atomic_state *state)
else
cdclk = 337500;
- /*
-* FIXME move the cdclk caclulation to
-* compute_config() so we can fail gracegully.
-*/
if (cdclk > dev_priv->max_cdclk_freq) {
- DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
- cdclk, dev_priv->max_cdclk_freq);
- cdclk = dev_priv->max_cdclk_freq;
+ DRM_INFO("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
+cdclk, dev_priv->max_cdclk_freq);
+ return -EINVAL;
}
to_intel_atomic_state(state)->cdclk = cdclk;
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/4] drm/i915: Do not acquire crtc state to check clock during modeset, v3.
Parallel modesets are still not allowed, but this will allow updating
a different crtc during a modeset if the clock is not changed.
Additionally when all pipes are DPMS off the cdclk will be lowered
to the minimum allowed.
Changes since v1:
- Add dev_priv->active_crtcs for tracking which crtcs are active.
- Rename min_cdclk to min_pixclk and move to dev_priv.
- Add a active_crtcs mask which is updated atomically.
- Add intel_atomic_state->modeset which is set on modesets.
- Commit new pixclk/active_crtcs right after state swap.
Changes since v2:
- Make the changes related to max_pixel_rate calculations more readable.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/i915/i915_drv.h | 5 ++
drivers/gpu/drm/i915/intel_atomic.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 139 +--
drivers/gpu/drm/i915/intel_drv.h | 7 +-
4 files changed, 112 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a32571f0d018..c7b76c2e8e09 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1821,8 +1821,13 @@ struct drm_i915_private {
struct intel_pipe_crc pipe_crc[I915_MAX_PIPES];
#endif
+ /* dpll and cdclk state is protected by connection_mutex */
int num_shared_dpll;
struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
+
+ unsigned int active_crtcs;
+ unsigned int min_pixclk[I915_MAX_PIPES];
+
int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
struct i915_workarounds workarounds;
diff --git a/drivers/gpu/drm/i915/intel_atomic.c
b/drivers/gpu/drm/i915/intel_atomic.c
index 643f342de33b..c4eadbc928b7 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -306,5 +306,5 @@ void intel_atomic_state_clear(struct drm_atomic_state *s)
{
struct intel_atomic_state *state = to_intel_atomic_state(s);
drm_atomic_state_default_clear(&state->base);
- state->dpll_set = false;
+ state->dpll_set = state->modeset = false;
}
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 3c46037b6e55..178a042f917e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5991,22 +5991,31 @@ static int broxton_calc_cdclk(struct drm_i915_private
*dev_priv,
static int intel_mode_max_pixclk(struct drm_device *dev,
struct drm_atomic_state *state)
{
- struct intel_crtc *intel_crtc;
- struct intel_crtc_state *crtc_state;
- int max_pixclk = 0;
+ struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *crtc_state;
+ unsigned max_pixclk = 0, i;
+ enum pipe pipe;
- for_each_intel_crtc(dev, intel_crtc) {
- crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
- if (IS_ERR(crtc_state))
- return PTR_ERR(crtc_state);
+ memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
+ sizeof(intel_state->min_pixclk));
- if (!crtc_state->base.enable)
- continue;
+ for_each_crtc_in_state(state, crtc, crtc_state, i) {
+ int pixclk = 0;
+
+ if (crtc_state->enable)
+ pixclk = crtc_state->adjusted_mode.crtc_clock;
- max_pixclk = max(max_pixclk,
-crtc_state->base.adjusted_mode.crtc_clock);
+ intel_state->min_pixclk[i] = pixclk;
}
+ if (!intel_state->active_crtcs)
+ return 0;
+
+ for_each_pipe(dev_priv, pipe)
+ max_pixclk = max(intel_state->min_pixclk[pipe], max_pixclk);
+
return max_pixclk;
}
@@ -6310,6 +6319,9 @@ static void intel_crtc_disable_noatomic(struct drm_crtc
*crtc)
for_each_power_domain(domain, domains)
intel_display_power_put(dev_priv, domain);
intel_crtc->enabled_power_domains = 0;
+
+ dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
+ dev_priv->min_pixclk[intel_crtc->pipe] = 0;
}
/*
@@ -9555,29 +9567,41 @@ static void broxton_modeset_commit_cdclk(struct
drm_atomic_state *old_state)
/* compute the max rate for new configuration */
static int ilk_max_pixel_rate(struct drm_atomic_state *state)
{
- struct intel_crtc *intel_crtc;
+ struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+ struct drm_i915_private *dev_priv = state->dev->dev_private;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *cstate;
struct intel_crtc_state *crtc_state;
- int max_pixel_rate = 0;
+ unsigned max_pixel_rate = 0, i;
+ enum pipe pipe;
- for_each_intel_crtc(state->dev, intel_crtc) {
- int pixe
[Intel-gfx] [PATCH 0/4] cdclk fixes
The first patch fixes a WARN_ON that happens when running kms_flip. Second patch fixes a FIXME. Now that the code's atomic the cdclk is calculated in compute_config so -EINVAL can be returned. The third patch introduces active_crtcs which is useful to have in general, and also calculates the state without locking other crtc's. This will allow modesets on newer platforms without locking all planes and crtc's. The final patch is a small removal of a FIXME for broxton. Hopefully useful to have. Maarten Lankhorst (4): drm/i915/skl: Do not allow scaling when crtc is disabled. drm/i915: Handle cdclk limits on broadwell. drm/i915: Do not acquire crtc state to check clock during modeset, v3. drm/i915/bxt: Use the bypass frequency if there are no active pipes. drivers/gpu/drm/i915/i915_drv.h | 5 ++ drivers/gpu/drm/i915/intel_atomic.c | 2 +- drivers/gpu/drm/i915/intel_display.c | 156 --- drivers/gpu/drm/i915/intel_drv.h | 7 +- 4 files changed, 119 insertions(+), 51 deletions(-) -- 2.1.0 ___ 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: Handle cdclk limits on broadwell.
On Tue, Nov 24, 2015 at 11:29:03AM +0100, Maarten Lankhorst wrote:
> As the comment indicates this can only fail gracefully when
> called from compute_config. Fortunately this is now what's happening,
> so the fixme can be removed and the DRM_ERROR downgraded.
>
> Signed-off-by: Maarten Lankhorst
> ---
> drivers/gpu/drm/i915/intel_display.c | 10 +++---
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 351ecb69e5eb..3c46037b6e55 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9677,14 +9677,10 @@ static int broadwell_modeset_calc_cdclk(struct
> drm_atomic_state *state)
> else
> cdclk = 337500;
>
> - /*
> - * FIXME move the cdclk caclulation to
> - * compute_config() so we can fail gracegully.
> - */
> if (cdclk > dev_priv->max_cdclk_freq) {
> - DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> - cdclk, dev_priv->max_cdclk_freq);
> - cdclk = dev_priv->max_cdclk_freq;
> + DRM_INFO("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> + cdclk, dev_priv->max_cdclk_freq);
This is a validation step, right? So the invalid value is never applied
to hardware and so doesn't really exist for anybody but the calling
userspace. Ergo, this shouldn't be logged for the user but only when
debugging the application -> DRM_DEBUG_KMS
Dreaming of an application-centric debug log,
-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/9] drm/tegra: Assign conn_state->connector when allocating connector state.
On Tue, Nov 24, 2015 at 10:34:29AM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst
> ---
> drivers/gpu/drm/tegra/dsi.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index f0a138ef68ce..6b8ae3d08eeb 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -751,8 +751,10 @@ static void tegra_dsi_connector_reset(struct
> drm_connector *connector)
> connector->state = NULL;
>
> state = kzalloc(sizeof(*state), GFP_KERNEL);
> - if (state)
> + if (state) {
> + state->base.connector = connector;
> connector->state = &state->base;
> + }
Hm, we might want __ versions of all the _reset hooks if this becomes more
common. I do wonder a bit why it isn't since a lot of drivers overwrite
state structures by now, and then the provided reset functions aren't
sufficient really.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
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 6/9] drm/i915: Update connector_mask during readout.
On Tue, Nov 24, 2015 at 10:34:33AM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst
> ---
> drivers/gpu/drm/i915/intel_display.c | 11 ---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index dcc7ec7665c2..21b1984e828b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15309,6 +15309,7 @@ static void intel_sanitize_crtc(struct intel_crtc
> *crtc)
> WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) <
> 0);
> crtc->base.state->active = crtc->active;
> crtc->base.enabled = crtc->active;
> + crtc->base.state->connector_mask = 0;
>
> /* Because we only establish the connector -> encoder ->
>* crtc links if something is active, this means the
> @@ -15344,20 +15345,24 @@ static void intel_sanitize_encoder(struct
> intel_encoder *encoder)
> {
> struct intel_connector *connector;
> struct drm_device *dev = encoder->base.dev;
> + struct drm_crtc *crtc = encoder->base.crtc;
> bool active = false;
>
> /* We need to check both for a crtc link (meaning that the
>* encoder is active and trying to read from a pipe) and the
>* pipe itself being active. */
> - bool has_active_crtc = encoder->base.crtc &&
> - to_intel_crtc(encoder->base.crtc)->active;
> + bool has_active_crtc = crtc && crtc->state->active;
>
> for_each_intel_connector(dev, connector) {
> if (connector->base.encoder != &encoder->base)
> continue;
>
> active = true;
> - break;
> + if (!has_active_crtc)
> + break;
> +
> + crtc->state->connector_mask |=
> + 1 << drm_connector_index(&connector->base);
We might want to use the official set_crtc_for_connector/plane in all our
hw state readout code. Is that possible? Would be perfect as a prep patch.
-Daniel
> }
>
> if (active && !has_active_crtc) {
> --
> 2.1.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
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 v1] drm/i915: Fix a false alert of memory leak when free LRC
On 23/11/15 22:30, Yu Dai wrote: On 11/23/2015 02:34 AM, Tvrtko Ursulin wrote: On 20/11/15 08:31, Daniel Vetter wrote: > On Thu, Nov 19, 2015 at 04:10:26PM -0800, yu@intel.com wrote: >> From: Alex Dai >> >> There is a memory leak warning message from i915_gem_context_clean >> when GuC submission is enabled. The reason is that when LRC is >> released, its ppgtt could be still referenced. The assumption that >> all VMAs are unbound during release of LRC is not true. >> >> v1: Move the code inside i915_gem_context_clean() to where ppgtt is >> released because it is not cleaning context anyway but ppgtt. >> >> Signed-off-by: Alex Dai > > retire__read drops the ctx (and hence ppgtt) reference too early, > resulting in us hitting the WARNING. See the giant thread with Tvrtko, > Chris and me: > > http://www.spinics.net/lists/intel-gfx/msg78918.html > > Would be great if someone could test the diff I posted in there. It doesn't work - I have posted my IGT snippet which I thought explained it. I thought moving the VMA list clean up into i915_ppgtt_release() should work. However, it creates a chicken & egg problem. ppgtt_release() rely on vma_unbound() to be called first to decrease its refcount. So calling vma_unbound() inside ppgtt_release() is not right. Problem req unreference in obj->active case. When it hits that path it will not move the VMA to inactive and the intel_execlists_retire_requests will be the last unreference from the retire worker which will trigger the WARN. I still think the problem comes from the assumption that when lrc is released, its all VMAs should be unbound. Precisely I mean the comments made for i915_gem_context_clean() - "This context is going away and we need to remove all VMAs still around." Really the lrc life cycle is different from ppgtt / VMAs. Check the line after i915_gem_context_clean(). It is ppgtt_put(). In the case lrc is freed early, It won't release ppgtt anyway because it is still referenced by VMAs. An it will be freed when no ref of GEM obj. Yes, but the last is just a consequence of how the code was laid out, not a hard requirement as far as I understand it. When context destructor runs that means two things: 1. GPU is done with all VMAs belonging to the VM. 2. USerspace also cannot reach them any more either. So VMAs have no reason to exist beyond that point. If they do, they can be left dangling under certain circumstances. See below. I posted an IGT which hits that -> http://patchwork.freedesktop.org/patch/65369/ And posted one give up on the active VMA mem leak patch -> http://patchwork.freedesktop.org/patch/65529/ This patch will silent the warning. But I think the i915_gem_context_clean() itself is unnecessary. I don't see any issue by deleting it. The check of VMA list is inside ppgtt_release() and the unbound should be aligned to GEM obj's life cycle but not lrc life cycle. i915_gem_context_clean solves a memory leak which is explained in the commit. If there is an extra reference on the GEM obj, like in the flink case, VMAs will not get unbound. Apart from various IGTs, you can also demonstrate this leak with fbcon and Xorg. Every time you re-start Xorg one VMA will be leaked since it imports the fbcon fb via flink. Second part of the story is that the WARN in i915_gem_context_clean is wrong because of how retirement works. So if we removed the WARN, cleanup still has some value to avoid memory leak in the above described Xorg case, or in any case where flink is in the picture and VMAs have been retired to inactive. Bug left standing would be that if the VMA happens to be flinked and on the active list, because of say being shared between rings and contexts, it would still be leaked. But it is less leaking than without the cleanup. I proposed another way of fixing that: http://patchwork.freedesktop.org/patch/65861/ I have no idea yet of GuC implications, I just spotted this parallel thread. And Mika has proposed something interesting - that we could just clean up the active VMA in context cleanup since we know it is a false one. However, again I don't know how that interacts with the GuC. Surely it cannot be freeing the context with stuff genuinely still active in the GuC? There is no interacts with GuC though. Just very easy to see the warning when GuC is enabled, says when run gem_close_race. The reason is that GuC does not use the execlist_queue (execlist_retired_req_list) which is deferred to retire worker. Same as ring submission mode, when GuC is enabled, whenever driver submits a new batch, it will try to release previous request. I don't know why intel_execlists_retire_requests is not called for this case. Probably because of the unpin. Deferring the retirement may just hide the issue. I bet you will see the warning more often if you change i915_gem_retire_requests_ring() to i915_gem_retire_requests() in i915_gem_execbuffer_reserve(). Hmmm.. gem_close_race uses flink, but why would it tr
Re: [Intel-gfx] [PATCH 7/9] drm/atomic: Small documentation fix.
On Tue, Nov 24, 2015 at 10:34:34AM +0100, Maarten Lankhorst wrote: > Use the correct function name for drm_atomic_clean_old_fb docs. > > Signed-off-by: Maarten Lankhorst Applied to topic/drm-misc, thanks. -Daniel > --- > drivers/gpu/drm/drm_atomic.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index f70350ca200f..5789649a4099 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1447,7 +1447,7 @@ static int atomic_set_prop(struct drm_atomic_state > *state, > } > > /** > - * drm_atomic_update_old_fb -- Unset old_fb pointers and set plane->fb > pointers. > + * drm_atomic_clean_old_fb -- Unset old_fb pointers and set plane->fb > pointers. > * > * @dev: drm device to check. > * @plane_mask: plane mask for planes that were updated. > -- > 2.1.0 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation 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/sysfs: Send out uevent when connector->force changes
On Fri, Nov 20, 2015 at 09:25:14AM +, Chris Wilson wrote:
> On Fri, Nov 20, 2015 at 09:11:00AM +0100, Daniel Vetter wrote:
> > On Thu, Nov 19, 2015 at 09:06:04PM +, Chris Wilson wrote:
> > > On Thu, Nov 19, 2015 at 05:46:50PM +0100, Daniel Vetter wrote:
> > > > To avoid even more code duplication punt this all to the probe worker,
> > > > which needs some slight adjustment to also generate a uevent when the
> > > > status has changed to due connector->force.
> > > >
> > > > v2: Instead of running the output_poll_work (which is kinda the wrong
> > > > thing and a layering violation since it's an internal of the probe
> > > > helpers), or calling ->detect (which is again a layering violation
> > > > since it's used only by probe helpers) just call the official
> > > > ->fill_modes function, like a GET_CONNECTOR ioctl call.
> > > >
> > > > v3: Restore the accidentally removed forced-probe for echo "detect" >
> > > > force.
> > > >
> > >
> > > > diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> > > > index 9ac4ffa6cce3..df66d9447cb0 100644
> > > > --- a/drivers/gpu/drm/drm_sysfs.c
> > > > +++ b/drivers/gpu/drm/drm_sysfs.c
> > > > @@ -167,47 +167,35 @@ static ssize_t status_store(struct device *device,
> > > > {
> > > > struct drm_connector *connector = to_drm_connector(device);
> > > > struct drm_device *dev = connector->dev;
> > > > - enum drm_connector_status old_status;
> > > > + enum drm_connector_force old_force;
> > > > int ret;
> > > >
> > > > ret = mutex_lock_interruptible(&dev->mode_config.mutex);
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - old_status = connector->status;
> > > > + old_force = connector->force;
> > > >
> > > > - if (sysfs_streq(buf, "detect")) {
> > > > + if (sysfs_streq(buf, "detect"))
> > > > connector->force = 0;
> > > > - connector->status = connector->funcs->detect(connector,
> > > > true);
> > > > - } else if (sysfs_streq(buf, "on")) {
> > > > + else if (sysfs_streq(buf, "on"))
> > > > connector->force = DRM_FORCE_ON;
> > > > - } else if (sysfs_streq(buf, "on-digital")) {
> > > > + else if (sysfs_streq(buf, "on-digital"))
> > > > connector->force = DRM_FORCE_ON_DIGITAL;
> > > > - } else if (sysfs_streq(buf, "off")) {
> > > > + else if (sysfs_streq(buf, "off"))
> > > > connector->force = DRM_FORCE_OFF;
> > > > - } else
> > > > + else
> > > > ret = -EINVAL;
> > > >
> > > > - if (ret == 0 && connector->force) {
> > > > - if (connector->force == DRM_FORCE_ON ||
> > > > - connector->force == DRM_FORCE_ON_DIGITAL)
> > > > - connector->status = connector_status_connected;
> > > > - else
> > > > - connector->status =
> > > > connector_status_disconnected;
> > > > - if (connector->funcs->force)
> > > > - connector->funcs->force(connector);
> > > > - }
> > > > -
> > > > - if (old_status != connector->status) {
> > > > - DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d
> > > > to %d\n",
> > > > + if (old_force != connector->force || !connector->force) {
> > > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force updated from %d
> > > > to %d or reprobing\n",
> > > > connector->base.id,
> > > > connector->name,
> > > > - old_status, connector->status);
> > > > + old_force, connector->force);
> > > >
> > > > - dev->mode_config.delayed_event = true;
> > > > - if (dev->mode_config.poll_enabled)
> > > > -
> > > > schedule_delayed_work(&dev->mode_config.output_poll_work,
> > > > - 0);
> > > > + connector->funcs->fill_modes(connector,
> > > > +dev->mode_config.max_width,
> > > > +
> > > > dev->mode_config.max_height);
> > >
> > > This now does fill_modes() before we call detect(). We rely on the
> > > ordering of detect() before doing fill_modes() as in many cases we use
> > > the EDID gathered in detect() to generate the modes (if I am not
> > > mistaken, I think we merged those patches to cache the EDID for a
> > > detection cycle).
> >
> > ->fill_modes = drm_helper_probe_single_connector_modes which then calls
> > ->detect. By going this way we avoid duplicating the "send uevent if
> > anything changed" logic all over the place, and ->detect becomes purely a
> > helper internal callback to get at the mode list for hotpluggeable
> > outputs.
>
> Ok, that struck me as a little surprising - I was thinking of lower level
> get_
Re: [Intel-gfx] [PATCH 2/9] drm/tegra: Assign conn_state->connector when allocating connector state.
On Tue, Nov 24, 2015 at 11:37:47AM +0100, Daniel Vetter wrote:
> On Tue, Nov 24, 2015 at 10:34:29AM +0100, Maarten Lankhorst wrote:
> > Signed-off-by: Maarten Lankhorst
> > ---
> > drivers/gpu/drm/tegra/dsi.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> > index f0a138ef68ce..6b8ae3d08eeb 100644
> > --- a/drivers/gpu/drm/tegra/dsi.c
> > +++ b/drivers/gpu/drm/tegra/dsi.c
> > @@ -751,8 +751,10 @@ static void tegra_dsi_connector_reset(struct
> > drm_connector *connector)
> > connector->state = NULL;
> >
> > state = kzalloc(sizeof(*state), GFP_KERNEL);
> > - if (state)
> > + if (state) {
> > + state->base.connector = connector;
> > connector->state = &state->base;
> > + }
>
> Hm, we might want __ versions of all the _reset hooks if this becomes more
> common. I do wonder a bit why it isn't since a lot of drivers overwrite
> state structures by now, and then the provided reset functions aren't
> sufficient really.
We already have the __ versions for duplicate_state helpers, but the
problem with reset helpers is that they need to know the allocation
size...
Actually, that's true of the duplicate_state helpers as well, and the __
variants don't actually allocate the memory but rather just copy the
state from old to new. So we could do something just like that for the
reset helpers:
void __drm_atomic_helper_connector_reset(struct drm_connector
*connector,
struct drm_connector_state
*state)
{
state->base.connector = connector;
connector->state = state;
}
and use like this:
static void tegra_dsi_connector_reset(struct drm_connector *connector)
{
struct tegra_dsi_state *state;
...
state = kzalloc(sizeof(*state), GFP_KERNEL);
if (state)
__drm_atomic_helper_connector_reset(connector,
&state->base);
}
I think back at the time when we did this for duplicate_state helpers we
had a discussion about the usefulness of this, but this patchset clearly
shows that this kind of change, which applies to a number of drivers, is
going to happen again and again, so the only way to avoid mistakes or an
extensive set of changes across all drivers is by putting this into a
helper, even if it's only two assignments now.
Thierry
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v1] drm/i915: Fix a false alert of memory leak when free LRC
On Mon, Nov 23, 2015 at 10:34:59AM +, Tvrtko Ursulin wrote: > > On 20/11/15 08:31, Daniel Vetter wrote: > >On Thu, Nov 19, 2015 at 04:10:26PM -0800, yu@intel.com wrote: > >>From: Alex Dai > >> > >>There is a memory leak warning message from i915_gem_context_clean > >>when GuC submission is enabled. The reason is that when LRC is > >>released, its ppgtt could be still referenced. The assumption that > >>all VMAs are unbound during release of LRC is not true. > >> > >>v1: Move the code inside i915_gem_context_clean() to where ppgtt is > >>released because it is not cleaning context anyway but ppgtt. > >> > >>Signed-off-by: Alex Dai > > > >retire__read drops the ctx (and hence ppgtt) reference too early, > >resulting in us hitting the WARNING. See the giant thread with Tvrtko, > >Chris and me: > > > >http://www.spinics.net/lists/intel-gfx/msg78918.html > > > >Would be great if someone could test the diff I posted in there. > > It doesn't work - I have posted my IGT snippet which I thought explained it. > > Problem req unreference in obj->active case. When it hits that path it will > not move the VMA to inactive and the intel_execlists_retire_requests will be > the last unreference from the retire worker which will trigger the WARN. > > I posted an IGT which hits that -> > http://patchwork.freedesktop.org/patch/65369/ > > And posted one give up on the active VMA mem leak patch -> > http://patchwork.freedesktop.org/patch/65529/ Ok, I get things now. Fundamentally the problem is that we track active per-obj, but we want it per-vma. In a way this patch just diggs us deeper into that hole, which doesn't make me all too happy. Oh well. I'll pull in the warning removal. -Daniel > I have no idea yet of GuC implications, I just spotted this parallel thread. > > And Mika has proposed something interesting - that we could just clean up > the active VMA in context cleanup since we know it is a false one. > > However, again I don't know how that interacts with the GuC. Surely it > cannot be freeing the context with stuff genuinely still active in the GuC? > > Regards, > > Tvrtko > -- Daniel Vetter Software Engineer, Intel Corporation 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: Remove incorrect warning in context cleanup
On Fri, Nov 20, 2015 at 01:23:36PM +, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> Commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae
> Author: Tvrtko Ursulin
> Date: Mon Oct 5 13:26:36 2015 +0100
>
> drm/i915: Clean up associated VMAs on context destruction
>
> Added a warning based on an incorrect assumption that all VMAs
> in a VM will be on the inactive list at the point last reference
> to a context and VM is dropped.
>
> This is not true because i915_gem_object_retire__read will not
> put VMA on the inactive list until all activities on the object
> in question (in all VMs) have been retired.
>
> As a consequence, whether or not a context/VM will be destroyed
> with its VMAs still on the active list, can depend on completely
> unrelated activities using the same object from a different
> context or engine.
>
> Signed-off-by: Tvrtko Ursulin
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92638
> Testcase: igt/gem_request_retire/retire-vma-not-inactive
> Cc: Daniel Vetter
> Cc: Chris Wilson
> Cc: Michel Thierry
Queued for -next, thanks for the patch.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_gem_context.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 204dc7c0b2d6..59dba318213e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -141,8 +141,6 @@ static void i915_gem_context_clean(struct intel_context
> *ctx)
> if (!ppgtt)
> return;
>
> - WARN_ON(!list_empty(&ppgtt->base.active_list));
> -
> list_for_each_entry_safe(vma, next, &ppgtt->base.inactive_list,
>mm_list) {
> if (WARN_ON(__i915_vma_unbind_no_wait(vma)))
> --
> 1.9.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Announcement] 2015-Q3 release of XenGT - a Mediated Graphics Passthrough Solution from Intel
On Thu, Nov 19, 2015 at 01:02:36PM -0700, Alex Williamson wrote: > On Thu, 2015-11-19 at 04:06 +, Tian, Kevin wrote: > > > From: Alex Williamson [mailto:alex.william...@redhat.com] > > > Sent: Thursday, November 19, 2015 2:12 AM > > > > > > [cc +qemu-devel, +paolo, +gerd] > > > > > > Another area of extension is how to expose a framebuffer to QEMU for > > > seamless integration into a SPICE/VNC channel. For this I believe we > > > could use a new region, much like we've done to expose VGA access > > > through a vfio device file descriptor. An area within this new > > > framebuffer region could be directly mappable in QEMU while a > > > non-mappable page, at a standard location with standardized format, > > > provides a description of framebuffer and potentially even a > > > communication channel to synchronize framebuffer captures. This would > > > be new code for QEMU, but something we could share among all vGPU > > > implementations. > > > > Now GVT-g already provides an interface to decode framebuffer information, > > w/ an assumption that the framebuffer will be further composited into > > OpenGL APIs. So the format is defined according to OpenGL definition. > > Does that meet SPICE requirement? > > > > Another thing to be added. Framebuffers are frequently switched in > > reality. So either Qemu needs to poll or a notification mechanism is > > required. > > And since it's dynamic, having framebuffer page directly exposed in the > > new region might be tricky. We can just expose framebuffer information > > (including base, format, etc.) and let Qemu to map separately out of VFIO > > interface. > > Sure, we'll need to work out that interface, but it's also possible that > the framebuffer region is simply remapped to another area of the device > (ie. multiple interfaces mapping the same thing) by the vfio device > driver. Whether it's easier to do that or make the framebuffer region > reference another region is something we'll need to see. > > > And... this works fine with vGPU model since software knows all the > > detail about framebuffer. However in pass-through case, who do you expect > > to provide that information? Is it OK to introduce vGPU specific APIs in > > VFIO? > > Yes, vGPU may have additional features, like a framebuffer area, that > aren't present or optional for direct assignment. Obviously we support > direct assignment of GPUs for some vendors already without this feature. For exposing framebuffers for spice/vnc I highly recommend against anything that looks like a bar/fixed mmio range mapping. First this means the kernel driver needs to internally fake remapping, which isn't fun. Second we can't get at the memory in an easy fashion for hw-accelerated compositing. My recoomendation is to build the actual memory access for underlying framebuffers on top of dma-buf, so that it can be vacuumed up by e.g. the host gpu driver again for rendering. For userspace the generic part would simply be an invalidate-fb signal, with the new dma-buf supplied. Upsides: - You can composit stuff with the gpu. - VRAM and other kinds of resources (even stuff not visible in pci bars) can be represented. Downside: Tracking mapping changes on the guest side won't be any easier. This is mostly a problem for integrated gpus, since discrete ones usually require contiguous vram for scanout. I think saying "don't do that" is a valid option though, i.e. we're assuming that page mappings for a in-use scanout range never changes on the guest side. That is true for at least all the current linux drivers. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/9] drm/tegra: Assign conn_state->connector when allocating connector state.
Op 24-11-15 om 11:51 schreef Thierry Reding:
> On Tue, Nov 24, 2015 at 11:37:47AM +0100, Daniel Vetter wrote:
>> On Tue, Nov 24, 2015 at 10:34:29AM +0100, Maarten Lankhorst wrote:
>>> Signed-off-by: Maarten Lankhorst
>>> ---
>>> drivers/gpu/drm/tegra/dsi.c | 4 +++-
>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
>>> index f0a138ef68ce..6b8ae3d08eeb 100644
>>> --- a/drivers/gpu/drm/tegra/dsi.c
>>> +++ b/drivers/gpu/drm/tegra/dsi.c
>>> @@ -751,8 +751,10 @@ static void tegra_dsi_connector_reset(struct
>>> drm_connector *connector)
>>> connector->state = NULL;
>>>
>>> state = kzalloc(sizeof(*state), GFP_KERNEL);
>>> - if (state)
>>> + if (state) {
>>> + state->base.connector = connector;
>>> connector->state = &state->base;
>>> + }
>> Hm, we might want __ versions of all the _reset hooks if this becomes more
>> common. I do wonder a bit why it isn't since a lot of drivers overwrite
>> state structures by now, and then the provided reset functions aren't
>> sufficient really.
> We already have the __ versions for duplicate_state helpers, but the
> problem with reset helpers is that they need to know the allocation
> size...
>
> Actually, that's true of the duplicate_state helpers as well, and the __
> variants don't actually allocate the memory but rather just copy the
> state from old to new. So we could do something just like that for the
> reset helpers:
>
> void __drm_atomic_helper_connector_reset(struct drm_connector
> *connector,
>struct drm_connector_state
> *state)
> {
> state->base.connector = connector;
> connector->state = state;
> }
>
> and use like this:
>
> static void tegra_dsi_connector_reset(struct drm_connector *connector)
> {
> struct tegra_dsi_state *state;
> ...
> state = kzalloc(sizeof(*state), GFP_KERNEL);
> if (state)
> __drm_atomic_helper_connector_reset(connector,
> &state->base);
> }
>
> I think back at the time when we did this for duplicate_state helpers we
> had a discussion about the usefulness of this, but this patchset clearly
> shows that this kind of change, which applies to a number of drivers, is
> going to happen again and again, so the only way to avoid mistakes or an
> extensive set of changes across all drivers is by putting this into a
> helper, even if it's only two assignments now.
Yeah was considering it, but it felt a bit overkill for something that only
holds a pointer to crtc, best_encoder and connector..
If this is the way to go I'll resend
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/9] drm/i915: Update connector_mask during readout.
Op 24-11-15 om 11:38 schreef Daniel Vetter:
> On Tue, Nov 24, 2015 at 10:34:33AM +0100, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 11 ---
>> 1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index dcc7ec7665c2..21b1984e828b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -15309,6 +15309,7 @@ static void intel_sanitize_crtc(struct intel_crtc
>> *crtc)
>> WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, NULL) <
>> 0);
>> crtc->base.state->active = crtc->active;
>> crtc->base.enabled = crtc->active;
>> +crtc->base.state->connector_mask = 0;
>>
>> /* Because we only establish the connector -> encoder ->
>> * crtc links if something is active, this means the
>> @@ -15344,20 +15345,24 @@ static void intel_sanitize_encoder(struct
>> intel_encoder *encoder)
>> {
>> struct intel_connector *connector;
>> struct drm_device *dev = encoder->base.dev;
>> +struct drm_crtc *crtc = encoder->base.crtc;
>> bool active = false;
>>
>> /* We need to check both for a crtc link (meaning that the
>> * encoder is active and trying to read from a pipe) and the
>> * pipe itself being active. */
>> -bool has_active_crtc = encoder->base.crtc &&
>> -to_intel_crtc(encoder->base.crtc)->active;
>> +bool has_active_crtc = crtc && crtc->state->active;
>>
>> for_each_intel_connector(dev, connector) {
>> if (connector->base.encoder != &encoder->base)
>> continue;
>>
>> active = true;
>> -break;
>> +if (!has_active_crtc)
>> +break;
>> +
>> +crtc->state->connector_mask |=
>> +1 << drm_connector_index(&connector->base);
> We might want to use the official set_crtc_for_connector/plane in all our
> hw state readout code. Is that possible? Would be perfect as a prep patch.
>
No, unfortunately that needs the full atomic state because it calls
drm_atomic_get_crtc_state.
~Maarten
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/sysfs: Send out uevent when connector->force changes
On Tue, Nov 24, 2015 at 11:51:09AM +0100, Daniel Vetter wrote: > On Fri, Nov 20, 2015 at 09:25:14AM +, Chris Wilson wrote: > > And something like .fill_modes -> .probe (after removing .detect). > > Hm, not sure. For panels we never really probe anything, the important bit > is (at least for the caller in drm_crtc.c) that it fills in the > connectore->modes list. Given that I think the current name is okish. imo .fill_modes() does not imply that it communicates with the output, unlike say .detect(), .probe(), or .get_modes(). -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] [Announcement] 2015-Q3 release of XenGT - a Mediated Graphics Passthrough Solution from Intel
On Tue, Nov 24, 2015 at 12:19:18PM +0100, Daniel Vetter wrote: > Downside: Tracking mapping changes on the guest side won't be any easier. > This is mostly a problem for integrated gpus, since discrete ones usually > require contiguous vram for scanout. I think saying "don't do that" is a > valid option though, i.e. we're assuming that page mappings for a in-use > scanout range never changes on the guest side. That is true for at least > all the current linux drivers. Apart from we already suffer limitations of fixed mappings and have patches that want to change the page mapping of active scanouts. -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 i-g-t v2] lib/igt_kms: Introduce get_first_connected_output macro
On Mon, Nov 23, 2015 at 03:49:20PM +0200, Ander Conselvan De Oliveira wrote: > On Fri, 2015-11-20 at 18:56 -0800, Vivek Kasireddy wrote: > > In some cases, we just need one valid (connected) output to perform > > a test. This macro can help in these situations by not having to > > put the test code inside a for loop that iterates over all the outputs. > > > > v2: Added a brief documentation for this macro. > > > > Suggested-by: Matt Roper > > Cc: Thomas Wood > > Signed-off-by: Vivek Kasireddy > > --- > > lib/igt_kms.h | 12 > > 1 file changed, 12 insertions(+) > > > > diff --git a/lib/igt_kms.h b/lib/igt_kms.h > > index 965c47c..a0bb066 100644 > > --- a/lib/igt_kms.h > > +++ b/lib/igt_kms.h > > @@ -279,6 +279,18 @@ void igt_wait_for_vblank(int drm_fd, enum pipe pipe); > > for (int i__ = 0; (plane) = &(display)->pipes[(pipe)].planes[i__], \ > > i__ < (display)->pipes[(pipe)].n_planes; i__++) > > > > +/** > > + * get_first_connected_output: > > + * @display: Initialized igt_display_t type object > > + * @output: igt_output_t type object > > + * > > + * Returns: First valid (connected) output. > > + */ > > +#define get_first_connected_output(display, output)\ > > + for (int i__ = 0; i__ < (display)->n_outputs; i__++) \ > > + if ((output = &(display)->outputs[i__]), output->valid) \ > > + break > > + > > Is it possible that there is no valid output? > We also need to check at least crtc restrictions, otherwise bsw will fall over mightly. Or well anything with MIPI DSI encoders. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation 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/3] drm/i915: Use insert_page for pwrite_fast
On Fri, Nov 20, 2015 at 10:06:16AM +, Chris Wilson wrote: > On Fri, Nov 20, 2015 at 03:07:58PM +0530, Ankitprasad Sharma wrote: > > On Wed, 2015-11-18 at 10:59 +0100, Daniel Vetter wrote: > > > On Thu, Nov 05, 2015 at 05:15:59PM +0530, ankitprasad.r.sha...@intel.com > > > wrote: > > > > From: Ankitprasad Sharma > > > > > > > > In pwrite_fast, map an object page by page if obj_ggtt_pin fails. First, > > > > we try a nonblocking pin for the whole object (since that is fastest if > > > > reused), then failing that we try to grab one page in the mappable > > > > aperture. It also allows us to handle objects larger than the mappable > > > > aperture (e.g. if we need to pwrite with vGPU restricting the aperture > > > > to a measely 8MiB or something like that). > > > > > > We already have a fallback to the shmem pwrite. Why do we need this? > > This is mainly for the non-shmem backed objects, as we do not have > > fallback path for that. Agree for the shmem backed objects, as we > > already have a fallback. > > > > Would like to request Chris, if he can clarify further. > > Exactly that, with stolen we cannot use the shmem path so there exists > no fallback. In order to pwrite to stolen, the GTT path must be fully > capable. Ok, in that case this should probably be part of the stolen obj series, just for clarification. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation 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 01/12] drm/i915: Move disable_cxsr to the crtc_state.
Reviewed-by: Ander Conselvan de Oliveira
On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> intel_crtc->atomic will be removed later on, move this member
> to intel_crtc_state.
>
> Signed-off-by: Maarten Lankhorst
> ---
> drivers/gpu/drm/i915/intel_atomic.c | 1 +
> drivers/gpu/drm/i915/intel_display.c | 12 +++-
> drivers/gpu/drm/i915/intel_drv.h | 4 ++--
> 3 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index c4eadbc928b7..9f0638a37b6d 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -95,6 +95,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>
> crtc_state->update_pipe = false;
> crtc_state->disable_lp_wm = false;
> + crtc_state->disable_cxsr = false;
>
> return &crtc_state->base;
> }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index f8c332aee1e0..5ee64e67ad8a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4749,8 +4749,7 @@ static void intel_post_plane_update(struct intel_crtc
> *crtc)
>
> intel_frontbuffer_flip(dev, atomic->fb_bits);
>
> - if (atomic->disable_cxsr)
> - crtc->wm.cxsr_allowed = true;
> + crtc->wm.cxsr_allowed = true;
>
> if (crtc->atomic.update_wm_post)
> intel_update_watermarks(&crtc->base);
> @@ -4769,6 +4768,8 @@ static void intel_pre_plane_update(struct intel_crtc
> *crtc)
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
> + struct intel_crtc_state *pipe_config =
> + to_intel_crtc_state(crtc->base.state);
>
> if (atomic->disable_fbc)
> intel_fbc_disable_crtc(crtc);
> @@ -4779,7 +4780,7 @@ static void intel_pre_plane_update(struct intel_crtc
> *crtc)
> if (atomic->pre_disable_primary)
> intel_pre_disable_primary(&crtc->base);
>
> - if (atomic->disable_cxsr) {
> + if (pipe_config->disable_cxsr) {
> crtc->wm.cxsr_allowed = false;
> intel_set_memory_cxsr(dev_priv, false);
> }
> @@ -11658,6 +11659,7 @@ static bool needs_scaling(struct intel_plane_state
> *state)
> int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> struct drm_plane_state *plane_state)
> {
> + struct intel_crtc_state *pipe_config =
> to_intel_crtc_state(crtc_state);
> struct drm_crtc *crtc = crtc_state->crtc;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct drm_plane *plane = plane_state->plane;
> @@ -11708,7 +11710,7 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
> intel_crtc->atomic.update_wm_pre = true;
> /* must disable cxsr around plane enable/disable */
> if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> - intel_crtc->atomic.disable_cxsr = true;
> + pipe_config->disable_cxsr = true;
> /* to potentially re-enable cxsr */
> intel_crtc->atomic.wait_vblank = true;
> intel_crtc->atomic.update_wm_post = true;
> @@ -11719,7 +11721,7 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
> if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> if (is_crtc_enabled)
> intel_crtc->atomic.wait_vblank = true;
> - intel_crtc->atomic.disable_cxsr = true;
> + pipe_config->disable_cxsr = true;
> }
> } else if (intel_wm_need_update(plane, plane_state)) {
> intel_crtc->atomic.update_wm_pre = true;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 725973ebf49f..dd89342832e2 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -370,7 +370,8 @@ struct intel_crtc_state {
> #define PIPE_CONFIG_QUIRK_MODE_SYNC_FLAGS(1<<0) /* unreliable sync
> mode.flags */
> unsigned long quirks;
>
> - bool update_pipe;
> + bool update_pipe; /* can a fast modeset be performed? */
> + bool disable_cxsr;
>
> /* Pipe source size (ie. panel fitter input size)
>* All planes will be positioned inside this space,
> @@ -533,7 +534,6 @@ struct intel_crtc_atomic_commit {
> /* Sleepable operations to perform before commit */
> bool disable_fbc;
> bool disable_ips;
> - bool disable_cxsr;
> bool pre_disable_primary;
> bool update_wm_pre, update_wm_post;
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 10/12] drm/i915/gen9: Turn DC handling into a power well
On Tue, Nov 24, 2015 at 01:09:03AM +0200, Imre Deak wrote: > On Mon, 2015-11-23 at 14:58 -0800, Matt Roper wrote: > > On Mon, Nov 16, 2015 at 04:20:01PM +0100, Patrik Jakobsson wrote: > > > Handle DC off as a power well where enabling the power well will > > > prevent > > > the DMC to enter selected DC states (required around modesets and > > > Aux > > > A). Disabling the power well will allow DC states again. For now > > > the > > > highest DC state is DC6 for Skylake and DC5 for Broxton but will be > > > configurable for Skylake in a later patch. > > > > > > v2: Check both DC5 and DC6 bits in power well enabled function > > > (Ville) > > > v3: > > > - Remove unneeded DC_OFF case in skl_set_power_well() (Imre) > > > - Add PW2 dependency to DC_OFF (Imre) > > > v4: Put DC_OFF before PW2 in BXT power well array > > > > > > Signed-off-by: Patrik Jakobsson > > > Reviewed-by: Imre Deak > > > > I've been seeing a BXT regression on recent di-nightly where DPMS off > > causes the entire platform to power down[1] instead of just the > > display; > > my bisect lands on this commit as the culprit. Any idea what the > > cause > > could be? I can reproduce by either letting the system sit idle long > > enough at an fb console, or by doing an "xset dpms force off" in X. > > Unfortunately I don't have a functioning serial console on this > > platform, so I can't get any messages that may show up around the > > DPMS > > operation. I've attached my boot-time dmesg output in case that > > helps. > > > > Subsequent commits seem to depend on the changes here, so I haven't > > reverted this commit directly on di-nightly, but I confirmed that if > > I > > checkout this commit directly I see DPMS problems, whereas its HEAD~1 > > works as expected. > > The power well support on BXT is not stable atm, we need to apply at > least a similar set of fixes as we did for SKL. So for now I would > suggest disabling it, by booting with i915.disable_power_well=0 until > things are fixed. This should've been made the default option earlier, > I forgot about this. I will follow up with the patch to that extent. I guess we should pull in that patch asap. Other problem is that current igt tests aren't too good at obeying the dsi encoder/pipe restrictions, so atm pm_rpm just skips. That needs to be fixed too. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: PSR: Mask LPSP hw tracking back again.
On Mon, Nov 23, 2015 at 01:52:37PM -0800, Rodrigo Vivi wrote:
> Hi Daniel
>
> Would you please consider merging patches 2,3 and 4 from this series
> that are ready to get merged?
> They don't depend on patch 1 that is under review yet.
Done.
-Daniel
>
> Thanks,
>
> On Wed, Nov 18, 2015 at 1:49 PM, Rodrigo Vivi wrote:
> > When we introduced PSR we let LPSP masked allowing us to get PSR
> > independently from the audio runtime PM. However in one of the
> > attempts to get PSR enabled by default one user reported one specific
> > case where he would miss screen updates if scrolling the firefox in a
> > Gnome environment when i915 runtime pm was enabled. So for
> > this specific case that (I could never create an i-g-t test case)
> > we decided to remove the LPSP mask and let HW tracking taking care of
> > this case. The mask got removed later by my
> > commit 09108b90f04 ("drm/i915: PSR: Remove Low Power HW tracking mask.")
> >
> > So we started depending on audio driver again, what is bad.
> >
> > With previous commit
> > "drm/i915: PSR: Let's rely more on frontbuffer tracking."
> > we transfered the PSR exit responsability totally to SW frontbuffer
> > tracking. So now can safelly shut off a bit the HW tracking, or
> > at least this case that makes us to depend on other drivers.
> >
> > v2: Update commit message since this patch by itself doesn't solve
> > the bugzilla entries.
> >
> > v3: Another attempt to improve commit message.
> >
> > Cc: Paulo Zanoni
> > Tested-by: Brian Norris
> > Signed-off-by: Rodrigo Vivi
> > ---
> > drivers/gpu/drm/i915/intel_psr.c | 9 +++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index b0e343c..b1b88d1 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -400,9 +400,14 @@ void intel_psr_enable(struct intel_dp *intel_dp)
> > skl_psr_setup_su_vsc(intel_dp);
> > }
> >
> > - /* Avoid continuous PSR exit by masking memup and hpd */
> > + /*
> > +* Per Spec: Avoid continuous PSR exit by masking MEMUP and
> > HPD.
> > +* Also mask LPSP to avoid dependency on other drivers that
> > +* might block runtime_pm besides preventing other hw
> > tracking
> > +* issues now we can rely on frontbuffer tracking.
> > +*/
> > I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP
> > |
> > - EDP_PSR_DEBUG_MASK_HPD);
> > + EDP_PSR_DEBUG_MASK_HPD |
> > EDP_PSR_DEBUG_MASK_LPSP);
> >
> > /* Enable PSR on the panel */
> > hsw_psr_enable_sink(intel_dp);
> > --
> > 2.4.3
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
--
Daniel Vetter
Software Engineer, Intel Corporation
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: Remove PSR Perf Counter for SKL+
On Mon, Nov 23, 2015 at 02:16:40PM -0800, Rodrigo Vivi wrote:
> Whenever DMC firmware put the HW into DC State a bunch
> of registers including this perf counter is reset to 0.
>
> Even with PSR active and working we could still read
> "Performance_Counter: 0" what will misslead people to believe
> PSR is broken. For instance on SKL we can only see PC10
> residency with screen on if PSR is working properly.
> However Performance_Counter was showing 0.
>
> Even if it restored properly on DC6 exit we don't want to
> give users the wrong impression that PSR is not working
> while we know for sure it is.
>
> So, it is better to remove this counter information while
> we don't have a better way to track PSR residency.
>
> Signed-off-by: Rodrigo Vivi
> Reviewed-by: Durgadoss R
Queued for -next, thanks for the patch.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index b28da6f..a728ff1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2582,8 +2582,11 @@ static int i915_edp_psr_status(struct seq_file *m,
> void *data)
> }
> seq_puts(m, "\n");
>
> - /* CHV PSR has no kind of performance counter */
> - if (HAS_DDI(dev)) {
> + /*
> + * VLV/CHV PSR has no kind of performance counter
> + * SKL+ Perf counter is reset to 0 everytime DC state is entered
> + */
> + if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> psrperf = I915_READ(EDP_PSR_PERF_CNT) &
> EDP_PSR_PERF_CNT_MASK;
>
> --
> 2.4.3
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
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: Also disable PSR on Sink when disabling it on Source.
On Tue, Nov 24, 2015 at 04:40:08AM +, Jindal, Sonika wrote: > Reviewed-by: Sonika Jindal > > -Original Message- > From: Vivi, Rodrigo > Sent: Tuesday, November 24, 2015 3:50 AM > To: intel-gfx@lists.freedesktop.org > Cc: Vivi, Rodrigo; Jindal, Sonika > Subject: [PATCH] drm/i915: Also disable PSR on Sink when disabling it on > Source. > > It is not a bad idea to disable the PSR feature on Sink when we are disabling > on the Source. > > v2: Move dpcd write inside mutex protected area as suggested by Sonika. > > Cc: Sonika Jindal > Suggested-by: Sonika Jindal > Signed-off-by: Rodrigo Vivi Queued for -next, thanks for the patch. -Daniel > --- > drivers/gpu/drm/i915/intel_psr.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > b/drivers/gpu/drm/i915/intel_psr.c > index 2b2f84d..3bbb270 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -518,11 +518,15 @@ void intel_psr_disable(struct intel_dp *intel_dp) > return; > } > > + /* Disable PSR on Source */ > if (HAS_DDI(dev)) > hsw_psr_disable(intel_dp); > else > vlv_psr_disable(intel_dp); > > + /* Disable PSR on Sink */ > + drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); > + > dev_priv->psr.enabled = NULL; > mutex_unlock(&dev_priv->psr.lock); > > -- > 2.4.3 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Announcement] 2015-Q3 release of XenGT - a Mediated Graphics Passthrough Solution from Intel
Hi, > > Yes, vGPU may have additional features, like a framebuffer area, that > > aren't present or optional for direct assignment. Obviously we support > > direct assignment of GPUs for some vendors already without this feature. > > For exposing framebuffers for spice/vnc I highly recommend against > anything that looks like a bar/fixed mmio range mapping. First this means > the kernel driver needs to internally fake remapping, which isn't fun. Sure. I don't think we should remap here. More below. > My recoomendation is to build the actual memory access for underlying > framebuffers on top of dma-buf, so that it can be vacuumed up by e.g. the > host gpu driver again for rendering. We want that too ;) Some more background: OpenGL support in qemu is still young and emerging, and we are actually building on dma-bufs here. There are a bunch of different ways how guest display output is handled. At the end of the day it boils down to only two fundamental cases though: (a) Where qemu doesn't need access to the guest framebuffer - qemu directly renders via opengl (works today with virtio-gpu and will be in the qemu 2.5 release) - qemu passed on the dma-buf to spice client for local display (experimental code exists). - qemu feeds the guest display into gpu-assisted video encoder to send a stream over the network (no code yet). (b) Where qemu must read the guest framebuffer. - qemu's builtin vnc server. - qemu writing screenshots to file. - (non-opengl legacy code paths for local display, will hopefully disappear long-term though ...) So, the question is how to support (b) best. Even with OpenGL support in qemu improving over time I don't expect this going away completely anytime soon. I think it makes sense to have a special vfio region for that. I don't think remapping makes sense there. It doesn't need to be "live", it doesn't need support high refresh rates. Placing a copy of the guest framebuffer there on request (and convert from tiled to linear while being at it) is perfectly fine. qemu has a adaptive update rate and will stop doing frequent update requests when the vnc client disconnects, so there will be nothing to do if nobody wants actually see the guest display. Possible alternative approach would be to import a dma-buf, then use glReadPixels(). I suspect when doing the copy in the kernel the driver could ask just the gpu to blit the guest framebuffer. Don't know gfx hardware good enough to be sure though, comments are welcome. cheers, Gerd ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't override output type for DDI HDMI
On Fri, Nov 20, 2015 at 12:18:41PM +0100, Takashi Iwai wrote:
> On Thu, 19 Nov 2015 17:04:20 +0100,
> Takashi Iwai wrote:
> >
> > On Thu, 19 Nov 2015 16:51:05 +0100,
> > Daniel Vetter wrote:
> > >
> > > On Thu, Nov 19, 2015 at 12:09:56PM +0100, Takashi Iwai wrote:
> > > > Currently a DDI port may register the DP hotplug handler even though
> > > > it's used with HDMI, and the DP HPD handler overrides the encoder
> > > > type forcibly to DP. This caused the inconsistency on a machine
> > > > connected with a HDMI monitor; upon a hotplug event, the DDI port is
> > > > suddenly switched to be handled as a DP although the same monitor is
> > > > kept connected, and this leads to the erroneous blank output.
> > > >
> > > > This patch papers over the bug by excluding the previous HDMI encoder
> > > > type from this override. This should be fixed more fundamentally,
> > > > e.g. by moving the encoder type reset from the HPD or by having
> > > > individual encoder objects for HDMI and DP. But since the bug has
> > > > been present for a long time (3.17), it's better to have a
> > > > quick-n-dirty fix for now, and keep working on a cleaner fix.
> > > >
> > > > Bugzilla: http://bugzilla.opensuse.org/show_bug.cgi?id=955190
> > > > Fixes: 0e32b39ceed6 ('drm/i915: add DP 1.2 MST support (v0.7)')
> > > > Cc: # v3.17+
> > > > Signed-off-by: Takashi Iwai
> > >
> > > If you plug in a hmid screen (using a level-shifter adapter), then a DP
> > > screen, does still everything work?
> >
> > That was my slight concern, too, although it seems working as long as
> > I tested several HSW machines. Maybe better to double-check.
>
> I checked again some machines, and they all seem working.
>
> Actually the encoder type is set again in each detection function
> (intel_hdmi_detect(), intel_dp_detect()) no matter with or without my
> workaround. The problem happens when a HPD is triggered only to DP
> while the HDMI is kept on. Then intel_hdmi_detect() won't be called
> so the wrongly overridden encoder type remains.
>
> Why the override is still needed in intel_dp_hpd_pulse() isn't clear
> to me, but at least it influences on the port power domain handling.
> And my change won't affect in this regard.
My suspicion is that we need this to reset between
INTEL_OUTPUT_DISPLAYPORT and INTEL_OUTPUT_DP_MST.
Thanks for testing this. With the above explanation added to the commit
message: Reviewed-by: Daniel Vetter
Jani, can you please pick this up?
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
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 v1] drm/i915: Fix a false alert of memory leak when free LRC
On Tue, Nov 24, 2015 at 11:57:22AM +0100, Daniel Vetter wrote: > On Mon, Nov 23, 2015 at 10:34:59AM +, Tvrtko Ursulin wrote: > > > > On 20/11/15 08:31, Daniel Vetter wrote: > > >On Thu, Nov 19, 2015 at 04:10:26PM -0800, yu@intel.com wrote: > > >>From: Alex Dai > > >> > > >>There is a memory leak warning message from i915_gem_context_clean > > >>when GuC submission is enabled. The reason is that when LRC is > > >>released, its ppgtt could be still referenced. The assumption that > > >>all VMAs are unbound during release of LRC is not true. > > >> > > >>v1: Move the code inside i915_gem_context_clean() to where ppgtt is > > >>released because it is not cleaning context anyway but ppgtt. > > >> > > >>Signed-off-by: Alex Dai > > > > > >retire__read drops the ctx (and hence ppgtt) reference too early, > > >resulting in us hitting the WARNING. See the giant thread with Tvrtko, > > >Chris and me: > > > > > >http://www.spinics.net/lists/intel-gfx/msg78918.html > > > > > >Would be great if someone could test the diff I posted in there. > > > > It doesn't work - I have posted my IGT snippet which I thought explained it. > > > > Problem req unreference in obj->active case. When it hits that path it will > > not move the VMA to inactive and the intel_execlists_retire_requests will be > > the last unreference from the retire worker which will trigger the WARN. > > > > I posted an IGT which hits that -> > > http://patchwork.freedesktop.org/patch/65369/ > > > > And posted one give up on the active VMA mem leak patch -> > > http://patchwork.freedesktop.org/patch/65529/ > > Ok, I get things now. Fundamentally the problem is that we track active > per-obj, but we want it per-vma. In a way this patch just diggs us deeper > into that hole, which doesn't make me all too happy. Oh well. > > I'll pull in the warning removal. Just revert the incomplete patch, rather than the warning that the patch is suspect. -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 v1] drm/i915: Fix a false alert of memory leak when free LRC
On Tue, Nov 24, 2015 at 11:57:22AM +0100, Daniel Vetter wrote: > Ok, I get things now. Fundamentally the problem is that we track active > per-obj, but we want it per-vma. In a way this patch just diggs us deeper > into that hole, which doesn't make me all too happy. Oh well. Why not, I've been chasing reviewers for vma tracking for the last 15 months. -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: Remove incorrect warning in context cleanup
On Tue, Nov 24, 2015 at 11:58:22AM +0100, Daniel Vetter wrote: > On Fri, Nov 20, 2015 at 01:23:36PM +, Tvrtko Ursulin wrote: > > From: Tvrtko Ursulin > > > > Commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae > > Author: Tvrtko Ursulin > > Date: Mon Oct 5 13:26:36 2015 +0100 > > > > drm/i915: Clean up associated VMAs on context destruction > > > > Added a warning based on an incorrect assumption that all VMAs > > in a VM will be on the inactive list at the point last reference > > to a context and VM is dropped. > > > > This is not true because i915_gem_object_retire__read will not > > put VMA on the inactive list until all activities on the object > > in question (in all VMs) have been retired. > > > > As a consequence, whether or not a context/VM will be destroyed > > with its VMAs still on the active list, can depend on completely > > unrelated activities using the same object from a different > > context or engine. > > > > Signed-off-by: Tvrtko Ursulin > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92638 > > Testcase: igt/gem_request_retire/retire-vma-not-inactive > > Cc: Daniel Vetter > > Cc: Chris Wilson > > Cc: Michel Thierry > > Queued for -next, thanks for the patch. The WARN_ON is accurate though. The original patch fails to fix even the limited aspect of the bug it claimed to. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Solve IVB FDI bifurcation problems using atomic
From: Ville Syrjälä
Now that we have atomic modesetting, we can use it to solve the IVB
pipe B/C FDI lane sharing issue. That is, we can force a modeset on
pipe B if pipe C requires the FDI lanes currently used by pipe B.
To achieve this we do the following:
First stage:
- first compute the initial pipe config for each modeset crtc
- initial max FDI estimate will be 4 for pipe B, 2 for pipe C
- also need to run through the encoder .compute_config()
hooks once to update has_pch_encoder
- and we need to compute an initial number of required FDI lanes.
Note that we only need something that makes fdi_lanes > 0 speak
the truth, so there's no need to check against the max here
Second stage:
- Check if pipe C requires any FDI lanes, and based on that
update the max_fdi_lanes for both pipe B and pipe C
- Check if pipe B conflicts with the new requirements,
and if so, add it to the state forcing a modeset (in case
it already wasn't included)
Third stage:
- run through the rest of the pipe config computation, incuding
recomputing the FDI lane requirement and checking it against the
max
- this will signal that we should recompute things with a lower
bpp by returning -EAGAIN
- we keep doing this in a loop as long -EAGAIN is returned
If some other error occurs we bail out as usual
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/intel_display.c | 353 ++-
drivers/gpu/drm/i915/intel_drv.h | 1 +
2 files changed, 222 insertions(+), 132 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 68fb449ded77..082323c2aa8c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6492,87 +6492,120 @@ static int pipe_required_fdi_lanes(struct
intel_crtc_state *crtc_state)
return 0;
}
-static int ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
-struct intel_crtc_state *pipe_config)
+static void
+intel_modeset_pipe_config_start(struct intel_crtc *crtc,
+ struct intel_crtc_state *pipe_config);
+static int
+intel_modeset_pipe_config_continue(struct intel_crtc *crtc,
+ struct intel_crtc_state *pipe_config);
+
+static int intel_add_crtc_modeset_to_state(struct drm_atomic_state *state,
+ struct intel_crtc *crtc)
{
- struct drm_atomic_state *state = pipe_config->base.state;
- struct intel_crtc *other_crtc;
- struct intel_crtc_state *other_crtc_state;
+ struct intel_crtc_state *crtc_state;
+ int ret;
- DRM_DEBUG_KMS("checking fdi config on pipe %c, lanes %i\n",
- pipe_name(pipe), pipe_config->fdi_lanes);
- if (pipe_config->fdi_lanes > 4) {
- DRM_DEBUG_KMS("invalid fdi lane config on pipe %c: %i lanes\n",
- pipe_name(pipe), pipe_config->fdi_lanes);
- return -EINVAL;
- }
+ crtc_state = intel_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
- if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
- if (pipe_config->fdi_lanes > 2) {
- DRM_DEBUG_KMS("only 2 lanes on haswell, required: %i
lanes\n",
- pipe_config->fdi_lanes);
- return -EINVAL;
- } else {
- return 0;
- }
- }
+ crtc_state->base.mode_changed = true;
- if (INTEL_INFO(dev)->num_pipes == 2)
- return 0;
+ ret = drm_atomic_add_affected_connectors(state, &crtc->base);
+ if (ret)
+ return ret;
- /* Ivybridge 3 pipe is really complicated */
- switch (pipe) {
- case PIPE_A:
- return 0;
- case PIPE_B:
- if (pipe_config->fdi_lanes <= 2)
- return 0;
+ intel_modeset_pipe_config_start(crtc, crtc_state);
- other_crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev,
PIPE_C));
- other_crtc_state =
- intel_atomic_get_crtc_state(state, other_crtc);
- if (IS_ERR(other_crtc_state))
- return PTR_ERR(other_crtc_state);
+ /*
+* Must run through this at least once for every new
+* pipe to make sure has_pch_encoder and fdi_lanes are
+* set up to something half decent.
+*/
+ ret = intel_modeset_pipe_config_continue(crtc, crtc_state);
+ if (ret)
+ return ret;
- if (pipe_required_fdi_lanes(other_crtc_state) > 0) {
- DRM_DEBUG_KMS("invalid shared fdi lane config on pipe
%c: %i lanes\n",
- pipe_name(pipe), pipe_config->fdi_lanes);
- return -EINVAL;
- }
-
Re: [Intel-gfx] [PATCH i-g-t v2] lib/igt_kms: Introduce get_first_connected_output macro
On Fri, Nov 20, 2015 at 06:56:50PM -0800, Vivek Kasireddy wrote: > In some cases, we just need one valid (connected) output to perform > a test. This macro can help in these situations by not having to > put the test code inside a for loop that iterates over all the outputs. > > v2: Added a brief documentation for this macro. > > Suggested-by: Matt Roper > Cc: Thomas Wood > Signed-off-by: Vivek Kasireddy > --- > lib/igt_kms.h | 12 > 1 file changed, 12 insertions(+) > > diff --git a/lib/igt_kms.h b/lib/igt_kms.h > index 965c47c..a0bb066 100644 > --- a/lib/igt_kms.h > +++ b/lib/igt_kms.h > @@ -279,6 +279,18 @@ void igt_wait_for_vblank(int drm_fd, enum pipe pipe); > for (int i__ = 0; (plane) = &(display)->pipes[(pipe)].planes[i__], \ >i__ < (display)->pipes[(pipe)].n_planes; i__++) > > +/** > + * get_first_connected_output: > + * @display: Initialized igt_display_t type object > + * @output: igt_output_t type object > + * > + * Returns: First valid (connected) output. > + */ > +#define get_first_connected_output(display, output) \ > + for (int i__ = 0; i__ < (display)->n_outputs; i__++) \ > + if ((output = &(display)->outputs[i__]), output->valid) \ > + break Why is this a fragile macro instead of a simple function that just returns what we need? > + > #define IGT_FIXED(i,f) ((i) << 16 | (f)) > > void igt_enable_connectors(void); > -- > 2.4.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] drm/i915/guc: Move wait for GuC out of spinlock/unlock
On Mon, Nov 23, 2015 at 03:02:58PM -0800, yu@intel.com wrote:
> From: Alex Dai
>
> When GuC Work Queue is full, driver will wait GuC for avaliable
> space by delaying 1ms. The wait needs to be out of spinlockirq /
> unlock. Otherwise, lockup happens because jiffies won't be updated
> dur to irq is disabled.
>
> Issue is found in igt/gem_close_race.
>
> Signed-off-by: Alex Dai
> ---
> drivers/gpu/drm/i915/i915_guc_submission.c | 27 +--
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 0a6b007..1418397 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -201,10 +201,13 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
> union guc_doorbell_qw *db;
> void *base;
> int attempt = 2, ret = -EAGAIN;
> + unsigned long flags;
>
> base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
We don't need kmap_atomic anymore here now, since it's outside of the
spinlock.
> desc = base + gc->proc_desc_offset;
>
> + spin_lock_irqsave(&gc->wq_lock, flags);
Please don't use the super-generic _irqsave. It's expensive and results in
fragile code when someone accidentally reuses something in an interrupt
handler that was never meant to run in that context.
Instead please use the most specific funtion:
- spin_lock if you know you are in irq context.
- sipn_lock_irq if you know you are not.
- spin_lock_irqsave should be a big warning sign that your code has
layering issues.
Please audit the entire guc code for the above two issues.
> +
> /* Update the tail so it is visible to GuC */
> desc->tail = gc->wq_tail;
>
> @@ -248,7 +251,10 @@ static int guc_ring_doorbell(struct i915_guc_client *gc)
> db_exc.cookie = 1;
> }
>
> + spin_unlock_irqrestore(&gc->wq_lock, flags);
> +
> kunmap_atomic(base);
> +
> return ret;
> }
>
> @@ -487,16 +493,16 @@ static int guc_get_workqueue_space(struct
> i915_guc_client *gc, u32 *offset)
> struct guc_process_desc *desc;
> void *base;
> u32 size = sizeof(struct guc_wq_item);
> - int ret = 0, timeout_counter = 200;
> + int ret = -ETIMEDOUT, timeout_counter = 200;
> + unsigned long flags;
>
> base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> desc = base + gc->proc_desc_offset;
>
> while (timeout_counter-- > 0) {
> - ret = wait_for_atomic(CIRC_SPACE(gc->wq_tail, desc->head,
> - gc->wq_size) >= size, 1);
> + spin_lock_irqsave(&gc->wq_lock, flags);
>
> - if (!ret) {
> + if (CIRC_SPACE(gc->wq_tail, desc->head, gc->wq_size) >= size) {
> *offset = gc->wq_tail;
>
> /* advance the tail for next workqueue item */
> @@ -505,7 +511,13 @@ static int guc_get_workqueue_space(struct
> i915_guc_client *gc, u32 *offset)
>
> /* this will break the loop */
> timeout_counter = 0;
> + ret = 0;
> }
> +
> + spin_unlock_irqrestore(&gc->wq_lock, flags);
> +
> + if (timeout_counter)
> + usleep_range(1000, 2000);
Do we really not have a interrupt/signal from the guc when it has cleared
up some space?
> };
>
> kunmap_atomic(base);
> @@ -597,19 +609,17 @@ int i915_guc_submit(struct i915_guc_client *client,
> {
> struct intel_guc *guc = client->guc;
> enum intel_ring_id ring_id = rq->ring->id;
> - unsigned long flags;
> int q_ret, b_ret;
>
> /* Need this because of the deferred pin ctx and ring */
> /* Shall we move this right after ring is pinned? */
> lr_context_update(rq);
>
> - spin_lock_irqsave(&client->wq_lock, flags);
> -
> q_ret = guc_add_workqueue_item(client, rq);
> if (q_ret == 0)
> b_ret = guc_ring_doorbell(client);
>
> + spin_lock(&guc->host2guc_lock);
So at first I thought there's a race now, but then I looked at what
host2guc and wq_lock protect. It seems like the only thing they do is
protect against debugfs, all the real protection against inconsistent
state is done through dev->struct_mutex.
Can't we just rip out all this spinlock business from the guc code?
It would be easier than fixing up the races in here.
-Daniel
> client->submissions[ring_id] += 1;
> if (q_ret) {
> client->q_fail += 1;
> @@ -620,9 +630,6 @@ int i915_guc_submit(struct i915_guc_client *client,
> } else {
> client->retcode = 0;
> }
> - spin_unlock_irqrestore(&client->wq_lock, flags);
> -
> - spin_lock(&guc->host2guc_lock);
> guc->submissions[ring_id] += 1;
> guc->last_seqno[ring_id] = rq->seqno;
> spin_unlock(&guc->host2g
Re: [Intel-gfx] [PATCH] drm/i915: Tidy aliasing_gtt_bind_vma()
On Fri, Nov 20, 2015 at 10:27:18AM +, Chris Wilson wrote:
> In commit 0a878716265e9af9f697264dc2e858fcc060d833
> Author: Daniel Vetter
> Date: Thu Oct 15 14:23:01 2015 +0200
>
> drm/i915: restore ggtt double-bind avoidance
>
> we wrote the ggtt_bind_vma() observing a number of cleanups we could do
> over the template of aliasing_gtt_bind_vma(). Now let's apply the
> cleanups we made there back to the original. The essence is to avoid
> redundant variables and assignements, and by doing so make the code
> easier to read.
>
> Signed-off-by: Chris Wilson
> Cc: Daniel Vetter
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 19 +--
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 21667ad3c685..a09f8f0510d5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2561,32 +2561,31 @@ static int aliasing_gtt_bind_vma(struct i915_vma *vma,
>enum i915_cache_level cache_level,
>u32 flags)
> {
> - struct drm_device *dev = vma->vm->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_i915_gem_object *obj = vma->obj;
> - struct sg_table *pages = obj->pages;
> - u32 pte_flags = 0;
> + u32 pte_flags;
> int ret;
>
> ret = i915_get_ggtt_vma_pages(vma);
> if (ret)
> return ret;
> - pages = vma->ggtt_view.pages;
>
> /* Currently applicable only to VLV */
> - if (obj->gt_ro)
> + pte_flags = 0;
> + if (vma->obj->gt_ro)
> pte_flags |= PTE_READ_ONLY;
>
>
> if (flags & GLOBAL_BIND) {
> - vma->vm->insert_entries(vma->vm, pages,
> + vma->vm->insert_entries(vma->vm,
> + vma->ggtt_view.pages,
> vma->node.start,
> cache_level, pte_flags);
> }
>
> if (flags & LOCAL_BIND) {
> - struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
> - appgtt->base.insert_entries(&appgtt->base, pages,
> + struct i915_hw_ppgtt *appgtt =
> + to_i915(vma->vm->dev)->mm.aliasing_ppgtt;
> + appgtt->base.insert_entries(&appgtt->base,
> + vma->ggtt_view.pages,
> vma->node.start,
> cache_level, pte_flags);
> }
> --
> 2.6.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
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: Don't override output type for DDI HDMI
On Tue, Nov 24, 2015 at 01:44:46PM +0100, Daniel Vetter wrote:
> On Fri, Nov 20, 2015 at 12:18:41PM +0100, Takashi Iwai wrote:
> > On Thu, 19 Nov 2015 17:04:20 +0100,
> > Takashi Iwai wrote:
> > >
> > > On Thu, 19 Nov 2015 16:51:05 +0100,
> > > Daniel Vetter wrote:
> > > >
> > > > On Thu, Nov 19, 2015 at 12:09:56PM +0100, Takashi Iwai wrote:
> > > > > Currently a DDI port may register the DP hotplug handler even though
> > > > > it's used with HDMI, and the DP HPD handler overrides the encoder
> > > > > type forcibly to DP. This caused the inconsistency on a machine
> > > > > connected with a HDMI monitor; upon a hotplug event, the DDI port is
> > > > > suddenly switched to be handled as a DP although the same monitor is
> > > > > kept connected, and this leads to the erroneous blank output.
> > > > >
> > > > > This patch papers over the bug by excluding the previous HDMI encoder
> > > > > type from this override. This should be fixed more fundamentally,
> > > > > e.g. by moving the encoder type reset from the HPD or by having
> > > > > individual encoder objects for HDMI and DP. But since the bug has
> > > > > been present for a long time (3.17), it's better to have a
> > > > > quick-n-dirty fix for now, and keep working on a cleaner fix.
> > > > >
> > > > > Bugzilla: http://bugzilla.opensuse.org/show_bug.cgi?id=955190
> > > > > Fixes: 0e32b39ceed6 ('drm/i915: add DP 1.2 MST support (v0.7)')
> > > > > Cc: # v3.17+
> > > > > Signed-off-by: Takashi Iwai
> > > >
> > > > If you plug in a hmid screen (using a level-shifter adapter), then a DP
> > > > screen, does still everything work?
> > >
> > > That was my slight concern, too, although it seems working as long as
> > > I tested several HSW machines. Maybe better to double-check.
> >
> > I checked again some machines, and they all seem working.
> >
> > Actually the encoder type is set again in each detection function
> > (intel_hdmi_detect(), intel_dp_detect()) no matter with or without my
> > workaround. The problem happens when a HPD is triggered only to DP
> > while the HDMI is kept on. Then intel_hdmi_detect() won't be called
> > so the wrongly overridden encoder type remains.
> >
> > Why the override is still needed in intel_dp_hpd_pulse() isn't clear
> > to me, but at least it influences on the port power domain handling.
> > And my change won't affect in this regard.
>
> My suspicion is that we need this to reset between
> INTEL_OUTPUT_DISPLAYPORT and INTEL_OUTPUT_DP_MST.
INTEL_OUTPUT_DP_MST is the type for fake mst encoders, and those
don't have a .hpd_pulse() hook to begin with.
--
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: Remove incorrect warning in context cleanup
On 24/11/15 12:53, Chris Wilson wrote: On Tue, Nov 24, 2015 at 11:58:22AM +0100, Daniel Vetter wrote: On Fri, Nov 20, 2015 at 01:23:36PM +, Tvrtko Ursulin wrote: From: Tvrtko Ursulin Commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae Author: Tvrtko Ursulin Date: Mon Oct 5 13:26:36 2015 +0100 drm/i915: Clean up associated VMAs on context destruction Added a warning based on an incorrect assumption that all VMAs in a VM will be on the inactive list at the point last reference to a context and VM is dropped. This is not true because i915_gem_object_retire__read will not put VMA on the inactive list until all activities on the object in question (in all VMs) have been retired. As a consequence, whether or not a context/VM will be destroyed with its VMAs still on the active list, can depend on completely unrelated activities using the same object from a different context or engine. Signed-off-by: Tvrtko Ursulin Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92638 Testcase: igt/gem_request_retire/retire-vma-not-inactive Cc: Daniel Vetter Cc: Chris Wilson Cc: Michel Thierry Queued for -next, thanks for the patch. The WARN_ON is accurate though. The original patch fails to fix even the limited aspect of the bug it claimed to. That is not true. It only makes it a bit more limited, and not by its fault even. Even with that it makes things a bit better, not worse. And does not impede your VMA rewrite at all. For which I did offer help to review as you send out in manageable chunks. If it is not realistically possible to split it out and do in increments, then it would be more constructive to discuss how to do it than to keep it in limbo for 15 months, as you say, and use it as a reason to shoot down everything else. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/guc: Move wait for GuC out of spinlock/unlock
On ti, 2015-11-24 at 14:04 +0100, Daniel Vetter wrote:
> On Mon, Nov 23, 2015 at 03:02:58PM -0800, yu@intel.com wrote:
> > From: Alex Dai
> >
> > When GuC Work Queue is full, driver will wait GuC for avaliable
> > space by delaying 1ms. The wait needs to be out of spinlockirq /
> > unlock. Otherwise, lockup happens because jiffies won't be updated
> > dur to irq is disabled.
> >
> > Issue is found in igt/gem_close_race.
> >
> > Signed-off-by: Alex Dai
> > ---
> > drivers/gpu/drm/i915/i915_guc_submission.c | 27 +-
> > -
> > 1 file changed, 17 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> > b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index 0a6b007..1418397 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -201,10 +201,13 @@ static int guc_ring_doorbell(struct
> > i915_guc_client *gc)
> > union guc_doorbell_qw *db;
> > void *base;
> > int attempt = 2, ret = -EAGAIN;
> > + unsigned long flags;
> >
> > base = kmap_atomic(i915_gem_object_get_page(gc-
> > >client_obj, 0));
>
> We don't need kmap_atomic anymore here now, since it's outside of the
> spinlock.
>
> > desc = base + gc->proc_desc_offset;
> >
> > + spin_lock_irqsave(&gc->wq_lock, flags);
>
> Please don't use the super-generic _irqsave. It's expensive and
> results in
> fragile code when someone accidentally reuses something in an
> interrupt
> handler that was never meant to run in that context.
>
> Instead please use the most specific funtion:
> - spin_lock if you know you are in irq context.
> - sipn_lock_irq if you know you are not.
Right, and simply spin_lock() if the lock is not taken in IRQ context
ever.
> - spin_lock_irqsave should be a big warning sign that your code has
> layering issues.
>
> Please audit the entire guc code for the above two issues.
Agreed, it looks inconsistent atm: we do spin_lock(wq_lock) from
debugfs and spin_lock_irq(wq_lock) from i915_guc_submit(). Neither of
them are called from IRQ context AFAICS, in which case a simple
spin_lock() would do.
--Imre
> > +
> > /* Update the tail so it is visible to GuC */
> > desc->tail = gc->wq_tail;
> >
> > @@ -248,7 +251,10 @@ static int guc_ring_doorbell(struct
> > i915_guc_client *gc)
> > db_exc.cookie = 1;
> > }
> >
> > + spin_unlock_irqrestore(&gc->wq_lock, flags);
> > +
> > kunmap_atomic(base);
> > +
> > return ret;
> > }
> >
> > @@ -487,16 +493,16 @@ static int guc_get_workqueue_space(struct
> > i915_guc_client *gc, u32 *offset)
> > struct guc_process_desc *desc;
> > void *base;
> > u32 size = sizeof(struct guc_wq_item);
> > - int ret = 0, timeout_counter = 200;
> > + int ret = -ETIMEDOUT, timeout_counter = 200;
> > + unsigned long flags;
> >
> > base = kmap_atomic(i915_gem_object_get_page(gc-
> > >client_obj, 0));
> > desc = base + gc->proc_desc_offset;
> >
> > while (timeout_counter-- > 0) {
> > - ret = wait_for_atomic(CIRC_SPACE(gc->wq_tail,
> > desc->head,
> > - gc->wq_size) >= size, 1);
> > + spin_lock_irqsave(&gc->wq_lock, flags);
> >
> > - if (!ret) {
> > + if (CIRC_SPACE(gc->wq_tail, desc->head, gc-
> > >wq_size) >= size) {
> > *offset = gc->wq_tail;
> >
> > /* advance the tail for next workqueue
> > item */
> > @@ -505,7 +511,13 @@ static int guc_get_workqueue_space(struct
> > i915_guc_client *gc, u32 *offset)
> >
> > /* this will break the loop */
> > timeout_counter = 0;
> > + ret = 0;
> > }
> > +
> > + spin_unlock_irqrestore(&gc->wq_lock, flags);
> > +
> > + if (timeout_counter)
> > + usleep_range(1000, 2000);
>
> Do we really not have a interrupt/signal from the guc when it has
> cleared
> up some space?
>
> > };
> >
> > kunmap_atomic(base);
> > @@ -597,19 +609,17 @@ int i915_guc_submit(struct i915_guc_client
> > *client,
> > {
> > struct intel_guc *guc = client->guc;
> > enum intel_ring_id ring_id = rq->ring->id;
> > - unsigned long flags;
> > int q_ret, b_ret;
> >
> > /* Need this because of the deferred pin ctx and ring */
> > /* Shall we move this right after ring is pinned? */
> > lr_context_update(rq);
> >
> > - spin_lock_irqsave(&client->wq_lock, flags);
> > -
> > q_ret = guc_add_workqueue_item(client, rq);
> > if (q_ret == 0)
> > b_ret = guc_ring_doorbell(client);
> >
> > + spin_lock(&guc->host2guc_lock);
>
> So at first I thought there's a race now, but then I looked at what
> host2guc and wq_lock protect. It seems like the only thing they do is
> protect against debugfs, all the real protection against inconsistent
> state is done through dev->struct_mutex.
>
> Can't we just rip out all this
Re: [Intel-gfx] [PATCH] drm/i915: Remove incorrect warning in context cleanup
On Tue, Nov 24, 2015 at 01:17:57PM +, Tvrtko Ursulin wrote: > > On 24/11/15 12:53, Chris Wilson wrote: > >On Tue, Nov 24, 2015 at 11:58:22AM +0100, Daniel Vetter wrote: > >>On Fri, Nov 20, 2015 at 01:23:36PM +, Tvrtko Ursulin wrote: > >>>From: Tvrtko Ursulin > >>> > >>>Commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae > >>>Author: Tvrtko Ursulin > >>>Date: Mon Oct 5 13:26:36 2015 +0100 > >>> > >>> drm/i915: Clean up associated VMAs on context destruction > >>> > >>>Added a warning based on an incorrect assumption that all VMAs > >>>in a VM will be on the inactive list at the point last reference > >>>to a context and VM is dropped. > >>> > >>>This is not true because i915_gem_object_retire__read will not > >>>put VMA on the inactive list until all activities on the object > >>>in question (in all VMs) have been retired. > >>> > >>>As a consequence, whether or not a context/VM will be destroyed > >>>with its VMAs still on the active list, can depend on completely > >>>unrelated activities using the same object from a different > >>>context or engine. > >>> > >>>Signed-off-by: Tvrtko Ursulin > >>>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92638 > >>>Testcase: igt/gem_request_retire/retire-vma-not-inactive > >>>Cc: Daniel Vetter > >>>Cc: Chris Wilson > >>>Cc: Michel Thierry > >> > >>Queued for -next, thanks for the patch. > > > >The WARN_ON is accurate though. The original patch fails to fix even the > >limited aspect of the bug it claimed to. > > That is not true. It only makes it a bit more limited, and not by > its fault even. Even with that it makes things a bit better, not > worse. It makes the code worse for very limited improvement, for which we did not have a publically reported bug, i.e. the impact is very small. > And does not impede your VMA rewrite at all. For which I did offer > help to review as you send out in manageable chunks. I have been. -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: Remove incorrect warning in context cleanup
Hi, On 24 November 2015 at 13:27, Chris Wilson wrote: > On Tue, Nov 24, 2015 at 01:17:57PM +, Tvrtko Ursulin wrote: >> On 24/11/15 12:53, Chris Wilson wrote: >> >The WARN_ON is accurate though. The original patch fails to fix even the >> >limited aspect of the bug it claimed to. >> >> That is not true. It only makes it a bit more limited, and not by >> its fault even. Even with that it makes things a bit better, not >> worse. > > It makes the code worse for very limited improvement, for which we did > not have a publically reported bug, i.e. the impact is very small. I can get the person who reported it to me to raise a Bugzilla complaining about the WARN_ON if you like ... Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Announcement] 2015-Q3 release of XenGT - a Mediated Graphics Passthrough Solution from Intel
On Tue, Nov 24, 2015 at 01:38:55PM +0100, Gerd Hoffmann wrote: > Hi, > > > > Yes, vGPU may have additional features, like a framebuffer area, that > > > aren't present or optional for direct assignment. Obviously we support > > > direct assignment of GPUs for some vendors already without this feature. > > > > For exposing framebuffers for spice/vnc I highly recommend against > > anything that looks like a bar/fixed mmio range mapping. First this means > > the kernel driver needs to internally fake remapping, which isn't fun. > > Sure. I don't think we should remap here. More below. > > > My recoomendation is to build the actual memory access for underlying > > framebuffers on top of dma-buf, so that it can be vacuumed up by e.g. the > > host gpu driver again for rendering. > > We want that too ;) > > Some more background: > > OpenGL support in qemu is still young and emerging, and we are actually > building on dma-bufs here. There are a bunch of different ways how > guest display output is handled. At the end of the day it boils down to > only two fundamental cases though: > > (a) Where qemu doesn't need access to the guest framebuffer > - qemu directly renders via opengl (works today with virtio-gpu > and will be in the qemu 2.5 release) > - qemu passed on the dma-buf to spice client for local display > (experimental code exists). > - qemu feeds the guest display into gpu-assisted video encoder > to send a stream over the network (no code yet). > > (b) Where qemu must read the guest framebuffer. > - qemu's builtin vnc server. > - qemu writing screenshots to file. > - (non-opengl legacy code paths for local display, will > hopefully disappear long-term though ...) > > So, the question is how to support (b) best. Even with OpenGL support > in qemu improving over time I don't expect this going away completely > anytime soon. > > I think it makes sense to have a special vfio region for that. I don't > think remapping makes sense there. It doesn't need to be "live", it > doesn't need support high refresh rates. Placing a copy of the guest > framebuffer there on request (and convert from tiled to linear while > being at it) is perfectly fine. qemu has a adaptive update rate and > will stop doing frequent update requests when the vnc client > disconnects, so there will be nothing to do if nobody wants actually see > the guest display. > > Possible alternative approach would be to import a dma-buf, then use > glReadPixels(). I suspect when doing the copy in the kernel the driver > could ask just the gpu to blit the guest framebuffer. Don't know gfx > hardware good enough to be sure though, comments are welcome. Generally the kernel can't do gpu blts since the required massive state setup is only in the userspace side of the GL driver stack. But glReadPixels can do tricks for detiling, and if you use pixel buffer objects or something similar it'll even be amortized reasonably. But there's some work to add generic mmap support to dma-bufs, and for really simple case (where we don't have a gl driver to handle the dma-buf specially) for untiled framebuffers that would be all we need? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] About dealing with CSB.context element switch in execlist mode.
Hi Gurus: I'm wondering what's the right approach to deal with the context switch reason: element_switch? According to b-spec, one ELSP submission may include two elements, when one element is finished, HW will move to process next element, the previous context will be scheduled out with a "element_switch" context switch reason. I saw that i915 would try to start a new ELSP write which may contain two new elements when it found a "element_switch" CSB in the context switch handler. I'm a bit confused here, as HW may be still running a context at this time, I'm not sure if two new elements can be submitted at this time. So I think maybe my understanding about this context switch reason might be wrong. Anyone can educate me how to deal with the "element_switch" CSB? Thanks, Zhi. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Don't override output type for DDI HDMI
On Tue, Nov 24, 2015 at 03:08:17PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 24, 2015 at 01:44:46PM +0100, Daniel Vetter wrote:
> > On Fri, Nov 20, 2015 at 12:18:41PM +0100, Takashi Iwai wrote:
> > > On Thu, 19 Nov 2015 17:04:20 +0100,
> > > Takashi Iwai wrote:
> > > >
> > > > On Thu, 19 Nov 2015 16:51:05 +0100,
> > > > Daniel Vetter wrote:
> > > > >
> > > > > On Thu, Nov 19, 2015 at 12:09:56PM +0100, Takashi Iwai wrote:
> > > > > > Currently a DDI port may register the DP hotplug handler even though
> > > > > > it's used with HDMI, and the DP HPD handler overrides the encoder
> > > > > > type forcibly to DP. This caused the inconsistency on a machine
> > > > > > connected with a HDMI monitor; upon a hotplug event, the DDI port is
> > > > > > suddenly switched to be handled as a DP although the same monitor is
> > > > > > kept connected, and this leads to the erroneous blank output.
> > > > > >
> > > > > > This patch papers over the bug by excluding the previous HDMI
> > > > > > encoder
> > > > > > type from this override. This should be fixed more fundamentally,
> > > > > > e.g. by moving the encoder type reset from the HPD or by having
> > > > > > individual encoder objects for HDMI and DP. But since the bug has
> > > > > > been present for a long time (3.17), it's better to have a
> > > > > > quick-n-dirty fix for now, and keep working on a cleaner fix.
> > > > > >
> > > > > > Bugzilla: http://bugzilla.opensuse.org/show_bug.cgi?id=955190
> > > > > > Fixes: 0e32b39ceed6 ('drm/i915: add DP 1.2 MST support (v0.7)')
> > > > > > Cc: # v3.17+
> > > > > > Signed-off-by: Takashi Iwai
> > > > >
> > > > > If you plug in a hmid screen (using a level-shifter adapter), then a
> > > > > DP
> > > > > screen, does still everything work?
> > > >
> > > > That was my slight concern, too, although it seems working as long as
> > > > I tested several HSW machines. Maybe better to double-check.
> > >
> > > I checked again some machines, and they all seem working.
> > >
> > > Actually the encoder type is set again in each detection function
> > > (intel_hdmi_detect(), intel_dp_detect()) no matter with or without my
> > > workaround. The problem happens when a HPD is triggered only to DP
> > > while the HDMI is kept on. Then intel_hdmi_detect() won't be called
> > > so the wrongly overridden encoder type remains.
> > >
> > > Why the override is still needed in intel_dp_hpd_pulse() isn't clear
> > > to me, but at least it influences on the port power domain handling.
> > > And my change won't affect in this regard.
> >
> > My suspicion is that we need this to reset between
> > INTEL_OUTPUT_DISPLAYPORT and INTEL_OUTPUT_DP_MST.
>
> INTEL_OUTPUT_DP_MST is the type for fake mst encoders, and those
> don't have a .hpd_pulse() hook to begin with.
Hm ... that would mean we only need this for the initial hotplug when we
go _UNKOWN -> _DISPLAYPORT. Oh well, putting it into the state properly,
derived from what userspace wants, should be the proper fix anyway. But
for stable this is good enough.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915: Set connector_state->connector using the helper.
The atomic helper sets connector_state->connector, which the i915
code didn't. This will become a problem when we start using it.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/i915/intel_display.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 2917fef33f31..d1acdcba580f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6455,13 +6455,11 @@ static void intel_connector_check_state(struct
intel_connector *connector)
int intel_connector_init(struct intel_connector *connector)
{
- struct drm_connector_state *connector_state;
+ drm_atomic_helper_connector_reset(&connector->base);
- connector_state = kzalloc(sizeof *connector_state, GFP_KERNEL);
- if (!connector_state)
+ if (!connector->base.state)
return -ENOMEM;
- connector->base.state = connector_state;
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 3/3] drm/tegra: Use __drm_atomic_helper_reset_connector for subclassing connector state.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/tegra/dsi.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index f0a138ef68ce..33ad50487f2e 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -745,14 +745,11 @@ static void tegra_dsi_soft_reset(struct tegra_dsi *dsi)
static void tegra_dsi_connector_reset(struct drm_connector *connector)
{
- struct tegra_dsi_state *state;
+ struct tegra_dsi_state *state =
+ kzalloc(sizeof(*state), GFP_KERNEL);
kfree(connector->state);
- connector->state = NULL;
-
- state = kzalloc(sizeof(*state), GFP_KERNEL);
- if (state)
- connector->state = &state->base;
+ __drm_atomic_helper_connector_reset(connector, &state->base);
}
static struct drm_connector_state *
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/3] drm/atomic: Add __drm_atomic_helper_connector_reset.
This is useful for drivers that subclass connector_state, like tegra.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/drm_atomic_helper.c | 30 ++
include/drm/drm_atomic_helper.h | 2 ++
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index cee31d43cd1c..5d23f818faec 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2359,6 +2359,28 @@ void drm_atomic_helper_plane_destroy_state(struct
drm_plane *plane,
EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
/**
+ * __drm_atomic_helper_connector_reset - default ->reset hook for connectors
+ * @connector: drm connector
+ * @conn_state: connector state to assign
+ *
+ * Initializes conn_state and assigns it to connector->state, usually
+ * required at when initializing the drivers or when called from the
+ * ->reset hook.
+ *
+ * This is useful for drivers that subclass the connector state.
+ */
+void
+__drm_atomic_helper_connector_reset(struct drm_connector *connector,
+ struct drm_connector_state *conn_state)
+{
+ if (conn_state)
+ conn_state->connector = connector;
+
+ connector->state = conn_state;
+}
+EXPORT_SYMBOL(__drm_atomic_helper_connector_reset);
+
+/**
* drm_atomic_helper_connector_reset - default ->reset hook for connectors
* @connector: drm connector
*
@@ -2368,11 +2390,11 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
*/
void drm_atomic_helper_connector_reset(struct drm_connector *connector)
{
- kfree(connector->state);
- connector->state = kzalloc(sizeof(*connector->state), GFP_KERNEL);
+ struct drm_connector_state *conn_state =
+ kzalloc(sizeof(*conn_state), GFP_KERNEL);
- if (connector->state)
- connector->state->connector = connector;
+ kfree(connector->state);
+ __drm_atomic_helper_connector_reset(connector, conn_state);
}
EXPORT_SYMBOL(drm_atomic_helper_connector_reset);
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 8cba54a2a0a0..3e00faeea3d0 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -118,6 +118,8 @@ void __drm_atomic_helper_plane_destroy_state(struct
drm_plane *plane,
void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
struct drm_plane_state *state);
+void __drm_atomic_helper_connector_reset(struct drm_connector *connector,
+struct drm_connector_state
*conn_state);
void drm_atomic_helper_connector_reset(struct drm_connector *connector);
void
__drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector,
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Move the mb() following release-mmap into release-mmap
On Fri, Nov 20, 2015 at 10:31:38AM +, Chris Wilson wrote:
> As paranoia, we want to ensure that the CPU's PTEs have been revoked for
> the object before we return from i915_gem_release_mmap(). This allows us
> to rely on there being no outstanding memory accesses and guarantees
> serialisation of the code against concurrent access just by calling
> i915_gem_release_mmap().
>
> v2: Reduce the mb() into a wmb() following the revoke.
>
> Signed-off-by: Chris Wilson
> Cc: Tvrtko Ursulin
> Cc: "Goel, Akash" ---
> drivers/gpu/drm/i915/i915_gem.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index df234d00b376..09c829f38786 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1903,11 +1903,21 @@ out:
> void
> i915_gem_release_mmap(struct drm_i915_gem_object *obj)
> {
> + /* Serialisation between user GTT access and our code depends upon
> + * revoking the CPU's PTE whilst the mutex is held. The next user
> + * pagefault then has to wait until we release the mutex.
> + */
> + lockdep_assert_held(&obj->base.dev->struct_mutex);
lockdep_assert_held is a nop without lockdep, that's why I prefer
WARN_ON(!mutex_is_locked). Either way:
Reviewed-by: Daniel Vetter
> +
> if (!obj->fault_mappable)
> return;
>
> drm_vma_node_unmap(&obj->base.vma_node,
> obj->base.dev->anon_inode->i_mapping);
> +
> + /* Ensure that the CPU's PTE are revoked before we return */
> + wmb();
> +
> obj->fault_mappable = false;
> }
>
> @@ -3212,9 +3222,6 @@ static void i915_gem_object_finish_gtt(struct
> drm_i915_gem_object *obj)
> if ((obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0)
> return;
>
> - /* Wait for any direct GTT access to complete */
> - mb();
> -
> old_read_domains = obj->base.read_domains;
> old_write_domain = obj->base.write_domain;
>
> --
> 2.6.2
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
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 01/12] drm/i915: Convert i915_semaphores_is_enabled over to drm_i915_private
On Fri, Nov 20, 2015 at 12:43:41PM +, Chris Wilson wrote:
> We only need our private device struct for checking semapahore status,
> and to reduce churn later convert the parameter over now.
>
> Signed-off-by: Chris Wilson
Assuming gcc hasn't become unhappy:
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> drivers/gpu/drm/i915/i915_dma.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.c | 8
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/i915_gem.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_context.c | 2 +-
> drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
> drivers/gpu/drm/i915/intel_ringbuffer.c | 20 ++--
> 8 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 8890b115a2c0..359436162f3d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2859,7 +2859,7 @@ static int i915_semaphore_status(struct seq_file *m,
> void *unused)
> int num_rings = hweight32(INTEL_INFO(dev)->ring_mask);
> int i, j, ret;
>
> - if (!i915_semaphore_is_enabled(dev)) {
> + if (!i915_semaphore_is_enabled(dev_priv)) {
> seq_puts(m, "Semaphores are disabled\n");
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index cd79ef114b8e..d41bf214fc84 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -127,7 +127,7 @@ static int i915_getparam(struct drm_device *dev, void
> *data,
> value = 1;
> break;
> case I915_PARAM_HAS_SEMAPHORES:
> - value = i915_semaphore_is_enabled(dev);
> + value = i915_semaphore_is_enabled(dev_priv);
> break;
> case I915_PARAM_HAS_PRIME_VMAP_FLUSH:
> value = 1;
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 1d887459e37f..10c4cc2cece9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -512,9 +512,9 @@ void intel_detect_pch(struct drm_device *dev)
> pci_dev_put(pch);
> }
>
> -bool i915_semaphore_is_enabled(struct drm_device *dev)
> +bool i915_semaphore_is_enabled(struct drm_i915_private *dev_priv)
> {
> - if (INTEL_INFO(dev)->gen < 6)
> + if (INTEL_INFO(dev_priv)->gen < 6)
> return false;
>
> if (i915.semaphores >= 0)
> @@ -525,12 +525,12 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
> return false;
>
> /* Until we get further testing... */
> - if (IS_GEN8(dev))
> + if (IS_GEN8(dev_priv))
> return false;
>
> #ifdef CONFIG_INTEL_IOMMU
> /* Enable semaphores on SNB when IO remapping is off */
> - if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped)
> + if (INTEL_INFO(dev_priv)->gen == 6 && intel_iommu_gfx_mapped)
> return false;
> #endif
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 343a0a723d2c..fadf2ceb1f72 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3311,7 +3311,7 @@ extern void intel_detect_pch(struct drm_device *dev);
> extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
> extern int intel_enable_rc6(const struct drm_device *dev);
>
> -extern bool i915_semaphore_is_enabled(struct drm_device *dev);
> +extern bool i915_semaphore_is_enabled(struct drm_i915_private *dev_priv);
> int i915_reg_read_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file);
> int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 69d8d5fc750b..e6ac049a4698 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3150,7 +3150,7 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
> if (i915_gem_request_completed(from_req, true))
> return 0;
>
> - if (!i915_semaphore_is_enabled(obj->base.dev)) {
> + if (!i915_semaphore_is_enabled(to_i915(obj->base.dev))) {
> struct drm_i915_private *i915 = to_i915(obj->base.dev);
> ret = __i915_wait_request(from_req,
>
> atomic_read(&i915->gpu_error.reset_counter),
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index f2f1e913f17a..55d2985c1dbb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -483,7 +483,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32
> hw_flags)
> u32 flags = hw_flags | MI_MM_SPACE_GTT;
> const int num_rings =
> /* Use an extended w/a on ivb+ if signalling from
Re: [Intel-gfx] [PATCH] drm/i915: Remove incorrect warning in context cleanup
On 24/11/15 13:27, Chris Wilson wrote: On Tue, Nov 24, 2015 at 01:17:57PM +, Tvrtko Ursulin wrote: On 24/11/15 12:53, Chris Wilson wrote: On Tue, Nov 24, 2015 at 11:58:22AM +0100, Daniel Vetter wrote: On Fri, Nov 20, 2015 at 01:23:36PM +, Tvrtko Ursulin wrote: From: Tvrtko Ursulin Commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae Author: Tvrtko Ursulin Date: Mon Oct 5 13:26:36 2015 +0100 drm/i915: Clean up associated VMAs on context destruction Added a warning based on an incorrect assumption that all VMAs in a VM will be on the inactive list at the point last reference to a context and VM is dropped. This is not true because i915_gem_object_retire__read will not put VMA on the inactive list until all activities on the object in question (in all VMs) have been retired. As a consequence, whether or not a context/VM will be destroyed with its VMAs still on the active list, can depend on completely unrelated activities using the same object from a different context or engine. Signed-off-by: Tvrtko Ursulin Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92638 Testcase: igt/gem_request_retire/retire-vma-not-inactive Cc: Daniel Vetter Cc: Chris Wilson Cc: Michel Thierry Queued for -next, thanks for the patch. The WARN_ON is accurate though. The original patch fails to fix even the limited aspect of the bug it claimed to. That is not true. It only makes it a bit more limited, and not by its fault even. Even with that it makes things a bit better, not worse. It makes the code worse for very limited improvement, for which we did not have a publically reported bug, i.e. the impact is very small. Well impact was huge for Android userspace but you are probably right that BZ was not created for that. It was somewhat related to https://bugs.freedesktop.org/show_bug.cgi?id=87477 on small memory configurations if I remember correctly. Although that hasn't been correctly captured in there or a new entry forked off. We have on the other hand added an IGT for it gem_ppgtt/flink-and-close-vma-leak so I don't think your argument is fair. Especially if the rewrite of it all is imminent - so the worse code, even if you think it is so much worse which I disagree with, is only in there temporary. And the memory leak was real even with fbcon and Xorg which I am sure you know. And does not impede your VMA rewrite at all. For which I did offer help to review as you send out in manageable chunks. I have been. And I have reviewed some, no? Feel free to ping me if I missed some. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Move the mb() following release-mmap into release-mmap
On Tue, Nov 24, 2015 at 02:36:43PM +0100, Daniel Vetter wrote:
> On Fri, Nov 20, 2015 at 10:31:38AM +, Chris Wilson wrote:
> > As paranoia, we want to ensure that the CPU's PTEs have been revoked for
> > the object before we return from i915_gem_release_mmap(). This allows us
> > to rely on there being no outstanding memory accesses and guarantees
> > serialisation of the code against concurrent access just by calling
> > i915_gem_release_mmap().
> >
> > v2: Reduce the mb() into a wmb() following the revoke.
> >
> > Signed-off-by: Chris Wilson
> > Cc: Tvrtko Ursulin
> > Cc: "Goel, Akash" > ---
> > drivers/gpu/drm/i915/i915_gem.c | 13 ++---
> > 1 file changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index df234d00b376..09c829f38786 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1903,11 +1903,21 @@ out:
> > void
> > i915_gem_release_mmap(struct drm_i915_gem_object *obj)
> > {
> > + /* Serialisation between user GTT access and our code depends upon
> > +* revoking the CPU's PTE whilst the mutex is held. The next user
> > +* pagefault then has to wait until we release the mutex.
> > +*/
> > + lockdep_assert_held(&obj->base.dev->struct_mutex);
>
> lockdep_assert_held is a nop without lockdep, that's why I prefer
> WARN_ON(!mutex_is_locked). Either way:
I thought someone proposed adding i915_assert_held() or something
at one point? Or did I just imagine it?
>
> Reviewed-by: Daniel Vetter
>
> > +
> > if (!obj->fault_mappable)
> > return;
> >
> > drm_vma_node_unmap(&obj->base.vma_node,
> >obj->base.dev->anon_inode->i_mapping);
> > +
> > + /* Ensure that the CPU's PTE are revoked before we return */
> > + wmb();
> > +
> > obj->fault_mappable = false;
> > }
> >
> > @@ -3212,9 +3222,6 @@ static void i915_gem_object_finish_gtt(struct
> > drm_i915_gem_object *obj)
> > if ((obj->base.read_domains & I915_GEM_DOMAIN_GTT) == 0)
> > return;
> >
> > - /* Wait for any direct GTT access to complete */
> > - mb();
> > -
> > old_read_domains = obj->base.read_domains;
> > old_write_domain = obj->base.write_domain;
> >
> > --
> > 2.6.2
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> ___
> 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 02/12] drm/i915: Use the new rq->i915 field where appropriate
On Fri, Nov 20, 2015 at 12:43:42PM +, Chris Wilson wrote:
> In a few frequent cases, having a direct pointer to the drm_i915_private
> from the request is very useful.
>
> Signed-off-by: Chris Wilson
req->i915 is a bit inconsistent imo, i915_dev would be better. Anyway, not
something you started, just makes the code a bit harder to speed-read ;-)
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_gem.c| 22 +++---
> drivers/gpu/drm/i915/i915_gem_context.c| 23 +--
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +--
> drivers/gpu/drm/i915/intel_lrc.c | 8 +++
> drivers/gpu/drm/i915/intel_pm.c| 3 +--
> drivers/gpu/drm/i915/intel_ringbuffer.c| 36
> +-
> 6 files changed, 39 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e6ac049a4698..5c3d11d020f0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1221,8 +1221,7 @@ int __i915_wait_request(struct drm_i915_gem_request
> *req,
> struct intel_rps_client *rps)
> {
> struct intel_engine_cs *ring = i915_gem_request_get_ring(req);
> - struct drm_device *dev = ring->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_i915_private *dev_priv = req->i915;
> const bool irq_test_in_progress =
> ACCESS_ONCE(dev_priv->gpu_error.test_irq_rings) &
> intel_ring_flag(ring);
> int state = interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
> @@ -1336,7 +1335,6 @@ out:
> int i915_gem_request_add_to_client(struct drm_i915_gem_request *req,
> struct drm_file *file)
> {
> - struct drm_i915_private *dev_private;
> struct drm_i915_file_private *file_priv;
>
> WARN_ON(!req || !file || req->file_priv);
> @@ -1347,7 +1345,6 @@ int i915_gem_request_add_to_client(struct
> drm_i915_gem_request *req,
> if (req->file_priv)
> return -EINVAL;
>
> - dev_private = req->ring->dev->dev_private;
> file_priv = file->driver_priv;
>
> spin_lock(&file_priv->mm.lock);
> @@ -1425,18 +1422,16 @@ __i915_gem_request_retire__upto(struct
> drm_i915_gem_request *req)
> int
> i915_wait_request(struct drm_i915_gem_request *req)
> {
> - struct drm_device *dev;
> struct drm_i915_private *dev_priv;
> bool interruptible;
> int ret;
>
> BUG_ON(req == NULL);
>
> - dev = req->ring->dev;
> - dev_priv = dev->dev_private;
> + dev_priv = req->i915;
> interruptible = dev_priv->mm.interruptible;
>
> - BUG_ON(!mutex_is_locked(&dev->struct_mutex));
> + BUG_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex));
>
> ret = i915_gem_check_wedge(&dev_priv->gpu_error, interruptible);
> if (ret)
> @@ -2568,7 +2563,7 @@ void __i915_add_request(struct drm_i915_gem_request
> *request,
> return;
>
> ring = request->ring;
> - dev_priv = ring->dev->dev_private;
> + dev_priv = request->i915;
> ringbuf = request->ringbuf;
>
> /*
> @@ -3150,8 +3145,8 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
> if (i915_gem_request_completed(from_req, true))
> return 0;
>
> - if (!i915_semaphore_is_enabled(to_i915(obj->base.dev))) {
> - struct drm_i915_private *i915 = to_i915(obj->base.dev);
> + if (!i915_semaphore_is_enabled(from_req->i915)) {
> + struct drm_i915_private *i915 = from_req->i915;
> ret = __i915_wait_request(from_req,
>
> atomic_read(&i915->gpu_error.reset_counter),
> i915->mm.interruptible,
> @@ -4648,13 +4643,12 @@ err:
> int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice)
> {
> struct intel_engine_cs *ring = req->ring;
> - struct drm_device *dev = ring->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_i915_private *dev_priv = req->i915;
> u32 reg_base = GEN7_L3LOG_BASE + (slice * 0x200);
> u32 *remap_info = dev_priv->l3_parity.remap_info[slice];
> int i, ret;
>
> - if (!HAS_L3_DPF(dev) || !remap_info)
> + if (!HAS_L3_DPF(dev_priv) || !remap_info)
> return 0;
>
> ret = intel_ring_begin(req, GEN7_L3LOG_SIZE / 4 * 3);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 55d2985c1dbb..82a9f7197d32 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -483,8 +483,8 @@ mi_set_context(struct drm_i915_gem_request *req, u32
> hw_flags)
> u32 flags = hw_flags | MI_MM_SPACE_GTT;
> const int num_rings =
> /* Use an extended w/a on ivb+ if signalling from other rings */
> - i915_semaph
Re: [Intel-gfx] [PATCH] drm/i915: Remove incorrect warning in context cleanup
On Tue, Nov 24, 2015 at 01:29:07PM +, Daniel Stone wrote: > Hi, > > On 24 November 2015 at 13:27, Chris Wilson wrote: > > On Tue, Nov 24, 2015 at 01:17:57PM +, Tvrtko Ursulin wrote: > >> On 24/11/15 12:53, Chris Wilson wrote: > >> >The WARN_ON is accurate though. The original patch fails to fix even the > >> >limited aspect of the bug it claimed to. > >> > >> That is not true. It only makes it a bit more limited, and not by > >> its fault even. Even with that it makes things a bit better, not > >> worse. > > > > It makes the code worse for very limited improvement, for which we did > > not have a publically reported bug, i.e. the impact is very small. > > I can get the person who reported it to me to raise a Bugzilla > complaining about the WARN_ON if you like ... This is about the original bug, for with the bugfix resulted in the WARN_ON now being removed here. The underlying problem (I think, it's a maze) is that our vma active tracking is a bit ... underwhelming. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation 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 02/12] drm/i915: Calculate watermark related members in the crtc_state, v3.
On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> This removes pre/post_wm_update from intel_crtc->atomic, and
> creates atomic state for it in intel_crtc.
>
> Changes since v1:
> - Rebase on top of wm changes.
> Changes since v2:
> - Split disable_cxsr into a separate patch.
>
> Signed-off-by: Maarten Lankhorst
> ---
> drivers/gpu/drm/i915/intel_atomic.c | 1 +
> drivers/gpu/drm/i915/intel_display.c | 31 +--
> drivers/gpu/drm/i915/intel_drv.h | 2 +-
> 3 files changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index 9f0638a37b6d..4625f8a9ba12 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -96,6 +96,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> crtc_state->update_pipe = false;
> crtc_state->disable_lp_wm = false;
> crtc_state->disable_cxsr = false;
> + crtc_state->wm_changed = false;
>
> return &crtc_state->base;
> }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 5ee64e67ad8a..db4995406277 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4741,6 +4741,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
> static void intel_post_plane_update(struct intel_crtc *crtc)
> {
> struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
> + struct intel_crtc_state *pipe_config =
> + to_intel_crtc_state(crtc->base.state);
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> @@ -4751,7 +4753,7 @@ static void intel_post_plane_update(struct intel_crtc
> *crtc)
>
> crtc->wm.cxsr_allowed = true;
>
> - if (crtc->atomic.update_wm_post)
> + if (pipe_config->wm_changed)
> intel_update_watermarks(&crtc->base);
This adds an extra call to intel_update_watermarks() for the case where
previously update_wm_pre would be set. This won't cause extra register writes
because of the dirty check, but I think it deserves a note in the commit
message.
>
> if (atomic->update_fbc)
> @@ -4784,6 +4786,9 @@ static void intel_pre_plane_update(struct intel_crtc
> *crtc)
> crtc->wm.cxsr_allowed = false;
> intel_set_memory_cxsr(dev_priv, false);
> }
> +
> + if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
> + intel_update_watermarks(&crtc->base);
> }
>
> static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned
> plane_mask)
> @@ -11706,25 +11711,18 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>plane->base.id, was_visible, visible,
>turn_off, turn_on, mode_changed);
>
> - if (turn_on) {
> - intel_crtc->atomic.update_wm_pre = true;
> - /* must disable cxsr around plane enable/disable */
> - if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> - pipe_config->disable_cxsr = true;
> - /* to potentially re-enable cxsr */
> - intel_crtc->atomic.wait_vblank = true;
> - intel_crtc->atomic.update_wm_post = true;
> - }
> - } else if (turn_off) {
> - intel_crtc->atomic.update_wm_post = true;
> + if (turn_on || turn_off) {
> + pipe_config->wm_changed = true;
> +
> /* must disable cxsr around plane enable/disable */
> if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> if (is_crtc_enabled)
> intel_crtc->atomic.wait_vblank = true;
> pipe_config->disable_cxsr = true;
> }
> - } else if (intel_wm_need_update(plane, plane_state)) {
> - intel_crtc->atomic.update_wm_pre = true;
> + } else if ((was_visible || visible) &&
So this avoids watermark changes when the plane is not visible before or after
the update. Wouldn't it be better to fix intel_wm_need_update() instead if
returns true in that case?
Ander
> +intel_wm_need_update(plane, plane_state)) {
> + pipe_config->wm_changed = true;
> }
>
> if (visible || was_visible)
> @@ -11869,7 +11867,7 @@ static int intel_crtc_atomic_check(struct drm_crtc
> *crtc,
> }
>
> if (mode_changed && !crtc_state->active)
> - intel_crtc->atomic.update_wm_post = true;
> + pipe_config->wm_changed = true;
>
> if (mode_changed && crtc_state->enable &&
> dev_priv->display.crtc_compute_clock &&
> @@ -13762,9 +13760,6 @@ static void intel_begin_crtc_commit(struct drm_crtc
> *crtc,
> to_intel_crtc_state(old_crtc_state);
> bool modeset = needs_modeset(crtc->state);
>
> - if (intel_crtc->atomic.update_wm_pre)
> -
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Suppress spurious CPU FIFO underruns on ILK-IVB
On Mon, Nov 23, 2015 at 04:55:17PM +0200, Ville Syrjälä wrote: > On Mon, Nov 23, 2015 at 02:28:29PM +, Chris Wilson wrote: > > On Mon, Nov 23, 2015 at 04:22:08PM +0200, Ville Syrjälä wrote: > > > On Sat, Nov 21, 2015 at 10:51:50AM +, Chris Wilson wrote: > > > > On Fri, Nov 20, 2015 at 10:09:18PM +0200, ville.syrj...@linux.intel.com > > > > wrote: > > > > > From: Ville Syrjälä > > > > > > > > > > We still get spurious pipe underruns on ILK/SNB/IVB under two > > > > > circumstances when dealing with PCH ports: > > > > > * When the pipe has been disabled, but FDI RX/TX is still enabled > > > > > * During FDI link training > > > > > > > > > > Both cases seem to happen at least when we do VGA+HDMI cloning > > > > > from the same pipe. I don't think I've seen them when not cloning, > > > > > but can't be 100% sure. > > > > > > > > > > Disable underrun reporting around those places to eliminate the > > > > > dmesg errors. > > > > > > > > > > Testcase: igt/kms_setmode/basic-clone-single-crtc > > > > > Signed-off-by: Ville Syrjälä > > > > Acked-by: Chris Wilson > > > > > > > > I wondered if logging the suppressed errors would be of any use? > > > > > > Hmm. Maybe just to confirm that they still happen, and thus suppressing > > > is still neded. > > > > > > > Does > > > > the check_cpu_fifo work if the reporting is disabled? Could we do a > > > > manual check and DRM_DEBUG_KMS() if the enable did generate a failure. > > > > If the check_cpu_fifo does work, won't we still get the error from the > > > > added check in atomic_commit()? > > > > > > The check only looks at pipes that have underrun reporting enabled. I > > > suppose it might be possible to have it check all the pipes. At the > > > point where we call it no explicit suppression should be happening, so > > > the only reason why underrun reporting would be disabled on any pipe is > > > due to detecting a previous underrun via the interrupt. > > > > Does the hw flag the underrun even if the interrupt is disabled? > > (Playing games with PIPESTAT_IER?) Could we then report (debug) that an > > underrun happened before we re-enable reporting? > > Hmm. We use the IMR to disable this stuff on many platforms, so we > do lose the IIR. So we'd need to frob IER instead to make this work > universally. Currently it would only work for GMCH, IVB/HSW, > CPT/PPT/LPT/WPT since PIPESTAT/ERR_INT/SERR_INT don't have IMR. Feels a bit too risky imo, changing all that code. Also, even in tests where underruns reproduce readily it's not 100%, so the occasional debug message won't be solid enough evidence. For me I think as long as we hide underruns only while doing modesets (where there's some expectation of them happening) we should be perfectly fine wrt test coverage. The real benefit is when enabling/disabling planes, fbc and all that stuff. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Announcement] 2015-Q3 release of XenGT - a Mediated Graphics Passthrough Solution from Intel
Hi, > But there's some work to add generic mmap support to dma-bufs, and for > really simple case (where we don't have a gl driver to handle the dma-buf > specially) for untiled framebuffers that would be all we need? Not requiring gl is certainly a bonus, people might want build qemu without opengl support to reduce the attach surface and/or package dependency chain. And, yes, requirements for the non-gl rendering path are pretty low. qemu needs something it can mmap, and which it can ask pixman to handle. Preferred format is PIXMAN_x8r8g8b8 (qemu uses that internally in alot of places so this avoids conversions). Current plan is to have a special vfio region (not visible to the guest) where the framebuffer lives, with one or two pages at the end for meta data (format and size). Status field is there too and will be used by qemu to request updates and the kernel to signal update completion. Guess I should write that down as vfio rfc patch ... I don't think it makes sense to have fields to notify qemu about which framebuffer regions have been updated, I'd expect with full-screen composing we have these days this information isn't available anyway. Maybe a flag telling whenever there have been updates or not, so qemu can skip update processing in case we have the screensaver showing a black screen all day long. cheers, Gerd ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Check for underruns after crtc disable
On Mon, Nov 23, 2015 at 02:42:21PM +, Chris Wilson wrote: > On Mon, Nov 23, 2015 at 04:10:32PM +0200, Ville Syrjälä wrote: > > On Sat, Nov 21, 2015 at 10:49:04AM +, Chris Wilson wrote: > > > On Fri, Nov 20, 2015 at 10:09:20PM +0200, ville.syrj...@linux.intel.com > > > wrote: > > > > From: Ville Syrjälä > > > > > > > > To get a better idea if underruns occurred during crtc disabling, > > > > let's check for them explicitly. This helps in cases where the > > > > error interrupt isn't active, or there is no underrun interrupt > > > > support at all. > > > > > > > > Signed-off-by: Ville Syrjälä > > > > > > Would this be better the vblank after enabling? > > > > We do that too. > > Oh, crtc disabling. I can't read. Hm, should we also double-check before disabling, to catch anything that happened right before the modeset and avoid confusing it with underruns happening during the modeset? But this is a good idea already. Reviewed-by: Daniel Vetter -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Suppress spurious CPU FIFO underruns on ILK-IVB
On Fri, Nov 20, 2015 at 10:09:18PM +0200, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä
>
> We still get spurious pipe underruns on ILK/SNB/IVB under two
> circumstances when dealing with PCH ports:
> * When the pipe has been disabled, but FDI RX/TX is still enabled
> * During FDI link training
>
> Both cases seem to happen at least when we do VGA+HDMI cloning
> from the same pipe. I don't think I've seen them when not cloning,
> but can't be 100% sure.
>
> Disable underrun reporting around those places to eliminate the
> dmesg errors.
>
> Testcase: igt/kms_setmode/basic-clone-single-crtc
> Signed-off-by: Ville Syrjälä
hsw seems to have a similar problem when both vga and hdmi are used, even
when not cloned. Bad thing is that somehow the underrun reporting gets
into a non-recoverable state and stuck there, so that no underrun and also
nothing else (specifically crc irqs, that's why bat notices) work any
more.
Might be worth a shot to try the same trick there. Of course we still need
to figure out why it can't recover from this, too.
Anyway just an aside, ack on the entire series.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 20 +++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 68fb449ded77..8a8104b7947d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4137,6 +4137,12 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
> I915_WRITE(FDI_RX_TUSIZE1(pipe),
> I915_READ(PIPE_DATA_M1(pipe)) & TU_SIZE_MASK);
>
> + /*
> + * Sometimes spurious CPU pipe underruns happen during FDI
> + * training, at least with VGA+HDMI cloning. Suppress them.
> + */
> + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
> +
> /* For PCH output, training FDI link */
> dev_priv->display.fdi_link_train(crtc);
>
> @@ -4170,6 +4176,8 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
>
> intel_fdi_normal_train(crtc);
>
> + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> +
> /* For PCH DP, enable TRANS_DP_CTL */
> if (HAS_PCH_CPT(dev) && intel_crtc->config->has_dp_encoder) {
> const struct drm_display_mode *adjusted_mode =
> @@ -5062,12 +5070,22 @@ static void ironlake_crtc_disable(struct drm_crtc
> *crtc)
> drm_crtc_vblank_off(crtc);
> assert_vblank_disabled(crtc);
>
> + /*
> + * Sometimes spurious CPU pipe underruns happen when the
> + * pipe is already disabled, but FDI RX/TX is still enabled.
> + * Happens at least with VGA+HDMI cloning. Suppress them.
> + */
> + if (intel_crtc->config->has_pch_encoder)
> + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
> +
> intel_disable_pipe(intel_crtc);
>
> ironlake_pfit_disable(intel_crtc, false);
>
> - if (intel_crtc->config->has_pch_encoder)
> + if (intel_crtc->config->has_pch_encoder) {
> ironlake_fdi_disable(crtc);
> + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> + }
>
> for_each_encoder_on_crtc(dev, crtc, encoder)
> if (encoder->post_disable)
> --
> 2.4.10
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
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: Remove incorrect warning in context cleanup
Hey, On 24 November 2015 at 13:59, Daniel Vetter wrote: > On Tue, Nov 24, 2015 at 01:29:07PM +, Daniel Stone wrote: >> On 24 November 2015 at 13:27, Chris Wilson wrote: >> > On Tue, Nov 24, 2015 at 01:17:57PM +, Tvrtko Ursulin wrote: >> >> On 24/11/15 12:53, Chris Wilson wrote: >> >> >The WARN_ON is accurate though. The original patch fails to fix even the >> >> >limited aspect of the bug it claimed to. >> >> >> >> That is not true. It only makes it a bit more limited, and not by >> >> its fault even. Even with that it makes things a bit better, not >> >> worse. >> > >> > It makes the code worse for very limited improvement, for which we did >> > not have a publically reported bug, i.e. the impact is very small. >> >> I can get the person who reported it to me to raise a Bugzilla >> complaining about the WARN_ON if you like ... > > This is about the original bug, for with the bugfix resulted in the > WARN_ON now being removed here. The underlying problem (I think, it's a > maze) is that our vma active tracking is a bit ... underwhelming. Sure, which is fair enough, but OTOH is there an actual plan for redoing the VMA tracking? Cheers, Daniel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [Announcement] 2015-Q3 release of XenGT - a Mediated Graphics Passthrough Solution from Intel
On Tue, Nov 24, 2015 at 03:12:31PM +0100, Gerd Hoffmann wrote: > Hi, > > > But there's some work to add generic mmap support to dma-bufs, and for > > really simple case (where we don't have a gl driver to handle the dma-buf > > specially) for untiled framebuffers that would be all we need? > > Not requiring gl is certainly a bonus, people might want build qemu > without opengl support to reduce the attach surface and/or package > dependency chain. > > And, yes, requirements for the non-gl rendering path are pretty low. > qemu needs something it can mmap, and which it can ask pixman to handle. > Preferred format is PIXMAN_x8r8g8b8 (qemu uses that internally in alot > of places so this avoids conversions). > > Current plan is to have a special vfio region (not visible to the guest) > where the framebuffer lives, with one or two pages at the end for meta > data (format and size). Status field is there too and will be used by > qemu to request updates and the kernel to signal update completion. > Guess I should write that down as vfio rfc patch ... > > I don't think it makes sense to have fields to notify qemu about which > framebuffer regions have been updated, I'd expect with full-screen > composing we have these days this information isn't available anyway. > Maybe a flag telling whenever there have been updates or not, so qemu > can skip update processing in case we have the screensaver showing a > black screen all day long. GL, wayland, X, EGL and soonish Android's surface flinger (hwc already has it afaik) all track damage. There's plans to add the same to the atomic kms api too. But if you do damage tracking you really don't want to support (maybe allow for perf reasons if the guest is stupid) frontbuffer rendering, which means you need buffer handles + damage, and not a static region. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation 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/3] drm/i915: Check for underruns after crtc disable
On Tue, Nov 24, 2015 at 03:14:23PM +0100, Daniel Vetter wrote: > On Mon, Nov 23, 2015 at 02:42:21PM +, Chris Wilson wrote: > > On Mon, Nov 23, 2015 at 04:10:32PM +0200, Ville Syrjälä wrote: > > > On Sat, Nov 21, 2015 at 10:49:04AM +, Chris Wilson wrote: > > > > On Fri, Nov 20, 2015 at 10:09:20PM +0200, ville.syrj...@linux.intel.com > > > > wrote: > > > > > From: Ville Syrjälä > > > > > > > > > > To get a better idea if underruns occurred during crtc disabling, > > > > > let's check for them explicitly. This helps in cases where the > > > > > error interrupt isn't active, or there is no underrun interrupt > > > > > support at all. > > > > > > > > > > Signed-off-by: Ville Syrjälä > > > > > > > > Would this be better the vblank after enabling? > > > > > > We do that too. > > > > Oh, crtc disabling. I can't read. > > Hm, should we also double-check before disabling, to catch anything that > happened right before the modeset and avoid confusing it with underruns > happening during the modeset? Hmm. Yeah that might be a decent idea. Although we don't wait for the planes to be disabled before we disable the pipe so in theory some weird underrun stemming from the plane disable might still get mixed in there. > > But this is a good idea already. > > Reviewed-by: Daniel Vetter > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- 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 02/12] drm/i915: Use the new rq->i915 field where appropriate
On Tue, Nov 24, 2015 at 02:57:04PM +0100, Daniel Vetter wrote: > On Fri, Nov 20, 2015 at 12:43:42PM +, Chris Wilson wrote: > > In a few frequent cases, having a direct pointer to the drm_i915_private > > from the request is very useful. > > > > Signed-off-by: Chris Wilson > > req->i915 is a bit inconsistent imo, i915_dev would be better. Anyway, not > something you started, just makes the code a bit harder to speed-read ;-) How does it strike you as inconsistent? rq->i915 was nicer :-p The idea I have been trying to sell is i915->drm->dev for the struct drm_i915_private / struct drm_device / struct device triplet. -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: take a power domain reference while checking the HDMI live status
On pe, 2015-11-20 at 13:13 +0200, Jani Nikula wrote: > On Fri, 20 Nov 2015, Imre Deak wrote: > > On Fri, 2015-11-20 at 11:54 +0200, Imre Deak wrote: > > > On Thu, 2015-11-19 at 21:17 +0200, Ville Syrjälä wrote: > > > > On Thu, Nov 19, 2015 at 09:13:04PM +0200, Imre Deak wrote: > > > > > On Thu, 2015-11-19 at 21:08 +0200, Ville Syrjälä wrote: > > > > > > On Thu, Nov 19, 2015 at 08:55:01PM +0200, Imre Deak wrote: > > > > > > > There are platforms that don't need the full GMBUS power > > > > > > > domain > > > > > > > (PCH, BXT) while others do (VLV/CHV). For optimizing this > > > > > > > we > > > > > > > would need to add a new power domain, but it's not clear > > > > > > > how > > > > > > > much > > > > > > > we > > > > > > > would benefit given the short time we hold the reference. > > > > > > > So > > > > > > > for > > > > > > > now > > > > > > > let's keep things simple. > > > > > > > > > > > > Actually on PCH platforms the gmbus domain means just just > > > > > > an > > > > > > rpm ref since the gmbus hw lives in the PCH. > > > > > > > > > > Ah right. > > > > > > > > > > > And IIRC on BXT gmbus lives in pw0 so same deal really. > > > > > > > > > > It's in PW2 there. I'll fix the commit message. > > > > > > > > Doh. Not sure where I got the PW0 zero idea. Maybe I confused > > > > gmbus > > > > with hpd. But yes, you're right about that. > > > > > > > > > > > > > > > And for vlv/chv we should just need the disp2d well, which > > > > > > is exactly what we get with the gmbus domain. > > > > > > > > > > > > So I don't think there's actually anything to optimize here > > > > > > with current platforms. > > > > > > > > > > > > Both patches look fine to me: > > > > > > Reviewed-by: Ville Syrjälä > > > > > > Pushed 1/2 and 2/2 with the fixed commit message to dinq. Thanks > > > for > > > the review. > > > > I just noticed that 2/2 fixes the following commit which is in > > v4.4-rc1 > > already: > > > > commit 237ed86c693d8a8e4db476976aeb30df4deac74b > > Author: Sonika Jindal > > Date: Tue Sep 15 09:44:20 2015 +0530 > > > > drm/i915: Check live status before reading edid > > > > so AFAIU this should've gone through -fixes. Jani can you still > > apply > > it there? > > Otherwise no problem but this has a dependency on > > commit f0ab43e6c338896cadee64ced3fc30a5343890d9 > Author: Ville Syrjälä > Date: Mon Nov 9 16:48:19 2015 +0100 > > drm/i915: Introduce a gmbus power domain > > and didn't bother figuring out how deep the rabbit hole goes. > > If you want this in -fixes, please either send a -fixes specific > patch, > or see how many commits would need to be backported. Ok. I backported the 5 patches we'd need. The only issue with that AFAICS is to resolve any conflict between drm-intel-fixes and dinq, but that looks manageable. I did have to resolve one conflict in one of the patches while backporting, so I pushed them to https://github.com/ideak/linux/commits/gmbus-fix-backport I can also send the rebased versions to the list if needed. --Imre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/12] drm/i915: Unify intel_logical_ring_emit and intel_ring_emit
On Fri, Nov 20, 2015 at 12:43:43PM +, Chris Wilson wrote:
> Both perform the same actions with more or less indirection, so just
> unify the code.
>
> Signed-off-by: Chris Wilson
> ---
> drivers/gpu/drm/i915/i915_gem.c| 2 +-
> drivers/gpu/drm/i915/i915_gem_context.c| 8 +-
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 34 -
> drivers/gpu/drm/i915/i915_gem_gtt.c| 26 +++
> drivers/gpu/drm/i915/intel_display.c | 26 +++
> drivers/gpu/drm/i915/intel_lrc.c | 118
> ++---
> drivers/gpu/drm/i915/intel_lrc.h | 21 -
> drivers/gpu/drm/i915/intel_mocs.c | 30
> drivers/gpu/drm/i915/intel_overlay.c | 42 +-
> drivers/gpu/drm/i915/intel_ringbuffer.c| 107 +-
> drivers/gpu/drm/i915/intel_ringbuffer.h| 15 +---
> 11 files changed, 195 insertions(+), 234 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5c3d11d020f0..c60105e36263 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4642,7 +4642,7 @@ err:
>
> int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice)
> {
> - struct intel_engine_cs *ring = req->ring;
> + struct intel_ringbuffer *ring = req->ringbuf;
> struct drm_i915_private *dev_priv = req->i915;
> u32 reg_base = GEN7_L3LOG_BASE + (slice * 0x200);
> u32 *remap_info = dev_priv->l3_parity.remap_info[slice];
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 82a9f7197d32..c3adc121aab4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -479,7 +479,7 @@ i915_gem_context_get(struct drm_i915_file_private
> *file_priv, u32 id)
> static inline int
> mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
> {
> - struct intel_engine_cs *ring = req->ring;
> + struct intel_ringbuffer *ring = req->ringbuf;
> u32 flags = hw_flags | MI_MM_SPACE_GTT;
> const int num_rings =
> /* Use an extended w/a on ivb+ if signalling from other rings */
> @@ -494,7 +494,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32
> hw_flags)
>* itlb_before_ctx_switch.
>*/
> if (IS_GEN6(req->i915)) {
> - ret = ring->flush(req, I915_GEM_GPU_DOMAINS, 0);
> + ret = req->ring->flush(req, I915_GEM_GPU_DOMAINS, 0);
> if (ret)
> return ret;
> }
> @@ -522,7 +522,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32
> hw_flags)
>
> intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
> for_each_ring(signaller, req->i915, i) {
> - if (signaller == ring)
> + if (signaller == req->ring)
> continue;
>
> intel_ring_emit(ring,
> RING_PSMI_CTL(signaller->mmio_base));
> @@ -547,7 +547,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32
> hw_flags)
>
> intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
> for_each_ring(signaller, req->i915, i) {
> - if (signaller == ring)
> + if (signaller == req->ring)
> continue;
Tsk, unrelated hunks above. And maybe a cocci to s/req->ring/req->engine/.
At least mention in the commit message that your on a crusade against
superflous ring/dev/whatever pointers and arguments while in the area.
> intel_ring_emit(ring,
> RING_PSMI_CTL(signaller->mmio_base));
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index e8164b644e0e..5f1b9b7df051 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1113,14 +1113,12 @@ i915_gem_execbuffer_retire_commands(struct
> i915_execbuffer_params *params)
> }
>
> static int
> -i915_reset_gen7_sol_offsets(struct drm_device *dev,
> - struct drm_i915_gem_request *req)
> +i915_reset_gen7_sol_offsets(struct drm_i915_gem_request *req)
> {
> - struct intel_engine_cs *ring = req->ring;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> + struct intel_ringbuffer *ring = req->ringbuf;
> int ret, i;
>
> - if (!IS_GEN7(dev) || ring != &dev_priv->ring[RCS]) {
> + if (!IS_GEN7(req->i915) || req->ring->id != RCS) {
> DRM_DEBUG("sol reset is gen7/rcs only\n");
> return -EINVAL;
> }
> @@ -1198,9 +1196,8 @@ i915_gem_ringbuffer_submission(struct
> i915_execbuffer_params *params,
> struct drm_i915_gem_execbuffer2 *args,
> struct l
Re: [Intel-gfx] [PATCH 04/12] drm/i915: Unify intel_ring_begin()
On Fri, Nov 20, 2015 at 12:43:44PM +, Chris Wilson wrote:
> Combine the near identical implementations of intel_logical_ring_begin()
> and intel_ring_begin() - the only difference is that the logical wait
> has to check for a matching ring (which is assumed by legacy).
>
> Signed-off-by: Chris Wilson
Hm, I originally punted on this one since OCD-me wanted to move
engine->request_list to ring->request_list first. But hey just adding the
check works too and gives us the immediate improvement faster!
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/intel_lrc.c| 149
> ++--
> drivers/gpu/drm/i915/intel_lrc.h| 1 -
> drivers/gpu/drm/i915/intel_mocs.c | 12 +--
> drivers/gpu/drm/i915/intel_ringbuffer.c | 115
> 4 files changed, 71 insertions(+), 206 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> b/drivers/gpu/drm/i915/intel_lrc.c
> index 0db23c474045..02f798e4c726 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -646,48 +646,6 @@ int intel_logical_ring_alloc_request_extras(struct
> drm_i915_gem_request *request
> return 0;
> }
>
> -static int logical_ring_wait_for_space(struct drm_i915_gem_request *req,
> -int bytes)
> -{
> - struct intel_ringbuffer *ringbuf = req->ringbuf;
> - struct intel_engine_cs *ring = req->ring;
> - struct drm_i915_gem_request *target;
> - unsigned space;
> - int ret;
> -
> - if (intel_ring_space(ringbuf) >= bytes)
> - return 0;
> -
> - /* The whole point of reserving space is to not wait! */
> - WARN_ON(ringbuf->reserved_in_use);
> -
> - list_for_each_entry(target, &ring->request_list, list) {
> - /*
> - * The request queue is per-engine, so can contain requests
> - * from multiple ringbuffers. Here, we must ignore any that
> - * aren't from the ringbuffer we're considering.
> - */
> - if (target->ringbuf != ringbuf)
> - continue;
> -
> - /* Would completion of this request free enough space? */
> - space = __intel_ring_space(target->postfix, ringbuf->tail,
> -ringbuf->size);
> - if (space >= bytes)
> - break;
> - }
> -
> - if (WARN_ON(&target->list == &ring->request_list))
> - return -ENOSPC;
> -
> - ret = i915_wait_request(target);
> - if (ret)
> - return ret;
> -
> - ringbuf->space = space;
> - return 0;
> -}
> -
> /*
> * intel_logical_ring_advance_and_submit() - advance the tail and submit the
> workload
> * @request: Request to advance the logical ringbuffer of.
> @@ -708,97 +666,6 @@ intel_logical_ring_advance_and_submit(struct
> drm_i915_gem_request *request)
> execlists_context_queue(request);
> }
>
> -static void __wrap_ring_buffer(struct intel_ringbuffer *ringbuf)
> -{
> - int rem = ringbuf->size - ringbuf->tail;
> - memset(ringbuf->virtual_start + ringbuf->tail, 0, rem);
> -
> - ringbuf->tail = 0;
> - intel_ring_update_space(ringbuf);
> -}
> -
> -static int logical_ring_prepare(struct drm_i915_gem_request *req, int bytes)
> -{
> - struct intel_ringbuffer *ringbuf = req->ringbuf;
> - int remain_usable = ringbuf->effective_size - ringbuf->tail;
> - int remain_actual = ringbuf->size - ringbuf->tail;
> - int ret, total_bytes, wait_bytes = 0;
> - bool need_wrap = false;
> -
> - if (ringbuf->reserved_in_use)
> - total_bytes = bytes;
> - else
> - total_bytes = bytes + ringbuf->reserved_size;
> -
> - if (unlikely(bytes > remain_usable)) {
> - /*
> - * Not enough space for the basic request. So need to flush
> - * out the remainder and then wait for base + reserved.
> - */
> - wait_bytes = remain_actual + total_bytes;
> - need_wrap = true;
> - } else {
> - if (unlikely(total_bytes > remain_usable)) {
> - /*
> - * The base request will fit but the reserved space
> - * falls off the end. So only need to to wait for the
> - * reserved size after flushing out the remainder.
> - */
> - wait_bytes = remain_actual + ringbuf->reserved_size;
> - need_wrap = true;
> - } else if (total_bytes > ringbuf->space) {
> - /* No wrapping required, just waiting. */
> - wait_bytes = total_bytes;
> - }
> - }
> -
> - if (wait_bytes) {
> - ret = logical_ring_wait_for_space(req, wait_bytes);
> - if (unlikely(ret))
> - return ret;
> -
> - if (need_wrap)
> - __wra
Re: [Intel-gfx] [PATCH 02/12] drm/i915: Use the new rq->i915 field where appropriate
On Tue, Nov 24, 2015 at 02:23:03PM +, Chris Wilson wrote: > On Tue, Nov 24, 2015 at 02:57:04PM +0100, Daniel Vetter wrote: > > On Fri, Nov 20, 2015 at 12:43:42PM +, Chris Wilson wrote: > > > In a few frequent cases, having a direct pointer to the drm_i915_private > > > from the request is very useful. > > > > > > Signed-off-by: Chris Wilson > > > > req->i915 is a bit inconsistent imo, i915_dev would be better. Anyway, not > > something you started, just makes the code a bit harder to speed-read ;-) > > How does it strike you as inconsistent? rq->i915 was nicer :-p > > The idea I have been trying to sell is i915->drm->dev for the struct > drm_i915_private / struct drm_device / struct device triplet. Hm, we should do an overview section in i915 docs about best practices in variable naming. I'm serious about this, since we have similar fun with modeset stuff: - Preferred: crtc for struct intel_crtc, drm_crtc for struct drm_crtc. - Transitinoal: crtc for struct drm_crtc, intel_crtc for struct intel_crtc. But if you have only one, then don't use that. Maybe we should mention intel_ vs. i915_ vs. platform prefixes too? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation 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 05/12] drm/i915: Remove the identical implementations of request space reservation
On Fri, Nov 20, 2015 at 12:43:45PM +, Chris Wilson wrote:
> Now that we share intel_ring_begin(), reserving space for the tail of
> the request is identical between legacy/execlists and so the tautology
> can be removed.
>
> Signed-off-by: Chris Wilson
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_gem.c | 7 +++
> drivers/gpu/drm/i915/intel_lrc.c| 15 ---
> drivers/gpu/drm/i915/intel_lrc.h| 1 -
> drivers/gpu/drm/i915/intel_ringbuffer.c | 15 ---
> drivers/gpu/drm/i915/intel_ringbuffer.h | 3 ---
> 5 files changed, 3 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c60105e36263..030fc9d14385 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2743,10 +2743,9 @@ int i915_gem_request_alloc(struct intel_engine_cs
> *ring,
>* to be redone if the request is not actually submitted straight
>* away, e.g. because a GPU scheduler has deferred it.
>*/
> - if (i915.enable_execlists)
> - ret = intel_logical_ring_reserve_space(req);
> - else
> - ret = intel_ring_reserve_space(req);
> + intel_ring_reserved_space_reserve(req->ringbuf,
> + MIN_SPACE_FOR_ADD_REQUEST);
> + ret = intel_ring_begin(req, 0);
> if (ret) {
> /*
>* At this point, the request is fully allocated even if not
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> b/drivers/gpu/drm/i915/intel_lrc.c
> index 02f798e4c726..2458804c521d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -666,21 +666,6 @@ intel_logical_ring_advance_and_submit(struct
> drm_i915_gem_request *request)
> execlists_context_queue(request);
> }
>
> -int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request)
> -{
> - /*
> - * The first call merely notes the reserve request and is common for
> - * all back ends. The subsequent localised _begin() call actually
> - * ensures that the reservation is available. Without the begin, if
> - * the request creator immediately submitted the request without
> - * adding any commands to it then there might not actually be
> - * sufficient room for the submission commands.
> - */
> - intel_ring_reserved_space_reserve(request->ringbuf,
> MIN_SPACE_FOR_ADD_REQUEST);
> -
> - return intel_ring_begin(request, 0);
> -}
> -
> /**
> * execlists_submission() - submit a batchbuffer for execution, Execlists
> style
> * @dev: DRM device.
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h
> b/drivers/gpu/drm/i915/intel_lrc.h
> index 5402eca78a07..9981e989fcaf 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -38,7 +38,6 @@
>
> /* Logical Rings */
> int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request
> *request);
> -int intel_logical_ring_reserve_space(struct drm_i915_gem_request *request);
> void intel_logical_ring_stop(struct intel_engine_cs *ring);
> void intel_logical_ring_cleanup(struct intel_engine_cs *ring);
> int intel_logical_rings_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index fbee790ddaf0..9821c2a8074a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2168,21 +2168,6 @@ int intel_ring_alloc_request_extras(struct
> drm_i915_gem_request *request)
> return 0;
> }
>
> -int intel_ring_reserve_space(struct drm_i915_gem_request *request)
> -{
> - /*
> - * The first call merely notes the reserve request and is common for
> - * all back ends. The subsequent localised _begin() call actually
> - * ensures that the reservation is available. Without the begin, if
> - * the request creator immediately submitted the request without
> - * adding any commands to it then there might not actually be
> - * sufficient room for the submission commands.
> - */
> - intel_ring_reserved_space_reserve(request->ringbuf,
> MIN_SPACE_FOR_ADD_REQUEST);
> -
> - return intel_ring_begin(request, 0);
> -}
> -
> void intel_ring_reserved_space_reserve(struct intel_ringbuffer *ringbuf, int
> size)
> {
> WARN_ON(ringbuf->reserved_size);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
> b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 212b402818f9..368d9b0454ed 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -487,7 +487,4 @@ void intel_ring_reserved_space_use(struct
> intel_ringbuffer *ringbuf);
> /* Finish with the reserved space - for use by i915_add_request() only. */
> void intel_ring_reserved_space_end(struct intel_ringbuffer *ringbuf);
>
> -/* Legacy ringbuffer specific portion of res
Re: [Intel-gfx] [PATCH 06/12] drm/i915: Rename request->ring to request->engine
On Fri, Nov 20, 2015 at 12:43:46PM +, Chris Wilson wrote:
> In order to disambiguate between the pointer to the intel_engine_cs
> (called ring) and the intel_ringbuffer (called ringbuf), rename
> s/ring/engine/.
>
> Signed-off-by: Chris Wilson
Ah, here we go. Slight amend for the commit message:
"Where known that req is non-NULL replace get_ring() with req->engine too
while at it. And also use dev_priv more while touching code."
With that Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 11 ++-
> drivers/gpu/drm/i915/i915_drv.h | 12 +--
> drivers/gpu/drm/i915/i915_gem.c | 76
> drivers/gpu/drm/i915/i915_gem_context.c | 63 ++---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 8 +-
> drivers/gpu/drm/i915/i915_gem_gtt.c | 49 +-
> drivers/gpu/drm/i915/i915_gem_render_state.c | 18 ++--
> drivers/gpu/drm/i915/i915_gpu_error.c| 3 +-
> drivers/gpu/drm/i915/i915_trace.h| 34 +++
> drivers/gpu/drm/i915/intel_display.c | 12 +--
> drivers/gpu/drm/i915/intel_lrc.c | 128
> +--
> drivers/gpu/drm/i915/intel_mocs.c| 10 +--
> drivers/gpu/drm/i915/intel_ringbuffer.c | 66 +++---
> 13 files changed, 239 insertions(+), 251 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 359436162f3d..56375c36b381 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -190,8 +190,7 @@ describe_obj(struct seq_file *m, struct
> drm_i915_gem_object *obj)
> seq_printf(m, " (%s mappable)", s);
> }
> if (obj->last_write_req != NULL)
> - seq_printf(m, " (%s)",
> -
> i915_gem_request_get_ring(obj->last_write_req)->name);
> + seq_printf(m, " (%s)", obj->last_write_req->engine->name);
> if (obj->frontbuffer_bits)
> seq_printf(m, " (frontbuffer: 0x%03x)", obj->frontbuffer_bits);
> }
> @@ -594,14 +593,14 @@ static int i915_gem_pageflip_info(struct seq_file *m,
> void *data)
> pipe, plane);
> }
> if (work->flip_queued_req) {
> - struct intel_engine_cs *ring =
> -
> i915_gem_request_get_ring(work->flip_queued_req);
> + struct intel_engine_cs *engine =
> + work->flip_queued_req->engine;
>
> seq_printf(m, "Flip queued on %s at seqno %x,
> next seqno %x [current breadcrumb %x], completed? %d\n",
> -ring->name,
> +engine->name,
>
> i915_gem_request_get_seqno(work->flip_queued_req),
> dev_priv->next_seqno,
> -ring->get_seqno(ring, true),
> +engine->get_seqno(engine, true),
>
> i915_gem_request_completed(work->flip_queued_req, true));
> } else
> seq_printf(m, "Flip not associated with any
> ring\n");
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fadf2ceb1f72..9ce8b3fcb3a0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2156,7 +2156,7 @@ struct drm_i915_gem_request {
>
> /** On Which ring this request was generated */
> struct drm_i915_private *i915;
> - struct intel_engine_cs *ring;
> + struct intel_engine_cs *engine;
>
> /** GEM sequence number associated with this request. */
> uint32_t seqno;
> @@ -2240,9 +2240,9 @@ i915_gem_request_get_seqno(struct drm_i915_gem_request
> *req)
> }
>
> static inline struct intel_engine_cs *
> -i915_gem_request_get_ring(struct drm_i915_gem_request *req)
> +i915_gem_request_get_engine(struct drm_i915_gem_request *req)
> {
> - return req ? req->ring : NULL;
> + return req ? req->engine : NULL;
> }
>
> static inline struct drm_i915_gem_request *
> @@ -2256,7 +2256,7 @@ i915_gem_request_reference(struct drm_i915_gem_request
> *req)
> static inline void
> i915_gem_request_unreference(struct drm_i915_gem_request *req)
> {
> - WARN_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
> + WARN_ON(!mutex_is_locked(&req->i915->dev->struct_mutex));
> kref_put(&req->ref, i915_gem_request_free);
> }
>
> @@ -2268,7 +2268,7 @@ i915_gem_request_unreference__unlocked(struct
> drm_i915_gem_request *req)
> if (!req)
> return;
>
> - dev = req->ring->dev;
> + dev = req->i915->dev;
> if (kref_put_mutex(&req->ref, i915_gem_request_free,
> &dev
Re: [Intel-gfx] [PATCH 1/3] drm/i915: Suppress spurious CPU FIFO underruns on ILK-IVB
On Tue, Nov 24, 2015 at 03:16:24PM +0100, Daniel Vetter wrote:
> On Fri, Nov 20, 2015 at 10:09:18PM +0200, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä
> >
> > We still get spurious pipe underruns on ILK/SNB/IVB under two
> > circumstances when dealing with PCH ports:
> > * When the pipe has been disabled, but FDI RX/TX is still enabled
> > * During FDI link training
> >
> > Both cases seem to happen at least when we do VGA+HDMI cloning
> > from the same pipe. I don't think I've seen them when not cloning,
> > but can't be 100% sure.
> >
> > Disable underrun reporting around those places to eliminate the
> > dmesg errors.
> >
> > Testcase: igt/kms_setmode/basic-clone-single-crtc
> > Signed-off-by: Ville Syrjälä
>
> hsw seems to have a similar problem when both vga and hdmi are used, even
> when not cloned. Bad thing is that somehow the underrun reporting gets
> into a non-recoverable state and stuck there, so that no underrun and also
> nothing else (specifically crc irqs, that's why bat notices) work any
> more.
>
> Might be worth a shot to try the same trick there. Of course we still need
> to figure out why it can't recover from this, too.
I don't have a HSW/BDW with VGA on me. I suppose I might need to go
find one and see what's what.
Series pushed to dinq. Thanks for the acks and reviews.
>
> Anyway just an aside, ack on the entire series.
> -Daniel
>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 20 +++-
> > 1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 68fb449ded77..8a8104b7947d 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4137,6 +4137,12 @@ static void ironlake_pch_enable(struct drm_crtc
> > *crtc)
> > I915_WRITE(FDI_RX_TUSIZE1(pipe),
> >I915_READ(PIPE_DATA_M1(pipe)) & TU_SIZE_MASK);
> >
> > + /*
> > +* Sometimes spurious CPU pipe underruns happen during FDI
> > +* training, at least with VGA+HDMI cloning. Suppress them.
> > +*/
> > + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
> > +
> > /* For PCH output, training FDI link */
> > dev_priv->display.fdi_link_train(crtc);
> >
> > @@ -4170,6 +4176,8 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
> >
> > intel_fdi_normal_train(crtc);
> >
> > + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> > +
> > /* For PCH DP, enable TRANS_DP_CTL */
> > if (HAS_PCH_CPT(dev) && intel_crtc->config->has_dp_encoder) {
> > const struct drm_display_mode *adjusted_mode =
> > @@ -5062,12 +5070,22 @@ static void ironlake_crtc_disable(struct drm_crtc
> > *crtc)
> > drm_crtc_vblank_off(crtc);
> > assert_vblank_disabled(crtc);
> >
> > + /*
> > +* Sometimes spurious CPU pipe underruns happen when the
> > +* pipe is already disabled, but FDI RX/TX is still enabled.
> > +* Happens at least with VGA+HDMI cloning. Suppress them.
> > +*/
> > + if (intel_crtc->config->has_pch_encoder)
> > + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
> > +
> > intel_disable_pipe(intel_crtc);
> >
> > ironlake_pfit_disable(intel_crtc, false);
> >
> > - if (intel_crtc->config->has_pch_encoder)
> > + if (intel_crtc->config->has_pch_encoder) {
> > ironlake_fdi_disable(crtc);
> > + intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
> > + }
> >
> > for_each_encoder_on_crtc(dev, crtc, encoder)
> > if (encoder->post_disable)
> > --
> > 2.4.10
> >
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
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 07/12] drm/i915: Rename request->ringbuf to request->ring
On Fri, Nov 20, 2015 at 12:43:47PM +, Chris Wilson wrote:
> Now that we have disambuigated ring and engine, we can use the clearer
> and more consistent name for the intel_ringbuffer pointer in the
> request.
>
> Signed-off-by: Chris Wilson
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_drv.h| 2 +-
> drivers/gpu/drm/i915/i915_gem.c| 28 +++---
> drivers/gpu/drm/i915/i915_gem_context.c| 2 +-
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +-
> drivers/gpu/drm/i915/i915_gem_gtt.c| 6 +-
> drivers/gpu/drm/i915/intel_display.c | 10 +-
> drivers/gpu/drm/i915/intel_lrc.c | 149
> ++---
> drivers/gpu/drm/i915/intel_mocs.c | 32 +++
> drivers/gpu/drm/i915/intel_overlay.c | 42
> drivers/gpu/drm/i915/intel_ringbuffer.c| 86 -
> 10 files changed, 178 insertions(+), 183 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9ce8b3fcb3a0..b7eaa2deb437 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2185,7 +2185,7 @@ struct drm_i915_gem_request {
>* context.
>*/
> struct intel_context *ctx;
> - struct intel_ringbuffer *ringbuf;
> + struct intel_ringbuffer *ring;
>
> /** Batch buffer related to this request if any (used for
> error state dump only) */
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5a1b51a27fe3..d6706dd4117c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1386,7 +1386,7 @@ static void i915_gem_request_retire(struct
> drm_i915_gem_request *request)
>* Note this requires that we are always called in request
>* completion order.
>*/
> - request->ringbuf->last_retired_head = request->postfix;
> + request->ring->last_retired_head = request->postfix;
>
> list_del_init(&request->list);
> i915_gem_request_remove_from_client(request);
> @@ -2553,7 +2553,7 @@ void __i915_add_request(struct drm_i915_gem_request
> *request,
> {
> struct intel_engine_cs *engine;
> struct drm_i915_private *dev_priv;
> - struct intel_ringbuffer *ringbuf;
> + struct intel_ringbuffer *ring;
> u32 request_start;
> int ret;
>
> @@ -2562,16 +2562,16 @@ void __i915_add_request(struct drm_i915_gem_request
> *request,
>
> engine = request->engine;
> dev_priv = request->i915;
> - ringbuf = request->ringbuf;
> + ring = request->ring;
>
> /*
>* To ensure that this call will not fail, space for its emissions
>* should already have been reserved in the ring buffer. Let the ring
>* know that it is time to use that space up.
>*/
> - intel_ring_reserved_space_use(ringbuf);
> + intel_ring_reserved_space_use(ring);
>
> - request_start = intel_ring_get_tail(ringbuf);
> + request_start = intel_ring_get_tail(ring);
> /*
>* Emit any outstanding flushes - execbuf can fail to emit the flush
>* after having emitted the batchbuffer command. Hence we need to fix
> @@ -2593,14 +2593,14 @@ void __i915_add_request(struct drm_i915_gem_request
> *request,
>* GPU processing the request, we never over-estimate the
>* position of the head.
>*/
> - request->postfix = intel_ring_get_tail(ringbuf);
> + request->postfix = intel_ring_get_tail(ring);
>
> if (i915.enable_execlists)
> ret = engine->emit_request(request);
> else {
> ret = engine->add_request(request);
>
> - request->tail = intel_ring_get_tail(ringbuf);
> + request->tail = intel_ring_get_tail(ring);
> }
> /* Not allowed to fail! */
> WARN(ret, "emit|add_request failed: %d!\n", ret);
> @@ -2629,7 +2629,7 @@ void __i915_add_request(struct drm_i915_gem_request
> *request,
> intel_mark_busy(dev_priv->dev);
>
> /* Sanity check that the reserved size was large enough. */
> - intel_ring_reserved_space_end(ringbuf);
> + intel_ring_reserved_space_end(ring);
> }
>
> static bool i915_context_is_banned(struct drm_i915_private *dev_priv,
> @@ -2741,7 +2741,7 @@ int i915_gem_request_alloc(struct intel_engine_cs
> *engine,
>* to be redone if the request is not actually submitted straight
>* away, e.g. because a GPU scheduler has deferred it.
>*/
> - intel_ring_reserved_space_reserve(req->ringbuf,
> + intel_ring_reserved_space_reserve(req->ring,
> MIN_SPACE_FOR_ADD_REQUEST);
> ret = intel_ring_begin(req, 0);
> if (ret) {
> @@ -2764,7 +2764,7 @@ err:
>
> void i915_gem_request_cancel(struct drm_i915_gem_request *req)
> {
> - intel_ring_reserved_space_cancel(req->ringbuf);
> + intel_ring_reserved_space_cancel(req->ring);
Re: [Intel-gfx] [PATCH 03/12] drm/i915: Unify intel_logical_ring_emit and intel_ring_emit
On Tue, Nov 24, 2015 at 03:33:12PM +0100, Daniel Vetter wrote:
> On Fri, Nov 20, 2015 at 12:43:43PM +, Chris Wilson wrote:
> > @@ -547,7 +547,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32
> > hw_flags)
> >
> > intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(num_rings));
> > for_each_ring(signaller, req->i915, i) {
> > - if (signaller == ring)
> > + if (signaller == req->ring)
> > continue;
>
> Tsk, unrelated hunks above. And maybe a cocci to s/req->ring/req->engine/.
> At least mention in the commit message that your on a crusade against
> superflous ring/dev/whatever pointers and arguments while in the area.
It was hard to tell where one patch began and another ended.
> > @@ -1275,9 +1269,9 @@ i915_gem_ringbuffer_submission(struct
> > i915_execbuffer_params *params,
> > exec_start = params->batch_obj_vm_offset +
> > params->args_batch_start_offset;
> >
> > - ret = ring->dispatch_execbuffer(params->request,
> > - exec_start, exec_len,
> > - params->dispatch_flags);
> > + ret = params->ring->dispatch_execbuffer(params->request,
> > + exec_start, exec_len,
> > + params->dispatch_flags);
>
> I guess we should just shovel all the stuff in params into request
> eventually ...
Have you been reading ahead? The only argument for these exact
parameters to be recorded would be for hangcheck. But we do get to
cleanup execbuf dispatch eventually as well.
> > diff --git a/drivers/gpu/drm/i915/intel_overlay.c
> > b/drivers/gpu/drm/i915/intel_overlay.c
> > index 444542696a2c..5e9c7c15a84b 100644
> > --- a/drivers/gpu/drm/i915/intel_overlay.c
> > +++ b/drivers/gpu/drm/i915/intel_overlay.c
> > @@ -433,9 +433,9 @@ static int intel_overlay_release_old_vid(struct
> > intel_overlay *overlay)
> > return ret;
> > }
> >
> > - intel_ring_emit(ring, MI_WAIT_FOR_EVENT |
> > MI_WAIT_FOR_OVERLAY_FLIP);
> > - intel_ring_emit(ring, MI_NOOP);
> > - intel_ring_advance(ring);
> > + intel_ring_emit(req->ringbuf, MI_WAIT_FOR_EVENT |
> > MI_WAIT_FOR_OVERLAY_FLIP);
> > + intel_ring_emit(req->ringbuf, MI_NOOP);
> > + intel_ring_advance(req->ringbuf);
> >
> > ret = intel_overlay_do_wait_request(overlay, req,
> >
> > intel_overlay_release_old_vid_tail);
>
> Any reason for not doing your usual transform to intel_ringbuffer for the
> local ring here in the overlay code? Or does that happen when
> intel_ring_begin falls?
Later patches looked a bit neater with not adding a temporary
intel_engine_cs *ring; intel_ringbuf *ringbuf. That was my thinking at
least.
> At the end of this crusade we should add some pretty kerneldoc for our
> ringbuffer interface.
request_create
ring_begin{ ring_emit }ring_advance
request_commit (add_request)
But one may ask how do we go from request to ring. Wouldn't it be neat
to have that explicit in the interface:
req = request_create()
..
ring = ring_begin(req);
ring_emit(ring)...
ring_advance(ring)
...
request_commit(req);
Deja vu.
-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 02/12] drm/i915: Calculate watermark related members in the crtc_state, v3.
Op 24-11-15 om 15:03 schreef Ander Conselvan De Oliveira:
> On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
>> This removes pre/post_wm_update from intel_crtc->atomic, and
>> creates atomic state for it in intel_crtc.
>>
>> Changes since v1:
>> - Rebase on top of wm changes.
>> Changes since v2:
>> - Split disable_cxsr into a separate patch.
>>
>> Signed-off-by: Maarten Lankhorst
>> ---
>> drivers/gpu/drm/i915/intel_atomic.c | 1 +
>> drivers/gpu/drm/i915/intel_display.c | 31 +--
>> drivers/gpu/drm/i915/intel_drv.h | 2 +-
>> 3 files changed, 15 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index 9f0638a37b6d..4625f8a9ba12 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -96,6 +96,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>> crtc_state->update_pipe = false;
>> crtc_state->disable_lp_wm = false;
>> crtc_state->disable_cxsr = false;
>> +crtc_state->wm_changed = false;
>>
>> return &crtc_state->base;
>> }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 5ee64e67ad8a..db4995406277 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4741,6 +4741,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>> static void intel_post_plane_update(struct intel_crtc *crtc)
>> {
>> struct intel_crtc_atomic_commit *atomic = &crtc->atomic;
>> +struct intel_crtc_state *pipe_config =
>> +to_intel_crtc_state(crtc->base.state);
>> struct drm_device *dev = crtc->base.dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> @@ -4751,7 +4753,7 @@ static void intel_post_plane_update(struct intel_crtc
>> *crtc)
>>
>> crtc->wm.cxsr_allowed = true;
>>
>> -if (crtc->atomic.update_wm_post)
>> +if (pipe_config->wm_changed)
>> intel_update_watermarks(&crtc->base);
> This adds an extra call to intel_update_watermarks() for the case where
> previously update_wm_pre would be set. This won't cause extra register writes
> because of the dirty check, but I think it deserves a note in the commit
> message.
I think intel_update_watermarks needs to be fixed to take a crtc_state and
whether pre/post commit is used.
It looks to me like doing anything else could bug if watermarks are changed
with 1 plane becoming visible and the other invisible..
>>
>> if (atomic->update_fbc)
>> @@ -4784,6 +4786,9 @@ static void intel_pre_plane_update(struct intel_crtc
>> *crtc)
>> crtc->wm.cxsr_allowed = false;
>> intel_set_memory_cxsr(dev_priv, false);
>> }
>> +
>> +if (!needs_modeset(&pipe_config->base) && pipe_config->wm_changed)
>> +intel_update_watermarks(&crtc->base);
>> }
>>
>> static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned
>> plane_mask)
>> @@ -11706,25 +11711,18 @@ int intel_plane_atomic_calc_changes(struct
>> drm_crtc_state *crtc_state,
>> plane->base.id, was_visible, visible,
>> turn_off, turn_on, mode_changed);
>>
>> -if (turn_on) {
>> -intel_crtc->atomic.update_wm_pre = true;
>> -/* must disable cxsr around plane enable/disable */
>> -if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>> -pipe_config->disable_cxsr = true;
>> -/* to potentially re-enable cxsr */
>> -intel_crtc->atomic.wait_vblank = true;
>> -intel_crtc->atomic.update_wm_post = true;
>> -}
>> -} else if (turn_off) {
>> -intel_crtc->atomic.update_wm_post = true;
>> +if (turn_on || turn_off) {
>> +pipe_config->wm_changed = true;
>> +
>> /* must disable cxsr around plane enable/disable */
>> if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>> if (is_crtc_enabled)
>> intel_crtc->atomic.wait_vblank = true;
>> pipe_config->disable_cxsr = true;
>> }
>> -} else if (intel_wm_need_update(plane, plane_state)) {
>> -intel_crtc->atomic.update_wm_pre = true;
>> +} else if ((was_visible || visible) &&
> So this avoids watermark changes when the plane is not visible before or after
> the update. Wouldn't it be better to fix intel_wm_need_update() instead if
> returns true in that case?
If visible is changed wm_changed = true is always set. It would go through the
(turn_on || turn_off) case.
I guess checking was_visible || visible is overkill, and could be changed to
just visible && intel_wm_need_update(plane, plane_state).
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/12] drm/i915: Rename backpointer from intel_ringbuffer to intel_engine_cs
On Fri, Nov 20, 2015 at 12:43:48PM +, Chris Wilson wrote:
> Having ringbuf->ring point to an engine is confusing, so rename it once
> again to ring->engine.
>
> Signed-off-by: Chris Wilson
This massively conflicts with
commit e84fe80337dc85cca07d0417ea97edbec4789d8b
Author: Nick Hoath
Date: Fri Sep 11 12:53:46 2015 +0100
drm/i915: Split alloc from init for lrc
since this patch touches piles of old code now removed. But no functional
conflict afaics, so Acked-by: Daniel Vetter
Maybe rebase before sending ;-)
-Daniel
> ---
> drivers/gpu/drm/i915/intel_lrc.c| 46
> -
> drivers/gpu/drm/i915/intel_ringbuffer.c | 36 +-
> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +-
> 3 files changed, 42 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> b/drivers/gpu/drm/i915/intel_lrc.c
> index 5c37922c3cde..346f5889738e 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2089,13 +2089,13 @@ void intel_lr_context_free(struct intel_context *ctx)
> if (ctx_obj) {
> struct intel_ringbuffer *ringbuf =
> ctx->engine[i].ringbuf;
> - struct intel_engine_cs *ring = ringbuf->ring;
> + struct intel_engine_cs *engine = ringbuf->engine;
>
> - if (ctx == ring->default_context) {
> + if (ctx == engine->default_context) {
> intel_unpin_ringbuffer_obj(ringbuf);
> i915_gem_object_ggtt_unpin(ctx_obj);
> }
> - WARN_ON(ctx->engine[ring->id].pin_count);
> + WARN_ON(ctx->engine[engine->id].pin_count);
> intel_destroy_ringbuffer_obj(ringbuf);
> kfree(ringbuf);
> drm_gem_object_unreference(&ctx_obj->base);
> @@ -2158,19 +2158,19 @@ static void lrc_setup_hardware_status_page(struct
> intel_engine_cs *ring,
> * Return: non-zero on error.
> */
> int intel_lr_context_deferred_create(struct intel_context *ctx,
> - struct intel_engine_cs *ring)
> + struct intel_engine_cs *engine)
> {
> - const bool is_global_default_ctx = (ctx == ring->default_context);
> - struct drm_device *dev = ring->dev;
> + const bool is_global_default_ctx = (ctx == engine->default_context);
> + struct drm_device *dev = engine->dev;
> struct drm_i915_gem_object *ctx_obj;
> uint32_t context_size;
> struct intel_ringbuffer *ringbuf;
> int ret;
>
> WARN_ON(ctx->legacy_hw_ctx.rcs_state != NULL);
> - WARN_ON(ctx->engine[ring->id].state);
> + WARN_ON(ctx->engine[engine->id].state);
>
> - context_size = round_up(get_lr_context_size(ring), 4096);
> + context_size = round_up(get_lr_context_size(engine), 4096);
>
> ctx_obj = i915_gem_alloc_object(dev, context_size);
> if (!ctx_obj) {
> @@ -2191,12 +2191,12 @@ int intel_lr_context_deferred_create(struct
> intel_context *ctx,
> ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL);
> if (!ringbuf) {
> DRM_DEBUG_DRIVER("Failed to allocate ringbuffer %s\n",
> - ring->name);
> + engine->name);
> ret = -ENOMEM;
> goto error_unpin_ctx;
> }
>
> - ringbuf->ring = ring;
> + ringbuf->engine = engine;
>
> ringbuf->size = 32 * PAGE_SIZE;
> ringbuf->effective_size = ringbuf->size;
> @@ -2210,7 +2210,7 @@ int intel_lr_context_deferred_create(struct
> intel_context *ctx,
> if (ret) {
> DRM_DEBUG_DRIVER(
> "Failed to allocate ringbuffer obj %s: %d\n",
> - ring->name, ret);
> + engine->name, ret);
> goto error_free_rbuf;
> }
>
> @@ -2219,38 +2219,38 @@ int intel_lr_context_deferred_create(struct
> intel_context *ctx,
> if (ret) {
> DRM_ERROR(
> "Failed to pin and map ringbuffer %s:
> %d\n",
> - ring->name, ret);
> + engine->name, ret);
> goto error_destroy_rbuf;
> }
> }
>
> }
>
> - ret = populate_lr_context(ctx, ctx_obj, ring, ringbuf);
> + ret = populate_lr_context(ctx, ctx_obj, engine, ringbuf);
> if (ret) {
> DRM_DEBUG_DRIVER("Failed to populate LRC: %d\n", ret);
> goto error;
> }
>
> - ctx->engine[ring->id].ringbuf = ringbuf;
> - ctx->engine[ring->id].state = ctx_obj;
> + ctx->engine[engine->id].ringbuf = ringbuf;
> +
Re: [Intel-gfx] [PATCH 04/12] drm/i915: Unify intel_ring_begin()
On Tue, Nov 24, 2015 at 03:38:46PM +0100, Daniel Vetter wrote: > On Fri, Nov 20, 2015 at 12:43:44PM +, Chris Wilson wrote: > > Combine the near identical implementations of intel_logical_ring_begin() > > and intel_ring_begin() - the only difference is that the logical wait > > has to check for a matching ring (which is assumed by legacy). > > > > Signed-off-by: Chris Wilson > > Hm, I originally punted on this one since OCD-me wanted to move > engine->request_list to ring->request_list first. But hey just adding the > check works too and gives us the immediate improvement faster! I was too lazy to add the breadcrumb list I used last time. Or rather, I used that list for more than just wait_for_space() and didn't want to try and get the whole semaphore/interrupt mitigation reviewed. There is still the complaint for media wakeups caused by our breadcrumb being too heavy handed. -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: Don't register CRT connector when it's fused off
On Mon, Nov 23, 2015 at 05:07:29PM +0200, Ville Syrjälä wrote: > On Sat, Nov 21, 2015 at 10:44:12AM +, Chris Wilson wrote: > > On Fri, Nov 20, 2015 at 10:35:41PM +0200, ville.syrj...@linux.intel.com > > wrote: > > > From: Ville Syrjälä > > > > > > On some machines the CRT connector may be fused off. The weird thing > > > about this setup is that the ADPA register works otherwise normally, > > > except the enable bit is hardwired to 0. No one knows of any fuse > > > register that would tell us if this is the case, so the only thing we > > > can do (apart from a blacklist) is to try and set the enable bit and see > > > if it sticks. If not, we don't register the connector at all. Obviously > > > if the bit is already set when loading the driver we can just assume it > > > works. > > > > > > I've smoke tested this approach on several machines (GMCH and PCH), > > > some with actual CRT connectors, some with shadow connectors, and > > > obviously the machine (IVB) with the fused off connector. So far > > > I've not seen any ill effects from this probe. > > > > > > The main benefit is that we can actually run igt on machines with > > > fused off connectors, without totally upsetting the state checker. > > > > > > Signed-off-by: Ville Syrjälä > > > > If we can't enable the VGA port, there's not much we can do with it. > > > > Acked-by: Chris Wilson > > > > I'm just a bit hesistant, in case there are machine out there that > > simply do not report the hw state back (doubtful) or there are people > > who make use of the false VGA as a fake output (like me, or perhaps > > kvm-over-ip). I'll survive, I just use a fake HDMI output if there is no > > VGA. > > Based on my experience most machines that lack the physical connector > still have a fully working ADPA register. I have two such machines > myself. There's just this one machine at our office where it's > totally fused off, and running igt on it is no fun unless we get rid > of the connector. Patch merged, thanks for the ack. -- 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 09/12] drm/i915: Rename intel_context[engine].ringbuf
On Fri, Nov 20, 2015 at 12:43:49PM +, Chris Wilson wrote:
> Perform s/ringbuf/ring/ on the context struct for consistency with the
> ring/engine split.
>
> Signed-off-by: Chris Wilson
I presume all ringbuf are now gone?
Anyway, Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 4 +-
> drivers/gpu/drm/i915/i915_gpu_error.c | 4 +-
> drivers/gpu/drm/i915/intel_lrc.c| 85
> -
> drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++---
> 5 files changed, 52 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 56375c36b381..630717fec688 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1950,7 +1950,7 @@ static int i915_context_status(struct seq_file *m, void
> *unused)
> struct drm_i915_gem_object *ctx_obj =
> ctx->engine[i].state;
> struct intel_ringbuffer *ringbuf =
> - ctx->engine[i].ringbuf;
> + ctx->engine[i].ring;
>
> seq_printf(m, "%s: ", ring->name);
> if (ctx_obj)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b7eaa2deb437..d8bd58cbb727 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -882,9 +882,9 @@ struct intel_context {
>
> /* Execlists */
> bool rcs_initialized;
> - struct {
> + struct intel_context_engine {
> struct drm_i915_gem_object *state;
> - struct intel_ringbuffer *ringbuf;
> + struct intel_ringbuffer *ring;
> int pin_count;
> } engine[I915_NUM_RINGS];
>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 974e3481e449..8b37f72bd91f 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1040,9 +1040,9 @@ static void i915_gem_record_rings(struct drm_device
> *dev,
>* executed).
>*/
> if (request)
> - rbuf = request->ctx->engine[ring->id].ringbuf;
> + rbuf = request->ctx->engine[ring->id].ring;
> else
> - rbuf =
> ring->default_context->engine[ring->id].ringbuf;
> + rbuf =
> ring->default_context->engine[ring->id].ring;
> } else
> rbuf = ring->buffer;
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> b/drivers/gpu/drm/i915/intel_lrc.c
> index 346f5889738e..222ae8383f48 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -381,24 +381,24 @@ static void execlists_submit_requests(struct
> drm_i915_gem_request *rq0,
> execlists_elsp_write(rq0, rq1);
> }
>
> -static void execlists_context_unqueue(struct intel_engine_cs *ring)
> +static void execlists_context_unqueue(struct intel_engine_cs *engine)
> {
> struct drm_i915_gem_request *req0 = NULL, *req1 = NULL;
> struct drm_i915_gem_request *cursor = NULL, *tmp = NULL;
>
> - assert_spin_locked(&ring->execlist_lock);
> + assert_spin_locked(&engine->execlist_lock);
>
> /*
>* If irqs are not active generate a warning as batches that finish
>* without the irqs may get lost and a GPU Hang may occur.
>*/
> - WARN_ON(!intel_irqs_enabled(ring->dev->dev_private));
> + WARN_ON(!intel_irqs_enabled(engine->dev->dev_private));
>
> - if (list_empty(&ring->execlist_queue))
> + if (list_empty(&engine->execlist_queue))
> return;
>
> /* Try to read in pairs */
> - list_for_each_entry_safe(cursor, tmp, &ring->execlist_queue,
> + list_for_each_entry_safe(cursor, tmp, &engine->execlist_queue,
>execlist_link) {
> if (!req0) {
> req0 = cursor;
> @@ -408,7 +408,7 @@ static void execlists_context_unqueue(struct
> intel_engine_cs *ring)
> cursor->elsp_submitted = req0->elsp_submitted;
> list_del(&req0->execlist_link);
> list_add_tail(&req0->execlist_link,
> - &ring->execlist_retired_req_list);
> + &engine->execlist_retired_req_list);
> req0 = cursor;
> } else {
> req1 = cursor;
> @@ -416,7 +416,7 @@ static void execlists_context_unqueue(struct
> intel_engine_cs *ring)
> }
> }
>
> - if (IS_GEN8(ring->dev) || IS_GEN9(ring->dev)) {
> + if (IS_GEN8(engine->dev) ||
Re: [Intel-gfx] [PATCH 10/12] drm/i915: Reduce the pointer dance of i915_is_ggtt()
On Fri, Nov 20, 2015 at 12:43:50PM +, Chris Wilson wrote:
> The multiple levels of indirect do nothing but hinder the compiler and
> the pointer chasing turns to be quite painful but painless to fix.
>
> Signed-off-by: Chris Wilson
vma->is_ggtt = vm->is_ggtt is robust enough for me to carry it twice.
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_debugfs.c| 14 ++---
> drivers/gpu/drm/i915/i915_drv.h| 9 +
> drivers/gpu/drm/i915/i915_gem.c| 32
> +-
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 ++---
> drivers/gpu/drm/i915/i915_gem_gtt.c| 21 +---
> drivers/gpu/drm/i915/i915_gem_gtt.h| 3 +++
> drivers/gpu/drm/i915/i915_gpu_error.c | 2 +-
> drivers/gpu/drm/i915/i915_trace.h | 27 -
> 8 files changed, 44 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 630717fec688..d83f35c8df34 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -123,8 +123,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct
> drm_i915_gem_object *obj)
> struct i915_vma *vma;
>
> list_for_each_entry(vma, &obj->vma_list, vma_link) {
> - if (i915_is_ggtt(vma->vm) &&
> - drm_mm_node_allocated(&vma->node))
> + if (vma->is_ggtt && drm_mm_node_allocated(&vma->node))
> size += vma->node.size;
> }
>
> @@ -171,12 +170,11 @@ describe_obj(struct seq_file *m, struct
> drm_i915_gem_object *obj)
> seq_printf(m, " (fence: %d)", obj->fence_reg);
> list_for_each_entry(vma, &obj->vma_list, vma_link) {
> seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
> -i915_is_ggtt(vma->vm) ? "g" : "pp",
> +vma->is_ggtt ? "g" : "pp",
> vma->node.start, vma->node.size);
> - if (i915_is_ggtt(vma->vm))
> - seq_printf(m, ", type: %u)", vma->ggtt_view.type);
> - else
> - seq_puts(m, ")");
> + if (vma->is_ggtt)
> + seq_printf(m, ", type: %u", vma->ggtt_view.type);
> + seq_puts(m, ")");
> }
> if (obj->stolen)
> seq_printf(m, " (stolen: %08llx)", obj->stolen->start);
> @@ -348,7 +346,7 @@ static int per_file_stats(int id, void *ptr, void *data)
> if (!drm_mm_node_allocated(&vma->node))
> continue;
>
> - if (i915_is_ggtt(vma->vm)) {
> + if (vma->is_ggtt) {
> stats->global += obj->base.size;
> continue;
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d8bd58cbb727..e86e1188deb0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3023,18 +3023,11 @@ bool i915_gem_obj_is_pinned(struct
> drm_i915_gem_object *obj);
> /* Some GGTT VM helpers */
> #define i915_obj_to_ggtt(obj) \
> (&((struct drm_i915_private *)(obj)->base.dev->dev_private)->gtt.base)
> -static inline bool i915_is_ggtt(struct i915_address_space *vm)
> -{
> - struct i915_address_space *ggtt =
> - &((struct drm_i915_private *)(vm)->dev->dev_private)->gtt.base;
> - return vm == ggtt;
> -}
>
> static inline struct i915_hw_ppgtt *
> i915_vm_to_ppgtt(struct i915_address_space *vm)
> {
> - WARN_ON(i915_is_ggtt(vm));
> -
> + WARN_ON(vm->is_ggtt);
> return container_of(vm, struct i915_hw_ppgtt, base);
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d6706dd4117c..390cb9c15bad 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3300,8 +3300,7 @@ int i915_vma_unbind(struct i915_vma *vma)
>* cause memory corruption through use-after-free.
>*/
>
> - if (i915_is_ggtt(vma->vm) &&
> - vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {
> + if (vma->is_ggtt && vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {
> i915_gem_object_finish_gtt(obj);
>
> /* release the fence reg _after_ flushing */
> @@ -3316,7 +3315,7 @@ int i915_vma_unbind(struct i915_vma *vma)
> vma->bound = 0;
>
> list_del_init(&vma->mm_list);
> - if (i915_is_ggtt(vma->vm)) {
> + if (vma->is_ggtt) {
> if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL) {
> obj->map_and_fenceable = false;
> } else if (vma->ggtt_view.pages) {
> @@ -3427,7 +3426,7 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object
> *obj,
> struct i915_vma *vma;
> int ret;
>
> - if (i915_is_ggtt(vm)) {
> + if (vm->is_ggtt) {
>
Re: [Intel-gfx] [PATCH 07/12] drm/i915: Rename request->ringbuf to request->ring
On 20/11/15 12:43, Chris Wilson wrote:
Now that we have disambuigated ring and engine, we can use the clearer
and more consistent name for the intel_ringbuffer pointer in the
request.
Signed-off-by: Chris Wilson
---
drivers/gpu/drm/i915/i915_drv.h| 2 +-
drivers/gpu/drm/i915/i915_gem.c| 28 +++---
drivers/gpu/drm/i915/i915_gem_context.c| 2 +-
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +-
drivers/gpu/drm/i915/i915_gem_gtt.c| 6 +-
drivers/gpu/drm/i915/intel_display.c | 10 +-
drivers/gpu/drm/i915/intel_lrc.c | 149 ++---
drivers/gpu/drm/i915/intel_mocs.c | 32 +++
drivers/gpu/drm/i915/intel_overlay.c | 42
drivers/gpu/drm/i915/intel_ringbuffer.c| 86 -
10 files changed, 178 insertions(+), 183 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9ce8b3fcb3a0..b7eaa2deb437 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2185,7 +2185,7 @@ struct drm_i915_gem_request {
* context.
*/
struct intel_context *ctx;
- struct intel_ringbuffer *ringbuf;
+ struct intel_ringbuffer *ring;
What was the problem with ringbuf? Struct is still called ringbuf and
the files as well after the patch series.
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/12] drm/i915: Rename backpointer from intel_ringbuffer to intel_engine_cs
On Tue, Nov 24, 2015 at 03:58:13PM +0100, Daniel Vetter wrote: > On Fri, Nov 20, 2015 at 12:43:48PM +, Chris Wilson wrote: > > Having ringbuf->ring point to an engine is confusing, so rename it once > > again to ring->engine. > > > > Signed-off-by: Chris Wilson > > This massively conflicts with > > commit e84fe80337dc85cca07d0417ea97edbec4789d8b > Author: Nick Hoath > Date: Fri Sep 11 12:53:46 2015 +0100 > > drm/i915: Split alloc from init for lrc > > since this patch touches piles of old code now removed. But no functional > conflict afaics, so Acked-by: Daniel Vetter > > Maybe rebase before sending ;-) Depends on how much you value testing. -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 08/12] drm/i915: Rename backpointer from intel_ringbuffer to intel_engine_cs
On Tue, Nov 24, 2015 at 03:58:13PM +0100, Daniel Vetter wrote: > On Fri, Nov 20, 2015 at 12:43:48PM +, Chris Wilson wrote: > > Having ringbuf->ring point to an engine is confusing, so rename it once > > again to ring->engine. > > > > Signed-off-by: Chris Wilson > > This massively conflicts with > > commit e84fe80337dc85cca07d0417ea97edbec4789d8b > Author: Nick Hoath > Date: Fri Sep 11 12:53:46 2015 +0100 > > drm/i915: Split alloc from init for lrc That patch still didn't implement what I suggested in review to remove more code and reduce the layering violation. What I was trying to get Nick to achieve was http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=210680144b780b491de1de9b49125d9042054eb6 -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 09/12] drm/i915: Rename intel_context[engine].ringbuf
On 20/11/15 12:43, Chris Wilson wrote: Perform s/ringbuf/ring/ on the context struct for consistency with the ring/engine split. Signed-off-by: Chris Wilson --- drivers/gpu/drm/i915/i915_debugfs.c | 2 +- drivers/gpu/drm/i915/i915_drv.h | 4 +- drivers/gpu/drm/i915/i915_gpu_error.c | 4 +- drivers/gpu/drm/i915/intel_lrc.c| 85 - drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++--- 5 files changed, 52 insertions(+), 55 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 56375c36b381..630717fec688 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1950,7 +1950,7 @@ static int i915_context_status(struct seq_file *m, void *unused) struct drm_i915_gem_object *ctx_obj = ctx->engine[i].state; struct intel_ringbuffer *ringbuf = Elsewhere you were renaming this to ring, why not here? I am not sure it makes sense (renaming) but it is at least not consistent. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/12] drm/i915: Rename request->ringbuf to request->ring
On Tue, Nov 24, 2015 at 03:08:09PM +, Tvrtko Ursulin wrote:
>
> On 20/11/15 12:43, Chris Wilson wrote:
> >Now that we have disambuigated ring and engine, we can use the clearer
> >and more consistent name for the intel_ringbuffer pointer in the
> >request.
> >
> >Signed-off-by: Chris Wilson
> >---
> > drivers/gpu/drm/i915/i915_drv.h| 2 +-
> > drivers/gpu/drm/i915/i915_gem.c| 28 +++---
> > drivers/gpu/drm/i915/i915_gem_context.c| 2 +-
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 4 +-
> > drivers/gpu/drm/i915/i915_gem_gtt.c| 6 +-
> > drivers/gpu/drm/i915/intel_display.c | 10 +-
> > drivers/gpu/drm/i915/intel_lrc.c | 149
> > ++---
> > drivers/gpu/drm/i915/intel_mocs.c | 32 +++
> > drivers/gpu/drm/i915/intel_overlay.c | 42
> > drivers/gpu/drm/i915/intel_ringbuffer.c| 86 -
> > 10 files changed, 178 insertions(+), 183 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >b/drivers/gpu/drm/i915/i915_drv.h
> >index 9ce8b3fcb3a0..b7eaa2deb437 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -2185,7 +2185,7 @@ struct drm_i915_gem_request {
> > * context.
> > */
> > struct intel_context *ctx;
> >-struct intel_ringbuffer *ringbuf;
> >+struct intel_ringbuffer *ring;
>
> What was the problem with ringbuf? Struct is still called ringbuf
> and the files as well after the patch series.
It introduced a major naming clash with existing code. I am trying to
remove the needlessly duplicated interfaces, and restore the historic
naming conventions.
-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 09/12] drm/i915: Rename intel_context[engine].ringbuf
On Tue, Nov 24, 2015 at 03:09:42PM +, Tvrtko Ursulin wrote: > > On 20/11/15 12:43, Chris Wilson wrote: > >Perform s/ringbuf/ring/ on the context struct for consistency with the > >ring/engine split. > > > >Signed-off-by: Chris Wilson > >--- > > drivers/gpu/drm/i915/i915_debugfs.c | 2 +- > > drivers/gpu/drm/i915/i915_drv.h | 4 +- > > drivers/gpu/drm/i915/i915_gpu_error.c | 4 +- > > drivers/gpu/drm/i915/intel_lrc.c| 85 > > - > > drivers/gpu/drm/i915/intel_ringbuffer.c | 12 ++--- > > 5 files changed, 52 insertions(+), 55 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > >b/drivers/gpu/drm/i915/i915_debugfs.c > >index 56375c36b381..630717fec688 100644 > >--- a/drivers/gpu/drm/i915/i915_debugfs.c > >+++ b/drivers/gpu/drm/i915/i915_debugfs.c > >@@ -1950,7 +1950,7 @@ static int i915_context_status(struct seq_file *m, > >void *unused) > > struct drm_i915_gem_object *ctx_obj = > > ctx->engine[i].state; > > struct intel_ringbuffer *ringbuf = > > Elsewhere you were renaming this to ring, why not here? I am not > sure it makes sense (renaming) but it is at least not consistent. If I change everything at once, the patches get ignored. -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
