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

Commit Message

Alexandru-Cosmin Gheorghe Aug. 21, 2018, 6:30 p.m. UTC

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 <[e-mail]>
---
 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(-)

Comments

Liviu Dudau Aug. 22, 2018, 1:07 p.m. UTC | #1

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.

This reads wrong for the first time reader. Maybe make the 'cpp'
explicit like you do later on?

>         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 <[e-mail]>
> ---
>  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;

I think you can keep the division by h_div on the same line, you're not
spilling over the limit that much compared to the next line of code.

> +	/*
> +	 * 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);

Maybe you wanted to use WARN_ONCE() ? Otherwise drm_format_tile_width() prints a
warning everytime you call it?

> +	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);

Same here, WARN_ONCE().

> +	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)) {
> +			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) {
>  			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
> 

Reviewed-by: Liviu Dudau <[e-mail]>

Best regards,
Liviu

Ville Syrjälä Aug. 22, 2018, 1:18 p.m. UTC | #2

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 <[e-mail]>
> ---
>  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;
> +	}

Feels rather wrong to me to put such hardware specific limitations into
the core.

> +
>  	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)) {
> +			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) {
>  			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
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Alexandru-Cosmin Gheorghe Aug. 22, 2018, 1:36 p.m. UTC | #3

On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä 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 <[e-mail]>
> > ---
> >  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;
> > +	}
> 
> Feels rather wrong to me to put such hardware specific limitations into
> the core.

Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't
work.

An alternative, would be to include it in the driver and put an WARN
in drm_fb_cma_get_gem_addr in case someone else uses it with a
src_x/src_y tile multiples.

What do you think about that ?

> 
> > +
> >  	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)) {
> > +			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) {
> >  			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
> > 
> > _______________________________________________
> > dri-devel mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Ville Syrjälä Aug. 22, 2018, 1:45 p.m. UTC | #4

On Wed, Aug 22, 2018 at 02:36:06PM +0100, Alexandru-Cosmin Gheorghe wrote:
> On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä 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 <[e-mail]>
> > > ---
> > >  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;
> > > +	}
> > 
> > Feels rather wrong to me to put such hardware specific limitations into
> > the core.
> 
> Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't
> work.

If that guy is supposed to give you a tile aligned address then it could
just do that and leave it up to the driver to deal with the remainder of
the coordinates?

> 
> An alternative, would be to include it in the driver and put an WARN
> in drm_fb_cma_get_gem_addr in case someone else uses it with a
> src_x/src_y tile multiples.
> 
> What do you think about that ?
> 
> > 
> > > +
> > >  	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;

Presumably this thing should be using the clipped coordinates instead of
the raw user provided ones?

> > >  
> > >  	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)) {
> > > +			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) {
> > >  			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
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> > _______________________________________________
> > dri-devel mailing list
> > [e-mail]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Cheers,
> Alex G

Alexandru-Cosmin Gheorghe Aug. 22, 2018, 2:05 p.m. UTC | #5

On Wed, Aug 22, 2018 at 04:45:34PM +0300, Ville Syrjälä wrote:
> On Wed, Aug 22, 2018 at 02:36:06PM +0100, Alexandru-Cosmin Gheorghe wrote:
> > On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä 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 <[e-mail]>
> > > > ---
> > > >  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;
> > > > +	}
> > > 
> > > Feels rather wrong to me to put such hardware specific limitations into
> > > the core.
> > 
> > Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't
> > work.
> 
> If that guy is supposed to give you a tile aligned address then it could
> just do that and leave it up to the driver to deal with the remainder of
> the coordinates?
> 
> > 
> > An alternative, would be to include it in the driver and put an WARN
> > in drm_fb_cma_get_gem_addr in case someone else uses it with a
> > src_x/src_y tile multiples.
> > 
> > What do you think about that ?
> > 
> > > 
> > > > +
> > > >  	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;
> 
> Presumably this thing should be using the clipped coordinates instead of
> the raw user provided ones?

So you expect the driver to thinker with state->src_x and state->src_y
to be tiled aligned?

What do you expect to happen if this function gets passed a
src_x/src_y that's not tile aligned, WARN about it or just silently
return the beginning of the tile and document that behaviour.

> 
> > > >  
> > > >  	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)) {
> > > > +			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) {
> > > >  			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
> > > > 
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > [e-mail]
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> > > _______________________________________________
> > > dri-devel mailing list
> > > [e-mail]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > -- 
> > Cheers,
> > Alex G
> 
> -- 
> Ville Syrjälä
> Intel

Daniel Vetter Aug. 22, 2018, 7:48 p.m. UTC | #6

On Wed, Aug 22, 2018 at 4:05 PM, Alexandru-Cosmin Gheorghe
<[e-mail]> wrote:
> On Wed, Aug 22, 2018 at 04:45:34PM +0300, Ville Syrjälä wrote:
>> On Wed, Aug 22, 2018 at 02:36:06PM +0100, Alexandru-Cosmin Gheorghe wrote:
>> > On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä 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 <[e-mail]>
>> > > > ---
>> > > >  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;
>> > > > +       }
>> > >
>> > > Feels rather wrong to me to put such hardware specific limitations into
>> > > the core.
>> >
>> > Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't
>> > work.
>>
>> If that guy is supposed to give you a tile aligned address then it could
>> just do that and leave it up to the driver to deal with the remainder of
>> the coordinates?
>>
>> >
>> > An alternative, would be to include it in the driver and put an WARN
>> > in drm_fb_cma_get_gem_addr in case someone else uses it with a
>> > src_x/src_y tile multiples.
>> >
>> > What do you think about that ?
>> >
>> > >
>> > > > +
>> > > >         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;
>>
>> Presumably this thing should be using the clipped coordinates instead of
>> the raw user provided ones?
>
> So you expect the driver to thinker with state->src_x and state->src_y
> to be tiled aligned?
>
> What do you expect to happen if this function gets passed a
> src_x/src_y that's not tile aligned, WARN about it or just silently
> return the beginning of the tile and document that behaviour.

WARN really hard and return NULL or something for any tiled format.
Imo a plain address just doesn't make sense for a tile layout: To get
a good starting point you need both a base address (which will be
aligned to tile rows ofc), plus a x/y offset within. The driver can
then decided whether it must have a tile offset of 0, or what exactly
it can handle. So yeah I think we need a new helper here that gets
this right.
-Daniel

>
>>
>> > > >
>> > > >         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)) {
>> > > > +                       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) {
>> > > >                         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
>> > > >
>> > > > _______________________________________________
>> > > > dri-devel mailing list
>> > > > [e-mail]
>> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> > >
>> > > --
>> > > Ville Syrjälä
>> > > Intel
>> > > _______________________________________________
>> > > dri-devel mailing list
>> > > [e-mail]
>> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> > --
>> > Cheers,
>> > Alex G
>>
>> --
>> Ville Syrjälä
>> Intel
>
> --
> Cheers,
> Alex G

Daniel Vetter Aug. 22, 2018, 8:03 p.m. UTC | #7

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 <[e-mail]>

Just general comment: I think it'd be awesome (and also a great
demonstration that the tile_h/w approach is sound) if we could subsume the
DRM_FORMAT_MOD_SAMSUNG_64_32_TILE stuff into this.

Probably need a core change to drm_get_format_info for this pair (and
reject it for everything else), with the correct tile sizes set.
-Daniel

> ---
>  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)) {
> +			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) {
>  			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 Aug. 22, 2018, 8:18 p.m. UTC | #8

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 <[e-mail]>
> ---
>  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
>

Ville Syrjälä Aug. 23, 2018, 2:30 p.m. UTC | #9

On Wed, Aug 22, 2018 at 09:48:29PM +0200, Daniel Vetter wrote:
> On Wed, Aug 22, 2018 at 4:05 PM, Alexandru-Cosmin Gheorghe
> <[e-mail]> wrote:
> > On Wed, Aug 22, 2018 at 04:45:34PM +0300, Ville Syrjälä wrote:
> >> On Wed, Aug 22, 2018 at 02:36:06PM +0100, Alexandru-Cosmin Gheorghe wrote:
> >> > On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä 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 <[e-mail]>
> >> > > > ---
> >> > > >  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;
> >> > > > +       }
> >> > >
> >> > > Feels rather wrong to me to put such hardware specific limitations into
> >> > > the core.
> >> >
> >> > Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't
> >> > work.
> >>
> >> If that guy is supposed to give you a tile aligned address then it could
> >> just do that and leave it up to the driver to deal with the remainder of
> >> the coordinates?
> >>
> >> >
> >> > An alternative, would be to include it in the driver and put an WARN
> >> > in drm_fb_cma_get_gem_addr in case someone else uses it with a
> >> > src_x/src_y tile multiples.
> >> >
> >> > What do you think about that ?
> >> >
> >> > >
> >> > > > +
> >> > > >         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;
> >>
> >> Presumably this thing should be using the clipped coordinates instead of
> >> the raw user provided ones?
> >
> > So you expect the driver to thinker with state->src_x and state->src_y
> > to be tiled aligned?

I was referring to using state->src.x1 etc. Using src_x/y will cause you
to scan out the wrong stuff if the plane top/left edge got clipped.
And as a side note: drivers are not even allowed to muck with src_x/y
etc. because that would corrupt the state that user has set up.

> >
> > What do you expect to happen if this function gets passed a
> > src_x/src_y that's not tile aligned, WARN about it or just silently
> > return the beginning of the tile and document that behaviour.
> 
> WARN really hard and return NULL or something for any tiled format.
> Imo a plain address just doesn't make sense for a tile layout: To get
> a good starting point you need both a base address (which will be
> aligned to tile rows ofc), plus a x/y offset within. The driver can
> then decided whether it must have a tile offset of 0, or what exactly
> it can handle. So yeah I think we need a new helper here that gets
> this right.

Yeah, something that either returns the remaining intra-tile x/y
offsets, or the tile aligned x/y offsets and lets the driver deal
with the remainder in whatever way is appropriate (which could even
be -EINVAL in case the hw can't simply handle it). The latter being
pretty much how i915 does things.

Alexandru-Cosmin Gheorghe Aug. 23, 2018, 5:19 p.m. UTC | #10

On Thu, Aug 23, 2018 at 05:30:36PM +0300, Ville Syrjälä wrote:
> On Wed, Aug 22, 2018 at 09:48:29PM +0200, Daniel Vetter wrote:
> > On Wed, Aug 22, 2018 at 4:05 PM, Alexandru-Cosmin Gheorghe
> > <[e-mail]> wrote:
> > > On Wed, Aug 22, 2018 at 04:45:34PM +0300, Ville Syrjälä wrote:
> > >> On Wed, Aug 22, 2018 at 02:36:06PM +0100, Alexandru-Cosmin Gheorghe wrote:
> > >> > On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä 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 <[e-mail]>
> > >> > > > ---
> > >> > > >  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;
> > >> > > > +       }
> > >> > >
> > >> > > Feels rather wrong to me to put such hardware specific limitations into
> > >> > > the core.
> > >> >
> > >> > Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't
> > >> > work.
> > >>
> > >> If that guy is supposed to give you a tile aligned address then it could
> > >> just do that and leave it up to the driver to deal with the remainder of
> > >> the coordinates?
> > >>
> > >> >
> > >> > An alternative, would be to include it in the driver and put an WARN
> > >> > in drm_fb_cma_get_gem_addr in case someone else uses it with a
> > >> > src_x/src_y tile multiples.
> > >> >
> > >> > What do you think about that ?
> > >> >
> > >> > >
> > >> > > > +
> > >> > > >         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;
> > >>
> > >> Presumably this thing should be using the clipped coordinates instead of
> > >> the raw user provided ones?
> > >
> > > So you expect the driver to thinker with state->src_x and state->src_y
> > > to be tiled aligned?
> 
> I was referring to using state->src.x1 etc. Using src_x/y will cause you
> to scan out the wrong stuff if the plane top/left edge got clipped.
> And as a side note: drivers are not even allowed to muck with src_x/y
> etc. because that would corrupt the state that user has set up.

Yes, it seems this function should have used src.x1, instead of
src_x/y, but I'm not sure it can be easily fix since not all the
drivers calling it call drm_atomic_helper_check_plane_state that
populates src/dst rectangles.

> 
> > >
> > > What do you expect to happen if this function gets passed a
> > > src_x/src_y that's not tile aligned, WARN about it or just silently
> > > return the beginning of the tile and document that behaviour.
> > 
> > WARN really hard and return NULL or something for any tiled format.
> > Imo a plain address just doesn't make sense for a tile layout: To get
> > a good starting point you need both a base address (which will be
> > aligned to tile rows ofc), plus a x/y offset within. The driver can
> > then decided whether it must have a tile offset of 0, or what exactly
> > it can handle. So yeah I think we need a new helper here that gets
> > this right.
> 
> Yeah, something that either returns the remaining intra-tile x/y
> offsets, or the tile aligned x/y offsets and lets the driver deal
> with the remainder in whatever way is appropriate (which could even
> be -EINVAL in case the hw can't simply handle it). The latter being
> pretty much how i915 does things.

Looking at DRM_FORMAT_MOD_SAMSUNG_64_32_TILE with its Z shape
ordering of the tiles, it doesn't seem there could be any generic way
of getting the address of a pixel, especially using just the tile_w
and tile_h.



> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjälä Aug. 23, 2018, 5:25 p.m. UTC | #11

On Thu, Aug 23, 2018 at 06:19:31PM +0100, Alexandru-Cosmin Gheorghe wrote:
> On Thu, Aug 23, 2018 at 05:30:36PM +0300, Ville Syrjälä wrote:
> > On Wed, Aug 22, 2018 at 09:48:29PM +0200, Daniel Vetter wrote:
> > > On Wed, Aug 22, 2018 at 4:05 PM, Alexandru-Cosmin Gheorghe
> > > <[e-mail]> wrote:
> > > > On Wed, Aug 22, 2018 at 04:45:34PM +0300, Ville Syrjälä wrote:
> > > >> On Wed, Aug 22, 2018 at 02:36:06PM +0100, Alexandru-Cosmin Gheorghe wrote:
> > > >> > On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä 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 <[e-mail]>
> > > >> > > > ---
> > > >> > > >  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;
> > > >> > > > +       }
> > > >> > >
> > > >> > > Feels rather wrong to me to put such hardware specific limitations into
> > > >> > > the core.
> > > >> >
> > > >> > Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't
> > > >> > work.
> > > >>
> > > >> If that guy is supposed to give you a tile aligned address then it could
> > > >> just do that and leave it up to the driver to deal with the remainder of
> > > >> the coordinates?
> > > >>
> > > >> >
> > > >> > An alternative, would be to include it in the driver and put an WARN
> > > >> > in drm_fb_cma_get_gem_addr in case someone else uses it with a
> > > >> > src_x/src_y tile multiples.
> > > >> >
> > > >> > What do you think about that ?
> > > >> >
> > > >> > >
> > > >> > > > +
> > > >> > > >         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;
> > > >>
> > > >> Presumably this thing should be using the clipped coordinates instead of
> > > >> the raw user provided ones?
> > > >
> > > > So you expect the driver to thinker with state->src_x and state->src_y
> > > > to be tiled aligned?
> > 
> > I was referring to using state->src.x1 etc. Using src_x/y will cause you
> > to scan out the wrong stuff if the plane top/left edge got clipped.
> > And as a side note: drivers are not even allowed to muck with src_x/y
> > etc. because that would corrupt the state that user has set up.
> 
> Yes, it seems this function should have used src.x1, instead of
> src_x/y, but I'm not sure it can be easily fix since not all the
> drivers calling it call drm_atomic_helper_check_plane_state that
> populates src/dst rectangles.

Time to fix those drivers?

Alexandru-Cosmin Gheorghe Aug. 23, 2018, 5:43 p.m. UTC | #12

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 <[e-mail]>
> > ---
> >  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

Alexandru-Cosmin Gheorghe Aug. 23, 2018, 5:56 p.m. UTC | #13

On Thu, Aug 23, 2018 at 08:25:46PM +0300, Ville Syrjälä wrote:
> On Thu, Aug 23, 2018 at 06:19:31PM +0100, Alexandru-Cosmin Gheorghe wrote:
> > On Thu, Aug 23, 2018 at 05:30:36PM +0300, Ville Syrjälä wrote:
> > > On Wed, Aug 22, 2018 at 09:48:29PM +0200, Daniel Vetter wrote:
> > > > On Wed, Aug 22, 2018 at 4:05 PM, Alexandru-Cosmin Gheorghe
> > > > <[e-mail]> wrote:
> > > > > On Wed, Aug 22, 2018 at 04:45:34PM +0300, Ville Syrjälä wrote:
> > > > >> On Wed, Aug 22, 2018 at 02:36:06PM +0100, Alexandru-Cosmin Gheorghe wrote:
> > > > >> > On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä 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 <[e-mail]>
> > > > >> > > > ---
> > > > >> > > >  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;
> > > > >> > > > +       }
> > > > >> > >
> > > > >> > > Feels rather wrong to me to put such hardware specific limitations into
> > > > >> > > the core.
> > > > >> >
> > > > >> > Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't
> > > > >> > work.
> > > > >>
> > > > >> If that guy is supposed to give you a tile aligned address then it could
> > > > >> just do that and leave it up to the driver to deal with the remainder of
> > > > >> the coordinates?
> > > > >>
> > > > >> >
> > > > >> > An alternative, would be to include it in the driver and put an WARN
> > > > >> > in drm_fb_cma_get_gem_addr in case someone else uses it with a
> > > > >> > src_x/src_y tile multiples.
> > > > >> >
> > > > >> > What do you think about that ?
> > > > >> >
> > > > >> > >
> > > > >> > > > +
> > > > >> > > >         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;
> > > > >>
> > > > >> Presumably this thing should be using the clipped coordinates instead of
> > > > >> the raw user provided ones?
> > > > >
> > > > > So you expect the driver to thinker with state->src_x and state->src_y
> > > > > to be tiled aligned?
> > > 
> > > I was referring to using state->src.x1 etc. Using src_x/y will cause you
> > > to scan out the wrong stuff if the plane top/left edge got clipped.
> > > And as a side note: drivers are not even allowed to muck with src_x/y
> > > etc. because that would corrupt the state that user has set up.
> > 
> > Yes, it seems this function should have used src.x1, instead of
> > src_x/y, but I'm not sure it can be easily fix since not all the
> > drivers calling it call drm_atomic_helper_check_plane_state that
> > populates src/dst rectangles.
> 
> Time to fix those drivers?

I'm bit reluctant to add restrictions/checks in other peoples drivers
without being able to test it.

> 
> -- 
> Ville Syrjälä
> Intel

Daniel Vetter Aug. 31, 2018, 8:03 a.m. UTC | #14

On Thu, Aug 23, 2018 at 06:19:31PM +0100, Alexandru-Cosmin Gheorghe wrote:
> On Thu, Aug 23, 2018 at 05:30:36PM +0300, Ville Syrjälä wrote:
> > On Wed, Aug 22, 2018 at 09:48:29PM +0200, Daniel Vetter wrote:
> > > On Wed, Aug 22, 2018 at 4:05 PM, Alexandru-Cosmin Gheorghe
> > > <[e-mail]> wrote:
> > > > On Wed, Aug 22, 2018 at 04:45:34PM +0300, Ville Syrjälä wrote:
> > > >> On Wed, Aug 22, 2018 at 02:36:06PM +0100, Alexandru-Cosmin Gheorghe wrote:
> > > >> > On Wed, Aug 22, 2018 at 04:18:54PM +0300, Ville Syrjälä 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 <[e-mail]>
> > > >> > > > ---
> > > >> > > >  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;
> > > >> > > > +       }
> > > >> > >
> > > >> > > Feels rather wrong to me to put such hardware specific limitations into
> > > >> > > the core.
> > > >> >
> > > >> > Yeah, the problem is that otherwise drm_fb_cma_get_gem_addr wouldn't
> > > >> > work.
> > > >>
> > > >> If that guy is supposed to give you a tile aligned address then it could
> > > >> just do that and leave it up to the driver to deal with the remainder of
> > > >> the coordinates?
> > > >>
> > > >> >
> > > >> > An alternative, would be to include it in the driver and put an WARN
> > > >> > in drm_fb_cma_get_gem_addr in case someone else uses it with a
> > > >> > src_x/src_y tile multiples.
> > > >> >
> > > >> > What do you think about that ?
> > > >> >
> > > >> > >
> > > >> > > > +
> > > >> > > >         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;
> > > >>
> > > >> Presumably this thing should be using the clipped coordinates instead of
> > > >> the raw user provided ones?
> > > >
> > > > So you expect the driver to thinker with state->src_x and state->src_y
> > > > to be tiled aligned?
> > 
> > I was referring to using state->src.x1 etc. Using src_x/y will cause you
> > to scan out the wrong stuff if the plane top/left edge got clipped.
> > And as a side note: drivers are not even allowed to muck with src_x/y
> > etc. because that would corrupt the state that user has set up.
> 
> Yes, it seems this function should have used src.x1, instead of
> src_x/y, but I'm not sure it can be easily fix since not all the
> drivers calling it call drm_atomic_helper_check_plane_state that
> populates src/dst rectangles.
> 
> > 
> > > >
> > > > What do you expect to happen if this function gets passed a
> > > > src_x/src_y that's not tile aligned, WARN about it or just silently
> > > > return the beginning of the tile and document that behaviour.
> > > 
> > > WARN really hard and return NULL or something for any tiled format.
> > > Imo a plain address just doesn't make sense for a tile layout: To get
> > > a good starting point you need both a base address (which will be
> > > aligned to tile rows ofc), plus a x/y offset within. The driver can
> > > then decided whether it must have a tile offset of 0, or what exactly
> > > it can handle. So yeah I think we need a new helper here that gets
> > > this right.
> > 
> > Yeah, something that either returns the remaining intra-tile x/y
> > offsets, or the tile aligned x/y offsets and lets the driver deal
> > with the remainder in whatever way is appropriate (which could even
> > be -EINVAL in case the hw can't simply handle it). The latter being
> > pretty much how i915 does things.
> 
> Looking at DRM_FORMAT_MOD_SAMSUNG_64_32_TILE with its Z shape
> ordering of the tiles, it doesn't seem there could be any generic way
> of getting the address of a pixel, especially using just the tile_w
> and tile_h.

The Z-layout within a macroblock is totally irrelevant. What I have in
mind is something like intel_compute_tile_offset, but with explicit return
values instead of filling in the plane state (makes it easier to
integrate). What this does (more or less at least):

- Align the base address of the framebuffer to the nearest full tile row.
  This is a full physical address.

- From that base address, which is fully aligned to tiles (so doesn't deal
  with Z-shaped or anything like that), compute the remaining x/y offset
  in _pixels_. Not bytes, because as you correctly note, for many tile
  layouts a byte offset makes no sense at all.

Then drivers can convert that pixel offset into something meaningful for
them:
- Some have direct x/y pixel offset registers.
- Some just outright require that you align with the tile/block, so
  anything != 0 would be converted to a -EINVAL.
- Some have other means (like converting to a byte address, because they
  only deal with tile/block formats where that makes sense).

This concept was derived from the gallium format stuff (that's at least
where I've seen it first), and 3d gpus deal with a _lot_ fancier tile
layouts than even NV12MT :-) And it works.
-Daniel

Daniel Vetter Aug. 31, 2018, 8:14 a.m. UTC | #15

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 <[e-mail]>
> > > ---
> > >  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

Ville Syrjälä Aug. 31, 2018, 11:20 a.m. UTC | #16

On Fri, Aug 31, 2018 at 10:14:14AM +0200, Daniel Vetter wrote:
> 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 <[e-mail]>
> > > > ---
> > > >  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.

Since everyone is already using the linear pitch approach I'd suggest
sticking with that. Might make life a little easier for generic
userspace (though it still needs to know other details of the tiling
format of course).

Daniel Vetter Aug. 31, 2018, 3:12 p.m. UTC | #17

On Fri, Aug 31, 2018 at 02:20:31PM +0300, Ville Syrjälä wrote:
> On Fri, Aug 31, 2018 at 10:14:14AM +0200, Daniel Vetter wrote:
> > 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 <[e-mail]>
> > > > > ---
> > > > >  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.
> 
> Since everyone is already using the linear pitch approach I'd suggest
> sticking with that. Might make life a little easier for generic
> userspace (though it still needs to know other details of the tiling
> format of course).

Yeah, that's what I meant to add/clarify. The fake linear pitch is, to the
best of my knowledge, already the standard we're using in drm.
-Daniel

Alexandru-Cosmin Gheorghe Aug. 31, 2018, 4:26 p.m. UTC | #18

Hi, 

On Fri, Aug 31, 2018 at 05:12:24PM +0200, Daniel Vetter wrote:
> On Fri, Aug 31, 2018 at 02:20:31PM +0300, Ville Syrjälä wrote:
> > On Fri, Aug 31, 2018 at 10:14:14AM +0200, Daniel Vetter wrote:
> > > 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 <[e-mail]>
> > > > > > ---
> > > > > >  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.

Is more or less mali_pitch, rather than 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.

That's exactly what I'm proposing, for linear tile formats we should
use a pitch that naturaly has sense which is drm_pitch * tile_h.

For any other shape of arranging arranging the tiles like the Z shape
of NV12MT, the fake pitch meaning would remain unchanged.

> > > 
> > > 
> > > 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.
> > 
> > Since everyone is already using the linear pitch approach I'd suggest
> > sticking with that. Might make life a little easier for generic
> > userspace (though it still needs to know other details of the tiling
> > format of course).
> 
> Yeah, that's what I meant to add/clarify. The fake linear pitch is, to the
> best of my knowledge, already the standard we're using in drm.
> -Daniel

Everyone except Mali.
Still, it seems like a good idea to transform something "fake" in
something with meaning, of course without breaking the userspace
:). 

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

Ville Syrjälä Aug. 31, 2018, 4:49 p.m. UTC | #19

On Fri, Aug 31, 2018 at 05:12:24PM +0200, Daniel Vetter wrote:
> On Fri, Aug 31, 2018 at 02:20:31PM +0300, Ville Syrjälä wrote:
> > On Fri, Aug 31, 2018 at 10:14:14AM +0200, Daniel Vetter wrote:
> > > 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 <[e-mail]>
> > > > > > ---
> > > > > >  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.
> > 
> > Since everyone is already using the linear pitch approach I'd suggest
> > sticking with that. Might make life a little easier for generic
> > userspace (though it still needs to know other details of the tiling
> > format of course).
> 
> Yeah, that's what I meant to add/clarify. The fake linear pitch is, to the
> best of my knowledge, already the standard we're using in drm.

And upon further reflection that was in fact even baked into some
of the code in framebuffer_check() since years ago. But those sanity
checks can't catch someone using the tile row size as the pitch since
that's always greater or equal to the linear pitch, but clearly the
way the code was written assumes linear pitch.

Daniel Vetter Sept. 3, 2018, 7:26 a.m. UTC | #20

On Fri, Aug 31, 2018 at 05:26:37PM +0100, Alexandru-Cosmin Gheorghe wrote:
> Hi, 
> 
> On Fri, Aug 31, 2018 at 05:12:24PM +0200, Daniel Vetter wrote:
> > On Fri, Aug 31, 2018 at 02:20:31PM +0300, Ville Syrjälä wrote:
> > > On Fri, Aug 31, 2018 at 10:14:14AM +0200, Daniel Vetter wrote:
> > > > 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 <[e-mail]>
> > > > > > > ---
> > > > > > >  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.
> 
> Is more or less mali_pitch, rather than 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.
> 
> That's exactly what I'm proposing, for linear tile formats we should
> use a pitch that naturaly has sense which is drm_pitch * tile_h.
> 
> For any other shape of arranging arranging the tiles like the Z shape
> of NV12MT, the fake pitch meaning would remain unchanged.
> 
> > > > 
> > > > 
> > > > 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.
> > > 
> > > Since everyone is already using the linear pitch approach I'd suggest
> > > sticking with that. Might make life a little easier for generic
> > > userspace (though it still needs to know other details of the tiling
> > > format of course).
> > 
> > Yeah, that's what I meant to add/clarify. The fake linear pitch is, to the
> > best of my knowledge, already the standard we're using in drm.
> > -Daniel
> 
> Everyone except Mali.
> Still, it seems like a good idea to transform something "fake" in
> something with meaning, of course without breaking the userspace
> :). 

Do you mean this is already used like this in drm/malidp code? Or does
"mali" here stand for the blob? Need to know, so we know what we'd break.

As Ville points out in his reply, drm_pitch is pretty much what the
drm_framebuffer pitch is defined as, so we can't change that.
-Daniel

Alexandru-Cosmin Gheorghe Sept. 3, 2018, 8:14 a.m. UTC | #21

On Mon, Sep 03, 2018 at 09:26:24AM +0200, Daniel Vetter wrote:
> On Fri, Aug 31, 2018 at 05:26:37PM +0100, Alexandru-Cosmin Gheorghe wrote:
> > Hi, 
> > 
> > On Fri, Aug 31, 2018 at 05:12:24PM +0200, Daniel Vetter wrote:
> > > On Fri, Aug 31, 2018 at 02:20:31PM +0300, Ville Syrjälä wrote:
> > > > On Fri, Aug 31, 2018 at 10:14:14AM +0200, Daniel Vetter wrote:
> > > > > 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 <[e-mail]>
> > > > > > > > ---
> > > > > > > >  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.
> > 
> > Is more or less mali_pitch, rather than 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.
> > 
> > That's exactly what I'm proposing, for linear tile formats we should
> > use a pitch that naturaly has sense which is drm_pitch * tile_h.
> > 
> > For any other shape of arranging arranging the tiles like the Z shape
> > of NV12MT, the fake pitch meaning would remain unchanged.
> > 
> > > > > 
> > > > > 
> > > > > 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.
> > > > 
> > > > Since everyone is already using the linear pitch approach I'd suggest
> > > > sticking with that. Might make life a little easier for generic
> > > > userspace (though it still needs to know other details of the tiling
> > > > format of course).
> > > 
> > > Yeah, that's what I meant to add/clarify. The fake linear pitch is, to the
> > > best of my knowledge, already the standard we're using in drm.
> > > -Daniel
> > 
> > Everyone except Mali.
> > Still, it seems like a good idea to transform something "fake" in
> > something with meaning, of course without breaking the userspace
> > :). 
> 
> Do you mean this is already used like this in drm/malidp code? Or does
> "mali" here stand for the blob? Need to know, so we know what we'd break.

Blob and a bunch of internal stuff, nothing in the mainline. That's
why I was trying to use the same convention in drm/malidp. 

> 
> As Ville points out in his reply, drm_pitch is pretty much what the
> drm_framebuffer pitch is defined as, so we can't change that.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Patch

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)) {
+			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) {
 			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__ */