| Message ID | 20190227230408.31390-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Fix atomic state leak when resetting HDMI link | expand |
On Wed, Feb 27, 2019 at 03:04:08PM -0800, José Roberto de Souza wrote: > drm_atomic_commit() call chain already takes care of adding > connectors and planes, so lets no add then manually if not changing > their states. The specific callgraph would make review easier. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Lyude Paul <lyude@redhat.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 34dd5823398a..c22ddde2dfc1 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -3975,14 +3975,6 @@ static int modeset_pipe(struct drm_crtc *crtc, > > crtc_state->mode_changed = true; > > - ret = drm_atomic_add_affected_connectors(state, crtc); > - if (ret) > - goto out; > - > - ret = drm_atomic_add_affected_planes(state, crtc); > - if (ret) > - goto out; > - > ret = drm_atomic_commit(state); > out: > drm_atomic_state_put(state); > -- > 2.21.0
On Thu, 2019-02-28 at 13:37 +0200, Ville Syrjälä wrote: > On Wed, Feb 27, 2019 at 03:04:08PM -0800, José Roberto de Souza > wrote: > > drm_atomic_commit() call chain already takes care of adding > > connectors and planes, so lets no add then manually if not changing > > their states. > > The specific callgraph would make review easier. Appending this to the commit message is enough? drm_atomic_commit() drm_atomic_check_only() config->funcs->atomic_check()/intel_atomic_check() drm_atomic_helper_check() drm_atomic_helper_check_modeset() for_each_oldnew_crtc_in_state() drm_atomic_add_affected _connectors() drm_atomic_add_affected _planes() > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Lyude Paul <lyude@redhat.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 8 -------- > > 1 file changed, 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index 34dd5823398a..c22ddde2dfc1 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -3975,14 +3975,6 @@ static int modeset_pipe(struct drm_crtc > > *crtc, > > > > crtc_state->mode_changed = true; > > > > - ret = drm_atomic_add_affected_connectors(state, crtc); > > - if (ret) > > - goto out; > > - > > - ret = drm_atomic_add_affected_planes(state, crtc); > > - if (ret) > > - goto out; > > - > > ret = drm_atomic_commit(state); > > out: > > drm_atomic_state_put(state); > > -- > > 2.21.0
On Thu, Feb 28, 2019 at 09:27:48PM +0000, Souza, Jose wrote: > On Thu, 2019-02-28 at 13:37 +0200, Ville Syrjälä wrote: > > On Wed, Feb 27, 2019 at 03:04:08PM -0800, José Roberto de Souza > > wrote: > > > drm_atomic_commit() call chain already takes care of adding > > > connectors and planes, so lets no add then manually if not changing > > > their states. > > > > The specific callgraph would make review easier. > > Appending this to the commit message is enough? > > drm_atomic_commit() > drm_atomic_check_only() > config->funcs->atomic_check()/intel_atomic_check() > drm_atomic_helper_check() > drm_atomic_helper_check_modeset() > for_each_oldnew_crtc_in_state() > drm_atomic_add_affected > _connectors() > drm_atomic_add_affected > _planes() Yes, that helps. Thanks. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Hmm. I wonder if fastboot has actually broken this code. Maybe we need to set connectors_changed instead of mode_changed to guarantee the full modeset... > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Lyude Paul <lyude@redhat.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_ddi.c | 8 -------- > > > 1 file changed, 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > b/drivers/gpu/drm/i915/intel_ddi.c > > > index 34dd5823398a..c22ddde2dfc1 100644 > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > @@ -3975,14 +3975,6 @@ static int modeset_pipe(struct drm_crtc > > > *crtc, > > > > > > crtc_state->mode_changed = true; > > > > > > - ret = drm_atomic_add_affected_connectors(state, crtc); > > > - if (ret) > > > - goto out; > > > - > > > - ret = drm_atomic_add_affected_planes(state, crtc); > > > - if (ret) > > > - goto out; > > > - > > > ret = drm_atomic_commit(state); > > > out: > > > drm_atomic_state_put(state); > > > -- > > > 2.21.0
On Fri, 2019-03-01 at 15:43 +0200, Ville Syrjälä wrote: > On Thu, Feb 28, 2019 at 09:27:48PM +0000, Souza, Jose wrote: > > On Thu, 2019-02-28 at 13:37 +0200, Ville Syrjälä wrote: > > > On Wed, Feb 27, 2019 at 03:04:08PM -0800, José Roberto de Souza > > > wrote: > > > > drm_atomic_commit() call chain already takes care of adding > > > > connectors and planes, so lets no add then manually if not > > > > changing > > > > their states. > > > > > > The specific callgraph would make review easier. > > > > Appending this to the commit message is enough? > > > > drm_atomic_commit() > > drm_atomic_check_only() > > config->funcs->atomic_check()/intel_atomic_check() > > drm_atomic_helper_check() > > drm_atomic_helper_check_modeset() > > for_each_oldnew_crtc_in_state() > > drm_atomic_add_affected > > _connectors() > > drm_atomic_add_affected > > _planes() > > Yes, that helps. Thanks. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Hmm. I wonder if fastboot has actually broken this code. Maybe we > need > to set connectors_changed instead of mode_changed to guarantee the > full > modeset... Just tested and you are right, sending a separated patch to fix this. > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Lyude Paul <lyude@redhat.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_ddi.c | 8 -------- > > > > 1 file changed, 8 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > > b/drivers/gpu/drm/i915/intel_ddi.c > > > > index 34dd5823398a..c22ddde2dfc1 100644 > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > > @@ -3975,14 +3975,6 @@ static int modeset_pipe(struct drm_crtc > > > > *crtc, > > > > > > > > crtc_state->mode_changed = true; > > > > > > > > - ret = drm_atomic_add_affected_connectors(state, crtc); > > > > - if (ret) > > > > - goto out; > > > > - > > > > - ret = drm_atomic_add_affected_planes(state, crtc); > > > > - if (ret) > > > > - goto out; > > > > - > > > > ret = drm_atomic_commit(state); > > > > out: > > > > drm_atomic_state_put(state); > > > > -- > > > > 2.21.0 > >
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 34dd5823398a..c22ddde2dfc1 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -3975,14 +3975,6 @@ static int modeset_pipe(struct drm_crtc *crtc, crtc_state->mode_changed = true; - ret = drm_atomic_add_affected_connectors(state, crtc); - if (ret) - goto out; - - ret = drm_atomic_add_affected_planes(state, crtc); - if (ret) - goto out; - ret = drm_atomic_commit(state); out: drm_atomic_state_put(state);
drm_atomic_commit() call chain already takes care of adding connectors and planes, so lets no add then manually if not changing their states. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Lyude Paul <lyude@redhat.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 8 -------- 1 file changed, 8 deletions(-)