All of lore.kernel.org
 help / color / mirror / Atom feed
From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Kalyan Kondapally <kalyan.kondapally@intel.com>
Subject: Re: [PATCH] drm/i915: Treat cursor plane as another sprite plane for BSW
Date: Wed, 2 Mar 2016 13:55:48 +0100	[thread overview]
Message-ID: <56D6E2D4.10508@linux.intel.com> (raw)
In-Reply-To: <1456874837-16833-1-git-send-email-dhinakaran.pandiyan@intel.com>

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

  parent reply	other threads:[~2016-03-02 12:55 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
2016-03-02 14:03 ` [PATCH] " Ville Syrjälä
2016-03-04  1:43   ` Pandiyan, Dhinakaran
2016-03-23 13:26 ` Maarten Lankhorst

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56D6E2D4.10508@linux.intel.com \
    --to=maarten.lankhorst@linux.intel.com \
    --cc=dhinakaran.pandiyan@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=kalyan.kondapally@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
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.