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@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]