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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Nov 06, 2019 at 01:45:05PM +0100, Andrzej Pietrasiewicz wrote:
> Hi Daniel,
> 
> Thank you for review,
> 
> W dniu 05.11.2019 o 10:22, Daniel Vetter pisze:
> > On Mon, Nov 04, 2019 at 11:12:25PM +0100, Andrzej Pietrasiewicz wrote:
> > > These are useful for other users of afbc, e.g. rockchip.
> > > 
> 
> <snip>
> 
> > > +
> > > +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;
> > > +
> > > +	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;
> > > +
> > > +	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;
> > > +	}
> > > +
> > > +	if (size < afbc_size) {
> > > +		DRM_DEBUG_KMS("buffer size (%zu) too small for AFBC buffer size = %u\n",
> > > +			      size, afbc_size
> > > +		);
> > > +
> > > +		return false;
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +EXPORT_SYMBOL(drm_afbc_check_fb_size_ret);
> > > +
> > > +bool drm_afbc_check_fb_size(u32 pitch, int bpp,
> > > +			    u32 w, u32 h, u32 superblk_w, u32 superblk_h,
> > > +			    size_t size, u32 offset, u32 hdr_align)
> > > +{
> > > +	u32 payload_offset, total_size;
> > > +
> > > +	return drm_afbc_check_fb_size_ret(pitch, bpp, w, h,
> > > +					  superblk_w, superblk_h,
> > > +					  size, offset, hdr_align,
> > > +					  &payload_offset, &total_size);
> > > +}
> > > +EXPORT_SYMBOL(drm_afbc_check_fb_size);
> > 
> > Why don't we have one overall "check afbc parameters against buffer"
> > function?
> > 
> 
> I noticed that different drivers have different needs (malidp
> and rockchip are kind of similar, but komeda is a bit different).
> That is why the helpers are only building blocks out of which
> each driver builds its own checking logic. In particular komeda
> wants some by-products of the check stored in its internal data
> structures, hence drm_afbc_check_fb_size_ret() and its wrapper
> drm_afbc_check_fb_size() which ignores the "out" parameters.
> 
> If I wanted to create one overall "check afbc parameters" I'd have
> to come up with a way to pass driver-specific requirements to it
> and then inside the function have some logic to decide what to
> check against what. Do you think it is worth it?

Hm I figured there's at least two parts of this:
- Generic checking of afbc against the fb parameters, i.e. is it big
  enough, correctly aligned, all that.
- Additional driver checks, which might need some of the same parameters
  again.

The idea behind asking for the first part is that maybe we should put that
into the core as part of the addfb checks (like we do for the tiled NV12
thing already). Then drivers only need to do additional checks on top for
their specific constraints. For that you could expose some helpers ofc (to
get the payload_offset and anything else computed you might need).

But the basic drm_afbc_check_fb_size() should imo be done unconditionally
for any afbc modifier. To make sure we don't have lots of drivers with
different bugs in that thing.
-Daniel
-- 
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