Re: [Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state
Op 03-11-16 om 16:11 schreef Ville Syrjälä: > On Wed, Nov 02, 2016 at 09:28:46AM +0100, Maarten Lankhorst wrote: >> Op 01-11-16 om 14:41 schreef Ville Syrjälä: >>> On Tue, Nov 01, 2016 at 02:34:00PM +0100, Maarten Lankhorst wrote: Op 01-11-16 om 14:09 schreef Ville Syrjälä: > On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote: >> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to >> replace the old for_each_xxx_in_state ones. This is useful for >1 flip >> depth and getting rid of all xxx->state dereferences. >> >> Signed-off-by: Maarten Lankhorst>> --- >> drivers/gpu/drm/drm_atomic.c | 6 +++ >> drivers/gpu/drm/drm_atomic_helper.c | 52 +-- >> drivers/gpu/drm/i915/intel_display.c | 11 ++--- >> include/drm/drm_atomic.h | 81 >> ++-- >> include/drm/drm_atomic_helper.h | 3 ++ >> 5 files changed, 142 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index 5dd70540219c..120775fcfb70 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state >> *state, >> return ERR_PTR(-ENOMEM); >> >> state->crtcs[index].state = crtc_state; >> +state->crtcs[index].old_state = crtc->state; >> +state->crtcs[index].new_state = crtc_state; >> state->crtcs[index].ptr = crtc; >> crtc_state->state = state; >> >> @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state >> *state, >> >> state->planes[index].state = plane_state; >> state->planes[index].ptr = plane; >> +state->planes[index].old_state = plane->state; >> +state->planes[index].new_state = plane_state; >> plane_state->state = state; >> >> DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n", >> @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct >> drm_atomic_state *state, >> >> drm_connector_reference(connector); >> state->connectors[index].state = connector_state; >> +state->connectors[index].old_state = connector->state; >> +state->connectors[index].new_state = connector_state; >> state->connectors[index].ptr = connector; >> connector_state->state = state; >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c >> b/drivers/gpu/drm/drm_atomic_helper.c >> index 07b432f43b98..fcb6e5b55217 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all); >> * >> * See also: >> * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(), >> - * drm_atomic_helper_resume() >> + * drm_atomic_helper_resume(), >> drm_atomic_helper_commit_duplicated_state() >> */ >> struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device >> *dev) >> { >> @@ -2536,6 +2536,52 @@ unlock: >> EXPORT_SYMBOL(drm_atomic_helper_suspend); >> >> /** >> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state >> + * @state: duplicated atomic state to commit >> + * @ctx: pointer to acquire_ctx to use for commit. >> + * @nonblock: commit the state non-blocking. >> + * >> + * The state returned by drm_atomic_helper_duplicate_state() and >> + * drm_atomic_helper_suspend() is partially invalid, and needs to >> + * be fixed up before commit. > So the old_state pointers are potentially invalid because whatever->state > may have changed since we duplicated the state? Yes when you use drm_atomic_helper_duplicate_state, and commit a different state before committing the duplicated state. This only happens during suspend/resume and gpu reset. >>> Should we maybe have something like drm_atomic_refresh_old_state(state)? >>> Would allow the caller then to check something in the old vs. new state >>> before committing? >> iirc that was the first approach I took, but then you shouldn't do anything >> with a duplicated state after >> creating a except commit it, so creating a commit_duplicated_state should be >> enough. >> >> Can you think of any case where you do the following? >> >> Duplicate state >> commit different state >> Look at duplicated state, tinker with it, commit it. > Not really. Oh, and we do still run the thing through the check phase > when we commit it, don't we? That sounds like it should let the driver > do whatever it needs to do. > So my only other concern is just the 'bool nonblock' parameter. It's a > bit
Re: [Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state
On Tue, Nov 01, 2016 at 02:34:00PM +0100, Maarten Lankhorst wrote: > Op 01-11-16 om 14:09 schreef Ville Syrjälä: > > On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote: > >> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to > >> replace the old for_each_xxx_in_state ones. This is useful for >1 flip > >> depth and getting rid of all xxx->state dereferences. > >> > >> Signed-off-by: Maarten Lankhorst> >> --- > >> drivers/gpu/drm/drm_atomic.c | 6 +++ > >> drivers/gpu/drm/drm_atomic_helper.c | 52 +-- > >> drivers/gpu/drm/i915/intel_display.c | 11 ++--- > >> include/drm/drm_atomic.h | 81 > >> ++-- > >> include/drm/drm_atomic_helper.h | 3 ++ > >> 5 files changed, 142 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > >> index 5dd70540219c..120775fcfb70 100644 > >> --- a/drivers/gpu/drm/drm_atomic.c > >> +++ b/drivers/gpu/drm/drm_atomic.c > >> @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state > >> *state, > >>return ERR_PTR(-ENOMEM); > >> > >>state->crtcs[index].state = crtc_state; > >> + state->crtcs[index].old_state = crtc->state; > >> + state->crtcs[index].new_state = crtc_state; > >>state->crtcs[index].ptr = crtc; > >>crtc_state->state = state; > >> > >> @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state > >> *state, > >> > >>state->planes[index].state = plane_state; > >>state->planes[index].ptr = plane; > >> + state->planes[index].old_state = plane->state; > >> + state->planes[index].new_state = plane_state; > >>plane_state->state = state; > >> > >>DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n", > >> @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state > >> *state, > >> > >>drm_connector_reference(connector); > >>state->connectors[index].state = connector_state; > >> + state->connectors[index].old_state = connector->state; > >> + state->connectors[index].new_state = connector_state; > >>state->connectors[index].ptr = connector; > >>connector_state->state = state; > >> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c > >> b/drivers/gpu/drm/drm_atomic_helper.c > >> index 07b432f43b98..fcb6e5b55217 100644 > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >> @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all); > >> * > >> * See also: > >> * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(), > >> - * drm_atomic_helper_resume() > >> + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state() > >> */ > >> struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) > >> { > >> @@ -2536,6 +2536,52 @@ unlock: > >> EXPORT_SYMBOL(drm_atomic_helper_suspend); > >> > >> /** > >> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state > >> + * @state: duplicated atomic state to commit > >> + * @ctx: pointer to acquire_ctx to use for commit. > >> + * @nonblock: commit the state non-blocking. > >> + * > >> + * The state returned by drm_atomic_helper_duplicate_state() and > >> + * drm_atomic_helper_suspend() is partially invalid, and needs to > >> + * be fixed up before commit. > > So the old_state pointers are potentially invalid because whatever->state > > may have changed since we duplicated the state? > > Yes when you use drm_atomic_helper_duplicate_state, and commit a different > state before committing the duplicated state. > This only happens during suspend/resume and gpu reset. Kerneldoc should imo mention that suspend/resume is the only case this is valid, and even then it depends upon the driver. Proper atomic commits never keep drm_atomic_state around when dropping locks, so this can't happen with an atomic ioctl. Please also directly cross-reference all these functions. -Daniel > > ~Maarten > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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 01/19] drm/atomic: Add new iterators over all state
On Wed, Nov 02, 2016 at 09:28:46AM +0100, Maarten Lankhorst wrote: > Op 01-11-16 om 14:41 schreef Ville Syrjälä: > > On Tue, Nov 01, 2016 at 02:34:00PM +0100, Maarten Lankhorst wrote: > >> Op 01-11-16 om 14:09 schreef Ville Syrjälä: > >>> On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote: > Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to > replace the old for_each_xxx_in_state ones. This is useful for >1 flip > depth and getting rid of all xxx->state dereferences. > > Signed-off-by: Maarten Lankhorst> --- > drivers/gpu/drm/drm_atomic.c | 6 +++ > drivers/gpu/drm/drm_atomic_helper.c | 52 +-- > drivers/gpu/drm/i915/intel_display.c | 11 ++--- > include/drm/drm_atomic.h | 81 > ++-- > include/drm/drm_atomic_helper.h | 3 ++ > 5 files changed, 142 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 5dd70540219c..120775fcfb70 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state > *state, > return ERR_PTR(-ENOMEM); > > state->crtcs[index].state = crtc_state; > +state->crtcs[index].old_state = crtc->state; > +state->crtcs[index].new_state = crtc_state; > state->crtcs[index].ptr = crtc; > crtc_state->state = state; > > @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state > *state, > > state->planes[index].state = plane_state; > state->planes[index].ptr = plane; > +state->planes[index].old_state = plane->state; > +state->planes[index].new_state = plane_state; > plane_state->state = state; > > DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n", > @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct > drm_atomic_state *state, > > drm_connector_reference(connector); > state->connectors[index].state = connector_state; > +state->connectors[index].old_state = connector->state; > +state->connectors[index].new_state = connector_state; > state->connectors[index].ptr = connector; > connector_state->state = state; > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index 07b432f43b98..fcb6e5b55217 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all); > * > * See also: > * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(), > - * drm_atomic_helper_resume() > + * drm_atomic_helper_resume(), > drm_atomic_helper_commit_duplicated_state() > */ > struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device > *dev) > { > @@ -2536,6 +2536,52 @@ unlock: > EXPORT_SYMBOL(drm_atomic_helper_suspend); > > /** > + * drm_atomic_helper_commit_duplicated_state - commit duplicated state > + * @state: duplicated atomic state to commit > + * @ctx: pointer to acquire_ctx to use for commit. > + * @nonblock: commit the state non-blocking. > + * > + * The state returned by drm_atomic_helper_duplicate_state() and > + * drm_atomic_helper_suspend() is partially invalid, and needs to > + * be fixed up before commit. > >>> So the old_state pointers are potentially invalid because whatever->state > >>> may have changed since we duplicated the state? > >> Yes when you use drm_atomic_helper_duplicate_state, and commit a different > >> state before committing the duplicated state. > >> This only happens during suspend/resume and gpu reset. > > Should we maybe have something like drm_atomic_refresh_old_state(state)? > > Would allow the caller then to check something in the old vs. new state > > before committing? > > iirc that was the first approach I took, but then you shouldn't do anything > with a duplicated state after > creating a except commit it, so creating a commit_duplicated_state should be > enough. > > Can you think of any case where you do the following? > > Duplicate state > commit different state > Look at duplicated state, tinker with it, commit it. Not really. Oh, and we do still run the thing through the check phase when we commit it, don't we? That sounds like it should let the driver do whatever it needs to do. So my only other concern is just the 'bool nonblock' parameter. It's a bit funny looking that we pass that in, then branch off to the
Re: [Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state
Op 01-11-16 om 14:41 schreef Ville Syrjälä: > On Tue, Nov 01, 2016 at 02:34:00PM +0100, Maarten Lankhorst wrote: >> Op 01-11-16 om 14:09 schreef Ville Syrjälä: >>> On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote: Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to replace the old for_each_xxx_in_state ones. This is useful for >1 flip depth and getting rid of all xxx->state dereferences. Signed-off-by: Maarten Lankhorst--- drivers/gpu/drm/drm_atomic.c | 6 +++ drivers/gpu/drm/drm_atomic_helper.c | 52 +-- drivers/gpu/drm/i915/intel_display.c | 11 ++--- include/drm/drm_atomic.h | 81 ++-- include/drm/drm_atomic_helper.h | 3 ++ 5 files changed, 142 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 5dd70540219c..120775fcfb70 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, return ERR_PTR(-ENOMEM); state->crtcs[index].state = crtc_state; + state->crtcs[index].old_state = crtc->state; + state->crtcs[index].new_state = crtc_state; state->crtcs[index].ptr = crtc; crtc_state->state = state; @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state, state->planes[index].state = plane_state; state->planes[index].ptr = plane; + state->planes[index].old_state = plane->state; + state->planes[index].new_state = plane_state; plane_state->state = state; DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n", @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state, drm_connector_reference(connector); state->connectors[index].state = connector_state; + state->connectors[index].old_state = connector->state; + state->connectors[index].new_state = connector_state; state->connectors[index].ptr = connector; connector_state->state = state; diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 07b432f43b98..fcb6e5b55217 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all); * * See also: * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(), - * drm_atomic_helper_resume() + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state() */ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) { @@ -2536,6 +2536,52 @@ unlock: EXPORT_SYMBOL(drm_atomic_helper_suspend); /** + * drm_atomic_helper_commit_duplicated_state - commit duplicated state + * @state: duplicated atomic state to commit + * @ctx: pointer to acquire_ctx to use for commit. + * @nonblock: commit the state non-blocking. + * + * The state returned by drm_atomic_helper_duplicate_state() and + * drm_atomic_helper_suspend() is partially invalid, and needs to + * be fixed up before commit. >>> So the old_state pointers are potentially invalid because whatever->state >>> may have changed since we duplicated the state? >> Yes when you use drm_atomic_helper_duplicate_state, and commit a different >> state before committing the duplicated state. >> This only happens during suspend/resume and gpu reset. > Should we maybe have something like drm_atomic_refresh_old_state(state)? > Would allow the caller then to check something in the old vs. new state > before committing? iirc that was the first approach I took, but then you shouldn't do anything with a duplicated state after creating a except commit it, so creating a commit_duplicated_state should be enough. Can you think of any case where you do the following? Duplicate state commit different state Look at duplicated state, tinker with it, commit it. ~Maarten ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state
On Tue, Nov 01, 2016 at 02:34:00PM +0100, Maarten Lankhorst wrote: > Op 01-11-16 om 14:09 schreef Ville Syrjälä: > > On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote: > >> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to > >> replace the old for_each_xxx_in_state ones. This is useful for >1 flip > >> depth and getting rid of all xxx->state dereferences. > >> > >> Signed-off-by: Maarten Lankhorst> >> --- > >> drivers/gpu/drm/drm_atomic.c | 6 +++ > >> drivers/gpu/drm/drm_atomic_helper.c | 52 +-- > >> drivers/gpu/drm/i915/intel_display.c | 11 ++--- > >> include/drm/drm_atomic.h | 81 > >> ++-- > >> include/drm/drm_atomic_helper.h | 3 ++ > >> 5 files changed, 142 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > >> index 5dd70540219c..120775fcfb70 100644 > >> --- a/drivers/gpu/drm/drm_atomic.c > >> +++ b/drivers/gpu/drm/drm_atomic.c > >> @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state > >> *state, > >>return ERR_PTR(-ENOMEM); > >> > >>state->crtcs[index].state = crtc_state; > >> + state->crtcs[index].old_state = crtc->state; > >> + state->crtcs[index].new_state = crtc_state; > >>state->crtcs[index].ptr = crtc; > >>crtc_state->state = state; > >> > >> @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state > >> *state, > >> > >>state->planes[index].state = plane_state; > >>state->planes[index].ptr = plane; > >> + state->planes[index].old_state = plane->state; > >> + state->planes[index].new_state = plane_state; > >>plane_state->state = state; > >> > >>DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n", > >> @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state > >> *state, > >> > >>drm_connector_reference(connector); > >>state->connectors[index].state = connector_state; > >> + state->connectors[index].old_state = connector->state; > >> + state->connectors[index].new_state = connector_state; > >>state->connectors[index].ptr = connector; > >>connector_state->state = state; > >> > >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c > >> b/drivers/gpu/drm/drm_atomic_helper.c > >> index 07b432f43b98..fcb6e5b55217 100644 > >> --- a/drivers/gpu/drm/drm_atomic_helper.c > >> +++ b/drivers/gpu/drm/drm_atomic_helper.c > >> @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all); > >> * > >> * See also: > >> * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(), > >> - * drm_atomic_helper_resume() > >> + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state() > >> */ > >> struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) > >> { > >> @@ -2536,6 +2536,52 @@ unlock: > >> EXPORT_SYMBOL(drm_atomic_helper_suspend); > >> > >> /** > >> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state > >> + * @state: duplicated atomic state to commit > >> + * @ctx: pointer to acquire_ctx to use for commit. > >> + * @nonblock: commit the state non-blocking. > >> + * > >> + * The state returned by drm_atomic_helper_duplicate_state() and > >> + * drm_atomic_helper_suspend() is partially invalid, and needs to > >> + * be fixed up before commit. > > So the old_state pointers are potentially invalid because whatever->state > > may have changed since we duplicated the state? > > Yes when you use drm_atomic_helper_duplicate_state, and commit a different > state before committing the duplicated state. > This only happens during suspend/resume and gpu reset. Should we maybe have something like drm_atomic_refresh_old_state(state)? Would allow the caller then to check something in the old vs. new state before committing? -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state
Op 01-11-16 om 14:09 schreef Ville Syrjälä: > On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote: >> Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to >> replace the old for_each_xxx_in_state ones. This is useful for >1 flip >> depth and getting rid of all xxx->state dereferences. >> >> Signed-off-by: Maarten Lankhorst>> --- >> drivers/gpu/drm/drm_atomic.c | 6 +++ >> drivers/gpu/drm/drm_atomic_helper.c | 52 +-- >> drivers/gpu/drm/i915/intel_display.c | 11 ++--- >> include/drm/drm_atomic.h | 81 >> ++-- >> include/drm/drm_atomic_helper.h | 3 ++ >> 5 files changed, 142 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >> index 5dd70540219c..120775fcfb70 100644 >> --- a/drivers/gpu/drm/drm_atomic.c >> +++ b/drivers/gpu/drm/drm_atomic.c >> @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, >> return ERR_PTR(-ENOMEM); >> >> state->crtcs[index].state = crtc_state; >> +state->crtcs[index].old_state = crtc->state; >> +state->crtcs[index].new_state = crtc_state; >> state->crtcs[index].ptr = crtc; >> crtc_state->state = state; >> >> @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state >> *state, >> >> state->planes[index].state = plane_state; >> state->planes[index].ptr = plane; >> +state->planes[index].old_state = plane->state; >> +state->planes[index].new_state = plane_state; >> plane_state->state = state; >> >> DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n", >> @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state >> *state, >> >> drm_connector_reference(connector); >> state->connectors[index].state = connector_state; >> +state->connectors[index].old_state = connector->state; >> +state->connectors[index].new_state = connector_state; >> state->connectors[index].ptr = connector; >> connector_state->state = state; >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c >> b/drivers/gpu/drm/drm_atomic_helper.c >> index 07b432f43b98..fcb6e5b55217 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all); >> * >> * See also: >> * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(), >> - * drm_atomic_helper_resume() >> + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state() >> */ >> struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) >> { >> @@ -2536,6 +2536,52 @@ unlock: >> EXPORT_SYMBOL(drm_atomic_helper_suspend); >> >> /** >> + * drm_atomic_helper_commit_duplicated_state - commit duplicated state >> + * @state: duplicated atomic state to commit >> + * @ctx: pointer to acquire_ctx to use for commit. >> + * @nonblock: commit the state non-blocking. >> + * >> + * The state returned by drm_atomic_helper_duplicate_state() and >> + * drm_atomic_helper_suspend() is partially invalid, and needs to >> + * be fixed up before commit. > So the old_state pointers are potentially invalid because whatever->state > may have changed since we duplicated the state? Yes when you use drm_atomic_helper_duplicate_state, and commit a different state before committing the duplicated state. This only happens during suspend/resume and gpu reset. ~Maarten ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state
On Mon, Oct 17, 2016 at 02:37:00PM +0200, Maarten Lankhorst wrote: > Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to > replace the old for_each_xxx_in_state ones. This is useful for >1 flip > depth and getting rid of all xxx->state dereferences. > > Signed-off-by: Maarten Lankhorst> --- > drivers/gpu/drm/drm_atomic.c | 6 +++ > drivers/gpu/drm/drm_atomic_helper.c | 52 +-- > drivers/gpu/drm/i915/intel_display.c | 11 ++--- > include/drm/drm_atomic.h | 81 > ++-- > include/drm/drm_atomic_helper.h | 3 ++ > 5 files changed, 142 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 5dd70540219c..120775fcfb70 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, > return ERR_PTR(-ENOMEM); > > state->crtcs[index].state = crtc_state; > + state->crtcs[index].old_state = crtc->state; > + state->crtcs[index].new_state = crtc_state; > state->crtcs[index].ptr = crtc; > crtc_state->state = state; > > @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state, > > state->planes[index].state = plane_state; > state->planes[index].ptr = plane; > + state->planes[index].old_state = plane->state; > + state->planes[index].new_state = plane_state; > plane_state->state = state; > > DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n", > @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state > *state, > > drm_connector_reference(connector); > state->connectors[index].state = connector_state; > + state->connectors[index].old_state = connector->state; > + state->connectors[index].new_state = connector_state; > state->connectors[index].ptr = connector; > connector_state->state = state; > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index 07b432f43b98..fcb6e5b55217 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all); > * > * See also: > * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(), > - * drm_atomic_helper_resume() > + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state() > */ > struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) > { > @@ -2536,6 +2536,52 @@ unlock: > EXPORT_SYMBOL(drm_atomic_helper_suspend); > > /** > + * drm_atomic_helper_commit_duplicated_state - commit duplicated state > + * @state: duplicated atomic state to commit > + * @ctx: pointer to acquire_ctx to use for commit. > + * @nonblock: commit the state non-blocking. > + * > + * The state returned by drm_atomic_helper_duplicate_state() and > + * drm_atomic_helper_suspend() is partially invalid, and needs to > + * be fixed up before commit. So the old_state pointers are potentially invalid because whatever->state may have changed since we duplicated the state? > + * > + * Returns: > + * 0 on success or a negative error code on failure. > + * > + * See also: > + * drm_atomic_helper_suspend() > + */ > +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, > + struct drm_modeset_acquire_ctx > *ctx, > + bool nonblock) > +{ > + int i; > + struct drm_plane *plane; > + struct drm_plane_state *plane_state; > + struct drm_connector *connector; > + struct drm_connector_state *conn_state; > + struct drm_crtc *crtc; > + struct drm_crtc_state *crtc_state; > + > + state->acquire_ctx = ctx; > + > + for_each_new_plane_in_state(state, plane, plane_state, i) > + state->planes[i].old_state = plane->state; > + > + for_each_new_crtc_in_state(state, crtc, crtc_state, i) > + state->crtcs[i].old_state = crtc->state; > + > + for_each_new_connector_in_state(state, connector, conn_state, i) > + state->connectors[i].old_state = connector->state; > + > + if (nonblock) > + return drm_atomic_nonblocking_commit(state); > + else > + return drm_atomic_commit(state); > +} > +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state); > + > +/** > * drm_atomic_helper_resume - subsystem-level resume helper > * @dev: DRM device > * @state: atomic state to resume to > @@ -2558,9 +2604,9 @@ int drm_atomic_helper_resume(struct drm_device *dev, > int err; > > drm_mode_config_reset(dev); > + > drm_modeset_lock_all(dev); > - state->acquire_ctx = config->acquire_ctx; > - err = drm_atomic_commit(state); > + err =
[Intel-gfx] [PATCH 01/19] drm/atomic: Add new iterators over all state
Add for_each_(old)(new)_(plane,connector,crtc)_in_state iterators to replace the old for_each_xxx_in_state ones. This is useful for >1 flip depth and getting rid of all xxx->state dereferences. Signed-off-by: Maarten Lankhorst--- drivers/gpu/drm/drm_atomic.c | 6 +++ drivers/gpu/drm/drm_atomic_helper.c | 52 +-- drivers/gpu/drm/i915/intel_display.c | 11 ++--- include/drm/drm_atomic.h | 81 ++-- include/drm/drm_atomic_helper.h | 3 ++ 5 files changed, 142 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 5dd70540219c..120775fcfb70 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -278,6 +278,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, return ERR_PTR(-ENOMEM); state->crtcs[index].state = crtc_state; + state->crtcs[index].old_state = crtc->state; + state->crtcs[index].new_state = crtc_state; state->crtcs[index].ptr = crtc; crtc_state->state = state; @@ -640,6 +642,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state, state->planes[index].state = plane_state; state->planes[index].ptr = plane; + state->planes[index].old_state = plane->state; + state->planes[index].new_state = plane_state; plane_state->state = state; DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n", @@ -931,6 +935,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state, drm_connector_reference(connector); state->connectors[index].state = connector_state; + state->connectors[index].old_state = connector->state; + state->connectors[index].new_state = connector_state; state->connectors[index].ptr = connector; connector_state->state = state; diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 07b432f43b98..fcb6e5b55217 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2495,7 +2495,7 @@ EXPORT_SYMBOL(drm_atomic_helper_disable_all); * * See also: * drm_atomic_helper_duplicate_state(), drm_atomic_helper_disable_all(), - * drm_atomic_helper_resume() + * drm_atomic_helper_resume(), drm_atomic_helper_commit_duplicated_state() */ struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev) { @@ -2536,6 +2536,52 @@ unlock: EXPORT_SYMBOL(drm_atomic_helper_suspend); /** + * drm_atomic_helper_commit_duplicated_state - commit duplicated state + * @state: duplicated atomic state to commit + * @ctx: pointer to acquire_ctx to use for commit. + * @nonblock: commit the state non-blocking. + * + * The state returned by drm_atomic_helper_duplicate_state() and + * drm_atomic_helper_suspend() is partially invalid, and needs to + * be fixed up before commit. + * + * Returns: + * 0 on success or a negative error code on failure. + * + * See also: + * drm_atomic_helper_suspend() + */ +int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, + struct drm_modeset_acquire_ctx *ctx, + bool nonblock) +{ + int i; + struct drm_plane *plane; + struct drm_plane_state *plane_state; + struct drm_connector *connector; + struct drm_connector_state *conn_state; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + + state->acquire_ctx = ctx; + + for_each_new_plane_in_state(state, plane, plane_state, i) + state->planes[i].old_state = plane->state; + + for_each_new_crtc_in_state(state, crtc, crtc_state, i) + state->crtcs[i].old_state = crtc->state; + + for_each_new_connector_in_state(state, connector, conn_state, i) + state->connectors[i].old_state = connector->state; + + if (nonblock) + return drm_atomic_nonblocking_commit(state); + else + return drm_atomic_commit(state); +} +EXPORT_SYMBOL(drm_atomic_helper_commit_duplicated_state); + +/** * drm_atomic_helper_resume - subsystem-level resume helper * @dev: DRM device * @state: atomic state to resume to @@ -2558,9 +2604,9 @@ int drm_atomic_helper_resume(struct drm_device *dev, int err; drm_mode_config_reset(dev); + drm_modeset_lock_all(dev); - state->acquire_ctx = config->acquire_ctx; - err = drm_atomic_commit(state); + err = drm_atomic_helper_commit_duplicated_state(state, config->acquire_ctx, false); drm_modeset_unlock_all(dev); return err; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a12e093c54cf..3bd3f6a93c12 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3497,7 +3497,8 @@ static void