Re: [Intel-gfx] [PATCH v2 07/20] drm/i915: Rework primary plane stuff slightly.
On Tue, Jul 07, 2015 at 04:02:32PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 13:16 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:18AM +0200, Maarten Lankhorst wrote:
Make sure the primary plane is set up correctly. This is done by
setting plane_state-src and plane_state-crtc.
All non-primary planes get disabled.
Too terse commit message, fails to mention that this is about hw
readout completely. Also should mention that this removes the
initial_planes hack.
Signed-off-by: Maarten Lankhorst maarten.lankho...@linux.intel.com
---
drivers/gpu/drm/i915/intel_atomic.c | 7 --
drivers/gpu/drm/i915/intel_display.c | 167
+--
drivers/gpu/drm/i915/intel_drv.h | 2 -
3 files changed, 60 insertions(+), 116 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c
b/drivers/gpu/drm/i915/intel_atomic.c
index 429677a111d5..b593612a917d 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -98,13 +98,6 @@ int intel_atomic_check(struct drm_device *dev,
return -EINVAL;
}
- if (crtc_state
- crtc_state-quirks PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
- ret = drm_atomic_add_affected_planes(state,
nuclear_crtc-base);
- if (ret)
- return ret;
- }
-
ret = drm_atomic_helper_check_planes(dev, state);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index eb7c2e2819b7..fa1102392eca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -109,6 +109,7 @@ static void skl_init_scalers(struct drm_device *dev,
struct intel_crtc *intel_cr
struct intel_crtc_state *crtc_state);
static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
int num_connectors);
+static void intel_pre_disable_primary(struct drm_crtc *crtc);
static struct intel_encoder *intel_find_encoder(struct intel_connector
*connector, int pipe)
{
@@ -2582,11 +2583,12 @@ intel_find_initial_plane_obj(struct intel_crtc
*intel_crtc,
{
struct drm_device *dev = intel_crtc-base.dev;
struct drm_i915_private *dev_priv = dev-dev_private;
- struct drm_crtc *c;
- struct intel_crtc *i;
struct drm_i915_gem_object *obj;
- struct drm_plane *primary = intel_crtc-base.primary;
struct drm_framebuffer *fb;
+ struct drm_plane *primary = intel_crtc-base.primary;
+ struct intel_plane *p;
+ struct intel_plane_state *plane_state =
+ to_intel_plane_state(primary-state);
if (!plane_config-fb)
return;
@@ -2602,16 +2604,11 @@ intel_find_initial_plane_obj(struct intel_crtc
*intel_crtc,
* Failed to alloc the obj, check to see if we should share
* an fb with another CRTC instead
*/
- for_each_crtc(dev, c) {
- i = to_intel_crtc(c);
-
- if (c == intel_crtc-base)
- continue;
-
- if (!i-active)
+ for_each_intel_plane(dev, p) {
+ if (p-base.type != DRM_PLANE_TYPE_PRIMARY)
continue;
- fb = c-primary-fb;
+ fb = p-base.state-fb;
This seems to break the sharing logic completely: We want to check primary
planes of all other crtcs to see whether we could merged the fb together.
We don't even read out plane state for non-primary planes, so the below fb
check will never be non-NULL.
I thought this was about multiple planes sharing the same fb with same offset.
And as such checking for the crtc is unnecessary, for the current crtc it
will be be NULL here.
This only reads out the current fb, not different ones.
And sharing the same fb with other crtc's is done in intel_fbdev_init_bios.
This is about sharing the same fb but across different crtcs - bios never
enables more than the primary plane anyway. And you can't rely upon
fbdev_init_bios since that's not run at all when I915_FBDEV=n.
So yes with current code this loop here reconstruct the shared between
primary planes on different crtcs (if the stolen allocator tells us that
our range is occupied already). fbdev_init_bios just tries to create a
config matching the one the bios has set up (and then pick a suitable fb
for fbcon from the ones already allocated).
Maybe we should extract this as try_to_find_shared_fb or similar to make
the code self-explanatory?
if (!fb)
continue;
@@ -2622,18 +2619,28 @@ intel_find_initial_plane_obj(struct intel_crtc
*intel_crtc,
}
}
+ intel_pre_disable_primary(intel_crtc-base);
+ to_intel_plane(primary)-disable_plane(primary, intel_crtc-base);
+
return;
valid_fb:
+ drm_framebuffer_reference(fb);
+ primary-fb = plane_state-base.fb = fb;
+ plane_state-base.crtc = primary-crtc =
Re: [Intel-gfx] [PATCH v2 07/20] drm/i915: Rework primary plane stuff slightly.
Op 08-07-15 om 11:27 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 04:02:32PM +0200, Maarten Lankhorst wrote:
Op 07-07-15 om 13:16 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:18AM +0200, Maarten Lankhorst wrote:
Make sure the primary plane is set up correctly. This is done by
setting plane_state-src and plane_state-crtc.
All non-primary planes get disabled.
Too terse commit message, fails to mention that this is about hw
readout completely. Also should mention that this removes the
initial_planes hack.
Signed-off-by: Maarten Lankhorst maarten.lankho...@linux.intel.com
---
drivers/gpu/drm/i915/intel_atomic.c | 7 --
drivers/gpu/drm/i915/intel_display.c | 167
+--
drivers/gpu/drm/i915/intel_drv.h | 2 -
3 files changed, 60 insertions(+), 116 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c
b/drivers/gpu/drm/i915/intel_atomic.c
index 429677a111d5..b593612a917d 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -98,13 +98,6 @@ int intel_atomic_check(struct drm_device *dev,
return -EINVAL;
}
- if (crtc_state
- crtc_state-quirks PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
- ret = drm_atomic_add_affected_planes(state,
nuclear_crtc-base);
- if (ret)
- return ret;
- }
-
ret = drm_atomic_helper_check_planes(dev, state);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index eb7c2e2819b7..fa1102392eca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -109,6 +109,7 @@ static void skl_init_scalers(struct drm_device *dev,
struct intel_crtc *intel_cr
struct intel_crtc_state *crtc_state);
static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
int num_connectors);
+static void intel_pre_disable_primary(struct drm_crtc *crtc);
static struct intel_encoder *intel_find_encoder(struct intel_connector
*connector, int pipe)
{
@@ -2582,11 +2583,12 @@ intel_find_initial_plane_obj(struct intel_crtc
*intel_crtc,
{
struct drm_device *dev = intel_crtc-base.dev;
struct drm_i915_private *dev_priv = dev-dev_private;
- struct drm_crtc *c;
- struct intel_crtc *i;
struct drm_i915_gem_object *obj;
- struct drm_plane *primary = intel_crtc-base.primary;
struct drm_framebuffer *fb;
+ struct drm_plane *primary = intel_crtc-base.primary;
+ struct intel_plane *p;
+ struct intel_plane_state *plane_state =
+ to_intel_plane_state(primary-state);
if (!plane_config-fb)
return;
@@ -2602,16 +2604,11 @@ intel_find_initial_plane_obj(struct intel_crtc
*intel_crtc,
* Failed to alloc the obj, check to see if we should share
* an fb with another CRTC instead
*/
- for_each_crtc(dev, c) {
- i = to_intel_crtc(c);
-
- if (c == intel_crtc-base)
- continue;
-
- if (!i-active)
+ for_each_intel_plane(dev, p) {
+ if (p-base.type != DRM_PLANE_TYPE_PRIMARY)
continue;
- fb = c-primary-fb;
+ fb = p-base.state-fb;
This seems to break the sharing logic completely: We want to check primary
planes of all other crtcs to see whether we could merged the fb together.
We don't even read out plane state for non-primary planes, so the below fb
check will never be non-NULL.
I thought this was about multiple planes sharing the same fb with same
offset.
And as such checking for the crtc is unnecessary, for the current crtc it
will be be NULL here.
This only reads out the current fb, not different ones.
And sharing the same fb with other crtc's is done in intel_fbdev_init_bios.
This is about sharing the same fb but across different crtcs - bios never
enables more than the primary plane anyway. And you can't rely upon
fbdev_init_bios since that's not run at all when I915_FBDEV=n.
So yes with current code this loop here reconstruct the shared between
primary planes on different crtcs (if the stolen allocator tells us that
our range is occupied already). fbdev_init_bios just tries to create a
config matching the one the bios has set up (and then pick a suitable fb
for fbcon from the ones already allocated).
Maybe we should extract this as try_to_find_shared_fb or similar to make
the code self-explanatory?
if (!fb)
continue;
@@ -2622,18 +2619,28 @@ intel_find_initial_plane_obj(struct intel_crtc
*intel_crtc,
}
}
+ intel_pre_disable_primary(intel_crtc-base);
+ to_intel_plane(primary)-disable_plane(primary, intel_crtc-base);
+
return;
valid_fb:
+ drm_framebuffer_reference(fb);
+ primary-fb = plane_state-base.fb = fb;
+ plane_state-base.crtc = primary-crtc = intel_crtc-base;
+
+ plane_state-base.src_x = plane_state-base.src_y
Re: [Intel-gfx] [PATCH v2 07/20] drm/i915: Rework primary plane stuff slightly.
On Tue, Jul 07, 2015 at 09:08:18AM +0200, Maarten Lankhorst wrote:
Make sure the primary plane is set up correctly. This is done by
setting plane_state-src and plane_state-crtc.
All non-primary planes get disabled.
Too terse commit message, fails to mention that this is about hw
readout completely. Also should mention that this removes the
initial_planes hack.
Signed-off-by: Maarten Lankhorst maarten.lankho...@linux.intel.com
---
drivers/gpu/drm/i915/intel_atomic.c | 7 --
drivers/gpu/drm/i915/intel_display.c | 167
+--
drivers/gpu/drm/i915/intel_drv.h | 2 -
3 files changed, 60 insertions(+), 116 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c
b/drivers/gpu/drm/i915/intel_atomic.c
index 429677a111d5..b593612a917d 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -98,13 +98,6 @@ int intel_atomic_check(struct drm_device *dev,
return -EINVAL;
}
- if (crtc_state
- crtc_state-quirks PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
- ret = drm_atomic_add_affected_planes(state,
nuclear_crtc-base);
- if (ret)
- return ret;
- }
-
ret = drm_atomic_helper_check_planes(dev, state);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index eb7c2e2819b7..fa1102392eca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -109,6 +109,7 @@ static void skl_init_scalers(struct drm_device *dev,
struct intel_crtc *intel_cr
struct intel_crtc_state *crtc_state);
static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
int num_connectors);
+static void intel_pre_disable_primary(struct drm_crtc *crtc);
static struct intel_encoder *intel_find_encoder(struct intel_connector
*connector, int pipe)
{
@@ -2582,11 +2583,12 @@ intel_find_initial_plane_obj(struct intel_crtc
*intel_crtc,
{
struct drm_device *dev = intel_crtc-base.dev;
struct drm_i915_private *dev_priv = dev-dev_private;
- struct drm_crtc *c;
- struct intel_crtc *i;
struct drm_i915_gem_object *obj;
- struct drm_plane *primary = intel_crtc-base.primary;
struct drm_framebuffer *fb;
+ struct drm_plane *primary = intel_crtc-base.primary;
+ struct intel_plane *p;
+ struct intel_plane_state *plane_state =
+ to_intel_plane_state(primary-state);
if (!plane_config-fb)
return;
@@ -2602,16 +2604,11 @@ intel_find_initial_plane_obj(struct intel_crtc
*intel_crtc,
* Failed to alloc the obj, check to see if we should share
* an fb with another CRTC instead
*/
- for_each_crtc(dev, c) {
- i = to_intel_crtc(c);
-
- if (c == intel_crtc-base)
- continue;
-
- if (!i-active)
+ for_each_intel_plane(dev, p) {
+ if (p-base.type != DRM_PLANE_TYPE_PRIMARY)
continue;
- fb = c-primary-fb;
+ fb = p-base.state-fb;
This seems to break the sharing logic completely: We want to check primary
planes of all other crtcs to see whether we could merged the fb together.
We don't even read out plane state for non-primary planes, so the below fb
check will never be non-NULL.
if (!fb)
continue;
@@ -2622,18 +2619,28 @@ intel_find_initial_plane_obj(struct intel_crtc
*intel_crtc,
}
}
+ intel_pre_disable_primary(intel_crtc-base);
+ to_intel_plane(primary)-disable_plane(primary, intel_crtc-base);
+
return;
valid_fb:
+ drm_framebuffer_reference(fb);
+ primary-fb = plane_state-base.fb = fb;
+ plane_state-base.crtc = primary-crtc = intel_crtc-base;
+
+ plane_state-base.src_x = plane_state-base.src_y = 0;
+ plane_state-base.src_w = fb-width 16;
+ plane_state-base.src_h = fb-height 16;
+
+ plane_state-base.crtc_x = plane_state-base.src_y = 0;
+ plane_state-base.crtc_w = fb-width;
+ plane_state-base.crtc_h = fb-height;
+
+ plane_state-visible = true;
obj = intel_fb_obj(fb);
if (obj-tiling_mode != I915_TILING_NONE)
dev_priv-preserve_bios_swizzle = true;
-
- primary-fb = fb;
- primary-crtc = primary-state-crtc = intel_crtc-base;
- update_state_fb(primary);
Do we still have other users of update_state_fb left?
- intel_crtc-base.state-plane_mask |= (1 drm_plane_index(primary));
- obj-frontbuffer_bits |= to_intel_plane(primary)-frontbuffer_bit;
}
static void i9xx_update_primary_plane(struct drm_crtc *crtc,
@@ -11761,34 +11768,6 @@ static bool check_encoder_cloning(struct
drm_atomic_state *state,
return true;
}
-static void
Re: [Intel-gfx] [PATCH v2 07/20] drm/i915: Rework primary plane stuff slightly.
Op 07-07-15 om 13:16 schreef Daniel Vetter:
On Tue, Jul 07, 2015 at 09:08:18AM +0200, Maarten Lankhorst wrote:
Make sure the primary plane is set up correctly. This is done by
setting plane_state-src and plane_state-crtc.
All non-primary planes get disabled.
Too terse commit message, fails to mention that this is about hw
readout completely. Also should mention that this removes the
initial_planes hack.
Signed-off-by: Maarten Lankhorst maarten.lankho...@linux.intel.com
---
drivers/gpu/drm/i915/intel_atomic.c | 7 --
drivers/gpu/drm/i915/intel_display.c | 167
+--
drivers/gpu/drm/i915/intel_drv.h | 2 -
3 files changed, 60 insertions(+), 116 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c
b/drivers/gpu/drm/i915/intel_atomic.c
index 429677a111d5..b593612a917d 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -98,13 +98,6 @@ int intel_atomic_check(struct drm_device *dev,
return -EINVAL;
}
-if (crtc_state
-crtc_state-quirks PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
-ret = drm_atomic_add_affected_planes(state,
nuclear_crtc-base);
-if (ret)
-return ret;
-}
-
ret = drm_atomic_helper_check_planes(dev, state);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index eb7c2e2819b7..fa1102392eca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -109,6 +109,7 @@ static void skl_init_scalers(struct drm_device *dev,
struct intel_crtc *intel_cr
struct intel_crtc_state *crtc_state);
static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state,
int num_connectors);
+static void intel_pre_disable_primary(struct drm_crtc *crtc);
static struct intel_encoder *intel_find_encoder(struct intel_connector
*connector, int pipe)
{
@@ -2582,11 +2583,12 @@ intel_find_initial_plane_obj(struct intel_crtc
*intel_crtc,
{
struct drm_device *dev = intel_crtc-base.dev;
struct drm_i915_private *dev_priv = dev-dev_private;
-struct drm_crtc *c;
-struct intel_crtc *i;
struct drm_i915_gem_object *obj;
-struct drm_plane *primary = intel_crtc-base.primary;
struct drm_framebuffer *fb;
+struct drm_plane *primary = intel_crtc-base.primary;
+struct intel_plane *p;
+struct intel_plane_state *plane_state =
+to_intel_plane_state(primary-state);
if (!plane_config-fb)
return;
@@ -2602,16 +2604,11 @@ intel_find_initial_plane_obj(struct intel_crtc
*intel_crtc,
* Failed to alloc the obj, check to see if we should share
* an fb with another CRTC instead
*/
-for_each_crtc(dev, c) {
-i = to_intel_crtc(c);
-
-if (c == intel_crtc-base)
-continue;
-
-if (!i-active)
+for_each_intel_plane(dev, p) {
+if (p-base.type != DRM_PLANE_TYPE_PRIMARY)
continue;
-fb = c-primary-fb;
+fb = p-base.state-fb;
This seems to break the sharing logic completely: We want to check primary
planes of all other crtcs to see whether we could merged the fb together.
We don't even read out plane state for non-primary planes, so the below fb
check will never be non-NULL.
I thought this was about multiple planes sharing the same fb with same offset.
And as such checking for the crtc is unnecessary, for the current crtc it will
be be NULL here.
This only reads out the current fb, not different ones.
And sharing the same fb with other crtc's is done in intel_fbdev_init_bios.
if (!fb)
continue;
@@ -2622,18 +2619,28 @@ intel_find_initial_plane_obj(struct intel_crtc
*intel_crtc,
}
}
+intel_pre_disable_primary(intel_crtc-base);
+to_intel_plane(primary)-disable_plane(primary, intel_crtc-base);
+
return;
valid_fb:
+drm_framebuffer_reference(fb);
+primary-fb = plane_state-base.fb = fb;
+plane_state-base.crtc = primary-crtc = intel_crtc-base;
+
+plane_state-base.src_x = plane_state-base.src_y = 0;
+plane_state-base.src_w = fb-width 16;
+plane_state-base.src_h = fb-height 16;
+
+plane_state-base.crtc_x = plane_state-base.src_y = 0;
+plane_state-base.crtc_w = fb-width;
+plane_state-base.crtc_h = fb-height;
+
+plane_state-visible = true;
obj = intel_fb_obj(fb);
if (obj-tiling_mode != I915_TILING_NONE)
dev_priv-preserve_bios_swizzle = true;
-
-primary-fb = fb;
-primary-crtc = primary-state-crtc = intel_crtc-base;
-update_state_fb(primary);
Do we still have other users of update_state_fb left?
Just the page flip handler.
-intel_crtc-base.state-plane_mask |= (1
