[PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.

[PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.

[Sandeep Ramankutty]

Change: Add support to change the pixel format, base surface address, tiling mode, tiling
offset on the flow during the primary plane flip.

Issue: This support is needed by hardware composer to meet the performance
optimization requirement for 3D benchmark. This patch enables change in
pixel format and tiling params without adding and removing the framebuffer.
Adding and removing the framebuffer impacts performance.

Change Details-
drm: Defined function pointer set_pixelformat in drm_crtc_funcs. drm_crtc.c is modified
to execute the function to update pixel format.
drm/i915: intel_crtc_set_pixel_format() implemented in i915 driver code (intel_display.c)
and set to drm_crtc_funcs->set_pixelformat.
haswell_set_pixelformat() implemented to update pixel format. This is specific to Haswell
haswell_update_plane() implemented to update changes in tiling offset, surface
base address and stride-fb->pitches[0]. Sends MI command to update the params
in primary plane control registers.

Change-Id: I4b0af83f641b2b79ee4f5c3401790d7d90ccd9ef
Signed-off-by: Pallavi G <pallavi.g@intel.com>
Signed-off-by: Sandeep Ramakutty <sandeepx.ramakutty@intel.com>
---
 drivers/gpu/drm/drm_crtc.c           |   15 +++-
 drivers/gpu/drm/i915/intel_display.c |  156 ++++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h               |    8 ++
 3 files changed, 176 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index c9f9f3d..4739b6c 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3518,9 +3518,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	}
 
 	if (crtc->fb->pixel_format != fb->pixel_format) {
-		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
-		ret = -EINVAL;
-		goto out;
+		if (crtc->funcs->set_pixelformat == NULL) {
+			DRM_DEBUG_KMS("Pixel format change not allowed");
+			ret = -EINVAL;
+			goto out;
+		}
+		/* supports dynamic change in pixel format */
+		ret = crtc->funcs->set_pixelformat(crtc, fb->pixel_format);
+		if (ret) {
+			DRM_DEBUG_KMS("Pixel format change failed %d",
+							fb->pixel_format);
+			goto out;
+		}
 	}
 
 	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 955df91..7bfe088 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -45,6 +45,7 @@ bool intel_pipe_has_type(struct drm_crtc *crtc, int type);
 static void intel_increase_pllclock(struct drm_crtc *crtc);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
 
+
 typedef struct {
 	int	min, max;
 } intel_range_t;
@@ -1959,6 +1960,128 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 	return 0;
 }
 
+/* Set Pixel format for Haswell using MI commands */
+static int haswell_set_pixelformat(struct drm_crtc *crtc, u32 pixel_format)
+{
+	u32 dspcntr, reg;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
+	int ret = 0;
+
+	reg = DSPCNTR(intel_crtc->pipe);
+	dspcntr = I915_READ(reg);
+	DRM_DEBUG_DRIVER("pixel format = %d\n", pixel_format);
+	/* Mask out pixel format bits in case we change it */
+	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
+	switch (pixel_format) {
+	case DRM_FORMAT_C8:
+		dspcntr |= DISPPLANE_8BPP;
+		break;
+	case DRM_FORMAT_XRGB1555:
+	case DRM_FORMAT_ARGB1555:
+		dspcntr |= DISPPLANE_BGRX555;
+		break;
+	case DRM_FORMAT_RGB565:
+		dspcntr |= DISPPLANE_BGRX565;
+		break;
+	case DRM_FORMAT_XRGB8888:
+	case DRM_FORMAT_ARGB8888:
+		dspcntr |= DISPPLANE_BGRX888;
+		break;
+	case DRM_FORMAT_XBGR8888:
+	case DRM_FORMAT_ABGR8888:
+		dspcntr |= DISPPLANE_RGBX888;
+		break;
+	case DRM_FORMAT_XRGB2101010:
+	case DRM_FORMAT_ARGB2101010:
+		dspcntr |= DISPPLANE_BGRX101010;
+		break;
+	case DRM_FORMAT_XBGR2101010:
+	case DRM_FORMAT_ABGR2101010:
+		dspcntr |= DISPPLANE_RGBX101010;
+		break;
+	default:
+		DRM_ERROR("Unsupported pixel format 0x%08x\n", pixel_format);
+		return -EINVAL;
+	}
+
+	/* Write pixel format update command to ring */
+	ret = intel_ring_begin(ring, 4);
+	if (ret) {
+		DRM_ERROR("MI Command emit failed.\n");
+		return ret;
+	}
+	intel_ring_emit(ring, MI_NOOP);
+	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
+	intel_ring_emit(ring, reg);
+	intel_ring_emit(ring, dspcntr);
+	intel_ring_advance(ring);
+	return 0;
+}
+
+/* Update the primary plane registers. Uses MI commands */
+static int haswell_update_plane(struct drm_crtc *crtc,
+				struct drm_framebuffer *fb, int x, int y)
+{
+	int ret;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
+	struct drm_i915_gem_object *obj;
+	int plane = intel_crtc->plane;
+
+	switch (plane) {
+	case 0:
+	case 1:
+	case 2:
+		break;
+	default:
+		DRM_ERROR("Can't update plane %c\n", plane_name(plane));
+		return -EINVAL;
+	}
+	obj = to_intel_framebuffer(fb)->obj;
+
+	/* Set pixel format */
+	haswell_set_pixelformat(crtc, fb->pixel_format);
+
+	/* Set tiling offsets. Tiling mode is not set here as    *
+	* it is set from intel_gen7_queue_flip. Send MI Command  *
+	* to change -                                            *
+	* 1. Tiling offset                                       *
+	* 2. stride - fb->pitches[0]                             *
+	* 2. surface base address                                *
+	* Linear offset and tile offset is same for Haswell      */
+	intel_crtc->dspaddr_offset =
+		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
+						fb->bits_per_pixel / 8,
+						fb->pitches[0]);
+
+	DRM_DEBUG_KMS("Writing base %08X %08lX %d %d %d\n",
+			obj->gtt_offset, intel_crtc->dspaddr_offset,
+			x, y, fb->pitches[0]);
+
+	/* Emit MI commands here */
+	ret = intel_ring_begin(ring, 10);
+	if (ret)
+		return ret;
+	intel_ring_emit(ring, MI_NOOP);
+	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
+	intel_ring_emit(ring, DSPOFFSET(plane));
+	intel_ring_emit(ring, (y << 16) | x);
+	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
+	intel_ring_emit(ring, DSPSTRIDE(plane));
+	intel_ring_emit(ring, fb->pitches[0]);
+	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
+	intel_ring_emit(ring, DSPSURF(plane));
+	intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
+	intel_ring_advance(ring);
+
+	return 0;
+}
+
 static int ironlake_update_plane(struct drm_crtc *crtc,
 				 struct drm_framebuffer *fb, int x, int y)
 {
@@ -7494,9 +7617,26 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	struct drm_framebuffer *old_fb = crtc->fb;
 	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_framebuffer *intel_fb;
 	struct intel_unpin_work *work;
 	unsigned long flags;
 	int ret;
+	/*
+	* TILEOFF registers can't be changed via MI display flips.
+	* Changes in pitches[0] and offsets[0] updates the primary plane
+	* control registers.
+	*/
+	intel_fb = to_intel_framebuffer(crtc->fb);
+	if ((IS_HASWELL(dev)) &&
+		((obj->tiling_mode != intel_fb->obj->tiling_mode) ||
+		(fb->offsets[0] != crtc->fb->offsets[0]) ||
+		(fb->pitches[0] != crtc->fb->pitches[0]))) {
+			DRM_DEBUG_DRIVER(" crtc fb: pitch = %d offset = %d\n",
+			crtc->fb->pitches[0], crtc->fb->offsets[0]);
+			DRM_DEBUG_DRIVER(" input fb: pitch = %d offset = %d\n",
+			fb->pitches[0], fb->offsets[0]);
+			haswell_update_plane(crtc, fb, 0, 0);
+	}
 
 	work = kzalloc(sizeof *work, GFP_KERNEL);
 	if (work == NULL)
@@ -8766,6 +8906,20 @@ out_config:
 	return ret;
 }
 
+/* Callback function - Called if change in pixel format is detected.
+* Sends MI command to update change in pixel format.
+*/
+static int intel_crtc_set_pixel_format(struct drm_crtc *crtc, u32 pixel_format)
+{
+	struct drm_device *dev = crtc->dev;
+	if (IS_HASWELL(dev)) {
+		return haswell_set_pixelformat(crtc, pixel_format);
+	} else {
+		DRM_ERROR("Pixel format change not allowed.\n");
+		return -EINVAL;
+	}
+}
+
 static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.cursor_set = intel_crtc_cursor_set,
 	.cursor_move = intel_crtc_cursor_move,
@@ -8773,6 +8927,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.set_config = intel_crtc_set_config,
 	.destroy = intel_crtc_destroy,
 	.page_flip = intel_crtc_page_flip,
+	.set_pixelformat = intel_crtc_set_pixel_format,
 };
 
 static void intel_cpu_pll_init(struct drm_device *dev)
@@ -10180,3 +10335,4 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
 	}
 }
 #endif
+
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e215bcc..9a0ea92 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -323,6 +323,7 @@ struct drm_plane;
  * @set_property: called when a property is changed
  * @set_config: apply a new CRTC configuration
  * @page_flip: initiate a page flip
+ * @set_pixelformat: apply new pixel format to primary plane control register
  *
  * The drm_crtc_funcs structure is the central CRTC management structure
  * in the DRM.  Each CRTC controls one or more connectors (note that the name
@@ -369,6 +370,13 @@ struct drm_crtc_funcs {
 
 	int (*set_property)(struct drm_crtc *crtc,
 			    struct drm_property *property, uint64_t val);
+	/*
+	* Update the primary plane pixel format register during page flip.
+	* To support dynamic change in pixel format define the callback
+	* function for set_pixelformat.
+	*/
+	int (*set_pixelformat)(struct drm_crtc *crtc,
+				uint32_t pixel_format);
 };
 
 /**
-- 
1.7.9.5

Re: [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.

[Ville Syrjälä]

On Fri, Sep 13, 2013 at 07:05:31PM +0530, Sandeep Ramankutty wrote:
> Change: Add support to change the pixel format, base surface address, tiling mode, tiling
> offset on the flow during the primary plane flip.
> 
> Issue: This support is needed by hardware composer to meet the performance
> optimization requirement for 3D benchmark. This patch enables change in
> pixel format and tiling params without adding and removing the framebuffer.
> Adding and removing the framebuffer impacts performance.

We can't do it quite like this unfortunately. Watermarksm may need to
be updated carefully if the bpp changes. Also updating multiple plane
registers isn't always atomic, so we anyway need the vblank evade
tricks to make it look pretty. I suppose it might be possible to make
all that work via the ring, but my plan is to go for MMIO, and later
investigate if we can optimize it by using the ring.

As far as the lowlevel details go, you supposedly need a SRM after
each LRI aimed at the display registers. Otherwise I guess you could
even blast all the registers with a single LRI.

> Change Details-
> drm: Defined function pointer set_pixelformat in drm_crtc_funcs. drm_crtc.c is modified
> to execute the function to update pixel format.
> drm/i915: intel_crtc_set_pixel_format() implemented in i915 driver code (intel_display.c)
> and set to drm_crtc_funcs->set_pixelformat.
> haswell_set_pixelformat() implemented to update pixel format. This is specific to Haswell
> haswell_update_plane() implemented to update changes in tiling offset, surface
> base address and stride-fb->pitches[0]. Sends MI command to update the params
> in primary plane control registers.
> 
> Change-Id: I4b0af83f641b2b79ee4f5c3401790d7d90ccd9ef
> Signed-off-by: Pallavi G <pallavi.g@intel.com>
> Signed-off-by: Sandeep Ramakutty <sandeepx.ramakutty@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c           |   15 +++-
>  drivers/gpu/drm/i915/intel_display.c |  156 ++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h               |    8 ++
>  3 files changed, 176 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index c9f9f3d..4739b6c 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3518,9 +3518,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	}
>  
>  	if (crtc->fb->pixel_format != fb->pixel_format) {
> -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> -		ret = -EINVAL;
> -		goto out;
> +		if (crtc->funcs->set_pixelformat == NULL) {
> +			DRM_DEBUG_KMS("Pixel format change not allowed");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		/* supports dynamic change in pixel format */
> +		ret = crtc->funcs->set_pixelformat(crtc, fb->pixel_format);
> +		if (ret) {
> +			DRM_DEBUG_KMS("Pixel format change failed %d",
> +							fb->pixel_format);
> +			goto out;
> +		}
>  	}
>  
>  	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 955df91..7bfe088 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -45,6 +45,7 @@ bool intel_pipe_has_type(struct drm_crtc *crtc, int type);
>  static void intel_increase_pllclock(struct drm_crtc *crtc);
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
>  
> +
>  typedef struct {
>  	int	min, max;
>  } intel_range_t;
> @@ -1959,6 +1960,128 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  	return 0;
>  }
>  
> +/* Set Pixel format for Haswell using MI commands */
> +static int haswell_set_pixelformat(struct drm_crtc *crtc, u32 pixel_format)
> +{
> +	u32 dspcntr, reg;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> +	int ret = 0;
> +
> +	reg = DSPCNTR(intel_crtc->pipe);
> +	dspcntr = I915_READ(reg);
> +	DRM_DEBUG_DRIVER("pixel format = %d\n", pixel_format);
> +	/* Mask out pixel format bits in case we change it */
> +	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
> +	switch (pixel_format) {
> +	case DRM_FORMAT_C8:
> +		dspcntr |= DISPPLANE_8BPP;
> +		break;
> +	case DRM_FORMAT_XRGB1555:
> +	case DRM_FORMAT_ARGB1555:
> +		dspcntr |= DISPPLANE_BGRX555;
> +		break;
> +	case DRM_FORMAT_RGB565:
> +		dspcntr |= DISPPLANE_BGRX565;
> +		break;
> +	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_ARGB8888:
> +		dspcntr |= DISPPLANE_BGRX888;
> +		break;
> +	case DRM_FORMAT_XBGR8888:
> +	case DRM_FORMAT_ABGR8888:
> +		dspcntr |= DISPPLANE_RGBX888;
> +		break;
> +	case DRM_FORMAT_XRGB2101010:
> +	case DRM_FORMAT_ARGB2101010:
> +		dspcntr |= DISPPLANE_BGRX101010;
> +		break;
> +	case DRM_FORMAT_XBGR2101010:
> +	case DRM_FORMAT_ABGR2101010:
> +		dspcntr |= DISPPLANE_RGBX101010;
> +		break;
> +	default:
> +		DRM_ERROR("Unsupported pixel format 0x%08x\n", pixel_format);
> +		return -EINVAL;
> +	}
> +
> +	/* Write pixel format update command to ring */
> +	ret = intel_ring_begin(ring, 4);
> +	if (ret) {
> +		DRM_ERROR("MI Command emit failed.\n");
> +		return ret;
> +	}
> +	intel_ring_emit(ring, MI_NOOP);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, reg);
> +	intel_ring_emit(ring, dspcntr);
> +	intel_ring_advance(ring);
> +	return 0;
> +}
> +
> +/* Update the primary plane registers. Uses MI commands */
> +static int haswell_update_plane(struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb, int x, int y)
> +{
> +	int ret;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> +	struct drm_i915_gem_object *obj;
> +	int plane = intel_crtc->plane;
> +
> +	switch (plane) {
> +	case 0:
> +	case 1:
> +	case 2:
> +		break;
> +	default:
> +		DRM_ERROR("Can't update plane %c\n", plane_name(plane));
> +		return -EINVAL;
> +	}
> +	obj = to_intel_framebuffer(fb)->obj;
> +
> +	/* Set pixel format */
> +	haswell_set_pixelformat(crtc, fb->pixel_format);
> +
> +	/* Set tiling offsets. Tiling mode is not set here as    *
> +	* it is set from intel_gen7_queue_flip. Send MI Command  *
> +	* to change -                                            *
> +	* 1. Tiling offset                                       *
> +	* 2. stride - fb->pitches[0]                             *
> +	* 2. surface base address                                *
> +	* Linear offset and tile offset is same for Haswell      */
> +	intel_crtc->dspaddr_offset =
> +		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
> +						fb->bits_per_pixel / 8,
> +						fb->pitches[0]);
> +
> +	DRM_DEBUG_KMS("Writing base %08X %08lX %d %d %d\n",
> +			obj->gtt_offset, intel_crtc->dspaddr_offset,
> +			x, y, fb->pitches[0]);
> +
> +	/* Emit MI commands here */
> +	ret = intel_ring_begin(ring, 10);
> +	if (ret)
> +		return ret;
> +	intel_ring_emit(ring, MI_NOOP);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, DSPOFFSET(plane));
> +	intel_ring_emit(ring, (y << 16) | x);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, DSPSTRIDE(plane));
> +	intel_ring_emit(ring, fb->pitches[0]);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, DSPSURF(plane));
> +	intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
> +	intel_ring_advance(ring);
> +
> +	return 0;
> +}
> +
>  static int ironlake_update_plane(struct drm_crtc *crtc,
>  				 struct drm_framebuffer *fb, int x, int y)
>  {
> @@ -7494,9 +7617,26 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	struct drm_framebuffer *old_fb = crtc->fb;
>  	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_framebuffer *intel_fb;
>  	struct intel_unpin_work *work;
>  	unsigned long flags;
>  	int ret;
> +	/*
> +	* TILEOFF registers can't be changed via MI display flips.
> +	* Changes in pitches[0] and offsets[0] updates the primary plane
> +	* control registers.
> +	*/
> +	intel_fb = to_intel_framebuffer(crtc->fb);
> +	if ((IS_HASWELL(dev)) &&
> +		((obj->tiling_mode != intel_fb->obj->tiling_mode) ||
> +		(fb->offsets[0] != crtc->fb->offsets[0]) ||
> +		(fb->pitches[0] != crtc->fb->pitches[0]))) {
> +			DRM_DEBUG_DRIVER(" crtc fb: pitch = %d offset = %d\n",
> +			crtc->fb->pitches[0], crtc->fb->offsets[0]);
> +			DRM_DEBUG_DRIVER(" input fb: pitch = %d offset = %d\n",
> +			fb->pitches[0], fb->offsets[0]);
> +			haswell_update_plane(crtc, fb, 0, 0);
> +	}
>  
>  	work = kzalloc(sizeof *work, GFP_KERNEL);
>  	if (work == NULL)
> @@ -8766,6 +8906,20 @@ out_config:
>  	return ret;
>  }
>  
> +/* Callback function - Called if change in pixel format is detected.
> +* Sends MI command to update change in pixel format.
> +*/
> +static int intel_crtc_set_pixel_format(struct drm_crtc *crtc, u32 pixel_format)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	if (IS_HASWELL(dev)) {
> +		return haswell_set_pixelformat(crtc, pixel_format);
> +	} else {
> +		DRM_ERROR("Pixel format change not allowed.\n");
> +		return -EINVAL;
> +	}
> +}
> +
>  static const struct drm_crtc_funcs intel_crtc_funcs = {
>  	.cursor_set = intel_crtc_cursor_set,
>  	.cursor_move = intel_crtc_cursor_move,
> @@ -8773,6 +8927,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
>  	.set_config = intel_crtc_set_config,
>  	.destroy = intel_crtc_destroy,
>  	.page_flip = intel_crtc_page_flip,
> +	.set_pixelformat = intel_crtc_set_pixel_format,
>  };
>  
>  static void intel_cpu_pll_init(struct drm_device *dev)
> @@ -10180,3 +10335,4 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
>  	}
>  }
>  #endif
> +
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index e215bcc..9a0ea92 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -323,6 +323,7 @@ struct drm_plane;
>   * @set_property: called when a property is changed
>   * @set_config: apply a new CRTC configuration
>   * @page_flip: initiate a page flip
> + * @set_pixelformat: apply new pixel format to primary plane control register
>   *
>   * The drm_crtc_funcs structure is the central CRTC management structure
>   * in the DRM.  Each CRTC controls one or more connectors (note that the name
> @@ -369,6 +370,13 @@ struct drm_crtc_funcs {
>  
>  	int (*set_property)(struct drm_crtc *crtc,
>  			    struct drm_property *property, uint64_t val);
> +	/*
> +	* Update the primary plane pixel format register during page flip.
> +	* To support dynamic change in pixel format define the callback
> +	* function for set_pixelformat.
> +	*/
> +	int (*set_pixelformat)(struct drm_crtc *crtc,
> +				uint32_t pixel_format);
>  };
>  
>  /**
> -- 
> 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

Re: [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.

[Chris Wilson]

On Fri, Sep 13, 2013 at 07:05:31PM +0530, Sandeep Ramankutty wrote:
> Change: Add support to change the pixel format, base surface address, tiling mode, tiling
> offset on the flow during the primary plane flip.

Tell us why first. Why this patch is necesary is the fundamental reason
that we need to record in the changelog. Discuss the background and the
motivation that leads us to require a kernel patch.
 
> Issue: This support is needed by hardware composer to meet the performance
> optimization requirement for 3D benchmark. This patch enables change in
> pixel format and tiling params without adding and removing the framebuffer.
> Adding and removing the framebuffer impacts performance.

So you say. Examples are vital, especially if you make a claim as
towards performance. And then to back those up with tests to make sure
nothing is broken. The other question is how do we end up regularly
flipping between pixel formats that it shows up in a performance
benchmark.
 
> Change Details-
> drm: Defined function pointer set_pixelformat in drm_crtc_funcs. drm_crtc.c is modified
> to execute the function to update pixel format.
> drm/i915: intel_crtc_set_pixel_format() implemented in i915 driver code (intel_display.c)
> and set to drm_crtc_funcs->set_pixelformat.
> haswell_set_pixelformat() implemented to update pixel format. This is specific to Haswell
> haswell_update_plane() implemented to update changes in tiling offset, surface
> base address and stride-fb->pitches[0]. Sends MI command to update the params
> in primary plane control registers.

You don't need to repeat the patch, just tell us what complications you
encountered and how you resolved them, or what issues remain.

> 
> Change-Id: I4b0af83f641b2b79ee4f5c3401790d7d90ccd9ef
> Signed-off-by: Pallavi G <pallavi.g@intel.com>
> Signed-off-by: Sandeep Ramakutty <sandeepx.ramakutty@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c           |   15 +++-
>  drivers/gpu/drm/i915/intel_display.c |  156 ++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h               |    8 ++
>  3 files changed, 176 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index c9f9f3d..4739b6c 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3518,9 +3518,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	}
>  
>  	if (crtc->fb->pixel_format != fb->pixel_format) {
> -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> -		ret = -EINVAL;
> -		goto out;
> +		if (crtc->funcs->set_pixelformat == NULL) {
> +			DRM_DEBUG_KMS("Pixel format change not allowed");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		/* supports dynamic change in pixel format */
> +		ret = crtc->funcs->set_pixelformat(crtc, fb->pixel_format);

Bleh.

Just install a default set_pixelformat that returns -EINVAL and lose the
i++; /* increment i after reading */

Also the current use of crtc->funcs->set_pixelformat suggests confusion
is paramount.

> +		if (ret) {
> +			DRM_DEBUG_KMS("Pixel format change failed %d",
> +							fb->pixel_format);
> +			goto out;
> +		}

What happens if the flip fails afterwards? Where's the unwind? Where's
the protection that prevents the format change whilst the current
scanout is still visible? Perhaps this being the ring emission is being
attempted far too early.

>  	}
>  
>  	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 955df91..7bfe088 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -45,6 +45,7 @@ bool intel_pipe_has_type(struct drm_crtc *crtc, int type);
>  static void intel_increase_pllclock(struct drm_crtc *crtc);
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
>  
> +
>  typedef struct {
>  	int	min, max;
>  } intel_range_t;
> @@ -1959,6 +1960,128 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  	return 0;
>  }
>  
> +/* Set Pixel format for Haswell using MI commands */
> +static int haswell_set_pixelformat(struct drm_crtc *crtc, u32 pixel_format)
> +{
> +	u32 dspcntr, reg;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> +	int ret = 0;

Interesting choice of ring.

> +	reg = DSPCNTR(intel_crtc->pipe);
> +	dspcntr = I915_READ(reg);
> +	DRM_DEBUG_DRIVER("pixel format = %d\n", pixel_format);
> +	/* Mask out pixel format bits in case we change it */
> +	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
> +	switch (pixel_format) {
> +	case DRM_FORMAT_C8:
> +		dspcntr |= DISPPLANE_8BPP;
> +		break;
> +	case DRM_FORMAT_XRGB1555:
> +	case DRM_FORMAT_ARGB1555:
> +		dspcntr |= DISPPLANE_BGRX555;
> +		break;
> +	case DRM_FORMAT_RGB565:
> +		dspcntr |= DISPPLANE_BGRX565;
> +		break;
> +	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_ARGB8888:
> +		dspcntr |= DISPPLANE_BGRX888;
> +		break;
> +	case DRM_FORMAT_XBGR8888:
> +	case DRM_FORMAT_ABGR8888:
> +		dspcntr |= DISPPLANE_RGBX888;
> +		break;
> +	case DRM_FORMAT_XRGB2101010:
> +	case DRM_FORMAT_ARGB2101010:
> +		dspcntr |= DISPPLANE_BGRX101010;
> +		break;
> +	case DRM_FORMAT_XBGR2101010:
> +	case DRM_FORMAT_ABGR2101010:
> +		dspcntr |= DISPPLANE_RGBX101010;
> +		break;
> +	default:
> +		DRM_ERROR("Unsupported pixel format 0x%08x\n", pixel_format);
> +		return -EINVAL;
> +	}
> +
> +	/* Write pixel format update command to ring */
> +	ret = intel_ring_begin(ring, 4);
> +	if (ret) {
> +		DRM_ERROR("MI Command emit failed.\n");
> +		return ret;
> +	}
> +	intel_ring_emit(ring, MI_NOOP);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, reg);
> +	intel_ring_emit(ring, dspcntr);
> +	intel_ring_advance(ring);
> +	return 0;
> +}
> +
> +/* Update the primary plane registers. Uses MI commands */
> +static int haswell_update_plane(struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb, int x, int y)
> +{
> +	int ret;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> +	struct drm_i915_gem_object *obj;
> +	int plane = intel_crtc->plane;
> +
> +	switch (plane) {
> +	case 0:
> +	case 1:
> +	case 2:
> +		break;
> +	default:
> +		DRM_ERROR("Can't update plane %c\n", plane_name(plane));
> +		return -EINVAL;
> +	}
> +	obj = to_intel_framebuffer(fb)->obj;
> +
> +	/* Set pixel format */
> +	haswell_set_pixelformat(crtc, fb->pixel_format);
> +
> +	/* Set tiling offsets. Tiling mode is not set here as    *
> +	* it is set from intel_gen7_queue_flip. Send MI Command  *
> +	* to change -                                            *
> +	* 1. Tiling offset                                       *
> +	* 2. stride - fb->pitches[0]                             *
> +	* 2. surface base address                                *
> +	* Linear offset and tile offset is same for Haswell      */

The first rule of adding code is to be consistent with the surrounding
code.

> +	intel_crtc->dspaddr_offset =
> +		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
> +						fb->bits_per_pixel / 8,
> +						fb->pitches[0]);
> +
> +	DRM_DEBUG_KMS("Writing base %08X %08lX %d %d %d\n",
> +			obj->gtt_offset, intel_crtc->dspaddr_offset,
> +			x, y, fb->pitches[0]);
> +
> +	/* Emit MI commands here */
> +	ret = intel_ring_begin(ring, 10);
> +	if (ret)
> +		return ret;
> +	intel_ring_emit(ring, MI_NOOP);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, DSPOFFSET(plane));
> +	intel_ring_emit(ring, (y << 16) | x);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, DSPSTRIDE(plane));
> +	intel_ring_emit(ring, fb->pitches[0]);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, DSPSURF(plane));
> +	intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
> +	intel_ring_advance(ring);
> +
> +	return 0;
> +}
> +
>  static int ironlake_update_plane(struct drm_crtc *crtc,
>  				 struct drm_framebuffer *fb, int x, int y)
>  {
> @@ -7494,9 +7617,26 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	struct drm_framebuffer *old_fb = crtc->fb;
>  	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_framebuffer *intel_fb;
>  	struct intel_unpin_work *work;
>  	unsigned long flags;
>  	int ret;
> +	/*
> +	* TILEOFF registers can't be changed via MI display flips.
> +	* Changes in pitches[0] and offsets[0] updates the primary plane
> +	* control registers.
> +	*/
> +	intel_fb = to_intel_framebuffer(crtc->fb);
> +	if ((IS_HASWELL(dev)) &&
> +		((obj->tiling_mode != intel_fb->obj->tiling_mode) ||
> +		(fb->offsets[0] != crtc->fb->offsets[0]) ||
> +		(fb->pitches[0] != crtc->fb->pitches[0]))) {
> +			DRM_DEBUG_DRIVER(" crtc fb: pitch = %d offset = %d\n",
> +			crtc->fb->pitches[0], crtc->fb->offsets[0]);
> +			DRM_DEBUG_DRIVER(" input fb: pitch = %d offset = %d\n",
> +			fb->pitches[0], fb->offsets[0]);
> +			haswell_update_plane(crtc, fb, 0, 0);

We do expect code to at least bear some resemblence to the CODING_STYLE.

Also this techinique does not look to be haswell specific, and with some
rearrangement could be written much more neatly.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.

[Ramakutty, SandeepX]

Hi Chris,

This patch is necessary for applying pixel format changes without creating new frame buffer. The pixel format will be changed at page flip.  
Background - The present kernel code does not support applying pixel format to the current frame buffer. Hence hardware composer needs to create a new frame buffer each time it needs to change the frame buffer.  If the application/game tries to change the pixel format very frequently, this affects performance.  
Motivation - This change enables the hardware composer to apply the new pixel format without creating  a new framebuffer. This improves the performance at a time when the application frequently changes the pixel format.

Let me know if any queries.

Regards,
Sandeep

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Friday, September 13, 2013 8:08 PM
To: Ramakutty, SandeepX
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.

On Fri, Sep 13, 2013 at 07:05:31PM +0530, Sandeep Ramankutty wrote:
> Change: Add support to change the pixel format, base surface address, 
> tiling mode, tiling offset on the flow during the primary plane flip.

Tell us why first. Why this patch is necesary is the fundamental reason that we need to record in the changelog. Discuss the background and the motivation that leads us to require a kernel patch.
 
> Issue: This support is needed by hardware composer to meet the 
> performance optimization requirement for 3D benchmark. This patch 
> enables change in pixel format and tiling params without adding and removing the framebuffer.
> Adding and removing the framebuffer impacts performance.

So you say. Examples are vital, especially if you make a claim as towards performance. And then to back those up with tests to make sure nothing is broken. The other question is how do we end up regularly flipping between pixel formats that it shows up in a performance benchmark.
 
> Change Details-
> drm: Defined function pointer set_pixelformat in drm_crtc_funcs. 
> drm_crtc.c is modified to execute the function to update pixel format.
> drm/i915: intel_crtc_set_pixel_format() implemented in i915 driver 
> code (intel_display.c) and set to drm_crtc_funcs->set_pixelformat.
> haswell_set_pixelformat() implemented to update pixel format. This is 
> specific to Haswell
> haswell_update_plane() implemented to update changes in tiling offset, 
> surface base address and stride-fb->pitches[0]. Sends MI command to 
> update the params in primary plane control registers.

You don't need to repeat the patch, just tell us what complications you encountered and how you resolved them, or what issues remain.

> 
> Change-Id: I4b0af83f641b2b79ee4f5c3401790d7d90ccd9ef
> Signed-off-by: Pallavi G <pallavi.g@intel.com>
> Signed-off-by: Sandeep Ramakutty <sandeepx.ramakutty@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c           |   15 +++-
>  drivers/gpu/drm/i915/intel_display.c |  156 ++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h               |    8 ++
>  3 files changed, 176 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c 
> index c9f9f3d..4739b6c 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3518,9 +3518,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	}
>  
>  	if (crtc->fb->pixel_format != fb->pixel_format) {
> -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> -		ret = -EINVAL;
> -		goto out;
> +		if (crtc->funcs->set_pixelformat == NULL) {
> +			DRM_DEBUG_KMS("Pixel format change not allowed");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		/* supports dynamic change in pixel format */
> +		ret = crtc->funcs->set_pixelformat(crtc, fb->pixel_format);

Bleh.

Just install a default set_pixelformat that returns -EINVAL and lose the
i++; /* increment i after reading */

Also the current use of crtc->funcs->set_pixelformat suggests confusion is paramount.

> +		if (ret) {
> +			DRM_DEBUG_KMS("Pixel format change failed %d",
> +							fb->pixel_format);
> +			goto out;
> +		}

What happens if the flip fails afterwards? Where's the unwind? Where's the protection that prevents the format change whilst the current scanout is still visible? Perhaps this being the ring emission is being attempted far too early.

>  	}
>  
>  	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { diff --git 
> a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 955df91..7bfe088 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -45,6 +45,7 @@ bool intel_pipe_has_type(struct drm_crtc *crtc, int 
> type);  static void intel_increase_pllclock(struct drm_crtc *crtc);  
> static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
>  
> +
>  typedef struct {
>  	int	min, max;
>  } intel_range_t;
> @@ -1959,6 +1960,128 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  	return 0;
>  }
>  
> +/* Set Pixel format for Haswell using MI commands */ static int 
> +haswell_set_pixelformat(struct drm_crtc *crtc, u32 pixel_format) {
> +	u32 dspcntr, reg;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> +	int ret = 0;

Interesting choice of ring.

> +	reg = DSPCNTR(intel_crtc->pipe);
> +	dspcntr = I915_READ(reg);
> +	DRM_DEBUG_DRIVER("pixel format = %d\n", pixel_format);
> +	/* Mask out pixel format bits in case we change it */
> +	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
> +	switch (pixel_format) {
> +	case DRM_FORMAT_C8:
> +		dspcntr |= DISPPLANE_8BPP;
> +		break;
> +	case DRM_FORMAT_XRGB1555:
> +	case DRM_FORMAT_ARGB1555:
> +		dspcntr |= DISPPLANE_BGRX555;
> +		break;
> +	case DRM_FORMAT_RGB565:
> +		dspcntr |= DISPPLANE_BGRX565;
> +		break;
> +	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_ARGB8888:
> +		dspcntr |= DISPPLANE_BGRX888;
> +		break;
> +	case DRM_FORMAT_XBGR8888:
> +	case DRM_FORMAT_ABGR8888:
> +		dspcntr |= DISPPLANE_RGBX888;
> +		break;
> +	case DRM_FORMAT_XRGB2101010:
> +	case DRM_FORMAT_ARGB2101010:
> +		dspcntr |= DISPPLANE_BGRX101010;
> +		break;
> +	case DRM_FORMAT_XBGR2101010:
> +	case DRM_FORMAT_ABGR2101010:
> +		dspcntr |= DISPPLANE_RGBX101010;
> +		break;
> +	default:
> +		DRM_ERROR("Unsupported pixel format 0x%08x\n", pixel_format);
> +		return -EINVAL;
> +	}
> +
> +	/* Write pixel format update command to ring */
> +	ret = intel_ring_begin(ring, 4);
> +	if (ret) {
> +		DRM_ERROR("MI Command emit failed.\n");
> +		return ret;
> +	}
> +	intel_ring_emit(ring, MI_NOOP);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, reg);
> +	intel_ring_emit(ring, dspcntr);
> +	intel_ring_advance(ring);
> +	return 0;
> +}
> +
> +/* Update the primary plane registers. Uses MI commands */ static int 
> +haswell_update_plane(struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb, int x, int y) {
> +	int ret;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> +	struct drm_i915_gem_object *obj;
> +	int plane = intel_crtc->plane;
> +
> +	switch (plane) {
> +	case 0:
> +	case 1:
> +	case 2:
> +		break;
> +	default:
> +		DRM_ERROR("Can't update plane %c\n", plane_name(plane));
> +		return -EINVAL;
> +	}
> +	obj = to_intel_framebuffer(fb)->obj;
> +
> +	/* Set pixel format */
> +	haswell_set_pixelformat(crtc, fb->pixel_format);
> +
> +	/* Set tiling offsets. Tiling mode is not set here as    *
> +	* it is set from intel_gen7_queue_flip. Send MI Command  *
> +	* to change -                                            *
> +	* 1. Tiling offset                                       *
> +	* 2. stride - fb->pitches[0]                             *
> +	* 2. surface base address                                *
> +	* Linear offset and tile offset is same for Haswell      */

The first rule of adding code is to be consistent with the surrounding code.

> +	intel_crtc->dspaddr_offset =
> +		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
> +						fb->bits_per_pixel / 8,
> +						fb->pitches[0]);
> +
> +	DRM_DEBUG_KMS("Writing base %08X %08lX %d %d %d\n",
> +			obj->gtt_offset, intel_crtc->dspaddr_offset,
> +			x, y, fb->pitches[0]);
> +
> +	/* Emit MI commands here */
> +	ret = intel_ring_begin(ring, 10);
> +	if (ret)
> +		return ret;
> +	intel_ring_emit(ring, MI_NOOP);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, DSPOFFSET(plane));
> +	intel_ring_emit(ring, (y << 16) | x);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, DSPSTRIDE(plane));
> +	intel_ring_emit(ring, fb->pitches[0]);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, DSPSURF(plane));
> +	intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
> +	intel_ring_advance(ring);
> +
> +	return 0;
> +}
> +
>  static int ironlake_update_plane(struct drm_crtc *crtc,
>  				 struct drm_framebuffer *fb, int x, int y)  { @@ -7494,9 +7617,26 
> @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	struct drm_framebuffer *old_fb = crtc->fb;
>  	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_framebuffer *intel_fb;
>  	struct intel_unpin_work *work;
>  	unsigned long flags;
>  	int ret;
> +	/*
> +	* TILEOFF registers can't be changed via MI display flips.
> +	* Changes in pitches[0] and offsets[0] updates the primary plane
> +	* control registers.
> +	*/
> +	intel_fb = to_intel_framebuffer(crtc->fb);
> +	if ((IS_HASWELL(dev)) &&
> +		((obj->tiling_mode != intel_fb->obj->tiling_mode) ||
> +		(fb->offsets[0] != crtc->fb->offsets[0]) ||
> +		(fb->pitches[0] != crtc->fb->pitches[0]))) {
> +			DRM_DEBUG_DRIVER(" crtc fb: pitch = %d offset = %d\n",
> +			crtc->fb->pitches[0], crtc->fb->offsets[0]);
> +			DRM_DEBUG_DRIVER(" input fb: pitch = %d offset = %d\n",
> +			fb->pitches[0], fb->offsets[0]);
> +			haswell_update_plane(crtc, fb, 0, 0);

We do expect code to at least bear some resemblence to the CODING_STYLE.

Also this techinique does not look to be haswell specific, and with some rearrangement could be written much more neatly.
-Chris

--
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.

[Chris Wilson]

On Mon, Sep 23, 2013 at 11:32:40AM +0000, Ramakutty, SandeepX wrote:
> Hi Chris,
> 
> This patch is necessary for applying pixel format changes without creating new frame buffer.

No, it doesn't. As pixelformat is only specified with AddFB2, it appears
there is something else required to allow changing format on an existing
fb on the fly.

> The pixel format will be changed at page flip.  
> Background - The present kernel code does not support applying pixel format to the current frame buffer. Hence hardware composer needs to create a new frame buffer each time it needs to change the frame buffer.  If the application/game tries to change the pixel format very frequently, this affects performance.  
> Motivation - This change enables the hardware composer to apply the new pixel format without creating  a new framebuffer. This improves the performance at a time when the application frequently changes the pixel format.

That's obvious from the patch. I was hoping for more details on the use
case to demonstrate the necessity of this approach, I still do not see
a scenario that would require frequent changes in scanout pixelformat.

Quantify, quantify, quantify. Then we know whether the approach meets
your goal, and more importantly detect any future regressions.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.

[Ramakutty, SandeepX]

Hi Ville ,

Thanks for the feedback.

We verified without updating watermarks and found that the video playback, 3DMark and GLBenchmark plays fine. There was no underrun error too.
Cases tried out-
Pixel format changed to 32BPP when water mark calculated based on 16BPP
Pixel format changed to 16BPP when water mark calculated based on 32BPP.

In both cases, we verified with video playback, 3DMark and GLBenchmark and did not see any underrun error.  Is there any specific situations in which not setting WM can throw up an issue? 

With MMIO there may be a possibility of flickering. The pixel format may be updated well before a page flip. With MMIO, the format will be written to register before a page flip and this can cause unwanted effects.  Hence we used MI commands as this will get updated much nearer to a page flip.

What is  SRM?

Regards,
Sandeep

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Friday, September 13, 2013 7:56 PM
To: Ramakutty, SandeepX
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.

On Fri, Sep 13, 2013 at 07:05:31PM +0530, Sandeep Ramankutty wrote:
> Change: Add support to change the pixel format, base surface address, 
> tiling mode, tiling offset on the flow during the primary plane flip.
> 
> Issue: This support is needed by hardware composer to meet the 
> performance optimization requirement for 3D benchmark. This patch 
> enables change in pixel format and tiling params without adding and removing the framebuffer.
> Adding and removing the framebuffer impacts performance.

We can't do it quite like this unfortunately. Watermarksm may need to be updated carefully if the bpp changes. Also updating multiple plane registers isn't always atomic, so we anyway need the vblank evade tricks to make it look pretty. I suppose it might be possible to make all that work via the ring, but my plan is to go for MMIO, and later investigate if we can optimize it by using the ring.

As far as the lowlevel details go, you supposedly need a SRM after each LRI aimed at the display registers. Otherwise I guess you could even blast all the registers with a single LRI.

> Change Details-
> drm: Defined function pointer set_pixelformat in drm_crtc_funcs. 
> drm_crtc.c is modified to execute the function to update pixel format.
> drm/i915: intel_crtc_set_pixel_format() implemented in i915 driver 
> code (intel_display.c) and set to drm_crtc_funcs->set_pixelformat.
> haswell_set_pixelformat() implemented to update pixel format. This is 
> specific to Haswell
> haswell_update_plane() implemented to update changes in tiling offset, 
> surface base address and stride-fb->pitches[0]. Sends MI command to 
> update the params in primary plane control registers.
> 
> Change-Id: I4b0af83f641b2b79ee4f5c3401790d7d90ccd9ef
> Signed-off-by: Pallavi G <pallavi.g@intel.com>
> Signed-off-by: Sandeep Ramakutty <sandeepx.ramakutty@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c           |   15 +++-
>  drivers/gpu/drm/i915/intel_display.c |  156 ++++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h               |    8 ++
>  3 files changed, 176 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c 
> index c9f9f3d..4739b6c 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -3518,9 +3518,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	}
>  
>  	if (crtc->fb->pixel_format != fb->pixel_format) {
> -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> -		ret = -EINVAL;
> -		goto out;
> +		if (crtc->funcs->set_pixelformat == NULL) {
> +			DRM_DEBUG_KMS("Pixel format change not allowed");
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		/* supports dynamic change in pixel format */
> +		ret = crtc->funcs->set_pixelformat(crtc, fb->pixel_format);
> +		if (ret) {
> +			DRM_DEBUG_KMS("Pixel format change failed %d",
> +							fb->pixel_format);
> +			goto out;
> +		}
>  	}
>  
>  	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { diff --git 
> a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 955df91..7bfe088 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -45,6 +45,7 @@ bool intel_pipe_has_type(struct drm_crtc *crtc, int 
> type);  static void intel_increase_pllclock(struct drm_crtc *crtc);  
> static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
>  
> +
>  typedef struct {
>  	int	min, max;
>  } intel_range_t;
> @@ -1959,6 +1960,128 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  	return 0;
>  }
>  
> +/* Set Pixel format for Haswell using MI commands */ static int 
> +haswell_set_pixelformat(struct drm_crtc *crtc, u32 pixel_format) {
> +	u32 dspcntr, reg;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> +	int ret = 0;
> +
> +	reg = DSPCNTR(intel_crtc->pipe);
> +	dspcntr = I915_READ(reg);
> +	DRM_DEBUG_DRIVER("pixel format = %d\n", pixel_format);
> +	/* Mask out pixel format bits in case we change it */
> +	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
> +	switch (pixel_format) {
> +	case DRM_FORMAT_C8:
> +		dspcntr |= DISPPLANE_8BPP;
> +		break;
> +	case DRM_FORMAT_XRGB1555:
> +	case DRM_FORMAT_ARGB1555:
> +		dspcntr |= DISPPLANE_BGRX555;
> +		break;
> +	case DRM_FORMAT_RGB565:
> +		dspcntr |= DISPPLANE_BGRX565;
> +		break;
> +	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_ARGB8888:
> +		dspcntr |= DISPPLANE_BGRX888;
> +		break;
> +	case DRM_FORMAT_XBGR8888:
> +	case DRM_FORMAT_ABGR8888:
> +		dspcntr |= DISPPLANE_RGBX888;
> +		break;
> +	case DRM_FORMAT_XRGB2101010:
> +	case DRM_FORMAT_ARGB2101010:
> +		dspcntr |= DISPPLANE_BGRX101010;
> +		break;
> +	case DRM_FORMAT_XBGR2101010:
> +	case DRM_FORMAT_ABGR2101010:
> +		dspcntr |= DISPPLANE_RGBX101010;
> +		break;
> +	default:
> +		DRM_ERROR("Unsupported pixel format 0x%08x\n", pixel_format);
> +		return -EINVAL;
> +	}
> +
> +	/* Write pixel format update command to ring */
> +	ret = intel_ring_begin(ring, 4);
> +	if (ret) {
> +		DRM_ERROR("MI Command emit failed.\n");
> +		return ret;
> +	}
> +	intel_ring_emit(ring, MI_NOOP);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, reg);
> +	intel_ring_emit(ring, dspcntr);
> +	intel_ring_advance(ring);
> +	return 0;
> +}
> +
> +/* Update the primary plane registers. Uses MI commands */ static int 
> +haswell_update_plane(struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb, int x, int y) {
> +	int ret;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> +	struct drm_i915_gem_object *obj;
> +	int plane = intel_crtc->plane;
> +
> +	switch (plane) {
> +	case 0:
> +	case 1:
> +	case 2:
> +		break;
> +	default:
> +		DRM_ERROR("Can't update plane %c\n", plane_name(plane));
> +		return -EINVAL;
> +	}
> +	obj = to_intel_framebuffer(fb)->obj;
> +
> +	/* Set pixel format */
> +	haswell_set_pixelformat(crtc, fb->pixel_format);
> +
> +	/* Set tiling offsets. Tiling mode is not set here as    *
> +	* it is set from intel_gen7_queue_flip. Send MI Command  *
> +	* to change -                                            *
> +	* 1. Tiling offset                                       *
> +	* 2. stride - fb->pitches[0]                             *
> +	* 2. surface base address                                *
> +	* Linear offset and tile offset is same for Haswell      */
> +	intel_crtc->dspaddr_offset =
> +		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
> +						fb->bits_per_pixel / 8,
> +						fb->pitches[0]);
> +
> +	DRM_DEBUG_KMS("Writing base %08X %08lX %d %d %d\n",
> +			obj->gtt_offset, intel_crtc->dspaddr_offset,
> +			x, y, fb->pitches[0]);
> +
> +	/* Emit MI commands here */
> +	ret = intel_ring_begin(ring, 10);
> +	if (ret)
> +		return ret;
> +	intel_ring_emit(ring, MI_NOOP);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, DSPOFFSET(plane));
> +	intel_ring_emit(ring, (y << 16) | x);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, DSPSTRIDE(plane));
> +	intel_ring_emit(ring, fb->pitches[0]);
> +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +	intel_ring_emit(ring, DSPSURF(plane));
> +	intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
> +	intel_ring_advance(ring);
> +
> +	return 0;
> +}
> +
>  static int ironlake_update_plane(struct drm_crtc *crtc,
>  				 struct drm_framebuffer *fb, int x, int y)  { @@ -7494,9 +7617,26 
> @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	struct drm_framebuffer *old_fb = crtc->fb;
>  	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_framebuffer *intel_fb;
>  	struct intel_unpin_work *work;
>  	unsigned long flags;
>  	int ret;
> +	/*
> +	* TILEOFF registers can't be changed via MI display flips.
> +	* Changes in pitches[0] and offsets[0] updates the primary plane
> +	* control registers.
> +	*/
> +	intel_fb = to_intel_framebuffer(crtc->fb);
> +	if ((IS_HASWELL(dev)) &&
> +		((obj->tiling_mode != intel_fb->obj->tiling_mode) ||
> +		(fb->offsets[0] != crtc->fb->offsets[0]) ||
> +		(fb->pitches[0] != crtc->fb->pitches[0]))) {
> +			DRM_DEBUG_DRIVER(" crtc fb: pitch = %d offset = %d\n",
> +			crtc->fb->pitches[0], crtc->fb->offsets[0]);
> +			DRM_DEBUG_DRIVER(" input fb: pitch = %d offset = %d\n",
> +			fb->pitches[0], fb->offsets[0]);
> +			haswell_update_plane(crtc, fb, 0, 0);
> +	}
>  
>  	work = kzalloc(sizeof *work, GFP_KERNEL);
>  	if (work == NULL)
> @@ -8766,6 +8906,20 @@ out_config:
>  	return ret;
>  }
>  
> +/* Callback function - Called if change in pixel format is detected.
> +* Sends MI command to update change in pixel format.
> +*/
> +static int intel_crtc_set_pixel_format(struct drm_crtc *crtc, u32 
> +pixel_format) {
> +	struct drm_device *dev = crtc->dev;
> +	if (IS_HASWELL(dev)) {
> +		return haswell_set_pixelformat(crtc, pixel_format);
> +	} else {
> +		DRM_ERROR("Pixel format change not allowed.\n");
> +		return -EINVAL;
> +	}
> +}
> +
>  static const struct drm_crtc_funcs intel_crtc_funcs = {
>  	.cursor_set = intel_crtc_cursor_set,
>  	.cursor_move = intel_crtc_cursor_move, @@ -8773,6 +8927,7 @@ static 
> const struct drm_crtc_funcs intel_crtc_funcs = {
>  	.set_config = intel_crtc_set_config,
>  	.destroy = intel_crtc_destroy,
>  	.page_flip = intel_crtc_page_flip,
> +	.set_pixelformat = intel_crtc_set_pixel_format,
>  };
>  
>  static void intel_cpu_pll_init(struct drm_device *dev) @@ -10180,3 
> +10335,4 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
>  	}
>  }
>  #endif
> +
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 
> e215bcc..9a0ea92 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -323,6 +323,7 @@ struct drm_plane;
>   * @set_property: called when a property is changed
>   * @set_config: apply a new CRTC configuration
>   * @page_flip: initiate a page flip
> + * @set_pixelformat: apply new pixel format to primary plane control 
> + register
>   *
>   * The drm_crtc_funcs structure is the central CRTC management structure
>   * in the DRM.  Each CRTC controls one or more connectors (note that 
> the name @@ -369,6 +370,13 @@ struct drm_crtc_funcs {
>  
>  	int (*set_property)(struct drm_crtc *crtc,
>  			    struct drm_property *property, uint64_t val);
> +	/*
> +	* Update the primary plane pixel format register during page flip.
> +	* To support dynamic change in pixel format define the callback
> +	* function for set_pixelformat.
> +	*/
> +	int (*set_pixelformat)(struct drm_crtc *crtc,
> +				uint32_t pixel_format);
>  };
>  
>  /**
> --
> 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

Re: [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.

[Ville Syrjälä]

On Thu, Oct 31, 2013 at 12:15:17PM +0000, Ramakutty, SandeepX wrote:
> Hi Ville ,
> 
> Thanks for the feedback.
> 
> We verified without updating watermarks and found that the video playback, 3DMark and GLBenchmark plays fine. There was no underrun error too.
> Cases tried out-
> Pixel format changed to 32BPP when water mark calculated based on 16BPP
> Pixel format changed to 16BPP when water mark calculated based on 32BPP.
> 
> In both cases, we verified with video playback, 3DMark and GLBenchmark and did not see any underrun error.  Is there any specific situations in which not setting WM can throw up an issue? 
> 
> With MMIO there may be a possibility of flickering. The pixel format may be updated well before a page flip. With MMIO, the format will be written to register before a page flip and this can cause unwanted effects.  Hence we used MI commands as this will get updated much nearer to a page flip.
> 
> What is  SRM?

"store register mem". It reads the register and stores the result to
memory.

BSpec tells us that there can only be one outstanding LRI targeted at
the display registers at any given. So you need to do LRI+SRM for every
display register you want to write w/ LRI.

> 
> Regards,
> Sandeep
> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Friday, September 13, 2013 7:56 PM
> To: Ramakutty, SandeepX
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.
> 
> On Fri, Sep 13, 2013 at 07:05:31PM +0530, Sandeep Ramankutty wrote:
> > Change: Add support to change the pixel format, base surface address, 
> > tiling mode, tiling offset on the flow during the primary plane flip.
> > 
> > Issue: This support is needed by hardware composer to meet the 
> > performance optimization requirement for 3D benchmark. This patch 
> > enables change in pixel format and tiling params without adding and removing the framebuffer.
> > Adding and removing the framebuffer impacts performance.
> 
> We can't do it quite like this unfortunately. Watermarksm may need to be updated carefully if the bpp changes. Also updating multiple plane registers isn't always atomic, so we anyway need the vblank evade tricks to make it look pretty. I suppose it might be possible to make all that work via the ring, but my plan is to go for MMIO, and later investigate if we can optimize it by using the ring.
> 
> As far as the lowlevel details go, you supposedly need a SRM after each LRI aimed at the display registers. Otherwise I guess you could even blast all the registers with a single LRI.
> 
> > Change Details-
> > drm: Defined function pointer set_pixelformat in drm_crtc_funcs. 
> > drm_crtc.c is modified to execute the function to update pixel format.
> > drm/i915: intel_crtc_set_pixel_format() implemented in i915 driver 
> > code (intel_display.c) and set to drm_crtc_funcs->set_pixelformat.
> > haswell_set_pixelformat() implemented to update pixel format. This is 
> > specific to Haswell
> > haswell_update_plane() implemented to update changes in tiling offset, 
> > surface base address and stride-fb->pitches[0]. Sends MI command to 
> > update the params in primary plane control registers.
> > 
> > Change-Id: I4b0af83f641b2b79ee4f5c3401790d7d90ccd9ef
> > Signed-off-by: Pallavi G <pallavi.g@intel.com>
> > Signed-off-by: Sandeep Ramakutty <sandeepx.ramakutty@intel.com>
> > ---
> >  drivers/gpu/drm/drm_crtc.c           |   15 +++-
> >  drivers/gpu/drm/i915/intel_display.c |  156 ++++++++++++++++++++++++++++++++++
> >  include/drm/drm_crtc.h               |    8 ++
> >  3 files changed, 176 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c 
> > index c9f9f3d..4739b6c 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -3518,9 +3518,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  	}
> >  
> >  	if (crtc->fb->pixel_format != fb->pixel_format) {
> > -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > -		ret = -EINVAL;
> > -		goto out;
> > +		if (crtc->funcs->set_pixelformat == NULL) {
> > +			DRM_DEBUG_KMS("Pixel format change not allowed");
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +		/* supports dynamic change in pixel format */
> > +		ret = crtc->funcs->set_pixelformat(crtc, fb->pixel_format);
> > +		if (ret) {
> > +			DRM_DEBUG_KMS("Pixel format change failed %d",
> > +							fb->pixel_format);
> > +			goto out;
> > +		}
> >  	}
> >  
> >  	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { diff --git 
> > a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 955df91..7bfe088 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -45,6 +45,7 @@ bool intel_pipe_has_type(struct drm_crtc *crtc, int 
> > type);  static void intel_increase_pllclock(struct drm_crtc *crtc);  
> > static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
> >  
> > +
> >  typedef struct {
> >  	int	min, max;
> >  } intel_range_t;
> > @@ -1959,6 +1960,128 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> >  	return 0;
> >  }
> >  
> > +/* Set Pixel format for Haswell using MI commands */ static int 
> > +haswell_set_pixelformat(struct drm_crtc *crtc, u32 pixel_format) {
> > +	u32 dspcntr, reg;
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> > +	int ret = 0;
> > +
> > +	reg = DSPCNTR(intel_crtc->pipe);
> > +	dspcntr = I915_READ(reg);
> > +	DRM_DEBUG_DRIVER("pixel format = %d\n", pixel_format);
> > +	/* Mask out pixel format bits in case we change it */
> > +	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
> > +	switch (pixel_format) {
> > +	case DRM_FORMAT_C8:
> > +		dspcntr |= DISPPLANE_8BPP;
> > +		break;
> > +	case DRM_FORMAT_XRGB1555:
> > +	case DRM_FORMAT_ARGB1555:
> > +		dspcntr |= DISPPLANE_BGRX555;
> > +		break;
> > +	case DRM_FORMAT_RGB565:
> > +		dspcntr |= DISPPLANE_BGRX565;
> > +		break;
> > +	case DRM_FORMAT_XRGB8888:
> > +	case DRM_FORMAT_ARGB8888:
> > +		dspcntr |= DISPPLANE_BGRX888;
> > +		break;
> > +	case DRM_FORMAT_XBGR8888:
> > +	case DRM_FORMAT_ABGR8888:
> > +		dspcntr |= DISPPLANE_RGBX888;
> > +		break;
> > +	case DRM_FORMAT_XRGB2101010:
> > +	case DRM_FORMAT_ARGB2101010:
> > +		dspcntr |= DISPPLANE_BGRX101010;
> > +		break;
> > +	case DRM_FORMAT_XBGR2101010:
> > +	case DRM_FORMAT_ABGR2101010:
> > +		dspcntr |= DISPPLANE_RGBX101010;
> > +		break;
> > +	default:
> > +		DRM_ERROR("Unsupported pixel format 0x%08x\n", pixel_format);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Write pixel format update command to ring */
> > +	ret = intel_ring_begin(ring, 4);
> > +	if (ret) {
> > +		DRM_ERROR("MI Command emit failed.\n");
> > +		return ret;
> > +	}
> > +	intel_ring_emit(ring, MI_NOOP);
> > +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > +	intel_ring_emit(ring, reg);
> > +	intel_ring_emit(ring, dspcntr);
> > +	intel_ring_advance(ring);
> > +	return 0;
> > +}
> > +
> > +/* Update the primary plane registers. Uses MI commands */ static int 
> > +haswell_update_plane(struct drm_crtc *crtc,
> > +				struct drm_framebuffer *fb, int x, int y) {
> > +	int ret;
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> > +	struct drm_i915_gem_object *obj;
> > +	int plane = intel_crtc->plane;
> > +
> > +	switch (plane) {
> > +	case 0:
> > +	case 1:
> > +	case 2:
> > +		break;
> > +	default:
> > +		DRM_ERROR("Can't update plane %c\n", plane_name(plane));
> > +		return -EINVAL;
> > +	}
> > +	obj = to_intel_framebuffer(fb)->obj;
> > +
> > +	/* Set pixel format */
> > +	haswell_set_pixelformat(crtc, fb->pixel_format);
> > +
> > +	/* Set tiling offsets. Tiling mode is not set here as    *
> > +	* it is set from intel_gen7_queue_flip. Send MI Command  *
> > +	* to change -                                            *
> > +	* 1. Tiling offset                                       *
> > +	* 2. stride - fb->pitches[0]                             *
> > +	* 2. surface base address                                *
> > +	* Linear offset and tile offset is same for Haswell      */
> > +	intel_crtc->dspaddr_offset =
> > +		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
> > +						fb->bits_per_pixel / 8,
> > +						fb->pitches[0]);
> > +
> > +	DRM_DEBUG_KMS("Writing base %08X %08lX %d %d %d\n",
> > +			obj->gtt_offset, intel_crtc->dspaddr_offset,
> > +			x, y, fb->pitches[0]);
> > +
> > +	/* Emit MI commands here */
> > +	ret = intel_ring_begin(ring, 10);
> > +	if (ret)
> > +		return ret;
> > +	intel_ring_emit(ring, MI_NOOP);
> > +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > +	intel_ring_emit(ring, DSPOFFSET(plane));
> > +	intel_ring_emit(ring, (y << 16) | x);
> > +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > +	intel_ring_emit(ring, DSPSTRIDE(plane));
> > +	intel_ring_emit(ring, fb->pitches[0]);
> > +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > +	intel_ring_emit(ring, DSPSURF(plane));
> > +	intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
> > +	intel_ring_advance(ring);
> > +
> > +	return 0;
> > +}
> > +
> >  static int ironlake_update_plane(struct drm_crtc *crtc,
> >  				 struct drm_framebuffer *fb, int x, int y)  { @@ -7494,9 +7617,26 
> > @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  	struct drm_framebuffer *old_fb = crtc->fb;
> >  	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_framebuffer *intel_fb;
> >  	struct intel_unpin_work *work;
> >  	unsigned long flags;
> >  	int ret;
> > +	/*
> > +	* TILEOFF registers can't be changed via MI display flips.
> > +	* Changes in pitches[0] and offsets[0] updates the primary plane
> > +	* control registers.
> > +	*/
> > +	intel_fb = to_intel_framebuffer(crtc->fb);
> > +	if ((IS_HASWELL(dev)) &&
> > +		((obj->tiling_mode != intel_fb->obj->tiling_mode) ||
> > +		(fb->offsets[0] != crtc->fb->offsets[0]) ||
> > +		(fb->pitches[0] != crtc->fb->pitches[0]))) {
> > +			DRM_DEBUG_DRIVER(" crtc fb: pitch = %d offset = %d\n",
> > +			crtc->fb->pitches[0], crtc->fb->offsets[0]);
> > +			DRM_DEBUG_DRIVER(" input fb: pitch = %d offset = %d\n",
> > +			fb->pitches[0], fb->offsets[0]);
> > +			haswell_update_plane(crtc, fb, 0, 0);
> > +	}
> >  
> >  	work = kzalloc(sizeof *work, GFP_KERNEL);
> >  	if (work == NULL)
> > @@ -8766,6 +8906,20 @@ out_config:
> >  	return ret;
> >  }
> >  
> > +/* Callback function - Called if change in pixel format is detected.
> > +* Sends MI command to update change in pixel format.
> > +*/
> > +static int intel_crtc_set_pixel_format(struct drm_crtc *crtc, u32 
> > +pixel_format) {
> > +	struct drm_device *dev = crtc->dev;
> > +	if (IS_HASWELL(dev)) {
> > +		return haswell_set_pixelformat(crtc, pixel_format);
> > +	} else {
> > +		DRM_ERROR("Pixel format change not allowed.\n");
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >  static const struct drm_crtc_funcs intel_crtc_funcs = {
> >  	.cursor_set = intel_crtc_cursor_set,
> >  	.cursor_move = intel_crtc_cursor_move, @@ -8773,6 +8927,7 @@ static 
> > const struct drm_crtc_funcs intel_crtc_funcs = {
> >  	.set_config = intel_crtc_set_config,
> >  	.destroy = intel_crtc_destroy,
> >  	.page_flip = intel_crtc_page_flip,
> > +	.set_pixelformat = intel_crtc_set_pixel_format,
> >  };
> >  
> >  static void intel_cpu_pll_init(struct drm_device *dev) @@ -10180,3 
> > +10335,4 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
> >  	}
> >  }
> >  #endif
> > +
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 
> > e215bcc..9a0ea92 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -323,6 +323,7 @@ struct drm_plane;
> >   * @set_property: called when a property is changed
> >   * @set_config: apply a new CRTC configuration
> >   * @page_flip: initiate a page flip
> > + * @set_pixelformat: apply new pixel format to primary plane control 
> > + register
> >   *
> >   * The drm_crtc_funcs structure is the central CRTC management structure
> >   * in the DRM.  Each CRTC controls one or more connectors (note that 
> > the name @@ -369,6 +370,13 @@ struct drm_crtc_funcs {
> >  
> >  	int (*set_property)(struct drm_crtc *crtc,
> >  			    struct drm_property *property, uint64_t val);
> > +	/*
> > +	* Update the primary plane pixel format register during page flip.
> > +	* To support dynamic change in pixel format define the callback
> > +	* function for set_pixelformat.
> > +	*/
> > +	int (*set_pixelformat)(struct drm_crtc *crtc,
> > +				uint32_t pixel_format);
> >  };
> >  
> >  /**
> > --
> > 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

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.

[Ramakutty, SandeepX]

Hi Ville,

Thanks for the information. Here MI_LOAD_REGISTER_IMM is used and not MI_LOAD_REGISTER_MEM. Is SRM needed after MI_LOAD_REGISTER_IMM? BSpec for MI_LOAD_REGISTER_IMM  does not mention any thing about using SRM, but it is there for MI_LOAD_REGISTER_MEM. 

Regards,
Sandeep

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Thursday, October 31, 2013 5:54 PM
To: Ramakutty, SandeepX
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.

On Thu, Oct 31, 2013 at 12:15:17PM +0000, Ramakutty, SandeepX wrote:
> Hi Ville ,
> 
> Thanks for the feedback.
> 
> We verified without updating watermarks and found that the video playback, 3DMark and GLBenchmark plays fine. There was no underrun error too.
> Cases tried out-
> Pixel format changed to 32BPP when water mark calculated based on 
> 16BPP Pixel format changed to 16BPP when water mark calculated based on 32BPP.
> 
> In both cases, we verified with video playback, 3DMark and GLBenchmark and did not see any underrun error.  Is there any specific situations in which not setting WM can throw up an issue? 
> 
> With MMIO there may be a possibility of flickering. The pixel format may be updated well before a page flip. With MMIO, the format will be written to register before a page flip and this can cause unwanted effects.  Hence we used MI commands as this will get updated much nearer to a page flip.
> 
> What is  SRM?

"store register mem". It reads the register and stores the result to memory.

BSpec tells us that there can only be one outstanding LRI targeted at the display registers at any given. So you need to do LRI+SRM for every display register you want to write w/ LRI.

> 
> Regards,
> Sandeep
> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Friday, September 13, 2013 7:56 PM
> To: Ramakutty, SandeepX
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.
> 
> On Fri, Sep 13, 2013 at 07:05:31PM +0530, Sandeep Ramankutty wrote:
> > Change: Add support to change the pixel format, base surface 
> > address, tiling mode, tiling offset on the flow during the primary plane flip.
> > 
> > Issue: This support is needed by hardware composer to meet the 
> > performance optimization requirement for 3D benchmark. This patch 
> > enables change in pixel format and tiling params without adding and removing the framebuffer.
> > Adding and removing the framebuffer impacts performance.
> 
> We can't do it quite like this unfortunately. Watermarksm may need to be updated carefully if the bpp changes. Also updating multiple plane registers isn't always atomic, so we anyway need the vblank evade tricks to make it look pretty. I suppose it might be possible to make all that work via the ring, but my plan is to go for MMIO, and later investigate if we can optimize it by using the ring.
> 
> As far as the lowlevel details go, you supposedly need a SRM after each LRI aimed at the display registers. Otherwise I guess you could even blast all the registers with a single LRI.
> 
> > Change Details-
> > drm: Defined function pointer set_pixelformat in drm_crtc_funcs. 
> > drm_crtc.c is modified to execute the function to update pixel format.
> > drm/i915: intel_crtc_set_pixel_format() implemented in i915 driver 
> > code (intel_display.c) and set to drm_crtc_funcs->set_pixelformat.
> > haswell_set_pixelformat() implemented to update pixel format. This 
> > is specific to Haswell
> > haswell_update_plane() implemented to update changes in tiling 
> > offset, surface base address and stride-fb->pitches[0]. Sends MI 
> > command to update the params in primary plane control registers.
> > 
> > Change-Id: I4b0af83f641b2b79ee4f5c3401790d7d90ccd9ef
> > Signed-off-by: Pallavi G <pallavi.g@intel.com>
> > Signed-off-by: Sandeep Ramakutty <sandeepx.ramakutty@intel.com>
> > ---
> >  drivers/gpu/drm/drm_crtc.c           |   15 +++-
> >  drivers/gpu/drm/i915/intel_display.c |  156 ++++++++++++++++++++++++++++++++++
> >  include/drm/drm_crtc.h               |    8 ++
> >  3 files changed, 176 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c 
> > index c9f9f3d..4739b6c 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -3518,9 +3518,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  	}
> >  
> >  	if (crtc->fb->pixel_format != fb->pixel_format) {
> > -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > -		ret = -EINVAL;
> > -		goto out;
> > +		if (crtc->funcs->set_pixelformat == NULL) {
> > +			DRM_DEBUG_KMS("Pixel format change not allowed");
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +		/* supports dynamic change in pixel format */
> > +		ret = crtc->funcs->set_pixelformat(crtc, fb->pixel_format);
> > +		if (ret) {
> > +			DRM_DEBUG_KMS("Pixel format change failed %d",
> > +							fb->pixel_format);
> > +			goto out;
> > +		}
> >  	}
> >  
> >  	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { diff --git 
> > a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 955df91..7bfe088 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -45,6 +45,7 @@ bool intel_pipe_has_type(struct drm_crtc *crtc, 
> > int type);  static void intel_increase_pllclock(struct drm_crtc 
> > *crtc); static void intel_crtc_update_cursor(struct drm_crtc *crtc, 
> > bool on);
> >  
> > +
> >  typedef struct {
> >  	int	min, max;
> >  } intel_range_t;
> > @@ -1959,6 +1960,128 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> >  	return 0;
> >  }
> >  
> > +/* Set Pixel format for Haswell using MI commands */ static int 
> > +haswell_set_pixelformat(struct drm_crtc *crtc, u32 pixel_format) {
> > +	u32 dspcntr, reg;
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> > +	int ret = 0;
> > +
> > +	reg = DSPCNTR(intel_crtc->pipe);
> > +	dspcntr = I915_READ(reg);
> > +	DRM_DEBUG_DRIVER("pixel format = %d\n", pixel_format);
> > +	/* Mask out pixel format bits in case we change it */
> > +	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
> > +	switch (pixel_format) {
> > +	case DRM_FORMAT_C8:
> > +		dspcntr |= DISPPLANE_8BPP;
> > +		break;
> > +	case DRM_FORMAT_XRGB1555:
> > +	case DRM_FORMAT_ARGB1555:
> > +		dspcntr |= DISPPLANE_BGRX555;
> > +		break;
> > +	case DRM_FORMAT_RGB565:
> > +		dspcntr |= DISPPLANE_BGRX565;
> > +		break;
> > +	case DRM_FORMAT_XRGB8888:
> > +	case DRM_FORMAT_ARGB8888:
> > +		dspcntr |= DISPPLANE_BGRX888;
> > +		break;
> > +	case DRM_FORMAT_XBGR8888:
> > +	case DRM_FORMAT_ABGR8888:
> > +		dspcntr |= DISPPLANE_RGBX888;
> > +		break;
> > +	case DRM_FORMAT_XRGB2101010:
> > +	case DRM_FORMAT_ARGB2101010:
> > +		dspcntr |= DISPPLANE_BGRX101010;
> > +		break;
> > +	case DRM_FORMAT_XBGR2101010:
> > +	case DRM_FORMAT_ABGR2101010:
> > +		dspcntr |= DISPPLANE_RGBX101010;
> > +		break;
> > +	default:
> > +		DRM_ERROR("Unsupported pixel format 0x%08x\n", pixel_format);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Write pixel format update command to ring */
> > +	ret = intel_ring_begin(ring, 4);
> > +	if (ret) {
> > +		DRM_ERROR("MI Command emit failed.\n");
> > +		return ret;
> > +	}
> > +	intel_ring_emit(ring, MI_NOOP);
> > +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > +	intel_ring_emit(ring, reg);
> > +	intel_ring_emit(ring, dspcntr);
> > +	intel_ring_advance(ring);
> > +	return 0;
> > +}
> > +
> > +/* Update the primary plane registers. Uses MI commands */ static 
> > +int haswell_update_plane(struct drm_crtc *crtc,
> > +				struct drm_framebuffer *fb, int x, int y) {
> > +	int ret;
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> > +	struct drm_i915_gem_object *obj;
> > +	int plane = intel_crtc->plane;
> > +
> > +	switch (plane) {
> > +	case 0:
> > +	case 1:
> > +	case 2:
> > +		break;
> > +	default:
> > +		DRM_ERROR("Can't update plane %c\n", plane_name(plane));
> > +		return -EINVAL;
> > +	}
> > +	obj = to_intel_framebuffer(fb)->obj;
> > +
> > +	/* Set pixel format */
> > +	haswell_set_pixelformat(crtc, fb->pixel_format);
> > +
> > +	/* Set tiling offsets. Tiling mode is not set here as    *
> > +	* it is set from intel_gen7_queue_flip. Send MI Command  *
> > +	* to change -                                            *
> > +	* 1. Tiling offset                                       *
> > +	* 2. stride - fb->pitches[0]                             *
> > +	* 2. surface base address                                *
> > +	* Linear offset and tile offset is same for Haswell      */
> > +	intel_crtc->dspaddr_offset =
> > +		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
> > +						fb->bits_per_pixel / 8,
> > +						fb->pitches[0]);
> > +
> > +	DRM_DEBUG_KMS("Writing base %08X %08lX %d %d %d\n",
> > +			obj->gtt_offset, intel_crtc->dspaddr_offset,
> > +			x, y, fb->pitches[0]);
> > +
> > +	/* Emit MI commands here */
> > +	ret = intel_ring_begin(ring, 10);
> > +	if (ret)
> > +		return ret;
> > +	intel_ring_emit(ring, MI_NOOP);
> > +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > +	intel_ring_emit(ring, DSPOFFSET(plane));
> > +	intel_ring_emit(ring, (y << 16) | x);
> > +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > +	intel_ring_emit(ring, DSPSTRIDE(plane));
> > +	intel_ring_emit(ring, fb->pitches[0]);
> > +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > +	intel_ring_emit(ring, DSPSURF(plane));
> > +	intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
> > +	intel_ring_advance(ring);
> > +
> > +	return 0;
> > +}
> > +
> >  static int ironlake_update_plane(struct drm_crtc *crtc,
> >  				 struct drm_framebuffer *fb, int x, int y)  { @@ -7494,9 
> > +7617,26 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  	struct drm_framebuffer *old_fb = crtc->fb;
> >  	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +	struct intel_framebuffer *intel_fb;
> >  	struct intel_unpin_work *work;
> >  	unsigned long flags;
> >  	int ret;
> > +	/*
> > +	* TILEOFF registers can't be changed via MI display flips.
> > +	* Changes in pitches[0] and offsets[0] updates the primary plane
> > +	* control registers.
> > +	*/
> > +	intel_fb = to_intel_framebuffer(crtc->fb);
> > +	if ((IS_HASWELL(dev)) &&
> > +		((obj->tiling_mode != intel_fb->obj->tiling_mode) ||
> > +		(fb->offsets[0] != crtc->fb->offsets[0]) ||
> > +		(fb->pitches[0] != crtc->fb->pitches[0]))) {
> > +			DRM_DEBUG_DRIVER(" crtc fb: pitch = %d offset = %d\n",
> > +			crtc->fb->pitches[0], crtc->fb->offsets[0]);
> > +			DRM_DEBUG_DRIVER(" input fb: pitch = %d offset = %d\n",
> > +			fb->pitches[0], fb->offsets[0]);
> > +			haswell_update_plane(crtc, fb, 0, 0);
> > +	}
> >  
> >  	work = kzalloc(sizeof *work, GFP_KERNEL);
> >  	if (work == NULL)
> > @@ -8766,6 +8906,20 @@ out_config:
> >  	return ret;
> >  }
> >  
> > +/* Callback function - Called if change in pixel format is detected.
> > +* Sends MI command to update change in pixel format.
> > +*/
> > +static int intel_crtc_set_pixel_format(struct drm_crtc *crtc, u32
> > +pixel_format) {
> > +	struct drm_device *dev = crtc->dev;
> > +	if (IS_HASWELL(dev)) {
> > +		return haswell_set_pixelformat(crtc, pixel_format);
> > +	} else {
> > +		DRM_ERROR("Pixel format change not allowed.\n");
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >  static const struct drm_crtc_funcs intel_crtc_funcs = {
> >  	.cursor_set = intel_crtc_cursor_set,
> >  	.cursor_move = intel_crtc_cursor_move, @@ -8773,6 +8927,7 @@ 
> > static const struct drm_crtc_funcs intel_crtc_funcs = {
> >  	.set_config = intel_crtc_set_config,
> >  	.destroy = intel_crtc_destroy,
> >  	.page_flip = intel_crtc_page_flip,
> > +	.set_pixelformat = intel_crtc_set_pixel_format,
> >  };
> >  
> >  static void intel_cpu_pll_init(struct drm_device *dev) @@ -10180,3
> > +10335,4 @@ intel_display_print_error_state(struct 
> > +drm_i915_error_state_buf *m,
> >  	}
> >  }
> >  #endif
> > +
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index
> > e215bcc..9a0ea92 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -323,6 +323,7 @@ struct drm_plane;
> >   * @set_property: called when a property is changed
> >   * @set_config: apply a new CRTC configuration
> >   * @page_flip: initiate a page flip
> > + * @set_pixelformat: apply new pixel format to primary plane 
> > + control register
> >   *
> >   * The drm_crtc_funcs structure is the central CRTC management structure
> >   * in the DRM.  Each CRTC controls one or more connectors (note 
> > that the name @@ -369,6 +370,13 @@ struct drm_crtc_funcs {
> >  
> >  	int (*set_property)(struct drm_crtc *crtc,
> >  			    struct drm_property *property, uint64_t val);
> > +	/*
> > +	* Update the primary plane pixel format register during page flip.
> > +	* To support dynamic change in pixel format define the callback
> > +	* function for set_pixelformat.
> > +	*/
> > +	int (*set_pixelformat)(struct drm_crtc *crtc,
> > +				uint32_t pixel_format);
> >  };
> >  
> >  /**
> > --
> > 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

--
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.

[Ville Syrjälä]

On Tue, Nov 05, 2013 at 07:00:14AM +0000, Ramakutty, SandeepX wrote:
> Hi Ville,
> 
> Thanks for the information. Here MI_LOAD_REGISTER_IMM is used and not MI_LOAD_REGISTER_MEM. Is SRM needed after MI_LOAD_REGISTER_IMM? BSpec for MI_LOAD_REGISTER_IMM  does not mention any thing about using SRM, but it is there for MI_LOAD_REGISTER_MEM. 

Just checked BSpec and it's still there:

"Limited LRI cycles to the Display Engine 0x40000-0xBFFFF) are allowed,
 but must be spaced to allow only one pending at a time. This can be done
 by issuing an SRM to the same address immediately after each LRI."


> 
> Regards,
> Sandeep
> 
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Thursday, October 31, 2013 5:54 PM
> To: Ramakutty, SandeepX
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.
> 
> On Thu, Oct 31, 2013 at 12:15:17PM +0000, Ramakutty, SandeepX wrote:
> > Hi Ville ,
> > 
> > Thanks for the feedback.
> > 
> > We verified without updating watermarks and found that the video playback, 3DMark and GLBenchmark plays fine. There was no underrun error too.
> > Cases tried out-
> > Pixel format changed to 32BPP when water mark calculated based on 
> > 16BPP Pixel format changed to 16BPP when water mark calculated based on 32BPP.
> > 
> > In both cases, we verified with video playback, 3DMark and GLBenchmark and did not see any underrun error.  Is there any specific situations in which not setting WM can throw up an issue? 
> > 
> > With MMIO there may be a possibility of flickering. The pixel format may be updated well before a page flip. With MMIO, the format will be written to register before a page flip and this can cause unwanted effects.  Hence we used MI commands as this will get updated much nearer to a page flip.
> > 
> > What is  SRM?
> 
> "store register mem". It reads the register and stores the result to memory.
> 
> BSpec tells us that there can only be one outstanding LRI targeted at the display registers at any given. So you need to do LRI+SRM for every display register you want to write w/ LRI.
> 
> > 
> > Regards,
> > Sandeep
> > 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Friday, September 13, 2013 7:56 PM
> > To: Ramakutty, SandeepX
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Add support to change pixel format, tiling mode, tiling offset.
> > 
> > On Fri, Sep 13, 2013 at 07:05:31PM +0530, Sandeep Ramankutty wrote:
> > > Change: Add support to change the pixel format, base surface 
> > > address, tiling mode, tiling offset on the flow during the primary plane flip.
> > > 
> > > Issue: This support is needed by hardware composer to meet the 
> > > performance optimization requirement for 3D benchmark. This patch 
> > > enables change in pixel format and tiling params without adding and removing the framebuffer.
> > > Adding and removing the framebuffer impacts performance.
> > 
> > We can't do it quite like this unfortunately. Watermarksm may need to be updated carefully if the bpp changes. Also updating multiple plane registers isn't always atomic, so we anyway need the vblank evade tricks to make it look pretty. I suppose it might be possible to make all that work via the ring, but my plan is to go for MMIO, and later investigate if we can optimize it by using the ring.
> > 
> > As far as the lowlevel details go, you supposedly need a SRM after each LRI aimed at the display registers. Otherwise I guess you could even blast all the registers with a single LRI.
> > 
> > > Change Details-
> > > drm: Defined function pointer set_pixelformat in drm_crtc_funcs. 
> > > drm_crtc.c is modified to execute the function to update pixel format.
> > > drm/i915: intel_crtc_set_pixel_format() implemented in i915 driver 
> > > code (intel_display.c) and set to drm_crtc_funcs->set_pixelformat.
> > > haswell_set_pixelformat() implemented to update pixel format. This 
> > > is specific to Haswell
> > > haswell_update_plane() implemented to update changes in tiling 
> > > offset, surface base address and stride-fb->pitches[0]. Sends MI 
> > > command to update the params in primary plane control registers.
> > > 
> > > Change-Id: I4b0af83f641b2b79ee4f5c3401790d7d90ccd9ef
> > > Signed-off-by: Pallavi G <pallavi.g@intel.com>
> > > Signed-off-by: Sandeep Ramakutty <sandeepx.ramakutty@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_crtc.c           |   15 +++-
> > >  drivers/gpu/drm/i915/intel_display.c |  156 ++++++++++++++++++++++++++++++++++
> > >  include/drm/drm_crtc.h               |    8 ++
> > >  3 files changed, 176 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c 
> > > index c9f9f3d..4739b6c 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -3518,9 +3518,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> > >  	}
> > >  
> > >  	if (crtc->fb->pixel_format != fb->pixel_format) {
> > > -		DRM_DEBUG_KMS("Page flip is not allowed to change frame buffer format.\n");
> > > -		ret = -EINVAL;
> > > -		goto out;
> > > +		if (crtc->funcs->set_pixelformat == NULL) {
> > > +			DRM_DEBUG_KMS("Pixel format change not allowed");
> > > +			ret = -EINVAL;
> > > +			goto out;
> > > +		}
> > > +		/* supports dynamic change in pixel format */
> > > +		ret = crtc->funcs->set_pixelformat(crtc, fb->pixel_format);
> > > +		if (ret) {
> > > +			DRM_DEBUG_KMS("Pixel format change failed %d",
> > > +							fb->pixel_format);
> > > +			goto out;
> > > +		}
> > >  	}
> > >  
> > >  	if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { diff --git 
> > > a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 955df91..7bfe088 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -45,6 +45,7 @@ bool intel_pipe_has_type(struct drm_crtc *crtc, 
> > > int type);  static void intel_increase_pllclock(struct drm_crtc 
> > > *crtc); static void intel_crtc_update_cursor(struct drm_crtc *crtc, 
> > > bool on);
> > >  
> > > +
> > >  typedef struct {
> > >  	int	min, max;
> > >  } intel_range_t;
> > > @@ -1959,6 +1960,128 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> > >  	return 0;
> > >  }
> > >  
> > > +/* Set Pixel format for Haswell using MI commands */ static int 
> > > +haswell_set_pixelformat(struct drm_crtc *crtc, u32 pixel_format) {
> > > +	u32 dspcntr, reg;
> > > +	struct drm_device *dev = crtc->dev;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> > > +	int ret = 0;
> > > +
> > > +	reg = DSPCNTR(intel_crtc->pipe);
> > > +	dspcntr = I915_READ(reg);
> > > +	DRM_DEBUG_DRIVER("pixel format = %d\n", pixel_format);
> > > +	/* Mask out pixel format bits in case we change it */
> > > +	dspcntr &= ~DISPPLANE_PIXFORMAT_MASK;
> > > +	switch (pixel_format) {
> > > +	case DRM_FORMAT_C8:
> > > +		dspcntr |= DISPPLANE_8BPP;
> > > +		break;
> > > +	case DRM_FORMAT_XRGB1555:
> > > +	case DRM_FORMAT_ARGB1555:
> > > +		dspcntr |= DISPPLANE_BGRX555;
> > > +		break;
> > > +	case DRM_FORMAT_RGB565:
> > > +		dspcntr |= DISPPLANE_BGRX565;
> > > +		break;
> > > +	case DRM_FORMAT_XRGB8888:
> > > +	case DRM_FORMAT_ARGB8888:
> > > +		dspcntr |= DISPPLANE_BGRX888;
> > > +		break;
> > > +	case DRM_FORMAT_XBGR8888:
> > > +	case DRM_FORMAT_ABGR8888:
> > > +		dspcntr |= DISPPLANE_RGBX888;
> > > +		break;
> > > +	case DRM_FORMAT_XRGB2101010:
> > > +	case DRM_FORMAT_ARGB2101010:
> > > +		dspcntr |= DISPPLANE_BGRX101010;
> > > +		break;
> > > +	case DRM_FORMAT_XBGR2101010:
> > > +	case DRM_FORMAT_ABGR2101010:
> > > +		dspcntr |= DISPPLANE_RGBX101010;
> > > +		break;
> > > +	default:
> > > +		DRM_ERROR("Unsupported pixel format 0x%08x\n", pixel_format);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* Write pixel format update command to ring */
> > > +	ret = intel_ring_begin(ring, 4);
> > > +	if (ret) {
> > > +		DRM_ERROR("MI Command emit failed.\n");
> > > +		return ret;
> > > +	}
> > > +	intel_ring_emit(ring, MI_NOOP);
> > > +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > > +	intel_ring_emit(ring, reg);
> > > +	intel_ring_emit(ring, dspcntr);
> > > +	intel_ring_advance(ring);
> > > +	return 0;
> > > +}
> > > +
> > > +/* Update the primary plane registers. Uses MI commands */ static 
> > > +int haswell_update_plane(struct drm_crtc *crtc,
> > > +				struct drm_framebuffer *fb, int x, int y) {
> > > +	int ret;
> > > +	struct drm_device *dev = crtc->dev;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
> > > +	struct drm_i915_gem_object *obj;
> > > +	int plane = intel_crtc->plane;
> > > +
> > > +	switch (plane) {
> > > +	case 0:
> > > +	case 1:
> > > +	case 2:
> > > +		break;
> > > +	default:
> > > +		DRM_ERROR("Can't update plane %c\n", plane_name(plane));
> > > +		return -EINVAL;
> > > +	}
> > > +	obj = to_intel_framebuffer(fb)->obj;
> > > +
> > > +	/* Set pixel format */
> > > +	haswell_set_pixelformat(crtc, fb->pixel_format);
> > > +
> > > +	/* Set tiling offsets. Tiling mode is not set here as    *
> > > +	* it is set from intel_gen7_queue_flip. Send MI Command  *
> > > +	* to change -                                            *
> > > +	* 1. Tiling offset                                       *
> > > +	* 2. stride - fb->pitches[0]                             *
> > > +	* 2. surface base address                                *
> > > +	* Linear offset and tile offset is same for Haswell      */
> > > +	intel_crtc->dspaddr_offset =
> > > +		intel_gen4_compute_page_offset(&x, &y, obj->tiling_mode,
> > > +						fb->bits_per_pixel / 8,
> > > +						fb->pitches[0]);
> > > +
> > > +	DRM_DEBUG_KMS("Writing base %08X %08lX %d %d %d\n",
> > > +			obj->gtt_offset, intel_crtc->dspaddr_offset,
> > > +			x, y, fb->pitches[0]);
> > > +
> > > +	/* Emit MI commands here */
> > > +	ret = intel_ring_begin(ring, 10);
> > > +	if (ret)
> > > +		return ret;
> > > +	intel_ring_emit(ring, MI_NOOP);
> > > +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > > +	intel_ring_emit(ring, DSPOFFSET(plane));
> > > +	intel_ring_emit(ring, (y << 16) | x);
> > > +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > > +	intel_ring_emit(ring, DSPSTRIDE(plane));
> > > +	intel_ring_emit(ring, fb->pitches[0]);
> > > +	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> > > +	intel_ring_emit(ring, DSPSURF(plane));
> > > +	intel_ring_emit(ring, obj->gtt_offset + intel_crtc->dspaddr_offset);
> > > +	intel_ring_advance(ring);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int ironlake_update_plane(struct drm_crtc *crtc,
> > >  				 struct drm_framebuffer *fb, int x, int y)  { @@ -7494,9 
> > > +7617,26 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> > >  	struct drm_framebuffer *old_fb = crtc->fb;
> > >  	struct drm_i915_gem_object *obj = to_intel_framebuffer(fb)->obj;
> > >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > +	struct intel_framebuffer *intel_fb;
> > >  	struct intel_unpin_work *work;
> > >  	unsigned long flags;
> > >  	int ret;
> > > +	/*
> > > +	* TILEOFF registers can't be changed via MI display flips.
> > > +	* Changes in pitches[0] and offsets[0] updates the primary plane
> > > +	* control registers.
> > > +	*/
> > > +	intel_fb = to_intel_framebuffer(crtc->fb);
> > > +	if ((IS_HASWELL(dev)) &&
> > > +		((obj->tiling_mode != intel_fb->obj->tiling_mode) ||
> > > +		(fb->offsets[0] != crtc->fb->offsets[0]) ||
> > > +		(fb->pitches[0] != crtc->fb->pitches[0]))) {
> > > +			DRM_DEBUG_DRIVER(" crtc fb: pitch = %d offset = %d\n",
> > > +			crtc->fb->pitches[0], crtc->fb->offsets[0]);
> > > +			DRM_DEBUG_DRIVER(" input fb: pitch = %d offset = %d\n",
> > > +			fb->pitches[0], fb->offsets[0]);
> > > +			haswell_update_plane(crtc, fb, 0, 0);
> > > +	}
> > >  
> > >  	work = kzalloc(sizeof *work, GFP_KERNEL);
> > >  	if (work == NULL)
> > > @@ -8766,6 +8906,20 @@ out_config:
> > >  	return ret;
> > >  }
> > >  
> > > +/* Callback function - Called if change in pixel format is detected.
> > > +* Sends MI command to update change in pixel format.
> > > +*/
> > > +static int intel_crtc_set_pixel_format(struct drm_crtc *crtc, u32
> > > +pixel_format) {
> > > +	struct drm_device *dev = crtc->dev;
> > > +	if (IS_HASWELL(dev)) {
> > > +		return haswell_set_pixelformat(crtc, pixel_format);
> > > +	} else {
> > > +		DRM_ERROR("Pixel format change not allowed.\n");
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > >  static const struct drm_crtc_funcs intel_crtc_funcs = {
> > >  	.cursor_set = intel_crtc_cursor_set,
> > >  	.cursor_move = intel_crtc_cursor_move, @@ -8773,6 +8927,7 @@ 
> > > static const struct drm_crtc_funcs intel_crtc_funcs = {
> > >  	.set_config = intel_crtc_set_config,
> > >  	.destroy = intel_crtc_destroy,
> > >  	.page_flip = intel_crtc_page_flip,
> > > +	.set_pixelformat = intel_crtc_set_pixel_format,
> > >  };
> > >  
> > >  static void intel_cpu_pll_init(struct drm_device *dev) @@ -10180,3
> > > +10335,4 @@ intel_display_print_error_state(struct 
> > > +drm_i915_error_state_buf *m,
> > >  	}
> > >  }
> > >  #endif
> > > +
> > > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index
> > > e215bcc..9a0ea92 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -323,6 +323,7 @@ struct drm_plane;
> > >   * @set_property: called when a property is changed
> > >   * @set_config: apply a new CRTC configuration
> > >   * @page_flip: initiate a page flip
> > > + * @set_pixelformat: apply new pixel format to primary plane 
> > > + control register
> > >   *
> > >   * The drm_crtc_funcs structure is the central CRTC management structure
> > >   * in the DRM.  Each CRTC controls one or more connectors (note 
> > > that the name @@ -369,6 +370,13 @@ struct drm_crtc_funcs {
> > >  
> > >  	int (*set_property)(struct drm_crtc *crtc,
> > >  			    struct drm_property *property, uint64_t val);
> > > +	/*
> > > +	* Update the primary plane pixel format register during page flip.
> > > +	* To support dynamic change in pixel format define the callback
> > > +	* function for set_pixelformat.
> > > +	*/
> > > +	int (*set_pixelformat)(struct drm_crtc *crtc,
> > > +				uint32_t pixel_format);
> > >  };
> > >  
> > >  /**
> > > --
> > > 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
> 
> --
> Ville Syrjälä
> Intel OTC

-- 
Ville Syrjälä
Intel OTC