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

[Jesse Barnes]

On Wed, 29 Oct 2014 16:30:43 +0200
Ander Conselvan de Oliveira <conselvan2 at gmail.com> wrote:

> On 10/23/2014 09:50 PM, Jesse Barnes wrote:
> > This allows us to calculate the full pipe config before we do any
> > mode setting work.
> >
> > Signed-off-by: Jesse Barnes <jbarnes at virtuousgeek.org>
> > ---
> >   drivers/gpu/drm/i915/intel_display.c | 93
> > +++++++++++++++++++++++++----------- 1 file changed, 65
> > insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c index 6e5bc3c..d5f95e4 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10910,45 +10910,62 @@ static void update_scanline_offset(struct
> > intel_crtc *crtc) crtc->scanline_offset = 1;
> >   }
> >
> > +static int intel_modeset_compute_config(struct drm_crtc *crtc,
> 
> s/drm_crtc/intel_crtc/

Since 2/3 of the functions this one calls take a drm_crtc I figured I'd
leave it for a big cleanup patch (maybe cocci could do it).

> > +	/* Hack: Because we don't (yet) support global modeset on
> > multiple
> > +	 * crtcs, we don't keep track of the new mode for more
> > than one crtc.
> > +	 * Hence simply check whether any bit is set in
> > modeset_pipes in all the
> > +	 * pieces of code that are not yet converted to deal with
> > mutliple crtcs
> > +	 * changing their mode at the same time. */
> 
> This comment seems out of place here since it refers to checking for
> set bits in modeset_pipes. I guess the important part of it is to
> explain the fact that only one new pipe config is obtained here, so
> maybe it can reworded to make that clearer.
> 
> And there's another "if (modeset_pipes)" left in __intel_set_mode()
> to which this comment applies.

I tried to clear these up a bit, hopefully I didn't make things worse.

> 
> > +	pipe_config = intel_modeset_pipe_config(crtc, fb, mode);
> > +	if (IS_ERR(pipe_config)) {
> > +		ret = PTR_ERR(pipe_config);
> > +		pipe_config = NULL;
> > +
> > +		goto out;
> 
> Nothing will use pipe_config after this point, so there's no need to
> set it to NULL.

Yep, fixed.

> 
> > +	}
> > +	intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,
> > +			       "[modeset]");
> > +	to_intel_crtc(crtc)->new_config = pipe_config;
> > +
> > +out:
> > +	return ret;
> 
> There's nothing being done here, so we could avoid this goto dance
> and just use returns instead.

Yeah I just tend to use the "return only in one place" style, but I
have no strong preference if someone wants to change it.

> 
> Anyway, mostly nit picks, so either way,
> 
> Reviewed-by: Ander Conselvan de Oliveira 
> <ander.conselvan.de.oliveira at intel.com>

Thanks, I'll post v2 shortly.

Jesse