Nouveau - May 2017 - [RFC PATCH 0/3] drm/nouveau/dispnv04 overlay and primary fb format fixes

This came out of some debugging I was doing to figure out how BE mode works
on this hardware. Among other things, it came out that we're not exposing
16-bpp mode support and that the ARGB8888 mode that we do expose is broken.
Also the overlay logic was pretty broken, I must have only tested with very
"normal" overlay buffer sizes with modetest before.

That said, this code has only received literal testing on a NV34/G5 PPC combo.
I was poking at various registers on a NV34/x86 to make modetest display the
correct data though. That's where e.g. the pitch mask comes from.

I haven't at all tested on my NV05 or NV1x hardware. Should probably do that
before we push this out. But since I've already been sitting on these
patches
for a few weeks, thought I'd get them out there.

Ilia Mirkin (3):
  drm/nouveau/overlay: improve error detection, fix pitch setting
  drm/nouveau/overlay: add NV21 support
  drm/nouveau/dispnv04: fix exposed format list

 drivers/gpu/drm/nouveau/dispnv04/crtc.c    | 36 +++++++++++-
 drivers/gpu/drm/nouveau/dispnv04/overlay.c | 89 +++++++++++++++++++-----------
 2 files changed, 93 insertions(+), 32 deletions(-)

-- 
2.13.0

We were previously setting the pitch based on a perfectly packed buffer.
This does not necessarily happen. Either modetest started generating
such buffers recently, or earlier testing only happened with well-picked
overlay sizes.

While we're at it, beef up and refactor the error state detection.

Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
---
 drivers/gpu/drm/nouveau/dispnv04/overlay.c | 80 +++++++++++++++++++-----------
 1 file changed, 52 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c
b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
index e54944d23268..ee7df9ef2695 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
@@ -90,6 +90,42 @@ cos_mul(int degrees, int factor)
 }
 
 static int
+verify_fb(const struct drm_framebuffer *fb, uint32_t src_w, uint32_t src_h,
+	  uint32_t crtc_w, uint32_t crtc_h, bool scale_fail, bool offset_fail)
+{
+	if (fb->pitches[0] & 0x3f) {
+		DRM_DEBUG_KMS("Unsuitable framebuffer for plane: align 64: 0x%x\n",
+			      fb->pitches[0]);
+		return -EINVAL;
+	}
+
+	if (fb->pitches[0] >= 0x10000) {
+		DRM_DEBUG_KMS("Unsuitable framebuffer for plane: pitch 0x%x >=
0x10000\n",
+			      fb->pitches[0]);
+		return -EINVAL;
+	}
+
+	if (fb->pitches[1] && fb->pitches[0] != fb->pitches[1]) {
+		DRM_DEBUG_KMS("Unsuitable framebuffer for plane: diff pitches: 0x%x !=
0x%x\n",
+			      fb->pitches[0], fb->pitches[1]);
+		return -ERANGE;
+	}
+
+	if (scale_fail) {
+		DRM_DEBUG_KMS("Unsuitable framebuffer scaling: %dx%d ->
%dx%d\n",
+			      src_w, src_h, crtc_w, crtc_h);
+		return -ERANGE;
+	}
+
+	if (offset_fail) {
+		DRM_DEBUG_KMS("Unsuitable framebuffer offset\n");
+		return -ERANGE;
+	}
+
+	return 0;
+}
+
+static int
 nv10_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		  struct drm_framebuffer *fb, int crtc_x, int crtc_y,
 		  unsigned int crtc_w, unsigned int crtc_h,
@@ -107,7 +143,9 @@ nv10_update_plane(struct drm_plane *plane, struct drm_crtc
*crtc,
 	bool flip = nv_plane->flip;
 	int soff = NV_PCRTC0_SIZE * nv_crtc->index;
 	int soff2 = NV_PCRTC0_SIZE * !nv_crtc->index;
-	int format, ret;
+	unsigned shift = drm->client.device.info.chipset >= 0x30 ? 1 : 3;
+	unsigned format = 0;
+	int ret;
 
 	/* Source parameters given in 16.16 fixed point, ignore fractional. */
 	src_x >>= 16;
@@ -115,18 +153,11 @@ nv10_update_plane(struct drm_plane *plane, struct drm_crtc
*crtc,
 	src_w >>= 16;
 	src_h >>= 16;
 
-	format = ALIGN(src_w * 4, 0x100);
-
-	if (format > 0xffff)
-		return -ERANGE;
-
-	if (drm->client.device.info.chipset >= 0x30) {
-		if (crtc_w < (src_w >> 1) || crtc_h < (src_h >> 1))
-			return -ERANGE;
-	} else {
-		if (crtc_w < (src_w >> 3) || crtc_h < (src_h >> 3))
-			return -ERANGE;
-	}
+	ret = verify_fb(fb, src_w, src_h, crtc_w, crtc_h,
+			crtc_w < (src_w >> shift) || crtc_h < (src_h >> shift),
+			false);
+	if (ret)
+		return ret;
 
 	ret = nouveau_bo_pin(nv_fb->nvbo, TTM_PL_FLAG_VRAM, false);
 	if (ret)
@@ -160,7 +191,7 @@ nv10_update_plane(struct drm_plane *plane, struct drm_crtc
*crtc,
 		nvif_wr32(dev, NV_PVIDEO_UVPLANE_OFFSET_BUFF(flip),
 			nv_fb->nvbo->bo.offset + fb->offsets[1]);
 	}
-	nvif_wr32(dev, NV_PVIDEO_FORMAT(flip), format);
+	nvif_wr32(dev, NV_PVIDEO_FORMAT(flip), format | fb->pitches[0]);
 	nvif_wr32(dev, NV_PVIDEO_STOP, 0);
 	/* TODO: wait for vblank? */
 	nvif_wr32(dev, NV_PVIDEO_BUFFER, flip ? 0x10 : 0x1);
@@ -357,7 +388,7 @@ nv04_update_plane(struct drm_plane *plane, struct drm_crtc
*crtc,
 	struct nouveau_bo *cur = nv_plane->cur;
 	uint32_t overlay = 1;
 	int brightness = (nv_plane->brightness - 512) * 62 / 512;
-	int pitch, ret, i;
+	int ret, i;
 
 	/* Source parameters given in 16.16 fixed point, ignore fractional. */
 	src_x >>= 16;
@@ -365,17 +396,9 @@ nv04_update_plane(struct drm_plane *plane, struct drm_crtc
*crtc,
 	src_w >>= 16;
 	src_h >>= 16;
 
-	pitch = ALIGN(src_w * 4, 0x100);
-
-	if (pitch > 0xffff)
-		return -ERANGE;
-
-	/* TODO: Compute an offset? Not sure how to do this for YUYV. */
-	if (src_x != 0 || src_y != 0)
-		return -ERANGE;
-
-	if (crtc_w < src_w || crtc_h < src_h)
-		return -ERANGE;
+	ret = verify_fb(fb, src_w, src_h, crtc_w, crtc_h,
+			crtc_w < src_w || crtc_h < src_h,
+			src_x != 0 || src_y != 0);
 
 	ret = nouveau_bo_pin(nv_fb->nvbo, TTM_PL_FLAG_VRAM, false);
 	if (ret)
@@ -389,8 +412,9 @@ nv04_update_plane(struct drm_plane *plane, struct drm_crtc
*crtc,
 
 	for (i = 0; i < 2; i++) {
 		nvif_wr32(dev, NV_PVIDEO_BUFF0_START_ADDRESS + 4 * i,
-			nv_fb->nvbo->bo.offset);
-		nvif_wr32(dev, NV_PVIDEO_BUFF0_PITCH_LENGTH + 4 * i, pitch);
+			  nv_fb->nvbo->bo.offset);
+		nvif_wr32(dev, NV_PVIDEO_BUFF0_PITCH_LENGTH + 4 * i,
+			  fb->pitches[0]);
 		nvif_wr32(dev, NV_PVIDEO_BUFF0_OFFSET + 4 * i, 0);
 	}
 	nvif_wr32(dev, NV_PVIDEO_WINDOW_START, crtc_y << 16 | crtc_x);
-- 
2.13.0

Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
---
 drivers/gpu/drm/nouveau/dispnv04/overlay.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv04/overlay.c
b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
index ee7df9ef2695..96354a5ff21c 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/overlay.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/overlay.c
@@ -63,6 +63,7 @@ static uint32_t formats[] = {
 	DRM_FORMAT_YUYV,
 	DRM_FORMAT_UYVY,
 	DRM_FORMAT_NV12,
+	DRM_FORMAT_NV21,
 };
 
 /* Sine can be approximated with
@@ -177,16 +178,18 @@ nv10_update_plane(struct drm_plane *plane, struct drm_crtc
*crtc,
 	nvif_wr32(dev, NV_PVIDEO_POINT_OUT(flip), crtc_y << 16 | crtc_x);
 	nvif_wr32(dev, NV_PVIDEO_SIZE_OUT(flip), crtc_h << 16 | crtc_w);
 
-	if (fb->format->format != DRM_FORMAT_UYVY)
+	if (fb->format->format == DRM_FORMAT_YUYV ||
+	    fb->format->format == DRM_FORMAT_NV12)
 		format |= NV_PVIDEO_FORMAT_COLOR_LE_CR8YB8CB8YA8;
-	if (fb->format->format == DRM_FORMAT_NV12)
+	if (fb->format->format == DRM_FORMAT_NV12 ||
+	    fb->format->format == DRM_FORMAT_NV21)
 		format |= NV_PVIDEO_FORMAT_PLANAR;
 	if (nv_plane->iturbt_709)
 		format |= NV_PVIDEO_FORMAT_MATRIX_ITURBT709;
 	if (nv_plane->colorkey & (1 << 24))
 		format |= NV_PVIDEO_FORMAT_DISPLAY_COLOR_KEY;
 
-	if (fb->format->format == DRM_FORMAT_NV12) {
+	if (format & NV_PVIDEO_FORMAT_PLANAR) {
 		nvif_wr32(dev, NV_PVIDEO_UVPLANE_BASE(flip), 0);
 		nvif_wr32(dev, NV_PVIDEO_UVPLANE_OFFSET_BUFF(flip),
 			nv_fb->nvbo->bo.offset + fb->offsets[1]);
-- 
2.13.0

drm_crtc_init exposes the XRGB8888 and ARGB8888 formats. In actuality,
ARGB8888's 32-bit depth messes up some formulas that weren't meant for
it, and the alpha is failry meaningless for the primary plane.

The modesetting logic appears to be fully prepared for RGB565 as well as
XRGB1555 however, as tested with modetest.

Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
---
 drivers/gpu/drm/nouveau/dispnv04/crtc.c | 36 ++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
index 4b4b0b496262..c078811b4e11 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
@@ -1099,6 +1099,38 @@ static const struct drm_crtc_helper_funcs
nv04_crtc_helper_funcs = {
 	.disable = nv_crtc_disable,
 };
 
+static const uint32_t modeset_formats[] = {
+        DRM_FORMAT_XRGB8888,
+        DRM_FORMAT_RGB565,
+        DRM_FORMAT_XRGB1555,
+};
+
+static struct drm_plane *
+create_primary_plane(struct drm_device *dev)
+{
+        struct drm_plane *primary;
+        int ret;
+
+        primary = kzalloc(sizeof(*primary), GFP_KERNEL);
+        if (primary == NULL) {
+                DRM_DEBUG_KMS("Failed to allocate primary plane\n");
+                return NULL;
+        }
+
+        /* possible_crtc's will be filled in later by crtc_init */
+        ret = drm_universal_plane_init(dev, primary, 0,
+                                       &drm_primary_helper_funcs,
+                                       modeset_formats,
+                                       ARRAY_SIZE(modeset_formats),
+                                       DRM_PLANE_TYPE_PRIMARY, NULL);
+        if (ret) {
+                kfree(primary);
+                primary = NULL;
+        }
+
+        return primary;
+}
+
 int
 nv04_crtc_create(struct drm_device *dev, int crtc_num)
 {
@@ -1122,7 +1154,9 @@ nv04_crtc_create(struct drm_device *dev, int crtc_num)
 	nv_crtc->save = nv_crtc_save;
 	nv_crtc->restore = nv_crtc_restore;
 
-	drm_crtc_init(dev, &nv_crtc->base, &nv04_crtc_funcs);
+	drm_crtc_init_with_planes(dev, &nv_crtc->base,
+                                  create_primary_plane(dev), NULL,
+                                  &nv04_crtc_funcs, NULL);
 	drm_crtc_helper_add(&nv_crtc->base, &nv04_crtc_helper_funcs);
 	drm_mode_crtc_set_gamma_size(&nv_crtc->base, 256);
 
-- 
2.13.0

On 05/20/2017 12:57 PM, Ilia Mirkin wrote:
> This came out of some debugging I was doing to figure out how BE mode works
> on this hardware. Among other things, it came out that we're not
exposing
> 16-bpp mode support and that the ARGB8888 mode that we do expose is broken.
> Also the overlay logic was pretty broken, I must have only tested with very
> "normal" overlay buffer sizes with modetest before.
> 
> That said, this code has only received literal testing on a NV34/G5 PPC
combo.
> I was poking at various registers on a NV34/x86 to make modetest display
the
> correct data though. That's where e.g. the pitch mask comes from.
> 
> I haven't at all tested on my NV05 or NV1x hardware. Should probably do
that
> before we push this out. But since I've already been sitting on these
patches
> for a few weeks, thought I'd get them out there.
The patches look fine to me, but I can wait to merge them until you've 
tested the older HW if you prefer.

Ben.
> 
> Ilia Mirkin (3):
>    drm/nouveau/overlay: improve error detection, fix pitch setting
>    drm/nouveau/overlay: add NV21 support
>    drm/nouveau/dispnv04: fix exposed format list
> 
>   drivers/gpu/drm/nouveau/dispnv04/crtc.c    | 36 +++++++++++-
>   drivers/gpu/drm/nouveau/dispnv04/overlay.c | 89
+++++++++++++++++++-----------
>   2 files changed, 93 insertions(+), 32 deletions(-)
>