Re: [Intel-gfx] [PATCH 05/19] drm/atomic: Make add_affected_connectors look at crtc_state.

[Daniel Vetter]

On Thu, Nov 03, 2016 at 05:32:42PM +0200, Ville Syrjälä wrote:
> On Mon, Oct 17, 2016 at 02:37:04PM +0200, Maarten Lankhorst wrote:
> > This kills another dereference of connector->state. connector_mask
> > holds all unchanged connectors at least and any changed connectors
> > are already in state anyway.
> > 
> > Signed-off-by: Maarten Lankhorst 
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 11 ---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 120775fcfb70..1915ec44f7eb 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1230,8 +1230,13 @@ drm_atomic_add_affected_connectors(struct 
> > drm_atomic_state *state,
> > struct drm_mode_config *config = >dev->mode_config;
> > struct drm_connector *connector;
> > struct drm_connector_state *conn_state;
> > +   struct drm_crtc_state *crtc_state;
> > int ret;
> >  
> > +   crtc_state = drm_atomic_get_crtc_state(state, crtc);
> > +   if (IS_ERR(crtc_state))
> > +   return PTR_ERR(crtc_state);
> > +
> > ret = drm_modeset_lock(>connection_mutex, state->acquire_ctx);
> > if (ret)
> > return ret;
> > @@ -1240,11 +1245,11 @@ drm_atomic_add_affected_connectors(struct 
> > drm_atomic_state *state,
> >  crtc->base.id, crtc->name, state);
> >  
> > /*
> > -* Changed connectors are already in @state, so only need to look at the
> > -* current configuration.
> > +* Changed connectors are already in @state, so only need to look
> > +* at the connector_mask in crtc_state.
> >  */
> > drm_for_each_connector(connector, state->dev) {
> > -   if (connector->state->crtc != crtc)
> > +   if (!(crtc_state->connector_mask & (1 << 
> > drm_connector_index(connector
> > continue;
> 
> So this being the new crtc state, connector_mask will include all newly 
> enabled
> connectors (these will have been already added to the top level state), and
> also any connector that was already enabled and isn't getting disabled (these
> are the ones we need to explicitly add to the top level state here).
> 
> /me wishes the top level state would be renamed to drm_atomic_transaction or 
> something...

+1 on drm_atomic_transaction, that's an awesome name. Not sure how bad it
would be to roll this out though ...
-Daniel

> 
> Any connector that is getting disabled will already have been added to
> the top level state as well, so them not being included in the new crtc
> state's connectors_mask is fine.
> 
> Reviewed-by: Ville Syrjälä 
> 
> >  
> > conn_state = drm_atomic_get_connector_state(state, connector);
> > -- 
> > 2.7.4
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 05/19] drm/atomic: Make add_affected_connectors look at crtc_state.

[Ville Syrjälä]

On Mon, Oct 17, 2016 at 02:37:04PM +0200, Maarten Lankhorst wrote:
> This kills another dereference of connector->state. connector_mask
> holds all unchanged connectors at least and any changed connectors
> are already in state anyway.
> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/drm_atomic.c | 11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 120775fcfb70..1915ec44f7eb 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1230,8 +1230,13 @@ drm_atomic_add_affected_connectors(struct 
> drm_atomic_state *state,
>   struct drm_mode_config *config = >dev->mode_config;
>   struct drm_connector *connector;
>   struct drm_connector_state *conn_state;
> + struct drm_crtc_state *crtc_state;
>   int ret;
>  
> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
> +
>   ret = drm_modeset_lock(>connection_mutex, state->acquire_ctx);
>   if (ret)
>   return ret;
> @@ -1240,11 +1245,11 @@ drm_atomic_add_affected_connectors(struct 
> drm_atomic_state *state,
>crtc->base.id, crtc->name, state);
>  
>   /*
> -  * Changed connectors are already in @state, so only need to look at the
> -  * current configuration.
> +  * Changed connectors are already in @state, so only need to look
> +  * at the connector_mask in crtc_state.
>*/
>   drm_for_each_connector(connector, state->dev) {
> - if (connector->state->crtc != crtc)
> + if (!(crtc_state->connector_mask & (1 << 
> drm_connector_index(connector
>   continue;

So this being the new crtc state, connector_mask will include all newly enabled
connectors (these will have been already added to the top level state), and
also any connector that was already enabled and isn't getting disabled (these
are the ones we need to explicitly add to the top level state here).

/me wishes the top level state would be renamed to drm_atomic_transaction or 
something...

Any connector that is getting disabled will already have been added to
the top level state as well, so them not being included in the new crtc
state's connectors_mask is fine.

Reviewed-by: Ville Syrjälä 

>  
>   conn_state = drm_atomic_get_connector_state(state, connector);
> -- 
> 2.7.4
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 05/19] drm/atomic: Make add_affected_connectors look at crtc_state.

[Maarten Lankhorst]

This kills another dereference of connector->state. connector_mask
holds all unchanged connectors at least and any changed connectors
are already in state anyway.

Signed-off-by: Maarten Lankhorst 
---
 drivers/gpu/drm/drm_atomic.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 120775fcfb70..1915ec44f7eb 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1230,8 +1230,13 @@ drm_atomic_add_affected_connectors(struct 
drm_atomic_state *state,
struct drm_mode_config *config = >dev->mode_config;
struct drm_connector *connector;
struct drm_connector_state *conn_state;
+   struct drm_crtc_state *crtc_state;
int ret;
 
+   crtc_state = drm_atomic_get_crtc_state(state, crtc);
+   if (IS_ERR(crtc_state))
+   return PTR_ERR(crtc_state);
+
ret = drm_modeset_lock(>connection_mutex, state->acquire_ctx);
if (ret)
return ret;
@@ -1240,11 +1245,11 @@ drm_atomic_add_affected_connectors(struct 
drm_atomic_state *state,
 crtc->base.id, crtc->name, state);
 
/*
-* Changed connectors are already in @state, so only need to look at the
-* current configuration.
+* Changed connectors are already in @state, so only need to look
+* at the connector_mask in crtc_state.
 */
drm_for_each_connector(connector, state->dev) {
-   if (connector->state->crtc != crtc)
+   if (!(crtc_state->connector_mask & (1 << 
drm_connector_index(connector
continue;
 
conn_state = drm_atomic_get_connector_state(state, connector);
-- 
2.7.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx