[Intel-gfx] [PATCH 04/19] drm/i915: Allocate a crtc_state also when the crtc is being disabled

[Konduru, Chandra]

> -----Original Message-----
> From: Ander Conselvan De Oliveira [mailto:conselvan2 at gmail.com]
> Sent: Friday, March 20, 2015 1:41 AM
> To: Konduru, Chandra
> Cc: intel-gfx at lists.freedesktop.org; Roper, Matthew D
> Subject: Re: [PATCH 04/19] drm/i915: Allocate a crtc_state also when the crtc is
> being disabled
> 
> On Thu, 2015-03-19 at 23:23 +0000, Konduru, Chandra wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ander Conselvan De Oliveira [mailto:conselvan2 at gmail.com]
> > > Sent: Thursday, March 19, 2015 12:52 AM
> > > To: Konduru, Chandra
> > > Cc: intel-gfx at lists.freedesktop.org
> > > Subject: Re: [PATCH 04/19] drm/i915: Allocate a crtc_state also when
> > > the crtc is being disabled
> > >
> > > On Thu, 2015-03-19 at 00:12 +0000, Konduru, Chandra wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Conselvan De Oliveira, Ander
> > > > > Sent: Friday, March 13, 2015 2:49 AM
> > > > > To: intel-gfx at lists.freedesktop.org
> > > > > Cc: Konduru, Chandra; Conselvan De Oliveira, Ander
> > > > > Subject: [PATCH 04/19] drm/i915: Allocate a crtc_state also when
> > > > > the crtc is being disabled
> > > > >
> > > > > For consistency, allocate a new crtc_state for a crtc that is being
> disabled.
> > > > > Previously only the enabled value of the current state would change.
> > > > >
> > > > > Signed-off-by: Ander Conselvan de Oliveira
> > > > > <ander.conselvan.de.oliveira at intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c | 36
> > > > > +++++++++++++++++++++++++--------
> > > > > ---
> > > > >  1 file changed, 25 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > index b61e3f6..62b9021 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -11188,14 +11188,21 @@ intel_modeset_compute_config(struct
> > > > > drm_crtc *crtc,
> > > > >  			     unsigned *prepare_pipes,
> > > > >  			     unsigned *disable_pipes)  {
> > > > > +	struct drm_device *dev = crtc->dev;
> > > > >  	struct intel_crtc_state *pipe_config = NULL;
> > > > > +	struct intel_crtc *intel_crtc;
> > > > >  	int ret = 0;
> > > > >
> > > > >  	intel_modeset_affected_pipes(crtc, modeset_pipes,
> > > > >  				     prepare_pipes, disable_pipes);
> > > > >
> > > > > -	if ((*modeset_pipes) == 0)
> > > > > -		return NULL;
> > > > > +	for_each_intel_crtc_masked(dev, *disable_pipes, intel_crtc) {
> > > > > +		pipe_config = intel_atomic_get_crtc_state(state,
> intel_crtc);
> > > > > +		if (IS_ERR(pipe_config))
> > > > > +			return pipe_config;
> > > > > +
> > > > > +		pipe_config->base.enable = false;
> > > > > +	}
> > > > >
> > > > >  	/*
> > > > >  	 * Note this needs changes when we start tracking multiple
> > > > > modes @@ -
> > > > > 11203,18 +11210,25 @@ intel_modeset_compute_config(struct
> > > > > drm_crtc
> > > *crtc,
> > > > >  	 * (i.e. one pipe_config for each crtc) rather than just the one
> > > > >  	 * for this crtc.
> > > > >  	 */
> > > > > -	ret = intel_modeset_pipe_config(crtc, fb, mode, state);
> > > > > -	if (ret)
> > > > > -		return ERR_PTR(ret);
> > > > > +	for_each_intel_crtc_masked(dev, *modeset_pipes, intel_crtc) {
> > > > > +		/* FIXME: For now we still expect modeset_pipes has at
> most
> > > > > +		 * one bit set. */
> > > > > +		if (WARN_ON(&intel_crtc->base != crtc))
> > > > > +			continue;
> > > > >
> > > > > -	pipe_config = intel_atomic_get_crtc_state(state, to_intel_crtc(crtc));
> > > > > -	if (IS_ERR(pipe_config))
> > > > > -		return pipe_config;
> > > > > +		ret = intel_modeset_pipe_config(crtc, fb, mode, state);
> > > > > +		if (ret)
> > > > > +			return ERR_PTR(ret);
> > > > > +
> > > > > +		pipe_config = intel_atomic_get_crtc_state(state,
> intel_crtc);
> > > > > +		if (IS_ERR(pipe_config))
> > > > > +			return pipe_config;
> > > > >
> > > > > -	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> > > > > -			       "[modeset]");
> > > > > +		intel_dump_pipe_config(to_intel_crtc(crtc),
> pipe_config,
> > > > > +				       "[modeset]");
> > > > > +	}
> > > > >
> > > > > -	return pipe_config;
> > > > > +	return intel_atomic_get_crtc_state(state,
> > > > > +to_intel_crtc(crtc));
> > > > >  }
> > > >
> > > > Instead of calling 3 separate intel_atomic_get_crtc_state() Can
> > > > you have something like:
> > > > intel_modeset_compute_config()
> > > > {
> > > > 	pipe_config = intel_atomic_get_crtc_state(state, crtc);
> > > >
> > > > 	for each disabled pipe {
> > > > 		use pipe_config;
> > > > 	}
> > > >
> > > > 	for each mode_set pipe {
> > > > 		use pipe_config;
> > > > 	}
> > > >
> > > > 	return pipe_config;
> > > > }
> > > >
> > > >
> > > > Or the way currently done is to cover where disable_pipes !=
> modeset_pipes?
> > > > By the way, when can it happen?
> > >
> > > Yep, disable_pipes can be different than modeset_pipes if we the
> > > mode set "steals" a connector. For instance, we could have pipe A
> > > driving
> > > HDMI-1 and then mode set to pipe B to drive HDMI-1. Pipe B will
> > > steal the encoder from pipe A, and cause it to be disable. In that
> > > case disable_pipes will have the bit for pipe A set, while modeset_pipes will
> have the bit for pipe B set.
> >
> > 1)
> > Consider two simple scenarios:
> > Case1: User code moving HDMI from A to B:
> > drmModeSetCrtc(crtcA, HDMI);
> > drmModeSetCrtc(crtcB, HDMI); /* moving HDMI to pipe B */
> >
> > Case2: User code turning off HDMI:
> > drmModeSetCrtc(crtcA, HDMI);
> > drmModeSetCrtc(crtcA, disable);
> >
> > In both cases, driver will be disabling crtc for pipe A.
> > In case 1, there is no associated crtc_state or compute & commit but
> > directly triggering crtc_disable(crtcA).
> > In case 2, there is associated crtc_state and associated compute and
> > setmode calls crtc_disable(crtcA);
> >
> > Won't this cause trouble for low level functions (disable clocks,
> > connectors, encoders, planes etc. etc...) acting on variables being
> > computed and staged in their respective states?
> >     where case 1 calls with current crtc->config,
> >     and case 2 calls crt->config which is computed crtc_state
> 
> It is inconsistent, yes. But at the moment, for the disable case, we just duplicate
> the crtc_state and set crtc_state->base.enable = false.
> As things stand at the moment, the net effect should be the same: we call the
> disable hook before changing the current state, and after we change the states,
> the only field that changed was crtc_state->base.enable. The only difference is
> what does intel_crtc->config points to.

As things stand atm, this may be ok. But as soon as low level functions
truly depend on state and its hosting crtc state to act on, this becomes
an issue if crtc->config pointing to new state vs. current state.
I saw this as an issue while implementing scalers, but managed to make
it work. 
I think this is one another next steps as part of full atomic crtc.

> 
> > 2)
> > For example, to disable a plane differentiate between below two:
> > plane being called to disable with fb is valid
> > 	vs.
> > plane being called to disable with fb is null.
> >
> > There is crtc->active somehow to take care this, but I think this
> > should move to crtc_state. Same applies for any such other variables in crtc.
> > And respective resource's functions should check its hosting
> > crtc_state along with its own conditions to act on its resource.
> >
> > If not getting into this patch series, these changes should go into
> > next series for achieving crtc atomicity.
> 
> There's been discussion about crtc->active vs. crtc_state->base.active already.
> One problem is that the atomic semantics is different than the
> i915 one. 

Yeah, this is something we need to settle on.

> We use crtc->active internally to mean the pipe is really active, so we
> only turn that on in the enable crtc hook and immediately disable it in the disable
> hook. We then use that value for sanity checking.

I think it seems opposite to the atomic semantics. But will we be using
crtc_state->base.active in atomic semantics way? May be need to settle
on in next steps after this series if not now.

> 
> The atomic active field may actually be true before we are finished committing,
> so it may be true while the crtc is still off.
> 
> I think Matt had patches for this, but they were deferred until my patch series
> goes in.
> 
> > 3)
> > Also low level enable/disable functions can start causing confusion if
> > they aren't read/interpreted correctly. Either we should have
> > resource_commit which further calls resource->enable() or
> > resource->disable() depending on its own state and its hosting
> > resource state; or have resource_commit calling resource->update()
> > where it does either enable or disable based on state.
> >
> > We don't have above for crtc but should be done something like this if
> > not in this patch but sometime after in order to achieve crtc atomicity.
> 
> We will need something like that for the implementation of the atomic mode
> set, but I think we can treat that as an independent issue from this patch series.

That is fine as long as it is binned for next time.

> 
> 
> Ander
> 
> >
> >
> > >
> > >
> > > Ander
> > >
> > > >
> > > > >
> > > > >  static int __intel_set_mode_setup_plls(struct drm_device *dev,
> > > > > --
> > > > > 2.1.0
> > > >
> > >
> >
>