[Intel-gfx] [PATCH v5 02/11] drm/core: Allow attaching custom scaling mode properties

[Sean Paul]

On Tue, Apr 25, 2017 at 01:55:26PM +0200, Maarten Lankhorst wrote:
> Some connectors may not allow all scaling mode properties, this function will allow
> creating the scaling mode property with only the supported subset.
> 
> This will make it possible to convert i915 connectors to atomic.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: dri-devel at lists.freedesktop.org
> ---
>  drivers/gpu/drm/drm_atomic.c    |  3 ++-
>  drivers/gpu/drm/drm_connector.c | 45 +++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_connector.h     |  5 +++++
>  3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 25ea6f797a54..21db00d9a933 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1125,7 +1125,8 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
>  			state->link_status = val;
>  	} else if (property == config->aspect_ratio_property) {
>  		state->picture_aspect_ratio = val;
> -	} else if (property == config->scaling_mode_property) {
> +	} else if (property == config->scaling_mode_property ||
> +		   property == connector->scaling_mode_property) {

Since scaling_mode_property isn't part of the atomic ABI before this series,
have you considered just supporting scaling mode property on connectors for
atomic? Then you wouldn't need to check both, and you wouldn't have the issue of
stomping properties if they're both set in a commit.

>  		state->scaling_mode = val;
>  	} else if (connector->funcs->atomic_set_property) {
>  		return connector->funcs->atomic_set_property(connector,
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 9f847615ac74..49baa21b4ce7 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -954,6 +954,7 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
>  				drm_scaling_mode_enum_list,
>  				    ARRAY_SIZE(drm_scaling_mode_enum_list));
>  
> +

nit: kruft.

>  	dev->mode_config.scaling_mode_property = scaling_mode;
>  
>  	return 0;
> @@ -961,6 +962,50 @@ int drm_mode_create_scaling_mode_property(struct drm_device *dev)
>  EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
>  
>  /**
> + * drm_mode_connector_attach_custom_scaling_mode_property - TODO

It would be nice to fill in this TODO

> + */
> +int drm_mode_connector_attach_custom_scaling_mode_property(struct drm_connector *connector,
> +							   u32 scaling_mode_mask,
> +							   u32 default_scaler)
> +{
> +	struct drm_device *dev = connector->dev;
> +	struct drm_property *scaling_mode_property;
> +	int i;
> +
> +	if (!scaling_mode_mask)
> +		scaling_mode_mask = ~0U;

It's hard to interpret this code without the docs above, but it seems like we're
just covering the case where the caller interprets a mask of 0 as "don't care".
I'd rather not try to be smart about interpreting invalid input, instead, I'd
prefer just to return -EINVAL if scaling_mode_mask is 0 after the next line.

> +
> +	scaling_mode_mask &= (1U << ARRAY_SIZE(drm_scaling_mode_enum_list)) - 1;

scaling_mode_mask &= BIT(ARRAY_SIZE(drm_scaling_mode_enum_list)) - 1;

> +
> +	scaling_mode_property =
> +		drm_property_create(dev, 0, "scaling mode",
> +				    hweight32(scaling_mode_mask));
> +
> +	for (i = 0; i < ARRAY_SIZE(drm_scaling_mode_enum_list); i++)
> +		if (BIT(i) & scaling_mode_mask) {

nit: You can save a level of indentation if you do:

if (!(BIT(i) & scaling_mode_mask))
        continue;

> +			int ret;
> +
> +			ret = drm_property_add_enum(scaling_mode_property, i,
> +						    drm_scaling_mode_enum_list[i].type,
> +						    drm_scaling_mode_enum_list[i].name);
> +
> +			if (ret) {
> +				drm_property_destroy(dev, scaling_mode_property);
> +

nit: extra line

> +				return ret;
> +			}
> +		}
> +
> +	drm_object_attach_property(&connector->base,
> +				   scaling_mode_property, default_scaler);
> +
> +	connector->scaling_mode_property = scaling_mode_property;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_mode_connector_attach_custom_scaling_mode_property);
> +
> +/**
>   * drm_mode_create_aspect_ratio_property - create aspect ratio property
>   * @dev: DRM device
>   *
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 934143720e5a..f55ae02135c2 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -770,6 +770,8 @@ struct drm_connector {
>  	struct drm_property_blob *edid_blob_ptr;
>  	struct drm_object_properties properties;
>  
> +	struct drm_property *scaling_mode_property;
> +

Document please

>  	/**
>  	 * @path_blob_ptr:
>  	 *
> @@ -969,6 +971,9 @@ int drm_mode_create_tv_properties(struct drm_device *dev,
>  				  unsigned int num_modes,
>  				  const char * const modes[]);
>  int drm_mode_create_scaling_mode_property(struct drm_device *dev);
> +int drm_mode_connector_attach_custom_scaling_mode_property(struct drm_connector *connector,
> +							   u32 scaling_mode_mask,
> +							   u32 default_value);
>  int drm_mode_create_aspect_ratio_property(struct drm_device *dev);
>  int drm_mode_create_suggested_offset_properties(struct drm_device *dev);
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS