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:
>
> 1. 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
>
> 2. 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@xxxxxxx>
> ---
> 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.
> + 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
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
