| Message ID | 1455296121-4742-13-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 12, 2016 at 06:55:21PM +0200, Imre Deak wrote: > The assumption when adding the intel_display_power_is_enabled() checks > was that if it returns success the power can't be turned off afterwards > during the HW access, which is guaranteed by modeset locks. This isn't > always true, so make sure we hold a dedicated reference for the time of > the access. > > Signed-off-by: Imre Deak <imre.deak@intel.com> For patches 9-12: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> I'm assuming you're working on more patches to get rid of all the powe_is_enabled() checks, so that we can remove this fundamentally unsafe function evenutally? -Daniel > --- > drivers/gpu/drm/i915/intel_lvds.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c > index 811ddf7..30a8403 100644 > --- a/drivers/gpu/drm/i915/intel_lvds.c > +++ b/drivers/gpu/drm/i915/intel_lvds.c > @@ -76,22 +76,30 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder, > struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base); > enum intel_display_power_domain power_domain; > u32 tmp; > + bool ret; > > power_domain = intel_display_port_power_domain(encoder); > - if (!intel_display_power_is_enabled(dev_priv, power_domain)) > + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) > return false; > > + ret = false; > + > tmp = I915_READ(lvds_encoder->reg); > > if (!(tmp & LVDS_PORT_EN)) > - return false; > + goto out; > > if (HAS_PCH_CPT(dev)) > *pipe = PORT_TO_PIPE_CPT(tmp); > else > *pipe = PORT_TO_PIPE(tmp); > > - return true; > + ret = true; > + > +out: > + intel_display_power_put(dev_priv, power_domain); > + > + return ret; > } > > static void intel_lvds_get_config(struct intel_encoder *encoder, > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On ti, 2016-02-16 at 17:05 +0100, Daniel Vetter wrote: > On Fri, Feb 12, 2016 at 06:55:21PM +0200, Imre Deak wrote: > > The assumption when adding the intel_display_power_is_enabled() > > checks > > was that if it returns success the power can't be turned off > > afterwards > > during the HW access, which is guaranteed by modeset locks. This > > isn't > > always true, so make sure we hold a dedicated reference for the > > time of > > the access. > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > For patches 9-12: > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > I'm assuming you're working on more patches to get rid of all the > powe_is_enabled() checks, so that we can remove this fundamentally > unsafe > function evenutally? Mika noticed that I missed to convert the one in skl_ddb_get_hw_state(), I'll send a patch for it. For the other two remaining callers I gave an explanation in the cover letter as to why I haven't converted them. --Imre > -Daniel > > --- > > drivers/gpu/drm/i915/intel_lvds.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c > > b/drivers/gpu/drm/i915/intel_lvds.c > > index 811ddf7..30a8403 100644 > > --- a/drivers/gpu/drm/i915/intel_lvds.c > > +++ b/drivers/gpu/drm/i915/intel_lvds.c > > @@ -76,22 +76,30 @@ static bool intel_lvds_get_hw_state(struct > > intel_encoder *encoder, > > struct intel_lvds_encoder *lvds_encoder = > > to_lvds_encoder(&encoder->base); > > enum intel_display_power_domain power_domain; > > u32 tmp; > > + bool ret; > > > > power_domain = intel_display_port_power_domain(encoder); > > - if (!intel_display_power_is_enabled(dev_priv, > > power_domain)) > > + if (!intel_display_power_get_if_enabled(dev_priv, > > power_domain)) > > return false; > > > > + ret = false; > > + > > tmp = I915_READ(lvds_encoder->reg); > > > > if (!(tmp & LVDS_PORT_EN)) > > - return false; > > + goto out; > > > > if (HAS_PCH_CPT(dev)) > > *pipe = PORT_TO_PIPE_CPT(tmp); > > else > > *pipe = PORT_TO_PIPE(tmp); > > > > - return true; > > + ret = true; > > + > > +out: > > + intel_display_power_put(dev_priv, power_domain); > > + > > + return ret; > > } > > > > static void intel_lvds_get_config(struct intel_encoder *encoder, > > -- > > 2.5.0 > > > > _______________________________________________ > > 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_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 811ddf7..30a8403 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -76,22 +76,30 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder, struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base); enum intel_display_power_domain power_domain; u32 tmp; + bool ret; power_domain = intel_display_port_power_domain(encoder); - if (!intel_display_power_is_enabled(dev_priv, power_domain)) + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) return false; + ret = false; + tmp = I915_READ(lvds_encoder->reg); if (!(tmp & LVDS_PORT_EN)) - return false; + goto out; if (HAS_PCH_CPT(dev)) *pipe = PORT_TO_PIPE_CPT(tmp); else *pipe = PORT_TO_PIPE(tmp); - return true; + ret = true; + +out: + intel_display_power_put(dev_priv, power_domain); + + return ret; } static void intel_lvds_get_config(struct intel_encoder *encoder,
The assumption when adding the intel_display_power_is_enabled() checks was that if it returns success the power can't be turned off afterwards during the HW access, which is guaranteed by modeset locks. This isn't always true, so make sure we hold a dedicated reference for the time of the access. Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_lvds.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)