Re: [PATCH 8/8] drm/i915/skl: Provide a Skylake version of get_plane_config()

[Damien Lespiau <damien.lespiau@intel.com>]

On Fri, Oct 31, 2014 at 11:38:34AM +0000, Tvrtko Ursulin wrote:
> 
> On 10/29/2014 05:22 PM, Damien Lespiau wrote:
> >Universal planes have changed a bit the register organization.
> >
> >Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> >---
> >  drivers/gpu/drm/i915/intel_display.c | 108 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 101 insertions(+), 7 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >index 02b2a97..33e8112 100644
> >--- a/drivers/gpu/drm/i915/intel_display.c
> >+++ b/drivers/gpu/drm/i915/intel_display.c
> >@@ -2314,6 +2314,32 @@ static int i9xx_format_to_fourcc(int format)
> >  	}
> >  }
> >
> >+static int skl_format_to_fourcc(int format, bool rgb_order, bool alpha)
> >+{
> >+	switch (format) {
> >+	case PLANE_CTL_FORMAT_RGB_565:
> >+		return DRM_FORMAT_RGB565;
> >+	default:
> >+	case PLANE_CTL_FORMAT_XRGB_8888:
> >+		if (rgb_order) {
> >+			if (alpha)
> >+				return DRM_FORMAT_ABGR8888;
> >+			else
> >+				return DRM_FORMAT_XBGR8888;
> >+		} else {
> >+			if (alpha)
> >+				return DRM_FORMAT_ARGB8888;
> >+			else
> >+				return DRM_FORMAT_XRGB8888;
> >+		}
> >+	case PLANE_CTL_FORMAT_XRGB_2101010:
> >+		if (rgb_order)
> >+			return DRM_FORMAT_XBGR2101010;
> >+		else
> >+			return DRM_FORMAT_XRGB2101010;
> >+	}
> >+}
> 
> Are RGB and BGR wrong way round here?

Nop that's due to a mismatch of endianness between the DRM defines and
the register field (eg. intel_sprite.c is full of those inversions).

> >  static bool intel_alloc_plane_obj(struct intel_crtc *crtc,
> >  				  struct intel_plane_config *plane_config)
> >  {
> >@@ -7456,6 +7482,69 @@ static void ironlake_get_fdi_m_n_config(struct intel_crtc *crtc,
> >  				     &pipe_config->fdi_m_n, NULL);
> >  }
> >
> >+static void skylake_get_plane_config(struct intel_crtc *crtc,
> >+				      struct intel_plane_config *plane_config)
> >+{
> >+	struct drm_device *dev = crtc->base.dev;
> >+	struct drm_i915_private *dev_priv = dev->dev_private;
> >+	u32 val, base, offset, stride_mult;
> >+	int pipe = crtc->pipe;
> >+	int fourcc, pixel_format;
> >+	int aligned_height;
> >+	struct drm_framebuffer *fb;
> >+
> >+	fb = kzalloc(sizeof(struct intel_framebuffer), GFP_KERNEL);
> >+	if (!fb) {
> >+		DRM_DEBUG_KMS("failed to alloc fb\n");
> >+		return;
> >+	}
> >+
> >+	val = I915_READ(PLANE_CTL(pipe, 0));
> >+	if (val & PLANE_CTL_TILED_MASK)
> >+		plane_config->tiling = I915_TILING_X;
> >+
> >+	pixel_format = val & PLANE_CTL_FORMAT_MASK;
> >+	fourcc = skl_format_to_fourcc(pixel_format,
> >+				      val & PLANE_CTL_ORDER_RGBX,
> >+				      val & PLANE_CTL_ALPHA_MASK);
> >+	fb->pixel_format = fourcc;
> >+	fb->bits_per_pixel = drm_format_plane_cpp(fourcc, 0) * 8;
> >+
> >+	base = I915_READ(PLANE_SURF(pipe, 0)) & 0xfffff000;
> >+	plane_config->base = base;
> >+
> >+	offset = I915_READ(PLANE_OFFSET(pipe, 0));
> >+
> >+	val = I915_READ(PIPESRC(pipe));
> >+	fb->width = ((val >> 16) & 0xfff) + 1;
> >+	fb->height = ((val >> 0) & 0xfff) + 1;
> 
> Thinking how you are reading out plane register everywhere else, I assume
> this means _PIPEASRC is guaranteed to be configured to the same size as
> _PLANE_SIZE_1_A?

"guaranteed" is a strong word, but yes, I don't expect other
configuration. That said, we now have PLANE_SIZE on gen9+, so we might
as well use it, that's indeed more correct.

> >+	val = I915_READ(PLANE_STRIDE(pipe, 0));
> >+	switch (plane_config->tiling) {
> >+	case I915_TILING_NONE:
> >+		stride_mult = 64;
> >+		break;
> >+	case I915_TILING_X:
> >+		stride_mult = 512;
> >+		break;
> >+	default:
> >+		BUG();
> 
> As some other people I also dislike BUGs very much. WARN and put no fb on
> screen instead?

Sure, removed the BUG().

-- 
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx