* [PATCH] drm/i915: Treat cursor plane as another sprite plane for BSW
@ 2016-03-01 23:27 Dhinakaran Pandiyan
2016-03-02 7:55 ` ✗ Fi.CI.BAT: warning for " Patchwork
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Dhinakaran Pandiyan @ 2016-03-01 23:27 UTC (permalink / raw)
To: intel-gfx; +Cc: Kalyan Kondapally, Pandiyan Dhinakaran
From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Work around the CHV pipe C FIFO underruns that cause display failure by
enabling sprite plane for cursor.
This patch for BSW is based on Maarten Lankhorst's work that
enables universal plane support.
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com>
Signed-off-by: Pandiyan Dhinakaran <dhinakaran.pandiyan@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++++----------
drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++--
drivers/gpu/drm/i915/intel_pm.c | 51 +++++++++++++++++++++++++++++-------
drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++---------
4 files changed, 100 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 44fcff0..4a392d4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11941,9 +11941,14 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
bool is_crtc_enabled = crtc_state->active;
bool turn_off, turn_on, visible, was_visible;
struct drm_framebuffer *fb = plane_state->fb;
+ enum drm_plane_type plane_type = plane->type;
+
+ if (plane_type == DRM_PLANE_TYPE_CURSOR &&
+ !pipe_has_cursor_plane(to_i915(dev), intel_crtc->pipe))
+ plane_type = DRM_PLANE_TYPE_OVERLAY;
if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
- plane->type != DRM_PLANE_TYPE_CURSOR) {
+ plane_type != DRM_PLANE_TYPE_CURSOR) {
ret = skl_update_scaler_plane(
to_intel_crtc_state(crtc_state),
to_intel_plane_state(plane_state));
@@ -14472,7 +14477,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
struct intel_crtc_state *crtc_state = NULL;
struct drm_plane *primary = NULL;
struct drm_plane *cursor = NULL;
- int i, ret;
+ int i, ret, sprite;
intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
if (intel_crtc == NULL)
@@ -14499,9 +14504,32 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
if (!primary)
goto fail;
- cursor = intel_cursor_plane_create(dev, pipe);
- if (!cursor)
- goto fail;
+ if (pipe_has_cursor_plane(dev_priv, pipe)) {
+ cursor = intel_cursor_plane_create(dev, pipe);
+ if (!cursor)
+ goto fail;
+ }
+
+ for_each_sprite(dev_priv, pipe, sprite) {
+ enum drm_plane_type plane_type;
+ struct drm_plane *plane;
+
+ if (sprite + 1 < INTEL_INFO(dev_priv)->num_sprites[pipe] ||
+ pipe_has_cursor_plane(dev_priv, pipe))
+ plane_type = DRM_PLANE_TYPE_OVERLAY;
+ else
+ plane_type = DRM_PLANE_TYPE_CURSOR;
+
+ plane = intel_plane_init(dev, pipe, sprite, plane_type);
+ if (IS_ERR(plane)) {
+ DRM_DEBUG_KMS("pipe %c sprite %c init failed: %ld\n",
+ pipe_name(pipe), sprite_name(pipe, sprite), PTR_ERR(plane));
+ goto fail;
+ }
+
+ if (plane_type == DRM_PLANE_TYPE_CURSOR)
+ cursor = plane;
+ }
ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
cursor, &intel_crtc_funcs, NULL);
@@ -15534,7 +15562,6 @@ fail:
void intel_modeset_init(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- int sprite, ret;
enum pipe pipe;
struct intel_crtc *crtc;
@@ -15606,15 +15633,8 @@ void intel_modeset_init(struct drm_device *dev)
INTEL_INFO(dev)->num_pipes,
INTEL_INFO(dev)->num_pipes > 1 ? "s" : "");
- for_each_pipe(dev_priv, pipe) {
+ for_each_pipe(dev_priv, pipe)
intel_crtc_init(dev, pipe);
- for_each_sprite(dev_priv, pipe, sprite) {
- ret = intel_plane_init(dev, pipe, sprite);
- if (ret)
- DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
- pipe_name(pipe), sprite_name(pipe, sprite), ret);
- }
- }
intel_update_czclk(dev_priv);
intel_update_cdclk(dev);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cb413e2..218f8f8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -971,9 +971,18 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
return container_of(intel_hdmi, struct intel_digital_port, hdmi);
}
+static inline bool pipe_has_cursor_plane(struct drm_i915_private *dev_priv, int pipe)
+{
+ if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_C)
+ return false;
+
+ return true;
+}
+
/*
* Returns the number of planes for this pipe, ie the number of sprites + 1
- * (primary plane). This doesn't count the cursor plane then.
+ * (primary plane). This doesn't count the cursor plane except on CHV pipe C,
+ * which has a universal plane masked as cursor plane.
*/
static inline unsigned int intel_num_planes(struct intel_crtc *crtc)
{
@@ -1595,7 +1604,8 @@ bool intel_sdvo_init(struct drm_device *dev,
/* intel_sprite.c */
-int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
+struct drm_plane *intel_plane_init(struct drm_device *dev, enum pipe pipe,
+ int plane, enum drm_plane_type plane_type);
int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
struct drm_file *file_priv);
void intel_pipe_update_start(struct intel_crtc *crtc);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d33de95..0bab7d3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -941,7 +941,8 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
if (WARN_ON(htotal == 0))
htotal = 1;
- if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
+ if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
+ pipe_has_cursor_plane(dev_priv, plane->pipe)) {
/*
* FIXME the formula gives values that are
* too big for the cursor FIFO, and hence we
@@ -970,7 +971,8 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
struct intel_plane_state *state =
to_intel_plane_state(plane->base.state);
- if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
+ if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
+ pipe_has_cursor_plane(dev->dev_private, plane->pipe))
continue;
if (state->visible) {
@@ -984,7 +986,8 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
to_intel_plane_state(plane->base.state);
unsigned int rate;
- if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
+ if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
+ pipe_has_cursor_plane(dev->dev_private, plane->pipe)) {
plane->wm.fifo_size = 63;
continue;
}
@@ -1008,7 +1011,8 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
if (fifo_left == 0)
break;
- if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
+ if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
+ pipe_has_cursor_plane(dev->dev_private, plane->pipe))
continue;
/* give it all to the first plane if none are active */
@@ -1028,6 +1032,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
{
struct vlv_wm_state *wm_state = &crtc->wm_state;
int level;
+ enum drm_plane_type plane_type;
for (level = 0; level < wm_state->num_levels; level++) {
struct drm_device *dev = crtc->base.dev;
@@ -1038,7 +1043,13 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
wm_state->sr[level].cursor = 63 - wm_state->sr[level].cursor;
for_each_intel_plane_on_crtc(dev, crtc, plane) {
- switch (plane->base.type) {
+ plane_type = plane->base.type;
+
+ if (plane_type == DRM_PLANE_TYPE_CURSOR &&
+ !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
+ plane_type = DRM_PLANE_TYPE_OVERLAY;
+
+ switch (plane_type) {
int sprite;
case DRM_PLANE_TYPE_CURSOR:
wm_state->wm[level].cursor = plane->wm.fifo_size -
@@ -1065,6 +1076,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
struct intel_plane *plane;
int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
int level;
+ enum drm_plane_type plane_type;
memset(wm_state, 0, sizeof(*wm_state));
@@ -1092,10 +1104,15 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
if (!state->visible)
continue;
+ plane_type = plane->base.type;
+ if (plane_type == DRM_PLANE_TYPE_CURSOR &&
+ !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
+ plane_type = DRM_PLANE_TYPE_OVERLAY;
+
/* normal watermarks */
for (level = 0; level < wm_state->num_levels; level++) {
int wm = vlv_compute_wm_level(plane, crtc, state, level);
- int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
+ int max_wm = plane_type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
/* hack */
if (WARN_ON(level == 0 && wm > max_wm))
@@ -1104,7 +1121,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
if (wm > plane->wm.fifo_size)
break;
- switch (plane->base.type) {
+ switch (plane_type) {
int sprite;
case DRM_PLANE_TYPE_CURSOR:
wm_state->wm[level].cursor = wm;
@@ -1125,7 +1142,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
continue;
/* maxfifo watermarks */
- switch (plane->base.type) {
+ switch (plane_type) {
int sprite, level;
case DRM_PLANE_TYPE_CURSOR:
for (level = 0; level < wm_state->num_levels; level++)
@@ -1166,9 +1183,16 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
struct drm_i915_private *dev_priv = to_i915(dev);
struct intel_plane *plane;
int sprite0_start = 0, sprite1_start = 0, fifo_size = 0;
+ enum drm_plane_type plane_type;
for_each_intel_plane_on_crtc(dev, crtc, plane) {
- if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
+ plane_type = plane->base.type;
+
+ if (plane_type == DRM_PLANE_TYPE_CURSOR &&
+ !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
+ plane_type = DRM_PLANE_TYPE_OVERLAY;
+
+ if (plane_type == DRM_PLANE_TYPE_CURSOR) {
WARN_ON(plane->wm.fifo_size != 63);
continue;
}
@@ -3996,11 +4020,18 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
struct intel_plane *plane;
enum pipe pipe;
u32 val;
+ enum drm_plane_type plane_type;
vlv_read_wm_values(dev_priv, wm);
for_each_intel_plane(dev, plane) {
- switch (plane->base.type) {
+ plane_type = plane->base.type;
+
+ if (plane_type == DRM_PLANE_TYPE_CURSOR &&
+ !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
+ plane_type = DRM_PLANE_TYPE_OVERLAY;
+
+ switch (plane_type) {
int sprite;
case DRM_PLANE_TYPE_CURSOR:
plane->wm.fifo_size = 63;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 8821533..0b5104c 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1022,8 +1022,8 @@ static uint32_t skl_plane_formats[] = {
DRM_FORMAT_VYUY,
};
-int
-intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
+struct drm_plane *intel_plane_init(struct drm_device *dev, enum pipe pipe,
+ int plane, enum drm_plane_type plane_type)
{
struct intel_plane *intel_plane;
struct intel_plane_state *state;
@@ -1033,16 +1033,16 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
int ret;
if (INTEL_INFO(dev)->gen < 5)
- return -ENODEV;
+ return ERR_PTR(-ENODEV);
intel_plane = kzalloc(sizeof(*intel_plane), GFP_KERNEL);
if (!intel_plane)
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
state = intel_create_plane_state(&intel_plane->base);
if (!state) {
kfree(intel_plane);
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
}
intel_plane->base.state = &state->base;
@@ -1097,8 +1097,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
num_plane_formats = ARRAY_SIZE(skl_plane_formats);
break;
default:
- kfree(intel_plane);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out;
}
intel_plane->pipe = pipe;
@@ -1109,16 +1109,17 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
&intel_plane_funcs,
plane_formats, num_plane_formats,
- DRM_PLANE_TYPE_OVERLAY, NULL);
- if (ret) {
- kfree(intel_plane);
+ plane_type, NULL);
+ if (ret)
goto out;
- }
intel_create_rotation_property(dev, intel_plane);
drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
+ return &intel_plane->base;
out:
- return ret;
+ kfree(intel_plane);
+ kfree(state);
+ return ERR_PTR(ret);
}
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915: Treat cursor plane as another sprite plane for BSW
2016-03-01 23:27 [PATCH] drm/i915: Treat cursor plane as another sprite plane for BSW Dhinakaran Pandiyan
@ 2016-03-02 7:55 ` Patchwork
2016-03-02 12:55 ` [PATCH] " Maarten Lankhorst
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2016-03-02 7:55 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Treat cursor plane as another sprite plane for BSW
URL : https://patchwork.freedesktop.org/series/3996/
State : warning
== Summary ==
Series 3996v1 drm/i915: Treat cursor plane as another sprite plane for BSW
http://patchwork.freedesktop.org/api/1.0/series/3996/revisions/1/mbox/
Test gem_mmap_gtt:
Subgroup basic-copy:
pass -> DMESG-WARN (byt-nuc)
Test kms_flip:
Subgroup basic-flip-vs-dpms:
pass -> DMESG-WARN (ilk-hp8440p) UNSTABLE
Subgroup basic-flip-vs-modeset:
pass -> DMESG-WARN (hsw-brixbox)
dmesg-warn -> PASS (ilk-hp8440p) UNSTABLE
Subgroup basic-flip-vs-wf_vblank:
fail -> PASS (snb-x220t)
pass -> DMESG-WARN (hsw-brixbox)
Test kms_pipe_crc_basic:
Subgroup read-crc-pipe-b-frame-sequence:
pass -> DMESG-WARN (snb-x220t)
Test pm_rpm:
Subgroup basic-pci-d3-state:
dmesg-fail -> FAIL (snb-x220t)
pass -> DMESG-WARN (snb-dellxps)
pass -> DMESG-WARN (bsw-nuc-2)
bdw-nuci7 total:169 pass:158 dwarn:0 dfail:0 fail:0 skip:11
bdw-ultra total:169 pass:155 dwarn:0 dfail:0 fail:0 skip:14
bsw-nuc-2 total:169 pass:136 dwarn:2 dfail:0 fail:1 skip:30
byt-nuc total:169 pass:143 dwarn:1 dfail:0 fail:0 skip:25
hsw-brixbox total:169 pass:152 dwarn:2 dfail:0 fail:0 skip:15
hsw-gt2 total:169 pass:157 dwarn:1 dfail:0 fail:1 skip:10
ilk-hp8440p total:169 pass:117 dwarn:1 dfail:0 fail:1 skip:50
ivb-t430s total:169 pass:153 dwarn:0 dfail:0 fail:1 skip:15
skl-i5k-2 total:169 pass:153 dwarn:0 dfail:0 fail:0 skip:16
skl-i7k-2 total:169 pass:153 dwarn:0 dfail:0 fail:0 skip:16
snb-dellxps total:169 pass:143 dwarn:2 dfail:0 fail:1 skip:23
snb-x220t total:169 pass:144 dwarn:1 dfail:0 fail:2 skip:22
Results at /archive/results/CI_IGT_test/Patchwork_1509/
f9cadb616ff17d482312fba07db772b6604ce799 drm-intel-nightly: 2016y-03m-01d-17h-16m-32s UTC integration manifest
afc98604ca544a2d6529c332183457af26f95c14 drm/i915: Treat cursor plane as another sprite plane for BSW
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Treat cursor plane as another sprite plane for BSW
2016-03-01 23:27 [PATCH] drm/i915: Treat cursor plane as another sprite plane for BSW Dhinakaran Pandiyan
2016-03-02 7:55 ` ✗ Fi.CI.BAT: warning for " Patchwork
@ 2016-03-02 12:55 ` Maarten Lankhorst
2016-03-02 14:03 ` Ville Syrjälä
2016-03-23 13:26 ` Maarten Lankhorst
3 siblings, 0 replies; 6+ messages in thread
From: Maarten Lankhorst @ 2016-03-02 12:55 UTC (permalink / raw)
To: Dhinakaran Pandiyan, intel-gfx; +Cc: Kalyan Kondapally
Hey,
Op 02-03-16 om 00:27 schreef Dhinakaran Pandiyan:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> Work around the CHV pipe C FIFO underruns that cause display failure by
> enabling sprite plane for cursor.
>
> This patch for BSW is based on Maarten Lankhorst's work that
> enables universal plane support.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com>
> Signed-off-by: Pandiyan Dhinakaran <dhinakaran.pandiyan@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++++----------
> drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++--
> drivers/gpu/drm/i915/intel_pm.c | 51 +++++++++++++++++++++++++++++-------
> drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++---------
> 4 files changed, 100 insertions(+), 38 deletions(-)
Looks like CI is not unhappy with this patch, not sure I can review it since I'm the original author. :-)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 44fcff0..4a392d4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11941,9 +11941,14 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> bool is_crtc_enabled = crtc_state->active;
> bool turn_off, turn_on, visible, was_visible;
> struct drm_framebuffer *fb = plane_state->fb;
> + enum drm_plane_type plane_type = plane->type;
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), intel_crtc->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
>
> if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
> - plane->type != DRM_PLANE_TYPE_CURSOR) {
> + plane_type != DRM_PLANE_TYPE_CURSOR) {
> ret = skl_update_scaler_plane(
> to_intel_crtc_state(crtc_state),
> to_intel_plane_state(plane_state));
^Missing some plane->type -> plane_type in this function.
> <snip>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cb413e2..218f8f8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -971,9 +971,18 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
> return container_of(intel_hdmi, struct intel_digital_port, hdmi);
> }
>
> +static inline bool pipe_has_cursor_plane(struct drm_i915_private *dev_priv, int pipe)
> +{
> + if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_C)
> + return false;
> +
> + return true;
> +}
> +
^Might be worth adding a comment why this is the case.
> /*
> * Returns the number of planes for this pipe, ie the number of sprites + 1
> - * (primary plane). This doesn't count the cursor plane then.
> + * (primary plane). This doesn't count the cursor plane except on CHV pipe C,
> + * which has a universal plane masked as cursor plane.
> */
> static inline unsigned int intel_num_planes(struct intel_crtc *crtc)
> {
> @@ -1595,7 +1604,8 @@ bool intel_sdvo_init(struct drm_device *dev,
>
>
> /* intel_sprite.c */
> -int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
> +struct drm_plane *intel_plane_init(struct drm_device *dev, enum pipe pipe,
> + int plane, enum drm_plane_type plane_type);
> int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> void intel_pipe_update_start(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d33de95..0bab7d3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -941,7 +941,8 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
> if (WARN_ON(htotal == 0))
> htotal = 1;
>
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> + if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
> + pipe_has_cursor_plane(dev_priv, plane->pipe)) {
> /*
> * FIXME the formula gives values that are
> * too big for the cursor FIFO, and hence we
> @@ -970,7 +971,8 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
> struct intel_plane_state *state =
> to_intel_plane_state(plane->base.state);
>
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> + if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
> + pipe_has_cursor_plane(dev->dev_private, plane->pipe))
> continue;
>
> if (state->visible) {
> @@ -984,7 +986,8 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
> to_intel_plane_state(plane->base.state);
> unsigned int rate;
>
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> + if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
> + pipe_has_cursor_plane(dev->dev_private, plane->pipe)) {
> plane->wm.fifo_size = 63;
> continue;
> }
> @@ -1008,7 +1011,8 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
> if (fifo_left == 0)
> break;
>
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> + if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
> + pipe_has_cursor_plane(dev->dev_private, plane->pipe))
> continue;
>
> /* give it all to the first plane if none are active */
> @@ -1028,6 +1032,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
> {
> struct vlv_wm_state *wm_state = &crtc->wm_state;
> int level;
> + enum drm_plane_type plane_type;
>
> for (level = 0; level < wm_state->num_levels; level++) {
> struct drm_device *dev = crtc->base.dev;
> @@ -1038,7 +1043,13 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
> wm_state->sr[level].cursor = 63 - wm_state->sr[level].cursor;
>
> for_each_intel_plane_on_crtc(dev, crtc, plane) {
> - switch (plane->base.type) {
> + plane_type = plane->base.type;
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> +
> + switch (plane_type) {
> int sprite;
> case DRM_PLANE_TYPE_CURSOR:
> wm_state->wm[level].cursor = plane->wm.fifo_size -
> @@ -1065,6 +1076,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> struct intel_plane *plane;
> int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
> int level;
> + enum drm_plane_type plane_type;
>
> memset(wm_state, 0, sizeof(*wm_state));
>
> @@ -1092,10 +1104,15 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> if (!state->visible)
> continue;
>
> + plane_type = plane->base.type;
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> +
> /* normal watermarks */
> for (level = 0; level < wm_state->num_levels; level++) {
> int wm = vlv_compute_wm_level(plane, crtc, state, level);
> - int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
> + int max_wm = plane_type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
>
> /* hack */
> if (WARN_ON(level == 0 && wm > max_wm))
> @@ -1104,7 +1121,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> if (wm > plane->wm.fifo_size)
> break;
>
> - switch (plane->base.type) {
> + switch (plane_type) {
> int sprite;
> case DRM_PLANE_TYPE_CURSOR:
> wm_state->wm[level].cursor = wm;
> @@ -1125,7 +1142,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> continue;
>
> /* maxfifo watermarks */
> - switch (plane->base.type) {
> + switch (plane_type) {
> int sprite, level;
> case DRM_PLANE_TYPE_CURSOR:
> for (level = 0; level < wm_state->num_levels; level++)
> @@ -1166,9 +1183,16 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_plane *plane;
> int sprite0_start = 0, sprite1_start = 0, fifo_size = 0;
> + enum drm_plane_type plane_type;
>
> for_each_intel_plane_on_crtc(dev, crtc, plane) {
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> + plane_type = plane->base.type;
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
if (plane->type == CURSOR && pipe_has_cursor_plane) { WARN_ON(..); continue; } ? Nothing else needs plane_type here.
I can't comment on the watermark changes, but if CI is happy so am I.
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Treat cursor plane as another sprite plane for BSW
2016-03-01 23:27 [PATCH] drm/i915: Treat cursor plane as another sprite plane for BSW Dhinakaran Pandiyan
2016-03-02 7:55 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-03-02 12:55 ` [PATCH] " Maarten Lankhorst
@ 2016-03-02 14:03 ` Ville Syrjälä
2016-03-04 1:43 ` Pandiyan, Dhinakaran
2016-03-23 13:26 ` Maarten Lankhorst
3 siblings, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2016-03-02 14:03 UTC (permalink / raw)
To: Dhinakaran Pandiyan; +Cc: Kalyan Kondapally, intel-gfx
On Tue, Mar 01, 2016 at 03:27:17PM -0800, Dhinakaran Pandiyan wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> Work around the CHV pipe C FIFO underruns that cause display failure by
> enabling sprite plane for cursor.
>
> This patch for BSW is based on Maarten Lankhorst's work that
> enables universal plane support.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com>
> Signed-off-by: Pandiyan Dhinakaran <dhinakaran.pandiyan@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++++----------
> drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++--
> drivers/gpu/drm/i915/intel_pm.c | 51 +++++++++++++++++++++++++++++-------
> drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++---------
> 4 files changed, 100 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 44fcff0..4a392d4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11941,9 +11941,14 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> bool is_crtc_enabled = crtc_state->active;
> bool turn_off, turn_on, visible, was_visible;
> struct drm_framebuffer *fb = plane_state->fb;
> + enum drm_plane_type plane_type = plane->type;
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), intel_crtc->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
I think spreading this stuff all over is too messy. I would suggest adding some
kind of hw plane type to intel_plane instead.
>
> if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
> - plane->type != DRM_PLANE_TYPE_CURSOR) {
> + plane_type != DRM_PLANE_TYPE_CURSOR) {
> ret = skl_update_scaler_plane(
> to_intel_crtc_state(crtc_state),
> to_intel_plane_state(plane_state));
> @@ -14472,7 +14477,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> struct intel_crtc_state *crtc_state = NULL;
> struct drm_plane *primary = NULL;
> struct drm_plane *cursor = NULL;
> - int i, ret;
> + int i, ret, sprite;
>
> intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> if (intel_crtc == NULL)
> @@ -14499,9 +14504,32 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> if (!primary)
> goto fail;
>
> - cursor = intel_cursor_plane_create(dev, pipe);
> - if (!cursor)
> - goto fail;
> + if (pipe_has_cursor_plane(dev_priv, pipe)) {
> + cursor = intel_cursor_plane_create(dev, pipe);
> + if (!cursor)
> + goto fail;
> + }
> +
> + for_each_sprite(dev_priv, pipe, sprite) {
> + enum drm_plane_type plane_type;
> + struct drm_plane *plane;
> +
> + if (sprite + 1 < INTEL_INFO(dev_priv)->num_sprites[pipe] ||
> + pipe_has_cursor_plane(dev_priv, pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> + else
> + plane_type = DRM_PLANE_TYPE_CURSOR;
I'm thinking I might put this logic into the sprite init code.
Apart from these the main concern I have with all this is watermarks, cxsr,
vblank waits etc. I can't see anything here to explain how it's going to
actually work so I suspect there will be problems.
> +
> + plane = intel_plane_init(dev, pipe, sprite, plane_type);
> + if (IS_ERR(plane)) {
> + DRM_DEBUG_KMS("pipe %c sprite %c init failed: %ld\n",
> + pipe_name(pipe), sprite_name(pipe, sprite), PTR_ERR(plane));
> + goto fail;
> + }
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR)
> + cursor = plane;
> + }
>
> ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> cursor, &intel_crtc_funcs, NULL);
> @@ -15534,7 +15562,6 @@ fail:
> void intel_modeset_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - int sprite, ret;
> enum pipe pipe;
> struct intel_crtc *crtc;
>
> @@ -15606,15 +15633,8 @@ void intel_modeset_init(struct drm_device *dev)
> INTEL_INFO(dev)->num_pipes,
> INTEL_INFO(dev)->num_pipes > 1 ? "s" : "");
>
> - for_each_pipe(dev_priv, pipe) {
> + for_each_pipe(dev_priv, pipe)
> intel_crtc_init(dev, pipe);
> - for_each_sprite(dev_priv, pipe, sprite) {
> - ret = intel_plane_init(dev, pipe, sprite);
> - if (ret)
> - DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
> - pipe_name(pipe), sprite_name(pipe, sprite), ret);
> - }
> - }
>
> intel_update_czclk(dev_priv);
> intel_update_cdclk(dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cb413e2..218f8f8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -971,9 +971,18 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
> return container_of(intel_hdmi, struct intel_digital_port, hdmi);
> }
>
> +static inline bool pipe_has_cursor_plane(struct drm_i915_private *dev_priv, int pipe)
> +{
> + if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_C)
> + return false;
> +
> + return true;
> +}
> +
> /*
> * Returns the number of planes for this pipe, ie the number of sprites + 1
> - * (primary plane). This doesn't count the cursor plane then.
> + * (primary plane). This doesn't count the cursor plane except on CHV pipe C,
> + * which has a universal plane masked as cursor plane.
> */
> static inline unsigned int intel_num_planes(struct intel_crtc *crtc)
> {
> @@ -1595,7 +1604,8 @@ bool intel_sdvo_init(struct drm_device *dev,
>
>
> /* intel_sprite.c */
> -int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
> +struct drm_plane *intel_plane_init(struct drm_device *dev, enum pipe pipe,
> + int plane, enum drm_plane_type plane_type);
> int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> void intel_pipe_update_start(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d33de95..0bab7d3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -941,7 +941,8 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
> if (WARN_ON(htotal == 0))
> htotal = 1;
>
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> + if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
> + pipe_has_cursor_plane(dev_priv, plane->pipe)) {
> /*
> * FIXME the formula gives values that are
> * too big for the cursor FIFO, and hence we
> @@ -970,7 +971,8 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
> struct intel_plane_state *state =
> to_intel_plane_state(plane->base.state);
>
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> + if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
> + pipe_has_cursor_plane(dev->dev_private, plane->pipe))
> continue;
>
> if (state->visible) {
> @@ -984,7 +986,8 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
> to_intel_plane_state(plane->base.state);
> unsigned int rate;
>
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> + if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
> + pipe_has_cursor_plane(dev->dev_private, plane->pipe)) {
> plane->wm.fifo_size = 63;
> continue;
> }
> @@ -1008,7 +1011,8 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
> if (fifo_left == 0)
> break;
>
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> + if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
> + pipe_has_cursor_plane(dev->dev_private, plane->pipe))
> continue;
>
> /* give it all to the first plane if none are active */
> @@ -1028,6 +1032,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
> {
> struct vlv_wm_state *wm_state = &crtc->wm_state;
> int level;
> + enum drm_plane_type plane_type;
>
> for (level = 0; level < wm_state->num_levels; level++) {
> struct drm_device *dev = crtc->base.dev;
> @@ -1038,7 +1043,13 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
> wm_state->sr[level].cursor = 63 - wm_state->sr[level].cursor;
>
> for_each_intel_plane_on_crtc(dev, crtc, plane) {
> - switch (plane->base.type) {
> + plane_type = plane->base.type;
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> +
> + switch (plane_type) {
> int sprite;
> case DRM_PLANE_TYPE_CURSOR:
> wm_state->wm[level].cursor = plane->wm.fifo_size -
> @@ -1065,6 +1076,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> struct intel_plane *plane;
> int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
> int level;
> + enum drm_plane_type plane_type;
>
> memset(wm_state, 0, sizeof(*wm_state));
>
> @@ -1092,10 +1104,15 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> if (!state->visible)
> continue;
>
> + plane_type = plane->base.type;
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> +
> /* normal watermarks */
> for (level = 0; level < wm_state->num_levels; level++) {
> int wm = vlv_compute_wm_level(plane, crtc, state, level);
> - int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
> + int max_wm = plane_type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
>
> /* hack */
> if (WARN_ON(level == 0 && wm > max_wm))
> @@ -1104,7 +1121,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> if (wm > plane->wm.fifo_size)
> break;
>
> - switch (plane->base.type) {
> + switch (plane_type) {
> int sprite;
> case DRM_PLANE_TYPE_CURSOR:
> wm_state->wm[level].cursor = wm;
> @@ -1125,7 +1142,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> continue;
>
> /* maxfifo watermarks */
> - switch (plane->base.type) {
> + switch (plane_type) {
> int sprite, level;
> case DRM_PLANE_TYPE_CURSOR:
> for (level = 0; level < wm_state->num_levels; level++)
> @@ -1166,9 +1183,16 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_plane *plane;
> int sprite0_start = 0, sprite1_start = 0, fifo_size = 0;
> + enum drm_plane_type plane_type;
>
> for_each_intel_plane_on_crtc(dev, crtc, plane) {
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> + plane_type = plane->base.type;
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR) {
> WARN_ON(plane->wm.fifo_size != 63);
> continue;
> }
> @@ -3996,11 +4020,18 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> struct intel_plane *plane;
> enum pipe pipe;
> u32 val;
> + enum drm_plane_type plane_type;
>
> vlv_read_wm_values(dev_priv, wm);
>
> for_each_intel_plane(dev, plane) {
> - switch (plane->base.type) {
> + plane_type = plane->base.type;
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> +
> + switch (plane_type) {
> int sprite;
> case DRM_PLANE_TYPE_CURSOR:
> plane->wm.fifo_size = 63;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 8821533..0b5104c 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1022,8 +1022,8 @@ static uint32_t skl_plane_formats[] = {
> DRM_FORMAT_VYUY,
> };
>
> -int
> -intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> +struct drm_plane *intel_plane_init(struct drm_device *dev, enum pipe pipe,
> + int plane, enum drm_plane_type plane_type)
> {
> struct intel_plane *intel_plane;
> struct intel_plane_state *state;
> @@ -1033,16 +1033,16 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> int ret;
>
> if (INTEL_INFO(dev)->gen < 5)
> - return -ENODEV;
> + return ERR_PTR(-ENODEV);
>
> intel_plane = kzalloc(sizeof(*intel_plane), GFP_KERNEL);
> if (!intel_plane)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> state = intel_create_plane_state(&intel_plane->base);
> if (!state) {
> kfree(intel_plane);
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
> }
> intel_plane->base.state = &state->base;
>
> @@ -1097,8 +1097,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> num_plane_formats = ARRAY_SIZE(skl_plane_formats);
> break;
> default:
> - kfree(intel_plane);
> - return -ENODEV;
> + ret = -ENODEV;
> + goto out;
> }
>
> intel_plane->pipe = pipe;
> @@ -1109,16 +1109,17 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
> &intel_plane_funcs,
> plane_formats, num_plane_formats,
> - DRM_PLANE_TYPE_OVERLAY, NULL);
> - if (ret) {
> - kfree(intel_plane);
> + plane_type, NULL);
> + if (ret)
> goto out;
> - }
>
> intel_create_rotation_property(dev, intel_plane);
>
> drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> + return &intel_plane->base;
>
> out:
> - return ret;
> + kfree(intel_plane);
> + kfree(state);
> + return ERR_PTR(ret);
> }
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Treat cursor plane as another sprite plane for BSW
2016-03-02 14:03 ` Ville Syrjälä
@ 2016-03-04 1:43 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 6+ messages in thread
From: Pandiyan, Dhinakaran @ 2016-03-04 1:43 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: Kondapally, Kalyan, intel-gfx
Agreed, it does look messy.
As for the watermarks, we have verified that this patch works on Chrome OS with the kernel version 3.18. We have not seen any regressions yet. However, it requires the patch "drm/i915: Workaround CHV pipe C cursor fail" to be reverted. I will send out another version addressing the comments along with the revert.
________________________________________
From: Ville Syrjälä [ville.syrjala@linux.intel.com]
Sent: Wednesday, March 02, 2016 6:03 AM
To: Pandiyan, Dhinakaran
Cc: intel-gfx@lists.freedesktop.org; Kondapally, Kalyan
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Treat cursor plane as another sprite plane for BSW
On Tue, Mar 01, 2016 at 03:27:17PM -0800, Dhinakaran Pandiyan wrote:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> Work around the CHV pipe C FIFO underruns that cause display failure by
> enabling sprite plane for cursor.
>
> This patch for BSW is based on Maarten Lankhorst's work that
> enables universal plane support.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com>
> Signed-off-by: Pandiyan Dhinakaran <dhinakaran.pandiyan@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 48 +++++++++++++++++++++++----------
> drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++--
> drivers/gpu/drm/i915/intel_pm.c | 51 +++++++++++++++++++++++++++++-------
> drivers/gpu/drm/i915/intel_sprite.c | 25 +++++++++---------
> 4 files changed, 100 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 44fcff0..4a392d4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11941,9 +11941,14 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> bool is_crtc_enabled = crtc_state->active;
> bool turn_off, turn_on, visible, was_visible;
> struct drm_framebuffer *fb = plane_state->fb;
> + enum drm_plane_type plane_type = plane->type;
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), intel_crtc->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
I think spreading this stuff all over is too messy. I would suggest adding some
kind of hw plane type to intel_plane instead.
>
> if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
> - plane->type != DRM_PLANE_TYPE_CURSOR) {
> + plane_type != DRM_PLANE_TYPE_CURSOR) {
> ret = skl_update_scaler_plane(
> to_intel_crtc_state(crtc_state),
> to_intel_plane_state(plane_state));
> @@ -14472,7 +14477,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> struct intel_crtc_state *crtc_state = NULL;
> struct drm_plane *primary = NULL;
> struct drm_plane *cursor = NULL;
> - int i, ret;
> + int i, ret, sprite;
>
> intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
> if (intel_crtc == NULL)
> @@ -14499,9 +14504,32 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
> if (!primary)
> goto fail;
>
> - cursor = intel_cursor_plane_create(dev, pipe);
> - if (!cursor)
> - goto fail;
> + if (pipe_has_cursor_plane(dev_priv, pipe)) {
> + cursor = intel_cursor_plane_create(dev, pipe);
> + if (!cursor)
> + goto fail;
> + }
> +
> + for_each_sprite(dev_priv, pipe, sprite) {
> + enum drm_plane_type plane_type;
> + struct drm_plane *plane;
> +
> + if (sprite + 1 < INTEL_INFO(dev_priv)->num_sprites[pipe] ||
> + pipe_has_cursor_plane(dev_priv, pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> + else
> + plane_type = DRM_PLANE_TYPE_CURSOR;
I'm thinking I might put this logic into the sprite init code.
Apart from these the main concern I have with all this is watermarks, cxsr,
vblank waits etc. I can't see anything here to explain how it's going to
actually work so I suspect there will be problems.
> +
> + plane = intel_plane_init(dev, pipe, sprite, plane_type);
> + if (IS_ERR(plane)) {
> + DRM_DEBUG_KMS("pipe %c sprite %c init failed: %ld\n",
> + pipe_name(pipe), sprite_name(pipe, sprite), PTR_ERR(plane));
> + goto fail;
> + }
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR)
> + cursor = plane;
> + }
>
> ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> cursor, &intel_crtc_funcs, NULL);
> @@ -15534,7 +15562,6 @@ fail:
> void intel_modeset_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - int sprite, ret;
> enum pipe pipe;
> struct intel_crtc *crtc;
>
> @@ -15606,15 +15633,8 @@ void intel_modeset_init(struct drm_device *dev)
> INTEL_INFO(dev)->num_pipes,
> INTEL_INFO(dev)->num_pipes > 1 ? "s" : "");
>
> - for_each_pipe(dev_priv, pipe) {
> + for_each_pipe(dev_priv, pipe)
> intel_crtc_init(dev, pipe);
> - for_each_sprite(dev_priv, pipe, sprite) {
> - ret = intel_plane_init(dev, pipe, sprite);
> - if (ret)
> - DRM_DEBUG_KMS("pipe %c sprite %c init failed: %d\n",
> - pipe_name(pipe), sprite_name(pipe, sprite), ret);
> - }
> - }
>
> intel_update_czclk(dev_priv);
> intel_update_cdclk(dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cb413e2..218f8f8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -971,9 +971,18 @@ hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
> return container_of(intel_hdmi, struct intel_digital_port, hdmi);
> }
>
> +static inline bool pipe_has_cursor_plane(struct drm_i915_private *dev_priv, int pipe)
> +{
> + if (IS_CHERRYVIEW(dev_priv) && pipe == PIPE_C)
> + return false;
> +
> + return true;
> +}
> +
> /*
> * Returns the number of planes for this pipe, ie the number of sprites + 1
> - * (primary plane). This doesn't count the cursor plane then.
> + * (primary plane). This doesn't count the cursor plane except on CHV pipe C,
> + * which has a universal plane masked as cursor plane.
> */
> static inline unsigned int intel_num_planes(struct intel_crtc *crtc)
> {
> @@ -1595,7 +1604,8 @@ bool intel_sdvo_init(struct drm_device *dev,
>
>
> /* intel_sprite.c */
> -int intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane);
> +struct drm_plane *intel_plane_init(struct drm_device *dev, enum pipe pipe,
> + int plane, enum drm_plane_type plane_type);
> int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> void intel_pipe_update_start(struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d33de95..0bab7d3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -941,7 +941,8 @@ static uint16_t vlv_compute_wm_level(struct intel_plane *plane,
> if (WARN_ON(htotal == 0))
> htotal = 1;
>
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> + if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
> + pipe_has_cursor_plane(dev_priv, plane->pipe)) {
> /*
> * FIXME the formula gives values that are
> * too big for the cursor FIFO, and hence we
> @@ -970,7 +971,8 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
> struct intel_plane_state *state =
> to_intel_plane_state(plane->base.state);
>
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> + if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
> + pipe_has_cursor_plane(dev->dev_private, plane->pipe))
> continue;
>
> if (state->visible) {
> @@ -984,7 +986,8 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
> to_intel_plane_state(plane->base.state);
> unsigned int rate;
>
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> + if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
> + pipe_has_cursor_plane(dev->dev_private, plane->pipe)) {
> plane->wm.fifo_size = 63;
> continue;
> }
> @@ -1008,7 +1011,8 @@ static void vlv_compute_fifo(struct intel_crtc *crtc)
> if (fifo_left == 0)
> break;
>
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR)
> + if (plane->base.type == DRM_PLANE_TYPE_CURSOR &&
> + pipe_has_cursor_plane(dev->dev_private, plane->pipe))
> continue;
>
> /* give it all to the first plane if none are active */
> @@ -1028,6 +1032,7 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
> {
> struct vlv_wm_state *wm_state = &crtc->wm_state;
> int level;
> + enum drm_plane_type plane_type;
>
> for (level = 0; level < wm_state->num_levels; level++) {
> struct drm_device *dev = crtc->base.dev;
> @@ -1038,7 +1043,13 @@ static void vlv_invert_wms(struct intel_crtc *crtc)
> wm_state->sr[level].cursor = 63 - wm_state->sr[level].cursor;
>
> for_each_intel_plane_on_crtc(dev, crtc, plane) {
> - switch (plane->base.type) {
> + plane_type = plane->base.type;
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> +
> + switch (plane_type) {
> int sprite;
> case DRM_PLANE_TYPE_CURSOR:
> wm_state->wm[level].cursor = plane->wm.fifo_size -
> @@ -1065,6 +1076,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> struct intel_plane *plane;
> int sr_fifo_size = INTEL_INFO(dev)->num_pipes * 512 - 1;
> int level;
> + enum drm_plane_type plane_type;
>
> memset(wm_state, 0, sizeof(*wm_state));
>
> @@ -1092,10 +1104,15 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> if (!state->visible)
> continue;
>
> + plane_type = plane->base.type;
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> +
> /* normal watermarks */
> for (level = 0; level < wm_state->num_levels; level++) {
> int wm = vlv_compute_wm_level(plane, crtc, state, level);
> - int max_wm = plane->base.type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
> + int max_wm = plane_type == DRM_PLANE_TYPE_CURSOR ? 63 : 511;
>
> /* hack */
> if (WARN_ON(level == 0 && wm > max_wm))
> @@ -1104,7 +1121,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> if (wm > plane->wm.fifo_size)
> break;
>
> - switch (plane->base.type) {
> + switch (plane_type) {
> int sprite;
> case DRM_PLANE_TYPE_CURSOR:
> wm_state->wm[level].cursor = wm;
> @@ -1125,7 +1142,7 @@ static void vlv_compute_wm(struct intel_crtc *crtc)
> continue;
>
> /* maxfifo watermarks */
> - switch (plane->base.type) {
> + switch (plane_type) {
> int sprite, level;
> case DRM_PLANE_TYPE_CURSOR:
> for (level = 0; level < wm_state->num_levels; level++)
> @@ -1166,9 +1183,16 @@ static void vlv_pipe_set_fifo_size(struct intel_crtc *crtc)
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_plane *plane;
> int sprite0_start = 0, sprite1_start = 0, fifo_size = 0;
> + enum drm_plane_type plane_type;
>
> for_each_intel_plane_on_crtc(dev, crtc, plane) {
> - if (plane->base.type == DRM_PLANE_TYPE_CURSOR) {
> + plane_type = plane->base.type;
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR) {
> WARN_ON(plane->wm.fifo_size != 63);
> continue;
> }
> @@ -3996,11 +4020,18 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
> struct intel_plane *plane;
> enum pipe pipe;
> u32 val;
> + enum drm_plane_type plane_type;
>
> vlv_read_wm_values(dev_priv, wm);
>
> for_each_intel_plane(dev, plane) {
> - switch (plane->base.type) {
> + plane_type = plane->base.type;
> +
> + if (plane_type == DRM_PLANE_TYPE_CURSOR &&
> + !pipe_has_cursor_plane(to_i915(dev), plane->pipe))
> + plane_type = DRM_PLANE_TYPE_OVERLAY;
> +
> + switch (plane_type) {
> int sprite;
> case DRM_PLANE_TYPE_CURSOR:
> plane->wm.fifo_size = 63;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 8821533..0b5104c 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1022,8 +1022,8 @@ static uint32_t skl_plane_formats[] = {
> DRM_FORMAT_VYUY,
> };
>
> -int
> -intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> +struct drm_plane *intel_plane_init(struct drm_device *dev, enum pipe pipe,
> + int plane, enum drm_plane_type plane_type)
> {
> struct intel_plane *intel_plane;
> struct intel_plane_state *state;
> @@ -1033,16 +1033,16 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> int ret;
>
> if (INTEL_INFO(dev)->gen < 5)
> - return -ENODEV;
> + return ERR_PTR(-ENODEV);
>
> intel_plane = kzalloc(sizeof(*intel_plane), GFP_KERNEL);
> if (!intel_plane)
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
>
> state = intel_create_plane_state(&intel_plane->base);
> if (!state) {
> kfree(intel_plane);
> - return -ENOMEM;
> + return ERR_PTR(-ENOMEM);
> }
> intel_plane->base.state = &state->base;
>
> @@ -1097,8 +1097,8 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> num_plane_formats = ARRAY_SIZE(skl_plane_formats);
> break;
> default:
> - kfree(intel_plane);
> - return -ENODEV;
> + ret = -ENODEV;
> + goto out;
> }
>
> intel_plane->pipe = pipe;
> @@ -1109,16 +1109,17 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
> ret = drm_universal_plane_init(dev, &intel_plane->base, possible_crtcs,
> &intel_plane_funcs,
> plane_formats, num_plane_formats,
> - DRM_PLANE_TYPE_OVERLAY, NULL);
> - if (ret) {
> - kfree(intel_plane);
> + plane_type, NULL);
> + if (ret)
> goto out;
> - }
>
> intel_create_rotation_property(dev, intel_plane);
>
> drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
> + return &intel_plane->base;
>
> out:
> - return ret;
> + kfree(intel_plane);
> + kfree(state);
> + return ERR_PTR(ret);
> }
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Treat cursor plane as another sprite plane for BSW
2016-03-01 23:27 [PATCH] drm/i915: Treat cursor plane as another sprite plane for BSW Dhinakaran Pandiyan
` (2 preceding siblings ...)
2016-03-02 14:03 ` Ville Syrjälä
@ 2016-03-23 13:26 ` Maarten Lankhorst
3 siblings, 0 replies; 6+ messages in thread
From: Maarten Lankhorst @ 2016-03-23 13:26 UTC (permalink / raw)
To: Dhinakaran Pandiyan, intel-gfx; +Cc: Kalyan Kondapally
Op 02-03-16 om 00:27 schreef Dhinakaran Pandiyan:
> From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> Work around the CHV pipe C FIFO underruns that cause display failure by
> enabling sprite plane for cursor.
>
> This patch for BSW is based on Maarten Lankhorst's work that
> enables universal plane support.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Kalyan Kondapally <kalyan.kondapally@intel.com>
> Signed-off-by: Pandiyan Dhinakaran <dhinakaran.pandiyan@intel.com>
I've posted some comments, any update on this?
~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-03-23 13:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-01 23:27 [PATCH] drm/i915: Treat cursor plane as another sprite plane for BSW Dhinakaran Pandiyan
2016-03-02 7:55 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-03-02 12:55 ` [PATCH] " Maarten Lankhorst
2016-03-02 14:03 ` Ville Syrjälä
2016-03-04 1:43 ` Pandiyan, Dhinakaran
2016-03-23 13:26 ` Maarten Lankhorst
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.