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.star...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
