Re: [PATCH 11/13] drm/atomic: atomic connector properties

From: Daniel Vetter <daniel@ffwll.ch>
To: Rob Clark <robdclark@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 11/13] drm/atomic: atomic connector properties
Date: Wed, 17 Dec 2014 15:48:27 +0100	[thread overview]
Message-ID: <20141217144827.GR2711@phenom.ffwll.local> (raw)
In-Reply-To: <1418771141-16954-12-git-send-email-robdclark@gmail.com>

On Tue, Dec 16, 2014 at 06:05:39PM -0500, Rob Clark wrote:
> Expose the core connector state as properties so it can be updated via
> atomic ioctl.
> 
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Oh, didn't realize that you're sharing config->prop_crtc_id with plane and
connector. Please add a comment about that to drm_crtc.h.

Plus same comment about setting DRM_MODE_PROB_ATOMIC. With these two this
is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I wasn't to clear in my reply to the previous patch probably, but I think
the atomic filtering should be in an earlier patch. Best in the missing
one to add the modparam and file_priv->atomic_kms.
-Daniel

> ---
>  Documentation/DocBook/drm.tmpl | 11 +++++++++--
>  drivers/gpu/drm/drm_atomic.c   |  9 +++++++--
>  drivers/gpu/drm/drm_crtc.c     | 13 +++++++++----
>  3 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 282fa6b..15cb9b9 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2564,8 +2564,8 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >Description/Restrictions</td>
>  	</tr>
>  	<tr>
> -	<td rowspan="35" valign="top" >DRM</td>
> -	<td rowspan="4" valign="top" >Generic</td>
> +	<td rowspan="36" valign="top" >DRM</td>
> +	<td rowspan="5" valign="top" >Connector</td>
>  	<td valign="top" >“EDID”</td>
>  	<td valign="top" >BLOB | IMMUTABLE</td>
>  	<td valign="top" >0</td>
> @@ -2594,6 +2594,13 @@ void intel_crt_init(struct drm_device *dev)
>  	<td valign="top" >Contains tiling information for a connector.</td>
>  	</tr>
>  	<tr>
> +	<td valign="top" >“CRTC_ID”</td>
> +	<td valign="top" >OBJECT</td>
> +	<td valign="top" >DRM_MODE_OBJECT_CRTC</td>
> +	<td valign="top" >Connector</td>
> +	<td valign="top" >CRTC that connector is attached to (atomic)</td>
> +	</tr>
> +	<tr>
>  	<td rowspan="11" valign="top" >Plane</td>
>  	<td valign="top" >“type”</td>
>  	<td valign="top" >ENUM | IMMUTABLE</td>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c09a05a..71b48a0 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -595,7 +595,10 @@ int drm_atomic_connector_set_property(struct drm_connector *connector,
>  	struct drm_device *dev = connector->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
>  
> -	if (property == config->dpms_property) {
> +	if (property == config->prop_crtc_id) {
> +		struct drm_crtc *crtc = drm_crtc_find(dev, val);
> +		return drm_atomic_set_crtc_for_connector(state, crtc);
> +	} else if (property == config->dpms_property) {
>  		/* setting DPMS property requires special handling, which
>  		 * is done in legacy setprop path for us.  Disallow (for
>  		 * now?) atomic writes to DPMS property:
> @@ -629,7 +632,9 @@ int drm_atomic_connector_get_property(struct drm_connector *connector,
>  	struct drm_device *dev = connector->dev;
>  	struct drm_mode_config *config = &dev->mode_config;
>  
> -	if (property == config->dpms_property) {
> +	if (property == config->prop_crtc_id) {
> +		*val = (state->crtc) ? state->crtc->base.id : 0;
> +	} else if (property == config->dpms_property) {
>  		*val = connector->dpms;
>  	} else if (connector->funcs->atomic_get_property) {
>  		return connector->funcs->atomic_get_property(connector,
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index fd6f91d..39c3e06 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -844,6 +844,7 @@ int drm_connector_init(struct drm_device *dev,
>  		       const struct drm_connector_funcs *funcs,
>  		       int connector_type)
>  {
> +	struct drm_mode_config *config = &dev->mode_config;
>  	int ret;
>  	struct ida *connector_ida =
>  		&drm_connector_enum_list[connector_type].ida;
> @@ -882,16 +883,20 @@ int drm_connector_init(struct drm_device *dev,
>  
>  	/* We should add connectors at the end to avoid upsetting the connector
>  	 * index too much. */
> -	list_add_tail(&connector->head, &dev->mode_config.connector_list);
> -	dev->mode_config.num_connector++;
> +	list_add_tail(&connector->head, &config->connector_list);
> +	config->num_connector++;
>  
>  	if (connector_type != DRM_MODE_CONNECTOR_VIRTUAL)
>  		drm_object_attach_property(&connector->base,
> -					      dev->mode_config.edid_property,
> +					      config->edid_property,
>  					      0);
>  
>  	drm_object_attach_property(&connector->base,
> -				      dev->mode_config.dpms_property, 0);
> +				      config->dpms_property, 0);
> +
> +	if (has_atomic_properties(dev)) {
> +		drm_object_attach_property(&connector->base, config->prop_crtc_id, 0);
> +	}
>  
>  	connector->debugfs_entry = NULL;
>  
> -- 
> 2.1.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel