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

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Damien Lespiau <damien.lespiau@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 8/8] drm/i915/skl: Provide a Skylake version of get_plane_config()
Date: Fri, 31 Oct 2014 11:38:34 +0000	[thread overview]
Message-ID: <545374BA.4040508@linux.intel.com> (raw)
In-Reply-To: <1414603367-24964-9-git-send-email-damien.lespiau@intel.com>

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?

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

> +	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?

Regards,

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