* [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats.
@ 2018-09-20 10:27 Maarten Lankhorst
2018-09-20 10:27 ` [PATCH 1/8] drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail() Maarten Lankhorst
` (15 more replies)
0 siblings, 16 replies; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-20 10:27 UTC (permalink / raw)
To: intel-gfx
Some cleanups to scalers, cleanups to make atomic update look at
crtc_state->active_planes and handle that correctly, and forcing nv12
coordinates to be a multiple of 2.
Maarten Lankhorst (8):
drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()
drm/i915: Handle cursor updating active_planes correctly, v2.
drm/i915: Unconditionally clear plane visibility, v2.
drm/i915: Make intel_crtc_disable_planes() use active planes mask.
drm/i915: Replace call to commit_planes_on_crtc with internal update,
v2.
drm/i915: Clean up scaler setup.
drm/i915: Move programming plane scaler to its own function.
drm/i915: Force planar YUV coordinates to be a multiple of 2, v2.
drivers/gpu/drm/i915/i915_reg.h | 7 +-
drivers/gpu/drm/i915/intel_atomic.c | 108 ++++++++++++----------
drivers/gpu/drm/i915/intel_atomic_plane.c | 49 +++++-----
drivers/gpu/drm/i915/intel_display.c | 99 ++++++++++++--------
drivers/gpu/drm/i915/intel_drv.h | 4 +
drivers/gpu/drm/i915/intel_sprite.c | 99 ++++++++++++--------
6 files changed, 218 insertions(+), 148 deletions(-)
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 1/8] drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
@ 2018-09-20 10:27 ` Maarten Lankhorst
2018-09-20 10:27 ` [PATCH 2/8] drm/i915: Handle cursor updating active_planes correctly, v2 Maarten Lankhorst
` (14 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-20 10:27 UTC (permalink / raw)
To: intel-gfx
Use old/new_intel_crtc_state, and get rid of all the conversion casts
where they don't add anything.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++----------
1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fbcc56caffb6..58c188482c78 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12663,8 +12663,9 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
struct drm_i915_private *dev_priv = to_i915(dev);
struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+ struct intel_crtc_state *new_intel_crtc_state, *old_intel_crtc_state;
struct drm_crtc *crtc;
- struct intel_crtc_state *intel_cstate;
+ struct intel_crtc *intel_crtc;
u64 put_domains[I915_MAX_PIPES] = {};
int i;
@@ -12676,21 +12677,22 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ old_intel_crtc_state = to_intel_crtc_state(old_crtc_state);
+ new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
+ intel_crtc = to_intel_crtc(crtc);
if (needs_modeset(new_crtc_state) ||
to_intel_crtc_state(new_crtc_state)->update_pipe) {
- put_domains[to_intel_crtc(crtc)->pipe] =
+ put_domains[intel_crtc->pipe] =
modeset_get_crtc_power_domains(crtc,
- to_intel_crtc_state(new_crtc_state));
+ new_intel_crtc_state);
}
if (!needs_modeset(new_crtc_state))
continue;
- intel_pre_plane_update(to_intel_crtc_state(old_crtc_state),
- to_intel_crtc_state(new_crtc_state));
+ intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
if (old_crtc_state->active) {
intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
@@ -12701,7 +12703,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
*/
intel_crtc_disable_pipe_crc(intel_crtc);
- dev_priv->display.crtc_disable(to_intel_crtc_state(old_crtc_state), state);
+ dev_priv->display.crtc_disable(old_intel_crtc_state, state);
intel_crtc->active = false;
intel_fbc_disable(intel_crtc);
intel_disable_shared_dpll(intel_crtc);
@@ -12722,7 +12724,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
*/
if (INTEL_GEN(dev_priv) >= 9)
dev_priv->display.initial_watermarks(intel_state,
- to_intel_crtc_state(new_crtc_state));
+ new_intel_crtc_state);
}
}
}
@@ -12782,11 +12784,11 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
* TODO: Move this (and other cleanup) to an async worker eventually.
*/
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
- intel_cstate = to_intel_crtc_state(new_crtc_state);
+ new_intel_crtc_state = to_intel_crtc_state(new_crtc_state);
if (dev_priv->display.optimize_watermarks)
dev_priv->display.optimize_watermarks(intel_state,
- intel_cstate);
+ new_intel_crtc_state);
}
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 2/8] drm/i915: Handle cursor updating active_planes correctly, v2.
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
2018-09-20 10:27 ` [PATCH 1/8] drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail() Maarten Lankhorst
@ 2018-09-20 10:27 ` Maarten Lankhorst
2018-09-20 23:18 ` Matt Roper
2018-09-20 10:27 ` [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2 Maarten Lankhorst
` (13 subsequent siblings)
15 siblings, 1 reply; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-20 10:27 UTC (permalink / raw)
To: intel-gfx
While we may not update new_crtc_state, we may clear active_planes
if the new cursor update state will disable the cursor, but we fail
after. If this is immediately followed by a modeset disable, we may
soon not disable the planes correctly when we start depending on
active_planes.
Changes since v1:
- Clarify why we cannot swap crtc_state. (Matt)
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++-------
1 file changed, 28 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 58c188482c78..078cdcca88e1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13515,14 +13515,16 @@ intel_legacy_cursor_update(struct drm_plane *plane,
struct drm_plane_state *old_plane_state, *new_plane_state;
struct intel_plane *intel_plane = to_intel_plane(plane);
struct drm_framebuffer *old_fb;
- struct drm_crtc_state *crtc_state = crtc->state;
+ struct intel_crtc_state *crtc_state =
+ to_intel_crtc_state(crtc->state);
+ struct intel_crtc_state *new_crtc_state;
/*
* When crtc is inactive or there is a modeset pending,
* wait for it to complete in the slowpath
*/
- if (!crtc_state->active || needs_modeset(crtc_state) ||
- to_intel_crtc_state(crtc_state)->update_pipe)
+ if (!crtc_state->base.active || needs_modeset(&crtc_state->base) ||
+ crtc_state->update_pipe)
goto slow;
old_plane_state = plane->state;
@@ -13552,6 +13554,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
if (!new_plane_state)
return -ENOMEM;
+ new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(crtc));
+ if (!new_crtc_state) {
+ ret = -ENOMEM;
+ goto out_free;
+ }
+
drm_atomic_set_fb_for_plane(new_plane_state, fb);
new_plane_state->src_x = src_x;
@@ -13563,9 +13571,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
new_plane_state->crtc_w = crtc_w;
new_plane_state->crtc_h = crtc_h;
- ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
- to_intel_crtc_state(crtc->state), /* FIXME need a new crtc state? */
- to_intel_plane_state(plane->state),
+ ret = intel_plane_atomic_check_with_state(crtc_state, new_crtc_state,
+ to_intel_plane_state(old_plane_state),
to_intel_plane_state(new_plane_state));
if (ret)
goto out_free;
@@ -13587,10 +13594,21 @@ intel_legacy_cursor_update(struct drm_plane *plane,
/* Swap plane state */
plane->state = new_plane_state;
+ /*
+ * We cannot swap crtc_state as it may be in use by an atomic commit or
+ * page flip that's running simultaneously. If we swap crtc_state and
+ * destroy the old state, we will cause a use-after-free there.
+ *
+ * Only update active_planes, which is needed for our internal
+ * bookkeeping. Either value will do the right thing when updating
+ * planes atomically. If the cursor was part of the atomic update then
+ * we would have taken the slowpath.
+ */
+ crtc_state->active_planes = new_crtc_state->active_planes;
+
if (plane->state->visible) {
trace_intel_update_plane(plane, to_intel_crtc(crtc));
- intel_plane->update_plane(intel_plane,
- to_intel_crtc_state(crtc->state),
+ intel_plane->update_plane(intel_plane, crtc_state,
to_intel_plane_state(plane->state));
} else {
trace_intel_disable_plane(plane, to_intel_crtc(crtc));
@@ -13602,6 +13620,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
out_unlock:
mutex_unlock(&dev_priv->drm.struct_mutex);
out_free:
+ if (new_crtc_state)
+ intel_crtc_destroy_state(crtc, &new_crtc_state->base);
if (ret)
intel_plane_destroy_state(plane, new_plane_state);
else
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2.
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
2018-09-20 10:27 ` [PATCH 1/8] drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail() Maarten Lankhorst
2018-09-20 10:27 ` [PATCH 2/8] drm/i915: Handle cursor updating active_planes correctly, v2 Maarten Lankhorst
@ 2018-09-20 10:27 ` Maarten Lankhorst
2018-09-20 23:18 ` Matt Roper
2018-09-21 15:26 ` Ville Syrjälä
2018-09-20 10:27 ` [PATCH 4/8] drm/i915: Make intel_crtc_disable_planes() use active planes mask Maarten Lankhorst
` (12 subsequent siblings)
15 siblings, 2 replies; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-20 10:27 UTC (permalink / raw)
To: intel-gfx
We need to assume the plane has been visible before, even if no CRTC
is assigned to the plane. This is because nv12 will enable a a extra
plane and make it visible by marking it in crtc_state->active_planes
for intel_update_planes_on_crtc().
Additionally, clear visible flag in intel_plane_atomic_check, in case
we ever hit a bug with visibility. Our code implicitly assumes that
plane_state->visible is only true when crtc and fb are set,
so we will either null deref in intel_fbc_choose_crtc() or
do something bad during the actual commit which cares even more.
Changes since v1:
- Unconditionally clear crtc_state->active_planes as well.
- Reword commit message, since this is now a preparation patch for
NV12 Y / UV plane linking.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_atomic_plane.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index aabebe0d2e9b..f70e9cb9cf02 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -117,10 +117,13 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
struct intel_plane *intel_plane = to_intel_plane(plane);
int ret;
+ crtc_state->active_planes &= ~BIT(intel_plane->id);
+ intel_state->base.visible = false;
+
+ /* If this is a cursor plane, no further checks are needed. */
if (!intel_state->base.crtc && !old_plane_state->base.crtc)
return 0;
- intel_state->base.visible = false;
ret = intel_plane->check_plane(crtc_state, intel_state);
if (ret)
return ret;
@@ -128,8 +131,6 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
/* FIXME pre-g4x don't work like this */
if (state->visible)
crtc_state->active_planes |= BIT(intel_plane->id);
- else
- crtc_state->active_planes &= ~BIT(intel_plane->id);
if (state->visible && state->fb->format->format == DRM_FORMAT_NV12)
crtc_state->nv12_planes |= BIT(intel_plane->id);
@@ -152,6 +153,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
const struct drm_crtc_state *old_crtc_state;
struct drm_crtc_state *new_crtc_state;
+ new_plane_state->visible = false;
if (!crtc)
return 0;
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 4/8] drm/i915: Make intel_crtc_disable_planes() use active planes mask.
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
` (2 preceding siblings ...)
2018-09-20 10:27 ` [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2 Maarten Lankhorst
@ 2018-09-20 10:27 ` Maarten Lankhorst
2018-09-20 10:27 ` [PATCH 5/8] drm/i915: Replace call to commit_planes_on_crtc with internal update, v2 Maarten Lankhorst
` (11 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-20 10:27 UTC (permalink / raw)
To: intel-gfx
This will only disable planes we actually had marked as visible in
crtc_state->visible_planes and cleans up intel_crtc_disable_plane()
slightly.
This is also useful for when we start enabling NV12 support, in which
we will make the separate Y plane visible, but ignore the Y plane's state.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++--------------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 078cdcca88e1..1866362793f7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5402,24 +5402,23 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
intel_update_watermarks(crtc);
}
-static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
+static void intel_crtc_disable_planes(struct intel_crtc *crtc, unsigned plane_mask)
{
- struct drm_device *dev = crtc->dev;
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct drm_plane *p;
- int pipe = intel_crtc->pipe;
+ struct drm_device *dev = crtc->base.dev;
+ struct intel_plane *plane;
+ unsigned fb_bits = 0;
- intel_crtc_dpms_overlay_disable(intel_crtc);
+ intel_crtc_dpms_overlay_disable(crtc);
- drm_for_each_plane_mask(p, dev, plane_mask)
- to_intel_plane(p)->disable_plane(to_intel_plane(p), intel_crtc);
+ for_each_intel_plane_on_crtc(dev, crtc, plane) {
+ if (plane_mask & BIT(plane->id)) {
+ plane->disable_plane(plane, crtc);
- /*
- * FIXME: Once we grow proper nuclear flip support out of this we need
- * to compute the mask of flip planes precisely. For the time being
- * consider this a flip to a NULL plane.
- */
- intel_frontbuffer_flip(to_i915(dev), INTEL_FRONTBUFFER_ALL_MASK(pipe));
+ fb_bits |= plane->frontbuffer_bit;
+ }
+ }
+
+ intel_frontbuffer_flip(to_i915(dev), fb_bits);
}
static void intel_encoders_pre_pll_enable(struct drm_crtc *crtc,
@@ -12695,7 +12694,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
intel_pre_plane_update(old_intel_crtc_state, new_intel_crtc_state);
if (old_crtc_state->active) {
- intel_crtc_disable_planes(crtc, old_crtc_state->plane_mask);
+ intel_crtc_disable_planes(intel_crtc, old_intel_crtc_state->active_planes);
/*
* We need to disable pipe CRC before disabling the pipe,
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 5/8] drm/i915: Replace call to commit_planes_on_crtc with internal update, v2.
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
` (3 preceding siblings ...)
2018-09-20 10:27 ` [PATCH 4/8] drm/i915: Make intel_crtc_disable_planes() use active planes mask Maarten Lankhorst
@ 2018-09-20 10:27 ` Maarten Lankhorst
2018-09-20 10:27 ` [PATCH 6/8] drm/i915: Clean up scaler setup Maarten Lankhorst
` (10 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-20 10:27 UTC (permalink / raw)
To: intel-gfx
drm_atomic_helper_commit_planes_on_crtc calls begin_commit,
then plane_update hooks, then flush_commit. Because we keep our own
visibility tracking through plane_state->visible there's no need to
rely on the atomic hooks for this.
By explicitly writing our own helper, we can update visible planes
as needed, which is useful to make NV12 support work as intended.
Changes since v1:
- Reword commit message. (Matt Roper)
- Rename to intel_update_planes_on_crtc(). (Matt)
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
---
drivers/gpu/drm/i915/intel_atomic_plane.c | 41 ++++++++++++-----------
drivers/gpu/drm/i915/intel_display.c | 10 ++++--
drivers/gpu/drm/i915/intel_drv.h | 4 +++
3 files changed, 33 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
index f70e9cb9cf02..c7e55f1c032f 100644
--- a/drivers/gpu/drm/i915/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
@@ -166,29 +166,33 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
to_intel_plane_state(new_plane_state));
}
-static void intel_plane_atomic_update(struct drm_plane *plane,
- struct drm_plane_state *old_state)
+void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
+ struct intel_crtc *crtc,
+ struct intel_crtc_state *old_crtc_state,
+ struct intel_crtc_state *new_crtc_state)
{
- struct intel_atomic_state *state = to_intel_atomic_state(old_state->state);
- struct intel_plane *intel_plane = to_intel_plane(plane);
- const struct intel_plane_state *new_plane_state =
- intel_atomic_get_new_plane_state(state, intel_plane);
- struct drm_crtc *crtc = new_plane_state->base.crtc ?: old_state->crtc;
+ struct intel_plane_state *new_plane_state;
+ struct intel_plane *plane;
+ u32 update_mask;
+ int i;
+
+ update_mask = old_crtc_state->active_planes;
+ update_mask |= new_crtc_state->active_planes;
- if (new_plane_state->base.visible) {
- const struct intel_crtc_state *new_crtc_state =
- intel_atomic_get_new_crtc_state(state, to_intel_crtc(crtc));
+ for_each_new_intel_plane_in_state(old_state, plane, new_plane_state, i) {
+ if (crtc->pipe != plane->pipe ||
+ !(update_mask & BIT(plane->id)))
+ continue;
- trace_intel_update_plane(plane,
- to_intel_crtc(crtc));
+ if (new_plane_state->base.visible) {
+ trace_intel_update_plane(&plane->base, crtc);
- intel_plane->update_plane(intel_plane,
- new_crtc_state, new_plane_state);
- } else {
- trace_intel_disable_plane(plane,
- to_intel_crtc(crtc));
+ plane->update_plane(plane, new_crtc_state, new_plane_state);
+ } else {
+ trace_intel_disable_plane(&plane->base, crtc);
- intel_plane->disable_plane(intel_plane, to_intel_crtc(crtc));
+ plane->disable_plane(plane, crtc);
+ }
}
}
@@ -196,7 +200,6 @@ const struct drm_plane_helper_funcs intel_plane_helper_funcs = {
.prepare_fb = intel_prepare_plane_fb,
.cleanup_fb = intel_cleanup_plane_fb,
.atomic_check = intel_plane_atomic_check,
- .atomic_update = intel_plane_atomic_update,
};
/**
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1866362793f7..02374185b993 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10811,8 +10811,6 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
}
static const struct drm_crtc_helper_funcs intel_helper_funcs = {
- .atomic_begin = intel_begin_crtc_commit,
- .atomic_flush = intel_finish_crtc_commit,
.atomic_check = intel_crtc_atomic_check,
};
@@ -12481,6 +12479,7 @@ static void intel_update_crtc(struct drm_crtc *crtc,
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_crtc_state *old_intel_cstate = to_intel_crtc_state(old_crtc_state);
struct intel_crtc_state *pipe_config = to_intel_crtc_state(new_crtc_state);
bool modeset = needs_modeset(new_crtc_state);
struct intel_plane_state *new_plane_state =
@@ -12501,7 +12500,12 @@ static void intel_update_crtc(struct drm_crtc *crtc,
if (new_plane_state)
intel_fbc_enable(intel_crtc, pipe_config, new_plane_state);
- drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
+ intel_begin_crtc_commit(crtc, old_crtc_state);
+
+ intel_update_planes_on_crtc(to_intel_atomic_state(state), intel_crtc,
+ old_intel_cstate, pipe_config);
+
+ intel_finish_crtc_commit(crtc, old_crtc_state);
}
static void intel_update_crtcs(struct drm_atomic_state *state)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bf1c38728a59..8073a85d7178 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -2188,6 +2188,10 @@ struct drm_plane_state *intel_plane_duplicate_state(struct drm_plane *plane);
void intel_plane_destroy_state(struct drm_plane *plane,
struct drm_plane_state *state);
extern const struct drm_plane_helper_funcs intel_plane_helper_funcs;
+void intel_update_planes_on_crtc(struct intel_atomic_state *old_state,
+ struct intel_crtc *crtc,
+ struct intel_crtc_state *old_crtc_state,
+ struct intel_crtc_state *new_crtc_state);
int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_state,
struct intel_crtc_state *crtc_state,
const struct intel_plane_state *old_plane_state,
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 6/8] drm/i915: Clean up scaler setup.
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
` (4 preceding siblings ...)
2018-09-20 10:27 ` [PATCH 5/8] drm/i915: Replace call to commit_planes_on_crtc with internal update, v2 Maarten Lankhorst
@ 2018-09-20 10:27 ` Maarten Lankhorst
2018-09-20 23:19 ` Matt Roper
2018-09-20 10:27 ` [PATCH 7/8] drm/i915: Move programming plane scaler to its own function Maarten Lankhorst
` (9 subsequent siblings)
15 siblings, 1 reply; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-20 10:27 UTC (permalink / raw)
To: intel-gfx
On skylake we can switch to a high quality scaler mode when only 1 out
of 2 scalers are used, but on GLK and later bit 28 has a different
meaning. Don't set it, and make clear the distinction between
SKL and later PS values.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/i915_reg.h | 7 +-
drivers/gpu/drm/i915/intel_atomic.c | 108 +++++++++++++++------------
drivers/gpu/drm/i915/intel_display.c | 2 +-
3 files changed, 66 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4948b352bf4c..e7e6ca7f9665 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6850,11 +6850,12 @@ enum {
#define _PS_2B_CTRL 0x68A80
#define _PS_1C_CTRL 0x69180
#define PS_SCALER_EN (1 << 31)
-#define PS_SCALER_MODE_MASK (3 << 28)
-#define PS_SCALER_MODE_DYN (0 << 28)
-#define PS_SCALER_MODE_HQ (1 << 28)
+#define SKL_PS_SCALER_MODE_MASK (3 << 28)
+#define SKL_PS_SCALER_MODE_DYN (0 << 28)
+#define SKL_PS_SCALER_MODE_HQ (1 << 28)
#define SKL_PS_SCALER_MODE_NV12 (2 << 28)
#define PS_SCALER_MODE_PLANAR (1 << 29)
+#define PS_SCALER_MODE_PACKED (0 << 29)
#define PS_PLANE_SEL_MASK (7 << 25)
#define PS_PLANE_SEL(plane) (((plane) + 1) << 25)
#define PS_FILTER_MASK (3 << 23)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index b04952bacf77..40fba3f22f87 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -203,6 +203,62 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
drm_atomic_helper_crtc_destroy_state(crtc, state);
}
+static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
+ int num_scalers_need, struct intel_crtc *intel_crtc,
+ const char *name, int idx,
+ struct intel_plane_state *plane_state,
+ int *scaler_id)
+{
+ struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
+ int j;
+ u32 mode;
+
+ if (*scaler_id < 0) {
+ /* find a free scaler */
+ for (j = 0; j < intel_crtc->num_scalers; j++) {
+ if (scaler_state->scalers[j].in_use)
+ continue;
+
+ *scaler_id = j;
+ scaler_state->scalers[*scaler_id].in_use = 1;
+ }
+ }
+
+ if (WARN(*scaler_id < 0, "Cannot find scaler for %s:%d\n", name, idx))
+ return;
+
+ /* set scaler mode */
+ if (plane_state && plane_state->base.fb &&
+ plane_state->base.fb->format->is_yuv &&
+ plane_state->base.fb->format->num_planes > 1) {
+ if (INTEL_GEN(dev_priv) == 9 &&
+ !IS_GEMINILAKE(dev_priv))
+ mode = SKL_PS_SCALER_MODE_NV12;
+ else
+ mode = PS_SCALER_MODE_PLANAR;
+
+ } else if (INTEL_GEN(dev_priv) > 9 || IS_GEMINILAKE(dev_priv)) {
+ mode = PS_SCALER_MODE_PACKED;
+ } else if (num_scalers_need == 1 && intel_crtc->num_scalers > 1) {
+ /*
+ * when only 1 scaler is in use on a pipe with 2 scalers
+ * scaler 0 operates in high quality (HQ) mode.
+ * In this case use scaler 0 to take advantage of HQ mode
+ */
+ scaler_state->scalers[*scaler_id].in_use = 0;
+ *scaler_id = 0;
+ scaler_state->scalers[0].in_use = 1;
+ mode = SKL_PS_SCALER_MODE_HQ;
+ } else {
+ mode = SKL_PS_SCALER_MODE_DYN;
+ }
+
+ DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
+ intel_crtc->pipe, *scaler_id, name, idx);
+ scaler_state->scalers[*scaler_id].mode = mode;
+}
+
+
/**
* intel_atomic_setup_scalers() - setup scalers for crtc per staged requests
* @dev_priv: i915 device
@@ -232,7 +288,7 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
struct drm_atomic_state *drm_state = crtc_state->base.state;
struct intel_atomic_state *intel_state = to_intel_atomic_state(drm_state);
int num_scalers_need;
- int i, j;
+ int i;
num_scalers_need = hweight32(scaler_state->scaler_users);
@@ -304,59 +360,17 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
idx = plane->base.id;
/* plane on different crtc cannot be a scaler user of this crtc */
- if (WARN_ON(intel_plane->pipe != intel_crtc->pipe)) {
+ if (WARN_ON(intel_plane->pipe != intel_crtc->pipe))
continue;
- }
plane_state = intel_atomic_get_new_plane_state(intel_state,
intel_plane);
scaler_id = &plane_state->scaler_id;
}
- if (*scaler_id < 0) {
- /* find a free scaler */
- for (j = 0; j < intel_crtc->num_scalers; j++) {
- if (!scaler_state->scalers[j].in_use) {
- scaler_state->scalers[j].in_use = 1;
- *scaler_id = j;
- DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
- intel_crtc->pipe, *scaler_id, name, idx);
- break;
- }
- }
- }
-
- if (WARN_ON(*scaler_id < 0)) {
- DRM_DEBUG_KMS("Cannot find scaler for %s:%d\n", name, idx);
- continue;
- }
-
- /* set scaler mode */
- if ((INTEL_GEN(dev_priv) >= 9) &&
- plane_state && plane_state->base.fb &&
- plane_state->base.fb->format->format ==
- DRM_FORMAT_NV12) {
- if (INTEL_GEN(dev_priv) == 9 &&
- !IS_GEMINILAKE(dev_priv) &&
- !IS_SKYLAKE(dev_priv))
- scaler_state->scalers[*scaler_id].mode =
- SKL_PS_SCALER_MODE_NV12;
- else
- scaler_state->scalers[*scaler_id].mode =
- PS_SCALER_MODE_PLANAR;
- } else if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) {
- /*
- * when only 1 scaler is in use on either pipe A or B,
- * scaler 0 operates in high quality (HQ) mode.
- * In this case use scaler 0 to take advantage of HQ mode
- */
- *scaler_id = 0;
- scaler_state->scalers[0].in_use = 1;
- scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
- scaler_state->scalers[1].in_use = 0;
- } else {
- scaler_state->scalers[*scaler_id].mode = PS_SCALER_MODE_DYN;
- }
+ intel_atomic_setup_scaler(scaler_state, num_scalers_need,
+ intel_crtc, name, idx,
+ plane_state, scaler_id);
}
return 0;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 02374185b993..931898013506 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13963,7 +13963,7 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
struct intel_scaler *scaler = &scaler_state->scalers[i];
scaler->in_use = 0;
- scaler->mode = PS_SCALER_MODE_DYN;
+ scaler->mode = 0;
}
scaler_state->scaler_id = -1;
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 7/8] drm/i915: Move programming plane scaler to its own function.
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
` (5 preceding siblings ...)
2018-09-20 10:27 ` [PATCH 6/8] drm/i915: Clean up scaler setup Maarten Lankhorst
@ 2018-09-20 10:27 ` Maarten Lankhorst
2018-09-20 23:19 ` Matt Roper
2018-09-21 15:29 ` Ville Syrjälä
2018-09-20 10:27 ` [PATCH 8/8] drm/i915: Force planar YUV coordinates to be a multiple of 2, v2 Maarten Lankhorst
` (8 subsequent siblings)
15 siblings, 2 replies; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-20 10:27 UTC (permalink / raw)
To: intel-gfx
This cleans the code up slightly, and will make other changes easier.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_sprite.c | 90 +++++++++++++++++------------
1 file changed, 52 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index d4c8e10fc90b..7d3c7469d271 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -280,13 +280,63 @@ skl_plane_max_stride(struct intel_plane *plane,
return min(8192 * cpp, 32768);
}
+static void
+skl_program_scaler(struct drm_i915_private *dev_priv,
+ struct intel_plane *plane,
+ const struct intel_crtc_state *crtc_state,
+ const struct intel_plane_state *plane_state)
+{
+ enum plane_id plane_id = plane->id;
+ enum pipe pipe = plane->pipe;
+ int scaler_id = plane_state->scaler_id;
+ const struct intel_scaler *scaler =
+ &crtc_state->scaler_state.scalers[scaler_id];
+ int crtc_x = plane_state->base.dst.x1;
+ int crtc_y = plane_state->base.dst.y1;
+ uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
+ uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
+ u16 y_hphase, uv_rgb_hphase;
+ u16 y_vphase, uv_rgb_vphase;
+
+ /* Sizes are 0 based */
+ crtc_w--;
+ crtc_h--;
+
+ /* TODO: handle sub-pixel coordinates */
+ if (plane_state->base.fb->format->format == DRM_FORMAT_NV12) {
+ y_hphase = skl_scaler_calc_phase(1, false);
+ y_vphase = skl_scaler_calc_phase(1, false);
+
+ /* MPEG2 chroma siting convention */
+ uv_rgb_hphase = skl_scaler_calc_phase(2, true);
+ uv_rgb_vphase = skl_scaler_calc_phase(2, false);
+ } else {
+ /* not used */
+ y_hphase = 0;
+ y_vphase = 0;
+
+ uv_rgb_hphase = skl_scaler_calc_phase(1, false);
+ uv_rgb_vphase = skl_scaler_calc_phase(1, false);
+ }
+
+ I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),
+ PS_SCALER_EN | PS_PLANE_SEL(plane_id) | scaler->mode);
+ I915_WRITE_FW(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
+ I915_WRITE_FW(SKL_PS_VPHASE(pipe, scaler_id),
+ PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase));
+ I915_WRITE_FW(SKL_PS_HPHASE(pipe, scaler_id),
+ PS_Y_PHASE(y_hphase) | PS_UV_RGB_PHASE(uv_rgb_hphase));
+ I915_WRITE_FW(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
+ I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
+ ((crtc_w + 1) << 16)|(crtc_h + 1));
+}
+
void
skl_update_plane(struct intel_plane *plane,
const struct intel_crtc_state *crtc_state,
const struct intel_plane_state *plane_state)
{
struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
- const struct drm_framebuffer *fb = plane_state->base.fb;
enum plane_id plane_id = plane->id;
enum pipe pipe = plane->pipe;
u32 plane_ctl = plane_state->ctl;
@@ -296,8 +346,6 @@ skl_update_plane(struct intel_plane *plane,
u32 aux_stride = skl_plane_stride(plane_state, 1);
int crtc_x = plane_state->base.dst.x1;
int crtc_y = plane_state->base.dst.y1;
- uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
- uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
uint32_t x = plane_state->color_plane[0].x;
uint32_t y = plane_state->color_plane[0].y;
uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
@@ -307,8 +355,6 @@ skl_update_plane(struct intel_plane *plane,
/* Sizes are 0 based */
src_w--;
src_h--;
- crtc_w--;
- crtc_h--;
spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
@@ -333,39 +379,7 @@ skl_update_plane(struct intel_plane *plane,
/* program plane scaler */
if (plane_state->scaler_id >= 0) {
- int scaler_id = plane_state->scaler_id;
- const struct intel_scaler *scaler =
- &crtc_state->scaler_state.scalers[scaler_id];
- u16 y_hphase, uv_rgb_hphase;
- u16 y_vphase, uv_rgb_vphase;
-
- /* TODO: handle sub-pixel coordinates */
- if (fb->format->format == DRM_FORMAT_NV12) {
- y_hphase = skl_scaler_calc_phase(1, false);
- y_vphase = skl_scaler_calc_phase(1, false);
-
- /* MPEG2 chroma siting convention */
- uv_rgb_hphase = skl_scaler_calc_phase(2, true);
- uv_rgb_vphase = skl_scaler_calc_phase(2, false);
- } else {
- /* not used */
- y_hphase = 0;
- y_vphase = 0;
-
- uv_rgb_hphase = skl_scaler_calc_phase(1, false);
- uv_rgb_vphase = skl_scaler_calc_phase(1, false);
- }
-
- I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),
- PS_SCALER_EN | PS_PLANE_SEL(plane_id) | scaler->mode);
- I915_WRITE_FW(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
- I915_WRITE_FW(SKL_PS_VPHASE(pipe, scaler_id),
- PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase));
- I915_WRITE_FW(SKL_PS_HPHASE(pipe, scaler_id),
- PS_Y_PHASE(y_hphase) | PS_UV_RGB_PHASE(uv_rgb_hphase));
- I915_WRITE_FW(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
- I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
- ((crtc_w + 1) << 16)|(crtc_h + 1));
+ skl_program_scaler(dev_priv, plane, crtc_state, plane_state);
I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0);
} else {
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH 8/8] drm/i915: Force planar YUV coordinates to be a multiple of 2, v2.
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
` (6 preceding siblings ...)
2018-09-20 10:27 ` [PATCH 7/8] drm/i915: Move programming plane scaler to its own function Maarten Lankhorst
@ 2018-09-20 10:27 ` Maarten Lankhorst
2018-09-20 23:19 ` Matt Roper
2018-09-20 10:49 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Preparations for adding gen11 planar formats Patchwork
` (7 subsequent siblings)
15 siblings, 1 reply; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-20 10:27 UTC (permalink / raw)
To: intel-gfx
We can't make NV12 work any other way. The scaler doesn't handle odd
coordinates well, and we will get visual corruption on the screen.
Changes since v1:
- Put the check in intel_plane_check_src_coordinates. (Ville)
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
drivers/gpu/drm/i915/intel_sprite.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 7d3c7469d271..46c6336cb858 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -253,13 +253,20 @@ int intel_plane_check_src_coordinates(struct intel_plane_state *plane_state)
src->y2 = (src_y + src_h) << 16;
if (fb->format->is_yuv &&
- fb->format->format != DRM_FORMAT_NV12 &&
(src_x & 1 || src_w & 1)) {
DRM_DEBUG_KMS("src x/w (%u, %u) must be a multiple of 2 for YUV planes\n",
src_x, src_w);
return -EINVAL;
}
+ if (fb->format->is_yuv &&
+ fb->format->num_planes > 1 &&
+ (src_y & 1 || src_h & 1)) {
+ DRM_DEBUG_KMS("src y/h (%u, %u) must be a multiple of 2 for planar YUV planes\n",
+ src_y, src_h);
+ return -EINVAL;
+ }
+
return 0;
}
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 32+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Preparations for adding gen11 planar formats.
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
` (7 preceding siblings ...)
2018-09-20 10:27 ` [PATCH 8/8] drm/i915: Force planar YUV coordinates to be a multiple of 2, v2 Maarten Lankhorst
@ 2018-09-20 10:49 ` Patchwork
2018-09-20 10:52 ` ✗ Fi.CI.SPARSE: " Patchwork
` (6 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Patchwork @ 2018-09-20 10:49 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Preparations for adding gen11 planar formats.
URL : https://patchwork.freedesktop.org/series/49956/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
a9c37aaf2d75 drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()
6400b208e3c8 drm/i915: Handle cursor updating active_planes correctly, v2.
7fe53ceac6ce drm/i915: Unconditionally clear plane visibility, v2.
820dbdc8073e drm/i915: Make intel_crtc_disable_planes() use active planes mask.
-:26: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#26: FILE: drivers/gpu/drm/i915/intel_display.c:5405:
+static void intel_crtc_disable_planes(struct intel_crtc *crtc, unsigned plane_mask)
-:34: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#34: FILE: drivers/gpu/drm/i915/intel_display.c:5409:
+ unsigned fb_bits = 0;
total: 0 errors, 2 warnings, 0 checks, 45 lines checked
fcce59f9ee3a drm/i915: Replace call to commit_planes_on_crtc with internal update, v2.
9b4060cded61 drm/i915: Clean up scaler setup.
-:96: CHECK:LINE_SPACING: Please don't use multiple blank lines
#96: FILE: drivers/gpu/drm/i915/intel_atomic.c:261:
+
+
total: 0 errors, 0 warnings, 1 checks, 156 lines checked
013eb38b053f drm/i915: Move programming plane scaler to its own function.
-:66: CHECK:SPACING: spaces preferred around that '|' (ctx:VxV)
#66: FILE: drivers/gpu/drm/i915/intel_sprite.c:331:
+ ((crtc_w + 1) << 16)|(crtc_h + 1));
^
total: 0 errors, 0 warnings, 1 checks, 120 lines checked
fc8489af0af6 drm/i915: Force planar YUV coordinates to be a multiple of 2, v2.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* ✗ Fi.CI.SPARSE: warning for drm/i915: Preparations for adding gen11 planar formats.
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
` (8 preceding siblings ...)
2018-09-20 10:49 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Preparations for adding gen11 planar formats Patchwork
@ 2018-09-20 10:52 ` Patchwork
2018-09-20 11:14 ` ✓ Fi.CI.BAT: success " Patchwork
` (5 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Patchwork @ 2018-09-20 10:52 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Preparations for adding gen11 planar formats.
URL : https://patchwork.freedesktop.org/series/49956/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Commit: drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()
Okay!
Commit: drm/i915: Handle cursor updating active_planes correctly, v2.
Okay!
Commit: drm/i915: Unconditionally clear plane visibility, v2.
Okay!
Commit: drm/i915: Make intel_crtc_disable_planes() use active planes mask.
Okay!
Commit: drm/i915: Replace call to commit_planes_on_crtc with internal update, v2.
Okay!
Commit: drm/i915: Clean up scaler setup.
Okay!
Commit: drm/i915: Move programming plane scaler to its own function.
-O:drivers/gpu/drm/i915/intel_sprite.c:280:24: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_sprite.c:280:24: warning: expression using sizeof(void)
Commit: drm/i915: Force planar YUV coordinates to be a multiple of 2, v2.
Okay!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Preparations for adding gen11 planar formats.
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
` (9 preceding siblings ...)
2018-09-20 10:52 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2018-09-20 11:14 ` Patchwork
2018-09-20 12:10 ` ✗ Fi.CI.IGT: failure " Patchwork
` (4 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Patchwork @ 2018-09-20 11:14 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Preparations for adding gen11 planar formats.
URL : https://patchwork.freedesktop.org/series/49956/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4849 -> Patchwork_10236 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/49956/revisions/1/mbox/
== Known issues ==
Here are the changes found in Patchwork_10236 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@amdgpu/amd_prime@amd-to-i915:
fi-kbl-8809g: NOTRUN -> FAIL (fdo#107341)
igt@drv_selftest@live_coherency:
fi-gdg-551: PASS -> DMESG-FAIL (fdo#107164)
igt@gem_exec_suspend@basic-s3:
fi-bdw-samus: PASS -> INCOMPLETE (fdo#107773)
igt@kms_frontbuffer_tracking@basic:
fi-byt-clapper: PASS -> FAIL (fdo#103167)
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
fi-byt-clapper: PASS -> INCOMPLETE (fdo#102657)
==== Possible fixes ====
igt@drv_module_reload@basic-reload-inject:
fi-cfl-8700k: DMESG-WARN (fdo#106107) -> PASS
igt@kms_flip@basic-flip-vs-modeset:
fi-skl-6700hq: DMESG-WARN (fdo#105998) -> PASS +1
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
fi-cfl-8109u: INCOMPLETE (fdo#106070) -> PASS
igt@kms_psr@primary_page_flip:
fi-kbl-r: FAIL (fdo#107336) -> PASS
fdo#102657 https://bugs.freedesktop.org/show_bug.cgi?id=102657
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
fdo#106070 https://bugs.freedesktop.org/show_bug.cgi?id=106070
fdo#106107 https://bugs.freedesktop.org/show_bug.cgi?id=106107
fdo#107164 https://bugs.freedesktop.org/show_bug.cgi?id=107164
fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
fdo#107341 https://bugs.freedesktop.org/show_bug.cgi?id=107341
fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
== Participating hosts (51 -> 47) ==
Additional (1): fi-snb-2520m
Missing (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u
== Build changes ==
* Linux: CI_DRM_4849 -> Patchwork_10236
CI_DRM_4849: 30d45f32f6ac70d9e995e6e92286edaaecb26378 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4647: ae8187922d8de2bc739519da3bd40cf5f03f5e4f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10236: fc8489af0af63c70355ccb52b771ea072cc4df4d @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
fc8489af0af6 drm/i915: Force planar YUV coordinates to be a multiple of 2, v2.
013eb38b053f drm/i915: Move programming plane scaler to its own function.
9b4060cded61 drm/i915: Clean up scaler setup.
fcce59f9ee3a drm/i915: Replace call to commit_planes_on_crtc with internal update, v2.
820dbdc8073e drm/i915: Make intel_crtc_disable_planes() use active planes mask.
7fe53ceac6ce drm/i915: Unconditionally clear plane visibility, v2.
6400b208e3c8 drm/i915: Handle cursor updating active_planes correctly, v2.
a9c37aaf2d75 drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10236/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* ✗ Fi.CI.IGT: failure for drm/i915: Preparations for adding gen11 planar formats.
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
` (10 preceding siblings ...)
2018-09-20 11:14 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-09-20 12:10 ` Patchwork
2018-09-21 15:00 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Preparations for adding gen11 planar formats. (rev2) Patchwork
` (3 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Patchwork @ 2018-09-20 12:10 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Preparations for adding gen11 planar formats.
URL : https://patchwork.freedesktop.org/series/49956/
State : failure
== Summary ==
= CI Bug Log - changes from CI_DRM_4849_full -> Patchwork_10236_full =
== Summary - FAILURE ==
Serious unknown changes coming with Patchwork_10236_full absolutely need to be
verified manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_10236_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_10236_full:
=== IGT changes ===
==== Possible regressions ====
igt@kms_cursor_legacy@2x-cursor-vs-flip-atomic:
shard-hsw: PASS -> FAIL
igt@kms_plane_scaling@pipe-b-scaler-with-clipping-clamping:
shard-apl: PASS -> DMESG-WARN +1
igt@kms_universal_plane@universal-plane-pipe-b-sanity:
shard-glk: PASS -> DMESG-WARN +51
== Known issues ==
Here are the changes found in Patchwork_10236_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@kms_busy@extended-modeset-hang-newfb-render-c:
shard-kbl: PASS -> DMESG-WARN (fdo#107956) +1
igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-pwrite:
shard-glk: PASS -> FAIL (fdo#103167)
igt@kms_plane_scaling@pipe-a-scaler-with-clipping-clamping:
shard-glk: PASS -> DMESG-WARN (fdo#105763, fdo#106538) +1
shard-kbl: PASS -> DMESG-WARN (fdo#105604) +1
==== Possible fixes ====
igt@kms_cursor_crc@cursor-256x256-suspend:
shard-apl: FAIL (fdo#103232, fdo#103191) -> PASS
igt@kms_cursor_legacy@cursor-vs-flip-atomic:
shard-hsw: FAIL (fdo#103355) -> PASS
igt@kms_flip_tiling@flip-to-yf-tiled:
shard-apl: DMESG-WARN (fdo#103558, fdo#105602) -> PASS +3
igt@kms_setmode@basic:
shard-apl: FAIL (fdo#99912) -> PASS
igt@kms_vblank@pipe-c-ts-continuation-suspend:
shard-hsw: FAIL (fdo#104894) -> PASS
==== Warnings ====
igt@kms_cursor_legacy@basic-flip-before-cursor-atomic:
shard-snb: DMESG-WARN (fdo#107469) -> INCOMPLETE (fdo#105411)
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355
fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
fdo#104894 https://bugs.freedesktop.org/show_bug.cgi?id=104894
fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
fdo#105604 https://bugs.freedesktop.org/show_bug.cgi?id=105604
fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
fdo#107469 https://bugs.freedesktop.org/show_bug.cgi?id=107469
fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
== Participating hosts (5 -> 5) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_4849 -> Patchwork_10236
CI_DRM_4849: 30d45f32f6ac70d9e995e6e92286edaaecb26378 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4647: ae8187922d8de2bc739519da3bd40cf5f03f5e4f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10236: fc8489af0af63c70355ccb52b771ea072cc4df4d @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10236/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/8] drm/i915: Handle cursor updating active_planes correctly, v2.
2018-09-20 10:27 ` [PATCH 2/8] drm/i915: Handle cursor updating active_planes correctly, v2 Maarten Lankhorst
@ 2018-09-20 23:18 ` Matt Roper
2018-09-21 9:41 ` Maarten Lankhorst
0 siblings, 1 reply; 32+ messages in thread
From: Matt Roper @ 2018-09-20 23:18 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 20, 2018 at 12:27:05PM +0200, Maarten Lankhorst wrote:
> While we may not update new_crtc_state, we may clear active_planes
> if the new cursor update state will disable the cursor, but we fail
> after. If this is immediately followed by a modeset disable, we may
> soon not disable the planes correctly when we start depending on
> active_planes.
>
> Changes since v1:
> - Clarify why we cannot swap crtc_state. (Matt)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++-------
> 1 file changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 58c188482c78..078cdcca88e1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13515,14 +13515,16 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> struct drm_plane_state *old_plane_state, *new_plane_state;
> struct intel_plane *intel_plane = to_intel_plane(plane);
> struct drm_framebuffer *old_fb;
> - struct drm_crtc_state *crtc_state = crtc->state;
> + struct intel_crtc_state *crtc_state =
> + to_intel_crtc_state(crtc->state);
> + struct intel_crtc_state *new_crtc_state;
>
> /*
> * When crtc is inactive or there is a modeset pending,
> * wait for it to complete in the slowpath
> */
> - if (!crtc_state->active || needs_modeset(crtc_state) ||
> - to_intel_crtc_state(crtc_state)->update_pipe)
> + if (!crtc_state->base.active || needs_modeset(&crtc_state->base) ||
> + crtc_state->update_pipe)
> goto slow;
>
> old_plane_state = plane->state;
> @@ -13552,6 +13554,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> if (!new_plane_state)
> return -ENOMEM;
>
> + new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(crtc));
> + if (!new_crtc_state) {
> + ret = -ENOMEM;
> + goto out_free;
> + }
> +
> drm_atomic_set_fb_for_plane(new_plane_state, fb);
>
> new_plane_state->src_x = src_x;
> @@ -13563,9 +13571,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> new_plane_state->crtc_w = crtc_w;
> new_plane_state->crtc_h = crtc_h;
>
> - ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
> - to_intel_crtc_state(crtc->state), /* FIXME need a new crtc state? */
> - to_intel_plane_state(plane->state),
> + ret = intel_plane_atomic_check_with_state(crtc_state, new_crtc_state,
> + to_intel_plane_state(old_plane_state),
> to_intel_plane_state(new_plane_state));
> if (ret)
> goto out_free;
> @@ -13587,10 +13594,21 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> /* Swap plane state */
> plane->state = new_plane_state;
>
> + /*
> + * We cannot swap crtc_state as it may be in use by an atomic commit or
> + * page flip that's running simultaneously. If we swap crtc_state and
> + * destroy the old state, we will cause a use-after-free there.
Just to confirm, the commit running simultaneously would have to have
already dropped locks (specifically the crtc lock) and returned to
userspace for us to have this problem, right? So it's either a
non-blocking commit in the process of doing its programming via
workqueue, or a blocking commit that's done everything except
intel_atomic_cleanup_work?
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> + *
> + * Only update active_planes, which is needed for our internal
> + * bookkeeping. Either value will do the right thing when updating
> + * planes atomically. If the cursor was part of the atomic update then
> + * we would have taken the slowpath.
> + */
> + crtc_state->active_planes = new_crtc_state->active_planes;
> +
> if (plane->state->visible) {
> trace_intel_update_plane(plane, to_intel_crtc(crtc));
> - intel_plane->update_plane(intel_plane,
> - to_intel_crtc_state(crtc->state),
> + intel_plane->update_plane(intel_plane, crtc_state,
> to_intel_plane_state(plane->state));
> } else {
> trace_intel_disable_plane(plane, to_intel_crtc(crtc));
> @@ -13602,6 +13620,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> out_unlock:
> mutex_unlock(&dev_priv->drm.struct_mutex);
> out_free:
> + if (new_crtc_state)
> + intel_crtc_destroy_state(crtc, &new_crtc_state->base);
> if (ret)
> intel_plane_destroy_state(plane, new_plane_state);
> else
> --
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2.
2018-09-20 10:27 ` [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2 Maarten Lankhorst
@ 2018-09-20 23:18 ` Matt Roper
2018-09-21 15:26 ` Ville Syrjälä
1 sibling, 0 replies; 32+ messages in thread
From: Matt Roper @ 2018-09-20 23:18 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 20, 2018 at 12:27:06PM +0200, Maarten Lankhorst wrote:
> We need to assume the plane has been visible before, even if no CRTC
> is assigned to the plane. This is because nv12 will enable a a extra
You may want to clarify that this is future, gen11-style NV12, not
current, gen9-style NV12 since the current code obviously doesn't do
anything like this.
Otherwise,
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> plane and make it visible by marking it in crtc_state->active_planes
> for intel_update_planes_on_crtc().
>
> Additionally, clear visible flag in intel_plane_atomic_check, in case
> we ever hit a bug with visibility. Our code implicitly assumes that
> plane_state->visible is only true when crtc and fb are set,
> so we will either null deref in intel_fbc_choose_crtc() or
> do something bad during the actual commit which cares even more.
>
> Changes since v1:
> - Unconditionally clear crtc_state->active_planes as well.
> - Reword commit message, since this is now a preparation patch for
> NV12 Y / UV plane linking.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_atomic_plane.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index aabebe0d2e9b..f70e9cb9cf02 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -117,10 +117,13 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> struct intel_plane *intel_plane = to_intel_plane(plane);
> int ret;
>
> + crtc_state->active_planes &= ~BIT(intel_plane->id);
> + intel_state->base.visible = false;
> +
> + /* If this is a cursor plane, no further checks are needed. */
> if (!intel_state->base.crtc && !old_plane_state->base.crtc)
> return 0;
>
> - intel_state->base.visible = false;
> ret = intel_plane->check_plane(crtc_state, intel_state);
> if (ret)
> return ret;
> @@ -128,8 +131,6 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> /* FIXME pre-g4x don't work like this */
> if (state->visible)
> crtc_state->active_planes |= BIT(intel_plane->id);
> - else
> - crtc_state->active_planes &= ~BIT(intel_plane->id);
>
> if (state->visible && state->fb->format->format == DRM_FORMAT_NV12)
> crtc_state->nv12_planes |= BIT(intel_plane->id);
> @@ -152,6 +153,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> const struct drm_crtc_state *old_crtc_state;
> struct drm_crtc_state *new_crtc_state;
>
> + new_plane_state->visible = false;
> if (!crtc)
> return 0;
>
> --
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 6/8] drm/i915: Clean up scaler setup.
2018-09-20 10:27 ` [PATCH 6/8] drm/i915: Clean up scaler setup Maarten Lankhorst
@ 2018-09-20 23:19 ` Matt Roper
2018-09-21 14:44 ` [PATCH] drm/i915: Clean up scaler setup, v2 Maarten Lankhorst
0 siblings, 1 reply; 32+ messages in thread
From: Matt Roper @ 2018-09-20 23:19 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 20, 2018 at 12:27:09PM +0200, Maarten Lankhorst wrote:
> On skylake we can switch to a high quality scaler mode when only 1 out
> of 2 scalers are used, but on GLK and later bit 28 has a different
> meaning. Don't set it, and make clear the distinction between
> SKL and later PS values.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> drivers/gpu/drm/i915/i915_reg.h | 7 +-
> drivers/gpu/drm/i915/intel_atomic.c | 108 +++++++++++++++------------
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> 3 files changed, 66 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 4948b352bf4c..e7e6ca7f9665 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6850,11 +6850,12 @@ enum {
> #define _PS_2B_CTRL 0x68A80
> #define _PS_1C_CTRL 0x69180
> #define PS_SCALER_EN (1 << 31)
> -#define PS_SCALER_MODE_MASK (3 << 28)
> -#define PS_SCALER_MODE_DYN (0 << 28)
> -#define PS_SCALER_MODE_HQ (1 << 28)
> +#define SKL_PS_SCALER_MODE_MASK (3 << 28)
> +#define SKL_PS_SCALER_MODE_DYN (0 << 28)
> +#define SKL_PS_SCALER_MODE_HQ (1 << 28)
> #define SKL_PS_SCALER_MODE_NV12 (2 << 28)
> #define PS_SCALER_MODE_PLANAR (1 << 29)
> +#define PS_SCALER_MODE_PACKED (0 << 29)
> #define PS_PLANE_SEL_MASK (7 << 25)
> #define PS_PLANE_SEL(plane) (((plane) + 1) << 25)
> #define PS_FILTER_MASK (3 << 23)
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index b04952bacf77..40fba3f22f87 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -203,6 +203,62 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
> drm_atomic_helper_crtc_destroy_state(crtc, state);
> }
>
> +static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
> + int num_scalers_need, struct intel_crtc *intel_crtc,
> + const char *name, int idx,
> + struct intel_plane_state *plane_state,
> + int *scaler_id)
> +{
> + struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> + int j;
> + u32 mode;
> +
> + if (*scaler_id < 0) {
> + /* find a free scaler */
> + for (j = 0; j < intel_crtc->num_scalers; j++) {
> + if (scaler_state->scalers[j].in_use)
> + continue;
> +
> + *scaler_id = j;
> + scaler_state->scalers[*scaler_id].in_use = 1;
> + }
> + }
> +
> + if (WARN(*scaler_id < 0, "Cannot find scaler for %s:%d\n", name, idx))
> + return;
> +
> + /* set scaler mode */
> + if (plane_state && plane_state->base.fb &&
> + plane_state->base.fb->format->is_yuv &&
> + plane_state->base.fb->format->num_planes > 1) {
> + if (INTEL_GEN(dev_priv) == 9 &&
> + !IS_GEMINILAKE(dev_priv))
> + mode = SKL_PS_SCALER_MODE_NV12;
> + else
> + mode = PS_SCALER_MODE_PLANAR;
> +
> + } else if (INTEL_GEN(dev_priv) > 9 || IS_GEMINILAKE(dev_priv)) {
> + mode = PS_SCALER_MODE_PACKED;
> + } else if (num_scalers_need == 1 && intel_crtc->num_scalers > 1) {
> + /*
> + * when only 1 scaler is in use on a pipe with 2 scalers
> + * scaler 0 operates in high quality (HQ) mode.
> + * In this case use scaler 0 to take advantage of HQ mode
> + */
> + scaler_state->scalers[*scaler_id].in_use = 0;
> + *scaler_id = 0;
> + scaler_state->scalers[0].in_use = 1;
> + mode = SKL_PS_SCALER_MODE_HQ;
> + } else {
> + mode = SKL_PS_SCALER_MODE_DYN;
> + }
> +
> + DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
> + intel_crtc->pipe, *scaler_id, name, idx);
> + scaler_state->scalers[*scaler_id].mode = mode;
> +}
> +
> +
> /**
> * intel_atomic_setup_scalers() - setup scalers for crtc per staged requests
> * @dev_priv: i915 device
> @@ -232,7 +288,7 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
> struct drm_atomic_state *drm_state = crtc_state->base.state;
> struct intel_atomic_state *intel_state = to_intel_atomic_state(drm_state);
> int num_scalers_need;
> - int i, j;
> + int i;
>
> num_scalers_need = hweight32(scaler_state->scaler_users);
>
> @@ -304,59 +360,17 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
> idx = plane->base.id;
>
> /* plane on different crtc cannot be a scaler user of this crtc */
> - if (WARN_ON(intel_plane->pipe != intel_crtc->pipe)) {
> + if (WARN_ON(intel_plane->pipe != intel_crtc->pipe))
> continue;
> - }
>
> plane_state = intel_atomic_get_new_plane_state(intel_state,
> intel_plane);
> scaler_id = &plane_state->scaler_id;
> }
>
> - if (*scaler_id < 0) {
> - /* find a free scaler */
> - for (j = 0; j < intel_crtc->num_scalers; j++) {
> - if (!scaler_state->scalers[j].in_use) {
> - scaler_state->scalers[j].in_use = 1;
> - *scaler_id = j;
> - DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
> - intel_crtc->pipe, *scaler_id, name, idx);
> - break;
> - }
> - }
> - }
> -
> - if (WARN_ON(*scaler_id < 0)) {
> - DRM_DEBUG_KMS("Cannot find scaler for %s:%d\n", name, idx);
> - continue;
> - }
> -
> - /* set scaler mode */
> - if ((INTEL_GEN(dev_priv) >= 9) &&
> - plane_state && plane_state->base.fb &&
> - plane_state->base.fb->format->format ==
> - DRM_FORMAT_NV12) {
> - if (INTEL_GEN(dev_priv) == 9 &&
> - !IS_GEMINILAKE(dev_priv) &&
> - !IS_SKYLAKE(dev_priv))
> - scaler_state->scalers[*scaler_id].mode =
> - SKL_PS_SCALER_MODE_NV12;
> - else
> - scaler_state->scalers[*scaler_id].mode =
> - PS_SCALER_MODE_PLANAR;
> - } else if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) {
> - /*
> - * when only 1 scaler is in use on either pipe A or B,
> - * scaler 0 operates in high quality (HQ) mode.
> - * In this case use scaler 0 to take advantage of HQ mode
> - */
> - *scaler_id = 0;
> - scaler_state->scalers[0].in_use = 1;
> - scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
> - scaler_state->scalers[1].in_use = 0;
> - } else {
> - scaler_state->scalers[*scaler_id].mode = PS_SCALER_MODE_DYN;
> - }
> + intel_atomic_setup_scaler(scaler_state, num_scalers_need,
> + intel_crtc, name, idx,
> + plane_state, scaler_id);
> }
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 02374185b993..931898013506 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13963,7 +13963,7 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
> struct intel_scaler *scaler = &scaler_state->scalers[i];
>
> scaler->in_use = 0;
> - scaler->mode = PS_SCALER_MODE_DYN;
> + scaler->mode = 0;
> }
>
> scaler_state->scaler_id = -1;
> --
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 7/8] drm/i915: Move programming plane scaler to its own function.
2018-09-20 10:27 ` [PATCH 7/8] drm/i915: Move programming plane scaler to its own function Maarten Lankhorst
@ 2018-09-20 23:19 ` Matt Roper
2018-09-21 15:29 ` Ville Syrjälä
1 sibling, 0 replies; 32+ messages in thread
From: Matt Roper @ 2018-09-20 23:19 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 20, 2018 at 12:27:10PM +0200, Maarten Lankhorst wrote:
> This cleans the code up slightly, and will make other changes easier.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> drivers/gpu/drm/i915/intel_sprite.c | 90 +++++++++++++++++------------
> 1 file changed, 52 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index d4c8e10fc90b..7d3c7469d271 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -280,13 +280,63 @@ skl_plane_max_stride(struct intel_plane *plane,
> return min(8192 * cpp, 32768);
> }
>
> +static void
> +skl_program_scaler(struct drm_i915_private *dev_priv,
> + struct intel_plane *plane,
> + const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state)
> +{
> + enum plane_id plane_id = plane->id;
> + enum pipe pipe = plane->pipe;
> + int scaler_id = plane_state->scaler_id;
> + const struct intel_scaler *scaler =
> + &crtc_state->scaler_state.scalers[scaler_id];
> + int crtc_x = plane_state->base.dst.x1;
> + int crtc_y = plane_state->base.dst.y1;
> + uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
> + uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
> + u16 y_hphase, uv_rgb_hphase;
> + u16 y_vphase, uv_rgb_vphase;
> +
> + /* Sizes are 0 based */
> + crtc_w--;
> + crtc_h--;
> +
> + /* TODO: handle sub-pixel coordinates */
> + if (plane_state->base.fb->format->format == DRM_FORMAT_NV12) {
> + y_hphase = skl_scaler_calc_phase(1, false);
> + y_vphase = skl_scaler_calc_phase(1, false);
> +
> + /* MPEG2 chroma siting convention */
> + uv_rgb_hphase = skl_scaler_calc_phase(2, true);
> + uv_rgb_vphase = skl_scaler_calc_phase(2, false);
> + } else {
> + /* not used */
> + y_hphase = 0;
> + y_vphase = 0;
> +
> + uv_rgb_hphase = skl_scaler_calc_phase(1, false);
> + uv_rgb_vphase = skl_scaler_calc_phase(1, false);
> + }
> +
> + I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),
> + PS_SCALER_EN | PS_PLANE_SEL(plane_id) | scaler->mode);
> + I915_WRITE_FW(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
> + I915_WRITE_FW(SKL_PS_VPHASE(pipe, scaler_id),
> + PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> + I915_WRITE_FW(SKL_PS_HPHASE(pipe, scaler_id),
> + PS_Y_PHASE(y_hphase) | PS_UV_RGB_PHASE(uv_rgb_hphase));
> + I915_WRITE_FW(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
> + I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
> + ((crtc_w + 1) << 16)|(crtc_h + 1));
> +}
> +
> void
> skl_update_plane(struct intel_plane *plane,
> const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
> {
> struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> - const struct drm_framebuffer *fb = plane_state->base.fb;
> enum plane_id plane_id = plane->id;
> enum pipe pipe = plane->pipe;
> u32 plane_ctl = plane_state->ctl;
> @@ -296,8 +346,6 @@ skl_update_plane(struct intel_plane *plane,
> u32 aux_stride = skl_plane_stride(plane_state, 1);
> int crtc_x = plane_state->base.dst.x1;
> int crtc_y = plane_state->base.dst.y1;
> - uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
> - uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
> uint32_t x = plane_state->color_plane[0].x;
> uint32_t y = plane_state->color_plane[0].y;
> uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> @@ -307,8 +355,6 @@ skl_update_plane(struct intel_plane *plane,
> /* Sizes are 0 based */
> src_w--;
> src_h--;
> - crtc_w--;
> - crtc_h--;
>
> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>
> @@ -333,39 +379,7 @@ skl_update_plane(struct intel_plane *plane,
>
> /* program plane scaler */
> if (plane_state->scaler_id >= 0) {
> - int scaler_id = plane_state->scaler_id;
> - const struct intel_scaler *scaler =
> - &crtc_state->scaler_state.scalers[scaler_id];
> - u16 y_hphase, uv_rgb_hphase;
> - u16 y_vphase, uv_rgb_vphase;
> -
> - /* TODO: handle sub-pixel coordinates */
> - if (fb->format->format == DRM_FORMAT_NV12) {
> - y_hphase = skl_scaler_calc_phase(1, false);
> - y_vphase = skl_scaler_calc_phase(1, false);
> -
> - /* MPEG2 chroma siting convention */
> - uv_rgb_hphase = skl_scaler_calc_phase(2, true);
> - uv_rgb_vphase = skl_scaler_calc_phase(2, false);
> - } else {
> - /* not used */
> - y_hphase = 0;
> - y_vphase = 0;
> -
> - uv_rgb_hphase = skl_scaler_calc_phase(1, false);
> - uv_rgb_vphase = skl_scaler_calc_phase(1, false);
> - }
> -
> - I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),
> - PS_SCALER_EN | PS_PLANE_SEL(plane_id) | scaler->mode);
> - I915_WRITE_FW(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
> - I915_WRITE_FW(SKL_PS_VPHASE(pipe, scaler_id),
> - PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> - I915_WRITE_FW(SKL_PS_HPHASE(pipe, scaler_id),
> - PS_Y_PHASE(y_hphase) | PS_UV_RGB_PHASE(uv_rgb_hphase));
> - I915_WRITE_FW(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
> - I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
> - ((crtc_w + 1) << 16)|(crtc_h + 1));
> + skl_program_scaler(dev_priv, plane, crtc_state, plane_state);
>
> I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0);
> } else {
> --
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 8/8] drm/i915: Force planar YUV coordinates to be a multiple of 2, v2.
2018-09-20 10:27 ` [PATCH 8/8] drm/i915: Force planar YUV coordinates to be a multiple of 2, v2 Maarten Lankhorst
@ 2018-09-20 23:19 ` Matt Roper
0 siblings, 0 replies; 32+ messages in thread
From: Matt Roper @ 2018-09-20 23:19 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 20, 2018 at 12:27:11PM +0200, Maarten Lankhorst wrote:
> We can't make NV12 work any other way. The scaler doesn't handle odd
> coordinates well, and we will get visual corruption on the screen.
>
> Changes since v1:
> - Put the check in intel_plane_check_src_coordinates. (Ville)
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> ---
> drivers/gpu/drm/i915/intel_sprite.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 7d3c7469d271..46c6336cb858 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -253,13 +253,20 @@ int intel_plane_check_src_coordinates(struct intel_plane_state *plane_state)
> src->y2 = (src_y + src_h) << 16;
>
> if (fb->format->is_yuv &&
> - fb->format->format != DRM_FORMAT_NV12 &&
> (src_x & 1 || src_w & 1)) {
> DRM_DEBUG_KMS("src x/w (%u, %u) must be a multiple of 2 for YUV planes\n",
> src_x, src_w);
> return -EINVAL;
> }
>
> + if (fb->format->is_yuv &&
> + fb->format->num_planes > 1 &&
> + (src_y & 1 || src_h & 1)) {
> + DRM_DEBUG_KMS("src y/h (%u, %u) must be a multiple of 2 for planar YUV planes\n",
> + src_y, src_h);
> + return -EINVAL;
> + }
> +
> return 0;
> }
>
> --
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/8] drm/i915: Handle cursor updating active_planes correctly, v2.
2018-09-20 23:18 ` Matt Roper
@ 2018-09-21 9:41 ` Maarten Lankhorst
0 siblings, 0 replies; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-21 9:41 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
Op 21-09-18 om 01:18 schreef Matt Roper:
> On Thu, Sep 20, 2018 at 12:27:05PM +0200, Maarten Lankhorst wrote:
>> While we may not update new_crtc_state, we may clear active_planes
>> if the new cursor update state will disable the cursor, but we fail
>> after. If this is immediately followed by a modeset disable, we may
>> soon not disable the planes correctly when we start depending on
>> active_planes.
>>
>> Changes since v1:
>> - Clarify why we cannot swap crtc_state. (Matt)
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++-------
>> 1 file changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 58c188482c78..078cdcca88e1 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13515,14 +13515,16 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>> struct drm_plane_state *old_plane_state, *new_plane_state;
>> struct intel_plane *intel_plane = to_intel_plane(plane);
>> struct drm_framebuffer *old_fb;
>> - struct drm_crtc_state *crtc_state = crtc->state;
>> + struct intel_crtc_state *crtc_state =
>> + to_intel_crtc_state(crtc->state);
>> + struct intel_crtc_state *new_crtc_state;
>>
>> /*
>> * When crtc is inactive or there is a modeset pending,
>> * wait for it to complete in the slowpath
>> */
>> - if (!crtc_state->active || needs_modeset(crtc_state) ||
>> - to_intel_crtc_state(crtc_state)->update_pipe)
>> + if (!crtc_state->base.active || needs_modeset(&crtc_state->base) ||
>> + crtc_state->update_pipe)
>> goto slow;
>>
>> old_plane_state = plane->state;
>> @@ -13552,6 +13554,12 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>> if (!new_plane_state)
>> return -ENOMEM;
>>
>> + new_crtc_state = to_intel_crtc_state(intel_crtc_duplicate_state(crtc));
>> + if (!new_crtc_state) {
>> + ret = -ENOMEM;
>> + goto out_free;
>> + }
>> +
>> drm_atomic_set_fb_for_plane(new_plane_state, fb);
>>
>> new_plane_state->src_x = src_x;
>> @@ -13563,9 +13571,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>> new_plane_state->crtc_w = crtc_w;
>> new_plane_state->crtc_h = crtc_h;
>>
>> - ret = intel_plane_atomic_check_with_state(to_intel_crtc_state(crtc->state),
>> - to_intel_crtc_state(crtc->state), /* FIXME need a new crtc state? */
>> - to_intel_plane_state(plane->state),
>> + ret = intel_plane_atomic_check_with_state(crtc_state, new_crtc_state,
>> + to_intel_plane_state(old_plane_state),
>> to_intel_plane_state(new_plane_state));
>> if (ret)
>> goto out_free;
>> @@ -13587,10 +13594,21 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>> /* Swap plane state */
>> plane->state = new_plane_state;
>>
>> + /*
>> + * We cannot swap crtc_state as it may be in use by an atomic commit or
>> + * page flip that's running simultaneously. If we swap crtc_state and
>> + * destroy the old state, we will cause a use-after-free there.
> Just to confirm, the commit running simultaneously would have to have
> already dropped locks (specifically the crtc lock) and returned to
> userspace for us to have this problem, right? So it's either a
> non-blocking commit in the process of doing its programming via
> workqueue, or a blocking commit that's done everything except
> intel_atomic_cleanup_work?
>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Anything before drm_atomic_helper_commit_hw_done().
Cleanup work is done after, and isn't allowed to look at new state any more. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] drm/i915: Clean up scaler setup, v2.
2018-09-20 23:19 ` Matt Roper
@ 2018-09-21 14:44 ` Maarten Lankhorst
2018-09-21 16:40 ` Matt Roper
0 siblings, 1 reply; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-21 14:44 UTC (permalink / raw)
To: intel-gfx
On skylake we can switch to a high quality scaler mode when only 1 out
of 2 scalers are used, but on GLK and later bit 28 has a different
meaning. Don't set it, and make clear the distinction between
SKL and later PS values.
Changes since v1:
- Add missing break statement.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com> #v1
---
drivers/gpu/drm/i915/i915_reg.h | 7 +-
drivers/gpu/drm/i915/intel_atomic.c | 109 +++++++++++++++------------
drivers/gpu/drm/i915/intel_display.c | 2 +-
3 files changed, 67 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4948b352bf4c..e7e6ca7f9665 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6850,11 +6850,12 @@ enum {
#define _PS_2B_CTRL 0x68A80
#define _PS_1C_CTRL 0x69180
#define PS_SCALER_EN (1 << 31)
-#define PS_SCALER_MODE_MASK (3 << 28)
-#define PS_SCALER_MODE_DYN (0 << 28)
-#define PS_SCALER_MODE_HQ (1 << 28)
+#define SKL_PS_SCALER_MODE_MASK (3 << 28)
+#define SKL_PS_SCALER_MODE_DYN (0 << 28)
+#define SKL_PS_SCALER_MODE_HQ (1 << 28)
#define SKL_PS_SCALER_MODE_NV12 (2 << 28)
#define PS_SCALER_MODE_PLANAR (1 << 29)
+#define PS_SCALER_MODE_PACKED (0 << 29)
#define PS_PLANE_SEL_MASK (7 << 25)
#define PS_PLANE_SEL(plane) (((plane) + 1) << 25)
#define PS_FILTER_MASK (3 << 23)
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index b04952bacf77..c23a6ec40f60 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -203,6 +203,63 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
drm_atomic_helper_crtc_destroy_state(crtc, state);
}
+static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
+ int num_scalers_need, struct intel_crtc *intel_crtc,
+ const char *name, int idx,
+ struct intel_plane_state *plane_state,
+ int *scaler_id)
+{
+ struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
+ int j;
+ u32 mode;
+
+ if (*scaler_id < 0) {
+ /* find a free scaler */
+ for (j = 0; j < intel_crtc->num_scalers; j++) {
+ if (scaler_state->scalers[j].in_use)
+ continue;
+
+ *scaler_id = j;
+ scaler_state->scalers[*scaler_id].in_use = 1;
+ break;
+ }
+ }
+
+ if (WARN(*scaler_id < 0, "Cannot find scaler for %s:%d\n", name, idx))
+ return;
+
+ /* set scaler mode */
+ if (plane_state && plane_state->base.fb &&
+ plane_state->base.fb->format->is_yuv &&
+ plane_state->base.fb->format->num_planes > 1) {
+ if (INTEL_GEN(dev_priv) == 9 &&
+ !IS_GEMINILAKE(dev_priv))
+ mode = SKL_PS_SCALER_MODE_NV12;
+ else
+ mode = PS_SCALER_MODE_PLANAR;
+
+ } else if (INTEL_GEN(dev_priv) > 9 || IS_GEMINILAKE(dev_priv)) {
+ mode = PS_SCALER_MODE_PACKED;
+ } else if (num_scalers_need == 1 && intel_crtc->num_scalers > 1) {
+ /*
+ * when only 1 scaler is in use on a pipe with 2 scalers
+ * scaler 0 operates in high quality (HQ) mode.
+ * In this case use scaler 0 to take advantage of HQ mode
+ */
+ scaler_state->scalers[*scaler_id].in_use = 0;
+ *scaler_id = 0;
+ scaler_state->scalers[0].in_use = 1;
+ mode = SKL_PS_SCALER_MODE_HQ;
+ } else {
+ mode = SKL_PS_SCALER_MODE_DYN;
+ }
+
+ DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
+ intel_crtc->pipe, *scaler_id, name, idx);
+ scaler_state->scalers[*scaler_id].mode = mode;
+}
+
+
/**
* intel_atomic_setup_scalers() - setup scalers for crtc per staged requests
* @dev_priv: i915 device
@@ -232,7 +289,7 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
struct drm_atomic_state *drm_state = crtc_state->base.state;
struct intel_atomic_state *intel_state = to_intel_atomic_state(drm_state);
int num_scalers_need;
- int i, j;
+ int i;
num_scalers_need = hweight32(scaler_state->scaler_users);
@@ -304,59 +361,17 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
idx = plane->base.id;
/* plane on different crtc cannot be a scaler user of this crtc */
- if (WARN_ON(intel_plane->pipe != intel_crtc->pipe)) {
+ if (WARN_ON(intel_plane->pipe != intel_crtc->pipe))
continue;
- }
plane_state = intel_atomic_get_new_plane_state(intel_state,
intel_plane);
scaler_id = &plane_state->scaler_id;
}
- if (*scaler_id < 0) {
- /* find a free scaler */
- for (j = 0; j < intel_crtc->num_scalers; j++) {
- if (!scaler_state->scalers[j].in_use) {
- scaler_state->scalers[j].in_use = 1;
- *scaler_id = j;
- DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
- intel_crtc->pipe, *scaler_id, name, idx);
- break;
- }
- }
- }
-
- if (WARN_ON(*scaler_id < 0)) {
- DRM_DEBUG_KMS("Cannot find scaler for %s:%d\n", name, idx);
- continue;
- }
-
- /* set scaler mode */
- if ((INTEL_GEN(dev_priv) >= 9) &&
- plane_state && plane_state->base.fb &&
- plane_state->base.fb->format->format ==
- DRM_FORMAT_NV12) {
- if (INTEL_GEN(dev_priv) == 9 &&
- !IS_GEMINILAKE(dev_priv) &&
- !IS_SKYLAKE(dev_priv))
- scaler_state->scalers[*scaler_id].mode =
- SKL_PS_SCALER_MODE_NV12;
- else
- scaler_state->scalers[*scaler_id].mode =
- PS_SCALER_MODE_PLANAR;
- } else if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) {
- /*
- * when only 1 scaler is in use on either pipe A or B,
- * scaler 0 operates in high quality (HQ) mode.
- * In this case use scaler 0 to take advantage of HQ mode
- */
- *scaler_id = 0;
- scaler_state->scalers[0].in_use = 1;
- scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
- scaler_state->scalers[1].in_use = 0;
- } else {
- scaler_state->scalers[*scaler_id].mode = PS_SCALER_MODE_DYN;
- }
+ intel_atomic_setup_scaler(scaler_state, num_scalers_need,
+ intel_crtc, name, idx,
+ plane_state, scaler_id);
}
return 0;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 02374185b993..931898013506 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13963,7 +13963,7 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
struct intel_scaler *scaler = &scaler_state->scalers[i];
scaler->in_use = 0;
- scaler->mode = PS_SCALER_MODE_DYN;
+ scaler->mode = 0;
}
scaler_state->scaler_id = -1;
--
2.18.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 32+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Preparations for adding gen11 planar formats. (rev2)
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
` (11 preceding siblings ...)
2018-09-20 12:10 ` ✗ Fi.CI.IGT: failure " Patchwork
@ 2018-09-21 15:00 ` Patchwork
2018-09-21 15:03 ` ✗ Fi.CI.SPARSE: " Patchwork
` (2 subsequent siblings)
15 siblings, 0 replies; 32+ messages in thread
From: Patchwork @ 2018-09-21 15:00 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Preparations for adding gen11 planar formats. (rev2)
URL : https://patchwork.freedesktop.org/series/49956/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
aa09781991ac drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()
dadea6a97480 drm/i915: Handle cursor updating active_planes correctly, v2.
2cedf31aa420 drm/i915: Unconditionally clear plane visibility, v2.
7af87226f223 drm/i915: Make intel_crtc_disable_planes() use active planes mask.
-:26: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#26: FILE: drivers/gpu/drm/i915/intel_display.c:5405:
+static void intel_crtc_disable_planes(struct intel_crtc *crtc, unsigned plane_mask)
-:34: WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
#34: FILE: drivers/gpu/drm/i915/intel_display.c:5409:
+ unsigned fb_bits = 0;
total: 0 errors, 2 warnings, 0 checks, 45 lines checked
2f709c1eb234 drm/i915: Replace call to commit_planes_on_crtc with internal update, v2.
f4e6e0126f4b drm/i915: Clean up scaler setup, v2.
-:101: CHECK:LINE_SPACING: Please don't use multiple blank lines
#101: FILE: drivers/gpu/drm/i915/intel_atomic.c:262:
+
+
total: 0 errors, 0 warnings, 1 checks, 157 lines checked
d8fb63850500 drm/i915: Move programming plane scaler to its own function.
-:67: CHECK:SPACING: spaces preferred around that '|' (ctx:VxV)
#67: FILE: drivers/gpu/drm/i915/intel_sprite.c:331:
+ ((crtc_w + 1) << 16)|(crtc_h + 1));
^
total: 0 errors, 0 warnings, 1 checks, 120 lines checked
a1b43d95b54c drm/i915: Force planar YUV coordinates to be a multiple of 2, v2.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* ✗ Fi.CI.SPARSE: warning for drm/i915: Preparations for adding gen11 planar formats. (rev2)
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
` (12 preceding siblings ...)
2018-09-21 15:00 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Preparations for adding gen11 planar formats. (rev2) Patchwork
@ 2018-09-21 15:03 ` Patchwork
2018-09-21 15:21 ` ✓ Fi.CI.BAT: success " Patchwork
2018-09-21 16:23 ` ✓ Fi.CI.IGT: " Patchwork
15 siblings, 0 replies; 32+ messages in thread
From: Patchwork @ 2018-09-21 15:03 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Preparations for adding gen11 planar formats. (rev2)
URL : https://patchwork.freedesktop.org/series/49956/
State : warning
== Summary ==
$ dim sparse origin/drm-tip
Commit: drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()
Okay!
Commit: drm/i915: Handle cursor updating active_planes correctly, v2.
Okay!
Commit: drm/i915: Unconditionally clear plane visibility, v2.
Okay!
Commit: drm/i915: Make intel_crtc_disable_planes() use active planes mask.
Okay!
Commit: drm/i915: Replace call to commit_planes_on_crtc with internal update, v2.
Okay!
Commit: drm/i915: Clean up scaler setup, v2.
Okay!
Commit: drm/i915: Move programming plane scaler to its own function.
-O:drivers/gpu/drm/i915/intel_sprite.c:280:24: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/intel_sprite.c:280:24: warning: expression using sizeof(void)
Commit: drm/i915: Force planar YUV coordinates to be a multiple of 2, v2.
Okay!
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Preparations for adding gen11 planar formats. (rev2)
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
` (13 preceding siblings ...)
2018-09-21 15:03 ` ✗ Fi.CI.SPARSE: " Patchwork
@ 2018-09-21 15:21 ` Patchwork
2018-09-21 16:23 ` ✓ Fi.CI.IGT: " Patchwork
15 siblings, 0 replies; 32+ messages in thread
From: Patchwork @ 2018-09-21 15:21 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Preparations for adding gen11 planar formats. (rev2)
URL : https://patchwork.freedesktop.org/series/49956/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4860 -> Patchwork_10251 =
== Summary - SUCCESS ==
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/49956/revisions/2/mbox/
== Known issues ==
Here are the changes found in Patchwork_10251 that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@drv_selftest@live_evict:
fi-bsw-kefka: PASS -> DMESG-WARN (fdo#107709)
igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
fi-byt-clapper: PASS -> FAIL (fdo#103191, fdo#107362)
igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
{fi-skl-caroline}: PASS -> INCOMPLETE (fdo#104108, fdo#107556)
igt@kms_psr@primary_page_flip:
fi-kbl-7560u: PASS -> FAIL (fdo#107336)
==== Possible fixes ====
igt@gem_exec_suspend@basic-s3:
fi-blb-e6850: INCOMPLETE (fdo#107718) -> PASS
igt@gem_mmap_gtt@basic-short:
fi-glk-dsi: INCOMPLETE (fdo#103359, k.org#198133) -> PASS
igt@kms_frontbuffer_tracking@basic:
fi-byt-clapper: FAIL (fdo#103167) -> PASS
igt@kms_psr@primary_page_flip:
fi-kbl-r: FAIL (fdo#107336) -> PASS
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
fdo#107556 https://bugs.freedesktop.org/show_bug.cgi?id=107556
fdo#107709 https://bugs.freedesktop.org/show_bug.cgi?id=107709
fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133
== Participating hosts (52 -> 47) ==
Missing (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u
== Build changes ==
* Linux: CI_DRM_4860 -> Patchwork_10251
CI_DRM_4860: 31c6d083d8c6fe280e3daabf22521335762759b9 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4649: 19b0c74d20d9b53d4c82be14af0909a3b6846010 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10251: a1b43d95b54c934c70306c559a20d78dd53fda0d @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
a1b43d95b54c drm/i915: Force planar YUV coordinates to be a multiple of 2, v2.
d8fb63850500 drm/i915: Move programming plane scaler to its own function.
f4e6e0126f4b drm/i915: Clean up scaler setup, v2.
2f709c1eb234 drm/i915: Replace call to commit_planes_on_crtc with internal update, v2.
7af87226f223 drm/i915: Make intel_crtc_disable_planes() use active planes mask.
2cedf31aa420 drm/i915: Unconditionally clear plane visibility, v2.
dadea6a97480 drm/i915: Handle cursor updating active_planes correctly, v2.
aa09781991ac drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail()
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10251/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2.
2018-09-20 10:27 ` [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2 Maarten Lankhorst
2018-09-20 23:18 ` Matt Roper
@ 2018-09-21 15:26 ` Ville Syrjälä
2018-09-21 16:00 ` Maarten Lankhorst
1 sibling, 1 reply; 32+ messages in thread
From: Ville Syrjälä @ 2018-09-21 15:26 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 20, 2018 at 12:27:06PM +0200, Maarten Lankhorst wrote:
> We need to assume the plane has been visible before, even if no CRTC
> is assigned to the plane. This is because nv12 will enable a a extra
> plane and make it visible by marking it in crtc_state->active_planes
> for intel_update_planes_on_crtc().
>
> Additionally, clear visible flag in intel_plane_atomic_check, in case
> we ever hit a bug with visibility. Our code implicitly assumes that
> plane_state->visible is only true when crtc and fb are set,
> so we will either null deref in intel_fbc_choose_crtc() or
> do something bad during the actual commit which cares even more.
>
> Changes since v1:
> - Unconditionally clear crtc_state->active_planes as well.
> - Reword commit message, since this is now a preparation patch for
> NV12 Y / UV plane linking.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_atomic_plane.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> index aabebe0d2e9b..f70e9cb9cf02 100644
> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> @@ -117,10 +117,13 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> struct intel_plane *intel_plane = to_intel_plane(plane);
> int ret;
>
> + crtc_state->active_planes &= ~BIT(intel_plane->id);
nv12_planes too?
> + intel_state->base.visible = false;
> +
> + /* If this is a cursor plane, no further checks are needed. */
> if (!intel_state->base.crtc && !old_plane_state->base.crtc)
> return 0;
>
> - intel_state->base.visible = false;
> ret = intel_plane->check_plane(crtc_state, intel_state);
> if (ret)
> return ret;
> @@ -128,8 +131,6 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> /* FIXME pre-g4x don't work like this */
> if (state->visible)
> crtc_state->active_planes |= BIT(intel_plane->id);
> - else
> - crtc_state->active_planes &= ~BIT(intel_plane->id);
>
> if (state->visible && state->fb->format->format == DRM_FORMAT_NV12)
> crtc_state->nv12_planes |= BIT(intel_plane->id);
> @@ -152,6 +153,7 @@ static int intel_plane_atomic_check(struct drm_plane *plane,
> const struct drm_crtc_state *old_crtc_state;
> struct drm_crtc_state *new_crtc_state;
>
> + new_plane_state->visible = false;
> if (!crtc)
> return 0;
>
> --
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 7/8] drm/i915: Move programming plane scaler to its own function.
2018-09-20 10:27 ` [PATCH 7/8] drm/i915: Move programming plane scaler to its own function Maarten Lankhorst
2018-09-20 23:19 ` Matt Roper
@ 2018-09-21 15:29 ` Ville Syrjälä
1 sibling, 0 replies; 32+ messages in thread
From: Ville Syrjälä @ 2018-09-21 15:29 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Thu, Sep 20, 2018 at 12:27:10PM +0200, Maarten Lankhorst wrote:
> This cleans the code up slightly, and will make other changes easier.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_sprite.c | 90 +++++++++++++++++------------
> 1 file changed, 52 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index d4c8e10fc90b..7d3c7469d271 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -280,13 +280,63 @@ skl_plane_max_stride(struct intel_plane *plane,
> return min(8192 * cpp, 32768);
> }
>
> +static void
> +skl_program_scaler(struct drm_i915_private *dev_priv,
> + struct intel_plane *plane,
Could have dug these out from the states rather than passing in
explicitly. I prefer minimlism when it comes to function arguments.
> + const struct intel_crtc_state *crtc_state,
> + const struct intel_plane_state *plane_state)
> +{
> + enum plane_id plane_id = plane->id;
> + enum pipe pipe = plane->pipe;
> + int scaler_id = plane_state->scaler_id;
> + const struct intel_scaler *scaler =
> + &crtc_state->scaler_state.scalers[scaler_id];
> + int crtc_x = plane_state->base.dst.x1;
> + int crtc_y = plane_state->base.dst.y1;
> + uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
> + uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
> + u16 y_hphase, uv_rgb_hphase;
> + u16 y_vphase, uv_rgb_vphase;
> +
> + /* Sizes are 0 based */
> + crtc_w--;
> + crtc_h--;
> +
> + /* TODO: handle sub-pixel coordinates */
> + if (plane_state->base.fb->format->format == DRM_FORMAT_NV12) {
> + y_hphase = skl_scaler_calc_phase(1, false);
> + y_vphase = skl_scaler_calc_phase(1, false);
> +
> + /* MPEG2 chroma siting convention */
> + uv_rgb_hphase = skl_scaler_calc_phase(2, true);
> + uv_rgb_vphase = skl_scaler_calc_phase(2, false);
> + } else {
> + /* not used */
> + y_hphase = 0;
> + y_vphase = 0;
> +
> + uv_rgb_hphase = skl_scaler_calc_phase(1, false);
> + uv_rgb_vphase = skl_scaler_calc_phase(1, false);
> + }
> +
> + I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),
> + PS_SCALER_EN | PS_PLANE_SEL(plane_id) | scaler->mode);
> + I915_WRITE_FW(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
> + I915_WRITE_FW(SKL_PS_VPHASE(pipe, scaler_id),
> + PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> + I915_WRITE_FW(SKL_PS_HPHASE(pipe, scaler_id),
> + PS_Y_PHASE(y_hphase) | PS_UV_RGB_PHASE(uv_rgb_hphase));
> + I915_WRITE_FW(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
> + I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
> + ((crtc_w + 1) << 16)|(crtc_h + 1));
> +}
> +
> void
> skl_update_plane(struct intel_plane *plane,
> const struct intel_crtc_state *crtc_state,
> const struct intel_plane_state *plane_state)
> {
> struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> - const struct drm_framebuffer *fb = plane_state->base.fb;
> enum plane_id plane_id = plane->id;
> enum pipe pipe = plane->pipe;
> u32 plane_ctl = plane_state->ctl;
> @@ -296,8 +346,6 @@ skl_update_plane(struct intel_plane *plane,
> u32 aux_stride = skl_plane_stride(plane_state, 1);
> int crtc_x = plane_state->base.dst.x1;
> int crtc_y = plane_state->base.dst.y1;
> - uint32_t crtc_w = drm_rect_width(&plane_state->base.dst);
> - uint32_t crtc_h = drm_rect_height(&plane_state->base.dst);
> uint32_t x = plane_state->color_plane[0].x;
> uint32_t y = plane_state->color_plane[0].y;
> uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> @@ -307,8 +355,6 @@ skl_update_plane(struct intel_plane *plane,
> /* Sizes are 0 based */
> src_w--;
> src_h--;
> - crtc_w--;
> - crtc_h--;
>
> spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>
> @@ -333,39 +379,7 @@ skl_update_plane(struct intel_plane *plane,
>
> /* program plane scaler */
> if (plane_state->scaler_id >= 0) {
> - int scaler_id = plane_state->scaler_id;
> - const struct intel_scaler *scaler =
> - &crtc_state->scaler_state.scalers[scaler_id];
> - u16 y_hphase, uv_rgb_hphase;
> - u16 y_vphase, uv_rgb_vphase;
> -
> - /* TODO: handle sub-pixel coordinates */
> - if (fb->format->format == DRM_FORMAT_NV12) {
> - y_hphase = skl_scaler_calc_phase(1, false);
> - y_vphase = skl_scaler_calc_phase(1, false);
> -
> - /* MPEG2 chroma siting convention */
> - uv_rgb_hphase = skl_scaler_calc_phase(2, true);
> - uv_rgb_vphase = skl_scaler_calc_phase(2, false);
> - } else {
> - /* not used */
> - y_hphase = 0;
> - y_vphase = 0;
> -
> - uv_rgb_hphase = skl_scaler_calc_phase(1, false);
> - uv_rgb_vphase = skl_scaler_calc_phase(1, false);
> - }
> -
> - I915_WRITE_FW(SKL_PS_CTRL(pipe, scaler_id),
> - PS_SCALER_EN | PS_PLANE_SEL(plane_id) | scaler->mode);
> - I915_WRITE_FW(SKL_PS_PWR_GATE(pipe, scaler_id), 0);
> - I915_WRITE_FW(SKL_PS_VPHASE(pipe, scaler_id),
> - PS_Y_PHASE(y_vphase) | PS_UV_RGB_PHASE(uv_rgb_vphase));
> - I915_WRITE_FW(SKL_PS_HPHASE(pipe, scaler_id),
> - PS_Y_PHASE(y_hphase) | PS_UV_RGB_PHASE(uv_rgb_hphase));
> - I915_WRITE_FW(SKL_PS_WIN_POS(pipe, scaler_id), (crtc_x << 16) | crtc_y);
> - I915_WRITE_FW(SKL_PS_WIN_SZ(pipe, scaler_id),
> - ((crtc_w + 1) << 16)|(crtc_h + 1));
> + skl_program_scaler(dev_priv, plane, crtc_state, plane_state);
>
> I915_WRITE_FW(PLANE_POS(pipe, plane_id), 0);
> } else {
> --
> 2.18.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2.
2018-09-21 15:26 ` Ville Syrjälä
@ 2018-09-21 16:00 ` Maarten Lankhorst
2018-09-21 16:15 ` Ville Syrjälä
0 siblings, 1 reply; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-21 16:00 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Op 21-09-18 om 17:26 schreef Ville Syrjälä:
> On Thu, Sep 20, 2018 at 12:27:06PM +0200, Maarten Lankhorst wrote:
>> We need to assume the plane has been visible before, even if no CRTC
>> is assigned to the plane. This is because nv12 will enable a a extra
>> plane and make it visible by marking it in crtc_state->active_planes
>> for intel_update_planes_on_crtc().
>>
>> Additionally, clear visible flag in intel_plane_atomic_check, in case
>> we ever hit a bug with visibility. Our code implicitly assumes that
>> plane_state->visible is only true when crtc and fb are set,
>> so we will either null deref in intel_fbc_choose_crtc() or
>> do something bad during the actual commit which cares even more.
>>
>> Changes since v1:
>> - Unconditionally clear crtc_state->active_planes as well.
>> - Reword commit message, since this is now a preparation patch for
>> NV12 Y / UV plane linking.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_atomic_plane.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> index aabebe0d2e9b..f70e9cb9cf02 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>> @@ -117,10 +117,13 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>> struct intel_plane *intel_plane = to_intel_plane(plane);
>> int ret;
>>
>> + crtc_state->active_planes &= ~BIT(intel_plane->id);
> nv12_planes too?
No, we don't have to. We don't set nv12_planes on the Y plane. :)
In all other cases we clear it correctly.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2.
2018-09-21 16:00 ` Maarten Lankhorst
@ 2018-09-21 16:15 ` Ville Syrjälä
2018-09-21 16:20 ` Maarten Lankhorst
0 siblings, 1 reply; 32+ messages in thread
From: Ville Syrjälä @ 2018-09-21 16:15 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Fri, Sep 21, 2018 at 06:00:27PM +0200, Maarten Lankhorst wrote:
> Op 21-09-18 om 17:26 schreef Ville Syrjälä:
> > On Thu, Sep 20, 2018 at 12:27:06PM +0200, Maarten Lankhorst wrote:
> >> We need to assume the plane has been visible before, even if no CRTC
> >> is assigned to the plane. This is because nv12 will enable a a extra
> >> plane and make it visible by marking it in crtc_state->active_planes
> >> for intel_update_planes_on_crtc().
> >>
> >> Additionally, clear visible flag in intel_plane_atomic_check, in case
> >> we ever hit a bug with visibility. Our code implicitly assumes that
> >> plane_state->visible is only true when crtc and fb are set,
> >> so we will either null deref in intel_fbc_choose_crtc() or
> >> do something bad during the actual commit which cares even more.
> >>
> >> Changes since v1:
> >> - Unconditionally clear crtc_state->active_planes as well.
> >> - Reword commit message, since this is now a preparation patch for
> >> NV12 Y / UV plane linking.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >> drivers/gpu/drm/i915/intel_atomic_plane.c | 8 +++++---
> >> 1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> index aabebe0d2e9b..f70e9cb9cf02 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >> @@ -117,10 +117,13 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> >> struct intel_plane *intel_plane = to_intel_plane(plane);
> >> int ret;
> >>
> >> + crtc_state->active_planes &= ~BIT(intel_plane->id);
> > nv12_planes too?
> No, we don't have to. We don't set nv12_planes on the Y plane. :)
> In all other cases we clear it correctly.
I think sticking to single approach would be less confusing nonetheless.
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2.
2018-09-21 16:15 ` Ville Syrjälä
@ 2018-09-21 16:20 ` Maarten Lankhorst
2018-09-21 16:24 ` Ville Syrjälä
0 siblings, 1 reply; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-21 16:20 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
Op 21-09-18 om 18:15 schreef Ville Syrjälä:
> On Fri, Sep 21, 2018 at 06:00:27PM +0200, Maarten Lankhorst wrote:
>> Op 21-09-18 om 17:26 schreef Ville Syrjälä:
>>> On Thu, Sep 20, 2018 at 12:27:06PM +0200, Maarten Lankhorst wrote:
>>>> We need to assume the plane has been visible before, even if no CRTC
>>>> is assigned to the plane. This is because nv12 will enable a a extra
>>>> plane and make it visible by marking it in crtc_state->active_planes
>>>> for intel_update_planes_on_crtc().
>>>>
>>>> Additionally, clear visible flag in intel_plane_atomic_check, in case
>>>> we ever hit a bug with visibility. Our code implicitly assumes that
>>>> plane_state->visible is only true when crtc and fb are set,
>>>> so we will either null deref in intel_fbc_choose_crtc() or
>>>> do something bad during the actual commit which cares even more.
>>>>
>>>> Changes since v1:
>>>> - Unconditionally clear crtc_state->active_planes as well.
>>>> - Reword commit message, since this is now a preparation patch for
>>>> NV12 Y / UV plane linking.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/intel_atomic_plane.c | 8 +++++---
>>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>> index aabebe0d2e9b..f70e9cb9cf02 100644
>>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
>>>> @@ -117,10 +117,13 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
>>>> struct intel_plane *intel_plane = to_intel_plane(plane);
>>>> int ret;
>>>>
>>>> + crtc_state->active_planes &= ~BIT(intel_plane->id);
>>> nv12_planes too?
>> No, we don't have to. We don't set nv12_planes on the Y plane. :)
>> In all other cases we clear it correctly.
> I think sticking to single approach would be less confusing nonetheless.
>
Agreed, will fix it up when pushing this patch.
Can I add your r-b on it then?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* ✓ Fi.CI.IGT: success for drm/i915: Preparations for adding gen11 planar formats. (rev2)
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
` (14 preceding siblings ...)
2018-09-21 15:21 ` ✓ Fi.CI.BAT: success " Patchwork
@ 2018-09-21 16:23 ` Patchwork
15 siblings, 0 replies; 32+ messages in thread
From: Patchwork @ 2018-09-21 16:23 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Preparations for adding gen11 planar formats. (rev2)
URL : https://patchwork.freedesktop.org/series/49956/
State : success
== Summary ==
= CI Bug Log - changes from CI_DRM_4860_full -> Patchwork_10251_full =
== Summary - WARNING ==
Minor unknown changes coming with Patchwork_10251_full need to be verified
manually.
If you think the reported changes have nothing to do with the changes
introduced in Patchwork_10251_full, please notify your bug team to allow them
to document this new failure mode, which will reduce false positives in CI.
== Possible new issues ==
Here are the unknown changes that may have been introduced in Patchwork_10251_full:
=== IGT changes ===
==== Warnings ====
igt@pm_rc6_residency@rc6-accuracy:
shard-kbl: PASS -> SKIP
shard-snb: SKIP -> PASS
== Known issues ==
Here are the changes found in Patchwork_10251_full that come from known issues:
=== IGT changes ===
==== Issues hit ====
igt@gem_exec_await@wide-contexts:
shard-apl: PASS -> FAIL (fdo#106680)
shard-kbl: PASS -> FAIL (fdo#106680)
igt@gem_exec_schedule@pi-ringfull-vebox:
shard-glk: NOTRUN -> FAIL (fdo#103158)
igt@kms_available_modes_crc@available_mode_test_crc:
shard-glk: NOTRUN -> FAIL (fdo#106641)
igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
shard-glk: NOTRUN -> DMESG-WARN (fdo#107956) +1
igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
shard-glk: NOTRUN -> FAIL (fdo#106509, fdo#105454)
igt@kms_flip@plain-flip-fb-recreate-interruptible:
shard-kbl: PASS -> FAIL (fdo#100368)
igt@kms_setmode@basic:
shard-apl: PASS -> FAIL (fdo#99912)
shard-kbl: PASS -> FAIL (fdo#99912)
igt@perf@polling:
shard-hsw: PASS -> FAIL (fdo#102252)
==== Possible fixes ====
igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-a:
shard-apl: DMESG-WARN (fdo#107956) -> PASS
igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
shard-glk: FAIL (fdo#105363) -> PASS
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#103158 https://bugs.freedesktop.org/show_bug.cgi?id=103158
fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
fdo#105454 https://bugs.freedesktop.org/show_bug.cgi?id=105454
fdo#106509 https://bugs.freedesktop.org/show_bug.cgi?id=106509
fdo#106641 https://bugs.freedesktop.org/show_bug.cgi?id=106641
fdo#106680 https://bugs.freedesktop.org/show_bug.cgi?id=106680
fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
== Participating hosts (5 -> 5) ==
No changes in participating hosts
== Build changes ==
* Linux: CI_DRM_4860 -> Patchwork_10251
CI_DRM_4860: 31c6d083d8c6fe280e3daabf22521335762759b9 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4649: 19b0c74d20d9b53d4c82be14af0909a3b6846010 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10251: a1b43d95b54c934c70306c559a20d78dd53fda0d @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10251/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2.
2018-09-21 16:20 ` Maarten Lankhorst
@ 2018-09-21 16:24 ` Ville Syrjälä
0 siblings, 0 replies; 32+ messages in thread
From: Ville Syrjälä @ 2018-09-21 16:24 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Fri, Sep 21, 2018 at 06:20:37PM +0200, Maarten Lankhorst wrote:
> Op 21-09-18 om 18:15 schreef Ville Syrjälä:
> > On Fri, Sep 21, 2018 at 06:00:27PM +0200, Maarten Lankhorst wrote:
> >> Op 21-09-18 om 17:26 schreef Ville Syrjälä:
> >>> On Thu, Sep 20, 2018 at 12:27:06PM +0200, Maarten Lankhorst wrote:
> >>>> We need to assume the plane has been visible before, even if no CRTC
> >>>> is assigned to the plane. This is because nv12 will enable a a extra
> >>>> plane and make it visible by marking it in crtc_state->active_planes
> >>>> for intel_update_planes_on_crtc().
> >>>>
> >>>> Additionally, clear visible flag in intel_plane_atomic_check, in case
> >>>> we ever hit a bug with visibility. Our code implicitly assumes that
> >>>> plane_state->visible is only true when crtc and fb are set,
> >>>> so we will either null deref in intel_fbc_choose_crtc() or
> >>>> do something bad during the actual commit which cares even more.
> >>>>
> >>>> Changes since v1:
> >>>> - Unconditionally clear crtc_state->active_planes as well.
> >>>> - Reword commit message, since this is now a preparation patch for
> >>>> NV12 Y / UV plane linking.
> >>>>
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>> drivers/gpu/drm/i915/intel_atomic_plane.c | 8 +++++---
> >>>> 1 file changed, 5 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>> index aabebe0d2e9b..f70e9cb9cf02 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c
> >>>> @@ -117,10 +117,13 @@ int intel_plane_atomic_check_with_state(const struct intel_crtc_state *old_crtc_
> >>>> struct intel_plane *intel_plane = to_intel_plane(plane);
> >>>> int ret;
> >>>>
> >>>> + crtc_state->active_planes &= ~BIT(intel_plane->id);
> >>> nv12_planes too?
> >> No, we don't have to. We don't set nv12_planes on the Y plane. :)
> >> In all other cases we clear it correctly.
> > I think sticking to single approach would be less confusing nonetheless.
> >
> Agreed, will fix it up when pushing this patch.
>
> Can I add your r-b on it then?
Sure.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/i915: Clean up scaler setup, v2.
2018-09-21 14:44 ` [PATCH] drm/i915: Clean up scaler setup, v2 Maarten Lankhorst
@ 2018-09-21 16:40 ` Matt Roper
2018-09-21 17:32 ` Maarten Lankhorst
0 siblings, 1 reply; 32+ messages in thread
From: Matt Roper @ 2018-09-21 16:40 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: intel-gfx
On Fri, Sep 21, 2018 at 04:44:37PM +0200, Maarten Lankhorst wrote:
> On skylake we can switch to a high quality scaler mode when only 1 out
> of 2 scalers are used, but on GLK and later bit 28 has a different
> meaning. Don't set it, and make clear the distinction between
> SKL and later PS values.
>
> Changes since v1:
> - Add missing break statement.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> #v1
R-b for v2 as well.
> ---
> drivers/gpu/drm/i915/i915_reg.h | 7 +-
> drivers/gpu/drm/i915/intel_atomic.c | 109 +++++++++++++++------------
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> 3 files changed, 67 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 4948b352bf4c..e7e6ca7f9665 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6850,11 +6850,12 @@ enum {
> #define _PS_2B_CTRL 0x68A80
> #define _PS_1C_CTRL 0x69180
> #define PS_SCALER_EN (1 << 31)
> -#define PS_SCALER_MODE_MASK (3 << 28)
> -#define PS_SCALER_MODE_DYN (0 << 28)
> -#define PS_SCALER_MODE_HQ (1 << 28)
> +#define SKL_PS_SCALER_MODE_MASK (3 << 28)
> +#define SKL_PS_SCALER_MODE_DYN (0 << 28)
> +#define SKL_PS_SCALER_MODE_HQ (1 << 28)
> #define SKL_PS_SCALER_MODE_NV12 (2 << 28)
> #define PS_SCALER_MODE_PLANAR (1 << 29)
> +#define PS_SCALER_MODE_PACKED (0 << 29)
> #define PS_PLANE_SEL_MASK (7 << 25)
> #define PS_PLANE_SEL(plane) (((plane) + 1) << 25)
> #define PS_FILTER_MASK (3 << 23)
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index b04952bacf77..c23a6ec40f60 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -203,6 +203,63 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
> drm_atomic_helper_crtc_destroy_state(crtc, state);
> }
>
> +static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
> + int num_scalers_need, struct intel_crtc *intel_crtc,
> + const char *name, int idx,
> + struct intel_plane_state *plane_state,
> + int *scaler_id)
> +{
> + struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> + int j;
> + u32 mode;
> +
> + if (*scaler_id < 0) {
> + /* find a free scaler */
> + for (j = 0; j < intel_crtc->num_scalers; j++) {
> + if (scaler_state->scalers[j].in_use)
> + continue;
> +
> + *scaler_id = j;
> + scaler_state->scalers[*scaler_id].in_use = 1;
> + break;
> + }
> + }
> +
> + if (WARN(*scaler_id < 0, "Cannot find scaler for %s:%d\n", name, idx))
> + return;
> +
> + /* set scaler mode */
> + if (plane_state && plane_state->base.fb &&
> + plane_state->base.fb->format->is_yuv &&
> + plane_state->base.fb->format->num_planes > 1) {
> + if (INTEL_GEN(dev_priv) == 9 &&
> + !IS_GEMINILAKE(dev_priv))
> + mode = SKL_PS_SCALER_MODE_NV12;
> + else
> + mode = PS_SCALER_MODE_PLANAR;
> +
> + } else if (INTEL_GEN(dev_priv) > 9 || IS_GEMINILAKE(dev_priv)) {
> + mode = PS_SCALER_MODE_PACKED;
> + } else if (num_scalers_need == 1 && intel_crtc->num_scalers > 1) {
> + /*
> + * when only 1 scaler is in use on a pipe with 2 scalers
> + * scaler 0 operates in high quality (HQ) mode.
> + * In this case use scaler 0 to take advantage of HQ mode
> + */
> + scaler_state->scalers[*scaler_id].in_use = 0;
> + *scaler_id = 0;
> + scaler_state->scalers[0].in_use = 1;
> + mode = SKL_PS_SCALER_MODE_HQ;
> + } else {
> + mode = SKL_PS_SCALER_MODE_DYN;
> + }
> +
> + DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
> + intel_crtc->pipe, *scaler_id, name, idx);
> + scaler_state->scalers[*scaler_id].mode = mode;
> +}
> +
> +
> /**
> * intel_atomic_setup_scalers() - setup scalers for crtc per staged requests
> * @dev_priv: i915 device
> @@ -232,7 +289,7 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
> struct drm_atomic_state *drm_state = crtc_state->base.state;
> struct intel_atomic_state *intel_state = to_intel_atomic_state(drm_state);
> int num_scalers_need;
> - int i, j;
> + int i;
>
> num_scalers_need = hweight32(scaler_state->scaler_users);
>
> @@ -304,59 +361,17 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
> idx = plane->base.id;
>
> /* plane on different crtc cannot be a scaler user of this crtc */
> - if (WARN_ON(intel_plane->pipe != intel_crtc->pipe)) {
> + if (WARN_ON(intel_plane->pipe != intel_crtc->pipe))
> continue;
> - }
>
> plane_state = intel_atomic_get_new_plane_state(intel_state,
> intel_plane);
> scaler_id = &plane_state->scaler_id;
> }
>
> - if (*scaler_id < 0) {
> - /* find a free scaler */
> - for (j = 0; j < intel_crtc->num_scalers; j++) {
> - if (!scaler_state->scalers[j].in_use) {
> - scaler_state->scalers[j].in_use = 1;
> - *scaler_id = j;
> - DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
> - intel_crtc->pipe, *scaler_id, name, idx);
> - break;
> - }
> - }
> - }
> -
> - if (WARN_ON(*scaler_id < 0)) {
> - DRM_DEBUG_KMS("Cannot find scaler for %s:%d\n", name, idx);
> - continue;
> - }
> -
> - /* set scaler mode */
> - if ((INTEL_GEN(dev_priv) >= 9) &&
> - plane_state && plane_state->base.fb &&
> - plane_state->base.fb->format->format ==
> - DRM_FORMAT_NV12) {
> - if (INTEL_GEN(dev_priv) == 9 &&
> - !IS_GEMINILAKE(dev_priv) &&
> - !IS_SKYLAKE(dev_priv))
> - scaler_state->scalers[*scaler_id].mode =
> - SKL_PS_SCALER_MODE_NV12;
> - else
> - scaler_state->scalers[*scaler_id].mode =
> - PS_SCALER_MODE_PLANAR;
> - } else if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) {
> - /*
> - * when only 1 scaler is in use on either pipe A or B,
> - * scaler 0 operates in high quality (HQ) mode.
> - * In this case use scaler 0 to take advantage of HQ mode
> - */
> - *scaler_id = 0;
> - scaler_state->scalers[0].in_use = 1;
> - scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
> - scaler_state->scalers[1].in_use = 0;
> - } else {
> - scaler_state->scalers[*scaler_id].mode = PS_SCALER_MODE_DYN;
> - }
> + intel_atomic_setup_scaler(scaler_state, num_scalers_need,
> + intel_crtc, name, idx,
> + plane_state, scaler_id);
> }
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 02374185b993..931898013506 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13963,7 +13963,7 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
> struct intel_scaler *scaler = &scaler_state->scalers[i];
>
> scaler->in_use = 0;
> - scaler->mode = PS_SCALER_MODE_DYN;
> + scaler->mode = 0;
> }
>
> scaler_state->scaler_id = -1;
> --
> 2.18.0
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] drm/i915: Clean up scaler setup, v2.
2018-09-21 16:40 ` Matt Roper
@ 2018-09-21 17:32 ` Maarten Lankhorst
0 siblings, 0 replies; 32+ messages in thread
From: Maarten Lankhorst @ 2018-09-21 17:32 UTC (permalink / raw)
To: Matt Roper; +Cc: intel-gfx
Op 21-09-18 om 18:40 schreef Matt Roper:
> On Fri, Sep 21, 2018 at 04:44:37PM +0200, Maarten Lankhorst wrote:
>> On skylake we can switch to a high quality scaler mode when only 1 out
>> of 2 scalers are used, but on GLK and later bit 28 has a different
>> meaning. Don't set it, and make clear the distinction between
>> SKL and later PS values.
>>
>> Changes since v1:
>> - Add missing break statement.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> #v1
> R-b for v2 as well.
>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 7 +-
>> drivers/gpu/drm/i915/intel_atomic.c | 109 +++++++++++++++------------
>> drivers/gpu/drm/i915/intel_display.c | 2 +-
>> 3 files changed, 67 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 4948b352bf4c..e7e6ca7f9665 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6850,11 +6850,12 @@ enum {
>> #define _PS_2B_CTRL 0x68A80
>> #define _PS_1C_CTRL 0x69180
>> #define PS_SCALER_EN (1 << 31)
>> -#define PS_SCALER_MODE_MASK (3 << 28)
>> -#define PS_SCALER_MODE_DYN (0 << 28)
>> -#define PS_SCALER_MODE_HQ (1 << 28)
>> +#define SKL_PS_SCALER_MODE_MASK (3 << 28)
>> +#define SKL_PS_SCALER_MODE_DYN (0 << 28)
>> +#define SKL_PS_SCALER_MODE_HQ (1 << 28)
>> #define SKL_PS_SCALER_MODE_NV12 (2 << 28)
>> #define PS_SCALER_MODE_PLANAR (1 << 29)
>> +#define PS_SCALER_MODE_PACKED (0 << 29)
>> #define PS_PLANE_SEL_MASK (7 << 25)
>> #define PS_PLANE_SEL(plane) (((plane) + 1) << 25)
>> #define PS_FILTER_MASK (3 << 23)
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index b04952bacf77..c23a6ec40f60 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -203,6 +203,63 @@ intel_crtc_destroy_state(struct drm_crtc *crtc,
>> drm_atomic_helper_crtc_destroy_state(crtc, state);
>> }
>>
>> +static void intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_state,
>> + int num_scalers_need, struct intel_crtc *intel_crtc,
>> + const char *name, int idx,
>> + struct intel_plane_state *plane_state,
>> + int *scaler_id)
>> +{
>> + struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
>> + int j;
>> + u32 mode;
>> +
>> + if (*scaler_id < 0) {
>> + /* find a free scaler */
>> + for (j = 0; j < intel_crtc->num_scalers; j++) {
>> + if (scaler_state->scalers[j].in_use)
>> + continue;
>> +
>> + *scaler_id = j;
>> + scaler_state->scalers[*scaler_id].in_use = 1;
>> + break;
>> + }
>> + }
>> +
>> + if (WARN(*scaler_id < 0, "Cannot find scaler for %s:%d\n", name, idx))
>> + return;
>> +
>> + /* set scaler mode */
>> + if (plane_state && plane_state->base.fb &&
>> + plane_state->base.fb->format->is_yuv &&
>> + plane_state->base.fb->format->num_planes > 1) {
>> + if (INTEL_GEN(dev_priv) == 9 &&
>> + !IS_GEMINILAKE(dev_priv))
>> + mode = SKL_PS_SCALER_MODE_NV12;
>> + else
>> + mode = PS_SCALER_MODE_PLANAR;
>> +
>> + } else if (INTEL_GEN(dev_priv) > 9 || IS_GEMINILAKE(dev_priv)) {
>> + mode = PS_SCALER_MODE_PACKED;
>> + } else if (num_scalers_need == 1 && intel_crtc->num_scalers > 1) {
>> + /*
>> + * when only 1 scaler is in use on a pipe with 2 scalers
>> + * scaler 0 operates in high quality (HQ) mode.
>> + * In this case use scaler 0 to take advantage of HQ mode
>> + */
>> + scaler_state->scalers[*scaler_id].in_use = 0;
>> + *scaler_id = 0;
>> + scaler_state->scalers[0].in_use = 1;
>> + mode = SKL_PS_SCALER_MODE_HQ;
>> + } else {
>> + mode = SKL_PS_SCALER_MODE_DYN;
>> + }
>> +
>> + DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
>> + intel_crtc->pipe, *scaler_id, name, idx);
>> + scaler_state->scalers[*scaler_id].mode = mode;
>> +}
>> +
>> +
>> /**
>> * intel_atomic_setup_scalers() - setup scalers for crtc per staged requests
>> * @dev_priv: i915 device
>> @@ -232,7 +289,7 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
>> struct drm_atomic_state *drm_state = crtc_state->base.state;
>> struct intel_atomic_state *intel_state = to_intel_atomic_state(drm_state);
>> int num_scalers_need;
>> - int i, j;
>> + int i;
>>
>> num_scalers_need = hweight32(scaler_state->scaler_users);
>>
>> @@ -304,59 +361,17 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv,
>> idx = plane->base.id;
>>
>> /* plane on different crtc cannot be a scaler user of this crtc */
>> - if (WARN_ON(intel_plane->pipe != intel_crtc->pipe)) {
>> + if (WARN_ON(intel_plane->pipe != intel_crtc->pipe))
>> continue;
>> - }
>>
>> plane_state = intel_atomic_get_new_plane_state(intel_state,
>> intel_plane);
>> scaler_id = &plane_state->scaler_id;
>> }
>>
>> - if (*scaler_id < 0) {
>> - /* find a free scaler */
>> - for (j = 0; j < intel_crtc->num_scalers; j++) {
>> - if (!scaler_state->scalers[j].in_use) {
>> - scaler_state->scalers[j].in_use = 1;
>> - *scaler_id = j;
>> - DRM_DEBUG_KMS("Attached scaler id %u.%u to %s:%d\n",
>> - intel_crtc->pipe, *scaler_id, name, idx);
>> - break;
>> - }
>> - }
>> - }
>> -
>> - if (WARN_ON(*scaler_id < 0)) {
>> - DRM_DEBUG_KMS("Cannot find scaler for %s:%d\n", name, idx);
>> - continue;
>> - }
>> -
>> - /* set scaler mode */
>> - if ((INTEL_GEN(dev_priv) >= 9) &&
>> - plane_state && plane_state->base.fb &&
>> - plane_state->base.fb->format->format ==
>> - DRM_FORMAT_NV12) {
>> - if (INTEL_GEN(dev_priv) == 9 &&
>> - !IS_GEMINILAKE(dev_priv) &&
>> - !IS_SKYLAKE(dev_priv))
>> - scaler_state->scalers[*scaler_id].mode =
>> - SKL_PS_SCALER_MODE_NV12;
>> - else
>> - scaler_state->scalers[*scaler_id].mode =
>> - PS_SCALER_MODE_PLANAR;
>> - } else if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) {
>> - /*
>> - * when only 1 scaler is in use on either pipe A or B,
>> - * scaler 0 operates in high quality (HQ) mode.
>> - * In this case use scaler 0 to take advantage of HQ mode
>> - */
>> - *scaler_id = 0;
>> - scaler_state->scalers[0].in_use = 1;
>> - scaler_state->scalers[0].mode = PS_SCALER_MODE_HQ;
>> - scaler_state->scalers[1].in_use = 0;
>> - } else {
>> - scaler_state->scalers[*scaler_id].mode = PS_SCALER_MODE_DYN;
>> - }
>> + intel_atomic_setup_scaler(scaler_state, num_scalers_need,
>> + intel_crtc, name, idx,
>> + plane_state, scaler_id);
>> }
>>
>> return 0;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 02374185b993..931898013506 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13963,7 +13963,7 @@ static void intel_crtc_init_scalers(struct intel_crtc *crtc,
>> struct intel_scaler *scaler = &scaler_state->scalers[i];
>>
>> scaler->in_use = 0;
>> - scaler->mode = PS_SCALER_MODE_DYN;
>> + scaler->mode = 0;
>> }
>>
>> scaler_state->scaler_id = -1;
>> --
>> 2.18.0
>>
Thanks, pushed the series. :)
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2018-09-21 17:32 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-20 10:27 [PATCH 0/8] drm/i915: Preparations for adding gen11 planar formats Maarten Lankhorst
2018-09-20 10:27 ` [PATCH 1/8] drm/i915: Clean up casts to crtc_state in intel_atomic_commit_tail() Maarten Lankhorst
2018-09-20 10:27 ` [PATCH 2/8] drm/i915: Handle cursor updating active_planes correctly, v2 Maarten Lankhorst
2018-09-20 23:18 ` Matt Roper
2018-09-21 9:41 ` Maarten Lankhorst
2018-09-20 10:27 ` [PATCH 3/8] drm/i915: Unconditionally clear plane visibility, v2 Maarten Lankhorst
2018-09-20 23:18 ` Matt Roper
2018-09-21 15:26 ` Ville Syrjälä
2018-09-21 16:00 ` Maarten Lankhorst
2018-09-21 16:15 ` Ville Syrjälä
2018-09-21 16:20 ` Maarten Lankhorst
2018-09-21 16:24 ` Ville Syrjälä
2018-09-20 10:27 ` [PATCH 4/8] drm/i915: Make intel_crtc_disable_planes() use active planes mask Maarten Lankhorst
2018-09-20 10:27 ` [PATCH 5/8] drm/i915: Replace call to commit_planes_on_crtc with internal update, v2 Maarten Lankhorst
2018-09-20 10:27 ` [PATCH 6/8] drm/i915: Clean up scaler setup Maarten Lankhorst
2018-09-20 23:19 ` Matt Roper
2018-09-21 14:44 ` [PATCH] drm/i915: Clean up scaler setup, v2 Maarten Lankhorst
2018-09-21 16:40 ` Matt Roper
2018-09-21 17:32 ` Maarten Lankhorst
2018-09-20 10:27 ` [PATCH 7/8] drm/i915: Move programming plane scaler to its own function Maarten Lankhorst
2018-09-20 23:19 ` Matt Roper
2018-09-21 15:29 ` Ville Syrjälä
2018-09-20 10:27 ` [PATCH 8/8] drm/i915: Force planar YUV coordinates to be a multiple of 2, v2 Maarten Lankhorst
2018-09-20 23:19 ` Matt Roper
2018-09-20 10:49 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Preparations for adding gen11 planar formats Patchwork
2018-09-20 10:52 ` ✗ Fi.CI.SPARSE: " Patchwork
2018-09-20 11:14 ` ✓ Fi.CI.BAT: success " Patchwork
2018-09-20 12:10 ` ✗ Fi.CI.IGT: failure " Patchwork
2018-09-21 15:00 ` ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Preparations for adding gen11 planar formats. (rev2) Patchwork
2018-09-21 15:03 ` ✗ Fi.CI.SPARSE: " Patchwork
2018-09-21 15:21 ` ✓ Fi.CI.BAT: success " Patchwork
2018-09-21 16:23 ` ✓ Fi.CI.IGT: " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.