Re: [PATCH 1/3] drm/i915: remove pre-atomic check from SKL update_primary_plane

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 30, 2015 at 05:05:43PM -0300, Paulo Zanoni wrote:
> The comment suggests the check was there for some non-fully-atomic
> case, and I couldn't find a case where we wouldn't correctly
> initialize plane_state, so remove the check.
> 
> Let's leave a WARN there just in case.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e547fe7..88657cb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3131,27 +3131,19 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  					       fb->pixel_format);
>  	surf_addr = intel_plane_obj_offset(to_intel_plane(plane), obj, 0);
>  
> -	/*
> -	 * FIXME: intel_plane_state->src, dst aren't set when transitional
> -	 * update_plane helpers are called from legacy paths.
> -	 * Once full atomic crtc is available, below check can be avoided.
> -	 */
> -	if (drm_rect_width(&plane_state->src)) {
> -		scaler_id = plane_state->scaler_id;
> -		src_x = plane_state->src.x1 >> 16;
> -		src_y = plane_state->src.y1 >> 16;
> -		src_w = drm_rect_width(&plane_state->src) >> 16;
> -		src_h = drm_rect_height(&plane_state->src) >> 16;
> -		dst_x = plane_state->dst.x1;
> -		dst_y = plane_state->dst.y1;
> -		dst_w = drm_rect_width(&plane_state->dst);
> -		dst_h = drm_rect_height(&plane_state->dst);
> -
> -		WARN_ON(x != src_x || y != src_y);
> -	} else {
> -		src_w = intel_crtc->config->pipe_src_w;
> -		src_h = intel_crtc->config->pipe_src_h;
> -	}
> +	WARN_ON(drm_rect_width(&plane_state->src) == 0);
> +
> +	scaler_id = plane_state->scaler_id;
> +	src_x = plane_state->src.x1 >> 16;
> +	src_y = plane_state->src.y1 >> 16;
> +	src_w = drm_rect_width(&plane_state->src) >> 16;
> +	src_h = drm_rect_height(&plane_state->src) >> 16;
> +	dst_x = plane_state->dst.x1;
> +	dst_y = plane_state->dst.y1;
> +	dst_w = drm_rect_width(&plane_state->dst);
> +	dst_h = drm_rect_height(&plane_state->dst);
> +
> +	WARN_ON(x != src_x || y != src_y);

I guess I should resurrect some of my primary plane cleanup patches
since no one else has taken the bait :(

Anyway the patch makes sense, well, assuming we've gotten better at
maintaining the plane state. Maarten might be able to answer that.
Me, I'm too lazy to look right now, so I'll give this an ack.

Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	if (intel_rotation_90_or_270(rotation)) {
>  		/* stride = Surface height in tiles */
> -- 
> 2.5.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx