On Wed, 29 Oct 2014 16:30:43 +0200
Ander Conselvan de Oliveira <conselvan2@xxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
Thanks, I'll post v2 shortly.
Jesse
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
