[PATCH 1/2] drm/i915: Check crtc->active in intel_crtc_disable_planes

[PATCH 1/2] drm/i915: Check crtc->active in intel_crtc_disable_planes

[Daniel Vetter]

This was lost in

commit ce22dba92de22e951dee2ff89937a39754d2dd91
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Tue Apr 21 17:12:56 2015 +0300

    drm/i915: Move toggling planes out of crtc enable/disable.

and we still need that crtc->active check since the overall modeset
flow doesn't yet take dpms state into account properly. Fixes WARNING
backtraces on at least bdw/hsw due to the ips disabling code being
upset about being run on a switched-off pipe.

We don't need a corresponding change on the enable side since with the
old setCrtc semantics we always force-enable the pipe after a modeset.
And the dpms function intel_crtc_control already checks for ->active.

Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8aa803848f35..647b1404c441 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4854,6 +4854,9 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
 	struct intel_plane *intel_plane;
 	int pipe = intel_crtc->pipe;
 
+	if (!intel_crtc->active)
+		return;
+
 	intel_crtc_wait_for_pending_flips(crtc);
 
 	intel_pre_disable_primary(crtc);
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Use crtc_state->active in primary check_plane func

[Daniel Vetter]

Since

commit 8c7b5ccb729870e606321b3703e2c2e698c49a95
Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Date:   Tue Apr 21 17:13:19 2015 +0300

    drm/i915: Use atomic helpers for computing changed flags

we compute the plane state for a modeset before actually committing
any changes, which means crtc->active won't be correct yet. Looking at
future work in the modeset conversion targetting 4.3 the only places
where crtc_state->active isn't accurate is when disabling other CRTCs
than the one the modeset is for (when stealing connectors). Which
isn't the case here. And that's also confirmed by an audit, we do
unconditionally update crtc_state->active for the current pipe.

We also don't need to update any other plane check functions since we
only ever add the primary state to the modeset update right now.

Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 647b1404c441..ba9321998a41 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13276,7 +13276,7 @@ intel_check_primary_plane(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
-	if (intel_crtc->active) {
+	if (crtc_state->base.active) {
 		struct intel_plane_state *old_state =
 			to_intel_plane_state(plane->state);
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Use crtc_state->active in primary check_plane func

[Maarten Lankhorst]

Op 07-07-15 om 11:15 schreef Daniel Vetter:
> Since
>
> commit 8c7b5ccb729870e606321b3703e2c2e698c49a95
> Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Date:   Tue Apr 21 17:13:19 2015 +0300
>
>     drm/i915: Use atomic helpers for computing changed flags
>
> we compute the plane state for a modeset before actually committing
> any changes, which means crtc->active won't be correct yet. Looking at
> future work in the modeset conversion targetting 4.3 the only places
> where crtc_state->active isn't accurate is when disabling other CRTCs
> than the one the modeset is for (when stealing connectors). Which
> isn't the case here. And that's also confirmed by an audit, we do
> unconditionally update crtc_state->active for the current pipe.
>
> We also don't need to update any other plane check functions since we
> only ever add the primary state to the modeset update right now.
>
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 647b1404c441..ba9321998a41 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13276,7 +13276,7 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	if (ret)
>  		return ret;
>  
> -	if (intel_crtc->active) {
> +	if (crtc_state->base.active) {
>  		struct intel_plane_state *old_state =
>  			to_intel_plane_state(plane->state);
>  
I think this was probably a feature, not a bug. Since full atomic planes won't be part of v4.2
both patches look ok to me.

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Use crtc_state->active in primary check_plane func

[Daniel Vetter]

On Wed, Jul 08, 2015 at 12:24:07PM +0200, Maarten Lankhorst wrote:
> Op 07-07-15 om 11:15 schreef Daniel Vetter:
> > Since
> >
> > commit 8c7b5ccb729870e606321b3703e2c2e698c49a95
> > Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > Date:   Tue Apr 21 17:13:19 2015 +0300
> >
> >     drm/i915: Use atomic helpers for computing changed flags
> >
> > we compute the plane state for a modeset before actually committing
> > any changes, which means crtc->active won't be correct yet. Looking at
> > future work in the modeset conversion targetting 4.3 the only places
> > where crtc_state->active isn't accurate is when disabling other CRTCs
> > than the one the modeset is for (when stealing connectors). Which
> > isn't the case here. And that's also confirmed by an audit, we do
> > unconditionally update crtc_state->active for the current pipe.
> >
> > We also don't need to update any other plane check functions since we
> > only ever add the primary state to the modeset update right now.
> >
> > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 647b1404c441..ba9321998a41 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13276,7 +13276,7 @@ intel_check_primary_plane(struct drm_plane *plane,
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (intel_crtc->active) {
> > +	if (crtc_state->base.active) {
> >  		struct intel_plane_state *old_state =
> >  			to_intel_plane_state(plane->state);
> >  
> I think this was probably a feature, not a bug. Since full atomic planes won't be part of v4.2
> both patches look ok to me.

I wasn't sure - in dinq we didn't convert his as part of the patches which
switched to check state->active in check functions, but only later on was
removed in some seemingly unrelated refactor. Not sure why this didn't
blow up more ...

> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Both applied, thanks for review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx