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

From: Ander Conselvan De Oliveira <conselvan2@gmail.com>
To: "Konduru, Chandra" <chandra.konduru@intel.com>
Cc: "Vetter, Daniel" <daniel.vetter@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 13/21] drm/i915: Preserve scaler state when clearing crtc_state
Date: Wed, 18 Mar 2015 08:24:27 +0200	[thread overview]
Message-ID: <1426659867.2632.3.camel@gmail.com> (raw)
In-Reply-To: <76A9B330A4D78C4D99CB292C4CC06C0E36F7A00A@fmsmsx101.amr.corp.intel.com>

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

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