Re: [PATCH 9/9] drm/i915: Make all plane disables use 'update_plane'

[Ander Conselvan de Oliveira <conselvan2@gmail.com>]

On 11/24/2014 09:53 PM, Matt Roper wrote:
> If we extend the commit_plane handlers for each plane type to be able to
> handle fb=0, then we can easily implement plane disable via the
> update_plane handler.  The cursor plane already works this way, and this
> is the direction we need to go to integrate with the atomic plane
> handler.  We can now kill off the type-specific disable functions, as
> well as the redundant intel_plane_disable() (not to be confused with
> intel_disable_plane()).
>
> Note that prepare_plane_fb() only gets called as part of update_plane
> when fb!=NULL (by design, to match the semantics of the atomic plane
> helpers); this means that our commit_plane handlers need to handle the
> frontbuffer tracking for the disable case, even though they don't handle
> it for normal updates.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_display.c | 109 +++++++++++++++--------------------
>   drivers/gpu/drm/i915/intel_drv.h     |   2 +-
>   drivers/gpu/drm/i915/intel_sprite.c  |  71 ++++++-----------------
>   3 files changed, 67 insertions(+), 115 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index dd10dc6..fd1eaf5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3996,7 +3996,7 @@ static void intel_disable_planes(struct drm_crtc *crtc)
>   	drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) {
>   		intel_plane = to_intel_plane(plane);
>   		if (intel_plane->pipe == pipe)
> -			intel_plane_disable(&intel_plane->base);
> +			plane->funcs->disable_plane(plane);
>   	}
>   }
>
> @@ -11535,45 +11535,6 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>   	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
>   }
>
> -static int
> -intel_primary_plane_disable(struct drm_plane *plane)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct intel_crtc *intel_crtc;
> -
> -	if (!plane->fb)
> -		return 0;
> -
> -	BUG_ON(!plane->crtc);
> -
> -	intel_crtc = to_intel_crtc(plane->crtc);
> -
> -	/*
> -	 * Even though we checked plane->fb above, it's still possible that
> -	 * the primary plane has been implicitly disabled because the crtc
> -	 * coordinates given weren't visible, or because we detected
> -	 * that it was 100% covered by a sprite plane.  Or, the CRTC may be
> -	 * off and we've set a fb, but haven't actually turned on the CRTC yet.
> -	 * In either case, we need to unpin the FB and let the fb pointer get
> -	 * updated, but otherwise we don't need to touch the hardware.
> -	 */
> -	if (!intel_crtc->primary_enabled)
> -		goto disable_unpin;
> -
> -	intel_crtc_wait_for_pending_flips(plane->crtc);
> -	intel_disable_primary_hw_plane(plane, plane->crtc);
> -
> -disable_unpin:
> -	mutex_lock(&dev->struct_mutex);
> -	i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
> -			  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
> -	intel_unpin_fb_obj(intel_fb_obj(plane->fb));
> -	mutex_unlock(&dev->struct_mutex);
> -	plane->fb = NULL;
> -
> -	return 0;
> -}
> -
>   /**
>    * intel_prepare_plane_fb - Prepare fb for usage on plane
>    * @plane: drm plane to prepare for
> @@ -11678,10 +11639,12 @@ intel_check_primary_plane(struct drm_plane *plane,
>   	if (ret)
>   		return ret;
>
> -	intel_crtc_wait_for_pending_flips(crtc);
> -	if (intel_crtc_has_pending_flip(crtc)) {
> -		DRM_ERROR("pipe is still busy with an old pageflip\n");
> -		return -EBUSY;
> +	if (plane->crtc) {
> +		intel_crtc_wait_for_pending_flips(plane->crtc);
> +		if (intel_crtc_has_pending_flip(plane->crtc)) {
> +			DRM_ERROR("pipe is still busy with an old pageflip\n");
> +			return -EBUSY;
> +		}
>   	}
>
>   	return 0;
> @@ -11696,12 +11659,30 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   	struct drm_device *dev = plane->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	enum pipe pipe = intel_crtc->pipe;
>   	struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
>   	struct drm_rect *src = &state->src;
> +	enum pipe pipe = intel_plane->pipe;
>
> -	crtc->primary->fb = fb;
> +	if (!fb) {
> +		/*
> +		 * 'prepare' is never called when plane is being disabled, so
> +		 * we need to handle frontbuffer tracking here
> +		 */
> +		mutex_lock(&dev->struct_mutex);
> +		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
> +				  INTEL_FRONTBUFFER_PRIMARY(pipe));
> +		mutex_unlock(&dev->struct_mutex);
> +
> +		/*
> +		 * crtc may be passed as NULL when disabling, so set it to
> +		 * the proper value now
> +		 */
> +		crtc = plane->crtc;
> +		intel_crtc = to_intel_crtc(crtc);
> +	}
> +
> +	plane->fb = fb;
>   	crtc->x = src->x1 >> 16;
>   	crtc->y = src->y1 >> 16;
>
> @@ -11759,7 +11740,7 @@ intel_commit_primary_plane(struct drm_plane *plane,
>   			 * explicitly disables the plane by passing fb=0
>   			 * because plane->fb still gets set and pinned.
>   			 */
> -			intel_disable_primary_hw_plane(plane, crtc);
> +			intel_disable_primary_hw_plane(plane, plane->crtc);

I can't see why this hunk is necessary, since you set crtc = plane->crtc 
above for the !fb case. Or am I missing something?

>   		}
>
>   		intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
> @@ -11831,6 +11812,24 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   	return 0;
>   }
>
> +/**
> + * intel_disable_plane - disable a plane
> + * @plane: plane to disable
> + *
> + * General disable handler for all plane types.
> + */
> +int
> +intel_disable_plane(struct drm_plane *plane)
> +{
> +	if (!plane->fb)
> +		return 0;
> +
> +	BUG_ON(!plane->crtc);

WARN_ON? Maybe not since intel_update_plane will oops. I'm not sure 
what's the guideline here.

Anyway, the series look pretty good overall.

Cheers,
Ander

> +
> +	return plane->funcs->update_plane(plane, plane->crtc, NULL,
> +					  0, 0, 0, 0, 0, 0, 0, 0);
> +}
> +
>   /* Common destruction function for both primary and cursor planes */
>   static void intel_plane_destroy(struct drm_plane *plane)
>   {
> @@ -11841,7 +11840,7 @@ static void intel_plane_destroy(struct drm_plane *plane)
>
>   static const struct drm_plane_funcs intel_primary_plane_funcs = {
>   	.update_plane = intel_update_plane,
> -	.disable_plane = intel_primary_plane_disable,
> +	.disable_plane = intel_disable_plane,
>   	.destroy = intel_plane_destroy,
>   	.set_property = intel_plane_set_property
>   };
> @@ -11896,18 +11895,6 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>   }
>
>   static int
> -intel_cursor_plane_disable(struct drm_plane *plane)
> -{
> -	if (!plane->fb)
> -		return 0;
> -
> -	BUG_ON(!plane->crtc);
> -
> -	return plane->funcs->update_plane(plane, plane->crtc, NULL,
> -					  0, 0, 0, 0, 0, 0, 0, 0);
> -}
> -
> -static int
>   intel_check_cursor_plane(struct drm_plane *plane,
>   			 struct intel_plane_state *state)
>   {
> @@ -12030,7 +12017,7 @@ update:
>
>   static const struct drm_plane_funcs intel_cursor_plane_funcs = {
>   	.update_plane = intel_update_plane,
> -	.disable_plane = intel_cursor_plane_disable,
> +	.disable_plane = intel_disable_plane,
>   	.destroy = intel_plane_destroy,
>   	.set_property = intel_plane_set_property,
>   };
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5f7a071..da4da2c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1021,6 +1021,7 @@ int intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>   		       unsigned int crtc_w, unsigned int crtc_h,
>   		       uint32_t src_x, uint32_t src_y,
>   		       uint32_t src_w, uint32_t src_h);
> +int intel_disable_plane(struct drm_plane *plane);
>
>   /* intel_dp_mst.c */
>   int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
> @@ -1201,7 +1202,6 @@ int intel_plane_set_property(struct drm_plane *plane,
>   			     struct drm_property *prop,
>   			     uint64_t val);
>   int intel_plane_restore(struct drm_plane *plane);
> -void intel_plane_disable(struct drm_plane *plane);
>   int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>   			      struct drm_file *file_priv);
>   int intel_sprite_get_colorkey(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index bfd5270..bc5834b 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1109,7 +1109,12 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   	const struct drm_rect *clip = &state->clip;
>   	int hscale, vscale;
>   	int max_scale, min_scale;
> -	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
> +	int pixel_size;
> +
> +	if (!fb) {
> +		state->visible = false;
> +		return 0;
> +	}
>
>   	/* Don't modify another pipe's plane */
>   	if (intel_plane->pipe != intel_crtc->pipe) {
> @@ -1232,6 +1237,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>   		if (src_w < 3 || src_h < 3)
>   			state->visible = false;
>
> +		pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
>   		width_bytes = ((src_x * pixel_size) & 63) +
>   					src_w * pixel_size;
>
> @@ -1276,6 +1282,17 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   	bool primary_enabled;
>
>   	/*
> +	 * 'prepare' is never called when plane is being disabled, so we need
> +	 * to handle frontbuffer tracking here
> +	 */
> +	if (!fb) {
> +		mutex_lock(&dev->struct_mutex);
> +		i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
> +				  INTEL_FRONTBUFFER_SPRITE(pipe));
> +		mutex_unlock(&dev->struct_mutex);
> +	}
> +
> +	/*
>   	 * If the sprite is completely covering the primary plane,
>   	 * we can disable the primary and save power.
>   	 */
> @@ -1327,50 +1344,6 @@ intel_commit_sprite_plane(struct drm_plane *plane,
>   	}
>   }
>
> -static int
> -intel_disable_plane(struct drm_plane *plane)
> -{
> -	struct drm_device *dev = plane->dev;
> -	struct intel_plane *intel_plane = to_intel_plane(plane);
> -	struct intel_crtc *intel_crtc;
> -	enum pipe pipe;
> -
> -	if (!plane->fb)
> -		return 0;
> -
> -	if (WARN_ON(!plane->crtc))
> -		return -EINVAL;
> -
> -	intel_crtc = to_intel_crtc(plane->crtc);
> -	pipe = intel_crtc->pipe;
> -
> -	if (intel_crtc->active) {
> -		bool primary_was_enabled = intel_crtc->primary_enabled;
> -
> -		intel_crtc->primary_enabled = true;
> -
> -		intel_plane->disable_plane(plane, plane->crtc);
> -
> -		if (!primary_was_enabled && intel_crtc->primary_enabled)
> -			intel_post_enable_primary(plane->crtc);
> -	}
> -
> -	if (intel_plane->obj) {
> -		if (intel_crtc->active)
> -			intel_wait_for_vblank(dev, intel_plane->pipe);
> -
> -		mutex_lock(&dev->struct_mutex);
> -		intel_unpin_fb_obj(intel_plane->obj);
> -		i915_gem_track_fb(intel_plane->obj, NULL,
> -				  INTEL_FRONTBUFFER_SPRITE(pipe));
> -		mutex_unlock(&dev->struct_mutex);
> -
> -		intel_plane->obj = NULL;
> -	}
> -
> -	return 0;
> -}
> -
>   static void intel_destroy_plane(struct drm_plane *plane)
>   {
>   	struct intel_plane *intel_plane = to_intel_plane(plane);
> @@ -1478,14 +1451,6 @@ int intel_plane_restore(struct drm_plane *plane)
>   				  intel_plane->src_w, intel_plane->src_h);
>   }
>
> -void intel_plane_disable(struct drm_plane *plane)
> -{
> -	if (!plane->crtc || !plane->fb)
> -		return;
> -
> -	intel_disable_plane(plane);
> -}
> -
>   static const struct drm_plane_funcs intel_plane_funcs = {
>   	.update_plane = intel_update_plane,
>   	.disable_plane = intel_disable_plane,
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx