[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: 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

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.

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.


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