diff mbox

[13/21] drm/i915: Preserve scaler state when clearing crtc_state

Message ID 1426398946-5900-14-git-send-email-chandra.konduru@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chandra Konduru March 15, 2015, 5:55 a.m. UTC
crtc_state is cleared during mode set which wipes out complete
scaler state too. This is causing issues. To fix, ensure scaler
state is preserved because it contains not only crtc
scaler usage, but also planes using scalers on this crtc.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Daniel Vetter March 17, 2015, 2:17 p.m. UTC | #1
On Sat, Mar 14, 2015 at 10:55:38PM -0700, Chandra Konduru wrote:
> crtc_state is cleared during mode set which wipes out complete
> scaler state too. This is causing issues. To fix, ensure scaler
> state is preserved because it contains not only crtc
> scaler usage, but also planes using scalers on this crtc.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

I guess this is going to conflict with Ander's crtc state series. But
since you're signed up to review that I guess you will know what needs to
be changed here. Hopefully Ander's series will just take care of this.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e07c24e..9c3c6b1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10617,11 +10617,14 @@ static void
>  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_crtc_state tmp_state;
> +	struct intel_crtc_scaler_state scaler_state;
>  
> -	/* Clear only the intel specific part of the crtc state */
> +	/* Clear only the intel specific part of the crtc state excluding scalers */
>  	tmp_state = crtc_state->base;
> +	scaler_state = crtc_state->scaler_state;
>  	memset(crtc_state, 0, sizeof *crtc_state);
>  	crtc_state->base = tmp_state;
> +	crtc_state->scaler_state = scaler_state;
>  }
>  
>  static int
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chandra Konduru March 17, 2015, 6:11 p.m. UTC | #2
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, March 17, 2015 7:17 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 13/21] drm/i915: Preserve scaler state when
> clearing crtc_state
> 
> On Sat, Mar 14, 2015 at 10:55:38PM -0700, Chandra Konduru wrote:
> > crtc_state is cleared during mode set which wipes out complete scaler
> > state too. This is causing issues. To fix, ensure scaler state is
> > preserved because it contains not only crtc scaler usage, but also
> > planes using scalers on this crtc.
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> 
> I guess this is going to conflict with Ander's crtc state series. But since you're
> signed up to review that I guess you will know what needs to be changed here.
> Hopefully Ander's series will just take care of this.

Planes may have changed its scaling needs and updated scaler_users
is in crtc_state. Wiping out crtc state will wipeout staged scaler_state 
and cause incorrect behavior. So preserving computed (or staged) 
scaler state into crtc state.

Ander, 
Can you pls check for any issue here?

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index e07c24e..9c3c6b1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10617,11 +10617,14 @@ static void
> >  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)  {
> >  	struct drm_crtc_state tmp_state;
> > +	struct intel_crtc_scaler_state scaler_state;
> >
> > -	/* Clear only the intel specific part of the crtc state */
> > +	/* Clear only the intel specific part of the crtc state excluding
> > +scalers */
> >  	tmp_state = crtc_state->base;
> > +	scaler_state = crtc_state->scaler_state;
> >  	memset(crtc_state, 0, sizeof *crtc_state);
> >  	crtc_state->base = tmp_state;
> > +	crtc_state->scaler_state = scaler_state;
> >  }
> >
> >  static int
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Ander Conselvan de Oliveira March 18, 2015, 6:24 a.m. UTC | #3
On Tue, 2015-03-17 at 18:11 +0000, Konduru, Chandra wrote:
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Tuesday, March 17, 2015 7:17 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander; Vetter, Daniel
> > Subject: Re: [Intel-gfx] [PATCH 13/21] drm/i915: Preserve scaler state when
> > clearing crtc_state
> > 
> > On Sat, Mar 14, 2015 at 10:55:38PM -0700, Chandra Konduru wrote:
> > > crtc_state is cleared during mode set which wipes out complete scaler
> > > state too. This is causing issues. To fix, ensure scaler state is
> > > preserved because it contains not only crtc scaler usage, but also
> > > planes using scalers on this crtc.
> > >
> > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > 
> > I guess this is going to conflict with Ander's crtc state series. But since you're
> > signed up to review that I guess you will know what needs to be changed here.
> > Hopefully Ander's series will just take care of this.
> 
> Planes may have changed its scaling needs and updated scaler_users
> is in crtc_state. Wiping out crtc state will wipeout staged scaler_state 
> and cause incorrect behavior. So preserving computed (or staged) 
> scaler state into crtc state.
> 
> Ander, 
> Can you pls check for any issue here?

As is your patch shouldn't cause any trouble. The problem we have is
that the legacy modeset path assumes the new pipe_config is kzalloc'd. I
need to inspect all that code and check what really depends on the state
being zeroed, but I haven't got to it yet. Once that's done, we could
just duplicate the whole crtc state and be done with it.

Ander

> 
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |    5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index e07c24e..9c3c6b1 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -10617,11 +10617,14 @@ static void
> > >  clear_intel_crtc_state(struct intel_crtc_state *crtc_state)  {
> > >  	struct drm_crtc_state tmp_state;
> > > +	struct intel_crtc_scaler_state scaler_state;
> > >
> > > -	/* Clear only the intel specific part of the crtc state */
> > > +	/* Clear only the intel specific part of the crtc state excluding
> > > +scalers */
> > >  	tmp_state = crtc_state->base;
> > > +	scaler_state = crtc_state->scaler_state;
> > >  	memset(crtc_state, 0, sizeof *crtc_state);
> > >  	crtc_state->base = tmp_state;
> > > +	crtc_state->scaler_state = scaler_state;
> > >  }
> > >
> > >  static int
> > > --
> > > 1.7.9.5
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e07c24e..9c3c6b1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10617,11 +10617,14 @@  static void
 clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 {
 	struct drm_crtc_state tmp_state;
+	struct intel_crtc_scaler_state scaler_state;
 
-	/* Clear only the intel specific part of the crtc state */
+	/* Clear only the intel specific part of the crtc state excluding scalers */
 	tmp_state = crtc_state->base;
+	scaler_state = crtc_state->scaler_state;
 	memset(crtc_state, 0, sizeof *crtc_state);
 	crtc_state->base = tmp_state;
+	crtc_state->scaler_state = scaler_state;
 }
 
 static int