| Message ID | 20161201133001.28333-1-marta.lofstedt@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 01, 2016 at 03:30:01PM +0200, Marta Lofstedt wrote: > For non-CRTC displays, A connector not attached to a CRTC works. It is the active connectors that were broken. > the drm_atomic_helper_connector_dpms > will always set the connector back the old DPMS state > before returning. This makes it impossible to change > DPMS state. > > fixes: 0853695c3ba46f97dfc0b5885f7b7e640ca212dd > > Signed-off-by: Marta Lofstedt <marta.lofstedt@intel.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 494680c..6a5acb9 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2885,8 +2885,8 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > fail: > if (ret == -EDEADLK) > goto backoff; > - > - connector->dpms = old_mode; > + if (ret != 0) > + connector->dpms = old_mode; Patch however is correct. Fix the subject line to reflect the impact correctly, amend the fixes tag and add the appropriate CCs Fixes: 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state") Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Eric Engestrom <eric.engestrom@imgtec.com> Cc: Sean Paul <seanpaul@chromium.org> Cc: dri-devel@lists.freedesktop.org Cc: <drm-intel-fixes@lists.freedesktop.org> And mention if there was a testcase exercising the failure. If not please suggest one. -Chris
> -----Original Message----- > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > Sent: Thursday, December 1, 2016 4:15 PM > To: Lofstedt, Marta <marta.lofstedt@intel.com> > Cc: dri-devel@lists.freedesktop.org > Subject: Re: [PATCH] drm: allow changing DPMS mode for non-CRTC displays > > On Thu, Dec 01, 2016 at 03:30:01PM +0200, Marta Lofstedt wrote: > > For non-CRTC displays, > > A connector not attached to a CRTC works. It is the active connectors that > were broken. > > > the drm_atomic_helper_connector_dpms > > will always set the connector back the old DPMS state before > > returning. This makes it impossible to change DPMS state. > > > > fixes: 0853695c3ba46f97dfc0b5885f7b7e640ca212dd > > > > Signed-off-by: Marta Lofstedt <marta.lofstedt@intel.com> > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > > b/drivers/gpu/drm/drm_atomic_helper.c > > index 494680c..6a5acb9 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -2885,8 +2885,8 @@ int drm_atomic_helper_connector_dpms(struct > > drm_connector *connector, > > fail: > > if (ret == -EDEADLK) > > goto backoff; > > - > > - connector->dpms = old_mode; > > + if (ret != 0) > > + connector->dpms = old_mode; > > Patch however is correct. > > Fix the subject line to reflect the impact correctly, amend the fixes tag and > add the appropriate CCs > > Fixes: 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state") > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Eric Engestrom <eric.engestrom@imgtec.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: dri-devel@lists.freedesktop.org > Cc: <drm-intel-fixes@lists.freedesktop.org> > > And mention if there was a testcase exercising the failure. If not please > suggest one. Thanks for the comments Chris. About tests, I can't seem to find any test that is actually checking that we entered DPMS mode. The suggestion is to extend the kms_properties test, or maybe we should have a new dpms test. I CC Petri, so he can take part in such a discussion. /Marta > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Thursday, 2016-12-01 14:15:05 +0000, Chris Wilson wrote: > On Thu, Dec 01, 2016 at 03:30:01PM +0200, Marta Lofstedt wrote: > > For non-CRTC displays, > > A connector not attached to a CRTC works. It is the active connectors > that were broken. > > > the drm_atomic_helper_connector_dpms > > will always set the connector back the old DPMS state > > before returning. This makes it impossible to change > > DPMS state. > > > > fixes: 0853695c3ba46f97dfc0b5885f7b7e640ca212dd > > > > Signed-off-by: Marta Lofstedt <marta.lofstedt@intel.com> > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index 494680c..6a5acb9 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -2885,8 +2885,8 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > > fail: > > if (ret == -EDEADLK) > > goto backoff; > > - > > - connector->dpms = old_mode; > > + if (ret != 0) > > + connector->dpms = old_mode; > > Patch however is correct. > > Fix the subject line to reflect the impact correctly, amend the fixes > tag and add the appropriate CCs > > Fixes: 0853695c3ba4 ("drm: Add reference counting to drm_atomic_state") > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Eric Engestrom <eric.engestrom@imgtec.com> Good catch! Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com> I just had another look at the change (0853695c3ba4) and I don't believe there was any other unintended behaviour change. > Cc: Sean Paul <seanpaul@chromium.org> > Cc: dri-devel@lists.freedesktop.org > Cc: <drm-intel-fixes@lists.freedesktop.org> > > And mention if there was a testcase exercising the failure. If not > please suggest one. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 494680c..6a5acb9 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2885,8 +2885,8 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, fail: if (ret == -EDEADLK) goto backoff; - - connector->dpms = old_mode; + if (ret != 0) + connector->dpms = old_mode; drm_atomic_state_put(state); return ret;
For non-CRTC displays, the drm_atomic_helper_connector_dpms will always set the connector back the old DPMS state before returning. This makes it impossible to change DPMS state. fixes: 0853695c3ba46f97dfc0b5885f7b7e640ca212dd Signed-off-by: Marta Lofstedt <marta.lofstedt@intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)