On Wed, Aug 22, 2018 at 10:18:51PM +0200, Daniel Vetter wrote:
On Tue, Aug 21, 2018 at 07:30:03PM +0100, Alexandru Gheorghe wrote:
The previous patch added tile_w and tile_h, which represent the horizontal and vertical sizes of a tile.
This one uses that to plumb through drm core in order to be able to handle linear tile formats without the need for drivers to roll up their own implementation.
This patch had been written with Mali-dp X0L2 and X0L0 in mind which is a 1 plane YCbCr 420 format with a 2x2 tile, that uses in average 2 bytes per pixel and where tiles are laid out in a linear manner.
Now what are the restrictions:
- Pitch in bytes is expected to cover at least tile_h * width in
pixels. Due to this the places where the pitch is checked/used need to be updated to take into consideration the tile_w, tile_h and tile_size. tile_size = cpp * tile_w * tile_h
- When doing source cropping plane_src_x/y need to be a multiple of
tile_w/tile_h and we need to take into consideration the tile_w/tile_h when computing the start address.
For all non-tiled formats the tile_w and tile_h will be 1, so if I didn't miss anything nothing should change.
Regarding multi-planar linear tile formats, I'm not sure how those should be handle I kind of assumed that tile_h/tile_w will have to be divided by horizontal/subsampling. Anyway, I think it's best to just put an warning in there and handle it when someone tries to add support for them.
Signed-off-by: Alexandru Gheorghe alexandru-cosmin.gheorghe@arm.com
drivers/gpu/drm/drm_atomic.c | 8 +++ drivers/gpu/drm/drm_fb_cma_helper.c | 11 ++++- drivers/gpu/drm/drm_fourcc.c | 52 ++++++++++++++++++++ drivers/gpu/drm/drm_framebuffer.c | 19 +++++-- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 10 ++-- include/drm/drm_fourcc.h | 2 + 6 files changed, 94 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3eb061e11e2e..7a3e893a4cd1 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1087,6 +1087,14 @@ static int drm_atomic_plane_check(struct drm_plane *plane, return -ENOSPC; }
- /* Make sure source coordinates are a multiple of tile sizes */
- if ((state->src_x >> 16) % state->fb->format->tile_w ||
(state->src_y >> 16) % state->fb->format->tile_h) {DRM_DEBUG_ATOMIC("[PLANE:%d:%s] Source coordinates do not meet tile restrictions",plane->base.id, plane->name);return -EINVAL;- }
- if (plane_switching_crtc(state->state, plane, state)) { DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n", plane->base.id, plane->name);
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c b/drivers/gpu/drm/drm_fb_cma_helper.c index 47e0e2f6642d..4d8052adce67 100644 --- a/drivers/gpu/drm/drm_fb_cma_helper.c +++ b/drivers/gpu/drm/drm_fb_cma_helper.c @@ -87,6 +87,8 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, struct drm_gem_cma_object *obj; dma_addr_t paddr; u8 h_div = 1, v_div = 1;
u32 tile_w = drm_format_tile_width(fb->format, plane);
u32 tile_h = drm_format_tile_height(fb->format, plane);
obj = drm_fb_cma_get_gem_obj(fb, plane); if (!obj)
@@ -99,8 +101,13 @@ dma_addr_t drm_fb_cma_get_gem_addr(struct drm_framebuffer *fb, v_div = fb->format->vsub; }
- paddr += (fb->format->cpp[plane] * (state->src_x >> 16)) / h_div;
- paddr += (fb->pitches[plane] * (state->src_y >> 16)) / v_div;
paddr += (fb->format->cpp[plane] * tile_w * (state->src_x >> 16))
/ h_div;/*
* For tile formats pitches are expected to cover at least* width * tile_h pixels*/paddr += ((fb->pitches[plane] / tile_h) * (state->src_y >> 16)) / v_div;
return paddr;
} diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index f55cd93ba2d0..d6c9c5aa4036 100644 --- a/drivers/gpu/drm/drm_fourcc.c +++ b/drivers/gpu/drm/drm_fourcc.c @@ -557,3 +557,55 @@ int drm_format_plane_height(int height, uint32_t format, int plane) return height / info->vsub; } EXPORT_SYMBOL(drm_format_plane_height);
+/**
- drm_format_tile_width - width of a tile for tile formats, should be 1 for all
- non-tiled formats.
- @format: pixel format
- @plane: plane index
- Returns:
- The width of a tile, depending on the plane index and horizontal sub-sampling
- */
+uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane) +{
- WARN_ON(!info->tile_w);
- if (plane == 0 || info->tile_w == 1)
return info->tile_w;- /*
* Multi planar tiled formats have never been tested, check that* buffer restrictions and source cropping meet the format layout* expectations.*/- WARN_ON("Multi-planar tiled formats unsupported");
- WARN_ON(info->tile_w % info->hsub);
- return info->tile_w / info->hsub;
+} +EXPORT_SYMBOL(drm_format_tile_width);
+/**
- drm_format_tile_height - height of a tile for tile formats, should be 1 for
- all non-tiled formats.
- @format: pixel format
- @plane: plane index
- Returns:
- The height of a tile, depending on the plane index and vertical sub-sampling
- */
+uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane) +{
- WARN_ON(!info->tile_h);
- if (plane == 0 || info->tile_h == 1)
return info->tile_h;- /*
* Multi planar tiled formats have never been tested, check that* buffer restrictions and source cropping meet the format layout* expectations.*/- WARN_ON("Multi-planar tiled formats unsupported");
- WARN_ON(info->tile_h % info->vsub);
- return info->tile_h / info->vsub;
+} +EXPORT_SYMBOL(drm_format_tile_height); diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c index 781af1d42d76..57509e51cb80 100644 --- a/drivers/gpu/drm/drm_framebuffer.c +++ b/drivers/gpu/drm/drm_framebuffer.c @@ -191,19 +191,32 @@ static int framebuffer_check(struct drm_device *dev, unsigned int width = fb_plane_width(r->width, info, i); unsigned int height = fb_plane_height(r->height, info, i); unsigned int cpp = info->cpp[i];
unsigned int tile_w = drm_format_tile_width(info, i);unsigned int tile_h = drm_format_tile_height(info, i);unsigned int tile_size = cpp * tile_w * tile_h;unsigned int num_htiles;unsigned int num_vtiles;if (!r->handles[i]) { DRM_DEBUG_KMS("no buffer object handle for plane %d\n", i); return -EINVAL; }
if ((uint64_t) width * cpp > UINT_MAX)
if ((width % tile_w) || (height % tile_h)) {I think this is too strict. You can carve out a sub-part of anything really with a drm_framebuffer. See below, we only checked that width * cpp < pitches, so leftover bytes/bits was always ok.
Your driver might have additional constraints, but that's a different story.
This seem like the only thing that we have in common with DRM_FORMAT_MOD_SAMSUNG_64_32_TILE/NV12MT, which is even more restrictive than that and it wants the width to be a multiple of 128.
DRM_DEBUG_KMS("buffer width/height need to be a multiple of tile dimensions\n");return -EINVAL;}num_htiles = width / tile_w;num_vtiles = height / tile_h;if ((uint64_t)num_htiles * tile_size > UINT_MAX) return -ERANGE;
if ((uint64_t) height * r->pitches[i] + r->offsets[i] > UINT_MAX)
if ((uint64_t)num_vtiles * r->pitches[i] + r->offsets[i] > UINT_MAX) return -ERANGE;
if (r->pitches[i] < width * cpp) {
if (r->pitches[i] < num_htiles * tile_size) {Essentially you define ->pitches now to mean a row of tiles. At least for bigger tiled formats (using modifiers) we don't use it like that. But it also gets awkward quickly if we keep pitches to mean bytes in one row of pixels. For that case we'd have
That's how everything started, we have already internal userspace and drivers that expect and pass pitch as a row of tiles in bytes.
For the tiles format in this patchset(X0L2/X0L0) which orders the tiles in linear manner the pitch will at least get you at the beginning of a row.
I'm not sure I understand what's the meaning of a pitch for DRM_FORMAT_MOD_SAMSUNG_64_32_TILE which orders the tiles in Z shape. How far will a pitch for this format get you, it seems that using just witdth * cpp will get you somewhere in the middle.
pitches % (cpp * tile_w) == 0 pitches * align(height, tile_h) < size
That's at least the definition of tiling we've used in i915. It also matches what we've done for DRM_FORMAT_MOD_SAMSUNG_64_32_TILE.
This definition of pitches becomes a bit nonsense for fractional cpp, but well, cpp is by definition not fractional, so we can figure that out when we actually have that problem. -Daniel
DRM_DEBUG_KMS("bad pitch %u for plane %d\n", r->pitches[i], i); return -EINVAL; }diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 2810d4131411..3d01a1a9d5d2 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -161,6 +161,11 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, unsigned int width = mode_cmd->width / (i ? info->hsub : 1); unsigned int height = mode_cmd->height / (i ? info->vsub : 1); unsigned int min_size;
unsigned int tile_w = drm_format_tile_width(info, i);unsigned int tile_h = drm_format_tile_height(info, i);unsigned int tile_size = info->cpp[i] * tile_w * tile_h;unsigned int num_htiles = width / tile_w;unsigned int num_vtiles = height / tile_h;objs[i] = drm_gem_object_lookup(file, mode_cmd->handles[i]); if (!objs[i]) {
@@ -169,9 +174,8 @@ drm_gem_fb_create_with_funcs(struct drm_device *dev, struct drm_file *file, goto err_gem_object_put; }
min_size = (height - 1) * mode_cmd->pitches[i]+ width * info->cpp[i]+ mode_cmd->offsets[i];
min_size = (num_vtiles - 1) * mode_cmd->pitches[i]+ num_htiles * tile_size + mode_cmd->offsets[i];if (objs[i]->size < min_size) { drm_gem_object_put_unlocked(objs[i]);
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index 41681cf2b140..001afca9bcff 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -76,6 +76,8 @@ int drm_format_horz_chroma_subsampling(uint32_t format); int drm_format_vert_chroma_subsampling(uint32_t format); int drm_format_plane_width(int width, uint32_t format, int plane); int drm_format_plane_height(int height, uint32_t format, int plane); +uint32_t drm_format_tile_width(const struct drm_format_info *info, int plane); +uint32_t drm_format_tile_height(const struct drm_format_info *info, int plane); const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf);
#endif /* __DRM_FOURCC_H__ */
2.18.0
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch