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

Fri Mar 20 01:40:37 PDT 2015

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.

> 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. 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.

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.

Ander

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