On 19-04-17 17:48, Daniel Vetter wrote:
> On Wed, Apr 12, 2017 at 12:50:00PM +0200, Maarten Lankhorst wrote:
>> Some atomic properties are common between the various kinds of
>> connectors, for example a lot of them use panel fitting mode.
>> It makes sense to put a lot of it in a common place, so each
>> connector can use it while they're being converted.
>>
>> Implement the properties required for the connectors:
>> - scaling mode property
>> - force audio property
>> - broadcast rgb
>> - aspect ratio
>>
>> While at it, make clear that intel_digital_connector_atomic_get_property
>> is a hack that has to be removed when all connector properties
>> are converted to atomic.
>>
>> Changes since v1:
>> - Scaling mode and aspect ratio are partly handled in core now.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_atomic.c | 142
>> +++++++++++++++++++++++++++++++++--
>> drivers/gpu/drm/i915/intel_display.c | 14 +++-
>> drivers/gpu/drm/i915/intel_dp.c | 2 +-
>> drivers/gpu/drm/i915/intel_drv.h | 27 ++++++-
>> drivers/gpu/drm/i915/intel_dsi.c | 2 +-
>> drivers/gpu/drm/i915/intel_dvo.c | 2 +-
>> drivers/gpu/drm/i915/intel_lvds.c | 2 +-
>> drivers/gpu/drm/i915/intel_panel.c | 4 +-
>> 8 files changed, 180 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index 50fb1f76cc5f..eb72166675f0 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> Refactoring idea for the future: intel_atomic.c makes 0 sense, just makes
> sure we have lots of functionality spread all over imo. I think following
> the split-up model I've done in the drm core would be a lot better, with
> intel_connector/encoder/crtc/plane.c for the shared helper functions. And
> then maybe separate files for the per-platform stuff, e.g. i9xx_crtc.c,
> pch_crtc.c and so on.
>
>> @@ -36,7 +36,7 @@
>> #include "intel_drv.h"
>>
>> /**
>> - * intel_connector_atomic_get_property - fetch connector property value
>> + * intel_connector_atomic_get_property - fetch legacy connector property
>> value
>> * @connector: connector to fetch property for
>> * @state: state containing the property value
>> * @property: property to look up
>> @@ -45,12 +45,14 @@
>> * The DRM core does not store shadow copies of properties for
>> * atomic-capable drivers. This entrypoint is used to fetch
>> * the current value of a driver-specific connector property.
>> + *
>> + * This is a intermediary solution until all connectors are
>> + * converted to support full atomic properties.
>> */
>> -int
>> -intel_connector_atomic_get_property(struct drm_connector *connector,
>> - const struct drm_connector_state *state,
>> - struct drm_property *property,
>> - uint64_t *val)
>> +int intel_connector_atomic_get_property(struct drm_connector *connector,
>> + const struct drm_connector_state *state,
>> + struct drm_property *property,
>> + uint64_t *val)
>> {
>> int i;
>>
>> @@ -73,7 +75,133 @@ intel_connector_atomic_get_property(struct drm_connector
>> *connector,
>> return -EINVAL;
>> }
>>
>> -/*
>> +/**
>> + * intel_digital_connector_atomic_get_property - hook for
>> connector->atomic_get_property.
>> + * @connector: Connector to get the property for.
>> + * @state: Connector state to retrieve the property from.
>> + * @property: Property to retrieve.
>> + * @val: Return value for the property.
>> + *
>> + * Returns the atomic property value for a digital connector.
>> + */
>> +int intel_digital_connector_atomic_get_property(struct drm_connector
>> *connector,
>> + const struct
>> drm_connector_state *state,
>> + struct drm_property *property,
>> + uint64_t *val)
>> +{
>> + struct drm_device *dev = connector->dev;
>> + struct drm_i915_private *dev_priv = to_i915(dev);
>> + struct intel_digital_connector_state *intel_conn_state =
>> + to_intel_digital_connector_state(state);
>> +
>> + if (property == dev_priv->force_audio_property)
>> + *val = intel_conn_state->force_audio;
>> + else if (property == dev_priv->broadcast_rgb_property)
>> + *val = intel_conn_state->broadcast_rgb;
>> + else {
>> + DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * intel_digital_connector_atomic_set_property - hook for
>> connector->atomic_set_property.
>> + * @connector: Connector to set the property for.
>> + * @state: Connector state to set the property on.
>> + * @property: Property to set.
>> + * @val: New value for the property.
>> + *
>> + * Sets the atomic property value for a digital connector.
>> + */
>> +int intel_digital_connector_atomic_set_property(struct drm_connector
>> *connector,
>> + struct drm_connector_state
>> *state,
>> + struct drm_property *property,
>> + uint64_t val)
>> +{
>> + struct drm_device *dev = connector->dev;
>> + struct drm_i915_private *dev_priv = to_i915(dev);
>> + struct intel_digital_connector_state *intel_conn_state =
>> + to_intel_digital_connector_state(state);
>> +
>> + if (property == dev_priv->force_audio_property) {
>> + intel_conn_state->force_audio = val;
>> + return 0;
>> + }
>> +
>> + if (property == dev_priv->broadcast_rgb_property) {
>> + intel_conn_state->broadcast_rgb = val;
>> + return 0;
>> + }
>> +
>> + DRM_DEBUG_ATOMIC("Unknown property %s\n", property->name);
>> + return -EINVAL;
>> +}
>> +
>> +int intel_digital_connector_atomic_check(struct drm_connector *conn,
>> + struct drm_connector_state *new_state)
>> +{
>> + struct intel_digital_connector_state *new_conn_state =
>> + to_intel_digital_connector_state(new_state);
>> + struct drm_connector_state *old_state =
>> + drm_atomic_get_old_connector_state(new_state->state, conn);
>> + struct intel_digital_connector_state *old_conn_state =
>> + to_intel_digital_connector_state(old_state);
>> + struct intel_connector *intel_connector = to_intel_connector(conn);
>> + struct drm_crtc_state *crtc_state;
>> +
>> + if (intel_connector->panel.allowed_fitting_mode_mask &&
>> + (BIT(new_state->scaling_mode) &
>> ~intel_connector->panel.allowed_fitting_mode_mask)) {
>> + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] Panel fitting mode %u not
>> allowed.\n",
>> + conn->base.id, conn->name,
>> new_state->scaling_mode);
>> + return -EINVAL;
>> + }
> Range-checking like this should be done by correctling registering the
> proeprty ... Separate patch to fix that (if it's indeed broken)?
Not if there's hw with a combination of eDP, DSI or LVDS. Even if it' s just
prerelease testing hw.
This is matching the current behavior for setprop, so not much difference here.
:)
>> +
>> + if (!new_state->crtc)
>> + return 0;
>> +
>> + crtc_state = drm_atomic_get_new_crtc_state(new_state->state,
>> new_state->crtc);
>> +
>> + /* Invisible to fastset since it's a pure connector property */
>> + if (new_conn_state->base.scaling_mode !=
>> old_conn_state->base.scaling_mode)
>> + crtc_state->connectors_changed = true;
> Hm, I thought we put all the pfit register settings into the crtc state,
> so scaling_mode changes should also be possible to handle by fastset?
Quite possible, I didn't check if it was the case though, so I erred on the
side of caution.
> Looks all reasonable otherwise.
> -Daniel
>
>> +
>> + /*
>> + * These properties are handled by fastset, and might not end
>> + * up in a modeset.
>> + */
>> + if (new_conn_state->force_audio != old_conn_state->force_audio ||
>> + new_conn_state->broadcast_rgb != old_conn_state->broadcast_rgb ||
>> + new_conn_state->base.picture_aspect_ratio !=
>> old_conn_state->base.picture_aspect_ratio)
>> + crtc_state->mode_changed = true;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * intel_digital_connector_duplicate_state - duplicate connector state
>> + * @connector: digital connector
>> + *
>> + * Allocates and returns a copy of the connector state (both common and
>> + * digital connector specific) for the specified connector.
>> + *
>> + * Returns: The newly allocated connector state, or NULL on failure.
>> + */
>> +struct drm_connector_state *
>> +intel_digital_connector_duplicate_state(struct drm_connector *connector)
>> +{
>> + struct intel_digital_connector_state *state;
>> +
>> + state = kmemdup(connector->state, sizeof(*state), GFP_KERNEL);
>> + if (!state)
>> + return NULL;
>> +
>> + __drm_atomic_helper_connector_duplicate_state(connector, &state->base);
>> + return &state->base;
>> +}
>> +
>> +/**
>> * intel_crtc_duplicate_state - duplicate crtc state
>> * @crtc: drm crtc
>> *
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 48a546210d8b..5eb4ddb04797 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5956,11 +5956,21 @@ static void intel_connector_verify_state(struct
>> intel_connector *connector)
>>
>> int intel_connector_init(struct intel_connector *connector)
>> {
>> - drm_atomic_helper_connector_reset(&connector->base);
>> + struct intel_digital_connector_state *conn_state;
>>
>> - if (!connector->base.state)
>> + /*
>> + * Allocate enough memory to hold intel_digital_connector_state,
>> + * This might be a few bytes too many, but for connectors that don't
>> + * need it we'll free the state and allocate a smaller one on the first
>> + * succesful commit anyway.
>> + */
>> + conn_state = kzalloc(sizeof(*conn_state), GFP_KERNEL);
>> + if (!conn_state)
>> return -ENOMEM;
>>
>> + __drm_atomic_helper_connector_reset(&connector->base,
>> + &conn_state->base);
>> +
>> return 0;
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c
>> index 944b1449b7f9..0afd8f3037f0 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -5896,7 +5896,7 @@ static bool intel_edp_init_connector(struct intel_dp
>> *intel_dp,
>> pipe_name(pipe));
>> }
>>
>> - intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
>> + intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode,
>> 0);
>> intel_connector->panel.backlight.power = intel_edp_backlight_power;
>> intel_panel_setup_backlight(connector, pipe);
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index f78d7c5f3805..4df9e4531edc 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -262,6 +262,7 @@ struct intel_panel {
>> struct drm_display_mode *fixed_mode;
>> struct drm_display_mode *downclock_mode;
>> int fitting_mode;
>> + unsigned allowed_fitting_mode_mask;
>>
>> /* backlight */
>> struct {
>> @@ -323,6 +324,15 @@ struct intel_connector {
>> struct intel_dp *mst_port;
>> };
>>
>> +struct intel_digital_connector_state {
>> + struct drm_connector_state base;
>> +
>> + enum hdmi_force_audio force_audio;
>> + int broadcast_rgb;
>> +};
>> +
>> +#define to_intel_digital_connector_state(x) container_of(x, struct
>> intel_digital_connector_state, base)
>> +
>> struct dpll {
>> /* given values */
>> int n;
>> @@ -1668,7 +1678,8 @@ void intel_overlay_reset(struct drm_i915_private
>> *dev_priv);
>> /* intel_panel.c */
>> int intel_panel_init(struct intel_panel *panel,
>> struct drm_display_mode *fixed_mode,
>> - struct drm_display_mode *downclock_mode);
>> + struct drm_display_mode *downclock_mode,
>> + unsigned allowed_fitting_mode_mask);
>> void intel_panel_fini(struct intel_panel *panel);
>> void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
>> struct drm_display_mode *adjusted_mode);
>> @@ -1879,6 +1890,20 @@ int intel_connector_atomic_get_property(struct
>> drm_connector *connector,
>> const struct drm_connector_state *state,
>> struct drm_property *property,
>> uint64_t *val);
>> +
>> +int intel_digital_connector_atomic_get_property(struct drm_connector
>> *connector,
>> + const struct
>> drm_connector_state *state,
>> + struct drm_property *property,
>> + uint64_t *val);
>> +int intel_digital_connector_atomic_set_property(struct drm_connector
>> *connector,
>> + struct drm_connector_state
>> *state,
>> + struct drm_property *property,
>> + uint64_t val);
>> +int intel_digital_connector_atomic_check(struct drm_connector *conn,
>> + struct drm_connector_state *new_state);
>> +struct drm_connector_state *
>> +intel_digital_connector_duplicate_state(struct drm_connector *connector);
>> +
>> struct drm_crtc_state *intel_crtc_duplicate_state(struct drm_crtc *crtc);
>> void intel_crtc_destroy_state(struct drm_crtc *crtc,
>> struct drm_crtc_state *state);
>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c
>> b/drivers/gpu/drm/i915/intel_dsi.c
>> index 3ffe8b1f1d48..e787142025ac 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> @@ -1853,7 +1853,7 @@ void intel_dsi_init(struct drm_i915_private *dev_priv)
>> connector->display_info.width_mm = fixed_mode->width_mm;
>> connector->display_info.height_mm = fixed_mode->height_mm;
>>
>> - intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
>> + intel_panel_init(&intel_connector->panel, fixed_mode, NULL, 0);
>> intel_panel_setup_backlight(connector, INVALID_PIPE);
>>
>> intel_dsi_add_properties(intel_connector);
>> diff --git a/drivers/gpu/drm/i915/intel_dvo.c
>> b/drivers/gpu/drm/i915/intel_dvo.c
>> index c1544a53095d..0b723157c38b 100644
>> --- a/drivers/gpu/drm/i915/intel_dvo.c
>> +++ b/drivers/gpu/drm/i915/intel_dvo.c
>> @@ -554,7 +554,7 @@ void intel_dvo_init(struct drm_i915_private *dev_priv)
>> */
>> intel_panel_init(&intel_connector->panel,
>> intel_dvo_get_current_mode(connector),
>> - NULL);
>> + NULL, 0);
>> intel_dvo->panel_wants_dither = true;
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lvds.c
>> b/drivers/gpu/drm/i915/intel_lvds.c
>> index 8b942ef2b3ec..919d84290774 100644
>> --- a/drivers/gpu/drm/i915/intel_lvds.c
>> +++ b/drivers/gpu/drm/i915/intel_lvds.c
>> @@ -1181,7 +1181,7 @@ void intel_lvds_init(struct drm_i915_private *dev_priv)
>> out:
>> mutex_unlock(&dev->mode_config.mutex);
>>
>> - intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
>> + intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode,
>> 0);
>> intel_panel_setup_backlight(connector, INVALID_PIPE);
>>
>> lvds_encoder->is_dual_link = compute_is_dual_link_lvds(lvds_encoder);
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c
>> b/drivers/gpu/drm/i915/intel_panel.c
>> index cb50c527401f..c6ce15550e5e 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -1801,12 +1801,14 @@ intel_panel_init_backlight_funcs(struct intel_panel
>> *panel)
>>
>> int intel_panel_init(struct intel_panel *panel,
>> struct drm_display_mode *fixed_mode,
>> - struct drm_display_mode *downclock_mode)
>> + struct drm_display_mode *downclock_mode,
>> + unsigned allowed_fitting_mode_mask)
>> {
>> intel_panel_init_backlight_funcs(panel);
>>
>> panel->fixed_mode = fixed_mode;
>> panel->downclock_mode = downclock_mode;
>> + panel->allowed_fitting_mode_mask = allowed_fitting_mode_mask;
>>
>> return 0;
>> }
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
