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

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

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


Ander

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


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