Re: [PATCH v9] drm/i915/skl: Add support for SKL background color

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

On Fri, Feb 20, 2015 at 04:11:33PM -0800, Chandra Konduru wrote:
> This patch adds support for Skylake display pipe background color.
> 
> v2:
> - added property documentation to drm DocBook (Daniel Vetter)
> - moved property to drm_mode_config (Daniel Vetter)
> - change to set property to NULL once it is freed. (me)
> - change to make sure gamma/csc settings were retained during color change (me)
> - change to make sure background color is restored after a power event (me)
> 
> v3:
> - change to add gen check before writing background color (Matt Roper)
> 
> v4:
> - change to increase color from 8bpc to 16bpc (Daniel Vetter)
> 
> v5:
> - removed initialization of background color to 0 (Damien Lespiau)
> - added change log to commit message (Damien Lespiau)
> 
> v6:
> - optmization to use _PIPE() (Damien Lespiau)
> - updated max limit value in DocBook (Damien Lespiau)
> - simplied color conversion equation (Damien Lespiau)
> - always enable gamma and csc to align with update plane (Damien Lespiau)
> - move resume restoration code to haswell_crtc_enable (Damien Lespiau)
> 
> v7:
> - remove superfluous comments (Damien)
> - remove unused variables left over between v5 and v6 (Damien)
> 
> v8:
> - remove the destruction of the property, DRM core does that for us (Damien)
> 
> v9:
> - add ULL to a 64bits constant
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  Documentation/DocBook/drm.tmpl       |   10 ++++++-
>  drivers/gpu/drm/i915/i915_reg.h      |   10 +++++++
>  drivers/gpu/drm/i915/intel_display.c |   54 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |    2 ++
>  include/drm/drm_crtc.h               |    3 ++
>  5 files changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 249f0c9..c83ffe6 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2834,7 +2834,7 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >TBD</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="21" valign="top" >i915</td>
> +	<td rowspan="22" valign="top" >i915</td>
>  	<td rowspan="2" valign="top" >Generic</td>
>  	<td valign="top" >"Broadcast RGB"</td>
>  	<td valign="top" >ENUM</td>
> @@ -2858,6 +2858,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" >Range</td>
> +	<td valign="top" >Min=0, Max=0xFFFFFFFFFFFF</td>
> +	<td valign="top" >CRTC</td>
> +	<td valign="top" >Background color in 16bpc BGR (B-MSB, R-LSB)</td>

I'd prefer to settle on ARGB msb->lsb layout, unless there's already
precedent for something else. I think it's the most common component
order around, and so should result in least amount of code to stuff
the bits into the hardware registers.

> +	</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_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 040cd83..2d7a9f5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6758,6 +6758,16 @@ enum skl_disp_power_wells {
>  #define PIPE_CSC_POSTOFF_ME(pipe) _PIPE(pipe, _PIPE_A_CSC_POSTOFF_ME, _PIPE_B_CSC_POSTOFF_ME)
>  #define PIPE_CSC_POSTOFF_LO(pipe) _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) \
> +	_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 3b0fe9f..4dc5cb2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4413,6 +4413,20 @@ static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
>  	intel_wait_for_vblank(dev, other_active_crtc->pipe);
>  }
>  
> +static void skl_crtc_set_background_color(struct intel_crtc *intel_crtc)
> +{
> +	struct drm_device *dev = intel_crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (INTEL_INFO(dev)->gen < 9)
> +		return;
> +
> +	I915_WRITE(PIPE_BOTTOM_COLOR(intel_crtc->pipe),
> +		intel_crtc->background_color |
> +		PIPE_BOTTOM_GAMMA_ENABLE |
> +		PIPE_BOTTOM_CSC_ENABLE);
> +}
> +
>  static void haswell_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -4474,6 +4488,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  	 */
>  	intel_crtc_load_lut(crtc);
>  
> +	skl_crtc_set_background_color(intel_crtc);
> +
>  	intel_ddi_set_pipe_settings(crtc);
>  	intel_ddi_enable_transcoder_func(crtc);
>  
> @@ -9882,6 +9898,30 @@ out_hang:
>  	return ret;
>  }
>  
> +static int intel_crtc_set_property(struct drm_crtc *crtc,
> +	struct drm_property *property, uint64_t val)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	int i;
> +
> +	if (INTEL_INFO(dev)->gen >= 9) {
> +		if (property == dev->mode_config.background_color_property) {
> +			uint64_t bottom = 0;
> +
> +			/* BGR 16bpc ==> RGB 10bpc */
> +			for (i = 0; i < 3; i++)
> +				bottom |= (((val >> (i * 16 + 6)) & 0x3FF) << ((2 - i) * 10));
> +
> +			intel_crtc->background_color = (uint32_t) bottom;
> +
> +			skl_crtc_set_background_color(intel_crtc);
> +		}
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +
>  static struct drm_crtc_helper_funcs intel_helper_funcs = {
>  	.mode_set_base_atomic = intel_pipe_set_base_atomic,
>  	.load_lut = intel_crtc_load_lut,
> @@ -11660,6 +11700,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
>  	.page_flip = intel_crtc_page_flip,
>  	.atomic_duplicate_state = intel_crtc_duplicate_state,
>  	.atomic_destroy_state = intel_crtc_destroy_state,
> +	.set_property = intel_crtc_set_property,
>  };
>  
>  static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
> @@ -12341,6 +12382,19 @@ 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);
> +
> +	/* background color = 48 bit quantity: MSB BGR 16bpc LSB */
> +	if (INTEL_INFO(dev)->gen >= 9 &&
> +		!dev->mode_config.background_color_property)
> +		dev->mode_config.background_color_property =
> +			drm_property_create_range(dev, 0, "background_color", 0,
> +				0xFFFFFFFFFFFFULL);
> +
> +	if (dev->mode_config.background_color_property)
> +		drm_object_attach_property(&intel_crtc->base.base,
> +			dev->mode_config.background_color_property,
> +			intel_crtc->background_color);
> +
>  	return;
>  
>  fail:
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1de8e20..285f908 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -493,6 +493,8 @@ struct intel_crtc {
>  	struct intel_mmio_flip mmio_flip;
>  
>  	struct intel_crtc_atomic_commit atomic;
> +
> +	uint32_t background_color;
>  };
>  
>  struct intel_plane_wm_parameters {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index b1465d6..5f8c80a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1122,6 +1122,9 @@ struct drm_mode_config {
>  	struct drm_property *prop_crtc_id;
>  	struct drm_property *prop_active;
>  
> +	/* crtc properties */
> +	struct drm_property *background_color_property;
> +
>  	/* DVI-I properties */
>  	struct drm_property *dvi_i_subconnector_property;
>  	struct drm_property *dvi_i_select_subconnector_property;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> 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