On Mon, May 07, 2018 at 04:44:32PM +0200, Boris Brezillon wrote:
> We have 3 drivers defining the "underscan", "underscan hborder" and
> "underscan vborder" properties (radeon, amd and nouveau) and we are
> about to add the same kind of thing in VC4.
>
> Define generic underscan props and add new fields to the drm_connector
> state so that the property parsing logic can be shared by all DRM
> drivers.
>
> A driver can now attach underscan properties to its connector through
> the drm_connector_attach_underscan_properties() helper, and can
> check/apply the underscan setup based on the
> drm_connector_state->underscan fields.
>
> Signed-off-by: Boris Brezillon <boris.brezillon at bootlin.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 12 ++++
> drivers/gpu/drm/drm_connector.c | 120 ++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_connector.h | 78 ++++++++++++++++++++++++++
> 3 files changed, 210 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index dc850b4b6e21..b7312bd172c9 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1278,6 +1278,12 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector,
> return -EINVAL;
> }
> state->content_protection = val;
> + } else if (property == connector->underscan_mode_property) {
> + state->underscan.mode = val;
> + } else if (property == connector->underscan_hborder_property) {
> + state->underscan.hborder = val;
> + } else if (property == connector->underscan_vborder_property) {
> + state->underscan.vborder = val;
> } else if (connector->funcs->atomic_set_property) {
> return connector->funcs->atomic_set_property(connector,
> state, property, val);
> @@ -1359,6 +1365,12 @@ drm_atomic_connector_get_property(struct drm_connector *connector,
> *val = state->scaling_mode;
> } else if (property == connector->content_protection_property) {
> *val = state->content_protection;
> + } else if (property == connector->underscan_mode_property) {
> + *val = state->underscan.mode;
> + } else if (property == connector->underscan_hborder_property) {
> + *val = state->underscan.hborder;
> + } else if (property == connector->underscan_vborder_property) {
> + *val = state->underscan.vborder;
> } else if (connector->funcs->atomic_get_property) {
> return connector->funcs->atomic_get_property(connector,
> state, property, val);
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index dfc8ca1e9413..9937390b8a25 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -914,6 +914,31 @@ DRM_ENUM_NAME_FN(drm_get_content_protection_name, drm_cp_enum_list)
> * can also expose this property to external outputs, in which case they
> * must support "None", which should be the default (since external screens
> * have a built-in scaler).
I think a new section here would be good, to make it more obvious this is
a group of properties. Plus a reference to
drm_connector_attach_underscan_properties().
> + *
> + * underscan:
> + * This properties defines whether underscan is activated or not, and when
> + * it is activated, how the horizontal and vertical borders are calculated:
> + *
> + * off:
> + * Underscan is disabled. The output image shouldn't be scaled to
> + * take screen borders into account.
> + * on:
> + * Underscan is activated and horizontal and vertical borders are
> + * specified through the "underscan hborder" and
> + * "underscan vborder" properties.
> + * auto:
> + * Underscan is activated and horizontal and vertical borders are
> + * automatically chosen by the driver.
> + *
> + * underscan hborder:
> + * Horizontal border expressed in pixels. The border is symmetric, which
> + * means you'll have half of this value placed on the left and the other
> + * half on the right.
> + *
> + * underscan vborder:
> + * Vertical border expressed in pixels. The border is symmetric, which
> + * means you'll have half of this value placed on the top and the other
> + * half on the bottom.
> */
>
> int drm_connector_create_standard_properties(struct drm_device *dev)
> @@ -1108,6 +1133,101 @@ int drm_mode_create_tv_properties(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_mode_create_tv_properties);
>
> +static const struct drm_prop_enum_list drm_underscan_mode_enum_list[] = {
> + { DRM_UNDERSCAN_OFF, "off" },
> + { DRM_UNDERSCAN_ON, "on" },
> + { DRM_UNDERSCAN_AUTO, "auto" },
> +};
> +
> +/**
> + * drm_connector_attach_underscan_properties - attach atomic underscan
> + * properties
> + * @connector: connector to attach underscan mode properties on.
> + * @mode_mask: bitmask of %DRM_UNDERSCAN_XX modes encoding the supported
> + * underscan modes.
> + * @max_hborder: maximum size of the horizontal border expressed in pixels.
> + * Should be > 0.
> + * @max_vborder: maximum size of the vertical border expressed in pixels.
> + * Should be > 0.
> + *
> + * This is used to add support for underscan to atomic drivers.
> + * The underscan config will be set to &drm_connector_state.underscan
> + * and can be used from &drm_connector_helper_funcs->atomic_check for
> + * validation.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int drm_connector_attach_underscan_properties(struct drm_connector *connector,
> + u32 mode_mask, u64 max_hborder,
> + u64 max_vborder)
> +{
> + unsigned int i, nmodes = ARRAY_SIZE(drm_underscan_mode_enum_list);
> + struct drm_device *dev = connector->dev;
> + struct drm_property *prop;
> +
> + if (!max_hborder || !max_vborder)
> + return -EINVAL;
> +
> + if (!hweight32(mode_mask) || (mode_mask & ~GENMASK(nmodes - 1, 0)))
> + return -EINVAL;
> +
> + prop = drm_property_create(dev, DRM_MODE_PROP_ENUM, "underscan",
> + hweight32(mode_mask));
> + if (!prop)
> + return -ENOMEM;
> +
> + for (i = 0; i < ARRAY_SIZE(drm_underscan_mode_enum_list); i++) {
> + const struct drm_prop_enum_list *entry;
> + int ret;
> +
> + if (!(BIT(i) & mode_mask))
> + continue;
> +
> + entry = &drm_underscan_mode_enum_list[i];
> + ret = drm_property_add_enum(prop, entry->type, entry->name);
> + if (ret)
> + goto err_free_mode_prop;
> + }
> +
> + connector->underscan_mode_property = prop;
> +
> + prop = drm_property_create_range(dev, 0, "underscan hborder", 0,
> + max_hborder);
> + if (!prop)
> + goto err_free_mode_prop;
> +
> + connector->underscan_hborder_property = prop;
> +
> + prop = drm_property_create_range(dev, 0, "underscan vborder", 0,
> + max_vborder);
> + if (!prop)
> + goto err_free_hborder_prop;
> +
> + connector->underscan_vborder_property = prop;
> +
> + drm_object_attach_property(&connector->base,
> + connector->underscan_mode_property,
> + DRM_UNDERSCAN_OFF);
> + drm_object_attach_property(&connector->base,
> + connector->underscan_hborder_property, 0);
> + drm_object_attach_property(&connector->base,
> + connector->underscan_vborder_property, 0);
> +
> + return 0;
> +
> +err_free_hborder_prop:
> + drm_property_destroy(dev, connector->underscan_hborder_property);
> + connector->underscan_hborder_property = NULL;
> +
> +err_free_mode_prop:
> + drm_property_destroy(dev, connector->underscan_mode_property);
> + connector->underscan_mode_property = NULL;
> +
> + return -ENOMEM;
> +}
> +EXPORT_SYMBOL(drm_connector_attach_underscan_properties);
> +
> /**
> * drm_mode_create_scaling_mode_property - create scaling mode property
> * @dev: DRM device
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 675cc3f8cf85..46380ee6d4ee 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -372,6 +372,52 @@ struct drm_tv_connector_state {
> unsigned int hue;
> };
>
> +/**
> + * enum drm_underscan_mode - Underscan mode
> + *
> + * This enum is used to track the underscan mode.
> + *
> + * @DRM_UNDERSCAN_OFF: No underscan applied, the output image will be unchanged
> + * @DRM_UNDERSCAN_ON: Underscan is enabled, and horizontal/vertical border size
> + * are specified through the &struct_drm_underscan->hborder
> + * and &struct_drm_underscan->vborder fields.
> + * @DRM_UNDERSCAN_AUTO: Underscan is enabled and &struct_drm_underscan->hborder
> + * and &struct_drm_underscan->vborder are guessed by the
> + * driver.
> + */
> +enum drm_underscan_mode {
> + DRM_UNDERSCAN_OFF,
> + DRM_UNDERSCAN_ON,
> + DRM_UNDERSCAN_AUTO,
> +};
> +
> +/**
> + * struct drm_underscan - Underscan properties attached to a connector state
> + *
> + * This can be used to tell the CRTC how the image should be scaled/placed in
> + * order to cover fit in the display connected through this connector. Most of
> + * the time used to address situations where the display borders are hidden.
> + * Can also be used to compensate overscan done on the display side.
> + */
> +struct drm_underscan {
I'd call this drm_underscan_state, similar to how we have
drm_tv_connector_state.
> + /**
> + * @mode: Underscan mode.
> + */
> + enum drm_underscan_mode mode;
> +
> + /**
> + * @hborder: Horizontal border. This values encodes both the left and
> + * right borders: left_border = right_border = hborder / 2.
> + */
> + unsigned int hborder;
> +
> + /**
> + * @vborder: Vertical border. This values encodes both the top and
> + * bottom borders: top_border = bottom_border = vborder / 2.
> + */
> + unsigned int vborder;
> +};
> +
> /**
> * struct drm_connector_state - mutable connector state
> * @connector: backpointer to the connector
> @@ -429,6 +475,12 @@ struct drm_connector_state {
> * protection. This is most commonly used for HDCP.
> */
> unsigned int content_protection;
> +
> + /**
> + * @underscan: Underscan information. Most commonly used to adjust
> + * image when the display is doing overscan.
> + */
> + struct drm_underscan underscan;
> };
>
> /**
> @@ -892,6 +944,29 @@ struct drm_connector {
> */
> struct drm_property_blob *tile_blob_ptr;
>
> + /**
> + * @underscan_mode_property: Optional connector underscan mode. Used by
> + * the driver to scale the output image and compensate an overscan done
> + * on the display side.
> + */
> + struct drm_property *underscan_mode_property;
> +
> + /**
> + * @underscan_hborder_property: Optional connector underscan horizontal
> + * border (expressed in pixels). Used by the driver to adjust the
> + * output image position and compensate an overscan done on the display
> + * side.
> + */
> + struct drm_property *underscan_hborder_property;
> +
> + /**
> + * @underscan_hborder_property: Optional connector underscan vertical
> + * border (expressed in pixels). Used by the driver to adjust the
> + * output image position and compensate an overscan done on the display
> + * side.
> + */
> + struct drm_property *underscan_vborder_property;
Since you don't allow drivers to have a per-connector value range for
these properties these should be in drm_mode_config I think.
Yes the content_protection_property should probably have the same
treatment. Note that drm_property_blob does not derive from drm_property,
so that one is ok to be there. And the scaling property is generally
per-output.
-Daniel
> +
> /* should we poll this connector for connects and disconnects */
> /* hot plug detectable */
> #define DRM_CONNECTOR_POLL_HPD (1 << 0)
> @@ -1088,6 +1163,9 @@ int drm_mode_create_dvi_i_properties(struct drm_device *dev);
> int drm_mode_create_tv_properties(struct drm_device *dev,
> unsigned int num_modes,
> const char * const modes[]);
> +int drm_connector_attach_underscan_properties(struct drm_connector *connector,
> + u32 mode_mask, u64 max_hborder,
> + u64 max_vborder);
> int drm_mode_create_scaling_mode_property(struct drm_device *dev);
> int drm_connector_attach_scaling_mode_property(struct drm_connector *connector,
> u32 scaling_mode_mask);
> --
> 2.14.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
