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
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
