[PATCH v2 0/9] drm/exynos: misc fixes and more

[PATCH v2 0/9] drm/exynos: misc fixes and more

[Tobias Jakobi]

Hello,

this is the second iteration of [1], with the following changes:
- split patch 3/8 (suggested by Inki)
- zero init registers in patch 4/8 (suggested by Inki)
- rephrased commit description of patch 5/8 to make it more
  clear that this behaviour depends on the hw
- replace zero with linear modifier in patch 2/8


Summary:
(a) Enables support for NV12MT in the mixer.
(b) Sanitizes buffer pitch for HW with restrictions.
(c) Misc fixes

With best wishes,
Tobias

[1] https://www.spinics.net/lists/linux-samsung-soc/msg60235.html


Tobias Jakobi (9):
  drm/exynos: mixer: fix chroma comment in vp_video_buffer()
  drm/exynos: mixer: enable NV12MT support for the video plane
  drm/exynos: mixer: simplify vp_video_buffer()
  drm/exynos: mixer: simplify mixer_graph_buffer()
  drm/exynos: mixer: remove src offset from mixer_graph_buffer()
  drm/exynos: introduce BYTE_PITCH capability
  drm/exynos: add BYTE_PITCH cap for all supported planes
  drm/exynos: consistent use of cpp
  drm/exynos: simplify set_pixfmt() in DECON and FIMD drivers

 drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 17 +++++-----
 drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 13 +++-----
 drivers/gpu/drm/exynos/exynos_drm_drv.h       |  2 ++
 drivers/gpu/drm/exynos/exynos_drm_fb.c        |  2 ++
 drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 17 ++++------
 drivers/gpu/drm/exynos/exynos_drm_plane.c     | 37 +++++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_mixer.c         | 48 +++++++++------------------
 7 files changed, 75 insertions(+), 61 deletions(-)

-- 
2.13.0

[PATCH v2 1/9] drm/exynos: mixer: fix chroma comment in vp_video_buffer()

[Tobias Jakobi]

The current comment sounds like the division op is done to
compensate for some hardware erratum. But the chroma plane
having half the height of the luma plane is just the way
NV12/NV21 is defined, so clarify this behaviour.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index a998a8dd783c..c6d6f6d42900 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -532,7 +532,7 @@ static void vp_video_buffer(struct mixer_context *ctx,
 	/* setting size of input image */
 	vp_reg_write(res, VP_IMG_SIZE_Y, VP_IMG_HSIZE(fb->pitches[0]) |
 		VP_IMG_VSIZE(fb->height));
-	/* chroma height has to reduced by 2 to avoid chroma distorions */
+	/* the chroma plane for NV12/NV21 is half the height of the luma plane */
 	vp_reg_write(res, VP_IMG_SIZE_C, VP_IMG_HSIZE(fb->pitches[0]) |
 		VP_IMG_VSIZE(fb->height / 2));
 
-- 
2.13.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/9] drm/exynos: mixer: fix chroma comment in vp_video_buffer()

[Inki Dae]

2017년 08월 22일 23:19에 Tobias Jakobi 이(가) 쓴 글:
> The current comment sounds like the division op is done to
> compensate for some hardware erratum. But the chroma plane
> having half the height of the luma plane is just the way
> NV12/NV21 is defined, so clarify this behaviour.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index a998a8dd783c..c6d6f6d42900 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -532,7 +532,7 @@ static void vp_video_buffer(struct mixer_context *ctx,
>  	/* setting size of input image */
>  	vp_reg_write(res, VP_IMG_SIZE_Y, VP_IMG_HSIZE(fb->pitches[0]) |
>  		VP_IMG_VSIZE(fb->height));
> -	/* chroma height has to reduced by 2 to avoid chroma distorions */
> +	/* the chroma plane for NV12/NV21 is half the height of the luma plane */

WARNING: line over 80 characters
#86: FILE: drivers/gpu/drm/exynos/exynos_mixer.c:535:
+	/* the chroma plane for NV12/NV21 is half the height of the luma plane */

I just removed a word, 'the', in front of 'chroma' word.

Thanks,
Inki Dae


>  	vp_reg_write(res, VP_IMG_SIZE_C, VP_IMG_HSIZE(fb->pitches[0]) |
>  		VP_IMG_VSIZE(fb->height / 2));
>  
> 

[PATCH v2 2/9] drm/exynos: mixer: enable NV12MT support for the video plane

[Tobias Jakobi]

The video processor supports a tiled version of the NV12 format,
known as NV12MT in V4L2 terms. The support was removed in commit
083500baefd5f4c215a5a93aef2492c1aa775828 due to not being a real
pixel format, but rather NV12 with a special memory layout.

With the introduction of FB modifiers, we can now properly support
this format again.

Tested with a hacked up modetest from libdrm's test suite on
an ODROID-X2 (Exynos4412).

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.h   |  1 +
 drivers/gpu/drm/exynos/exynos_drm_fb.c    |  2 ++
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 27 +++++++++++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_mixer.c     |  6 +++++-
 4 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index a93de321706b..43afab4bebc3 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -91,6 +91,7 @@ struct exynos_drm_plane {
 #define EXYNOS_DRM_PLANE_CAP_DOUBLE	(1 << 0)
 #define EXYNOS_DRM_PLANE_CAP_SCALE	(1 << 1)
 #define EXYNOS_DRM_PLANE_CAP_ZPOS	(1 << 2)
+#define EXYNOS_DRM_PLANE_CAP_TILE	(1 << 3)
 
 /*
  * Exynos DRM plane configuration structure.
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index 73217c281c9a..a958818d552b 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -250,4 +250,6 @@ void exynos_drm_mode_config_init(struct drm_device *dev)
 
 	dev->mode_config.funcs = &exynos_drm_mode_config_funcs;
 	dev->mode_config.helper_private = &exynos_drm_mode_config_helpers;
+
+	dev->mode_config.allow_fb_modifiers = true;
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index 611b6fd65433..133af72f5c90 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -180,6 +180,29 @@ static struct drm_plane_funcs exynos_plane_funcs = {
 };
 
 static int
+exynos_drm_plane_check_format(const struct exynos_drm_plane_config *config,
+			      struct exynos_drm_plane_state *state)
+{
+	struct drm_framebuffer *fb = state->base.fb;
+
+	switch (fb->modifier) {
+	case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
+		if (!(config->capabilities & EXYNOS_DRM_PLANE_CAP_TILE))
+			return -ENOTSUPP;
+		break;
+
+	case DRM_FORMAT_MOD_LINEAR:
+		break;
+
+	default:
+		DRM_ERROR("unsupported pixel format modifier");
+		return -ENOTSUPP;
+	}
+
+	return 0;
+}
+
+static int
 exynos_drm_plane_check_size(const struct exynos_drm_plane_config *config,
 			    struct exynos_drm_plane_state *state)
 {
@@ -223,6 +246,10 @@ static int exynos_plane_atomic_check(struct drm_plane *plane,
 	/* translate state into exynos_state */
 	exynos_plane_mode_set(exynos_state);
 
+	ret = exynos_drm_plane_check_format(exynos_plane->config, exynos_state);
+	if (ret)
+		return ret;
+
 	ret = exynos_drm_plane_check_size(exynos_plane->config, exynos_state);
 	return ret;
 }
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index c6d6f6d42900..4c894d97aba3 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -148,7 +148,8 @@ static const struct exynos_drm_plane_config plane_configs[MIXER_WIN_NR] = {
 		.pixel_formats = vp_formats,
 		.num_pixel_formats = ARRAY_SIZE(vp_formats),
 		.capabilities = EXYNOS_DRM_PLANE_CAP_SCALE |
-				EXYNOS_DRM_PLANE_CAP_ZPOS,
+				EXYNOS_DRM_PLANE_CAP_ZPOS |
+				EXYNOS_DRM_PLANE_CAP_TILE,
 	},
 };
 
@@ -500,6 +501,9 @@ static void vp_video_buffer(struct mixer_context *ctx,
 		return;
 	}
 
+	if (fb->modifier == DRM_FORMAT_MOD_SAMSUNG_64_32_TILE)
+		tiled_mode = true;
+
 	luma_addr[0] = exynos_drm_fb_dma_addr(fb, 0);
 	chroma_addr[0] = exynos_drm_fb_dma_addr(fb, 1);
 
-- 
2.13.0

[PATCH v2 3/9] drm/exynos: mixer: simplify vp_video_buffer()

[Tobias Jakobi]

DRM core already checks in drm_atomic_plane_check() if the
pixelformat is valid. Hence we can drop the default case of
the switch statement and collapse most of the code.

Also rename the two booleans to reflect what true/false
actually means, and to avoid mixing CrCb/NV21 descriptions.

No functional change.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 4c894d97aba3..beef4d6c41ca 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -484,32 +484,18 @@ static void vp_video_buffer(struct mixer_context *ctx,
 	unsigned int priority = state->base.normalized_zpos + 1;
 	unsigned long flags;
 	dma_addr_t luma_addr[2], chroma_addr[2];
-	bool tiled_mode = false;
-	bool crcb_mode = false;
+	bool is_tiled, is_nv21;
 	u32 val;
 
-	switch (fb->format->format) {
-	case DRM_FORMAT_NV12:
-		crcb_mode = false;
-		break;
-	case DRM_FORMAT_NV21:
-		crcb_mode = true;
-		break;
-	default:
-		DRM_ERROR("pixel format for vp is wrong [%d].\n",
-				fb->format->format);
-		return;
-	}
-
-	if (fb->modifier == DRM_FORMAT_MOD_SAMSUNG_64_32_TILE)
-		tiled_mode = true;
+	is_nv21 = (fb->format->format == DRM_FORMAT_NV21);
+	is_tiled = (fb->modifier == DRM_FORMAT_MOD_SAMSUNG_64_32_TILE);
 
 	luma_addr[0] = exynos_drm_fb_dma_addr(fb, 0);
 	chroma_addr[0] = exynos_drm_fb_dma_addr(fb, 1);
 
 	if (mode->flags & DRM_MODE_FLAG_INTERLACE) {
 		__set_bit(MXR_BIT_INTERLACE, &ctx->flags);
-		if (tiled_mode) {
+		if (is_tiled) {
 			luma_addr[1] = luma_addr[0] + 0x40;
 			chroma_addr[1] = chroma_addr[0] + 0x40;
 		} else {
@@ -529,8 +515,8 @@ static void vp_video_buffer(struct mixer_context *ctx,
 	vp_reg_writemask(res, VP_MODE, val, VP_MODE_LINE_SKIP);
 
 	/* setup format */
-	val = (crcb_mode ? VP_MODE_NV21 : VP_MODE_NV12);
-	val |= (tiled_mode ? VP_MODE_MEM_TILED : VP_MODE_MEM_LINEAR);
+	val = (is_nv21 ? VP_MODE_NV21 : VP_MODE_NV12);
+	val |= (is_tiled ? VP_MODE_MEM_TILED : VP_MODE_MEM_LINEAR);
 	vp_reg_writemask(res, VP_MODE, val, VP_MODE_FMT_MASK);
 
 	/* setting size of input image */
-- 
2.13.0

[PATCH v2 4/9] drm/exynos: mixer: simplify mixer_graph_buffer()

[Tobias Jakobi]

DRM core already checks in drm_atomic_plane_check() if the
pixelformat is valid. Hence we can collapse the default case
of the switch statement with the XRGB8888 case.

No functional change.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index beef4d6c41ca..8d68de85bada 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -606,12 +606,9 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 
 	case DRM_FORMAT_XRGB8888:
 	case DRM_FORMAT_ARGB8888:
+	default:
 		fmt = MXR_FORMAT_ARGB8888;
 		break;
-
-	default:
-		DRM_DEBUG_KMS("pixelformat unsupported by mixer\n");
-		return;
 	}
 
 	/* ratio is already checked by common plane code */
-- 
2.13.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/9] drm/exynos: mixer: remove src offset from mixer_graph_buffer()

[Tobias Jakobi]

We always translate the dma address such that the offsets of
the source image are zero. Hence we can remove manipulation of
the MXR_GRAPHIC_SXY(win) register and just zero them once
in mixer_win_reset().

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 8d68de85bada..a540e50ceadc 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -584,7 +584,7 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 	unsigned long flags;
 	unsigned int win = plane->index;
 	unsigned int x_ratio = 0, y_ratio = 0;
-	unsigned int src_x_offset, src_y_offset, dst_x_offset, dst_y_offset;
+	unsigned int dst_x_offset, dst_y_offset;
 	dma_addr_t dma_addr;
 	unsigned int fmt;
 	u32 val;
@@ -618,12 +618,10 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 	dst_x_offset = state->crtc.x;
 	dst_y_offset = state->crtc.y;
 
-	/* converting dma address base and source offset */
+	/* translate dma address base s.t. the source image offset is zero */
 	dma_addr = exynos_drm_fb_dma_addr(fb, 0)
 		+ (state->src.x * fb->format->cpp[0])
 		+ (state->src.y * fb->pitches[0]);
-	src_x_offset = 0;
-	src_y_offset = 0;
 
 	if (mode->flags & DRM_MODE_FLAG_INTERLACE)
 		__set_bit(MXR_BIT_INTERLACE, &ctx->flags);
@@ -654,11 +652,6 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 	val |= MXR_GRP_WH_V_SCALE(y_ratio);
 	mixer_reg_write(res, MXR_GRAPHIC_WH(win), val);
 
-	/* setup offsets in source image */
-	val  = MXR_GRP_SXY_SX(src_x_offset);
-	val |= MXR_GRP_SXY_SY(src_y_offset);
-	mixer_reg_write(res, MXR_GRAPHIC_SXY(win), val);
-
 	/* setup offsets in display image */
 	val  = MXR_GRP_DXY_DX(dst_x_offset);
 	val |= MXR_GRP_DXY_DY(dst_y_offset);
@@ -735,6 +728,10 @@ static void mixer_win_reset(struct mixer_context *ctx)
 	if (test_bit(MXR_BIT_VP_ENABLED, &ctx->flags))
 		mixer_reg_writemask(res, MXR_CFG, 0, MXR_CFG_VP_ENABLE);
 
+	/* set all source image offsets to zero */
+	mixer_reg_write(res, MXR_GRAPHIC_SXY(0), 0);
+	mixer_reg_write(res, MXR_GRAPHIC_SXY(1), 0);
+
 	spin_unlock_irqrestore(&res->reg_slock, flags);
 }
 
-- 
2.13.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 6/9] drm/exynos: introduce BYTE_PITCH capability

[Tobias Jakobi]

In some of subdrivers we compute something like 'pitch / cpp' at some
point, silently assuming that the pitch (which is in bytes) is
divisible by the buffer's cpp. This must not be true, in particular
it depends on the underlying hardware.

If userspace should request such a setup, we should communicate this.

Introduce a new cap which indicates that the hardware supports a
pitch with 'byte-granularity'. If the cap is not set, assume that
we need a pitch aligned to cpp.

We set this cap in a later patch for the drivers/planes which
support it.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.h   |  1 +
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 10 ++++++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 43afab4bebc3..ec32632485d2 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -92,6 +92,7 @@ struct exynos_drm_plane {
 #define EXYNOS_DRM_PLANE_CAP_SCALE	(1 << 1)
 #define EXYNOS_DRM_PLANE_CAP_ZPOS	(1 << 2)
 #define EXYNOS_DRM_PLANE_CAP_TILE	(1 << 3)
+#define EXYNOS_DRM_PLANE_CAP_BYTE_PITCH	(1 << 4)
 
 /*
  * Exynos DRM plane configuration structure.
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index 133af72f5c90..17e47b8f0d6a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -185,6 +185,16 @@ exynos_drm_plane_check_format(const struct exynos_drm_plane_config *config,
 {
 	struct drm_framebuffer *fb = state->base.fb;
 
+	/*
+	 * Some blocks only allow to specify a buffer pitch in terms
+	 * of pixels. In these cases, we need to ensure that the pitch
+	 * provided by userspace is divisible by the cpp.
+	 */
+	if (!(config->capabilities & EXYNOS_DRM_PLANE_CAP_BYTE_PITCH)) {
+		if (fb->pitches[0] % fb->format->cpp[0])
+			return -ENOTSUPP;
+	}
+
 	switch (fb->modifier) {
 	case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
 		if (!(config->capabilities & EXYNOS_DRM_PLANE_CAP_TILE))
-- 
2.13.0

Re: [PATCH v2 6/9] drm/exynos: introduce BYTE_PITCH capability

[Marek Szyprowski]

Hi Tobias,

On 2017-08-22 16:19, Tobias Jakobi wrote:
> In some of subdrivers we compute something like 'pitch / cpp' at some
> point, silently assuming that the pitch (which is in bytes) is
> divisible by the buffer's cpp. This must not be true, in particular
> it depends on the underlying hardware.
>
> If userspace should request such a setup, we should communicate this.
>
> Introduce a new cap which indicates that the hardware supports a
> pitch with 'byte-granularity'. If the cap is not set, assume that
> we need a pitch aligned to cpp.
>
> We set this cap in a later patch for the drivers/planes which
> support it.
>
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>

I briefly checked the patch and it looks fine, but I really wonder whether
drivers should support such strange formats, when pitches are not multiple
of pixel size in bytes. I cannot find any sane use case for such formats.

Maybe it would make sense to add a check for it in DRM core? I also didn't
notice a check for that in any other drivers, but some of them also
compute 'pitch / cpp' values.

> ---
>   drivers/gpu/drm/exynos/exynos_drm_drv.h   |  1 +
>   drivers/gpu/drm/exynos/exynos_drm_plane.c | 10 ++++++++++
>   2 files changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 43afab4bebc3..ec32632485d2 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -92,6 +92,7 @@ struct exynos_drm_plane {
>   #define EXYNOS_DRM_PLANE_CAP_SCALE	(1 << 1)
>   #define EXYNOS_DRM_PLANE_CAP_ZPOS	(1 << 2)
>   #define EXYNOS_DRM_PLANE_CAP_TILE	(1 << 3)
> +#define EXYNOS_DRM_PLANE_CAP_BYTE_PITCH	(1 << 4)
>   
>   /*
>    * Exynos DRM plane configuration structure.
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> index 133af72f5c90..17e47b8f0d6a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> @@ -185,6 +185,16 @@ exynos_drm_plane_check_format(const struct exynos_drm_plane_config *config,
>   {
>   	struct drm_framebuffer *fb = state->base.fb;
>   
> +	/*
> +	 * Some blocks only allow to specify a buffer pitch in terms
> +	 * of pixels. In these cases, we need to ensure that the pitch
> +	 * provided by userspace is divisible by the cpp.
> +	 */
> +	if (!(config->capabilities & EXYNOS_DRM_PLANE_CAP_BYTE_PITCH)) {
> +		if (fb->pitches[0] % fb->format->cpp[0])
> +			return -ENOTSUPP;
> +	}
> +
>   	switch (fb->modifier) {
>   	case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
>   		if (!(config->capabilities & EXYNOS_DRM_PLANE_CAP_TILE))

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 6/9] drm/exynos: introduce BYTE_PITCH capability

[Tobias Jakobi]

Hello Marek,

first of all thanks for checking this!


Marek Szyprowski wrote:
> Hi Tobias,
> 
> On 2017-08-22 16:19, Tobias Jakobi wrote:
>> In some of subdrivers we compute something like 'pitch / cpp' at some
>> point, silently assuming that the pitch (which is in bytes) is
>> divisible by the buffer's cpp. This must not be true, in particular
>> it depends on the underlying hardware.
>>
>> If userspace should request such a setup, we should communicate this.
>>
>> Introduce a new cap which indicates that the hardware supports a
>> pitch with 'byte-granularity'. If the cap is not set, assume that
>> we need a pitch aligned to cpp.
>>
>> We set this cap in a later patch for the drivers/planes which
>> support it.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> 
> I briefly checked the patch and it looks fine, but I really wonder whether
> drivers should support such strange formats, when pitches are not multiple
> of pixel size in bytes. I cannot find any sane use case for such formats.
Apparantly the hw can do it, so why not support it? A potential use case I see
would be an application that uses a single buffer with multiple pixelformats.

Let buffer-size = width * height * sizeof(uint16_t), with width odd. Then the
buffer pitch is not divisible by 4. In case the hw supports it, we can still use
e.g. XRGB8888 with this setup.


> Maybe it would make sense to add a check for it in DRM core? I also didn't
> notice a check for that in any other drivers, but some of them also
> compute 'pitch / cpp' values.
I thought about some check in DRM core, but I didn't see a clean way to add it,
since the behaviour very much depends on the hw.  Well, except if you just want
to enforce 'pitch % cpp == 0' everywhere. Not sure how the rest of the DRM folks
thinks about it.

With best wishes,
Tobias



>> ---
>>   drivers/gpu/drm/exynos/exynos_drm_drv.h   |  1 +
>>   drivers/gpu/drm/exynos/exynos_drm_plane.c | 10 ++++++++++
>>   2 files changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> index 43afab4bebc3..ec32632485d2 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> @@ -92,6 +92,7 @@ struct exynos_drm_plane {
>>   #define EXYNOS_DRM_PLANE_CAP_SCALE    (1 << 1)
>>   #define EXYNOS_DRM_PLANE_CAP_ZPOS    (1 << 2)
>>   #define EXYNOS_DRM_PLANE_CAP_TILE    (1 << 3)
>> +#define EXYNOS_DRM_PLANE_CAP_BYTE_PITCH    (1 << 4)
>>     /*
>>    * Exynos DRM plane configuration structure.
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> index 133af72f5c90..17e47b8f0d6a 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> @@ -185,6 +185,16 @@ exynos_drm_plane_check_format(const struct
>> exynos_drm_plane_config *config,
>>   {
>>       struct drm_framebuffer *fb = state->base.fb;
>>   +    /*
>> +     * Some blocks only allow to specify a buffer pitch in terms
>> +     * of pixels. In these cases, we need to ensure that the pitch
>> +     * provided by userspace is divisible by the cpp.
>> +     */
>> +    if (!(config->capabilities & EXYNOS_DRM_PLANE_CAP_BYTE_PITCH)) {
>> +        if (fb->pitches[0] % fb->format->cpp[0])
>> +            return -ENOTSUPP;
>> +    }
>> +
>>       switch (fb->modifier) {
>>       case DRM_FORMAT_MOD_SAMSUNG_64_32_TILE:
>>           if (!(config->capabilities & EXYNOS_DRM_PLANE_CAP_TILE))
> 
> Best regards

[PATCH v2 7/9] drm/exynos: add BYTE_PITCH cap for all supported planes

[Tobias Jakobi]

Both FIMD and DECON5433 support a pitch in bytes.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 1 +
 drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index 5792ca88ab7a..62b50e0685b0 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -546,6 +546,7 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
 		ctx->configs[win].num_pixel_formats = ARRAY_SIZE(decon_formats);
 		ctx->configs[win].zpos = win;
 		ctx->configs[win].type = decon_win_types[tmp];
+		ctx->configs[win].capabilities = EXYNOS_DRM_PLANE_CAP_BYTE_PITCH;
 
 		ret = exynos_plane_init(drm_dev, &ctx->planes[win], win,
 					&ctx->configs[win]);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 60f93cad6643..e88597f6d356 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -986,6 +986,7 @@ static int fimd_bind(struct device *dev, struct device *master, void *data)
 		ctx->configs[i].num_pixel_formats = ARRAY_SIZE(fimd_formats);
 		ctx->configs[i].zpos = i;
 		ctx->configs[i].type = fimd_win_types[i];
+		ctx->configs[i].capabilities = EXYNOS_DRM_PLANE_CAP_BYTE_PITCH;
 		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
 					&ctx->configs[i]);
 		if (ret)
-- 
2.13.0

[PATCH v2 8/9] drm/exynos: consistent use of cpp

[Tobias Jakobi]

A recent commit (272725c7db4da1fd3229d944fc76d2e98e3a144e) has removed
the use of 'bits_per_pixel' in DRM. However the corresponding Exynos
driver code still uses the ambiguous 'bpp', even though it is now
initialized from fb->cpp[0].

Consistenly use 'cpp' in FIMD, DECON7 and DECON5433 drivers.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 12 ++++++------
 drivers/gpu/drm/exynos/exynos7_drm_decon.c    |  6 +++---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  8 ++++----
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index 62b50e0685b0..66ceca0af2a0 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -286,7 +286,7 @@ static void decon_win_set_pixfmt(struct decon_context *ctx, unsigned int win,
 		return;
 	}
 
-	DRM_DEBUG_KMS("bpp = %u\n", fb->format->cpp[0] * 8);
+	DRM_DEBUG_KMS("cpp = %u\n", fb->format->cpp[0]);
 
 	/*
 	 * In case of exynos, setting dma-burst to 16Word causes permanent
@@ -329,7 +329,7 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
 	struct decon_context *ctx = crtc->ctx;
 	struct drm_framebuffer *fb = state->base.fb;
 	unsigned int win = plane->index;
-	unsigned int bpp = fb->format->cpp[0];
+	unsigned int cpp = fb->format->cpp[0];
 	unsigned int pitch = fb->pitches[0];
 	dma_addr_t dma_addr = exynos_drm_fb_dma_addr(fb, 0);
 	u32 val;
@@ -365,11 +365,11 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
 	writel(val, ctx->addr + DECON_VIDW0xADD1B0(win));
 
 	if (!(ctx->out_type & IFTYPE_HDMI))
-		val = BIT_VAL(pitch - state->crtc.w * bpp, 27, 14)
-			| BIT_VAL(state->crtc.w * bpp, 13, 0);
+		val = BIT_VAL(pitch - state->crtc.w * cpp, 27, 14)
+			| BIT_VAL(state->crtc.w * cpp, 13, 0);
 	else
-		val = BIT_VAL(pitch - state->crtc.w * bpp, 29, 15)
-			| BIT_VAL(state->crtc.w * bpp, 14, 0);
+		val = BIT_VAL(pitch - state->crtc.w * cpp, 29, 15)
+			| BIT_VAL(state->crtc.w * cpp, 14, 0);
 	writel(val, ctx->addr + DECON_VIDW0xADD2(win));
 
 	decon_win_set_pixfmt(ctx, win, fb);
diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index 3e88269fdc2e..4662d55ed988 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -321,7 +321,7 @@ static void decon_win_set_pixfmt(struct decon_context *ctx, unsigned int win,
 		break;
 	}
 
-	DRM_DEBUG_KMS("bpp = %d\n", fb->format->cpp[0] * 8);
+	DRM_DEBUG_KMS("cpp = %d\n", fb->format->cpp[0]);
 
 	/*
 	 * In case of exynos, setting dma-burst to 16Word causes permanent
@@ -398,7 +398,7 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
 	unsigned int last_x;
 	unsigned int last_y;
 	unsigned int win = plane->index;
-	unsigned int bpp = fb->format->cpp[0];
+	unsigned int cpp = fb->format->cpp[0];
 	unsigned int pitch = fb->pitches[0];
 
 	if (ctx->suspended)
@@ -418,7 +418,7 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
 	val = (unsigned long)exynos_drm_fb_dma_addr(fb, 0);
 	writel(val, ctx->regs + VIDW_BUF_START(win));
 
-	padding = (pitch / bpp) - fb->width;
+	padding = (pitch / cpp) - fb->width;
 
 	/* buffer size */
 	writel(fb->width + padding, ctx->regs + VIDW_WHOLE_X(win));
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index e88597f6d356..9b83d1846589 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -718,13 +718,13 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
 	unsigned long val, size, offset;
 	unsigned int last_x, last_y, buf_offsize, line_size;
 	unsigned int win = plane->index;
-	unsigned int bpp = fb->format->cpp[0];
+	unsigned int cpp = fb->format->cpp[0];
 	unsigned int pitch = fb->pitches[0];
 
 	if (ctx->suspended)
 		return;
 
-	offset = state->src.x * bpp;
+	offset = state->src.x * cpp;
 	offset += state->src.y * pitch;
 
 	/* buffer start address */
@@ -743,8 +743,8 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
 			state->crtc.w, state->crtc.h);
 
 	/* buffer size */
-	buf_offsize = pitch - (state->crtc.w * bpp);
-	line_size = state->crtc.w * bpp;
+	buf_offsize = pitch - (state->crtc.w * cpp);
+	line_size = state->crtc.w * cpp;
 	val = VIDW_BUF_SIZE_OFFSET(buf_offsize) |
 		VIDW_BUF_SIZE_PAGEWIDTH(line_size) |
 		VIDW_BUF_SIZE_OFFSET_E(buf_offsize) |
-- 
2.13.0

[PATCH v2 9/9] drm/exynos: simplify set_pixfmt() in DECON and FIMD drivers

[Tobias Jakobi]

DRM core already checks the validity of the pixelformat.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 4 +---
 drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 7 +------
 drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 8 +-------
 3 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index 66ceca0af2a0..3dfba34be24d 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -277,13 +277,11 @@ static void decon_win_set_pixfmt(struct decon_context *ctx, unsigned int win,
 		val |= WINCONx_BURSTLEN_16WORD;
 		break;
 	case DRM_FORMAT_ARGB8888:
+	default:
 		val |= WINCONx_BPPMODE_32BPP_A8888;
 		val |= WINCONx_WSWP_F | WINCONx_BLD_PIX_F | WINCONx_ALPHA_SEL_F;
 		val |= WINCONx_BURSTLEN_16WORD;
 		break;
-	default:
-		DRM_ERROR("Proper pixel format is not set\n");
-		return;
 	}
 
 	DRM_DEBUG_KMS("cpp = %u\n", fb->format->cpp[0]);
diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index 4662d55ed988..615efcf7782a 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -309,16 +309,11 @@ static void decon_win_set_pixfmt(struct decon_context *ctx, unsigned int win,
 		val |= WINCONx_BURSTLEN_16WORD;
 		break;
 	case DRM_FORMAT_BGRA8888:
+	default:
 		val |= WINCONx_BPPMODE_32BPP_BGRA | WINCONx_BLD_PIX |
 			WINCONx_ALPHA_SEL;
 		val |= WINCONx_BURSTLEN_16WORD;
 		break;
-	default:
-		DRM_DEBUG_KMS("invalid pixel size so using unpacked 24bpp.\n");
-
-		val |= WINCONx_BPPMODE_24BPP_xRGB;
-		val |= WINCONx_BURSTLEN_16WORD;
-		break;
 	}
 
 	DRM_DEBUG_KMS("cpp = %d\n", fb->format->cpp[0]);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 9b83d1846589..91c62e7afdd5 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -583,18 +583,12 @@ static void fimd_win_set_pixfmt(struct fimd_context *ctx, unsigned int win,
 		val |= WINCONx_BURSTLEN_16WORD;
 		break;
 	case DRM_FORMAT_ARGB8888:
+	default:
 		val |= WINCON1_BPPMODE_25BPP_A1888
 			| WINCON1_BLD_PIX | WINCON1_ALPHA_SEL;
 		val |= WINCONx_WSWP;
 		val |= WINCONx_BURSTLEN_16WORD;
 		break;
-	default:
-		DRM_DEBUG_KMS("invalid pixel size so using unpacked 24bpp.\n");
-
-		val |= WINCON0_BPPMODE_24BPP_888;
-		val |= WINCONx_WSWP;
-		val |= WINCONx_BURSTLEN_16WORD;
-		break;
 	}
 
 	/*
-- 
2.13.0

Re: [PATCH v2 0/9] drm/exynos: misc fixes and more

[Inki Dae]

Hi Tobias,

Regarding below two patches, I'd like to have more review so I will consider to merge them in next turn. Sorry for this.
  drm/exynos: introduce BYTE_PITCH capability
  drm/exynos: add BYTE_PITCH cap for all supported planes

Thanks,
Inki Dae

2017년 08월 22일 23:19에 Tobias Jakobi 이(가) 쓴 글:
> Hello,
> 
> this is the second iteration of [1], with the following changes:
> - split patch 3/8 (suggested by Inki)
> - zero init registers in patch 4/8 (suggested by Inki)
> - rephrased commit description of patch 5/8 to make it more
>   clear that this behaviour depends on the hw
> - replace zero with linear modifier in patch 2/8
> 
> 
> Summary:
> (a) Enables support for NV12MT in the mixer.
> (b) Sanitizes buffer pitch for HW with restrictions.
> (c) Misc fixes
> 
> With best wishes,
> Tobias
> 
> [1] https://www.spinics.net/lists/linux-samsung-soc/msg60235.html
> 
> 
> Tobias Jakobi (9):
>   drm/exynos: mixer: fix chroma comment in vp_video_buffer()
>   drm/exynos: mixer: enable NV12MT support for the video plane
>   drm/exynos: mixer: simplify vp_video_buffer()
>   drm/exynos: mixer: simplify mixer_graph_buffer()
>   drm/exynos: mixer: remove src offset from mixer_graph_buffer()
>   drm/exynos: introduce BYTE_PITCH capability
>   drm/exynos: add BYTE_PITCH cap for all supported planes
>   drm/exynos: consistent use of cpp
>   drm/exynos: simplify set_pixfmt() in DECON and FIMD drivers
> 
>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 17 +++++-----
>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 13 +++-----
>  drivers/gpu/drm/exynos/exynos_drm_drv.h       |  2 ++
>  drivers/gpu/drm/exynos/exynos_drm_fb.c        |  2 ++
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 17 ++++------
>  drivers/gpu/drm/exynos/exynos_drm_plane.c     | 37 +++++++++++++++++++++
>  drivers/gpu/drm/exynos/exynos_mixer.c         | 48 +++++++++------------------
>  7 files changed, 75 insertions(+), 61 deletions(-)
> 

Re: [PATCH v2 0/9] drm/exynos: misc fixes and more

[Tobias Jakobi]

Inki Dae wrote:
> Hi Tobias,
> 
> Regarding below two patches, I'd like to have more review so I will consider to merge them in next turn. Sorry for this.
Thanks, calling out to potential reviewers then :-)

- Tobias


>   drm/exynos: introduce BYTE_PITCH capability
>   drm/exynos: add BYTE_PITCH cap for all supported planes
> 
> Thanks,
> Inki Dae
> 
> 2017년 08월 22일 23:19에 Tobias Jakobi 이(가) 쓴 글:
>> Hello,
>>
>> this is the second iteration of [1], with the following changes:
>> - split patch 3/8 (suggested by Inki)
>> - zero init registers in patch 4/8 (suggested by Inki)
>> - rephrased commit description of patch 5/8 to make it more
>>   clear that this behaviour depends on the hw
>> - replace zero with linear modifier in patch 2/8
>>
>>
>> Summary:
>> (a) Enables support for NV12MT in the mixer.
>> (b) Sanitizes buffer pitch for HW with restrictions.
>> (c) Misc fixes
>>
>> With best wishes,
>> Tobias
>>
>> [1] https://www.spinics.net/lists/linux-samsung-soc/msg60235.html
>>
>>
>> Tobias Jakobi (9):
>>   drm/exynos: mixer: fix chroma comment in vp_video_buffer()
>>   drm/exynos: mixer: enable NV12MT support for the video plane
>>   drm/exynos: mixer: simplify vp_video_buffer()
>>   drm/exynos: mixer: simplify mixer_graph_buffer()
>>   drm/exynos: mixer: remove src offset from mixer_graph_buffer()
>>   drm/exynos: introduce BYTE_PITCH capability
>>   drm/exynos: add BYTE_PITCH cap for all supported planes
>>   drm/exynos: consistent use of cpp
>>   drm/exynos: simplify set_pixfmt() in DECON and FIMD drivers
>>
>>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 17 +++++-----
>>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 13 +++-----
>>  drivers/gpu/drm/exynos/exynos_drm_drv.h       |  2 ++
>>  drivers/gpu/drm/exynos/exynos_drm_fb.c        |  2 ++
>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 17 ++++------
>>  drivers/gpu/drm/exynos/exynos_drm_plane.c     | 37 +++++++++++++++++++++
>>  drivers/gpu/drm/exynos/exynos_mixer.c         | 48 +++++++++------------------
>>  7 files changed, 75 insertions(+), 61 deletions(-)
>>