[PATCH v2 4/5] drm: Add support for handling linear tile formats

Daniel Vetter daniel at ffwll.ch
Fri Aug 31 08:14:14 UTC 2018


On Thu, Aug 23, 2018 at 06:43:40PM +0100, Alexandru-Cosmin Gheorghe wrote:
> 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:
> > > 
> > > 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 at 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.

So there's 2 interpretations of pitch if you have a tile/block layout:

- What you do, where pitch = num_tiles_per_row * tile_size. Adding pitch
  to the base address will then advance tile_h pixels. Let's call this
  alex_pitch.

- What I'm proposing, and what NV12MT does, and what every other driver
  does (afaik, I didn't carefully check them all): Pretend there's no
  tile, and compute the pitch as if it's a linear format. The reason
  behind this is that fairly often for tiled layouts (but not always), you
  have magic remapping hardware, which can present a linear layout to you.
  So for NV12MT that would resolve the Z-shape and macroblock layout, and
  give you a normal NV12 framebuffer. Let's call this drm_pitch. Note that
  the drm pitch does _not_ make any sense when you add it to the physical
  base address, only if you add mutliple's of tile_h, to get an entire
  tile row.


The relationship is drm_pitch = alex_pitch / tile_h. I'm not aware of any
format where that doesn't devide evenly, so in practice it doesn't matter
which one you pick. Imo either has up/downsides.

Cheers, Daniel

> 
> > 
> > 	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
> 
> -- 
> Cheers,
> Alex G

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch


More information about the dri-devel mailing list