Re: [PATCH v2 2/2] drm/i915/gen9: Add support for pipe background color (v2)

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

On Wed, Feb 10, 2016 at 06:32:59PM -0800, Matt Roper wrote:
> Gen9 platforms allow CRTC's to be programmed with a background/canvas
> color below the programmable planes.  Let's expose this as a property to
> allow userspace to program a desired value.
> 
> This patch is based on earlier work by Chandra Konduru; unfortunately
> the driver has evolved so much since his patches were written (in the
> pre-atomic era) that the functionality had to be pretty much completely
> rewritten for the new i915 atomic internals.
> 
> v2:
>  - Set initial background color (black) via proper helper function (Bob)
>  - Fix debugfs output
>  - General rebasing
> 
> Cc: Chandra Konduru <chandra.konduru@intel.com>
> Cc: Bob Paauwe <bob.j.paauwe@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  Documentation/DocBook/gpu.tmpl       | 10 +++++++-
>  drivers/gpu/drm/i915/i915_debugfs.c  |  8 +++++++
>  drivers/gpu/drm/i915/i915_reg.h      |  9 +++++++
>  drivers/gpu/drm/i915/intel_display.c | 46 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index fe6b36a..9e003cd 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -2092,7 +2092,7 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >TBD</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="20" valign="top" >i915</td>
> +	<td rowspan="21" valign="top" >i915</td>
>  	<td rowspan="2" valign="top" >Generic</td>
>  	<td valign="top" >"Broadcast RGB"</td>
>  	<td valign="top" >ENUM</td>
> @@ -2108,6 +2108,14 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >TBD</td>
>  	</tr>
>  	<tr>
> +	<td rowspan="1" valign="top" >CRTC</td>
> +	<td valign="top" >“background_color”</td>
> +	<td valign="top" >RGBA</td>
> +	<td valign="top" >&nbsp;</td>
> +	<td valign="top" >CRTC</td>
> +	<td valign="top" >Background color of regions not covered by a plane</td>
> +	</tr>
> +	<tr>
>  	<td rowspan="17" valign="top" >SDVO-TV</td>
>  	<td valign="top" >“mode”</td>
>  	<td valign="top" >ENUM</td>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index ec0c2a05e..e7352fc 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3104,6 +3104,14 @@ static int i915_display_info(struct seq_file *m, void *unused)
>  			intel_scaler_info(m, crtc);
>  			intel_plane_info(m, crtc);
>  		}
> +		if (INTEL_INFO(dev)->gen >= 9 && pipe_config->base.active) {
> +			struct drm_rgba background = pipe_config->base.background_color;
> +
> +			seq_printf(m, "\tbackground color (10bpc): r=%x g=%x b=%x\n",
> +				   DRM_RGBA_REDBITS(background, 10),
> +				   DRM_RGBA_GREENBITS(background, 10),
> +				   DRM_RGBA_BLUEBITS(background, 10));
> +		}
>  
>  		seq_printf(m, "\tunderrun reporting: cpu=%s pch=%s \n",
>  			   yesno(!crtc->cpu_fifo_underrun_disabled),
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 144586e..b0b014d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7649,6 +7649,15 @@ enum skl_disp_power_wells {
>  #define PIPE_CSC_POSTOFF_ME(pipe)	_MMIO_PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
>  #define PIPE_CSC_POSTOFF_LO(pipe)	_MMIO_PIPE(pipe, _PIPE_A_CSC_POSTOFF_LO, _PIPE_B_CSC_POSTOFF_LO)
>  
> +/* Skylake pipe bottom color */
> +#define _PIPE_BOTTOM_COLOR_A        0x70034
> +#define _PIPE_BOTTOM_COLOR_B        0x71034
> +#define _PIPE_BOTTOM_COLOR_C        0x72034
> +#define PIPE_BOTTOM_GAMMA_ENABLE   (1 << 31)
> +#define PIPE_BOTTOM_CSC_ENABLE     (1 << 30)
> +#define PIPE_BOTTOM_COLOR_MASK     0x3FFFFFFF
> +#define PIPE_BOTTOM_COLOR(pipe) _MMIO_PIPE(pipe, _PIPE_BOTTOM_COLOR_A, _PIPE_BOTTOM_COLOR_B)
> +
>  /* MIPI DSI registers */
>  
>  #define _MIPI_PORT(port, a, c)	_PORT3(port, a, 0, c)	/* ports A and C only */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 836bbdc..a616ac42 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3299,6 +3299,8 @@ static void intel_update_pipe_config(struct intel_crtc *crtc,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc_state *pipe_config =
>  		to_intel_crtc_state(crtc->base.state);
> +	struct drm_rgba background = pipe_config->base.background_color;
> +	uint32_t val;
>  
>  	/* drm_atomic_helper_update_legacy_modeset_state might not be called. */
>  	crtc->base.mode = crtc->base.state->mode;
> @@ -3335,6 +3337,26 @@ static void intel_update_pipe_config(struct intel_crtc *crtc,
>  		else if (old_crtc_state->pch_pfit.enabled)
>  			ironlake_pfit_disable(crtc, true);
>  	}
> +
> +	if (INTEL_INFO(dev)->gen >= 9) {
> +		/* BGR 16bpc ==> RGB 10bpc */
> +		val = DRM_RGBA_REDBITS(background, 10) << 20
> +		    | DRM_RGBA_GREENBITS(background, 10) << 10
> +		    | DRM_RGBA_BLUEBITS(background, 10);

I'm not a fan of the drm_rgba thing having alpha in the low bits. It
goes against the existing precedent set by eg. the colorkey ioctls
where alpha (if it would be used) would be in the high bits. I think I
complained about this before already, or maybe it was also about
some BGR vs. RGB ordering thing?

Looks like drm_rgba isn't actually part of this series, and I can't see
it in the tree either, so I guess it comes in via some other series?

> +
> +		/*
> +		 * Set CSC and gamma for bottom color.
> +		 *
> +		 * FIXME:  We turn these on unconditionally for now to match
> +		 * how we've setup the various planes.  Once the color
> +		 * management framework lands, it may or may not choose to
> +		 * set these bits.
> +		 */
> +		val |= PIPE_BOTTOM_CSC_ENABLE;
> +		val |= PIPE_BOTTOM_GAMMA_ENABLE;
> +
> +		I915_WRITE(PIPE_BOTTOM_COLOR(crtc->pipe), val);
> +	}
>  }
>  
>  static void intel_fdi_normal_train(struct drm_crtc *crtc)
> @@ -12032,6 +12054,9 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  							 pipe_config);
>  	}
>  
> +	if (crtc->state->background_color.v != crtc_state->background_color.v)
> +		pipe_config->update_pipe = true;
> +
>  	return ret;
>  }
>  
> @@ -13660,6 +13685,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
>  	.set_config = drm_atomic_helper_set_config,
>  	.destroy = intel_crtc_destroy,
>  	.page_flip = intel_crtc_page_flip,
> +	.set_property = drm_atomic_helper_crtc_set_property,
>  	.atomic_duplicate_state = intel_crtc_duplicate_state,
>  	.atomic_destroy_state = intel_crtc_destroy_state,
>  };
> @@ -14254,6 +14280,20 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr
>  	scaler_state->scaler_id = -1;
>  }
>  
> +static void intel_create_background_color_property(struct drm_device *dev,
> +						   struct intel_crtc *crtc)
> +{
> +	if (!dev->mode_config.prop_background_color)
> +		dev->mode_config.prop_background_color =
> +			drm_mode_create_background_color_property(dev);
> +	if (!dev->mode_config.prop_background_color)
> +		return;
> +
> +	drm_object_attach_property(&crtc->base.base,
> +				   dev->mode_config.prop_background_color,
> +				   crtc->base.state->background_color.v);
> +}
> +
>  static void intel_crtc_init(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -14329,6 +14369,12 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
>  
>  	WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe);
> +
> +	if (INTEL_INFO(dev)->gen >= 9) {
> +		crtc_state->base.background_color = drm_rgba(16, 0, 0, 0, 0);
> +		intel_create_background_color_property(dev, intel_crtc);
> +	}
> +
>  	return;
>  
>  fail:
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel