| Message ID | 20210426185612.13223-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Stop using crtc->index as the pipe | expand |
On Mon, 2021-04-26 at 21:56 +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The pipe crc code slipped theough the net when we tried to > eliminate all crtc->index==pipe abuses. Remedy that. > > And while at it get rid of those nasty intel_crtc+drm_crtc > pointer aliases. intel_crtc is broadly used, also we have the same for other structs like intel_connector, in my option that is better than _crtc x crtc. The crtc->index change looks good to me. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_pipe_crc.c | 51 ++++++++++--------- > 1 file changed, 26 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_pipe_crc.c b/drivers/gpu/drm/i915/display/intel_pipe_crc.c > index 0f6de96e6d43..acc64b87d29f 100644 > --- a/drivers/gpu/drm/i915/display/intel_pipe_crc.c > +++ b/drivers/gpu/drm/i915/display/intel_pipe_crc.c > @@ -580,13 +580,14 @@ int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name, > return -EINVAL; > } > > > > > -int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name) > +int intel_crtc_set_crc_source(struct drm_crtc *_crtc, const char *source_name) > { > - struct drm_i915_private *dev_priv = to_i915(crtc->dev); > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - struct intel_pipe_crc *pipe_crc = &intel_crtc->pipe_crc; > + struct intel_crtc *crtc = to_intel_crtc(_crtc); > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + struct intel_pipe_crc *pipe_crc = &crtc->pipe_crc; > enum intel_display_power_domain power_domain; > enum intel_pipe_crc_source source; > + enum pipe pipe = crtc->pipe; > intel_wakeref_t wakeref; > u32 val = 0; /* shut up gcc */ > int ret = 0; > @@ -597,7 +598,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name) > return -EINVAL; > } > > > > > - power_domain = POWER_DOMAIN_PIPE(crtc->index); > + power_domain = POWER_DOMAIN_PIPE(pipe); > wakeref = intel_display_power_get_if_enabled(dev_priv, power_domain); > if (!wakeref) { > drm_dbg_kms(&dev_priv->drm, > @@ -607,64 +608,64 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name) > > > > > enable = source != INTEL_PIPE_CRC_SOURCE_NONE; > if (enable) > - intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), true); > + intel_crtc_crc_setup_workarounds(crtc, true); > > > > > - ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val); > + ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val); > if (ret != 0) > goto out; > > > > > pipe_crc->source = source; > - intel_de_write(dev_priv, PIPE_CRC_CTL(crtc->index), val); > - intel_de_posting_read(dev_priv, PIPE_CRC_CTL(crtc->index)); > + intel_de_write(dev_priv, PIPE_CRC_CTL(pipe), val); > + intel_de_posting_read(dev_priv, PIPE_CRC_CTL(pipe)); > > > > > if (!source) { > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > - vlv_undo_pipe_scramble_reset(dev_priv, crtc->index); > + vlv_undo_pipe_scramble_reset(dev_priv, pipe); > } > > > > > pipe_crc->skipped = 0; > > > > > out: > if (!enable) > - intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), false); > + intel_crtc_crc_setup_workarounds(crtc, false); > > > > > intel_display_power_put(dev_priv, power_domain, wakeref); > > > > > return ret; > } > > > > > -void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc) > +void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc) > { > - struct drm_crtc *crtc = &intel_crtc->base; > - struct drm_i915_private *dev_priv = to_i915(crtc->dev); > - struct intel_pipe_crc *pipe_crc = &intel_crtc->pipe_crc; > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + struct intel_pipe_crc *pipe_crc = &crtc->pipe_crc; > + enum pipe pipe = crtc->pipe; > u32 val = 0; > > > > > - if (!crtc->crc.opened) > + if (!crtc->base.crc.opened) > return; > > > > > - if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val) < 0) > + if (get_new_crc_ctl_reg(dev_priv, pipe, &pipe_crc->source, &val) < 0) > return; > > > > > /* Don't need pipe_crc->lock here, IRQs are not generated. */ > pipe_crc->skipped = 0; > > > > > - intel_de_write(dev_priv, PIPE_CRC_CTL(crtc->index), val); > - intel_de_posting_read(dev_priv, PIPE_CRC_CTL(crtc->index)); > + intel_de_write(dev_priv, PIPE_CRC_CTL(pipe), val); > + intel_de_posting_read(dev_priv, PIPE_CRC_CTL(pipe)); > } > > > > > -void intel_crtc_disable_pipe_crc(struct intel_crtc *intel_crtc) > +void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc) > { > - struct drm_crtc *crtc = &intel_crtc->base; > - struct drm_i915_private *dev_priv = to_i915(crtc->dev); > - struct intel_pipe_crc *pipe_crc = &intel_crtc->pipe_crc; > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + struct intel_pipe_crc *pipe_crc = &crtc->pipe_crc; > + enum pipe pipe = crtc->pipe; > > > > > /* Swallow crc's until we stop generating them. */ > spin_lock_irq(&pipe_crc->lock); > pipe_crc->skipped = INT_MIN; > spin_unlock_irq(&pipe_crc->lock); > > > > > - intel_de_write(dev_priv, PIPE_CRC_CTL(crtc->index), 0); > - intel_de_posting_read(dev_priv, PIPE_CRC_CTL(crtc->index)); > + intel_de_write(dev_priv, PIPE_CRC_CTL(pipe), 0); > + intel_de_posting_read(dev_priv, PIPE_CRC_CTL(pipe)); > intel_synchronize_irq(dev_priv); > }
On Tue, Apr 27, 2021 at 12:07:21AM +0000, Souza, Jose wrote: > On Mon, 2021-04-26 at 21:56 +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > The pipe crc code slipped theough the net when we tried to > > eliminate all crtc->index==pipe abuses. Remedy that. > > > > And while at it get rid of those nasty intel_crtc+drm_crtc > > pointer aliases. > > intel_crtc is broadly used, Not anymore. We've cleaned up almost all of it. Looks like only ~40 left vs. ~600 the other name. Probably a good time to clean up the rest finally. > also we have the same for other structs like intel_connector, in my option that is better than _crtc x crtc. The _crtc is explicitly ugly to make sure people leave it well alone. Otherwise we can never get rid of these horrible aliasing pointers. It should only make an appearance in core/helper vfuncs and such. At some point I was even pondering some kind of macro magic to create semi-automatic wrappers so that we could always just use the intel_ types in our vfunc implementations. intel_crtc we've cleaned up the most I think, intel_encoder a bit less perhaps, and intel_connector not really at all. Hence you see a lot more of intel_connector floating around. We also don't usually use the intel_ types for connector states. Mainly because most of the time you don't need anyting from there.
On Tue, 27 Apr 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Tue, Apr 27, 2021 at 12:07:21AM +0000, Souza, Jose wrote: >> On Mon, 2021-04-26 at 21:56 +0300, Ville Syrjala wrote: >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> > The pipe crc code slipped theough the net when we tried to >> > eliminate all crtc->index==pipe abuses. Remedy that. >> > >> > And while at it get rid of those nasty intel_crtc+drm_crtc >> > pointer aliases. >> >> intel_crtc is broadly used, > > Not anymore. We've cleaned up almost all of it. Looks like only ~40 left > vs. ~600 the other name. Probably a good time to clean up the rest > finally. Ack. > >> also we have the same for other structs like intel_connector, in my option that is better than _crtc x crtc. > > The _crtc is explicitly ugly to make sure people leave it well > alone. Otherwise we can never get rid of these horrible aliasing > pointers. It should only make an appearance in core/helper vfuncs > and such. At some point I was even pondering some kind of macro > magic to create semi-automatic wrappers so that we could always > just use the intel_ types in our vfunc implementations. > > intel_crtc we've cleaned up the most I think, intel_encoder a bit less > perhaps, and intel_connector not really at all. Hence you see a lot more > of intel_connector floating around. We also don't usually use the intel_ > types for connector states. Mainly because most of the time you don't > need anyting from there. Ack. BR, Jani.
On Mon, 26 Apr 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The pipe crc code slipped theough the net when we tried to > eliminate all crtc->index==pipe abuses. Remedy that. > > And while at it get rid of those nasty intel_crtc+drm_crtc > pointer aliases. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/display/intel_pipe_crc.c | 51 ++++++++++--------- > 1 file changed, 26 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_pipe_crc.c b/drivers/gpu/drm/i915/display/intel_pipe_crc.c > index 0f6de96e6d43..acc64b87d29f 100644 > --- a/drivers/gpu/drm/i915/display/intel_pipe_crc.c > +++ b/drivers/gpu/drm/i915/display/intel_pipe_crc.c > @@ -580,13 +580,14 @@ int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name, > return -EINVAL; > } > > -int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name) > +int intel_crtc_set_crc_source(struct drm_crtc *_crtc, const char *source_name) > { > - struct drm_i915_private *dev_priv = to_i915(crtc->dev); > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - struct intel_pipe_crc *pipe_crc = &intel_crtc->pipe_crc; > + struct intel_crtc *crtc = to_intel_crtc(_crtc); > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + struct intel_pipe_crc *pipe_crc = &crtc->pipe_crc; > enum intel_display_power_domain power_domain; > enum intel_pipe_crc_source source; > + enum pipe pipe = crtc->pipe; > intel_wakeref_t wakeref; > u32 val = 0; /* shut up gcc */ > int ret = 0; > @@ -597,7 +598,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name) > return -EINVAL; > } > > - power_domain = POWER_DOMAIN_PIPE(crtc->index); > + power_domain = POWER_DOMAIN_PIPE(pipe); > wakeref = intel_display_power_get_if_enabled(dev_priv, power_domain); > if (!wakeref) { > drm_dbg_kms(&dev_priv->drm, > @@ -607,64 +608,64 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name) > > enable = source != INTEL_PIPE_CRC_SOURCE_NONE; > if (enable) > - intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), true); > + intel_crtc_crc_setup_workarounds(crtc, true); > > - ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val); > + ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val); > if (ret != 0) > goto out; > > pipe_crc->source = source; > - intel_de_write(dev_priv, PIPE_CRC_CTL(crtc->index), val); > - intel_de_posting_read(dev_priv, PIPE_CRC_CTL(crtc->index)); > + intel_de_write(dev_priv, PIPE_CRC_CTL(pipe), val); > + intel_de_posting_read(dev_priv, PIPE_CRC_CTL(pipe)); > > if (!source) { > if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) > - vlv_undo_pipe_scramble_reset(dev_priv, crtc->index); > + vlv_undo_pipe_scramble_reset(dev_priv, pipe); > } > > pipe_crc->skipped = 0; > > out: > if (!enable) > - intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), false); > + intel_crtc_crc_setup_workarounds(crtc, false); > > intel_display_power_put(dev_priv, power_domain, wakeref); > > return ret; > } > > -void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc) > +void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc) > { > - struct drm_crtc *crtc = &intel_crtc->base; > - struct drm_i915_private *dev_priv = to_i915(crtc->dev); > - struct intel_pipe_crc *pipe_crc = &intel_crtc->pipe_crc; > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + struct intel_pipe_crc *pipe_crc = &crtc->pipe_crc; > + enum pipe pipe = crtc->pipe; > u32 val = 0; > > - if (!crtc->crc.opened) > + if (!crtc->base.crc.opened) > return; > > - if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val) < 0) > + if (get_new_crc_ctl_reg(dev_priv, pipe, &pipe_crc->source, &val) < 0) > return; > > /* Don't need pipe_crc->lock here, IRQs are not generated. */ > pipe_crc->skipped = 0; > > - intel_de_write(dev_priv, PIPE_CRC_CTL(crtc->index), val); > - intel_de_posting_read(dev_priv, PIPE_CRC_CTL(crtc->index)); > + intel_de_write(dev_priv, PIPE_CRC_CTL(pipe), val); > + intel_de_posting_read(dev_priv, PIPE_CRC_CTL(pipe)); > } > > -void intel_crtc_disable_pipe_crc(struct intel_crtc *intel_crtc) > +void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc) > { > - struct drm_crtc *crtc = &intel_crtc->base; > - struct drm_i915_private *dev_priv = to_i915(crtc->dev); > - struct intel_pipe_crc *pipe_crc = &intel_crtc->pipe_crc; > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > + struct intel_pipe_crc *pipe_crc = &crtc->pipe_crc; > + enum pipe pipe = crtc->pipe; > > /* Swallow crc's until we stop generating them. */ > spin_lock_irq(&pipe_crc->lock); > pipe_crc->skipped = INT_MIN; > spin_unlock_irq(&pipe_crc->lock); > > - intel_de_write(dev_priv, PIPE_CRC_CTL(crtc->index), 0); > - intel_de_posting_read(dev_priv, PIPE_CRC_CTL(crtc->index)); > + intel_de_write(dev_priv, PIPE_CRC_CTL(pipe), 0); > + intel_de_posting_read(dev_priv, PIPE_CRC_CTL(pipe)); > intel_synchronize_irq(dev_priv); > }
On Tue, 2021-04-27 at 10:51 +0300, Jani Nikula wrote: > On Tue, 27 Apr 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Tue, Apr 27, 2021 at 12:07:21AM +0000, Souza, Jose wrote: > > > On Mon, 2021-04-26 at 21:56 +0300, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > The pipe crc code slipped theough the net when we tried to > > > > eliminate all crtc->index==pipe abuses. Remedy that. > > > > > > > > And while at it get rid of those nasty intel_crtc+drm_crtc > > > > pointer aliases. > > > > > > intel_crtc is broadly used, > > > > Not anymore. We've cleaned up almost all of it. Looks like only ~40 left > > vs. ~600 the other name. Probably a good time to clean up the rest > > finally. > > Ack. > > > > > > also we have the same for other structs like intel_connector, in my option that is better than _crtc x crtc. > > > > The _crtc is explicitly ugly to make sure people leave it well > > alone. Otherwise we can never get rid of these horrible aliasing > > pointers. It should only make an appearance in core/helper vfuncs > > and such. At some point I was even pondering some kind of macro > > magic to create semi-automatic wrappers so that we could always > > just use the intel_ types in our vfunc implementations. > > > > intel_crtc we've cleaned up the most I think, intel_encoder a bit less > > perhaps, and intel_connector not really at all. Hence you see a lot more > > of intel_connector floating around. We also don't usually use the intel_ > > types for connector states. Mainly because most of the time you don't > > need anyting from there. > > Ack. Fair enough then Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > > BR, > Jani. > > >
diff --git a/drivers/gpu/drm/i915/display/intel_pipe_crc.c b/drivers/gpu/drm/i915/display/intel_pipe_crc.c index 0f6de96e6d43..acc64b87d29f 100644 --- a/drivers/gpu/drm/i915/display/intel_pipe_crc.c +++ b/drivers/gpu/drm/i915/display/intel_pipe_crc.c @@ -580,13 +580,14 @@ int intel_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name, return -EINVAL; } -int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name) +int intel_crtc_set_crc_source(struct drm_crtc *_crtc, const char *source_name) { - struct drm_i915_private *dev_priv = to_i915(crtc->dev); - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - struct intel_pipe_crc *pipe_crc = &intel_crtc->pipe_crc; + struct intel_crtc *crtc = to_intel_crtc(_crtc); + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + struct intel_pipe_crc *pipe_crc = &crtc->pipe_crc; enum intel_display_power_domain power_domain; enum intel_pipe_crc_source source; + enum pipe pipe = crtc->pipe; intel_wakeref_t wakeref; u32 val = 0; /* shut up gcc */ int ret = 0; @@ -597,7 +598,7 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name) return -EINVAL; } - power_domain = POWER_DOMAIN_PIPE(crtc->index); + power_domain = POWER_DOMAIN_PIPE(pipe); wakeref = intel_display_power_get_if_enabled(dev_priv, power_domain); if (!wakeref) { drm_dbg_kms(&dev_priv->drm, @@ -607,64 +608,64 @@ int intel_crtc_set_crc_source(struct drm_crtc *crtc, const char *source_name) enable = source != INTEL_PIPE_CRC_SOURCE_NONE; if (enable) - intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), true); + intel_crtc_crc_setup_workarounds(crtc, true); - ret = get_new_crc_ctl_reg(dev_priv, crtc->index, &source, &val); + ret = get_new_crc_ctl_reg(dev_priv, pipe, &source, &val); if (ret != 0) goto out; pipe_crc->source = source; - intel_de_write(dev_priv, PIPE_CRC_CTL(crtc->index), val); - intel_de_posting_read(dev_priv, PIPE_CRC_CTL(crtc->index)); + intel_de_write(dev_priv, PIPE_CRC_CTL(pipe), val); + intel_de_posting_read(dev_priv, PIPE_CRC_CTL(pipe)); if (!source) { if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) - vlv_undo_pipe_scramble_reset(dev_priv, crtc->index); + vlv_undo_pipe_scramble_reset(dev_priv, pipe); } pipe_crc->skipped = 0; out: if (!enable) - intel_crtc_crc_setup_workarounds(to_intel_crtc(crtc), false); + intel_crtc_crc_setup_workarounds(crtc, false); intel_display_power_put(dev_priv, power_domain, wakeref); return ret; } -void intel_crtc_enable_pipe_crc(struct intel_crtc *intel_crtc) +void intel_crtc_enable_pipe_crc(struct intel_crtc *crtc) { - struct drm_crtc *crtc = &intel_crtc->base; - struct drm_i915_private *dev_priv = to_i915(crtc->dev); - struct intel_pipe_crc *pipe_crc = &intel_crtc->pipe_crc; + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + struct intel_pipe_crc *pipe_crc = &crtc->pipe_crc; + enum pipe pipe = crtc->pipe; u32 val = 0; - if (!crtc->crc.opened) + if (!crtc->base.crc.opened) return; - if (get_new_crc_ctl_reg(dev_priv, crtc->index, &pipe_crc->source, &val) < 0) + if (get_new_crc_ctl_reg(dev_priv, pipe, &pipe_crc->source, &val) < 0) return; /* Don't need pipe_crc->lock here, IRQs are not generated. */ pipe_crc->skipped = 0; - intel_de_write(dev_priv, PIPE_CRC_CTL(crtc->index), val); - intel_de_posting_read(dev_priv, PIPE_CRC_CTL(crtc->index)); + intel_de_write(dev_priv, PIPE_CRC_CTL(pipe), val); + intel_de_posting_read(dev_priv, PIPE_CRC_CTL(pipe)); } -void intel_crtc_disable_pipe_crc(struct intel_crtc *intel_crtc) +void intel_crtc_disable_pipe_crc(struct intel_crtc *crtc) { - struct drm_crtc *crtc = &intel_crtc->base; - struct drm_i915_private *dev_priv = to_i915(crtc->dev); - struct intel_pipe_crc *pipe_crc = &intel_crtc->pipe_crc; + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); + struct intel_pipe_crc *pipe_crc = &crtc->pipe_crc; + enum pipe pipe = crtc->pipe; /* Swallow crc's until we stop generating them. */ spin_lock_irq(&pipe_crc->lock); pipe_crc->skipped = INT_MIN; spin_unlock_irq(&pipe_crc->lock); - intel_de_write(dev_priv, PIPE_CRC_CTL(crtc->index), 0); - intel_de_posting_read(dev_priv, PIPE_CRC_CTL(crtc->index)); + intel_de_write(dev_priv, PIPE_CRC_CTL(pipe), 0); + intel_de_posting_read(dev_priv, PIPE_CRC_CTL(pipe)); intel_synchronize_irq(dev_priv); }