Re: [Intel-gfx] [PATCH v4 2/9] drm/i915: Add plumbing for digital connector state, v2.

[Maarten Lankhorst]

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 
>> ---
>>  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.
>> + * 

Re: [Intel-gfx] [PATCH v4 2/9] drm/i915: Add plumbing for digital connector state, v2.

[Daniel Vetter]

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 
> ---
>  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.
> +