Re: [PATCH 2/9] drm/i915/skl: Add blend_func to SKL/BXT sprite planes

[Matt Roper <matthew.d.roper@intel.com>]

On Mon, Jan 18, 2016 at 08:45:36PM +0530, Vandita Kulkarni wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
> 
> This patch adds the blend functions, and as per the
> blend function, updates the plane control register values
> 
> V2: Add blend support for all RGB8888 formats
> Fix the reg writes on plane_ctl_alpha bits.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: vandita kulkarni <vandita.kulkarni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 106 ++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h     |  11 +++-
>  drivers/gpu/drm/i915/intel_sprite.c  |   4 ++
>  3 files changed, 112 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 07ca19b..7e59a49 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3003,8 +3003,22 @@ static void skl_detach_scalers(struct intel_crtc *intel_crtc)
>  	}
>  }
>  
> -u32 skl_plane_ctl_format(uint32_t pixel_format)
> -{
> +u32 skl_plane_ctl_format(uint32_t pixel_format,
> +			 bool pre_multiplied,
> +			 bool drop_alpha)
> +{
> +	u32 plane_ctl_alpha = 0;
> +
> +	if (pixel_format == DRM_FORMAT_ABGR8888 ||
> +	    pixel_format == DRM_FORMAT_ARGB8888) {

I found this function somewhat non-intuitive since I figured the
combination of pixel format + alpha would already have been analyzed
in intel_plane_state_check_blend().  Maybe it would be simpler to just
have this function take a {disable,sw,hw} enum and have the check
function pick that enum instead of setting two flags that need further
disposition based on pixel format?

Not a huge deal, either way; I just think it's nicer to keep the
low-level bit-setting functions as simple as possible.


> +		if (drop_alpha)
> +			plane_ctl_alpha = PLANE_CTL_ALPHA_DISABLE;
> +		else if (pre_multiplied)
> +			plane_ctl_alpha = PLANE_CTL_ALPHA_SW_PREMULTIPLY;
> +		else
> +			plane_ctl_alpha = PLANE_CTL_ALPHA_HW_PREMULTIPLY;
> +	}
> +
>  	switch (pixel_format) {
>  	case DRM_FORMAT_C8:
>  		return PLANE_CTL_FORMAT_INDEXED;
> @@ -3020,11 +3034,11 @@ u32 skl_plane_ctl_format(uint32_t pixel_format)
>  	 * DRM_FORMAT) for user-space to configure that.
>  	 */
>  	case DRM_FORMAT_ABGR8888:
> -		return PLANE_CTL_FORMAT_XRGB_8888 | PLANE_CTL_ORDER_RGBX |
> -			PLANE_CTL_ALPHA_SW_PREMULTIPLY;
> +		return ((PLANE_CTL_FORMAT_XRGB_8888 | (PLANE_CTL_ORDER_RGBX
> +				& (~PLANE_CTL_ALPHA_MASK))) | plane_ctl_alpha);

Why do you need the "& ~PLANE_CTL_ALPHA_MASK" part here?
plane_ctl_alpha starts as 0 (all bits off) and none of the other things
your OR'ing in here (except plane_ctl_alpha) touch these bits.

Maybe this is a holdover from when our driver did RMW updates of
registers?


>  	case DRM_FORMAT_ARGB8888:
> -		return PLANE_CTL_FORMAT_XRGB_8888 |
> -			PLANE_CTL_ALPHA_SW_PREMULTIPLY;
> +		return ((PLANE_CTL_FORMAT_XRGB_8888 & ~PLANE_CTL_ALPHA_MASK)
> +						| plane_ctl_alpha);

Ditto

>  	case DRM_FORMAT_XRGB2101010:
>  		return PLANE_CTL_FORMAT_XRGB_2101010;
>  	case DRM_FORMAT_XBGR2101010:
> @@ -3113,7 +3127,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  		    PLANE_CTL_PIPE_GAMMA_ENABLE |
>  		    PLANE_CTL_PIPE_CSC_ENABLE;
>  
> -	plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
> +	plane_ctl |= skl_plane_ctl_format(fb->pixel_format, false, false);

Should these always be false?  We can set a crtc background (canvas)
color that would be visible with blending of the bottom-most plane.  The
canvas color property isn't upstream yet, but I did have some patches on
the mailing list to add it a while back.

>  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
>  	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
> @@ -11862,6 +11876,66 @@ static bool needs_scaling(struct intel_plane_state *state)
>  	return (src_w != dst_w || src_h != dst_h);
>  }
>  
> +static int intel_plane_state_check_blend(struct drm_plane_state *plane_state)
> +{
> +	struct drm_device *dev = plane_state->state->dev;
> +	struct intel_plane_state *state = to_intel_plane_state(plane_state);
> +	const struct drm_framebuffer *fb = plane_state->fb;
> +	const struct drm_blend_mode *mode = &state->base.blend_mode;
> +	bool has_per_pixel_blending;
> +
> +	/*
> +	 * We don't install the properties pre-SKL, so this is SKL+ specific
> +	 * code for now.
> +	 */
> +	if (INTEL_INFO(dev)->gen < 9)
> +		return 0;

This should be impossible to hit; maybe change to WARN_ON() to make
that invariant clear?

> +
> +	if (!fb)
> +		return 0;
> +
> +	has_per_pixel_blending = fb->pixel_format == DRM_FORMAT_ABGR8888 ||
> +				fb->pixel_format == DRM_FORMAT_RGBA8888 ||
> +				fb->pixel_format == DRM_FORMAT_ARGB8888 ||
> +				fb->pixel_format == DRM_FORMAT_BGRA8888;
> +
> +	state->premultiplied_alpha = false;
> +	state->drop_alpha = false;
> +
> +	switch (mode->func) {
> +	/*
> +	 * The 'AUTO' behaviour is the default and keeps compatibility with
> +	 * kernels before the introduction of the blend_func property:
> +	 *   - pre-multiplied alpha if the fb has an alpha channel
> +	 *   - usual DRM_BLEND_FUNC(ONE, ZERO) otherwise
> +	 */
> +	case DRM_BLEND_FUNC(AUTO, AUTO):
> +		if (has_per_pixel_blending)
> +			state->premultiplied_alpha = true;
> +		break;
> +	/* fbs without an alpha channel, or dropping the alpha channel */
> +	case DRM_BLEND_FUNC(ONE, ZERO):
> +		if (has_per_pixel_blending)
> +			state->drop_alpha = true;
> +		break;
> +	/* pre-multiplied alpha */
> +	case DRM_BLEND_FUNC(ONE, ONE_MINUS_SRC_ALPHA):
> +		if (!has_per_pixel_blending)
> +			return -EINVAL;
> +		state->premultiplied_alpha = true;
> +		break;
> +	/* non pre-multiplied alpha */
> +	case DRM_BLEND_FUNC(SRC_ALPHA, ONE_MINUS_SRC_ALPHA):
> +		if (!has_per_pixel_blending)
> +			return -EINVAL;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +

It would be good to add a DRM_DEBUG_KMS() to each of the error
conditions to help developers understand why they failed.  Just having
an atomic transaction fail with no indication why can be frustrating
(especially when that transaction set lots of different properties).


Matt

> +	return 0;
> +}
> +
>  int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  				    struct drm_plane_state *plane_state)
>  {
> @@ -12003,7 +12077,9 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  			intel_crtc->atomic.update_sprite_watermarks |=
>  				1 << i;
>  		}
> -
> +		ret = intel_plane_state_check_blend(plane_state);
> +		if (ret)
> +			return ret;
>  		break;
>  	}
>  	return 0;
> @@ -14184,6 +14260,20 @@ void intel_create_rotation_property(struct drm_device *dev, struct intel_plane *
>  				plane->base.state->rotation);
>  }
>  
> +void intel_plane_add_blend_properties(struct intel_plane *plane)
> +{
> +	struct drm_device *dev = plane->base.dev;
> +	struct drm_property *prop;
> +
> +	if (INTEL_INFO(dev)->gen < 9)
> +		return;
> +
> +	prop = dev->mode_config.prop_blend_func;
> +	if (prop)
> +		drm_object_attach_property(&plane->base.base, prop,
> +					   DRM_BLEND_FUNC(AUTO, AUTO));
> +}
> +
>  static int
>  intel_check_cursor_plane(struct drm_plane *plane,
>  			 struct intel_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e27954d..f99e1d9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -294,6 +294,11 @@ struct intel_plane_state {
>  	 *     update_scaler_plane.
>  	 */
>  	int scaler_id;
> +	/*
> +	 * blending related hw states
> +	 */
> +	bool premultiplied_alpha;	/* is the fb pre-multiplied? */
> +	bool drop_alpha;		/* drop the fb alpha channel */
>  
>  	struct drm_intel_sprite_colorkey ckey;
>  
> @@ -1167,6 +1172,7 @@ intel_rotation_90_or_270(unsigned int rotation)
>  
>  void intel_create_rotation_property(struct drm_device *dev,
>  					struct intel_plane *plane);
> +void intel_plane_add_blend_properties(struct intel_plane *plane);
>  
>  /* shared dpll functions */
>  struct intel_shared_dpll *intel_crtc_to_shared_dpll(struct intel_crtc *crtc);
> @@ -1241,11 +1247,14 @@ void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>  int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
>  int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
>  
> +u32 skl_plane_ctl_format(uint32_t pixel_format,
> +				bool pre_multiplied,
> +				bool drop_alpha);
> +
>  u32 intel_plane_obj_offset(struct intel_plane *intel_plane,
>  			   struct drm_i915_gem_object *obj,
>  			   unsigned int plane);
>  
> -u32 skl_plane_ctl_format(uint32_t pixel_format);
>  u32 skl_plane_ctl_tiling(uint64_t fb_modifier);
>  u32 skl_plane_ctl_rotation(unsigned int rotation);
>  
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 4d448b9..b7acfdf 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -211,6 +211,9 @@ skl_update_plane(struct drm_plane *drm_plane,
>  		PLANE_CTL_PIPE_CSC_ENABLE;
>  
>  	plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
> +	plane_ctl |= skl_plane_ctl_format(fb->pixel_format,
> +					plane_state->premultiplied_alpha,
> +					plane_state->drop_alpha);
>  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
>  
>  	rotation = plane_state->base.rotation;
> @@ -1123,6 +1126,7 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  	}
>  
>  	intel_create_rotation_property(dev, intel_plane);
> +	intel_plane_add_blend_properties(intel_plane);
>  
>  	drm_plane_helper_add(&intel_plane->base, &intel_plane_helper_funcs);
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx