Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1623620 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 6/6] drm: mali-dp: Add writeback connector Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-04-14 11:50 +0200
    Re: [PATCH 6/6] drm: mali-dp: Add writeback connector Brian Starkey <brian.starkey@arm.com> - 2017-04-18 19:40 +0200

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

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

[toc] | [next] | [standalone]

#1625492

On Fri, Apr 14, 2017 at 11:47:00AM +0200, Boris Brezillon wrote:
>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?
>

Yeah makes sense. These should be common to everyone until
cropping/scaling support is added.

>> +
>> +	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.
>

Yeah could do, but given our driver requires us to run through the
whole table to get the HW ID anyway it seemed like totally wasted
effort to do the same thing in the core.

It's probably a negligible overhead, but it's also unnecessary for
100% of the current writeback implementations ;-)

If a different driver is implemented such that the HW ID lookup isn't
an exhaustive list search then we could add a helper for them to use
which checks the blob.

Cheers,
-Brian

>> +		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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web