On Thu, Sep 4, 2014 at 4:26 AM, Damien Lespiau <damien.lesp...@intel.com>
wrote:
> Skylake makes primary planes the same as sprite planes and call the
> result "universal planes".
>
> This commit emulates a primary plane with plane 0, taking the
> opportunity to redefine primary and sprite registers to be identical now
> that the underlying hardware is. It also makes sense as plenty of fields
> have changed.
>
> v2: Rebase on top of the vma code.
>
> v3: Follow upstream evolution:
> - Drop return values.
> - Remove pipe checks since redudant and BUG instead.
> - Remove tiling checks and BUG instead.
> - Drop commented out DISP_MODIFY usage.
>
> v4: s/plane/primary_plane/
>
> v5: Misc fixes:
> - Fix the fields we need to clear up
> - Disable trickle feed
> - Correctly use PLANE_OFFSET for the panning
>
> v6: (Jesse)
> Use pipe src size when programming plane size. This makes cloned configs
> work correctly w/o the use of a panel fitter.
>
> v7: Rebase on top of Ville's rmw elimination series
>
> Signed-off-by: Damien Lespiau <damien.lesp...@intel.com> (v1,5,6,7)
> Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch> (v2,3)
> ---
> drivers/gpu/drm/i915/i915_reg.h | 110
> ++++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/intel_display.c | 88 +++++++++++++++++++++++++++-
> 2 files changed, 195 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 15c0eaa..087085c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -26,8 +26,8 @@
> #define _I915_REG_H_
>
> #define _PIPE(pipe, a, b) ((a) + (pipe)*((b)-(a)))
> +#define _PLANE(plane, a, b) _PIPE(plane, a, b)
> #define _TRANSCODER(tran, a, b) ((a) + (tran)*((b)-(a)))
> -
> #define _PORT(port, a, b) ((a) + (port)*((b)-(a)))
> #define _PIPE3(pipe, a, b, c) ((pipe) == PIPE_A ? (a) : \
> (pipe) == PIPE_B ? (b) : (c))
> @@ -4495,6 +4495,114 @@ enum punit_power_well {
> #define SPCONSTALPHA(pipe, plane) _PIPE(pipe * 2 + plane, _SPACONSTALPHA,
> _SPBCONSTALPHA)
> #define SPGAMC(pipe, plane) _PIPE(pipe * 2 + plane, _SPAGAMC, _SPBGAMC)
>
> +/* Skylake plane registers */
> +
> +#define _PLANE_CTL_1_A 0x70180
> +#define _PLANE_CTL_2_A 0x70280
> +#define _PLANE_CTL_3_A 0x70380
#first-bickesheding ;)
Since the definitions os 4_A, {1,2,3,4}_B and {1,2,3,4}_C are all in the
same spec with same bits below I'd prefer to define them here.
>
+#define PLANE_CTL_ENABLE (1 << 31)
> +#define PLANE_CTL_PIPE_GAMMA_ENABLE (1 << 30)
> +#define PLANE_CTL_FORMAT_MASK (0xf << 24)
> +#define PLANE_CTL_FORMAT_YUV422 ( 0 << 24)
> +#define PLANE_CTL_FORMAT_NV12 ( 1 << 24)
> +#define PLANE_CTL_FORMAT_XRGB_2101010 ( 2 << 24)
> +#define PLANE_CTL_FORMAT_XRGB_8888 ( 4 << 24)
> +#define PLANE_CTL_FORMAT_XRGB_16161616F ( 6 << 24)
> +#define PLANE_CTL_FORMAT_AYUV ( 8 << 24)
>
1010 missed or useless to add here?
> +#define PLANE_CTL_FORMAT_INDEXED ( 12 << 24)
> +#define PLANE_CTL_FORMAT_RGB_565 ( 14 << 24)
> +#define PLANE_CTL_PIPE_CSC_ENABLE (1 << 23)
> +#define PLANE_CTL_KEY_ENABLE (1 << 22)
>
Key is [22:21] and 01, i.e 22=0 also is a kind of Key enabled.
Or just the source matters?
In this case second bikesheding would be change for SOURCE_KEY_ENABLE
> +#define PLANE_CTL_ORDER_BGRX (0 << 20)
> +#define PLANE_CTL_ORDER_RGBX (1 << 20)
> +#define PLANE_CTL_YUV422_ORDER_MASK (0x3 << 16)
> +#define PLANE_CTL_YUV422_YUYV ( 0 << 16)
> +#define PLANE_CTL_YUV422_UYVY ( 1 << 16)
> +#define PLANE_CTL_YUV422_YVYU ( 2 << 16)
> +#define PLANE_CTL_YUV422_VYUY ( 3 << 16)
> +#define PLANE_CTL_DECOMPRESSION_ENABLE (1 << 15)
+#define PLANE_CTL_TRICKLE_FEED_DISABLE (1 << 14)
>
On Spec there is a restrictions: "Do not program this field to 1b." So I'd
prefer to declare FEED_ENABLE (0 << 14). Just to avoid have to always ~ it.
> +#define PLANE_CTL_PLANE_GAMMA_DISABLE (1 << 13)
> +#define PLANE_CTL_TILED_MASK (0x7 << 10)
> +#define PLANE_CTL_TILED_LINEAR ( 0 << 10)
> +#define PLANE_CTL_TILED_X ( 1 << 10)
> +#define PLANE_CTL_TILED_Y ( 4 << 10)
> +#define PLANE_CTL_TILED_YF ( 5 << 10)
> +#define PLANE_CTL_ALPHA_MASK (0x3 << 4)
> +#define PLANE_CTL_ALPHA_DISABLE ( 0 << 4)
> +#define PLANE_CTL_ALPHA_SW_PREMULTIPLY ( 2 << 4)
> +#define PLANE_CTL_ALPHA_HW_PREMULTIPLY ( 3 << 4)
> +#define _PLANE_STRIDE_1_A 0x70188
> +#define _PLANE_STRIDE_2_A 0x70288
> +#define _PLANE_STRIDE_3_A 0x70388
> +#define _PLANE_POS_1_A 0x7018c
> +#define _PLANE_POS_2_A 0x7028c
> +#define _PLANE_POS_3_A 0x7038c
> +#define _PLANE_SIZE_1_A 0x70190
> +#define _PLANE_SIZE_2_A 0x70290
> +#define _PLANE_SIZE_3_A 0x70390
> +#define _PLANE_SURF_1_A 0x7019c
> +#define _PLANE_SURF_2_A 0x7029c
> +#define _PLANE_SURF_3_A 0x7039c
> +#define _PLANE_OFFSET_1_A 0x701a4
> +#define _PLANE_OFFSET_2_A 0x702a4
> +#define _PLANE_OFFSET_3_A 0x703a4
> +
> +#define _PLANE_CTL_1_B 0x71180
> +#define _PLANE_CTL_2_B 0x71280
> +#define _PLANE_CTL_3_B 0x71380
> +#define _PLANE_CTL_1(pipe) _PIPE(pipe, _PLANE_CTL_1_A, _PLANE_CTL_1_B)
> +#define _PLANE_CTL_2(pipe) _PIPE(pipe, _PLANE_CTL_2_A, _PLANE_CTL_2_B)
> +#define _PLANE_CTL_3(pipe) _PIPE(pipe, _PLANE_CTL_3_A, _PLANE_CTL_3_B)
> +#define PLANE_CTL(pipe, plane) \
> + _PLANE(plane, _PLANE_CTL_1(pipe), _PLANE_CTL_2(pipe))
> +
> +#define _PLANE_STRIDE_1_B 0x71188
> +#define _PLANE_STRIDE_2_B 0x71288
> +#define _PLANE_STRIDE_3_B 0x71388
> +#define _PLANE_STRIDE_1(pipe) \
> + _PIPE(pipe, _PLANE_STRIDE_1_A, _PLANE_STRIDE_1_B)
> +#define _PLANE_STRIDE_2(pipe) \
> + _PIPE(pipe, _PLANE_STRIDE_2_A, _PLANE_STRIDE_2_B)
> +#define _PLANE_STRIDE_3(pipe) \
> + _PIPE(pipe, _PLANE_STRIDE_3_A, _PLANE_STRIDE_3_B)
> +#define PLANE_STRIDE(pipe, plane) \
> + _PLANE(plane, _PLANE_STRIDE_1(pipe), _PLANE_STRIDE_2(pipe))
> +
> +#define _PLANE_POS_1_B 0x7118c
> +#define _PLANE_POS_2_B 0x7128c
> +#define _PLANE_POS_3_B 0x7138c
> +#define _PLANE_POS_1(pipe) _PIPE(pipe, _PLANE_POS_1_A, _PLANE_POS_1_B)
> +#define _PLANE_POS_2(pipe) _PIPE(pipe, _PLANE_POS_2_A, _PLANE_POS_2_B)
> +#define _PLANE_POS_3(pipe) _PIPE(pipe, _PLANE_POS_3_A, _PLANE_POS_3_B)
> +#define PLANE_POS(pipe, plane) \
> + _PLANE(plane, _PLANE_POS_1(pipe), _PLANE_POS_2(pipe))
> +
> +#define _PLANE_SIZE_1_B 0x71190
> +#define _PLANE_SIZE_2_B 0x71290
> +#define _PLANE_SIZE_3_B 0x71390
> +#define _PLANE_SIZE_1(pipe) _PIPE(pipe, _PLANE_SIZE_1_A,
> _PLANE_SIZE_1_B)
> +#define _PLANE_SIZE_2(pipe) _PIPE(pipe, _PLANE_SIZE_2_A,
> _PLANE_SIZE_2_B)
> +#define _PLANE_SIZE_3(pipe) _PIPE(pipe, _PLANE_SIZE_3_A,
> _PLANE_SIZE_3_B)
> +#define PLANE_SIZE(pipe, plane) \
> + _PLANE(plane, _PLANE_SIZE_1(pipe), _PLANE_SIZE_2(pipe))
> +
> +#define _PLANE_SURF_1_B 0x7119c
> +#define _PLANE_SURF_2_B 0x7129c
> +#define _PLANE_SURF_3_B 0x7139c
> +#define _PLANE_SURF_1(pipe) _PIPE(pipe, _PLANE_SURF_1_A,
> _PLANE_SURF_1_B)
> +#define _PLANE_SURF_2(pipe) _PIPE(pipe, _PLANE_SURF_2_A,
> _PLANE_SURF_2_B)
> +#define _PLANE_SURF_3(pipe) _PIPE(pipe, _PLANE_SURF_3_A,
> _PLANE_SURF_3_B)
> +#define PLANE_SURF(pipe, plane) \
> + _PLANE(plane, _PLANE_SURF_1(pipe), _PLANE_SURF_2(pipe))
> +
> +#define _PLANE_OFFSET_1_B 0x711a4
> +#define _PLANE_OFFSET_2_B 0x712a4
> +#define _PLANE_OFFSET_1(pipe) _PIPE(pipe, _PLANE_OFFSET_1_A,
> _PLANE_OFFSET_1_B)
> +#define _PLANE_OFFSET_2(pipe) _PIPE(pipe, _PLANE_OFFSET_2_A,
> _PLANE_OFFSET_2_B)
> +#define PLANE_OFFSET(pipe, plane) \
> + _PLANE(plane, _PLANE_OFFSET_1(pipe), _PLANE_OFFSET_2(pipe))
> +
> /* VBIOS regs */
> #define VGACNTRL 0x71400
> # define VGA_DISP_DISABLE (1 << 31)
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 02236f9..f98d1c4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2640,6 +2640,86 @@ static void ironlake_update_primary_plane(struct
> drm_crtc *crtc,
> POSTING_READ(reg);
> }
>
> +static void skylake_update_primary_plane(struct drm_crtc *crtc,
> + struct drm_framebuffer *fb,
> + int x, int y)
> +{
> + 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_framebuffer *intel_fb;
> + struct drm_i915_gem_object *obj;
> + int pipe = intel_crtc->pipe;
> + u32 plane_ctl, stride;
> +
> + if (!intel_crtc->primary_enabled) {
> + I915_WRITE(PLANE_CTL(pipe, 0), 0);
> + I915_WRITE(PLANE_SURF(pipe, 0), 0);
> + POSTING_READ(PLANE_CTL(pipe, 0));
> + return;
> + }
> +
> + plane_ctl = PLANE_CTL_ENABLE |
> + PLANE_CTL_PIPE_GAMMA_ENABLE |
> + PLANE_CTL_PIPE_CSC_ENABLE;
> +
> + switch (fb->pixel_format) {
> + case DRM_FORMAT_RGB565:
> + plane_ctl |= PLANE_CTL_FORMAT_RGB_565;
> + break;
> + case DRM_FORMAT_XRGB8888:
> + plane_ctl |= PLANE_CTL_FORMAT_XRGB_8888;
> + break;
> + case DRM_FORMAT_XBGR8888:
> + plane_ctl |= PLANE_CTL_ORDER_RGBX;
> + plane_ctl |= PLANE_CTL_FORMAT_XRGB_8888;
> + break;
> + case DRM_FORMAT_XRGB2101010:
> + plane_ctl |= PLANE_CTL_FORMAT_XRGB_2101010;
> + break;
> + case DRM_FORMAT_XBGR2101010:
> + plane_ctl |= PLANE_CTL_ORDER_RGBX;
> + plane_ctl |= PLANE_CTL_FORMAT_XRGB_2101010;
> + break;
> + default:
> + BUG();
> + }
> +
> + intel_fb = to_intel_framebuffer(fb);
> + obj = intel_fb->obj;
> + switch (obj->tiling_mode) {
> + case I915_TILING_NONE:
> + stride = fb->pitches[0] >> 6;
>
A comment here would be good to explain this:
"For Linear memory this field specifies the stride in chunks of 64 bytes"
> + break;
> + case I915_TILING_X:
> + plane_ctl |= PLANE_CTL_TILED_X;
> + stride = fb->pitches[0] >> 9;
and something like or better than the following here:
"Fox X-tiled this field specifies the stride in number of tiles" 1/512
bytes.
> + break;
> + default:
>
is I915_TILING_Y impossible?
+ BUG();
> + }
> +
> + plane_ctl &= ~PLANE_CTL_TRICKLE_FEED_DISABLE;
> + plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> +
> + I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> +
> + DRM_DEBUG_KMS("Writing base %08lX %d,%d,%d,%d pitch=%d\n",
> + i915_gem_obj_ggtt_offset(obj),
> + x, y, fb->width, fb->height,
> + fb->pitches[0]);
> +
> + I915_WRITE(PLANE_POS(pipe, 0), 0);
> + I915_WRITE(PLANE_OFFSET(pipe, 0), (y << 16) | x);
> + I915_WRITE(PLANE_SIZE(pipe, 0),
> + (intel_crtc->config.pipe_src_h - 1) << 16 |
> + (intel_crtc->config.pipe_src_w - 1));
> + I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> + I915_WRITE(PLANE_SURF(pipe, 0), i915_gem_obj_ggtt_offset(obj));
> +
> + POSTING_READ(PLANE_SURF(pipe, 0));
> +}
> +
> /* Assume fb object is pinned & idle & fenced and just update base
> pointers */
> static int
> intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer
> *fb,
> @@ -12481,8 +12561,12 @@ static void intel_init_display(struct drm_device
> *dev)
> dev_priv->display.crtc_enable = haswell_crtc_enable;
> dev_priv->display.crtc_disable = haswell_crtc_disable;
> dev_priv->display.off = ironlake_crtc_off;
> - dev_priv->display.update_primary_plane =
> - ironlake_update_primary_plane;
> + if (INTEL_INFO(dev)->gen >= 9)
> + dev_priv->display.update_primary_plane =
> + skylake_update_primary_plane;
> + else
> + dev_priv->display.update_primary_plane =
> + ironlake_update_primary_plane;
> } else if (HAS_PCH_SPLIT(dev)) {
> dev_priv->display.get_pipe_config =
> ironlake_get_pipe_config;
> dev_priv->display.get_plane_config =
> ironlake_get_plane_config;
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Regardless the bikeshedings, and if all answers to the questions above
result in no change to the code, consider this
Reviewed-by: Rodrigo Vivi <rodrigo.v...@intel.com>
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
