[Intel-gfx] [PATCH v3 4/7] drm/atomic: Fix atomic helpers to use the new iterator macros.

[Laurent Pinchart]

Hi Maarten,

On Wednesday 18 Jan 2017 15:49:56 Maarten Lankhorst wrote:
> Op 17-01-17 om 02:01 schreef Laurent Pinchart:
> > On Monday 16 Jan 2017 10:37:41 Maarten Lankhorst wrote:
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >> ---
> >> 
> >>  drivers/gpu/drm/drm_atomic_helper.c | 293 +++++++++++++++---------------
> >>  1 file changed, 152 insertions(+), 141 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> >> b/drivers/gpu/drm/drm_atomic_helper.c index d153e8a72921..b26cf786ce12
> >> 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > 
> > [snip]
> > 
> >> @@ -245,22 +246,22 @@ steal_encoder(struct drm_atomic_state *state,
> >>  {
> >>  	struct drm_crtc_state *crtc_state;
> >>  	struct drm_connector *connector;
> >> -	struct drm_connector_state *connector_state;
> >> +	struct drm_connector_state *old_connector_state, *new_connector_state;
> > 
> > The kernel favours one variable declaration per line, and I think this
> > file does as well, at least mostly (this comment holds for the rest of
> > this patch and the other patches in the series).
> 
> This is the first I've heard of it..
> 
> ~/nfs/linux$ git grep 'int i,'  | wc -l
> 8490
> ~/nfs/linux$ git grep 'int i;'  | wc -l
> 23636
> 
> Based on this, I don't think it's uncommon to have multiple declarations per
> line.

It's not uncommon, no. I still think it hinders readability though, especially 
for long statements like here.

> >>  	int i;

[snip]

> >> @@ -974,18 +977,18 @@ void
> >> drm_atomic_helper_commit_modeset_enables(struct
> >> drm_device *dev,
> >> 		}
> >>  	}
> >> 
> >> -	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
> >> +	for_each_new_connector_in_state(old_state, connector, conn_state, i) {
> > 
> > Not strictly related to this patch, but I've been wondering how this
> > works, given that we need to loop over connectors in the new state, not
> > the old state. After some investigation I've realized that both the old
> > and new states contain the same list of objects, as we don't keep a copy
> > of the old global atomic state. old_state was the new state before the
> > state swap, and the swap operation sets state->/objects/[*].state to the
> > old state for all objects, but doesn't modify the object pointers. This is
> > possibly more of a question for Daniel, should this be captured in the
> > documentation somewhere (and is my understanding correct) ?
> 
> Yes. But that will go away soon after this patch + all drivers converted.

Will it ? Even with the new helpers we will have code looping over objects 
from the old state, like here. As the code intends on looping over objects in 
the new state this is confusing until you realize that the old state always 
contains the same objects as the new state. As Ville mentioned, 
drm_atomic_state would be better named drm_atomic_transaction, this would 
remove the ambiguity.

> This is why get_state should not be called during atomic commit, and
> get_existing_state will be removed.

[snip]

-- 
Regards,

Laurent Pinchart