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

From: Liviu Dudau <liviu.dudau@arm.com>
To: Daniel Stone <daniels@collabora.com>
Cc: Ayan Halder <Ayan.Halder@arm.com>,
	kernel@collabora.com, David Airlie <airlied@linux.ie>,
	Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
	linux-rockchip@lists.infradead.org,
	James Wang <james.qian.wang@arm.com>,
	dri-devel@lists.freedesktop.org,
	Mihail Atanassov <mihail.atanassov@arm.com>,
	Sean Paul <sean@poorly.run>
Subject: Re: [PATCHv2 1/4] drm/arm: Factor out generic afbc helpers
Date: Wed, 6 Nov 2019 10:28:58 +0000	[thread overview]
Message-ID: <20191106102858.gg2gjx7vhdo72sai@e110455-lin.cambridge.arm.com> (raw)
In-Reply-To: <f4aa14e89852c9ef46ade10e8eccdb66b1adc052.camel@collabora.com>

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