diff mbox series

[PATCHv2,1/4] drm/arm: Factor out generic afbc helpers

Message ID 20191104221228.3588-2-andrzej.p@collabora.com (mailing list archive)
State New, archived
Headers show
Series AFBC support for Rockchip | expand

Commit Message

Andrzej Pietrasiewicz Nov. 4, 2019, 10:12 p.m. UTC
These are useful for other users of afbc, e.g. rockchip.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/gpu/drm/Kconfig     |   8 +++
 drivers/gpu/drm/Makefile    |   1 +
 drivers/gpu/drm/arm/Kconfig |   1 +
 drivers/gpu/drm/drm_afbc.c  | 129 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_afbc.h      |  36 ++++++++++
 5 files changed, 175 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_afbc.c
 create mode 100644 include/drm/drm_afbc.h

Comments

Daniel Vetter Nov. 5, 2019, 9:22 a.m. UTC | #1
On Mon, Nov 04, 2019 at 11:12:25PM +0100, Andrzej Pietrasiewicz wrote:
> These are useful for other users of afbc, e.g. rockchip.
> 
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> ---
>  drivers/gpu/drm/Kconfig     |   8 +++
>  drivers/gpu/drm/Makefile    |   1 +
>  drivers/gpu/drm/arm/Kconfig |   1 +
>  drivers/gpu/drm/drm_afbc.c  | 129 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_afbc.h      |  36 ++++++++++
>  5 files changed, 175 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_afbc.c
>  create mode 100644 include/drm/drm_afbc.h
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 36357a36a281..ae1ca5e02bfe 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -205,6 +205,14 @@ config DRM_SCHED
>  	tristate
>  	depends on DRM
>  
> +config DRM_AFBC
> +	tristate
> +	depends on DRM
> +	help
> +	  AFBC is a proprietary lossless image compression protocol and format.
> +	  It provides fine-grained random access and minimizes the amount of
> +	  data transferred between IP blocks.

Uh a module option for 2 functions ... seems like massive overkill. Please
just put that into existing format helpers instead.

What's missing otoh is kernel doc for these.

> +
>  source "drivers/gpu/drm/i2c/Kconfig"
>  
>  source "drivers/gpu/drm/arm/Kconfig"
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 9f1c7c486f88..3a5d092ea514 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -31,6 +31,7 @@ drm-$(CONFIG_OF) += drm_of.o
>  drm-$(CONFIG_AGP) += drm_agpsupport.o
>  drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
>  drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
> +drm-$(CONFIG_DRM_AFBC) += drm_afbc.o
>  
>  drm_vram_helper-y := drm_gem_vram_helper.o \
>  		     drm_vram_helper_common.o
> diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
> index a204103b3efb..25c3dc408cda 100644
> --- a/drivers/gpu/drm/arm/Kconfig
> +++ b/drivers/gpu/drm/arm/Kconfig
> @@ -29,6 +29,7 @@ config DRM_MALI_DISPLAY
>  	select DRM_KMS_HELPER
>  	select DRM_KMS_CMA_HELPER
>  	select DRM_GEM_CMA_HELPER
> +	select DRM_AFBC
>  	select VIDEOMODE_HELPERS
>  	help
>  	  Choose this option if you want to compile the ARM Mali Display
> diff --git a/drivers/gpu/drm/drm_afbc.c b/drivers/gpu/drm/drm_afbc.c
> new file mode 100644
> index 000000000000..010ca9eb0480
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_afbc.c
> @@ -0,0 +1,129 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * (C) 2019 Collabora Ltd.
> + *
> + * author: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> + *
> + */
> +#include <linux/module.h>
> +
> +#include <drm/drm_afbc.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_gem.h>
> +#include <drm/drm_mode.h>
> +#include <drm/drm_print.h>
> +
> +#define AFBC_HEADER_SIZE		16
> +
> +bool drm_afbc_check_offset(struct drm_device *dev,
> +			   const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	if (mode_cmd->offsets[0] != 0) {
> +		DRM_DEBUG_KMS("AFBC buffers' plane offset should be 0\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(drm_afbc_check_offset);
> +
> +bool drm_afbc_check_size_align(struct drm_device *dev,
> +			       const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> +		if ((mode_cmd->width % 16) || (mode_cmd->height % 16)) {
> +			DRM_DEBUG_KMS(
> +				"AFBC buffer must be aligned to 16 pixels\n"
> +			);
> +			return false;
> +		}
> +		break;
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> +		/* fall through */
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4:
> +		/* fall through */
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4:
> +		/* fall through */
> +	default:
> +		DRM_DEBUG_KMS("Unsupported AFBC block size\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(drm_afbc_check_size_align);
> +
> +bool drm_afbc_get_superblk_wh(u64 modifier, u32 *w, u32 *h)
> +{
> +	switch (modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> +		*w = 16;
> +		*h = 16;
> +		break;
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> +		*w = 32;
> +		*h = 8;
> +		break;
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4:
> +		/* fall through */
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4:
> +		/* fall through */
> +	default:
> +		DRM_DEBUG_KMS("Invalid AFBC_FORMAT_MOD_BLOCK_SIZE: %lld.\n",
> +			      modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK);
> +		return false;
> +	}
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(drm_afbc_get_superblk_wh);
> +
> +bool drm_afbc_check_fb_size_ret(u32 pitch, int bpp,
> +				u32 w, u32 h, u32 superblk_w, u32 superblk_h,
> +				size_t size, u32 offset, u32 hdr_align,
> +				u32 *payload_off, u32 *total_size)
> +{
> +	int n_superblks = 0;
> +	u32 superblk_sz = 0;
> +	u32 afbc_size = 0;
> +
> +	n_superblks = (w / superblk_w) * (h / superblk_h);
> +	superblk_sz = (bpp * superblk_w * superblk_h) / BITS_PER_BYTE;
> +	afbc_size = ALIGN(n_superblks * AFBC_HEADER_SIZE, hdr_align);
> +	*payload_off = afbc_size;
> +
> +	afbc_size += n_superblks * ALIGN(superblk_sz, AFBC_SUPERBLK_ALIGNMENT);
> +	*total_size = afbc_size + offset;
> +
> +	if ((w * bpp) != (pitch * BITS_PER_BYTE)) {
> +		DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE) (=%u) should be same as width (=%u) * bpp (=%u)\n",
> +			      pitch * BITS_PER_BYTE, w, bpp
> +		);
> +		return false;
> +	}
> +
> +	if (size < afbc_size) {
> +		DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
> +			      size, afbc_size
> +		);
> +
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL(drm_afbc_check_fb_size_ret);
> +
> +bool drm_afbc_check_fb_size(u32 pitch, int bpp,
> +			    u32 w, u32 h, u32 superblk_w, u32 superblk_h,
> +			    size_t size, u32 offset, u32 hdr_align)
> +{
> +	u32 payload_offset, total_size;
> +
> +	return drm_afbc_check_fb_size_ret(pitch, bpp, w, h,
> +					  superblk_w, superblk_h,
> +					  size, offset, hdr_align,
> +					  &payload_offset, &total_size);
> +}
> +EXPORT_SYMBOL(drm_afbc_check_fb_size);

Why don't we have one overall "check afbc parameters against buffer"
function?

Also would be kinda neat if we could wire up that check code directly into
core addfb2 code, since afbc is a cross driver standard, everyone will
have to use the same code. Doing it that way would also solve your
kerneldoc need (since core code checks it directly). We already have an
example for the tiled nv12 format, which is also checked in core.

Cheers, Daniel

> diff --git a/include/drm/drm_afbc.h b/include/drm/drm_afbc.h
> new file mode 100644
> index 000000000000..b28ae2849f96
> --- /dev/null
> +++ b/include/drm/drm_afbc.h
> @@ -0,0 +1,36 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * (C) 2019 Collabora Ltd.
> + *
> + * author: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> + *
> + */
> +#ifndef __DRM_AFBC_H__
> +#define __DRM_AFBC_H__
> +
> +#include <linux/types.h>
> +
> +struct drm_device;
> +struct drm_mode_fb_cmd2;
> +struct drm_gem_object;
> +
> +#define AFBC_SUPERBLK_ALIGNMENT		128
> +
> +bool drm_afbc_check_offset(struct drm_device *dev,
> +			   const struct drm_mode_fb_cmd2 *mode_cmd);
> +
> +bool drm_afbc_check_size_align(struct drm_device *dev,
> +			       const struct drm_mode_fb_cmd2 *mode_cmd);
> +
> +bool drm_afbc_check_fb_size_ret(u32 pitch, int bpp,
> +				u32 w, u32 h, u32 superblk_w, u32 superblk_h,
> +				size_t size, u32 offset, u32 hdr_align,
> +				u32 *payload_off, u32 *total_size);
> +
> +bool drm_afbc_check_fb_size(u32 pitch, int bpp,
> +			    u32 w, u32 h, u32 superblk_w, u32 superblk_h,
> +			    size_t size, u32 offset, u32 hdr_align);
> +
> +bool drm_afbc_get_superblk_wh(u64 modifier, u32 *w, u32 *h);
> +
> +#endif /* __DRM_AFBC_H__ */
> -- 
> 2.17.1
>
Daniel Stone Nov. 5, 2019, 11:26 p.m. UTC | #2
Hi Andrzej,
Thanks for taking this on! It's looking better than v1 for sure. A few
things below:

On Mon, 2019-11-04 at 23:12 +0100, Andrzej Pietrasiewicz wrote:
> +bool drm_afbc_check_offset(struct drm_device *dev,
> +			   const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	if (mode_cmd->offsets[0] != 0) {
> +		DRM_DEBUG_KMS("AFBC buffers' plane offset should be
> 0\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(drm_afbc_check_offset);

Is this actually universally true? If the offset is sufficiently
aligned for (e.g.) DMA transfers to succeed, is there any reason why it
must be zero?

> +bool drm_afbc_check_size_align(struct drm_device *dev,
> +			       const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	switch (mode_cmd->modifier[0] &
> AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> +		if ((mode_cmd->width % 16) || (mode_cmd->height % 16))
> {

This is a dealbreaker for many resolutions: for example, 1366x768 isn't
cleanly divisible by 16 in width. So this means that we would have to
either use a larger buffer and crop, or scale, or something.

No userspace is prepared to align fb width/height to tile dimensions
like this, so this check will basically fail everywhere.

However, overallocation relative to the declared width/height isn't a
problem at all. In order to deal with horizontal alignment, you simply
need to ensure that the stride is a multiple of the tile width; for
vertical arrangement, that the buffer is large enough to contain
sufficient 'lines' to the next tile boundary.

i.e. rather than checking width/height, you should check:
  * fb_stride >= (ALIGN(fb_width, tile_width), bpp)
  * buf_size >= fb_stride * ALIGN(fb_height, tile_height)

This may force us to do some silly cropping games inside the Rockchip
KMS driver, but I feel it beats the alternative of breaking userspace.

> +			DRM_DEBUG_KMS(
> +				"AFBC buffer must be aligned to 16
> pixels\n"
> +			);
> +			return false;
> +		}
> +		break;
> +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> +		/* fall through */

It's also incongruous that 32x8 is unsupported here, but has a section
in get_superblk_wh; please harmonise them so this section either does
the checks as above, or that get_superblk_wh doesn't support 32x8
either.

> +bool drm_afbc_check_fb_size_ret(u32 pitch, int bpp,
> +				u32 w, u32 h, u32 superblk_w, u32
> superblk_h,
> +				size_t size, u32 offset, u32 hdr_align,
> +				u32 *payload_off, u32 *total_size)
> +{
> +	int n_superblks = 0;
> +	u32 superblk_sz = 0;
> +	u32 afbc_size = 0;

Please don't initialise the above three variables, given that you go on
to immediately change their values. In this case, initialising to zero
can only hide legitimate uninitialised-variable-use warnings.

> +	n_superblks = (w / superblk_w) * (h / superblk_h);
> +	superblk_sz = (bpp * superblk_w * superblk_h) / BITS_PER_BYTE;
> +	afbc_size = ALIGN(n_superblks * AFBC_HEADER_SIZE, hdr_align);
> +	*payload_off = afbc_size;
> +
> +	afbc_size += n_superblks * ALIGN(superblk_sz,
> AFBC_SUPERBLK_ALIGNMENT);
> +	*total_size = afbc_size + offset;

Generally these are referred to as 'tiles' rather than 'superblocks',
given that I would only expect one superblock per buffer, but if that's
the terminology AFBC uses then it might be better to stick with it.

> +	if ((w * bpp) != (pitch * BITS_PER_BYTE)) {
> +		DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE)
> (=%u) should be same as width (=%u) * bpp (=%u)\n",
> +			      pitch * BITS_PER_BYTE, w, bpp
> +		);
> +		return false;
> +	}

Having a too-small pitch is obviously a problem and we should reject
it. But is having a too-large pitch really a problem; does it need to
be an exact match, or can we support the case where the pitch is too
large but also tile-aligned? If we can, it would be very good to
support that.

Cheers,
Daniel
Liviu Dudau Nov. 6, 2019, 10:28 a.m. UTC | #3
Hi Andrzej,

On Tue, Nov 05, 2019 at 11:26:36PM +0000, Daniel Stone wrote:
> Hi Andrzej,
> Thanks for taking this on! It's looking better than v1 for sure. A few
> things below:
> 
> On Mon, 2019-11-04 at 23:12 +0100, Andrzej Pietrasiewicz wrote:
> > +bool drm_afbc_check_offset(struct drm_device *dev,
> > +			   const struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > +	if (mode_cmd->offsets[0] != 0) {
> > +		DRM_DEBUG_KMS("AFBC buffers' plane offset should be
> > 0\n");
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_afbc_check_offset);
> 
> Is this actually universally true? If the offset is sufficiently
> aligned for (e.g.) DMA transfers to succeed, is there any reason why it
> must be zero?
> 
> > +bool drm_afbc_check_size_align(struct drm_device *dev,
> > +			       const struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > +	switch (mode_cmd->modifier[0] &
> > AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > +	case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > +		if ((mode_cmd->width % 16) || (mode_cmd->height % 16))
> > {
> 
> This is a dealbreaker for many resolutions: for example, 1366x768 isn't
> cleanly divisible by 16 in width. So this means that we would have to
> either use a larger buffer and crop, or scale, or something.
> 
> No userspace is prepared to align fb width/height to tile dimensions
> like this, so this check will basically fail everywhere.

I agree with Daniel, for AFBC_FORMAT_MOD_BLOCK_SIZE_xxxx you need to check that the
allocated framebuffer's width and height are divisible by block size, not what the
resolution of the mode is.

Best regards,
Liviu

> 
> However, overallocation relative to the declared width/height isn't a
> problem at all. In order to deal with horizontal alignment, you simply
> need to ensure that the stride is a multiple of the tile width; for
> vertical arrangement, that the buffer is large enough to contain
> sufficient 'lines' to the next tile boundary.
> 
> i.e. rather than checking width/height, you should check:
>   * fb_stride >= (ALIGN(fb_width, tile_width), bpp)
>   * buf_size >= fb_stride * ALIGN(fb_height, tile_height)
> 
> This may force us to do some silly cropping games inside the Rockchip
> KMS driver, but I feel it beats the alternative of breaking userspace.
> 
> > +			DRM_DEBUG_KMS(
> > +				"AFBC buffer must be aligned to 16
> > pixels\n"
> > +			);
> > +			return false;
> > +		}
> > +		break;
> > +	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> > +		/* fall through */
> 
> It's also incongruous that 32x8 is unsupported here, but has a section
> in get_superblk_wh; please harmonise them so this section either does
> the checks as above, or that get_superblk_wh doesn't support 32x8
> either.
> 
> > +bool drm_afbc_check_fb_size_ret(u32 pitch, int bpp,
> > +				u32 w, u32 h, u32 superblk_w, u32
> > superblk_h,
> > +				size_t size, u32 offset, u32 hdr_align,
> > +				u32 *payload_off, u32 *total_size)
> > +{
> > +	int n_superblks = 0;
> > +	u32 superblk_sz = 0;
> > +	u32 afbc_size = 0;
> 
> Please don't initialise the above three variables, given that you go on
> to immediately change their values. In this case, initialising to zero
> can only hide legitimate uninitialised-variable-use warnings.
> 
> > +	n_superblks = (w / superblk_w) * (h / superblk_h);
> > +	superblk_sz = (bpp * superblk_w * superblk_h) / BITS_PER_BYTE;
> > +	afbc_size = ALIGN(n_superblks * AFBC_HEADER_SIZE, hdr_align);
> > +	*payload_off = afbc_size;
> > +
> > +	afbc_size += n_superblks * ALIGN(superblk_sz,
> > AFBC_SUPERBLK_ALIGNMENT);
> > +	*total_size = afbc_size + offset;
> 
> Generally these are referred to as 'tiles' rather than 'superblocks',
> given that I would only expect one superblock per buffer, but if that's
> the terminology AFBC uses then it might be better to stick with it.
> 
> > +	if ((w * bpp) != (pitch * BITS_PER_BYTE)) {
> > +		DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE)
> > (=%u) should be same as width (=%u) * bpp (=%u)\n",
> > +			      pitch * BITS_PER_BYTE, w, bpp
> > +		);
> > +		return false;
> > +	}
> 
> Having a too-small pitch is obviously a problem and we should reject
> it. But is having a too-large pitch really a problem; does it need to
> be an exact match, or can we support the case where the pitch is too
> large but also tile-aligned? If we can, it would be very good to
> support that.
> 
> Cheers,
> Daniel
>
Andrzej Pietrasiewicz Nov. 6, 2019, 12:45 p.m. UTC | #4
Hi Daniel,

Thank you for review,

W dniu 05.11.2019 o 10:22, Daniel Vetter pisze:
> On Mon, Nov 04, 2019 at 11:12:25PM +0100, Andrzej Pietrasiewicz wrote:
>> These are useful for other users of afbc, e.g. rockchip.
>>

<snip>

>> +
>> +bool drm_afbc_check_fb_size_ret(u32 pitch, int bpp,
>> +				u32 w, u32 h, u32 superblk_w, u32 superblk_h,
>> +				size_t size, u32 offset, u32 hdr_align,
>> +				u32 *payload_off, u32 *total_size)
>> +{
>> +	int n_superblks = 0;
>> +	u32 superblk_sz = 0;
>> +	u32 afbc_size = 0;
>> +
>> +	n_superblks = (w / superblk_w) * (h / superblk_h);
>> +	superblk_sz = (bpp * superblk_w * superblk_h) / BITS_PER_BYTE;
>> +	afbc_size = ALIGN(n_superblks * AFBC_HEADER_SIZE, hdr_align);
>> +	*payload_off = afbc_size;
>> +
>> +	afbc_size += n_superblks * ALIGN(superblk_sz, AFBC_SUPERBLK_ALIGNMENT);
>> +	*total_size = afbc_size + offset;
>> +
>> +	if ((w * bpp) != (pitch * BITS_PER_BYTE)) {
>> +		DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE) (=%u) should be same as width (=%u) * bpp (=%u)\n",
>> +			      pitch * BITS_PER_BYTE, w, bpp
>> +		);
>> +		return false;
>> +	}
>> +
>> +	if (size < afbc_size) {
>> +		DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
>> +			      size, afbc_size
>> +		);
>> +
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +EXPORT_SYMBOL(drm_afbc_check_fb_size_ret);
>> +
>> +bool drm_afbc_check_fb_size(u32 pitch, int bpp,
>> +			    u32 w, u32 h, u32 superblk_w, u32 superblk_h,
>> +			    size_t size, u32 offset, u32 hdr_align)
>> +{
>> +	u32 payload_offset, total_size;
>> +
>> +	return drm_afbc_check_fb_size_ret(pitch, bpp, w, h,
>> +					  superblk_w, superblk_h,
>> +					  size, offset, hdr_align,
>> +					  &payload_offset, &total_size);
>> +}
>> +EXPORT_SYMBOL(drm_afbc_check_fb_size);
> 
> Why don't we have one overall "check afbc parameters against buffer"
> function?
> 

I noticed that different drivers have different needs (malidp
and rockchip are kind of similar, but komeda is a bit different).
That is why the helpers are only building blocks out of which
each driver builds its own checking logic. In particular komeda
wants some by-products of the check stored in its internal data
structures, hence drm_afbc_check_fb_size_ret() and its wrapper
drm_afbc_check_fb_size() which ignores the "out" parameters.

If I wanted to create one overall "check afbc parameters" I'd have
to come up with a way to pass driver-specific requirements to it
and then inside the function have some logic to decide what to
check against what. Do you think it is worth it?

Andrzej
Daniel Vetter Nov. 7, 2019, 8:27 a.m. UTC | #5
On Wed, Nov 06, 2019 at 01:45:05PM +0100, Andrzej Pietrasiewicz wrote:
> Hi Daniel,
> 
> Thank you for review,
> 
> W dniu 05.11.2019 o 10:22, Daniel Vetter pisze:
> > On Mon, Nov 04, 2019 at 11:12:25PM +0100, Andrzej Pietrasiewicz wrote:
> > > These are useful for other users of afbc, e.g. rockchip.
> > > 
> 
> <snip>
> 
> > > +
> > > +bool drm_afbc_check_fb_size_ret(u32 pitch, int bpp,
> > > +				u32 w, u32 h, u32 superblk_w, u32 superblk_h,
> > > +				size_t size, u32 offset, u32 hdr_align,
> > > +				u32 *payload_off, u32 *total_size)
> > > +{
> > > +	int n_superblks = 0;
> > > +	u32 superblk_sz = 0;
> > > +	u32 afbc_size = 0;
> > > +
> > > +	n_superblks = (w / superblk_w) * (h / superblk_h);
> > > +	superblk_sz = (bpp * superblk_w * superblk_h) / BITS_PER_BYTE;
> > > +	afbc_size = ALIGN(n_superblks * AFBC_HEADER_SIZE, hdr_align);
> > > +	*payload_off = afbc_size;
> > > +
> > > +	afbc_size += n_superblks * ALIGN(superblk_sz, AFBC_SUPERBLK_ALIGNMENT);
> > > +	*total_size = afbc_size + offset;
> > > +
> > > +	if ((w * bpp) != (pitch * BITS_PER_BYTE)) {
> > > +		DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE) (=%u) should be same as width (=%u) * bpp (=%u)\n",
> > > +			      pitch * BITS_PER_BYTE, w, bpp
> > > +		);
> > > +		return false;
> > > +	}
> > > +
> > > +	if (size < afbc_size) {
> > > +		DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
> > > +			      size, afbc_size
> > > +		);
> > > +
> > > +		return false;
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +EXPORT_SYMBOL(drm_afbc_check_fb_size_ret);
> > > +
> > > +bool drm_afbc_check_fb_size(u32 pitch, int bpp,
> > > +			    u32 w, u32 h, u32 superblk_w, u32 superblk_h,
> > > +			    size_t size, u32 offset, u32 hdr_align)
> > > +{
> > > +	u32 payload_offset, total_size;
> > > +
> > > +	return drm_afbc_check_fb_size_ret(pitch, bpp, w, h,
> > > +					  superblk_w, superblk_h,
> > > +					  size, offset, hdr_align,
> > > +					  &payload_offset, &total_size);
> > > +}
> > > +EXPORT_SYMBOL(drm_afbc_check_fb_size);
> > 
> > Why don't we have one overall "check afbc parameters against buffer"
> > function?
> > 
> 
> I noticed that different drivers have different needs (malidp
> and rockchip are kind of similar, but komeda is a bit different).
> That is why the helpers are only building blocks out of which
> each driver builds its own checking logic. In particular komeda
> wants some by-products of the check stored in its internal data
> structures, hence drm_afbc_check_fb_size_ret() and its wrapper
> drm_afbc_check_fb_size() which ignores the "out" parameters.
> 
> If I wanted to create one overall "check afbc parameters" I'd have
> to come up with a way to pass driver-specific requirements to it
> and then inside the function have some logic to decide what to
> check against what. Do you think it is worth it?

Hm I figured there's at least two parts of this:
- Generic checking of afbc against the fb parameters, i.e. is it big
  enough, correctly aligned, all that.
- Additional driver checks, which might need some of the same parameters
  again.

The idea behind asking for the first part is that maybe we should put that
into the core as part of the addfb checks (like we do for the tiled NV12
thing already). Then drivers only need to do additional checks on top for
their specific constraints. For that you could expose some helpers ofc (to
get the payload_offset and anything else computed you might need).

But the basic drm_afbc_check_fb_size() should imo be done unconditionally
for any afbc modifier. To make sure we don't have lots of drivers with
different bugs in that thing.
-Daniel
Brian Starkey Nov. 7, 2019, 5:20 p.m. UTC | #6
Hi Daniel,

On Tue, Nov 05, 2019 at 11:26:36PM +0000, Daniel Stone wrote:
> Hi Andrzej,
> Thanks for taking this on! It's looking better than v1 for sure. A few
> things below:
>
> On Mon, 2019-11-04 at 23:12 +0100, Andrzej Pietrasiewicz wrote:
> > +bool drm_afbc_check_offset(struct drm_device *dev,
> > +                      const struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > +   if (mode_cmd->offsets[0] != 0) {
> > +           DRM_DEBUG_KMS("AFBC buffers' plane offset should be
> > 0\n");
> > +           return false;
> > +   }
> > +
> > +   return true;
> > +}
> > +EXPORT_SYMBOL_GPL(drm_afbc_check_offset);
>
> Is this actually universally true? If the offset is sufficiently
> aligned for (e.g.) DMA transfers to succeed, is there any reason why it
> must be zero?
>
> > +bool drm_afbc_check_size_align(struct drm_device *dev,
> > +                          const struct drm_mode_fb_cmd2 *mode_cmd)
> > +{
> > +   switch (mode_cmd->modifier[0] &
> > AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > +   case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > +           if ((mode_cmd->width % 16) || (mode_cmd->height % 16))
> > {
>
> This is a dealbreaker for many resolutions: for example, 1366x768 isn't
> cleanly divisible by 16 in width. So this means that we would have to
> either use a larger buffer and crop, or scale, or something.
>
> No userspace is prepared to align fb width/height to tile dimensions
> like this, so this check will basically fail everywhere.
>
> However, overallocation relative to the declared width/height isn't a
> problem at all. In order to deal with horizontal alignment, you simply
> need to ensure that the stride is a multiple of the tile width; for
> vertical arrangement, that the buffer is large enough to contain
> sufficient 'lines' to the next tile boundary.
>
> i.e. rather than checking width/height, you should check:
>   * fb_stride >= (ALIGN(fb_width, tile_width), bpp)
>   * buf_size >= fb_stride * ALIGN(fb_height, tile_height)

Well, sort of.

I agree with you that for horizontal padding, we can indeed use pitch.

However, the AFBC decoder(s) need to know exactly what the total
_allocated_ size in pixels of the buffer is - as this influences the
header size, and we need to know the header size to know where it ends
and the body begins.

I see a couple of possible ways forwards:

 - Keep it as-is. The restrictive checks ensure that there's no
   ambiguity and we use the fb width/height to determine the real
   allocated width/height. Userspace needs to be AFBC-aware and set up
   plane cropping to handle the alignment differences.

 - Use pitch to determine the "real" width, and internally in the
   kernel align height up to the next alignment boundary. This works
   OK, so long as there's no additional padding at the bottom of the
   buffer. This would work, but I can't figure a way to check/enforce
   that there's no additional padding at the bottom.

 - Something else...

The checks as-implemented were deliberately conservative, and don't
preclude doing some relaxation in the future.

On Android, gralloc is used to store the "real" allocated width/height
and this is used to set up the DRM API appropriately.

>
> This may force us to do some silly cropping games inside the Rockchip
> KMS driver, but I feel it beats the alternative of breaking userspace.

Well, nothing's going to get broken - it's just perhaps not ready to
turn on AFBC yet.

>
> > +                   DRM_DEBUG_KMS(
> > +                           "AFBC buffer must be aligned to 16
> > pixels\n"
> > +                   );
> > +                   return false;
> > +           }
> > +           break;
> > +   case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> > +           /* fall through */
>
> It's also incongruous that 32x8 is unsupported here, but has a section
> in get_superblk_wh; please harmonise them so this section either does
> the checks as above, or that get_superblk_wh doesn't support 32x8
> either.
>
> > +bool drm_afbc_check_fb_size_ret(u32 pitch, int bpp,
> > +                           u32 w, u32 h, u32 superblk_w, u32
> > superblk_h,
> > +                           size_t size, u32 offset, u32 hdr_align,
> > +                           u32 *payload_off, u32 *total_size)
> > +{
> > +   int n_superblks = 0;
> > +   u32 superblk_sz = 0;
> > +   u32 afbc_size = 0;
>
> Please don't initialise the above three variables, given that you go on
> to immediately change their values. In this case, initialising to zero
> can only hide legitimate uninitialised-variable-use warnings.
>
> > +   n_superblks = (w / superblk_w) * (h / superblk_h);
> > +   superblk_sz = (bpp * superblk_w * superblk_h) / BITS_PER_BYTE;
> > +   afbc_size = ALIGN(n_superblks * AFBC_HEADER_SIZE, hdr_align);
> > +   *payload_off = afbc_size;
> > +
> > +   afbc_size += n_superblks * ALIGN(superblk_sz,
> > AFBC_SUPERBLK_ALIGNMENT);
> > +   *total_size = afbc_size + offset;
>
> Generally these are referred to as 'tiles' rather than 'superblocks',
> given that I would only expect one superblock per buffer, but if that's
> the terminology AFBC uses then it might be better to stick with it.
>
> > +   if ((w * bpp) != (pitch * BITS_PER_BYTE)) {
> > +           DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE)
> > (=%u) should be same as width (=%u) * bpp (=%u)\n",
> > +                         pitch * BITS_PER_BYTE, w, bpp
> > +           );
> > +           return false;
> > +   }
>
> Having a too-small pitch is obviously a problem and we should reject
> it. But is having a too-large pitch really a problem; does it need to
> be an exact match, or can we support the case where the pitch is too
> large but also tile-aligned? If we can, it would be very good to
> support that.

The reason for forcing it to be exact is as I said above - we _must_
know what the "real" width and height is. Implementing this check to
force (pitch == width * bpp) ensures that, and also leaves the option
for us to relax to allow a larger pitch (as above) if that was the
preferred approach for alignment.

In general the current checks are deliberately designed to leave the
door open for future improvements without breaking anything.

Cheers,
-Brian

>
> Cheers,
> Daniel
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Daniel Vetter Nov. 7, 2019, 5:32 p.m. UTC | #7
On Thu, Nov 7, 2019 at 6:20 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
>
> Hi Daniel,
>
> On Tue, Nov 05, 2019 at 11:26:36PM +0000, Daniel Stone wrote:
> > Hi Andrzej,
> > Thanks for taking this on! It's looking better than v1 for sure. A few
> > things below:
> >
> > On Mon, 2019-11-04 at 23:12 +0100, Andrzej Pietrasiewicz wrote:
> > > +bool drm_afbc_check_offset(struct drm_device *dev,
> > > +                      const struct drm_mode_fb_cmd2 *mode_cmd)
> > > +{
> > > +   if (mode_cmd->offsets[0] != 0) {
> > > +           DRM_DEBUG_KMS("AFBC buffers' plane offset should be
> > > 0\n");
> > > +           return false;
> > > +   }
> > > +
> > > +   return true;
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_afbc_check_offset);
> >
> > Is this actually universally true? If the offset is sufficiently
> > aligned for (e.g.) DMA transfers to succeed, is there any reason why it
> > must be zero?
> >
> > > +bool drm_afbc_check_size_align(struct drm_device *dev,
> > > +                          const struct drm_mode_fb_cmd2 *mode_cmd)
> > > +{
> > > +   switch (mode_cmd->modifier[0] &
> > > AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > > +   case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > > +           if ((mode_cmd->width % 16) || (mode_cmd->height % 16))
> > > {
> >
> > This is a dealbreaker for many resolutions: for example, 1366x768 isn't
> > cleanly divisible by 16 in width. So this means that we would have to
> > either use a larger buffer and crop, or scale, or something.
> >
> > No userspace is prepared to align fb width/height to tile dimensions
> > like this, so this check will basically fail everywhere.
> >
> > However, overallocation relative to the declared width/height isn't a
> > problem at all. In order to deal with horizontal alignment, you simply
> > need to ensure that the stride is a multiple of the tile width; for
> > vertical arrangement, that the buffer is large enough to contain
> > sufficient 'lines' to the next tile boundary.
> >
> > i.e. rather than checking width/height, you should check:
> >   * fb_stride >= (ALIGN(fb_width, tile_width), bpp)
> >   * buf_size >= fb_stride * ALIGN(fb_height, tile_height)
>
> Well, sort of.
>
> I agree with you that for horizontal padding, we can indeed use pitch.
>
> However, the AFBC decoder(s) need to know exactly what the total
> _allocated_ size in pixels of the buffer is - as this influences the
> header size, and we need to know the header size to know where it ends
> and the body begins.
>
> I see a couple of possible ways forwards:
>
>  - Keep it as-is. The restrictive checks ensure that there's no
>    ambiguity and we use the fb width/height to determine the real
>    allocated width/height. Userspace needs to be AFBC-aware and set up
>    plane cropping to handle the alignment differences.
>
>  - Use pitch to determine the "real" width, and internally in the
>    kernel align height up to the next alignment boundary. This works
>    OK, so long as there's no additional padding at the bottom of the
>    buffer. This would work, but I can't figure a way to check/enforce
>    that there's no additional padding at the bottom.
>
>  - Something else...
>
> The checks as-implemented were deliberately conservative, and don't
> preclude doing some relaxation in the future.
>
> On Android, gralloc is used to store the "real" allocated width/height
> and this is used to set up the DRM API appropriately.

Fake stride + real visible h/w in the drmfb. Because that's how it
works with all the tiled formats already, and expecting userspace to
fudge this all correctly seems very backwards to me. In a way we had
that entire fake stride discussion already for the block size format
stuff already, but now in a different flavour.

Also I think that's more reasons why this should be no-opt-outable
code that's done for all drivers when we check framebuffers in addfb.
Plus then some helpers to get at computed values for any framebuffer
we know to be valid.
-Daniel

> > This may force us to do some silly cropping games inside the Rockchip
> > KMS driver, but I feel it beats the alternative of breaking userspace.
>
> Well, nothing's going to get broken - it's just perhaps not ready to
> turn on AFBC yet.
>
> >
> > > +                   DRM_DEBUG_KMS(
> > > +                           "AFBC buffer must be aligned to 16
> > > pixels\n"
> > > +                   );
> > > +                   return false;
> > > +           }
> > > +           break;
> > > +   case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> > > +           /* fall through */
> >
> > It's also incongruous that 32x8 is unsupported here, but has a section
> > in get_superblk_wh; please harmonise them so this section either does
> > the checks as above, or that get_superblk_wh doesn't support 32x8
> > either.
> >
> > > +bool drm_afbc_check_fb_size_ret(u32 pitch, int bpp,
> > > +                           u32 w, u32 h, u32 superblk_w, u32
> > > superblk_h,
> > > +                           size_t size, u32 offset, u32 hdr_align,
> > > +                           u32 *payload_off, u32 *total_size)
> > > +{
> > > +   int n_superblks = 0;
> > > +   u32 superblk_sz = 0;
> > > +   u32 afbc_size = 0;
> >
> > Please don't initialise the above three variables, given that you go on
> > to immediately change their values. In this case, initialising to zero
> > can only hide legitimate uninitialised-variable-use warnings.
> >
> > > +   n_superblks = (w / superblk_w) * (h / superblk_h);
> > > +   superblk_sz = (bpp * superblk_w * superblk_h) / BITS_PER_BYTE;
> > > +   afbc_size = ALIGN(n_superblks * AFBC_HEADER_SIZE, hdr_align);
> > > +   *payload_off = afbc_size;
> > > +
> > > +   afbc_size += n_superblks * ALIGN(superblk_sz,
> > > AFBC_SUPERBLK_ALIGNMENT);
> > > +   *total_size = afbc_size + offset;
> >
> > Generally these are referred to as 'tiles' rather than 'superblocks',
> > given that I would only expect one superblock per buffer, but if that's
> > the terminology AFBC uses then it might be better to stick with it.
> >
> > > +   if ((w * bpp) != (pitch * BITS_PER_BYTE)) {
> > > +           DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE)
> > > (=%u) should be same as width (=%u) * bpp (=%u)\n",
> > > +                         pitch * BITS_PER_BYTE, w, bpp
> > > +           );
> > > +           return false;
> > > +   }
> >
> > Having a too-small pitch is obviously a problem and we should reject
> > it. But is having a too-large pitch really a problem; does it need to
> > be an exact match, or can we support the case where the pitch is too
> > large but also tile-aligned? If we can, it would be very good to
> > support that.
>
> The reason for forcing it to be exact is as I said above - we _must_
> know what the "real" width and height is. Implementing this check to
> force (pitch == width * bpp) ensures that, and also leaves the option
> for us to relax to allow a larger pitch (as above) if that was the
> preferred approach for alignment.
>
> In general the current checks are deliberately designed to leave the
> door open for future improvements without breaking anything.
>
> Cheers,
> -Brian
>
> >
> > Cheers,
> > Daniel
> >
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Brian Starkey Nov. 7, 2019, 5:49 p.m. UTC | #8
Hi Daniel,

On Thu, Nov 07, 2019 at 06:32:01PM +0100, Daniel Vetter wrote:
> On Thu, Nov 7, 2019 at 6:20 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
> >
> > Hi Daniel,
> >
> > On Tue, Nov 05, 2019 at 11:26:36PM +0000, Daniel Stone wrote:
> > > Hi Andrzej,
> > > Thanks for taking this on! It's looking better than v1 for sure. A few
> > > things below:
> > >
> > > On Mon, 2019-11-04 at 23:12 +0100, Andrzej Pietrasiewicz wrote:
> > > > +bool drm_afbc_check_offset(struct drm_device *dev,
> > > > +                      const struct drm_mode_fb_cmd2 *mode_cmd)
> > > > +{
> > > > +   if (mode_cmd->offsets[0] != 0) {
> > > > +           DRM_DEBUG_KMS("AFBC buffers' plane offset should be
> > > > 0\n");
> > > > +           return false;
> > > > +   }
> > > > +
> > > > +   return true;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(drm_afbc_check_offset);
> > >
> > > Is this actually universally true? If the offset is sufficiently
> > > aligned for (e.g.) DMA transfers to succeed, is there any reason why it
> > > must be zero?
> > >
> > > > +bool drm_afbc_check_size_align(struct drm_device *dev,
> > > > +                          const struct drm_mode_fb_cmd2 *mode_cmd)
> > > > +{
> > > > +   switch (mode_cmd->modifier[0] &
> > > > AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > > > +   case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > > > +           if ((mode_cmd->width % 16) || (mode_cmd->height % 16))
> > > > {
> > >
> > > This is a dealbreaker for many resolutions: for example, 1366x768 isn't
> > > cleanly divisible by 16 in width. So this means that we would have to
> > > either use a larger buffer and crop, or scale, or something.
> > >
> > > No userspace is prepared to align fb width/height to tile dimensions
> > > like this, so this check will basically fail everywhere.
> > >
> > > However, overallocation relative to the declared width/height isn't a
> > > problem at all. In order to deal with horizontal alignment, you simply
> > > need to ensure that the stride is a multiple of the tile width; for
> > > vertical arrangement, that the buffer is large enough to contain
> > > sufficient 'lines' to the next tile boundary.
> > >
> > > i.e. rather than checking width/height, you should check:
> > >   * fb_stride >= (ALIGN(fb_width, tile_width), bpp)
> > >   * buf_size >= fb_stride * ALIGN(fb_height, tile_height)
> >
> > Well, sort of.
> >
> > I agree with you that for horizontal padding, we can indeed use pitch.
> >
> > However, the AFBC decoder(s) need to know exactly what the total
> > _allocated_ size in pixels of the buffer is - as this influences the
> > header size, and we need to know the header size to know where it ends
> > and the body begins.
> >
> > I see a couple of possible ways forwards:
> >
> >  - Keep it as-is. The restrictive checks ensure that there's no
> >    ambiguity and we use the fb width/height to determine the real
> >    allocated width/height. Userspace needs to be AFBC-aware and set up
> >    plane cropping to handle the alignment differences.
> >
> >  - Use pitch to determine the "real" width, and internally in the
> >    kernel align height up to the next alignment boundary. This works
> >    OK, so long as there's no additional padding at the bottom of the
> >    buffer. This would work, but I can't figure a way to check/enforce
> >    that there's no additional padding at the bottom.
> >
> >  - Something else...
> >
> > The checks as-implemented were deliberately conservative, and don't
> > preclude doing some relaxation in the future.
> >
> > On Android, gralloc is used to store the "real" allocated width/height
> > and this is used to set up the DRM API appropriately.
> 
> Fake stride + real visible h/w in the drmfb. Because that's how it
> works with all the tiled formats already, and expecting userspace to
> fudge this all correctly seems very backwards to me. In a way we had
> that entire fake stride discussion already for the block size format
> stuff already, but now in a different flavour.

Fake stride - like I said, no problem; sounds good. That solves one
dimension.

So do you have a proposal for how we determine what the allocated
height is in that case? I don't really see a way.

Thanks,
-Brian

> 
> Also I think that's more reasons why this should be no-opt-outable
> code that's done for all drivers when we check framebuffers in addfb.
> Plus then some helpers to get at computed values for any framebuffer
> we know to be valid.
> -Daniel
> 
> > > This may force us to do some silly cropping games inside the Rockchip
> > > KMS driver, but I feel it beats the alternative of breaking userspace.
> >
> > Well, nothing's going to get broken - it's just perhaps not ready to
> > turn on AFBC yet.
> >
> > >
> > > > +                   DRM_DEBUG_KMS(
> > > > +                           "AFBC buffer must be aligned to 16
> > > > pixels\n"
> > > > +                   );
> > > > +                   return false;
> > > > +           }
> > > > +           break;
> > > > +   case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> > > > +           /* fall through */
> > >
> > > It's also incongruous that 32x8 is unsupported here, but has a section
> > > in get_superblk_wh; please harmonise them so this section either does
> > > the checks as above, or that get_superblk_wh doesn't support 32x8
> > > either.
> > >
> > > > +bool drm_afbc_check_fb_size_ret(u32 pitch, int bpp,
> > > > +                           u32 w, u32 h, u32 superblk_w, u32
> > > > superblk_h,
> > > > +                           size_t size, u32 offset, u32 hdr_align,
> > > > +                           u32 *payload_off, u32 *total_size)
> > > > +{
> > > > +   int n_superblks = 0;
> > > > +   u32 superblk_sz = 0;
> > > > +   u32 afbc_size = 0;
> > >
> > > Please don't initialise the above three variables, given that you go on
> > > to immediately change their values. In this case, initialising to zero
> > > can only hide legitimate uninitialised-variable-use warnings.
> > >
> > > > +   n_superblks = (w / superblk_w) * (h / superblk_h);
> > > > +   superblk_sz = (bpp * superblk_w * superblk_h) / BITS_PER_BYTE;
> > > > +   afbc_size = ALIGN(n_superblks * AFBC_HEADER_SIZE, hdr_align);
> > > > +   *payload_off = afbc_size;
> > > > +
> > > > +   afbc_size += n_superblks * ALIGN(superblk_sz,
> > > > AFBC_SUPERBLK_ALIGNMENT);
> > > > +   *total_size = afbc_size + offset;
> > >
> > > Generally these are referred to as 'tiles' rather than 'superblocks',
> > > given that I would only expect one superblock per buffer, but if that's
> > > the terminology AFBC uses then it might be better to stick with it.
> > >
> > > > +   if ((w * bpp) != (pitch * BITS_PER_BYTE)) {
> > > > +           DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE)
> > > > (=%u) should be same as width (=%u) * bpp (=%u)\n",
> > > > +                         pitch * BITS_PER_BYTE, w, bpp
> > > > +           );
> > > > +           return false;
> > > > +   }
> > >
> > > Having a too-small pitch is obviously a problem and we should reject
> > > it. But is having a too-large pitch really a problem; does it need to
> > > be an exact match, or can we support the case where the pitch is too
> > > large but also tile-aligned? If we can, it would be very good to
> > > support that.
> >
> > The reason for forcing it to be exact is as I said above - we _must_
> > know what the "real" width and height is. Implementing this check to
> > force (pitch == width * bpp) ensures that, and also leaves the option
> > for us to relax to allow a larger pitch (as above) if that was the
> > preferred approach for alignment.
> >
> > In general the current checks are deliberately designed to leave the
> > door open for future improvements without breaking anything.
> >
> > Cheers,
> > -Brian
> >
> > >
> > > Cheers,
> > > Daniel
> > >
> > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Not sure how that snuck in.

> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Daniel Vetter Nov. 7, 2019, 7:28 p.m. UTC | #9
On Thu, Nov 07, 2019 at 05:49:14PM +0000, Brian Starkey wrote:
> Hi Daniel,
> 
> On Thu, Nov 07, 2019 at 06:32:01PM +0100, Daniel Vetter wrote:
> > On Thu, Nov 7, 2019 at 6:20 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
> > >
> > > Hi Daniel,
> > >
> > > On Tue, Nov 05, 2019 at 11:26:36PM +0000, Daniel Stone wrote:
> > > > Hi Andrzej,
> > > > Thanks for taking this on! It's looking better than v1 for sure. A few
> > > > things below:
> > > >
> > > > On Mon, 2019-11-04 at 23:12 +0100, Andrzej Pietrasiewicz wrote:
> > > > > +bool drm_afbc_check_offset(struct drm_device *dev,
> > > > > +                      const struct drm_mode_fb_cmd2 *mode_cmd)
> > > > > +{
> > > > > +   if (mode_cmd->offsets[0] != 0) {
> > > > > +           DRM_DEBUG_KMS("AFBC buffers' plane offset should be
> > > > > 0\n");
> > > > > +           return false;
> > > > > +   }
> > > > > +
> > > > > +   return true;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(drm_afbc_check_offset);
> > > >
> > > > Is this actually universally true? If the offset is sufficiently
> > > > aligned for (e.g.) DMA transfers to succeed, is there any reason why it
> > > > must be zero?
> > > >
> > > > > +bool drm_afbc_check_size_align(struct drm_device *dev,
> > > > > +                          const struct drm_mode_fb_cmd2 *mode_cmd)
> > > > > +{
> > > > > +   switch (mode_cmd->modifier[0] &
> > > > > AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > > > > +   case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > > > > +           if ((mode_cmd->width % 16) || (mode_cmd->height % 16))
> > > > > {
> > > >
> > > > This is a dealbreaker for many resolutions: for example, 1366x768 isn't
> > > > cleanly divisible by 16 in width. So this means that we would have to
> > > > either use a larger buffer and crop, or scale, or something.
> > > >
> > > > No userspace is prepared to align fb width/height to tile dimensions
> > > > like this, so this check will basically fail everywhere.
> > > >
> > > > However, overallocation relative to the declared width/height isn't a
> > > > problem at all. In order to deal with horizontal alignment, you simply
> > > > need to ensure that the stride is a multiple of the tile width; for
> > > > vertical arrangement, that the buffer is large enough to contain
> > > > sufficient 'lines' to the next tile boundary.
> > > >
> > > > i.e. rather than checking width/height, you should check:
> > > >   * fb_stride >= (ALIGN(fb_width, tile_width), bpp)
> > > >   * buf_size >= fb_stride * ALIGN(fb_height, tile_height)
> > >
> > > Well, sort of.
> > >
> > > I agree with you that for horizontal padding, we can indeed use pitch.
> > >
> > > However, the AFBC decoder(s) need to know exactly what the total
> > > _allocated_ size in pixels of the buffer is - as this influences the
> > > header size, and we need to know the header size to know where it ends
> > > and the body begins.
> > >
> > > I see a couple of possible ways forwards:
> > >
> > >  - Keep it as-is. The restrictive checks ensure that there's no
> > >    ambiguity and we use the fb width/height to determine the real
> > >    allocated width/height. Userspace needs to be AFBC-aware and set up
> > >    plane cropping to handle the alignment differences.
> > >
> > >  - Use pitch to determine the "real" width, and internally in the
> > >    kernel align height up to the next alignment boundary. This works
> > >    OK, so long as there's no additional padding at the bottom of the
> > >    buffer. This would work, but I can't figure a way to check/enforce
> > >    that there's no additional padding at the bottom.
> > >
> > >  - Something else...
> > >
> > > The checks as-implemented were deliberately conservative, and don't
> > > preclude doing some relaxation in the future.
> > >
> > > On Android, gralloc is used to store the "real" allocated width/height
> > > and this is used to set up the DRM API appropriately.
> > 
> > Fake stride + real visible h/w in the drmfb. Because that's how it
> > works with all the tiled formats already, and expecting userspace to
> > fudge this all correctly seems very backwards to me. In a way we had
> > that entire fake stride discussion already for the block size format
> > stuff already, but now in a different flavour.
> 
> Fake stride - like I said, no problem; sounds good. That solves one
> dimension.
> 
> So do you have a proposal for how we determine what the allocated
> height is in that case? I don't really see a way.

Could you compute the height by looking at the buffer size? Or does that
not help since the header stuff is generally rather small?

Otherwise I guess just round up height and hope it works. If we run into a
use-case where that doesn't work anymore somehow, then we get to rev all
the afbc modifiers and make them 2 planes. With that there's no such issue
anymore (which is why the intel compressed stuff has 2 planes).
-Daniel


> Thanks,
> -Brian
> 
> > 
> > Also I think that's more reasons why this should be no-opt-outable
> > code that's done for all drivers when we check framebuffers in addfb.
> > Plus then some helpers to get at computed values for any framebuffer
> > we know to be valid.
> > -Daniel
> > 
> > > > This may force us to do some silly cropping games inside the Rockchip
> > > > KMS driver, but I feel it beats the alternative of breaking userspace.
> > >
> > > Well, nothing's going to get broken - it's just perhaps not ready to
> > > turn on AFBC yet.
> > >
> > > >
> > > > > +                   DRM_DEBUG_KMS(
> > > > > +                           "AFBC buffer must be aligned to 16
> > > > > pixels\n"
> > > > > +                   );
> > > > > +                   return false;
> > > > > +           }
> > > > > +           break;
> > > > > +   case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> > > > > +           /* fall through */
> > > >
> > > > It's also incongruous that 32x8 is unsupported here, but has a section
> > > > in get_superblk_wh; please harmonise them so this section either does
> > > > the checks as above, or that get_superblk_wh doesn't support 32x8
> > > > either.
> > > >
> > > > > +bool drm_afbc_check_fb_size_ret(u32 pitch, int bpp,
> > > > > +                           u32 w, u32 h, u32 superblk_w, u32
> > > > > superblk_h,
> > > > > +                           size_t size, u32 offset, u32 hdr_align,
> > > > > +                           u32 *payload_off, u32 *total_size)
> > > > > +{
> > > > > +   int n_superblks = 0;
> > > > > +   u32 superblk_sz = 0;
> > > > > +   u32 afbc_size = 0;
> > > >
> > > > Please don't initialise the above three variables, given that you go on
> > > > to immediately change their values. In this case, initialising to zero
> > > > can only hide legitimate uninitialised-variable-use warnings.
> > > >
> > > > > +   n_superblks = (w / superblk_w) * (h / superblk_h);
> > > > > +   superblk_sz = (bpp * superblk_w * superblk_h) / BITS_PER_BYTE;
> > > > > +   afbc_size = ALIGN(n_superblks * AFBC_HEADER_SIZE, hdr_align);
> > > > > +   *payload_off = afbc_size;
> > > > > +
> > > > > +   afbc_size += n_superblks * ALIGN(superblk_sz,
> > > > > AFBC_SUPERBLK_ALIGNMENT);
> > > > > +   *total_size = afbc_size + offset;
> > > >
> > > > Generally these are referred to as 'tiles' rather than 'superblocks',
> > > > given that I would only expect one superblock per buffer, but if that's
> > > > the terminology AFBC uses then it might be better to stick with it.
> > > >
> > > > > +   if ((w * bpp) != (pitch * BITS_PER_BYTE)) {
> > > > > +           DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE)
> > > > > (=%u) should be same as width (=%u) * bpp (=%u)\n",
> > > > > +                         pitch * BITS_PER_BYTE, w, bpp
> > > > > +           );
> > > > > +           return false;
> > > > > +   }
> > > >
> > > > Having a too-small pitch is obviously a problem and we should reject
> > > > it. But is having a too-large pitch really a problem; does it need to
> > > > be an exact match, or can we support the case where the pitch is too
> > > > large but also tile-aligned? If we can, it would be very good to
> > > > support that.
> > >
> > > The reason for forcing it to be exact is as I said above - we _must_
> > > know what the "real" width and height is. Implementing this check to
> > > force (pitch == width * bpp) ensures that, and also leaves the option
> > > for us to relax to allow a larger pitch (as above) if that was the
> > > preferred approach for alignment.
> > >
> > > In general the current checks are deliberately designed to leave the
> > > door open for future improvements without breaking anything.
> > >
> > > Cheers,
> > > -Brian
> > >
> > > >
> > > > Cheers,
> > > > Daniel
> > > >
> > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> 
> Not sure how that snuck in.
> 
> > 
> > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Brian Starkey Nov. 8, 2019, 9:46 a.m. UTC | #10
Hi Daniel,

On Thu, Nov 07, 2019 at 08:28:08PM +0100, Daniel Vetter wrote:
> On Thu, Nov 07, 2019 at 05:49:14PM +0000, Brian Starkey wrote:
> > Hi Daniel,
> > 
> > On Thu, Nov 07, 2019 at 06:32:01PM +0100, Daniel Vetter wrote:
> > > On Thu, Nov 7, 2019 at 6:20 PM Brian Starkey <Brian.Starkey@arm.com> wrote:
> > > >
> > > > Hi Daniel,
> > > >
> > > > On Tue, Nov 05, 2019 at 11:26:36PM +0000, Daniel Stone wrote:
> > > > > Hi Andrzej,
> > > > > Thanks for taking this on! It's looking better than v1 for sure. A few
> > > > > things below:
> > > > >
> > > > > On Mon, 2019-11-04 at 23:12 +0100, Andrzej Pietrasiewicz wrote:
> > > > > > +bool drm_afbc_check_offset(struct drm_device *dev,
> > > > > > +                      const struct drm_mode_fb_cmd2 *mode_cmd)
> > > > > > +{
> > > > > > +   if (mode_cmd->offsets[0] != 0) {
> > > > > > +           DRM_DEBUG_KMS("AFBC buffers' plane offset should be
> > > > > > 0\n");
> > > > > > +           return false;
> > > > > > +   }
> > > > > > +
> > > > > > +   return true;
> > > > > > +}
> > > > > > +EXPORT_SYMBOL_GPL(drm_afbc_check_offset);
> > > > >
> > > > > Is this actually universally true? If the offset is sufficiently
> > > > > aligned for (e.g.) DMA transfers to succeed, is there any reason why it
> > > > > must be zero?
> > > > >
> > > > > > +bool drm_afbc_check_size_align(struct drm_device *dev,
> > > > > > +                          const struct drm_mode_fb_cmd2 *mode_cmd)
> > > > > > +{
> > > > > > +   switch (mode_cmd->modifier[0] &
> > > > > > AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > > > > > +   case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > > > > > +           if ((mode_cmd->width % 16) || (mode_cmd->height % 16))
> > > > > > {
> > > > >
> > > > > This is a dealbreaker for many resolutions: for example, 1366x768 isn't
> > > > > cleanly divisible by 16 in width. So this means that we would have to
> > > > > either use a larger buffer and crop, or scale, or something.
> > > > >
> > > > > No userspace is prepared to align fb width/height to tile dimensions
> > > > > like this, so this check will basically fail everywhere.
> > > > >
> > > > > However, overallocation relative to the declared width/height isn't a
> > > > > problem at all. In order to deal with horizontal alignment, you simply
> > > > > need to ensure that the stride is a multiple of the tile width; for
> > > > > vertical arrangement, that the buffer is large enough to contain
> > > > > sufficient 'lines' to the next tile boundary.
> > > > >
> > > > > i.e. rather than checking width/height, you should check:
> > > > >   * fb_stride >= (ALIGN(fb_width, tile_width), bpp)
> > > > >   * buf_size >= fb_stride * ALIGN(fb_height, tile_height)
> > > >
> > > > Well, sort of.
> > > >
> > > > I agree with you that for horizontal padding, we can indeed use pitch.
> > > >
> > > > However, the AFBC decoder(s) need to know exactly what the total
> > > > _allocated_ size in pixels of the buffer is - as this influences the
> > > > header size, and we need to know the header size to know where it ends
> > > > and the body begins.
> > > >
> > > > I see a couple of possible ways forwards:
> > > >
> > > >  - Keep it as-is. The restrictive checks ensure that there's no
> > > >    ambiguity and we use the fb width/height to determine the real
> > > >    allocated width/height. Userspace needs to be AFBC-aware and set up
> > > >    plane cropping to handle the alignment differences.
> > > >
> > > >  - Use pitch to determine the "real" width, and internally in the
> > > >    kernel align height up to the next alignment boundary. This works
> > > >    OK, so long as there's no additional padding at the bottom of the
> > > >    buffer. This would work, but I can't figure a way to check/enforce
> > > >    that there's no additional padding at the bottom.
> > > >
> > > >  - Something else...
> > > >
> > > > The checks as-implemented were deliberately conservative, and don't
> > > > preclude doing some relaxation in the future.
> > > >
> > > > On Android, gralloc is used to store the "real" allocated width/height
> > > > and this is used to set up the DRM API appropriately.
> > > 
> > > Fake stride + real visible h/w in the drmfb. Because that's how it
> > > works with all the tiled formats already, and expecting userspace to
> > > fudge this all correctly seems very backwards to me. In a way we had
> > > that entire fake stride discussion already for the block size format
> > > stuff already, but now in a different flavour.
> > 
> > Fake stride - like I said, no problem; sounds good. That solves one
> > dimension.
> > 
> > So do you have a proposal for how we determine what the allocated
> > height is in that case? I don't really see a way.
> 
> Could you compute the height by looking at the buffer size? Or does that
> not help since the header stuff is generally rather small?

I've wondered about that. We might be able to use it heuristically,
but it does place certain assumptions on the allocator - for instance
rounding up to a page order might cause problems.

> 
> Otherwise I guess just round up height and hope it works. If we run into a
> use-case where that doesn't work anymore somehow, then we get to rev all
> the afbc modifiers and make them 2 planes. With that there's no such issue
> anymore (which is why the intel compressed stuff has 2 planes).
> -Daniel
> 

We considered exposing the header explicitly as a plane before
originally submitting the modifiers; however the header and body are
linked in such a way that they aren't separable, so they aren't really
separate planes. Also we have multi-plane AFBC buffers, where each
plane has its own header and thus we'd need at least 6 planes to
describe it fully if we separate out the header.

I'm not entirely sure how a separate header plane would help with this
issue anyway.


We could round up the height, which should cover the common case. It
seem(ed) safest to start with the conservative restrictions.

My proposal for handling the case of additional vertical padding on
top of that would be to add a field in the modifier which indicates
how much additional vertical padding there is. I know the Broadcom
SAND modifiers do a similar thing. It does make the modifiers a bit
"non-opaque" though, as the modifier value can't be simply queried
from the modifier list.


... all of that said, the kernel interface is already rich enough to
support everything, if userspace understands that sometimes it needs
to crop.

Thanks,
-Brian

> 
> > Thanks,
> > -Brian
> > 
> > > 
> > > Also I think that's more reasons why this should be no-opt-outable
> > > code that's done for all drivers when we check framebuffers in addfb.
> > > Plus then some helpers to get at computed values for any framebuffer
> > > we know to be valid.
> > > -Daniel
> > > 
> > > > > This may force us to do some silly cropping games inside the Rockchip
> > > > > KMS driver, but I feel it beats the alternative of breaking userspace.
> > > >
> > > > Well, nothing's going to get broken - it's just perhaps not ready to
> > > > turn on AFBC yet.
> > > >
> > > > >
> > > > > > +                   DRM_DEBUG_KMS(
> > > > > > +                           "AFBC buffer must be aligned to 16
> > > > > > pixels\n"
> > > > > > +                   );
> > > > > > +                   return false;
> > > > > > +           }
> > > > > > +           break;
> > > > > > +   case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> > > > > > +           /* fall through */
> > > > >
> > > > > It's also incongruous that 32x8 is unsupported here, but has a section
> > > > > in get_superblk_wh; please harmonise them so this section either does
> > > > > the checks as above, or that get_superblk_wh doesn't support 32x8
> > > > > either.
> > > > >
> > > > > > +bool drm_afbc_check_fb_size_ret(u32 pitch, int bpp,
> > > > > > +                           u32 w, u32 h, u32 superblk_w, u32
> > > > > > superblk_h,
> > > > > > +                           size_t size, u32 offset, u32 hdr_align,
> > > > > > +                           u32 *payload_off, u32 *total_size)
> > > > > > +{
> > > > > > +   int n_superblks = 0;
> > > > > > +   u32 superblk_sz = 0;
> > > > > > +   u32 afbc_size = 0;
> > > > >
> > > > > Please don't initialise the above three variables, given that you go on
> > > > > to immediately change their values. In this case, initialising to zero
> > > > > can only hide legitimate uninitialised-variable-use warnings.
> > > > >
> > > > > > +   n_superblks = (w / superblk_w) * (h / superblk_h);
> > > > > > +   superblk_sz = (bpp * superblk_w * superblk_h) / BITS_PER_BYTE;
> > > > > > +   afbc_size = ALIGN(n_superblks * AFBC_HEADER_SIZE, hdr_align);
> > > > > > +   *payload_off = afbc_size;
> > > > > > +
> > > > > > +   afbc_size += n_superblks * ALIGN(superblk_sz,
> > > > > > AFBC_SUPERBLK_ALIGNMENT);
> > > > > > +   *total_size = afbc_size + offset;
> > > > >
> > > > > Generally these are referred to as 'tiles' rather than 'superblocks',
> > > > > given that I would only expect one superblock per buffer, but if that's
> > > > > the terminology AFBC uses then it might be better to stick with it.
> > > > >
> > > > > > +   if ((w * bpp) != (pitch * BITS_PER_BYTE)) {
> > > > > > +           DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE)
> > > > > > (=%u) should be same as width (=%u) * bpp (=%u)\n",
> > > > > > +                         pitch * BITS_PER_BYTE, w, bpp
> > > > > > +           );
> > > > > > +           return false;
> > > > > > +   }
> > > > >
> > > > > Having a too-small pitch is obviously a problem and we should reject
> > > > > it. But is having a too-large pitch really a problem; does it need to
> > > > > be an exact match, or can we support the case where the pitch is too
> > > > > large but also tile-aligned? If we can, it would be very good to
> > > > > support that.
> > > >
> > > > The reason for forcing it to be exact is as I said above - we _must_
> > > > know what the "real" width and height is. Implementing this check to
> > > > force (pitch == width * bpp) ensures that, and also leaves the option
> > > > for us to relax to allow a larger pitch (as above) if that was the
> > > > preferred approach for alignment.
> > > >
> > > > In general the current checks are deliberately designed to leave the
> > > > door open for future improvements without breaking anything.
> > > >
> > > > Cheers,
> > > > -Brian
> > > >
> > > > >
> > > > > Cheers,
> > > > > Daniel
> > > > >
> > > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > 
> > Not sure how that snuck in.
> > 
> > > 
> > > 
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
diff mbox series

Patch

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 36357a36a281..ae1ca5e02bfe 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -205,6 +205,14 @@  config DRM_SCHED
 	tristate
 	depends on DRM
 
+config DRM_AFBC
+	tristate
+	depends on DRM
+	help
+	  AFBC is a proprietary lossless image compression protocol and format.
+	  It provides fine-grained random access and minimizes the amount of
+	  data transferred between IP blocks.
+
 source "drivers/gpu/drm/i2c/Kconfig"
 
 source "drivers/gpu/drm/arm/Kconfig"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 9f1c7c486f88..3a5d092ea514 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -31,6 +31,7 @@  drm-$(CONFIG_OF) += drm_of.o
 drm-$(CONFIG_AGP) += drm_agpsupport.o
 drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
 drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
+drm-$(CONFIG_DRM_AFBC) += drm_afbc.o
 
 drm_vram_helper-y := drm_gem_vram_helper.o \
 		     drm_vram_helper_common.o
diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
index a204103b3efb..25c3dc408cda 100644
--- a/drivers/gpu/drm/arm/Kconfig
+++ b/drivers/gpu/drm/arm/Kconfig
@@ -29,6 +29,7 @@  config DRM_MALI_DISPLAY
 	select DRM_KMS_HELPER
 	select DRM_KMS_CMA_HELPER
 	select DRM_GEM_CMA_HELPER
+	select DRM_AFBC
 	select VIDEOMODE_HELPERS
 	help
 	  Choose this option if you want to compile the ARM Mali Display
diff --git a/drivers/gpu/drm/drm_afbc.c b/drivers/gpu/drm/drm_afbc.c
new file mode 100644
index 000000000000..010ca9eb0480
--- /dev/null
+++ b/drivers/gpu/drm/drm_afbc.c
@@ -0,0 +1,129 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * (C) 2019 Collabora Ltd.
+ *
+ * author: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
+ *
+ */
+#include <linux/module.h>
+
+#include <drm/drm_afbc.h>
+#include <drm/drm_device.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_gem.h>
+#include <drm/drm_mode.h>
+#include <drm/drm_print.h>
+
+#define AFBC_HEADER_SIZE		16
+
+bool drm_afbc_check_offset(struct drm_device *dev,
+			   const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	if (mode_cmd->offsets[0] != 0) {
+		DRM_DEBUG_KMS("AFBC buffers' plane offset should be 0\n");
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(drm_afbc_check_offset);
+
+bool drm_afbc_check_size_align(struct drm_device *dev,
+			       const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	switch (mode_cmd->modifier[0] & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
+		if ((mode_cmd->width % 16) || (mode_cmd->height % 16)) {
+			DRM_DEBUG_KMS(
+				"AFBC buffer must be aligned to 16 pixels\n"
+			);
+			return false;
+		}
+		break;
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
+		/* fall through */
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4:
+		/* fall through */
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4:
+		/* fall through */
+	default:
+		DRM_DEBUG_KMS("Unsupported AFBC block size\n");
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(drm_afbc_check_size_align);
+
+bool drm_afbc_get_superblk_wh(u64 modifier, u32 *w, u32 *h)
+{
+	switch (modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
+		*w = 16;
+		*h = 16;
+		break;
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
+		*w = 32;
+		*h = 8;
+		break;
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_64x4:
+		/* fall through */
+	case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8_64x4:
+		/* fall through */
+	default:
+		DRM_DEBUG_KMS("Invalid AFBC_FORMAT_MOD_BLOCK_SIZE: %lld.\n",
+			      modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK);
+		return false;
+	}
+	return true;
+}
+EXPORT_SYMBOL_GPL(drm_afbc_get_superblk_wh);
+
+bool drm_afbc_check_fb_size_ret(u32 pitch, int bpp,
+				u32 w, u32 h, u32 superblk_w, u32 superblk_h,
+				size_t size, u32 offset, u32 hdr_align,
+				u32 *payload_off, u32 *total_size)
+{
+	int n_superblks = 0;
+	u32 superblk_sz = 0;
+	u32 afbc_size = 0;
+
+	n_superblks = (w / superblk_w) * (h / superblk_h);
+	superblk_sz = (bpp * superblk_w * superblk_h) / BITS_PER_BYTE;
+	afbc_size = ALIGN(n_superblks * AFBC_HEADER_SIZE, hdr_align);
+	*payload_off = afbc_size;
+
+	afbc_size += n_superblks * ALIGN(superblk_sz, AFBC_SUPERBLK_ALIGNMENT);
+	*total_size = afbc_size + offset;
+
+	if ((w * bpp) != (pitch * BITS_PER_BYTE)) {
+		DRM_DEBUG_KMS("Invalid value of (pitch * BITS_PER_BYTE) (=%u) should be same as width (=%u) * bpp (=%u)\n",
+			      pitch * BITS_PER_BYTE, w, bpp
+		);
+		return false;
+	}
+
+	if (size < afbc_size) {
+		DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
+			      size, afbc_size
+		);
+
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL(drm_afbc_check_fb_size_ret);
+
+bool drm_afbc_check_fb_size(u32 pitch, int bpp,
+			    u32 w, u32 h, u32 superblk_w, u32 superblk_h,
+			    size_t size, u32 offset, u32 hdr_align)
+{
+	u32 payload_offset, total_size;
+
+	return drm_afbc_check_fb_size_ret(pitch, bpp, w, h,
+					  superblk_w, superblk_h,
+					  size, offset, hdr_align,
+					  &payload_offset, &total_size);
+}
+EXPORT_SYMBOL(drm_afbc_check_fb_size);
diff --git a/include/drm/drm_afbc.h b/include/drm/drm_afbc.h
new file mode 100644
index 000000000000..b28ae2849f96
--- /dev/null
+++ b/include/drm/drm_afbc.h
@@ -0,0 +1,36 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * (C) 2019 Collabora Ltd.
+ *
+ * author: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
+ *
+ */
+#ifndef __DRM_AFBC_H__
+#define __DRM_AFBC_H__
+
+#include <linux/types.h>
+
+struct drm_device;
+struct drm_mode_fb_cmd2;
+struct drm_gem_object;
+
+#define AFBC_SUPERBLK_ALIGNMENT		128
+
+bool drm_afbc_check_offset(struct drm_device *dev,
+			   const struct drm_mode_fb_cmd2 *mode_cmd);
+
+bool drm_afbc_check_size_align(struct drm_device *dev,
+			       const struct drm_mode_fb_cmd2 *mode_cmd);
+
+bool drm_afbc_check_fb_size_ret(u32 pitch, int bpp,
+				u32 w, u32 h, u32 superblk_w, u32 superblk_h,
+				size_t size, u32 offset, u32 hdr_align,
+				u32 *payload_off, u32 *total_size);
+
+bool drm_afbc_check_fb_size(u32 pitch, int bpp,
+			    u32 w, u32 h, u32 superblk_w, u32 superblk_h,
+			    size_t size, u32 offset, u32 hdr_align);
+
+bool drm_afbc_get_superblk_wh(u64 modifier, u32 *w, u32 *h);
+
+#endif /* __DRM_AFBC_H__ */