| Message ID | 1432137874-20543-5-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 20, 2015 at 06:04:26PM +0200, maarten.lankhorst@linux.intel.com wrote: > From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > It makes more sense there, since these are computation steps that can > fail. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> I've noticed that a few of the patches in this series were originally written by Ander, but seem to be missing his s-o-b line. I think you generally want to just append your line after his in that case. One other cosmetic note farther down. > --- > drivers/gpu/drm/i915/intel_display.c | 70 ++++++++++++++++++------------------ > 1 file changed, 35 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 81d5358efdde..e7aa8610cbdc 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12085,37 +12085,6 @@ static void update_scanline_offset(struct intel_crtc *crtc) > crtc->scanline_offset = 1; > } > > -static int > -intel_modeset_compute_config(struct drm_atomic_state *state) > -{ > - struct drm_crtc *crtc; > - struct drm_crtc_state *crtc_state; > - int ret, i; > - > - ret = drm_atomic_helper_check_modeset(state->dev, state); > - if (ret) > - return ret; > - > - for_each_crtc_in_state(state, crtc, crtc_state, i) { > - if (!crtc_state->enable && > - WARN_ON(crtc_state->active)) > - crtc_state->active = false; > - > - if (!crtc_state->enable) > - continue; > - > - ret = intel_modeset_pipe_config(crtc, state); > - if (ret) > - return ret; > - > - intel_dump_pipe_config(to_intel_crtc(crtc), > - to_intel_crtc_state(crtc_state), > - "[modeset]"); > - } > - > - return drm_atomic_helper_check_planes(state->dev, state); > -} > - > static int __intel_set_mode_setup_plls(struct drm_atomic_state *state) > { > struct drm_device *dev = state->dev; > @@ -12191,6 +12160,41 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state) > return 0; > } > > +static int > +intel_modeset_compute_config(struct drm_atomic_state *state) > +{ > + struct drm_crtc *crtc; > + struct drm_crtc_state *crtc_state; > + int ret, i; > + > + ret = drm_atomic_helper_check_modeset(state->dev, state); > + if (ret) > + return ret; > + > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + if (!crtc_state->enable && > + WARN_ON(crtc_state->active)) > + crtc_state->active = false; > + > + if (!crtc_state->enable) > + continue; > + > + ret = intel_modeset_pipe_config(crtc, state); > + if (ret) > + return ret; > + > + intel_dump_pipe_config(to_intel_crtc(crtc), > + to_intel_crtc_state(crtc_state), > + "[modeset]"); > + } > + > + ret = drm_atomic_helper_check_planes(state->dev, state); > + if (ret) > + return ret; > + > + return __intel_set_mode_checks(state); Just a cosmetic note, but maybe we should rename this function now? It's not called from __intel_set_mode anymore and it isn't really 'checks' (but rather setup that we intend to be done during the check phase), so the whole name seems a bit misleading now. Matt > +} > + > static int __intel_set_mode(struct drm_atomic_state *state) > { > struct drm_device *dev = state->dev; > @@ -12200,10 +12204,6 @@ static int __intel_set_mode(struct drm_atomic_state *state) > int ret = 0; > int i; > > - ret = __intel_set_mode_checks(state); > - if (ret < 0) > - return ret; > - > ret = drm_atomic_helper_prepare_planes(dev, state); > if (ret) > return ret; > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 29-05-15 om 02:55 schreef Matt Roper: > On Wed, May 20, 2015 at 06:04:26PM +0200, maarten.lankhorst@linux.intel.com wrote: >> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> >> >> It makes more sense there, since these are computation steps that can >> fail. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > I've noticed that a few of the patches in this series were originally > written by Ander, but seem to be missing his s-o-b line. I think you > generally want to just append your line after his in that case. > > One other cosmetic note farther down. > >> --- >> drivers/gpu/drm/i915/intel_display.c | 70 ++++++++++++++++++------------------ >> 1 file changed, 35 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 81d5358efdde..e7aa8610cbdc 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -12085,37 +12085,6 @@ static void update_scanline_offset(struct intel_crtc *crtc) >> crtc->scanline_offset = 1; >> } >> >> -static int >> -intel_modeset_compute_config(struct drm_atomic_state *state) >> -{ >> - struct drm_crtc *crtc; >> - struct drm_crtc_state *crtc_state; >> - int ret, i; >> - >> - ret = drm_atomic_helper_check_modeset(state->dev, state); >> - if (ret) >> - return ret; >> - >> - for_each_crtc_in_state(state, crtc, crtc_state, i) { >> - if (!crtc_state->enable && >> - WARN_ON(crtc_state->active)) >> - crtc_state->active = false; >> - >> - if (!crtc_state->enable) >> - continue; >> - >> - ret = intel_modeset_pipe_config(crtc, state); >> - if (ret) >> - return ret; >> - >> - intel_dump_pipe_config(to_intel_crtc(crtc), >> - to_intel_crtc_state(crtc_state), >> - "[modeset]"); >> - } >> - >> - return drm_atomic_helper_check_planes(state->dev, state); >> -} >> - >> static int __intel_set_mode_setup_plls(struct drm_atomic_state *state) >> { >> struct drm_device *dev = state->dev; >> @@ -12191,6 +12160,41 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state) >> return 0; >> } >> >> +static int >> +intel_modeset_compute_config(struct drm_atomic_state *state) >> +{ >> + struct drm_crtc *crtc; >> + struct drm_crtc_state *crtc_state; >> + int ret, i; >> + >> + ret = drm_atomic_helper_check_modeset(state->dev, state); >> + if (ret) >> + return ret; >> + >> + for_each_crtc_in_state(state, crtc, crtc_state, i) { >> + if (!crtc_state->enable && >> + WARN_ON(crtc_state->active)) >> + crtc_state->active = false; >> + >> + if (!crtc_state->enable) >> + continue; >> + >> + ret = intel_modeset_pipe_config(crtc, state); >> + if (ret) >> + return ret; >> + >> + intel_dump_pipe_config(to_intel_crtc(crtc), >> + to_intel_crtc_state(crtc_state), >> + "[modeset]"); >> + } >> + >> + ret = drm_atomic_helper_check_planes(state->dev, state); >> + if (ret) >> + return ret; >> + >> + return __intel_set_mode_checks(state); > Just a cosmetic note, but maybe we should rename this function now? > It's not called from __intel_set_mode anymore and it isn't really > 'checks' (but rather setup that we intend to be done during the check > phase), so the whole name seems a bit misleading now. > Later on intel_modeset_compute_config gets renamed to intel_atomic_check, and I conditionally run intel_set_mode_checks depending on whether a modeset is requested or not. I guess __intel_set_mode_checks could be renamed to intel_modeset_checks. ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 81d5358efdde..e7aa8610cbdc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12085,37 +12085,6 @@ static void update_scanline_offset(struct intel_crtc *crtc) crtc->scanline_offset = 1; } -static int -intel_modeset_compute_config(struct drm_atomic_state *state) -{ - struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; - int ret, i; - - ret = drm_atomic_helper_check_modeset(state->dev, state); - if (ret) - return ret; - - for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (!crtc_state->enable && - WARN_ON(crtc_state->active)) - crtc_state->active = false; - - if (!crtc_state->enable) - continue; - - ret = intel_modeset_pipe_config(crtc, state); - if (ret) - return ret; - - intel_dump_pipe_config(to_intel_crtc(crtc), - to_intel_crtc_state(crtc_state), - "[modeset]"); - } - - return drm_atomic_helper_check_planes(state->dev, state); -} - static int __intel_set_mode_setup_plls(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; @@ -12191,6 +12160,41 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state) return 0; } +static int +intel_modeset_compute_config(struct drm_atomic_state *state) +{ + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + int ret, i; + + ret = drm_atomic_helper_check_modeset(state->dev, state); + if (ret) + return ret; + + for_each_crtc_in_state(state, crtc, crtc_state, i) { + if (!crtc_state->enable && + WARN_ON(crtc_state->active)) + crtc_state->active = false; + + if (!crtc_state->enable) + continue; + + ret = intel_modeset_pipe_config(crtc, state); + if (ret) + return ret; + + intel_dump_pipe_config(to_intel_crtc(crtc), + to_intel_crtc_state(crtc_state), + "[modeset]"); + } + + ret = drm_atomic_helper_check_planes(state->dev, state); + if (ret) + return ret; + + return __intel_set_mode_checks(state); +} + static int __intel_set_mode(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; @@ -12200,10 +12204,6 @@ static int __intel_set_mode(struct drm_atomic_state *state) int ret = 0; int i; - ret = __intel_set_mode_checks(state); - if (ret < 0) - return ret; - ret = drm_atomic_helper_prepare_planes(dev, state); if (ret) return ret;