* Re: [PATCH] Fixes: fc2a69f3903d ("drm/atomic: Handling the case when, setting old crtc for plane")
2018-09-04 22:26 [PATCH] Fixes: fc2a69f3903d ("drm/atomic: Handling the case when, setting old crtc for plane") Karsten Hohmeier
@ 2018-09-05 9:23 ` Jani Nikula
2018-09-05 9:34 ` Daniel Vetter
2018-09-05 9:36 ` Maarten Lankhorst
2018-09-05 9:37 ` Eric Engestrom
2 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2018-09-05 9:23 UTC (permalink / raw)
To: Karsten Hohmeier, dri-devel
Cc: David Airlie, Daniel Vetter, Madhur Verma, stable,
Hemanshu Srivastava, Sean Paul, Satendra Singh Thakur
Subject: Revert "drm/atomic: Handling the case when setting old crtc for plane"
On Wed, 05 Sep 2018, Karsten Hohmeier <karsten@hohmatik.de> wrote:
> Original commit causes "plane A assertion failure" on lid close/lid open
> with older HP Compaq 6720s laptops (Intel Mobile GME965/GLE960).
> Full bug report at "https://bugs.freedesktop.org/show_bug.cgi?id=107827".
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107827
Fixes: fc2a69f3903d ("drm/atomic: Handling the case when setting old crtc for plane")
Cc: Satendra Singh Thakur <satendra.t@samsung.com>
Cc: Madhur Verma <madhur.verma@samsung.com>
Cc: Hemanshu Srivastava <hemanshu.s@samsung.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v4.18+
> ---
> drivers/gpu/drm/drm_atomic.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 895741e9cd7d..a59ded403657 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1433,9 +1433,7 @@ drm_atomic_set_crtc_for_plane(struct
> drm_plane_state *plane_state,
> {
> struct drm_plane *plane = plane_state->plane;
> struct drm_crtc_state *crtc_state;
> - /* Nothing to do for same crtc*/
> - if (plane_state->crtc == crtc)
> - return 0;
> +
> if (plane_state->crtc) {
> crtc_state = drm_atomic_get_crtc_state(plane_state->state,
> plane_state->crtc);
> --
> 2.11.0
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fixes: fc2a69f3903d ("drm/atomic: Handling the case when, setting old crtc for plane")
2018-09-05 9:23 ` Jani Nikula
@ 2018-09-05 9:34 ` Daniel Vetter
0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2018-09-05 9:34 UTC (permalink / raw)
To: Jani Nikula
Cc: Karsten Hohmeier, dri-devel, David Airlie, Daniel Vetter,
Madhur Verma, stable, Hemanshu Srivastava, Sean Paul,
Satendra Singh Thakur
On Wed, Sep 05, 2018 at 12:23:52PM +0300, Jani Nikula wrote:
>
> Subject: Revert "drm/atomic: Handling the case when setting old crtc for plane"
>
> On Wed, 05 Sep 2018, Karsten Hohmeier <karsten@hohmatik.de> wrote:
> > Original commit causes "plane A assertion failure" on lid close/lid open
> > with older HP Compaq 6720s laptops (Intel Mobile GME965/GLE960).
> > Full bug report at "https://bugs.freedesktop.org/show_bug.cgi?id=107827".
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107827
> Fixes: fc2a69f3903d ("drm/atomic: Handling the case when setting old crtc for plane")
> Cc: Satendra Singh Thakur <satendra.t@samsung.com>
> Cc: Madhur Verma <madhur.verma@samsung.com>
> Cc: Hemanshu Srivastava <hemanshu.s@samsung.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: dri-devel@lists.freedesktop.org
> Cc: <stable@vger.kernel.org> # v4.18+
Karsten, we also need your signed-off-by per
https://dri.freedesktop.org/docs/drm/process/submitting-patches.html?highlight=signed%20off#sign-your-work-the-developer-s-certificate-of-origin
Otherwise we can't apply the patch. Just reply here is enough.
Thanks, Daniel
>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 895741e9cd7d..a59ded403657 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1433,9 +1433,7 @@ drm_atomic_set_crtc_for_plane(struct
> > drm_plane_state *plane_state,
> > {
> > struct drm_plane *plane = plane_state->plane;
> > struct drm_crtc_state *crtc_state;
> > - /* Nothing to do for same crtc*/
> > - if (plane_state->crtc == crtc)
> > - return 0;
> > +
> > if (plane_state->crtc) {
> > crtc_state = drm_atomic_get_crtc_state(plane_state->state,
> > plane_state->crtc);
> > --
> > 2.11.0
> >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fixes: fc2a69f3903d ("drm/atomic: Handling the case when, setting old crtc for plane")
2018-09-04 22:26 [PATCH] Fixes: fc2a69f3903d ("drm/atomic: Handling the case when, setting old crtc for plane") Karsten Hohmeier
2018-09-05 9:23 ` Jani Nikula
@ 2018-09-05 9:36 ` Maarten Lankhorst
2018-09-05 9:37 ` Eric Engestrom
2 siblings, 0 replies; 6+ messages in thread
From: Maarten Lankhorst @ 2018-09-05 9:36 UTC (permalink / raw)
To: Karsten Hohmeier, dri-devel
Cc: Satendra Singh Thakur, Madhur Verma, Hemanshu Srivastava,
Daniel Vetter, Gustavo Padovan, Sean Paul, David Airlie, stable
Op 05-09-18 om 00:26 schreef Karsten Hohmeier:
> Original commit causes "plane A assertion failure" on lid close/lid open
> with older HP Compaq 6720s laptops (Intel Mobile GME965/GLE960).
> Full bug report at "https://bugs.freedesktop.org/show_bug.cgi?id=107827".
> ---
> drivers/gpu/drm/drm_atomic.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 895741e9cd7d..a59ded403657 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1433,9 +1433,7 @@ drm_atomic_set_crtc_for_plane(struct
> drm_plane_state *plane_state,
> {
> struct drm_plane *plane = plane_state->plane;
> struct drm_crtc_state *crtc_state;
> - /* Nothing to do for same crtc*/
> - if (plane_state->crtc == crtc)
> - return 0;
> +
> if (plane_state->crtc) {
> crtc_state = drm_atomic_get_crtc_state(plane_state->state,
> plane_state->crtc);
> --
> 2.11.0
>
>
This patch is missing a signed-off-by.
Can you add your signed-off-by, so we can apply this upstream?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Fixes: fc2a69f3903d ("drm/atomic: Handling the case when, setting old crtc for plane")
2018-09-04 22:26 [PATCH] Fixes: fc2a69f3903d ("drm/atomic: Handling the case when, setting old crtc for plane") Karsten Hohmeier
2018-09-05 9:23 ` Jani Nikula
2018-09-05 9:36 ` Maarten Lankhorst
@ 2018-09-05 9:37 ` Eric Engestrom
2018-09-06 4:39 ` Satendra Singh Thakur
2 siblings, 1 reply; 6+ messages in thread
From: Eric Engestrom @ 2018-09-05 9:37 UTC (permalink / raw)
To: Karsten Hohmeier
Cc: dri-devel, David Airlie, Daniel Vetter, Madhur Verma, stable,
Hemanshu Srivastava, Sean Paul, Satendra Singh Thakur
Hey Karsten, thanks for the patch! I don't know if it's correct or not,
but I'll comment on a few other things.
The title of the commit should be a description of what your commit does;
in this case you should probably keep the title that git gave you when
you did `git revert`, as well as the first line:
Revert "drm/atomic: Handling the case when setting old crtc for plane"
This reverts commit fc2a69f3903dfd97cd47f593e642b47918c949df.
After that comes your explanation of why this commit should be reverted,
which you've done here:
On Wednesday, 2018-09-05 00:26:27 +0200, Karsten Hohmeier wrote:
> Original commit causes "plane A assertion failure" on lid close/lid open
> with older HP Compaq 6720s laptops (Intel Mobile GME965/GLE960).
The "Fixes" line that Chris gave you should got at the end of the commit
message, after the explanation.
> Full bug report at "https://bugs.freedesktop.org/show_bug.cgi?id=107827".
The format for bug report references is (on its own line):
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107827
> ---
> drivers/gpu/drm/drm_atomic.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 895741e9cd7d..a59ded403657 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1433,9 +1433,7 @@ drm_atomic_set_crtc_for_plane(struct
> drm_plane_state *plane_state,
It looks like thunderbird mangled the patch when sending the email; it's
recommended to use `git send-email` instead, as it does the right thing:
https://git-scm.com/docs/git-send-email
> {
> struct drm_plane *plane = plane_state->plane;
> struct drm_crtc_state *crtc_state;
> - /* Nothing to do for same crtc*/
> - if (plane_state->crtc == crtc)
> - return 0;
> +
> if (plane_state->crtc) {
> crtc_state = drm_atomic_get_crtc_state(plane_state->state,
> plane_state->crtc);
> --
> 2.11.0
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Re: [PATCH] Fixes: fc2a69f3903d ("drm/atomic: Handling the case when, setting old crtc for plane")
2018-09-05 9:37 ` Eric Engestrom
@ 2018-09-06 4:39 ` Satendra Singh Thakur
0 siblings, 0 replies; 6+ messages in thread
From: Satendra Singh Thakur @ 2018-09-06 4:39 UTC (permalink / raw)
To: karsten
Cc: eric.engestrom, stable, gustavo, daniel.vetter, maarten.lankhorst,
seanpaul, airlied, dri-devel, sst2005, Satendra Singh Thakur
On Wed, 05 Sep 2018 at 10:37:39 +0100 Eric Engestrom wrote:
> Hey Karsten, thanks for the patch! I don't know if it's correct or not,
> but I'll comment on a few other things.
>
> The title of the commit should be a description of what your commit does;
> in this case you should probably keep the title that git gave you when
> you did `git revert`, as well as the first line:
>
> Revert "drm/atomic: Handling the case when setting old crtc for plane"
>
> This reverts commit fc2a69f3903dfd97cd47f593e642b47918c949df.
> After that comes your explanation of why this commit should be reverted,
> which you've done here:
>
> On Wednesday, 2018-09-05 00:26:27 +0200, Karsten Hohmeier wrote:
> > Original commit causes "plane A assertion failure" on lid close/lid open
> > with older HP Compaq 6720s laptops (Intel Mobile GME965/GLE960).
>
> The "Fixes" line that Chris gave you should got at the end of the commit
> message, after the explanation.
>
> > Full bug report at "https://bugs.freedesktop.org/show_bug.cgi?id=107827".
>
> The format for bug report references is (on its own line):
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107827
>
> > ---
> > drivers/gpu/drm/drm_atomic.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 895741e9cd7d..a59ded403657 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1433,9 +1433,7 @@ drm_atomic_set_crtc_for_plane(struct
> > drm_plane_state *plane_state,
>
> It looks like thunderbird mangled the patch when sending the email; it's
> recommended to use `git send-email` instead, as it does the right thing:
> https://git-scm.com/docs/git-send-email
>
> > {
> > struct drm_plane *plane = plane_state->plane;
> > struct drm_crtc_state *crtc_state;
> > - /* Nothing to do for same crtc*/
> > - if (plane_state->crtc == crtc)
> > - return 0;
> > +
> > if (plane_state->crtc) {
> > crtc_state = drm_atomic_get_crtc_state(plane_state->state,
> > plane_state->crtc);
> > --
Hi Karsten,
Thanks for the patch.
However, your own comments at the below link are contradictory
https://bugs.freedesktop.org/show_bug.cgi?id=107827
Here, you mention that the commit 82bcb0e7a45d was still in place and screen corruption is gone and logs are clean.
> Karsten Hohmeier 2018-09-05 17:11:14 UTC
> I cherry-picked commit 05c72e77ccda ("drm/i915: Nuke the LVDS lid notifier") on top of 4.18.5 as you suggested.
> Yes, it works.
> The screen corruption is gone and the logs are clean. The 82bcb0e7a45d ("drm/atomic: Handling the case when setting old crtc for plane") change was still in place.
>From above comments it looks like this patch was not the reason for screen corruption.
Please clarify and let me know if I am missing something here.
Thanks
-Satendra
^ permalink raw reply [flat|nested] 6+ messages in thread