| Message ID | 20180621130330.20219-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 21, 2018 at 04:03:30PM +0300, Jani Nikula wrote: > Commit 9504a8924759 ("drm/i915/vlv: Reset the ADPA in > vlv_display_power_well_init()") started calling intel_crt_reset() > directly, while we could just as well use the hooks and keep the > function static. > > Cc: Lyude <cpaul@redhat.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/intel_crt.c | 2 +- > drivers/gpu/drm/i915/intel_drv.h | 1 - > drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- > 3 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c > index 0c6bf82bb059..c2cb3b7a255b 100644 > --- a/drivers/gpu/drm/i915/intel_crt.c > +++ b/drivers/gpu/drm/i915/intel_crt.c > @@ -881,7 +881,7 @@ static int intel_crt_get_modes(struct drm_connector *connector) > return ret; > } > > -void intel_crt_reset(struct drm_encoder *encoder) > +static void intel_crt_reset(struct drm_encoder *encoder) > { > struct drm_i915_private *dev_priv = to_i915(encoder->dev); > struct intel_crt *crt = intel_encoder_to_crt(to_intel_encoder(encoder)); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 0c3ac0eafde0..b2002fee1b58 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1373,7 +1373,6 @@ void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv); > bool intel_crt_port_enabled(struct drm_i915_private *dev_priv, > i915_reg_t adpa_reg, enum pipe *pipe); > void intel_crt_init(struct drm_i915_private *dev_priv); > -void intel_crt_reset(struct drm_encoder *encoder); > > /* intel_ddi.c */ > void intel_ddi_fdi_post_disable(struct intel_encoder *intel_encoder, > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index de3a81034f77..0b3da5818383 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -972,7 +972,7 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv) > /* Re-enable the ADPA, if we have one */ > for_each_intel_encoder(&dev_priv->drm, encoder) { > if (encoder->type == INTEL_OUTPUT_ANALOG) > - intel_crt_reset(&encoder->base); > + encoder->base.funcs->reset(&encoder->base); I have a feeling I requested the direct call to make it less annoying to figure out what it's actually calling. But if people prefer the function pointer version I can live with that too. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > } > > i915_redisable_vga_power_on(dev_priv); > -- > 2.11.0
On Thu, 21 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Jun 21, 2018 at 04:03:30PM +0300, Jani Nikula wrote: >> Commit 9504a8924759 ("drm/i915/vlv: Reset the ADPA in >> vlv_display_power_well_init()") started calling intel_crt_reset() >> directly, while we could just as well use the hooks and keep the >> function static. >> >> Cc: Lyude <cpaul@redhat.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/intel_crt.c | 2 +- >> drivers/gpu/drm/i915/intel_drv.h | 1 - >> drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- >> 3 files changed, 2 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c >> index 0c6bf82bb059..c2cb3b7a255b 100644 >> --- a/drivers/gpu/drm/i915/intel_crt.c >> +++ b/drivers/gpu/drm/i915/intel_crt.c >> @@ -881,7 +881,7 @@ static int intel_crt_get_modes(struct drm_connector *connector) >> return ret; >> } >> >> -void intel_crt_reset(struct drm_encoder *encoder) >> +static void intel_crt_reset(struct drm_encoder *encoder) >> { >> struct drm_i915_private *dev_priv = to_i915(encoder->dev); >> struct intel_crt *crt = intel_encoder_to_crt(to_intel_encoder(encoder)); >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 0c3ac0eafde0..b2002fee1b58 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -1373,7 +1373,6 @@ void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv); >> bool intel_crt_port_enabled(struct drm_i915_private *dev_priv, >> i915_reg_t adpa_reg, enum pipe *pipe); >> void intel_crt_init(struct drm_i915_private *dev_priv); >> -void intel_crt_reset(struct drm_encoder *encoder); >> >> /* intel_ddi.c */ >> void intel_ddi_fdi_post_disable(struct intel_encoder *intel_encoder, >> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c >> index de3a81034f77..0b3da5818383 100644 >> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c >> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c >> @@ -972,7 +972,7 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv) >> /* Re-enable the ADPA, if we have one */ >> for_each_intel_encoder(&dev_priv->drm, encoder) { >> if (encoder->type == INTEL_OUTPUT_ANALOG) >> - intel_crt_reset(&encoder->base); >> + encoder->base.funcs->reset(&encoder->base); > > I have a feeling I requested the direct call to make it less annoying to > figure out what it's actually calling. But if people prefer the function > pointer version I can live with that too. True that, but I thought the direct call was a layering violation. To push, or not to push, that is the question. BR, Jani. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> } >> >> i915_redisable_vga_power_on(dev_priv); >> -- >> 2.11.0
On Wed, Jun 27, 2018 at 02:49:40PM +0300, Jani Nikula wrote: > On Thu, 21 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Thu, Jun 21, 2018 at 04:03:30PM +0300, Jani Nikula wrote: > >> Commit 9504a8924759 ("drm/i915/vlv: Reset the ADPA in > >> vlv_display_power_well_init()") started calling intel_crt_reset() > >> directly, while we could just as well use the hooks and keep the > >> function static. > >> > >> Cc: Lyude <cpaul@redhat.com> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_crt.c | 2 +- > >> drivers/gpu/drm/i915/intel_drv.h | 1 - > >> drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- > >> 3 files changed, 2 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c > >> index 0c6bf82bb059..c2cb3b7a255b 100644 > >> --- a/drivers/gpu/drm/i915/intel_crt.c > >> +++ b/drivers/gpu/drm/i915/intel_crt.c > >> @@ -881,7 +881,7 @@ static int intel_crt_get_modes(struct drm_connector *connector) > >> return ret; > >> } > >> > >> -void intel_crt_reset(struct drm_encoder *encoder) > >> +static void intel_crt_reset(struct drm_encoder *encoder) > >> { > >> struct drm_i915_private *dev_priv = to_i915(encoder->dev); > >> struct intel_crt *crt = intel_encoder_to_crt(to_intel_encoder(encoder)); > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >> index 0c3ac0eafde0..b2002fee1b58 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -1373,7 +1373,6 @@ void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv); > >> bool intel_crt_port_enabled(struct drm_i915_private *dev_priv, > >> i915_reg_t adpa_reg, enum pipe *pipe); > >> void intel_crt_init(struct drm_i915_private *dev_priv); > >> -void intel_crt_reset(struct drm_encoder *encoder); > >> > >> /* intel_ddi.c */ > >> void intel_ddi_fdi_post_disable(struct intel_encoder *intel_encoder, > >> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > >> index de3a81034f77..0b3da5818383 100644 > >> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > >> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > >> @@ -972,7 +972,7 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv) > >> /* Re-enable the ADPA, if we have one */ > >> for_each_intel_encoder(&dev_priv->drm, encoder) { > >> if (encoder->type == INTEL_OUTPUT_ANALOG) > >> - intel_crt_reset(&encoder->base); > >> + encoder->base.funcs->reset(&encoder->base); > > > > I have a feeling I requested the direct call to make it less annoying to > > figure out what it's actually calling. But if people prefer the function > > pointer version I can live with that too. > > True that, but I thought the direct call was a layering violation. > > To push, or not to push, that is the question. Lemma pile my bikeshed on top: Given that we specifically filter for the CRT encoder I think a direct call is better here, since a bit more clear what's going on. hw occasionally encouragse layering violations ... -Daniel > > BR, > Jani. > > > > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > >> } > >> > >> i915_redisable_vga_power_on(dev_priv); > >> -- > >> 2.11.0 > > -- > Jani Nikula, Intel Open Source Graphics Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 28 Jun 2018, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Jun 27, 2018 at 02:49:40PM +0300, Jani Nikula wrote: >> On Thu, 21 Jun 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: >> > On Thu, Jun 21, 2018 at 04:03:30PM +0300, Jani Nikula wrote: >> >> Commit 9504a8924759 ("drm/i915/vlv: Reset the ADPA in >> >> vlv_display_power_well_init()") started calling intel_crt_reset() >> >> directly, while we could just as well use the hooks and keep the >> >> function static. >> >> >> >> Cc: Lyude <cpaul@redhat.com> >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> --- >> >> drivers/gpu/drm/i915/intel_crt.c | 2 +- >> >> drivers/gpu/drm/i915/intel_drv.h | 1 - >> >> drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- >> >> 3 files changed, 2 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c >> >> index 0c6bf82bb059..c2cb3b7a255b 100644 >> >> --- a/drivers/gpu/drm/i915/intel_crt.c >> >> +++ b/drivers/gpu/drm/i915/intel_crt.c >> >> @@ -881,7 +881,7 @@ static int intel_crt_get_modes(struct drm_connector *connector) >> >> return ret; >> >> } >> >> >> >> -void intel_crt_reset(struct drm_encoder *encoder) >> >> +static void intel_crt_reset(struct drm_encoder *encoder) >> >> { >> >> struct drm_i915_private *dev_priv = to_i915(encoder->dev); >> >> struct intel_crt *crt = intel_encoder_to_crt(to_intel_encoder(encoder)); >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> >> index 0c3ac0eafde0..b2002fee1b58 100644 >> >> --- a/drivers/gpu/drm/i915/intel_drv.h >> >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> >> @@ -1373,7 +1373,6 @@ void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv); >> >> bool intel_crt_port_enabled(struct drm_i915_private *dev_priv, >> >> i915_reg_t adpa_reg, enum pipe *pipe); >> >> void intel_crt_init(struct drm_i915_private *dev_priv); >> >> -void intel_crt_reset(struct drm_encoder *encoder); >> >> >> >> /* intel_ddi.c */ >> >> void intel_ddi_fdi_post_disable(struct intel_encoder *intel_encoder, >> >> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c >> >> index de3a81034f77..0b3da5818383 100644 >> >> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c >> >> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c >> >> @@ -972,7 +972,7 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv) >> >> /* Re-enable the ADPA, if we have one */ >> >> for_each_intel_encoder(&dev_priv->drm, encoder) { >> >> if (encoder->type == INTEL_OUTPUT_ANALOG) >> >> - intel_crt_reset(&encoder->base); >> >> + encoder->base.funcs->reset(&encoder->base); >> > >> > I have a feeling I requested the direct call to make it less annoying to >> > figure out what it's actually calling. But if people prefer the function >> > pointer version I can live with that too. >> >> True that, but I thought the direct call was a layering violation. >> >> To push, or not to push, that is the question. > > Lemma pile my bikeshed on top: Given that we specifically filter for the > CRT encoder I think a direct call is better here, since a bit more clear > what's going on. hw occasionally encouragse layering violations ... Okay, I'll drop the patch for now. BR, Jani. > -Daniel > >> >> BR, >> Jani. >> >> > >> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> >> } >> >> >> >> i915_redisable_vga_power_on(dev_priv); >> >> -- >> >> 2.11.0 >> >> -- >> Jani Nikula, Intel Open Source Graphics Center >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 0c6bf82bb059..c2cb3b7a255b 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -881,7 +881,7 @@ static int intel_crt_get_modes(struct drm_connector *connector) return ret; } -void intel_crt_reset(struct drm_encoder *encoder) +static void intel_crt_reset(struct drm_encoder *encoder) { struct drm_i915_private *dev_priv = to_i915(encoder->dev); struct intel_crt *crt = intel_encoder_to_crt(to_intel_encoder(encoder)); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0c3ac0eafde0..b2002fee1b58 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1373,7 +1373,6 @@ void gen9_disable_guc_interrupts(struct drm_i915_private *dev_priv); bool intel_crt_port_enabled(struct drm_i915_private *dev_priv, i915_reg_t adpa_reg, enum pipe *pipe); void intel_crt_init(struct drm_i915_private *dev_priv); -void intel_crt_reset(struct drm_encoder *encoder); /* intel_ddi.c */ void intel_ddi_fdi_post_disable(struct intel_encoder *intel_encoder, diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index de3a81034f77..0b3da5818383 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -972,7 +972,7 @@ static void vlv_display_power_well_init(struct drm_i915_private *dev_priv) /* Re-enable the ADPA, if we have one */ for_each_intel_encoder(&dev_priv->drm, encoder) { if (encoder->type == INTEL_OUTPUT_ANALOG) - intel_crt_reset(&encoder->base); + encoder->base.funcs->reset(&encoder->base); } i915_redisable_vga_power_on(dev_priv);
Commit 9504a8924759 ("drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()") started calling intel_crt_reset() directly, while we could just as well use the hooks and keep the function static. Cc: Lyude <cpaul@redhat.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/intel_crt.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 1 - drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-)