Re: [PATCHv2 4/4] drm/rockchip: Add support for afbc

[Daniel Stone <daniels@collabora.com>]

Hi Andrzej,

On Mon, 2019-11-04 at 23:12 +0100, Andrzej Pietrasiewicz wrote:
> +	if (mode_cmd->modifier[0]) {

I believe this can still be DRM_FORMAT_MOD_INVALID, which != 0. You
probably want to explicitly check if it's an AFBC modifier.

> +		const struct drm_format_info *info;
> +		int bpp;
> +
> +		if (!drm_afbc_check_offset(dev, mode_cmd))
> +			return ERR_PTR(-EINVAL);
> +
> +		if (!drm_afbc_get_superblk_wh(mode_cmd->modifier[0],
> &w, &h))
> +			return ERR_PTR(-EINVAL);
> +
> +		if (w != 16 || h != 16) {
> +			DRM_DEV_ERROR(dev->dev,
> +				"Unsupported afbc tile w/h [%d/%d]\n",
> w, h);

This can just be a WARN_ONCE() or something, since it indicates an
impossible condition - the DRM core should've already rejected this
modifier as unsupported.

> +		if (mode_cmd->width > ROCKCHIP_MAX_AFBC_WIDTH) {
> +			DRM_DEV_ERROR(dev->dev,
> +				      "Unsupported width %d>%d\n",
> +				      mode_cmd->width,
> +				      ROCKCHIP_MAX_AFBC_WIDTH
> +			);

Userspace shouldn't be allowed to spam the log by triggering error
messages; please make this debug instead. Whilst you're there, adding
logs to the other error returns here might be useful.

> @@ -166,6 +179,7 @@ struct vop {
>  	/* optional internal rgb encoder */
>  	struct rockchip_rgb *rgb;
>  
> +	struct vop_win *afbc_win;

It seems like everywhere afbc_win is used, it's not actually used for
the window value, but rather just used as an is_afbc_enabled bool. In
that case, it would be better as a real bool, and living in either the
output or plane state.

This would eliminate the need to unset the variable as well.

Relatedly, can one VOP support multiple simultaneous windows using
AFBC? If not, the check that only one window is using AFBC is missing
from this patch.

> +static int vop_convert_afbc_format(uint32_t format)
> +{
> +	switch (format) {
> +	case DRM_FORMAT_XRGB8888:
> +	case DRM_FORMAT_ARGB8888:
> +	case DRM_FORMAT_XBGR8888:
> +	case DRM_FORMAT_ABGR8888:
> +		return AFBC_FMT_U8U8U8U8;
> +	case DRM_FORMAT_RGB888:
> +	case DRM_FORMAT_BGR888:
> +		return AFBC_FMT_U8U8U8;
> +	case DRM_FORMAT_RGB565:
> +	case DRM_FORMAT_BGR565:
> +		return AFBC_FMT_RGB565;
> +	/* either of the below should not be reachable */

Unreachable can be WARN_ONCE() rather than a silent return.

Other than that, this is looking a _lot_ nicer than v1 though!

Cheers,
Daniel

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