Re: [Intel-gfx] [PATCH 3/4] drm/i915: Switch to full atomic helpers for plane updates/disable, take two
-Original Message-
From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of
Matt Roper
Sent: Thursday, April 09, 2015 11:04 AM
To: Daniel Vetter
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915: Switch to full atomic helpers
for
plane updates/disable, take two
On Thu, Apr 09, 2015 at 02:46:19PM +0200, Daniel Vetter wrote:
On Wed, Apr 08, 2015 at 06:56:53PM -0700, Matt Roper wrote:
Switch from our plane update/disable entrypoints to use the full
atomic helpers (which generate a top-level atomic transaction)
rather than the transitional helpers (which only create/manipulate
orphaned plane states independent of a top-level transaction).
Various upcoming work (SKL scalers, atomic watermarks, etc.)
requires a full atomic transaction to behave properly/cleanly.
Last time we tried this, we had to back out the change because we
still call the drm_plane vfuncs directly from within our legacy
modesetting code. This potentially results in nested atomic
transactions, locking collisions, and other failures. To avoid that
problem again, we sidestep the issue by calling the transitional
helpers directly (rather than through a vfunc) when we're nested
inside of other legacy modesetting code.
Matt,
I rebased scaler patch 13/14 on top of 90/270 but hit into issues.
After much debugging, found below:
By keeping transitional helpers for some scenarios as you mentioned,
this is leaving intel_plane_state-src/dst rect values with 0s.
I recall having a src/dst rects copy in intel_plane_state is to
hold modified values due to any clipping etc. And modified values
will be used in platform specific update function
(i.e., skylake_update_primary_plane()). From these paths,
src/dst rects in both drm and intel plane states are same and works
by accessing drm_plane_state to make it work, but that isn't true
in other cases and should use intel_plane_state src/rects only.
Working on a fix but any suggestions/comments welcome...
However this does allow
legacy SetPlane() ioctl's to process an entire drm_atomic_state
transaction, which is important for upcoming patches.
Cc: Chandra Konduru chandra.kond...@intel.com
Signed-off-by: Matt Roper matthew.d.ro...@intel.com
Maarten is working to fix this properly (by wiring up plane states to
the transitional drm_atomic_state scaffolding from Ander), but seems
like a good interim idea to get back some solid testing for our atomic code.
Can I apply this without patches 12 right away or is there a tricky
depency?
-Daniel
I think this one can be applied independently of 12. I think this one might
be a
prereq for #4 to fully work as advertised though...without using the full
atomic
helpers, we simply don't have a 'start of transaction' point at which we can
clear
the existing atomic flags when using legacy plane ioctls.
Matt
---
drivers/gpu/drm/i915/intel_display.c | 24
drivers/gpu/drm/i915/intel_sprite.c | 10 +-
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 1cf91ad..3a74923 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5576,7 +5576,7 @@ static void intel_crtc_disable(struct drm_crtc
*crtc)
dev_priv-display.crtc_disable(crtc);
dev_priv-display.off(crtc);
- crtc-primary-funcs-disable_plane(crtc-primary);
+ drm_plane_helper_disable(crtc-primary);
/* Update computed state. */
list_for_each_entry(connector, dev-mode_config.connector_list,
head) { @@ -11731,11 +11731,11 @@ static int __intel_set_mode(struct
drm_crtc *crtc,
int vdisplay, hdisplay;
drm_crtc_get_hv_timing(mode, hdisplay, vdisplay);
- ret = primary-funcs-update_plane(primary, intel_crtc-base,
-fb, 0, 0,
-hdisplay, vdisplay,
-x 16, y 16,
-hdisplay 16, vdisplay
16);
+ ret = drm_plane_helper_update(primary, intel_crtc-base,
+ fb, 0, 0,
+ hdisplay, vdisplay,
+ x 16, y 16,
+ hdisplay 16, vdisplay 16);
}
/* Now enable the clocks, plane, pipe, and connectors that we set
up. */ @@ -12286,10 +12286,10 @@ static int intel_crtc_set_config(struct
drm_mode_set *set)
int vdisplay, hdisplay;
drm_crtc_get_hv_timing(set-mode, hdisplay, vdisplay);
- ret = primary-funcs-update_plane(primary, set-crtc, set-fb,
-0, 0, hdisplay
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Switch to full atomic helpers for plane updates/disable, take two
On Thu, Apr 09, 2015 at 02:46:19PM +0200, Daniel Vetter wrote:
On Wed, Apr 08, 2015 at 06:56:53PM -0700, Matt Roper wrote:
Switch from our plane update/disable entrypoints to use the full atomic
helpers (which generate a top-level atomic transaction) rather than the
transitional helpers (which only create/manipulate orphaned plane states
independent of a top-level transaction). Various upcoming work (SKL
scalers, atomic watermarks, etc.) requires a full atomic transaction to
behave properly/cleanly.
Last time we tried this, we had to back out the change because we still
call the drm_plane vfuncs directly from within our legacy modesetting
code. This potentially results in nested atomic transactions, locking
collisions, and other failures. To avoid that problem again, we
sidestep the issue by calling the transitional helpers directly (rather
than through a vfunc) when we're nested inside of other legacy
modesetting code. However this does allow legacy SetPlane() ioctl's to
process an entire drm_atomic_state transaction, which is important for
upcoming patches.
Cc: Chandra Konduru chandra.kond...@intel.com
Signed-off-by: Matt Roper matthew.d.ro...@intel.com
Maarten is working to fix this properly (by wiring up plane states to the
transitional drm_atomic_state scaffolding from Ander), but seems like a
good interim idea to get back some solid testing for our atomic code.
Can I apply this without patches 12 right away or is there a tricky
depency?
-Daniel
I think this one can be applied independently of 12. I think this one
might be a prereq for #4 to fully work as advertised though...without
using the full atomic helpers, we simply don't have a 'start of
transaction' point at which we can clear the existing atomic flags when
using legacy plane ioctls.
Matt
---
drivers/gpu/drm/i915/intel_display.c | 24
drivers/gpu/drm/i915/intel_sprite.c | 10 +-
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 1cf91ad..3a74923 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5576,7 +5576,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
dev_priv-display.crtc_disable(crtc);
dev_priv-display.off(crtc);
- crtc-primary-funcs-disable_plane(crtc-primary);
+ drm_plane_helper_disable(crtc-primary);
/* Update computed state. */
list_for_each_entry(connector, dev-mode_config.connector_list, head) {
@@ -11731,11 +11731,11 @@ static int __intel_set_mode(struct drm_crtc *crtc,
int vdisplay, hdisplay;
drm_crtc_get_hv_timing(mode, hdisplay, vdisplay);
- ret = primary-funcs-update_plane(primary, intel_crtc-base,
- fb, 0, 0,
- hdisplay, vdisplay,
- x 16, y 16,
- hdisplay 16, vdisplay
16);
+ ret = drm_plane_helper_update(primary, intel_crtc-base,
+ fb, 0, 0,
+ hdisplay, vdisplay,
+ x 16, y 16,
+ hdisplay 16, vdisplay 16);
}
/* Now enable the clocks, plane, pipe, and connectors that we set up. */
@@ -12286,10 +12286,10 @@ static int intel_crtc_set_config(struct
drm_mode_set *set)
int vdisplay, hdisplay;
drm_crtc_get_hv_timing(set-mode, hdisplay, vdisplay);
- ret = primary-funcs-update_plane(primary, set-crtc, set-fb,
- 0, 0, hdisplay, vdisplay,
- set-x 16, set-y 16,
- hdisplay 16, vdisplay
16);
+ ret = drm_plane_helper_update(primary, set-crtc, set-fb,
+ 0, 0, hdisplay, vdisplay,
+ set-x 16, set-y 16,
+ hdisplay 16, vdisplay 16);
/*
* We need to make sure the primary plane is re-enabled if it
@@ -12830,8 +12830,8 @@ void intel_plane_destroy(struct drm_plane *plane)
}
const struct drm_plane_funcs intel_plane_funcs = {
- .update_plane = drm_plane_helper_update,
- .disable_plane = drm_plane_helper_disable,
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
.destroy = intel_plane_destroy,
.set_property = drm_atomic_helper_plane_set_property,
.atomic_get_property = intel_plane_atomic_get_property,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Switch to full atomic helpers for plane updates/disable, take two
On Wed, Apr 08, 2015 at 06:56:53PM -0700, Matt Roper wrote:
Switch from our plane update/disable entrypoints to use the full atomic
helpers (which generate a top-level atomic transaction) rather than the
transitional helpers (which only create/manipulate orphaned plane states
independent of a top-level transaction). Various upcoming work (SKL
scalers, atomic watermarks, etc.) requires a full atomic transaction to
behave properly/cleanly.
Last time we tried this, we had to back out the change because we still
call the drm_plane vfuncs directly from within our legacy modesetting
code. This potentially results in nested atomic transactions, locking
collisions, and other failures. To avoid that problem again, we
sidestep the issue by calling the transitional helpers directly (rather
than through a vfunc) when we're nested inside of other legacy
modesetting code. However this does allow legacy SetPlane() ioctl's to
process an entire drm_atomic_state transaction, which is important for
upcoming patches.
Cc: Chandra Konduru chandra.kond...@intel.com
Signed-off-by: Matt Roper matthew.d.ro...@intel.com
Maarten is working to fix this properly (by wiring up plane states to the
transitional drm_atomic_state scaffolding from Ander), but seems like a
good interim idea to get back some solid testing for our atomic code.
Can I apply this without patches 12 right away or is there a tricky
depency?
-Daniel
---
drivers/gpu/drm/i915/intel_display.c | 24
drivers/gpu/drm/i915/intel_sprite.c | 10 +-
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 1cf91ad..3a74923 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5576,7 +5576,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
dev_priv-display.crtc_disable(crtc);
dev_priv-display.off(crtc);
- crtc-primary-funcs-disable_plane(crtc-primary);
+ drm_plane_helper_disable(crtc-primary);
/* Update computed state. */
list_for_each_entry(connector, dev-mode_config.connector_list, head) {
@@ -11731,11 +11731,11 @@ static int __intel_set_mode(struct drm_crtc *crtc,
int vdisplay, hdisplay;
drm_crtc_get_hv_timing(mode, hdisplay, vdisplay);
- ret = primary-funcs-update_plane(primary, intel_crtc-base,
-fb, 0, 0,
-hdisplay, vdisplay,
-x 16, y 16,
-hdisplay 16, vdisplay
16);
+ ret = drm_plane_helper_update(primary, intel_crtc-base,
+ fb, 0, 0,
+ hdisplay, vdisplay,
+ x 16, y 16,
+ hdisplay 16, vdisplay 16);
}
/* Now enable the clocks, plane, pipe, and connectors that we set up. */
@@ -12286,10 +12286,10 @@ static int intel_crtc_set_config(struct
drm_mode_set *set)
int vdisplay, hdisplay;
drm_crtc_get_hv_timing(set-mode, hdisplay, vdisplay);
- ret = primary-funcs-update_plane(primary, set-crtc, set-fb,
-0, 0, hdisplay, vdisplay,
-set-x 16, set-y 16,
-hdisplay 16, vdisplay
16);
+ ret = drm_plane_helper_update(primary, set-crtc, set-fb,
+ 0, 0, hdisplay, vdisplay,
+ set-x 16, set-y 16,
+ hdisplay 16, vdisplay 16);
/*
* We need to make sure the primary plane is re-enabled if it
@@ -12830,8 +12830,8 @@ void intel_plane_destroy(struct drm_plane *plane)
}
const struct drm_plane_funcs intel_plane_funcs = {
- .update_plane = drm_plane_helper_update,
- .disable_plane = drm_plane_helper_disable,
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
.destroy = intel_plane_destroy,
.set_property = drm_atomic_helper_plane_set_property,
.atomic_get_property = intel_plane_atomic_get_property,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index 492abcd..f4094d2 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1149,11 +1149,11 @@ int intel_plane_restore(struct drm_plane *plane)
if (!plane-crtc || !plane-state-fb)
return 0;
- return plane-funcs-update_plane(plane, plane-crtc, plane-state-fb,
-
[Intel-gfx] [PATCH 3/4] drm/i915: Switch to full atomic helpers for plane updates/disable, take two
Switch from our plane update/disable entrypoints to use the full atomic
helpers (which generate a top-level atomic transaction) rather than the
transitional helpers (which only create/manipulate orphaned plane states
independent of a top-level transaction). Various upcoming work (SKL
scalers, atomic watermarks, etc.) requires a full atomic transaction to
behave properly/cleanly.
Last time we tried this, we had to back out the change because we still
call the drm_plane vfuncs directly from within our legacy modesetting
code. This potentially results in nested atomic transactions, locking
collisions, and other failures. To avoid that problem again, we
sidestep the issue by calling the transitional helpers directly (rather
than through a vfunc) when we're nested inside of other legacy
modesetting code. However this does allow legacy SetPlane() ioctl's to
process an entire drm_atomic_state transaction, which is important for
upcoming patches.
Cc: Chandra Konduru chandra.kond...@intel.com
Signed-off-by: Matt Roper matthew.d.ro...@intel.com
---
drivers/gpu/drm/i915/intel_display.c | 24
drivers/gpu/drm/i915/intel_sprite.c | 10 +-
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 1cf91ad..3a74923 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5576,7 +5576,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
dev_priv-display.crtc_disable(crtc);
dev_priv-display.off(crtc);
- crtc-primary-funcs-disable_plane(crtc-primary);
+ drm_plane_helper_disable(crtc-primary);
/* Update computed state. */
list_for_each_entry(connector, dev-mode_config.connector_list, head) {
@@ -11731,11 +11731,11 @@ static int __intel_set_mode(struct drm_crtc *crtc,
int vdisplay, hdisplay;
drm_crtc_get_hv_timing(mode, hdisplay, vdisplay);
- ret = primary-funcs-update_plane(primary, intel_crtc-base,
- fb, 0, 0,
- hdisplay, vdisplay,
- x 16, y 16,
- hdisplay 16, vdisplay
16);
+ ret = drm_plane_helper_update(primary, intel_crtc-base,
+ fb, 0, 0,
+ hdisplay, vdisplay,
+ x 16, y 16,
+ hdisplay 16, vdisplay 16);
}
/* Now enable the clocks, plane, pipe, and connectors that we set up. */
@@ -12286,10 +12286,10 @@ static int intel_crtc_set_config(struct drm_mode_set
*set)
int vdisplay, hdisplay;
drm_crtc_get_hv_timing(set-mode, hdisplay, vdisplay);
- ret = primary-funcs-update_plane(primary, set-crtc, set-fb,
- 0, 0, hdisplay, vdisplay,
- set-x 16, set-y 16,
- hdisplay 16, vdisplay
16);
+ ret = drm_plane_helper_update(primary, set-crtc, set-fb,
+ 0, 0, hdisplay, vdisplay,
+ set-x 16, set-y 16,
+ hdisplay 16, vdisplay 16);
/*
* We need to make sure the primary plane is re-enabled if it
@@ -12830,8 +12830,8 @@ void intel_plane_destroy(struct drm_plane *plane)
}
const struct drm_plane_funcs intel_plane_funcs = {
- .update_plane = drm_plane_helper_update,
- .disable_plane = drm_plane_helper_disable,
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
.destroy = intel_plane_destroy,
.set_property = drm_atomic_helper_plane_set_property,
.atomic_get_property = intel_plane_atomic_get_property,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index 492abcd..f4094d2 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1149,11 +1149,11 @@ int intel_plane_restore(struct drm_plane *plane)
if (!plane-crtc || !plane-state-fb)
return 0;
- return plane-funcs-update_plane(plane, plane-crtc, plane-state-fb,
- plane-state-crtc_x, plane-state-crtc_y,
- plane-state-crtc_w, plane-state-crtc_h,
- plane-state-src_x, plane-state-src_y,
- plane-state-src_w, plane-state-src_h);
+ return drm_plane_helper_update(plane, plane-crtc, plane-state-fb,
+
