[Intel-gfx] [PATCH 1/6] drm/i915: factor out compute_config from __intel_set_mode v3

[Daniel Vetter]

On Thu, Nov 06, 2014 at 09:04:14AM +0000, Chris Wilson wrote:
> On Wed, Nov 05, 2014 at 02:26:06PM -0800, Jesse Barnes wrote:
> > +static int intel_set_mode(struct drm_crtc *crtc,
> > +			  struct drm_display_mode *mode,
> > +			  int x, int y, struct drm_framebuffer *fb)
> > +{
> > +	struct intel_crtc_config *pipe_config;
> > +	unsigned modeset_pipes, prepare_pipes, disable_pipes;
> > +
> > +	pipe_config = intel_modeset_compute_config(crtc, mode, fb,
> > +						   &modeset_pipes,
> > +						   &prepare_pipes,
> > +						   &disable_pipes);
> > +
> > +	if (IS_ERR(pipe_config))
> > +		return PTR_ERR(pipe_config);
> > +
> > +	return intel_set_mode_pipes(crtc, mode, x, y, fb, pipe_config,
> > +				    modeset_pipes, prepare_pipes,
> > +				    disable_pipes);
> > +}
> 
> intel_set_mode() -> intel_set_mode_pipes() -> __intel_set_mode() wins
> this morning's prize for causing confusion.
> 
> Does it make sense to wrap intel_crtc_config + pipes in a new struct to
> avoid passing 4 new parameters down the chain? Or will that just be
> extra churn to be rewritten by atomic?

Atomic has one big structure to track all the updated per-object states
(for crtcs, planes & connectors). Atm there's no provisions for
subclassing it, but would be trivial to add. Then we could throw all this
additional i915 state in there. I guess we might as well start with that.

One funny problem btw is all these ->new_ pointers we sprinkle all over
the place. Upstream atomic has completely free-standing new state objects,
with a bunch of helpers to fetch the new state for a given object from the
overall atomic update structure. So we have a bit of an impendance
mismatch. But I think we just need to handle that by setting (and
resetting when we don't actually commit the new state) these pointers when
entering the i915 atomic backend.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch