Re: [PATCH 12/89] drm/i915/skl: Implement thew new update_plane() for primary planes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On Thu, Sep 4, 2014 at 4:26 AM, Damien Lespiau <damien.lespiau@xxxxxxxxx> 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.lespiau@xxxxxxxxx> (v1,5,6,7)
Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx> (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@xxxxxxxxxxxxxxxxxxxxx
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.vivi@xxxxxxxxx>


--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux