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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
