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

[Andrzej Pietrasiewicz <andrzej.p@collabora.com>]

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?

Andrzej
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel