Re: [Intel-gfx] [PATCH 1/3] drm: Add an "expose 3d modes" property

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Sep 27, 2012 at 07:41:06PM +0100, Damien Lespiau wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
> 
> The "expose 3D modes" property can be attached to connectors to allow
> user space to indicate it can deal with 3D modes and that the drm driver
> should expose those 3D modes.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

I've thought a bit more about this and I don't like the connector prop so
much any more:
- connector properties stick around, so if you mix up your userspace
  things could go wrong (e.g. i-g-t tests and ddx).
- connector properties are exposed to users by default.
- it makes little sense for userspace to not enable this on all
  connectors.

So I think a per-file_priv option to not filter out 3d modes would be
better suited.

We don't yet have any driver-agnostic ioctl yet for such things, so would
require a bit more work ...
-Daniel


> ---
>  drivers/gpu/drm/drm_crtc.c | 35 ++++++++++++++++++++++++++++++++++-
>  include/drm/drm_crtc.h     |  6 ++++++
>  include/drm/drm_mode.h     |  4 ++++
>  3 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 6fbfc24..10a289c 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -841,6 +841,35 @@ int drm_mode_create_tv_properties(struct drm_device *dev, int num_modes,
>  }
>  EXPORT_SYMBOL(drm_mode_create_tv_properties);
>  
> +static const struct drm_prop_enum_list expose_3d_modes_list[] =
> +{
> +	{ DRM_MODE_EXPOSE_3D_MODES_OFF, "Off" },
> +	{ DRM_MODE_EXPOSE_3D_MODES_ON, "On" }
> +};
> +
> +/**
> + * drm_mode_create_s3d_properties - create stereo 3D properties
> + * @dev: DRM device
> + *
> + * This functions create properties that are used by the stereo 3D code. Those
> + * properties must be attached to the desired connectors afterwards.
> + */
> +int drm_mode_create_s3d_properties(struct drm_device *dev)
> +{
> +	struct drm_property *prop;
> +
> +	if (dev->mode_config.s3d_expose_modes_property)
> +		return 0;
> +
> +	prop = drm_property_create_enum(dev, 0, "expose 3D modes",
> +					expose_3d_modes_list,
> +					ARRAY_SIZE(expose_3d_modes_list));
> +	dev->mode_config.s3d_expose_modes_property = prop;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_create_s3d_properties);
> +
>  /**
>   * drm_mode_create_scaling_mode_property - create scaling mode property
>   * @dev: DRM device
> @@ -3170,12 +3199,16 @@ static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
>  {
>  	int ret = -EINVAL;
>  	struct drm_connector *connector = obj_to_connector(obj);
> +	struct drm_device *dev = connector->dev;
>  
>  	/* Do DPMS ourselves */
> -	if (property == connector->dev->mode_config.dpms_property) {
> +	if (property == dev->mode_config.dpms_property) {
>  		if (connector->funcs->dpms)
>  			(*connector->funcs->dpms)(connector, (int)value);
>  		ret = 0;
> +	} else if (property == dev->mode_config.s3d_expose_modes_property) {
> +		connector->expose_3d_modes = value;
> +		ret = 0;
>  	} else if (connector->funcs->set_property)
>  		ret = connector->funcs->set_property(connector, property, value);
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index bfacf0d..34372cb 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -578,6 +578,8 @@ struct drm_connector {
>  	/* requested DPMS state */
>  	int dpms;
>  
> +	int expose_3d_modes;
> +
>  	void *helper_private;
>  
>  	/* forced on connector */
> @@ -802,6 +804,9 @@ struct drm_mode_config {
>  	struct drm_property *tv_saturation_property;
>  	struct drm_property *tv_hue_property;
>  
> +	/* Stereo 3D properties */
> +	struct drm_property *s3d_expose_modes_property;
> +
>  	/* Optional properties */
>  	struct drm_property *scaling_mode_property;
>  	struct drm_property *dithering_mode_property;
> @@ -950,6 +955,7 @@ extern int drm_property_add_enum(struct drm_property *property, int index,
>  extern int drm_mode_create_dvi_i_properties(struct drm_device *dev);
>  extern int drm_mode_create_tv_properties(struct drm_device *dev, int num_formats,
>  				     char *formats[]);
> +extern int drm_mode_create_s3d_properties(struct drm_device *dev);
>  extern int drm_mode_create_scaling_mode_property(struct drm_device *dev);
>  extern int drm_mode_create_dithering_property(struct drm_device *dev);
>  extern int drm_mode_create_dirty_info_property(struct drm_device *dev);
> diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h
> index 3d6301b..45b19c6 100644
> --- a/include/drm/drm_mode.h
> +++ b/include/drm/drm_mode.h
> @@ -83,6 +83,10 @@
>  #define DRM_MODE_DIRTY_ON       1
>  #define DRM_MODE_DIRTY_ANNOTATE 2
>  
> +/* Expose 3D modes */
> +#define DRM_MODE_EXPOSE_3D_MODES_OFF	0
> +#define DRM_MODE_EXPOSE_3D_MODES_ON	1
> +
>  struct drm_mode_modeinfo {
>  	__u32 clock;
>  	__u16 hdisplay, hsync_start, hsync_end, htotal, hskew;
> -- 
> 1.7.11.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch