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(-)
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));
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));
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);
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 */
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 */
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); }
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))
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
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
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)
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) |
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; }
/*
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(-)
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(-)
dri-devel@lists.freedesktop.org
-
Inki Dae -
Marek Szyprowski -
Tobias Jakobi