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

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@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@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