| Message ID | 1433165247-15928-8-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 01, 2015 at 03:27:10PM +0200, Maarten Lankhorst wrote: > Move the check for encoder cloning here. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_atomic.c | 5 +- > drivers/gpu/drm/i915/intel_display.c | 131 ++++++++++++++++++++--------------- > 2 files changed, 80 insertions(+), 56 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index 156df1b59ddd..1edd1651c045 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -100,7 +100,10 @@ int intel_atomic_check(struct drm_device *dev, > if (ret) > return ret; > > - /* FIXME: move to crtc atomic check function once it is ready */ > + /* > + * FIXME: move to crtc atomic check function once this is > + * more atomic friendly. > + */ > ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state); > if (ret) > return ret; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 6f3e96930da5..0060784525dc 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11404,11 +11404,87 @@ out_hang: > return ret; > } > > +static bool encoders_cloneable(const struct intel_encoder *a, > + const struct intel_encoder *b) > +{ > + /* masks could be asymmetric, so check both ways */ > + return a == b || (a->cloneable & (1 << b->type) && > + b->cloneable & (1 << a->type)); > +} > + > +static bool check_single_encoder_cloning(struct drm_atomic_state *state, > + struct intel_crtc *crtc, > + struct intel_encoder *encoder) > +{ > + struct intel_encoder *source_encoder; > + struct drm_connector *connector; > + struct drm_connector_state *connector_state; > + int i; > + > + for_each_connector_in_state(state, connector, connector_state, i) { > + if (connector_state->crtc != &crtc->base) > + continue; > + > + source_encoder = > + to_intel_encoder(connector_state->best_encoder); > + if (!encoders_cloneable(encoder, source_encoder)) > + return false; > + } > + > + return true; > +} > + > +static bool check_encoder_cloning(struct drm_atomic_state *state, > + struct intel_crtc *crtc) > +{ > + struct intel_encoder *encoder; > + struct drm_connector *connector; > + struct drm_connector_state *connector_state; > + int i; > + > + for_each_connector_in_state(state, connector, connector_state, i) { > + if (connector_state->crtc != &crtc->base) > + continue; > + > + encoder = to_intel_encoder(connector_state->best_encoder); > + if (!check_single_encoder_cloning(state, crtc, encoder)) > + return false; > + } > + > + return true; > +} > + > +static int intel_atomic_check_crtc(struct drm_crtc *crtc, > + struct drm_crtc_state *crtc_state) The plane equivalent is 'intel_plane_atomic_check'...it would be nice if we could keep the naming consistent (intel_crtc_atomic_check). Not sure if this should go in intel_display.c or intel_atomic.c. Maybe a future patch can shuffle a bunch of stuff around so things are a bit more consistent than they are today. > +{ > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct drm_atomic_state *state = crtc_state->state; > + int idx = crtc->base.id; > + bool is_crtc_enabled = crtc_state->active; > + bool was_crtc_enabled = crtc->state->active; > + bool mode_changed = needs_modeset(crtc_state); > + > + if (mode_changed && !check_encoder_cloning(state, intel_crtc)) { > + DRM_DEBUG_KMS("rejecting invalid cloning configuration\n"); > + return -EINVAL; > + } > + > + I915_STATE_WARN(crtc->state->active != intel_crtc->active, > + "[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n", > + idx, crtc->state->active, intel_crtc->active); > + > + DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n", > + idx, was_crtc_enabled, is_crtc_enabled); Maybe just print this debug message when the values aren't equal (i.e., an actual change) to cut down on a little spam when it isn't interesting? Matt > + > + return 0; > +} > + > static const struct drm_crtc_helper_funcs intel_helper_funcs = { > .mode_set_base_atomic = intel_pipe_set_base_atomic, > .load_lut = intel_crtc_load_lut, > .atomic_begin = intel_begin_crtc_commit, > .atomic_flush = intel_finish_crtc_commit, > + .atomic_check = intel_atomic_check_crtc, > }; > > /* Transitional helper to copy current connector/encoder state to > @@ -11634,56 +11710,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, > } > } > > -static bool encoders_cloneable(const struct intel_encoder *a, > - const struct intel_encoder *b) > -{ > - /* masks could be asymmetric, so check both ways */ > - return a == b || (a->cloneable & (1 << b->type) && > - b->cloneable & (1 << a->type)); > -} > - > -static bool check_single_encoder_cloning(struct drm_atomic_state *state, > - struct intel_crtc *crtc, > - struct intel_encoder *encoder) > -{ > - struct intel_encoder *source_encoder; > - struct drm_connector *connector; > - struct drm_connector_state *connector_state; > - int i; > - > - for_each_connector_in_state(state, connector, connector_state, i) { > - if (connector_state->crtc != &crtc->base) > - continue; > - > - source_encoder = > - to_intel_encoder(connector_state->best_encoder); > - if (!encoders_cloneable(encoder, source_encoder)) > - return false; > - } > - > - return true; > -} > - > -static bool check_encoder_cloning(struct drm_atomic_state *state, > - struct intel_crtc *crtc) > -{ > - struct intel_encoder *encoder; > - struct drm_connector *connector; > - struct drm_connector_state *connector_state; > - int i; > - > - for_each_connector_in_state(state, connector, connector_state, i) { > - if (connector_state->crtc != &crtc->base) > - continue; > - > - encoder = to_intel_encoder(connector_state->best_encoder); > - if (!check_single_encoder_cloning(state, crtc, encoder)) > - return false; > - } > - > - return true; > -} > - > static bool check_digital_port_conflicts(struct drm_atomic_state *state) > { > struct drm_device *dev = state->dev; > @@ -11770,11 +11796,6 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > int i; > bool retry = true; > > - if (!check_encoder_cloning(state, to_intel_crtc(crtc))) { > - DRM_DEBUG_KMS("rejecting invalid cloning configuration\n"); > - return -EINVAL; > - } > - > clear_intel_crtc_state(pipe_config); > > pipe_config->cpu_transcoder = > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 03-06-15 om 03:28 schreef Matt Roper: > On Mon, Jun 01, 2015 at 03:27:10PM +0200, Maarten Lankhorst wrote: >> Move the check for encoder cloning here. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_atomic.c | 5 +- >> drivers/gpu/drm/i915/intel_display.c | 131 ++++++++++++++++++++--------------- >> 2 files changed, 80 insertions(+), 56 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c >> index 156df1b59ddd..1edd1651c045 100644 >> --- a/drivers/gpu/drm/i915/intel_atomic.c >> +++ b/drivers/gpu/drm/i915/intel_atomic.c >> @@ -100,7 +100,10 @@ int intel_atomic_check(struct drm_device *dev, >> if (ret) >> return ret; >> >> - /* FIXME: move to crtc atomic check function once it is ready */ >> + /* >> + * FIXME: move to crtc atomic check function once this is >> + * more atomic friendly. >> + */ >> ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state); >> if (ret) >> return ret; >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 6f3e96930da5..0060784525dc 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -11404,11 +11404,87 @@ out_hang: >> return ret; >> } >> >> +static bool encoders_cloneable(const struct intel_encoder *a, >> + const struct intel_encoder *b) >> +{ >> + /* masks could be asymmetric, so check both ways */ >> + return a == b || (a->cloneable & (1 << b->type) && >> + b->cloneable & (1 << a->type)); >> +} >> + >> +static bool check_single_encoder_cloning(struct drm_atomic_state *state, >> + struct intel_crtc *crtc, >> + struct intel_encoder *encoder) >> +{ >> + struct intel_encoder *source_encoder; >> + struct drm_connector *connector; >> + struct drm_connector_state *connector_state; >> + int i; >> + >> + for_each_connector_in_state(state, connector, connector_state, i) { >> + if (connector_state->crtc != &crtc->base) >> + continue; >> + >> + source_encoder = >> + to_intel_encoder(connector_state->best_encoder); >> + if (!encoders_cloneable(encoder, source_encoder)) >> + return false; >> + } >> + >> + return true; >> +} >> + >> +static bool check_encoder_cloning(struct drm_atomic_state *state, >> + struct intel_crtc *crtc) >> +{ >> + struct intel_encoder *encoder; >> + struct drm_connector *connector; >> + struct drm_connector_state *connector_state; >> + int i; >> + >> + for_each_connector_in_state(state, connector, connector_state, i) { >> + if (connector_state->crtc != &crtc->base) >> + continue; >> + >> + encoder = to_intel_encoder(connector_state->best_encoder); >> + if (!check_single_encoder_cloning(state, crtc, encoder)) >> + return false; >> + } >> + >> + return true; >> +} >> + >> +static int intel_atomic_check_crtc(struct drm_crtc *crtc, >> + struct drm_crtc_state *crtc_state) > The plane equivalent is 'intel_plane_atomic_check'...it would be nice if > we could keep the naming consistent (intel_crtc_atomic_check). > > Not sure if this should go in intel_display.c or intel_atomic.c. Maybe > a future patch can shuffle a bunch of stuff around so things are a bit > more consistent than they are today. That's my intention. But for now keeping everything in 1 place is a lot easier. After it's hooked up as atomic_check/atomic_commit I can move it to intel_atomic.c, but for now maintaining such a patch would be busywork since it would break down every time I make an adjustment. > >> +{ >> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> + struct drm_atomic_state *state = crtc_state->state; >> + int idx = crtc->base.id; >> + bool is_crtc_enabled = crtc_state->active; >> + bool was_crtc_enabled = crtc->state->active; >> + bool mode_changed = needs_modeset(crtc_state); >> + >> + if (mode_changed && !check_encoder_cloning(state, intel_crtc)) { >> + DRM_DEBUG_KMS("rejecting invalid cloning configuration\n"); >> + return -EINVAL; >> + } >> + >> + I915_STATE_WARN(crtc->state->active != intel_crtc->active, >> + "[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n", >> + idx, crtc->state->active, intel_crtc->active); >> + >> + DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n", >> + idx, was_crtc_enabled, is_crtc_enabled); > Maybe just print this debug message when the values aren't equal (i.e., > an actual change) to cut down on a little spam when it isn't > interesting? I think it's not interesting to keep this message any more. I'll remove it. ~Maarten
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 156df1b59ddd..1edd1651c045 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -100,7 +100,10 @@ int intel_atomic_check(struct drm_device *dev, if (ret) return ret; - /* FIXME: move to crtc atomic check function once it is ready */ + /* + * FIXME: move to crtc atomic check function once this is + * more atomic friendly. + */ ret = intel_atomic_setup_scalers(dev, nuclear_crtc, crtc_state); if (ret) return ret; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6f3e96930da5..0060784525dc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11404,11 +11404,87 @@ out_hang: return ret; } +static bool encoders_cloneable(const struct intel_encoder *a, + const struct intel_encoder *b) +{ + /* masks could be asymmetric, so check both ways */ + return a == b || (a->cloneable & (1 << b->type) && + b->cloneable & (1 << a->type)); +} + +static bool check_single_encoder_cloning(struct drm_atomic_state *state, + struct intel_crtc *crtc, + struct intel_encoder *encoder) +{ + struct intel_encoder *source_encoder; + struct drm_connector *connector; + struct drm_connector_state *connector_state; + int i; + + for_each_connector_in_state(state, connector, connector_state, i) { + if (connector_state->crtc != &crtc->base) + continue; + + source_encoder = + to_intel_encoder(connector_state->best_encoder); + if (!encoders_cloneable(encoder, source_encoder)) + return false; + } + + return true; +} + +static bool check_encoder_cloning(struct drm_atomic_state *state, + struct intel_crtc *crtc) +{ + struct intel_encoder *encoder; + struct drm_connector *connector; + struct drm_connector_state *connector_state; + int i; + + for_each_connector_in_state(state, connector, connector_state, i) { + if (connector_state->crtc != &crtc->base) + continue; + + encoder = to_intel_encoder(connector_state->best_encoder); + if (!check_single_encoder_cloning(state, crtc, encoder)) + return false; + } + + return true; +} + +static int intel_atomic_check_crtc(struct drm_crtc *crtc, + struct drm_crtc_state *crtc_state) +{ + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct drm_atomic_state *state = crtc_state->state; + int idx = crtc->base.id; + bool is_crtc_enabled = crtc_state->active; + bool was_crtc_enabled = crtc->state->active; + bool mode_changed = needs_modeset(crtc_state); + + if (mode_changed && !check_encoder_cloning(state, intel_crtc)) { + DRM_DEBUG_KMS("rejecting invalid cloning configuration\n"); + return -EINVAL; + } + + I915_STATE_WARN(crtc->state->active != intel_crtc->active, + "[CRTC:%i] mismatch between state->active(%i) and crtc->active(%i)\n", + idx, crtc->state->active, intel_crtc->active); + + DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n", + idx, was_crtc_enabled, is_crtc_enabled); + + return 0; +} + static const struct drm_crtc_helper_funcs intel_helper_funcs = { .mode_set_base_atomic = intel_pipe_set_base_atomic, .load_lut = intel_crtc_load_lut, .atomic_begin = intel_begin_crtc_commit, .atomic_flush = intel_finish_crtc_commit, + .atomic_check = intel_atomic_check_crtc, }; /* Transitional helper to copy current connector/encoder state to @@ -11634,56 +11710,6 @@ static void intel_dump_pipe_config(struct intel_crtc *crtc, } } -static bool encoders_cloneable(const struct intel_encoder *a, - const struct intel_encoder *b) -{ - /* masks could be asymmetric, so check both ways */ - return a == b || (a->cloneable & (1 << b->type) && - b->cloneable & (1 << a->type)); -} - -static bool check_single_encoder_cloning(struct drm_atomic_state *state, - struct intel_crtc *crtc, - struct intel_encoder *encoder) -{ - struct intel_encoder *source_encoder; - struct drm_connector *connector; - struct drm_connector_state *connector_state; - int i; - - for_each_connector_in_state(state, connector, connector_state, i) { - if (connector_state->crtc != &crtc->base) - continue; - - source_encoder = - to_intel_encoder(connector_state->best_encoder); - if (!encoders_cloneable(encoder, source_encoder)) - return false; - } - - return true; -} - -static bool check_encoder_cloning(struct drm_atomic_state *state, - struct intel_crtc *crtc) -{ - struct intel_encoder *encoder; - struct drm_connector *connector; - struct drm_connector_state *connector_state; - int i; - - for_each_connector_in_state(state, connector, connector_state, i) { - if (connector_state->crtc != &crtc->base) - continue; - - encoder = to_intel_encoder(connector_state->best_encoder); - if (!check_single_encoder_cloning(state, crtc, encoder)) - return false; - } - - return true; -} - static bool check_digital_port_conflicts(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; @@ -11770,11 +11796,6 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, int i; bool retry = true; - if (!check_encoder_cloning(state, to_intel_crtc(crtc))) { - DRM_DEBUG_KMS("rejecting invalid cloning configuration\n"); - return -EINVAL; - } - clear_intel_crtc_state(pipe_config); pipe_config->cpu_transcoder =
Move the check for encoder cloning here. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_atomic.c | 5 +- drivers/gpu/drm/i915/intel_display.c | 131 ++++++++++++++++++++--------------- 2 files changed, 80 insertions(+), 56 deletions(-)