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

[Daniel Stone <daniels@collabora.com>]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel