| Message ID | 1456874837-16833-1-git-send-email-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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); }