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

[Brian Starkey <Brian.Starkey@arm.com>]

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