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@xxxxxxxxxxxxxxx]
Sent: Wednesday, March 02, 2016 6:03 AM
To: Pandiyan, Dhinakaran
Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Kondapally, Kalyan
Subject: Re: [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@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Kalyan Kondapally <kalyan.kondapally@xxxxxxxxx>
> Signed-off-by: Pandiyan Dhinakaran <dhinakaran.pandiyan@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
