[Intel-gfx] [PATCH 3/5] drm/i915: add SNB and IVB video sprite support

Daniel Vetter daniel at ffwll.ch
Tue Nov 8 22:57:03 CET 2011


On Mon, Nov 07, 2011 at 10:02:54AM -0800, Jesse Barnes wrote:
> The video sprites support various video surface formats natively and can
> handle scaling as well.  So add support for them using the new DRM core
> sprite support functions.
> 
> Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>

I've not checked the watermarks, that kind of magic eludes me ;-) A few
other comments below.

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/Makefile        |    1 +
>  drivers/gpu/drm/i915/i915_reg.h      |  123 ++++++++++
>  drivers/gpu/drm/i915/intel_display.c |   31 ++-
>  drivers/gpu/drm/i915/intel_drv.h     |   22 ++
>  drivers/gpu/drm/i915/intel_fb.c      |    6 +
>  drivers/gpu/drm/i915/intel_sprite.c  |  428 ++++++++++++++++++++++++++++++++++
>  6 files changed, 600 insertions(+), 11 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_sprite.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 0ae6a7c..808b255 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -28,6 +28,7 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o i915_mem.o \
>  	  intel_dvo.o \
>  	  intel_ringbuffer.o \
>  	  intel_overlay.o \
> +	  intel_sprite.o \
>  	  intel_opregion.o \
>  	  dvo_ch7xxx.o \
>  	  dvo_ch7017.o \
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 5a09416..b2270fa 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2450,6 +2450,8 @@
>  #define WM3_LP_ILK		0x45110
>  #define  WM3_LP_EN		(1<<31)
>  #define WM1S_LP_ILK		0x45120
> +#define WM2S_LP_IVB		0x45124
> +#define WM3S_LP_IVB		0x45128
>  #define  WM1S_LP_EN		(1<<31)
>  
>  /* Memory latency timer register */
> @@ -2666,6 +2668,127 @@
>  #define _DSPBSURF		0x7119C
>  #define _DSPBTILEOFF		0x711A4
>  
> +/* Sprite A control */
> +#define _DVSACNTR		0x72180
> +#define   DVS_ENABLE		(1<<31)
> +#define   DVS_GAMMA_ENABLE	(1<<30)
> +#define   DVS_PIXFORMAT_MASK	(3<<25)
> +#define   DVS_FORMAT_YUV422	(0<<25)
> +#define   DVS_FORMAT_RGBX101010	(1<<25)
> +#define   DVS_FORMAT_RGBX888	(2<<25)
> +#define   DVS_FORMAT_RGBX161616	(3<<25)
> +#define   DVS_SOURCE_KEY	(1<<22)
> +#define   DVS_RGB_ORDER_RGBX	(1<<20)
> +#define   DVS_YUV_BYTE_ORDER_MASK (3<<16)
> +#define   DVS_YUV_ORDER_YUYV	(0<<16)
> +#define   DVS_YUV_ORDER_UYVY	(1<<16)
> +#define   DVS_YUV_ORDER_YVYU	(2<<16)
> +#define   DVS_YUV_ORDER_VYUY	(3<<16)
> +#define   DVS_DEST_KEY		(1<<2)
> +#define   DVS_TRICKLE_FEED_DISABLE (1<<14)
> +#define   DVS_TILED		(1<<10)
> +#define _DVSASTRIDE		0x72188
> +#define _DVSAPOS		0x7218c
> +#define _DVSASIZE		0x72190
> +#define _DVSAKEYVAL		0x72194
> +#define _DVSAKEYMSK		0x72198
> +#define _DVSASURF		0x7219c
> +#define _DVSAKEYMAXVAL		0x721a0
> +#define _DVSATILEOFF		0x721a4
> +#define _DVSASURFLIVE		0x721ac
> +#define _DVSASCALE		0x72204
> +#define   DVS_SCALE_ENABLE	(1<<31)
> +#define   DVS_FILTER_MASK	(3<<29)
> +#define   DVS_FILTER_MEDIUM	(0<<29)
> +#define   DVS_FILTER_ENHANCING	(1<<29)
> +#define   DVS_FILTER_SOFTENING	(2<<29)

BSpec has some bits for deinterlace support here, maybe add them just for
documentation.

> +#define _DVSAGAMC		0x72300
> +
> +#define _DVSBCNTR		0x73180
> +#define _DVSBSTRIDE		0x73188
> +#define _DVSBPOS		0x7318c
> +#define _DVSBSIZE		0x73190
> +#define _DVSBKEYVAL		0x73194
> +#define _DVSBKEYMSK		0x73198
> +#define _DVSBSURF		0x7319c
> +#define _DVSBKEYMAXVAL		0x731a0
> +#define _DVSBTILEOFF		0x731a4
> +#define _DVSBSURFLIVE		0x731ac
> +#define _DVSBSCALE		0x73204
> +#define _DVSBGAMC		0x73300
> +
> +#define DVSCNTR(pipe) _PIPE(pipe, _DVSACNTR, _DVSBCNTR)
> +#define DVSSTRIDE(pipe) _PIPE(pipe, _DVSASTRIDE, _DVSBSTRIDE)
> +#define DVSPOS(pipe) _PIPE(pipe, _DVSAPOS, _DVSBPOS)
> +#define DVSSURF(pipe) _PIPE(pipe, _DVSASURF, _DVSBSURF)
> +#define DVSSIZE(pipe) _PIPE(pipe, _DVSASIZE, _DVSBSIZE)
> +#define DVSSCALE(pipe) _PIPE(pipe, _DVSASCALE, _DVSBSCALE)
> +#define DVSTILEOFF(pipe) _PIPE(pipe, _DVSATILEOFF, _DVSBTILEOFF)

SNB register block and it's usage looks good.

> +
> +#define _SPRA_CTL		0x70280
> +#define   SPRITE_ENABLE			(1<<31)
> +#define   SPRITE_GAMMA_ENABLE		(1<<30)
> +#define   SPRITE_PIXFORMAT_MASK		(7<<25)
> +#define   SPRITE_FORMAT_YUV422		(0<<25)
> +#define   SPRITE_FORMAT_RGBX101010	(1<<25)
> +#define   SPRITE_FORMAT_RGBX888		(2<<25)
> +#define   SPRITE_FORMAT_RGBX161616	(3<<25)
> +#define   SPRITE_FORMAT_YUV444		(4<<25)
> +#define   SPRITE_FORMAT_XBGR101010	(5<<25)

That XBRG is a bit misleading, because the X here means extended range as
opposed to the usual gl meaning of unused. It sounds like a format with
shared exponent, gl seems to use E for that, i.e E2BGR101010.

> +#define   SPRITE_CSC_ENABLE		(1<<24)
> +#define   SPRITE_SOURCE_KEY		(1<<22)
> +#define   SPRITE_RGB_ORDER_RGBX		(1<<20) /* only for 888 and 161616 */
> +#define   SPRITE_YUV_TO_RGB_CSC_DISABLE	(1<<19)
> +#define   SPRITE_YUV_CSC_FORMAT_BT709	(1<<18) /* 0 is BT601 */
> +#define   SPRITE_YUV_BYTE_ORDER_MASK	(3<<16)
> +#define   SPRITE_YUV_ORDER_YUYV		(0<<16)
> +#define   SPRITE_YUV_ORDER_UYVY		(1<<16)
> +#define   SPRITE_YUV_ORDER_YVYU		(2<<16)
> +#define   SPRITE_YUV_ORDER_VYUY		(3<<16)
> +#define   SPRITE_TRICKLE_FEED_DISABLE	(1<<14)
> +#define   SPRITE_INT_GAMMA_ENABLE	(1<<13)
> +#define   SPRITE_TILED			(1<<10)
> +#define   SPRITE_DEST_KEY		(1<<2)
> +#define _SPRA_STRIDE		0x70288
> +#define _SPRA_POS		0x7028c
> +#define _SPRA_SIZE		0x70290
> +#define _SPRA_KEYVAL		0x70294
> +#define _SPRA_KEYMSK		0x70298
> +#define _SPRA_SURF		0x7029c
> +#define _SPRA_KEYMAX		0x702a0
> +#define _SPRA_TILEOFF		0x702a4
> +#define _SPRA_SCALE		0x70304
> +#define   SPRITE_SCALE_ENABLE	(1<<31)
> +#define   SPRITE_FILTER_MASK	(3<<29)
> +#define   SPRITE_FILTER_MEDIUM	(0<<29)
> +#define   SPRITE_FILTER_ENHANCING	(1<<29)
> +#define   SPRITE_FILTER_SOFTENING	(2<<29)

Again I'd find the interlace bit definitions useful.

> +#define _SPRA_GAMC		0x70400

Gamma regs are a bit funky, especially on SNB. I assume you'll add all the
necessary defines when set_gamma support shows up.

> +#define _SPRB_CTL		0x71280
> +#define _SPRB_STRIDE		0x71288
> +#define _SPRB_POS		0x7128c
> +#define _SPRB_SIZE		0x71290
> +#define _SPRB_KEYVAL		0x71294
> +#define _SPRB_KEYMSK		0x71298
> +#define _SPRB_SURF		0x7129c
> +#define _SPRB_KEYMAX		0x712a0
> +#define _SPRB_TILEOFF		0x712a4
> +#define _SPRB_SCALE		0x71304
> +#define _SPRB_GAMC		0x71400
> +
> +#define SPRCTL(pipe) _PIPE(pipe, _SPRA_CTL, _SPRB_CTL)
> +#define SPRSTRIDE(pipe) _PIPE(pipe, _SPRA_STRIDE, _SPRB_STRIDE)
> +#define SPRPOS(pipe) _PIPE(pipe, _SPRA_POS, _SPRB_POS)
> +#define SPRSIZE(pipe) _PIPE(pipe, _SPRA_SIZE, _SPRB_SIZE)
> +#define SPRKEYVAL(pipe) _PIPE(pipe, _SPRA_KEYVAL, _SPRB_KEYVAL)
> +#define SPRKEYMSK(pipe) _PIPE(pipe, _SPRA_KEYMSK, _SPRB_KEYMSK)
> +#define SPRSURF(pipe) _PIPE(pipe, _SPRA_SURF, _SPRB_SURF)
> +#define SPRKEYMAX(pipe) _PIPE(pipe, _SPRA_KEYMAX, _SPRB_KEYMAX)
> +#define SPRTILEOFF(pipe) _PIPE(pipe, _SPRA_TILEOFF, _SPRB_TILEOFF)
> +#define SPRSCALE(pipe) _PIPE(pipe, _SPRA_SCALE, _SPRB_SCALE)
> +#define SPRGAMC(pipe) _PIPE(pipe, _SPRA_GAMC, _SPRB_GAMC)

IVB register block looks good, too.

> +
>  /* VBIOS regs */
>  #define VGACNTRL		0x71400
>  # define VGA_DISP_DISABLE			(1 << 31)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b2061b0..09228dd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -915,8 +915,8 @@ static void assert_panel_unlocked(struct drm_i915_private *dev_priv,
>  	     pipe_name(pipe));
>  }
>  
> -static void assert_pipe(struct drm_i915_private *dev_priv,
> -			enum pipe pipe, bool state)
> +void assert_pipe(struct drm_i915_private *dev_priv,
> +		 enum pipe pipe, bool state)
>  {
>  	int reg;
>  	u32 val;
> @@ -929,8 +929,6 @@ static void assert_pipe(struct drm_i915_private *dev_priv,
>  	     "pipe %c assertion failure (expected %s, current %s)\n",
>  	     pipe_name(pipe), state_string(state), state_string(cur_state));
>  }
> -#define assert_pipe_enabled(d, p) assert_pipe(d, p, true)
> -#define assert_pipe_disabled(d, p) assert_pipe(d, p, false)
>  
>  static void assert_plane_enabled(struct drm_i915_private *dev_priv,
>  				 enum plane plane)
> @@ -4439,7 +4437,8 @@ static void ironlake_update_wm(struct drm_device *dev)
>  			    ILK_LP0_CURSOR_LATENCY,
>  			    &plane_wm, &cursor_wm)) {
>  		I915_WRITE(WM0_PIPEA_ILK,
> -			   (plane_wm << WM0_PIPE_PLANE_SHIFT) | cursor_wm);
> +			   (plane_wm << WM0_PIPE_PLANE_SHIFT) |
> +			   (plane_wm << WM0_PIPE_SPRITE_SHIFT) | cursor_wm);
>  		DRM_DEBUG_KMS("FIFO watermarks For pipe A -"
>  			      " plane %d, " "cursor: %d\n",
>  			      plane_wm, cursor_wm);
> @@ -4453,7 +4452,8 @@ static void ironlake_update_wm(struct drm_device *dev)
>  			    ILK_LP0_CURSOR_LATENCY,
>  			    &plane_wm, &cursor_wm)) {
>  		I915_WRITE(WM0_PIPEB_ILK,
> -			   (plane_wm << WM0_PIPE_PLANE_SHIFT) | cursor_wm);
> +			   (plane_wm << WM0_PIPE_PLANE_SHIFT) |
> +			   (plane_wm << WM0_PIPE_SPRITE_SHIFT) | cursor_wm);
>  		DRM_DEBUG_KMS("FIFO watermarks For pipe B -"
>  			      " plane %d, cursor: %d\n",
>  			      plane_wm, cursor_wm);
> @@ -4521,7 +4521,8 @@ static void sandybridge_update_wm(struct drm_device *dev)
>  			    &sandybridge_cursor_wm_info, latency,
>  			    &plane_wm, &cursor_wm)) {
>  		I915_WRITE(WM0_PIPEA_ILK,
> -			   (plane_wm << WM0_PIPE_PLANE_SHIFT) | cursor_wm);
> +			   (plane_wm << WM0_PIPE_PLANE_SHIFT) |
> +			   (plane_wm << WM0_PIPE_SPRITE_SHIFT) | cursor_wm);
>  		DRM_DEBUG_KMS("FIFO watermarks For pipe A -"
>  			      " plane %d, " "cursor: %d\n",
>  			      plane_wm, cursor_wm);
> @@ -4533,7 +4534,8 @@ static void sandybridge_update_wm(struct drm_device *dev)
>  			    &sandybridge_cursor_wm_info, latency,
>  			    &plane_wm, &cursor_wm)) {
>  		I915_WRITE(WM0_PIPEB_ILK,
> -			   (plane_wm << WM0_PIPE_PLANE_SHIFT) | cursor_wm);
> +			   (plane_wm << WM0_PIPE_PLANE_SHIFT) |
> +			   (plane_wm << WM0_PIPE_SPRITE_SHIFT) | cursor_wm);
>  		DRM_DEBUG_KMS("FIFO watermarks For pipe B -"
>  			      " plane %d, cursor: %d\n",
>  			      plane_wm, cursor_wm);
> @@ -4547,7 +4549,8 @@ static void sandybridge_update_wm(struct drm_device *dev)
>  			    &sandybridge_cursor_wm_info, latency,
>  			    &plane_wm, &cursor_wm)) {
>  		I915_WRITE(WM0_PIPEC_IVB,
> -			   (plane_wm << WM0_PIPE_PLANE_SHIFT) | cursor_wm);
> +			   (plane_wm << WM0_PIPE_PLANE_SHIFT) |
> +			   (plane_wm << WM0_PIPE_SPRITE_SHIFT) | cursor_wm);
>  		DRM_DEBUG_KMS("FIFO watermarks For pipe C -"
>  			      " plane %d, cursor: %d\n",
>  			      plane_wm, cursor_wm);
> @@ -7581,7 +7584,7 @@ int intel_framebuffer_init(struct drm_device *dev,
>  	if (obj->tiling_mode == I915_TILING_Y)
>  		return -EINVAL;
>  
> -	if (mode_cmd->pitch & 63)
> +	if (mode_cmd->pitches[0] & 63)
>  		return -EINVAL;
>  
>  	switch (mode_cmd->pixel_format) {
> @@ -8693,7 +8696,7 @@ static void i915_disable_vga(struct drm_device *dev)
>  void intel_modeset_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int i;
> +	int i, ret;
>  
>  	drm_mode_config_init(dev);
>  
> @@ -8723,6 +8726,12 @@ void intel_modeset_init(struct drm_device *dev)
>  
>  	for (i = 0; i < dev_priv->num_pipe; i++) {
>  		intel_crtc_init(dev, i);
> +		if (HAS_PCH_SPLIT(dev)) {
> +			ret = intel_plane_init(dev, i);
> +			if (ret)
> +				DRM_ERROR("plane %d init failed: %d\n",
> +					  i, ret);
> +		}
>  	}
>  
>  	/* Just disable it once at startup */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 23c5622..6284562 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -176,10 +176,26 @@ struct intel_crtc {
>  	bool use_pll_a;
>  };
>  
> +struct intel_plane {
> +	struct drm_plane base;
> +	enum pipe pipe;
> +	struct drm_i915_gem_object *obj;
> +	int max_downscale;
> +	u32 lut_r[1024], lut_g[1024], lut_b[1024];
> +	void (*update_plane)(struct drm_plane *plane,
> +			     struct drm_framebuffer *fb, unsigned long start,

start is a strange name for gtt_offset/fb_base/... I'd just pass the
i915_gem_object.

> +			     int crtc_x, int crtc_y,
> +			     unsigned int crtc_w, unsigned int crtc_h,
> +			     uint32_t x, uint32_t y,
> +			     uint32_t src_w, uint32_t src_h);
> +	void (*disable_plane)(struct drm_plane *plane);
> +};
> +
>  #define to_intel_crtc(x) container_of(x, struct intel_crtc, base)
>  #define to_intel_connector(x) container_of(x, struct intel_connector, base)
>  #define to_intel_encoder(x) container_of(x, struct intel_encoder, base)
>  #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
> +#define to_intel_plane(x) container_of(x, struct intel_plane, base)
>  
>  #define DIP_HEADER_SIZE	5
>  
> @@ -289,6 +305,7 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
>  extern bool intel_dpd_is_edp(struct drm_device *dev);
>  extern void intel_edp_link_config(struct intel_encoder *, int *, int *);
>  extern bool intel_encoder_is_pch_edp(struct drm_encoder *encoder);
> +extern int intel_plane_init(struct drm_device *dev, enum pipe pipe);
>  
>  /* intel_panel.c */
>  extern void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
> @@ -379,6 +396,11 @@ extern int intel_overlay_attrs(struct drm_device *dev, void *data,
>  extern void intel_fb_output_poll_changed(struct drm_device *dev);
>  extern void intel_fb_restore_mode(struct drm_device *dev);
>  
> +extern void assert_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
> +			bool state);
> +#define assert_pipe_enabled(d, p) assert_pipe(d, p, true)
> +#define assert_pipe_disabled(d, p) assert_pipe(d, p, false)
> +
>  extern void intel_init_clock_gating(struct drm_device *dev);
>  extern void intel_write_eld(struct drm_encoder *encoder,
>  			    struct drm_display_mode *mode);
> diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
> index dc1db4f..068b086 100644
> --- a/drivers/gpu/drm/i915/intel_fb.c
> +++ b/drivers/gpu/drm/i915/intel_fb.c
> @@ -270,8 +270,14 @@ void intel_fb_restore_mode(struct drm_device *dev)
>  {
>  	int ret;
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> +	struct drm_mode_config *config = &dev->mode_config;
> +	struct drm_plane *plane;
>  
>  	ret = drm_fb_helper_restore_fbdev_mode(&dev_priv->fbdev->helper);
>  	if (ret)
>  		DRM_DEBUG("failed to restore crtc mode\n");
> +
> +	/* Be sure to shut off any planes that may be active */
> +	list_for_each_entry(plane, &config->plane_list, head)
> +		plane->funcs->disable_plane(plane);

This should be part of the fb_helper above.

>  }
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> new file mode 100644
> index 0000000..f458921
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -0,0 +1,428 @@
> +/*
> + * Copyright © 2011 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + *
> + * Authors:
> + *   Jesse Barnes <jbarnes at virtuousgeek.org>
> + *
> + * New plane/sprite handling.
> + *
> + * The older chips had a separate interface for programming plane related
> + * registers; newer ones are much simpler and we can use the new DRM plane
> + * support.
> + */
> +#include "drmP.h"
> +#include "drm_crtc.h"
> +#include "intel_drv.h"
> +#include "i915_drm.h"
> +#include "i915_drv.h"
> +
> +/*
> + * Note on refcounting:
> + * When the user creates an fb for the GEM object to be used for the plane,
> + * a ref is taken on the object.  However, if the application exits before
> + * disabling the plane, the DRM close handling will free all the fbs and
> + * unless we take a ref on the object, it will be destroyed before the
> + * plane disable hook is called, causing obvious trouble with our efforts
> + * to look up and unpin the object.  So we take a ref after we move the
> + * object to the display plane so it won't be destroyed until our disable
> + * hook is called and we drop our private reference.
> + */

Actually, this is wrong. Before the fb gets destroyed we call
drm_framebuffer_cleanup which takes care of this problem. The fact that
- currently drivers call this instead of the drm core
- it's a ducttape solution instead of refcounting
doesn't really make it nice, but it works.

Aside: The insane refcoungting dance in page_flip because the drm core
doesn't know that we still need the to-be-flipped-out fb, it forgets about
it right away. Maybe that's the reason for the above confusion.

> +
> +static void
> +ivb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
> +		 unsigned long start, int crtc_x, int crtc_y,
> +		 unsigned int crtc_w, unsigned int crtc_h,
> +		 uint32_t x, uint32_t y,
> +		 uint32_t src_w, uint32_t src_h)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	int pipe = intel_plane->pipe;
> +	u32 sprctl, sprscale = 0;
> +	u32 reg = SPRCTL(pipe);
> +
> +	sprctl = I915_READ(reg);

reg isn't really a great var name. Imo just drop it, only used twice and
reduces readability.

> +
> +	/* Mask out pixel format bits in case we change it */
> +	sprctl &= ~(SPRITE_DEST_KEY | SPRITE_SOURCE_KEY);
> +	sprctl &= ~SPRITE_PIXFORMAT_MASK;
> +	sprctl &= ~SPRITE_RGB_ORDER_RGBX;
> +	sprctl &= ~SPRITE_YUV_BYTE_ORDER_MASK;
> +
> +	switch (fb->pixel_format) {
> +	case V4L2_PIX_FMT_BGR32:
> +		sprctl |= SPRITE_FORMAT_RGBX888;
> +		break;
> +	case V4L2_PIX_FMT_RGB32:
> +		sprctl |= SPRITE_FORMAT_RGBX888 | SPRITE_RGB_ORDER_RGBX;
> +		break;
> +	case V4L2_PIX_FMT_YUYV:
> +		sprctl |= SPRITE_FORMAT_YUV422 | SPRITE_YUV_ORDER_YUYV;
> +		break;
> +	case V4L2_PIX_FMT_YVYU:
> +		sprctl |= SPRITE_FORMAT_YUV422 | SPRITE_YUV_ORDER_YVYU;
> +		break;
> +	case V4L2_PIX_FMT_UYVY:
> +		sprctl |= SPRITE_FORMAT_YUV422 | SPRITE_YUV_ORDER_UYVY;
> +		break;
> +	case V4L2_PIX_FMT_VYUY:
> +		sprctl |= SPRITE_FORMAT_YUV422 | SPRITE_YUV_ORDER_VYUY;
> +		break;
> +	default:
> +		DRM_DEBUG_DRIVER("bad pixel format, assuming RGBX888\n");
> +		sprctl |= DVS_FORMAT_RGBX888;
> +		break;
> +	}
> +
> +	sprctl |= SPRITE_TILED;
> +
> +	/* must disable */
> +	sprctl |= SPRITE_TRICKLE_FEED_DISABLE;
> +	sprctl |= SPRITE_ENABLE;
> +
> +	/* Sizes are 0 based */
> +	src_w--;
> +	src_h--;
> +	crtc_w--;
> +	crtc_h--;
> +
> +	/* Adjust watermarks as needed */
> +	I915_WRITE(WM1S_LP_ILK, 0x100);
> +	I915_WRITE(WM2S_LP_IVB, 0x100);
> +	I915_WRITE(WM3S_LP_IVB, 0x100);
> +
> +	if (crtc_w != src_w || crtc_h != src_h)
> +		sprscale = SPRITE_SCALE_ENABLE | (src_h << 16) | src_w;
> +
> +	I915_WRITE(SPRSTRIDE(pipe), fb->pitch);
> +	I915_WRITE(SPRPOS(pipe), (crtc_y << 16) | crtc_x);
> +	I915_WRITE(SPRTILEOFF(pipe), (y << 16) | x);
> +	I915_WRITE(SPRSIZE(pipe), (crtc_h << 16) | crtc_w);
> +	I915_WRITE(SPRSCALE(pipe), sprscale);
> +	I915_WRITE(reg, sprctl);
> +	I915_WRITE(SPRSURF(pipe), start);
> +	POSTING_READ(SPRSURF(pipe));
> +}
> +
> +static void
> +ivb_disable_plane(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	int pipe = intel_plane->pipe;
> +
> +	I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE);
> +	I915_WRITE(SPRSCALE(pipe), 0);
> +	I915_WRITE(SPRSURF(pipe), 0);

Is that required or just paranoia? I haven't found anything in bspec
suggesting it's required.

> +	POSTING_READ(SPRSURF(pipe));
> +	intel_wait_for_vblank(dev, pipe);
> +}
> +
> +static void
> +snb_update_plane(struct drm_plane *plane, struct drm_framebuffer *fb,
> +		 unsigned long start, int crtc_x, int crtc_y,
> +		 unsigned int crtc_w, unsigned int crtc_h,
> +		 uint32_t x, uint32_t y,
> +		 uint32_t src_w, uint32_t src_h)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	int pipe = intel_plane->pipe;
> +	u32 dvscntr, dvsscale = 0;
> +	u32 reg = DVSCNTR(pipe);
> +
> +	dvscntr = I915_READ(reg);

dito about reg.

> +
> +	/* Mask out pixel format bits in case we change it */
> +	dvscntr &= ~(DVS_DEST_KEY | DVS_SOURCE_KEY);
> +	dvscntr &= ~DVS_PIXFORMAT_MASK;
> +	dvscntr &= ~DVS_RGB_ORDER_RGBX;
> +	dvscntr &= ~DVS_YUV_BYTE_ORDER_MASK;
> +
> +	switch (fb->pixel_format) {
> +	case V4L2_PIX_FMT_BGR32:
> +		dvscntr |= DVS_FORMAT_RGBX888;
> +		break;
> +	case V4L2_PIX_FMT_RGB32:
> +		dvscntr |= DVS_FORMAT_RGBX888 | DVS_RGB_ORDER_RGBX;
> +		break;
> +	case V4L2_PIX_FMT_YUYV:
> +		dvscntr |= DVS_FORMAT_YUV422 | DVS_YUV_ORDER_YUYV;
> +		break;
> +	case V4L2_PIX_FMT_YVYU:
> +		dvscntr |= DVS_FORMAT_YUV422 | DVS_YUV_ORDER_YVYU;
> +		break;
> +	case V4L2_PIX_FMT_UYVY:
> +		dvscntr |= DVS_FORMAT_YUV422 | DVS_YUV_ORDER_UYVY;
> +		break;
> +	case V4L2_PIX_FMT_VYUY:
> +		dvscntr |= DVS_FORMAT_YUV422 | DVS_YUV_ORDER_VYUY;
> +		break;
> +	default:
> +		DRM_DEBUG_DRIVER("bad pixel format, assuming RGBX888\n");
> +		dvscntr |= DVS_FORMAT_RGBX888;
> +		break;
> +	}
> +
> +	dvscntr |= DVS_TILED;
> +
> +	/* must disable */
> +	dvscntr |= DVS_TRICKLE_FEED_DISABLE;
> +	dvscntr |= DVS_ENABLE;
> +
> +	/* Sizes are 0 based */
> +	src_w--;
> +	src_h--;
> +	crtc_w--;
> +	crtc_h--;
> +
> +	if (crtc_w != src_w || crtc_h != src_h)
> +		dvsscale = DVS_SCALE_ENABLE | (src_h << 16) | src_w;
> +
> +	I915_WRITE(DVSSTRIDE(pipe), fb->pitch);
> +	I915_WRITE(DVSPOS(pipe), (crtc_y << 16) | crtc_x);
> +	I915_WRITE(DVSTILEOFF(pipe), (y << 16) | x);
> +	I915_WRITE(DVSSIZE(pipe), (crtc_h << 16) | crtc_w);
> +	I915_WRITE(DVSSCALE(pipe), dvsscale);
> +	I915_WRITE(reg, dvscntr);
> +	I915_WRITE(DVSSURF(pipe), start);
> +	POSTING_READ(DVSSURF(pipe));
> +}
> +
> +static void
> +snb_disable_plane(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	int pipe = intel_plane->pipe;
> +
> +	I915_WRITE(DVSCNTR(pipe), I915_READ(DVSCNTR(pipe)) & ~DVS_ENABLE);
> +	I915_WRITE(DVSSCALE(pipe), 0);
> +	I915_WRITE(DVSSURF(pipe), 0);

dito about paranoia

> +	POSTING_READ(DVSSURF(pipe));
> +	intel_wait_for_vblank(dev, pipe);
> +}
> +
> +static int
> +intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> +		   struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> +		   unsigned int crtc_w, unsigned int crtc_h,
> +		   uint32_t src_x, uint32_t src_y,
> +		   uint32_t src_w, uint32_t src_h)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_framebuffer *intel_fb;
> +	struct drm_i915_gem_object *obj, *old_obj;
> +	int pipe = intel_plane->pipe;
> +	unsigned long start;
> +	int ret = 0;
> +	int x = src_x >> 16, y = src_y >> 16;
> +	int primary_w = crtc->mode.hdisplay, primary_h = crtc->mode.vdisplay;
> +	bool disable_primary = false;
> +
> +	intel_fb = to_intel_framebuffer(fb);
> +	obj = intel_fb->obj;
> +
> +	old_obj = intel_plane->obj;
> +
> +	/* Pipe must be running... */
> +	if (!(I915_READ(PIPECONF(pipe)) & PIPECONF_ENABLE))
> +		return -EINVAL;

Ok, how does this tie in with dpms handling? This here feels like a major
cludge ... I suspect we want a dpms function on the plane that gets called
by the common dpms helper before switching of the crtc and after switching
it on. Or at least some driver-hack like I've done for the overlay.

> +
> +	if (crtc_x >= primary_w || crtc_y >= primary_h)
> +		return -EINVAL;
> +
> +	/* Don't modify another pipe's plane */
> +	if (intel_plane->pipe != intel_crtc->pipe)
> +		return -EINVAL;
> +
> +	/*
> +	 * Clamp the width & height into the visible area.  Note we don't
> +	 * try to scale the source if part of the visible region is offscreen.
> +	 * The caller must handle that by adjusting source offset and size.
> +	 */

Allowing the crtc dest rect to extend beyond that of the crtc and then refusing to
properly adjust tiling is a bit inconsistent. I call design-by-committee
on this one ;-)

If the goal is that this plane stuff is somewhat generically useable, I
think this needs to be fixed. If the goal is simply to keep the bubble
intact, I don't mind this as-is, because I don't care ;-)

> +	if (crtc_x < 0) {
> +		crtc_w += crtc_x;
> +		crtc_x = 0;
> +	}
> +	if (crtc_x + crtc_w > primary_w)
> +		crtc_w = primary_w - crtc_x;
> +
> +	if (crtc_y < 0) {
> +		crtc_h += crtc_y;
> +		crtc_y = 0;
> +	}
> +	if (crtc_y + crtc_h > primary_h)
> +		crtc_h = primary_h - crtc_y;
> +
> +	/*
> +	 * We can take a larger source and scale it down, but
> +	 * only so much...  16x is the max on SNB.
> +	 */
> +	if (((src_w * src_h) / (crtc_w * crtc_h)) > intel_plane->max_downscale)
> +		return -EINVAL;
> +
> +	/*
> +	 * If the sprite is completely covering the primary plane,
> +	 * we can disable the primary and save power.
> +	 */
> +	if ((crtc_x == 0) && (crtc_y == 0) &&
> +	    (crtc_w == primary_w) && (crtc_h == primary_h))
> +		disable_primary = true;
> +
> +	mutex_lock(&dev->struct_mutex);
> +
> +	if (obj->tiling_mode != I915_TILING_X) {
> +		DRM_ERROR("plane surfaces must be X tiled\n");
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}

Afaics the hw supports untiled buffers (you need to frob the linear offset
register instead of the x/y register for tiled surfaces). Does that not
work?

> +
> +	ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> +	if (ret) {
> +		DRM_ERROR("failed to pin object\n");
> +		goto out_unlock;
> +	}
> +
> +	drm_gem_object_reference(&obj->base);
> +
> +	intel_plane->obj = obj;
> +
> +	start = obj->gtt_offset;

Passing obj instead of start would also make untiled sprite easier ...

> +
> +	intel_plane->update_plane(plane, fb, start, crtc_x, crtc_y,
> +				  crtc_w, crtc_h, x, y, src_w, src_h);
> +
> +	/* Unpin old obj after new one is active to avoid ugliness */
> +	if (old_obj) {
> +		/*
> +		 * It's fairly common to simply update the position of
> +		 * an existing object.  In that case, we don't need to
> +		 * wait for vblank to avoid ugliness, we only need to
> +		 * do the pin & ref bookkeeping.
> +		 */
> +		if (old_obj != obj)
> +			intel_wait_for_vblank(dev, to_intel_crtc(crtc)->pipe);
> +		i915_gem_object_unpin(old_obj);
> +		drm_gem_object_unreference(&old_obj->base);
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&dev->struct_mutex);

Minor locking nitpick: You only need to hold around pin and unpin, not
over the vblank. Stopping gem for 20 msec just to show a sprite is not so
great. I know, our locking isn't really great in general and there are
many other places where we stall in similarly bad ways, still ...

> +
> +	return ret;
> +}
> +
> +static int
> +intel_disable_plane(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	int ret = 0;
> +
> +	mutex_lock(&dev->struct_mutex);
> +
> +	intel_plane->disable_plane(plane);

Again, move the vblank_wait which is inside the disable_plane outside of
the struct_mutex lock. We really need some generic infrastructure to run
something after the next vblank in a workqueu. Would clean up pageflip,
the legacy overlay and kill the vblank here ...

> +
> +	if (!intel_plane->obj)
> +		goto out_unlock;
> +
> +	ret = i915_gem_object_finish_gpu(intel_plane->obj);
> +	if (ret)
> +		goto out_unlock;

What's the reason for that finish_gpu here?

> +
> +	i915_gem_object_unpin(intel_plane->obj);
> +
> +	drm_gem_object_unreference(&intel_plane->obj->base);
> +
> +out_unlock:
> +	intel_plane->obj = NULL;
> +
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return ret;
> +}
> +
> +static void intel_destroy_plane(struct drm_plane *plane)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	intel_disable_plane(plane);
> +	drm_plane_cleanup(plane);
> +	kfree(intel_plane);
> +}
> +
> +static const struct drm_plane_funcs intel_plane_funcs = {
> +	.update_plane = intel_update_plane,
> +	.disable_plane = intel_disable_plane,
> +	.destroy = intel_destroy_plane,
> +};
> +
> +static uint32_t snb_plane_formats[] = {
> +	V4L2_PIX_FMT_BGR32,
> +	V4L2_PIX_FMT_RGB32,
> +	V4L2_PIX_FMT_YUYV,
> +	V4L2_PIX_FMT_YVYU,
> +	V4L2_PIX_FMT_UYVY,
> +	V4L2_PIX_FMT_VYUY,
> +};
> +
> +int
> +intel_plane_init(struct drm_device *dev, enum pipe pipe)
> +{
> +	struct intel_plane *intel_plane;
> +	unsigned long possible_crtcs;
> +
> +	if (!(IS_GEN6(dev) || IS_GEN7(dev))) {
> +		DRM_ERROR("new plane code only for SNB+\n");
> +		return -ENODEV;
> +	}
> +
> +	intel_plane = kzalloc(sizeof(struct intel_plane), GFP_KERNEL);
> +	if (!intel_plane)
> +		return -ENOMEM;
> +
> +	if (IS_GEN6(dev)) {
> +		intel_plane->max_downscale = 16;
> +		intel_plane->update_plane = snb_update_plane;
> +		intel_plane->disable_plane = snb_disable_plane;
> +	} else if (IS_GEN7(dev)) {
> +		intel_plane->max_downscale = 2;
> +		intel_plane->update_plane = ivb_update_plane;
> +		intel_plane->disable_plane = ivb_disable_plane;
> +	}
> +
> +	intel_plane->pipe = pipe;
> +	possible_crtcs = (1 << pipe);
> +	drm_plane_init(dev, &intel_plane->base, possible_crtcs,
> +		       &intel_plane_funcs, snb_plane_formats,
> +		       ARRAY_SIZE(snb_plane_formats));
> +
> +	return 0;
> +}
> +
> -- 
> 1.7.4.1
-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48



More information about the Intel-gfx mailing list