| Message ID | 1465382507-23085-4-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 08, 2016 at 01:41:38PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Rather than looping through encoders to see which encoder types > are being driven by the pipe, add an output_types bitmask into > the crtc state and populate it prior to compute_config and during > state readout. > > v2: Determine output_types before .compute_config() hooks are called > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 51 +++++++++++++++--------------------- > drivers/gpu/drm/i915/intel_drv.h | 5 ++++ > 2 files changed, 26 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 12ff79594bc1..eabace447b1c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -535,14 +535,7 @@ needs_modeset(struct drm_crtc_state *state) > */ > bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type) > { > - struct drm_device *dev = crtc->base.dev; > - struct intel_encoder *encoder; > - > - for_each_encoder_on_crtc(dev, &crtc->base, encoder) > - if (encoder->type == type) > - return true; > - > - return false; > + return crtc->config->output_types & (1 << type); This could become inline and then all those if (intel_pipe_has_type(crtc, VGA) || intel_pipe_has_type(crtc, LVDS) || intel_pipe_has_type(crtc, HDMI)) whatnots will just disapear into a single test. I won't say how long those loops have been upsetting me without doing anything about them. > @@ -12529,6 +12503,19 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > &pipe_config->pipe_src_w, > &pipe_config->pipe_src_h); > > + for_each_connector_in_state(state, connector, connector_state, i) { > + if (connector_state->crtc != crtc) > + continue; > + > + encoder = to_intel_encoder(connector_state->best_encoder); > + > + /* > + * Determine output_types before calling the .compute_config() > + * hooks so that the hooks can use this information safely. > + */ > + pipe_config->output_types |= 1 << encoder->type; pipe_config->output_types |= BIT(encoder->type); Not my personal favourite in obfuscation, but one available to us if we desire. The code looks correct, I can't comment on ordering between computation of mask and use though. -Chris
On Wed, Jun 08, 2016 at 02:25:25PM +0100, Chris Wilson wrote: > On Wed, Jun 08, 2016 at 01:41:38PM +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Rather than looping through encoders to see which encoder types > > are being driven by the pipe, add an output_types bitmask into > > the crtc state and populate it prior to compute_config and during > > state readout. > > > > v2: Determine output_types before .compute_config() hooks are called > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 51 +++++++++++++++--------------------- > > drivers/gpu/drm/i915/intel_drv.h | 5 ++++ > > 2 files changed, 26 insertions(+), 30 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 12ff79594bc1..eabace447b1c 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -535,14 +535,7 @@ needs_modeset(struct drm_crtc_state *state) > > */ > > bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type) > > { > > - struct drm_device *dev = crtc->base.dev; > > - struct intel_encoder *encoder; > > - > > - for_each_encoder_on_crtc(dev, &crtc->base, encoder) > > - if (encoder->type == type) > > - return true; > > - > > - return false; > > + return crtc->config->output_types & (1 << type); > > This could become inline and then all those > > if (intel_pipe_has_type(crtc, VGA) || > intel_pipe_has_type(crtc, LVDS) || > intel_pipe_has_type(crtc, HDMI)) > > whatnots will just disapear into a single test. Sounds sane. Or we could just kill the entire function and just open code it. Not sure hiding it in a function buys us anything at this point. > > I won't say how long those loops have been upsetting me without doing > anything about them. > > > @@ -12529,6 +12503,19 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > > &pipe_config->pipe_src_w, > > &pipe_config->pipe_src_h); > > > > + for_each_connector_in_state(state, connector, connector_state, i) { > > + if (connector_state->crtc != crtc) > > + continue; > > + > > + encoder = to_intel_encoder(connector_state->best_encoder); > > + > > + /* > > + * Determine output_types before calling the .compute_config() > > + * hooks so that the hooks can use this information safely. > > + */ > > + pipe_config->output_types |= 1 << encoder->type; > > pipe_config->output_types |= BIT(encoder->type); > > Not my personal favourite in obfuscation, but one available to us if we > desire. I've been a bit wary about BIT() on account of the UL in there. But I suppose it might not make much of a difference in the end.
On Wed, Jun 08, 2016 at 01:41:38PM +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Rather than looping through encoders to see which encoder types > are being driven by the pipe, add an output_types bitmask into > the crtc state and populate it prior to compute_config and during > state readout. > > v2: Determine output_types before .compute_config() hooks are called > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Not sure whether this will mesh with LSPCON perfectly, but the atomic way is to pass connector_state and crtc_state to all encoder functions. That's at least how the atomic helpers in the core work, and it imo works well. i915 isn't there yet, but we want that anyway. Once you have the connector_state, the connector is just a pointer away. And the functions which care can look up the connector type. I'd like to go that direction since it would align i915 more with core atomic. And that misalignment is imo a big reason for why our transition is so super painful. -Daniel > --- > drivers/gpu/drm/i915/intel_display.c | 51 +++++++++++++++--------------------- > drivers/gpu/drm/i915/intel_drv.h | 5 ++++ > 2 files changed, 26 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 12ff79594bc1..eabace447b1c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -535,14 +535,7 @@ needs_modeset(struct drm_crtc_state *state) > */ > bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type) > { > - struct drm_device *dev = crtc->base.dev; > - struct intel_encoder *encoder; > - > - for_each_encoder_on_crtc(dev, &crtc->base, encoder) > - if (encoder->type == type) > - return true; > - > - return false; > + return crtc->config->output_types & (1 << type); > } > > /** > @@ -552,28 +545,9 @@ bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type) > * encoder->crtc. > */ > static bool intel_pipe_will_have_type(const struct intel_crtc_state *crtc_state, > - int type) > + enum intel_output_type type) > { > - struct drm_atomic_state *state = crtc_state->base.state; > - struct drm_connector *connector; > - struct drm_connector_state *connector_state; > - struct intel_encoder *encoder; > - int i, num_connectors = 0; > - > - for_each_connector_in_state(state, connector, connector_state, i) { > - if (connector_state->crtc != crtc_state->base.crtc) > - continue; > - > - num_connectors++; > - > - encoder = to_intel_encoder(connector_state->best_encoder); > - if (encoder->type == type) > - return true; > - } > - > - WARN_ON(num_connectors == 0); > - > - return false; > + return crtc_state->output_types & (1 << type); > } > > /* > @@ -12529,6 +12503,19 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > &pipe_config->pipe_src_w, > &pipe_config->pipe_src_h); > > + for_each_connector_in_state(state, connector, connector_state, i) { > + if (connector_state->crtc != crtc) > + continue; > + > + encoder = to_intel_encoder(connector_state->best_encoder); > + > + /* > + * Determine output_types before calling the .compute_config() > + * hooks so that the hooks can use this information safely. > + */ > + pipe_config->output_types |= 1 << encoder->type; > + } > + > encoder_retry: > /* Ensure the port clock defaults are reset when retrying. */ > pipe_config->port_clock = 0; > @@ -12826,6 +12813,7 @@ intel_pipe_config_compare(struct drm_device *dev, > PIPE_CONF_CHECK_M_N_ALT(dp_m_n, dp_m2_n2); > > PIPE_CONF_CHECK_I(has_dsi_encoder); > + PIPE_CONF_CHECK_X(output_types); > > PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay); > PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal); > @@ -13093,8 +13081,10 @@ verify_crtc_state(struct drm_crtc *crtc, > "Encoder connected to wrong pipe %c\n", > pipe_name(pipe)); > > - if (active) > + if (active) { > + pipe_config->output_types |= 1 << encoder->type; > encoder->get_config(encoder, pipe_config); > + } > } > > if (!new_crtc_state->active) > @@ -15985,6 +15975,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > if (encoder->get_hw_state(encoder, &pipe)) { > crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > encoder->base.crtc = &crtc->base; > + crtc->config->output_types |= 1 << encoder->type; > encoder->get_config(encoder, crtc->config); > } else { > encoder->base.crtc = NULL; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index ebe7b3427e2e..3dae05e6d95b 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -512,6 +512,11 @@ struct intel_crtc_state { > /* DSI has special cases */ > bool has_dsi_encoder; > > + /* Bitmask of encoder types (enum intel_output_type) > + * driven by the pipe. > + */ > + unsigned int output_types; > + > /* Whether we should send NULL infoframes. Required for audio. */ > bool has_hdmi_sink; > > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jun 13, 2016 at 04:25:55PM +0200, Daniel Vetter wrote: > On Wed, Jun 08, 2016 at 01:41:38PM +0300, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Rather than looping through encoders to see which encoder types > > are being driven by the pipe, add an output_types bitmask into > > the crtc state and populate it prior to compute_config and during > > state readout. > > > > v2: Determine output_types before .compute_config() hooks are called > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Not sure whether this will mesh with LSPCON perfectly, but the atomic way > is to pass connector_state and crtc_state to all encoder functions. That's > at least how the atomic helpers in the core work, and it imo works well. > i915 isn't there yet, but we want that anyway. > > Once you have the connector_state, the connector is just a pointer away. > And the functions which care can look up the connector type. > > I'd like to go that direction since it would align i915 more with core > atomic. And that misalignment is imo a big reason for why our transition > is so super painful. We want the bitmask anyway since we want to figure things out about the output type in places where we have no connector state (eg. DPLL code , or when when we'd have to deal with multiple connector states (cloning). > -Daniel > > > --- > > drivers/gpu/drm/i915/intel_display.c | 51 +++++++++++++++--------------------- > > drivers/gpu/drm/i915/intel_drv.h | 5 ++++ > > 2 files changed, 26 insertions(+), 30 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 12ff79594bc1..eabace447b1c 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -535,14 +535,7 @@ needs_modeset(struct drm_crtc_state *state) > > */ > > bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type) > > { > > - struct drm_device *dev = crtc->base.dev; > > - struct intel_encoder *encoder; > > - > > - for_each_encoder_on_crtc(dev, &crtc->base, encoder) > > - if (encoder->type == type) > > - return true; > > - > > - return false; > > + return crtc->config->output_types & (1 << type); > > } > > > > /** > > @@ -552,28 +545,9 @@ bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type) > > * encoder->crtc. > > */ > > static bool intel_pipe_will_have_type(const struct intel_crtc_state *crtc_state, > > - int type) > > + enum intel_output_type type) > > { > > - struct drm_atomic_state *state = crtc_state->base.state; > > - struct drm_connector *connector; > > - struct drm_connector_state *connector_state; > > - struct intel_encoder *encoder; > > - int i, num_connectors = 0; > > - > > - for_each_connector_in_state(state, connector, connector_state, i) { > > - if (connector_state->crtc != crtc_state->base.crtc) > > - continue; > > - > > - num_connectors++; > > - > > - encoder = to_intel_encoder(connector_state->best_encoder); > > - if (encoder->type == type) > > - return true; > > - } > > - > > - WARN_ON(num_connectors == 0); > > - > > - return false; > > + return crtc_state->output_types & (1 << type); > > } > > > > /* > > @@ -12529,6 +12503,19 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, > > &pipe_config->pipe_src_w, > > &pipe_config->pipe_src_h); > > > > + for_each_connector_in_state(state, connector, connector_state, i) { > > + if (connector_state->crtc != crtc) > > + continue; > > + > > + encoder = to_intel_encoder(connector_state->best_encoder); > > + > > + /* > > + * Determine output_types before calling the .compute_config() > > + * hooks so that the hooks can use this information safely. > > + */ > > + pipe_config->output_types |= 1 << encoder->type; > > + } > > + > > encoder_retry: > > /* Ensure the port clock defaults are reset when retrying. */ > > pipe_config->port_clock = 0; > > @@ -12826,6 +12813,7 @@ intel_pipe_config_compare(struct drm_device *dev, > > PIPE_CONF_CHECK_M_N_ALT(dp_m_n, dp_m2_n2); > > > > PIPE_CONF_CHECK_I(has_dsi_encoder); > > + PIPE_CONF_CHECK_X(output_types); > > > > PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay); > > PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal); > > @@ -13093,8 +13081,10 @@ verify_crtc_state(struct drm_crtc *crtc, > > "Encoder connected to wrong pipe %c\n", > > pipe_name(pipe)); > > > > - if (active) > > + if (active) { > > + pipe_config->output_types |= 1 << encoder->type; > > encoder->get_config(encoder, pipe_config); > > + } > > } > > > > if (!new_crtc_state->active) > > @@ -15985,6 +15975,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > > if (encoder->get_hw_state(encoder, &pipe)) { > > crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > > encoder->base.crtc = &crtc->base; > > + crtc->config->output_types |= 1 << encoder->type; > > encoder->get_config(encoder, crtc->config); > > } else { > > encoder->base.crtc = NULL; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index ebe7b3427e2e..3dae05e6d95b 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -512,6 +512,11 @@ struct intel_crtc_state { > > /* DSI has special cases */ > > bool has_dsi_encoder; > > > > + /* Bitmask of encoder types (enum intel_output_type) > > + * driven by the pipe. > > + */ > > + unsigned int output_types; > > + > > /* Whether we should send NULL infoframes. Required for audio. */ > > bool has_hdmi_sink; > > > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 12ff79594bc1..eabace447b1c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -535,14 +535,7 @@ needs_modeset(struct drm_crtc_state *state) */ bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type) { - struct drm_device *dev = crtc->base.dev; - struct intel_encoder *encoder; - - for_each_encoder_on_crtc(dev, &crtc->base, encoder) - if (encoder->type == type) - return true; - - return false; + return crtc->config->output_types & (1 << type); } /** @@ -552,28 +545,9 @@ bool intel_pipe_has_type(struct intel_crtc *crtc, enum intel_output_type type) * encoder->crtc. */ static bool intel_pipe_will_have_type(const struct intel_crtc_state *crtc_state, - int type) + enum intel_output_type type) { - struct drm_atomic_state *state = crtc_state->base.state; - struct drm_connector *connector; - struct drm_connector_state *connector_state; - struct intel_encoder *encoder; - int i, num_connectors = 0; - - for_each_connector_in_state(state, connector, connector_state, i) { - if (connector_state->crtc != crtc_state->base.crtc) - continue; - - num_connectors++; - - encoder = to_intel_encoder(connector_state->best_encoder); - if (encoder->type == type) - return true; - } - - WARN_ON(num_connectors == 0); - - return false; + return crtc_state->output_types & (1 << type); } /* @@ -12529,6 +12503,19 @@ intel_modeset_pipe_config(struct drm_crtc *crtc, &pipe_config->pipe_src_w, &pipe_config->pipe_src_h); + for_each_connector_in_state(state, connector, connector_state, i) { + if (connector_state->crtc != crtc) + continue; + + encoder = to_intel_encoder(connector_state->best_encoder); + + /* + * Determine output_types before calling the .compute_config() + * hooks so that the hooks can use this information safely. + */ + pipe_config->output_types |= 1 << encoder->type; + } + encoder_retry: /* Ensure the port clock defaults are reset when retrying. */ pipe_config->port_clock = 0; @@ -12826,6 +12813,7 @@ intel_pipe_config_compare(struct drm_device *dev, PIPE_CONF_CHECK_M_N_ALT(dp_m_n, dp_m2_n2); PIPE_CONF_CHECK_I(has_dsi_encoder); + PIPE_CONF_CHECK_X(output_types); PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay); PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal); @@ -13093,8 +13081,10 @@ verify_crtc_state(struct drm_crtc *crtc, "Encoder connected to wrong pipe %c\n", pipe_name(pipe)); - if (active) + if (active) { + pipe_config->output_types |= 1 << encoder->type; encoder->get_config(encoder, pipe_config); + } } if (!new_crtc_state->active) @@ -15985,6 +15975,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) if (encoder->get_hw_state(encoder, &pipe)) { crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); encoder->base.crtc = &crtc->base; + crtc->config->output_types |= 1 << encoder->type; encoder->get_config(encoder, crtc->config); } else { encoder->base.crtc = NULL; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index ebe7b3427e2e..3dae05e6d95b 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -512,6 +512,11 @@ struct intel_crtc_state { /* DSI has special cases */ bool has_dsi_encoder; + /* Bitmask of encoder types (enum intel_output_type) + * driven by the pipe. + */ + unsigned int output_types; + /* Whether we should send NULL infoframes. Required for audio. */ bool has_hdmi_sink;