Re: [PATCH 10/13] drm/atomic: atomic plane properties

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Dec 16, 2014 at 06:05:38PM -0500, Rob Clark wrote:
> Expose the core plane state as properties, so they can be updated via
> atomic ioctl.
> 
> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx>

Just comments about the lack of PROP_ATOMIC and one suggestion for a
comment. With that addressed this is Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

> ---
>  Documentation/DocBook/drm.tmpl | 74 ++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/drm_atomic.c   | 69 ++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/drm_crtc.c     | 82 +++++++++++++++++++++++++++++++++++++++---
>  include/drm/drm_crtc.h         | 10 ++++++
>  4 files changed, 224 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 4b592ff..282fa6b 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2564,7 +2564,7 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >Description/Restrictions</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="25" valign="top" >DRM</td>
> +	<td rowspan="35" valign="top" >DRM</td>
>  	<td rowspan="4" valign="top" >Generic</td>
>  	<td valign="top" >“EDID”</td>
>  	<td valign="top" >BLOB | IMMUTABLE</td>
> @@ -2594,7 +2594,7 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >Contains tiling information for a connector.</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="1" valign="top" >Plane</td>
> +	<td rowspan="11" valign="top" >Plane</td>
>  	<td valign="top" >“type”</td>
>  	<td valign="top" >ENUM | IMMUTABLE</td>
>  	<td valign="top" >{ "Overlay", "Primary", "Cursor" }</td>
> @@ -2602,6 +2602,76 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >Plane type</td>
>  	</tr>
>  	<tr>
> +	<td valign="top" >“SRC_X”</td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min=0, Max=UINT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout source x coordinate in 16.16 fixed point (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“SRC_Y”</td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min=0, Max=UINT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout source y coordinate in 16.16 fixed point (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“SRC_W”</td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min=0, Max=UINT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout source width in 16.16 fixed point (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“SRC_H”</td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min=0, Max=UINT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout source height in 16.16 fixed point (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“CRTC_X”</td>
> +	<td valign="top" >SIGNED_RANGE</td>
> +	<td valign="top" >Min=INT_MIN, Max=INT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout CRTC (destination) x coordinate (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“CRTC_Y”</td>
> +	<td valign="top" >SIGNED_RANGE</td>
> +	<td valign="top" >Min=INT_MIN, Max=INT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout CRTC (destination) y coordinate (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“CRTC_W”</td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min=0, Max=UINT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout CRTC (destination) width (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“CRTC_H”</td>
> +	<td valign="top" >RANGE</td>
> +	<td valign="top" >Min=0, Max=UINT_MAX</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout CRTC (destination) height (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“FB_ID”</td>
> +	<td valign="top" >OBJECT</td>
> +	<td valign="top" >DRM_MODE_OBJECT_FB</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >Scanout framebuffer (atomic)</td>
> +	</tr>
> +	<tr>
> +	<td valign="top" >“CRTC_ID”</td>
> +	<td valign="top" >OBJECT</td>
> +	<td valign="top" >DRM_MODE_OBJECT_CRTC</td>
> +	<td valign="top" >Plane</td>
> +	<td valign="top" >CRTC that plane is attached to (atomic)</td>
> +	</tr>
> +	<tr>
>  	<td rowspan="2" valign="top" >DVI-I</td>
>  	<td valign="top" >“subconnector”</td>
>  	<td valign="top" >ENUM</td>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index afb830d..c09a05a 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -352,9 +352,41 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>  		struct drm_plane_state *state, struct drm_property *property,
>  		uint64_t val)
>  {
> -	if (plane->funcs->atomic_set_property)
> -		return plane->funcs->atomic_set_property(plane, state, property, val);
> -	return -EINVAL;
> +	struct drm_device *dev = plane->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->prop_fb_id) {
> +		struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, val);
> +		drm_atomic_set_fb_for_plane(state, fb);
> +		if (fb)
> +			drm_framebuffer_unreference(fb);
> +	} else if (property == config->prop_crtc_id) {
> +		struct drm_crtc *crtc = drm_crtc_find(dev, val);
> +		return drm_atomic_set_crtc_for_plane(state->state, plane, crtc);
> +	} else if (property == config->prop_crtc_x) {
> +		state->crtc_x = U642I64(val);
> +	} else if (property == config->prop_crtc_y) {
> +		state->crtc_y = U642I64(val);
> +	} else if (property == config->prop_crtc_w) {
> +		state->crtc_w = val;
> +	} else if (property == config->prop_crtc_h) {
> +		state->crtc_h = val;
> +	} else if (property == config->prop_src_x) {
> +		state->src_x = val;
> +	} else if (property == config->prop_src_y) {
> +		state->src_y = val;
> +	} else if (property == config->prop_src_w) {
> +		state->src_w = val;
> +	} else if (property == config->prop_src_h) {
> +		state->src_h = val;

We need to check for PROP_ATOMIC somewhere. Well more precisely

	if ((prop->flags & PROP_ATOMIC) && !file_priv->atomic_kms)
		return -EINVAL;

And with all the scaffolding for file_priv->atomic_kms like we have for
file_priv->universal_planes. I guess that should be directly in the legacy
get_props paths, since otherwise we can't get at the file_priv. Other
callers probably don't care about this filtering.


> +	} else if (plane->funcs->atomic_set_property) {
> +		return plane->funcs->atomic_set_property(plane, state,
> +				property, val);
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_plane_set_property);
>  
> @@ -374,9 +406,36 @@ int drm_atomic_plane_get_property(struct drm_plane *plane,
>  		const struct drm_plane_state *state,
>  		struct drm_property *property, uint64_t *val)
>  {
> -	if (plane->funcs->atomic_get_property)
> +	struct drm_device *dev = plane->dev;
> +	struct drm_mode_config *config = &dev->mode_config;
> +
> +	if (property == config->prop_fb_id) {
> +		*val = (state->fb) ? state->fb->base.id : 0;
> +	} else if (property == config->prop_crtc_id) {
> +		*val = (state->crtc) ? state->crtc->base.id : 0;
> +	} else if (property == config->prop_crtc_x) {
> +		*val = I642U64(state->crtc_x);
> +	} else if (property == config->prop_crtc_y) {
> +		*val = I642U64(state->crtc_y);
> +	} else if (property == config->prop_crtc_w) {
> +		*val = state->crtc_w;
> +	} else if (property == config->prop_crtc_h) {
> +		*val = state->crtc_h;
> +	} else if (property == config->prop_src_x) {
> +		*val = state->src_x;
> +	} else if (property == config->prop_src_y) {
> +		*val = state->src_y;
> +	} else if (property == config->prop_src_w) {
> +		*val = state->src_w;
> +	} else if (property == config->prop_src_h) {
> +		*val = state->src_h;
> +	} else if (plane->funcs->atomic_get_property) {
>  		return plane->funcs->atomic_get_property(plane, state, property, val);
> -	return -EINVAL;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_plane_get_property);
>  
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 62f5dc8..fd6f91d 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -819,6 +819,13 @@ static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
>  		      mode->interlace ?  " interlaced" : "");
>  }
>  
> +static bool
> +has_atomic_properties(struct drm_device *dev)
> +{
> +	struct drm_mode_config *config = &dev->mode_config;
> +	return !!config->funcs->atomic_get_property;

Yeah, I think we really want to call this config->uses_atomic_props, much
clearer what it does. And gets rid of this wrapper. Aside: Casting to bool
automatically applies the !! trick, so not needed here.

> +}
> +
>  /**
>   * drm_connector_init - Init a preallocated connector
>   * @dev: DRM device
> @@ -1174,6 +1181,7 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  			     const uint32_t *formats, uint32_t format_count,
>  			     enum drm_plane_type type)
>  {
> +	struct drm_mode_config *config = &dev->mode_config;
>  	int ret;
>  
>  	ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
> @@ -1198,15 +1206,28 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  	plane->possible_crtcs = possible_crtcs;
>  	plane->type = type;
>  
> -	list_add_tail(&plane->head, &dev->mode_config.plane_list);
> -	dev->mode_config.num_total_plane++;
> +	list_add_tail(&plane->head, &config->plane_list);
> +	config->num_total_plane++;
>  	if (plane->type == DRM_PLANE_TYPE_OVERLAY)
> -		dev->mode_config.num_overlay_plane++;
> +		config->num_overlay_plane++;
>  
>  	drm_object_attach_property(&plane->base,
> -				   dev->mode_config.plane_type_property,
> +				   config->plane_type_property,
>  				   plane->type);
>  
> +	if (has_atomic_properties(dev)) {
> +		drm_object_attach_property(&plane->base, config->prop_fb_id, 0);
> +		drm_object_attach_property(&plane->base, config->prop_crtc_id, 0);
> +		drm_object_attach_property(&plane->base, config->prop_crtc_x, 0);
> +		drm_object_attach_property(&plane->base, config->prop_crtc_y, 0);
> +		drm_object_attach_property(&plane->base, config->prop_crtc_w, 0);
> +		drm_object_attach_property(&plane->base, config->prop_crtc_h, 0);
> +		drm_object_attach_property(&plane->base, config->prop_src_x, 0);
> +		drm_object_attach_property(&plane->base, config->prop_src_y, 0);
> +		drm_object_attach_property(&plane->base, config->prop_src_w, 0);
> +		drm_object_attach_property(&plane->base, config->prop_src_h, 0);

s/0/DRM_MODE_PROP_ATOMIC/ in all the above.

> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_universal_plane_init);
> @@ -1365,6 +1386,59 @@ static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
>  		return -ENOMEM;
>  	dev->mode_config.tile_property = prop;
>  
> +	prop = drm_property_create_range(dev, 0, "SRC_X", 0, UINT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_src_x = prop;
> +
> +	prop = drm_property_create_range(dev, 0, "SRC_Y", 0, UINT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_src_y = prop;
> +
> +	prop = drm_property_create_range(dev, 0, "SRC_W", 0, UINT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_src_w = prop;
> +
> +	prop = drm_property_create_range(dev, 0, "SRC_H", 0, UINT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_src_h = prop;
> +
> +	prop = drm_property_create_signed_range(dev, 0, "CRTC_X",
> +			INT_MIN, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_crtc_x = prop;
> +
> +	prop = drm_property_create_signed_range(dev, 0, "CRTC_Y",
> +			INT_MIN, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_crtc_y = prop;
> +
> +	prop = drm_property_create_range(dev, 0, "CRTC_W", 0, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_crtc_w = prop;
> +
> +	prop = drm_property_create_range(dev, 0, "CRTC_H", 0, INT_MAX);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_crtc_h = prop;
> +
> +	prop = drm_property_create_object(dev, 0, "FB_ID", DRM_MODE_OBJECT_FB);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_fb_id = prop;
> +
> +	prop = drm_property_create_object(dev, 0,
> +			"CRTC_ID", DRM_MODE_OBJECT_CRTC);
> +	if (!prop)
> +		return -ENOMEM;
> +	dev->mode_config.prop_crtc_id = prop;
> +
>  	return 0;
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index ca8ba72..48fcd98 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -1085,6 +1085,16 @@ struct drm_mode_config {
>  	struct drm_property *tile_property;
>  	struct drm_property *plane_type_property;
>  	struct drm_property *rotation_property;

Imo a comment here like /* plane atomic props */ to keep our sanity would
be good.

> +	struct drm_property *prop_src_x;
> +	struct drm_property *prop_src_y;
> +	struct drm_property *prop_src_w;
> +	struct drm_property *prop_src_h;
> +	struct drm_property *prop_crtc_x;
> +	struct drm_property *prop_crtc_y;
> +	struct drm_property *prop_crtc_w;
> +	struct drm_property *prop_crtc_h;
> +	struct drm_property *prop_fb_id;
> +	struct drm_property *prop_crtc_id;
>  
>  	/* DVI-I properties */
>  	struct drm_property *dvi_i_subconnector_property;
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux