Re: [PATCH 6/6] drm: mali-dp: Add writeback connector

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Fri, 25 Nov 2016 16:49:04 +0000
Brian Starkey <brian.starkey@arm.com> wrote:

> +static int
> +malidp_mw_encoder_atomic_check(struct drm_encoder *encoder,
> +			       struct drm_crtc_state *crtc_state,
> +			       struct drm_connector_state *conn_state)
> +{
> +	struct malidp_mw_connector_state *mw_state = to_mw_state(conn_state);
> +	struct malidp_drm *malidp = encoder->dev->dev_private;
> +	struct drm_framebuffer *fb;
> +	int i, n_planes;
> +
> +	if (!conn_state->writeback_job || !conn_state->writeback_job->fb)
> +		return 0;
> +
> +	fb = conn_state->writeback_job->fb;
> +	if ((fb->width != crtc_state->mode.hdisplay) ||
> +	    (fb->height != crtc_state->mode.vdisplay)) {
> +		DRM_DEBUG_KMS("Invalid framebuffer size %ux%u\n",
> +				fb->width, fb->height);
> +		return -EINVAL;
> +	}

These checks look pretty generic to me. Shouldn't we have a default
helper doing that?

> +
> +	mw_state->format =
> +		malidp_hw_get_format_id(&malidp->dev->map, SE_MEMWRITE,
> +					fb->pixel_format);
> +	if (mw_state->format == MALIDP_INVALID_FORMAT_ID) {

Same goes here. By adding a format_types table similar to what is
exposed in drm_plane [1], we could do this check in the core. The only
thing left to the driver is the 4CC -> driver-specific-id conversion.

> +		struct drm_format_name_buf format_name;
> +
> +		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> +			      drm_get_format_name(fb->pixel_format, &format_name));
> +		return -EINVAL;
> +	}
> +
> +	n_planes = drm_format_num_planes(fb->pixel_format);
> +	for (i = 0; i < n_planes; i++) {
> +		struct drm_gem_cma_object *obj = drm_fb_cma_get_gem_obj(fb, i);
> +		if (!malidp_hw_pitch_valid(malidp->dev, fb->pitches[i])) {
> +			DRM_DEBUG_KMS("Invalid pitch %u for plane %d\n",
> +				      fb->pitches[i], i);
> +			return -EINVAL;
> +		}
> +		mw_state->pitches[i] = fb->pitches[i];
> +		mw_state->addrs[i] = obj->paddr + fb->offsets[i];
> +	}
> +	mw_state->n_planes = n_planes;
> +
> +	return 0;
> +}


[1]http://lxr.free-electrons.com/source/include/drm/drm_plane.h#L482