Re: [PATCH] drm/i915: implement HSW display sequences for package C8+

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jun 05, 2013 at 08:04:48PM +0200, Daniel Vetter wrote:
> On Wed, Jun 5, 2013 at 7:20 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> >>   So all together I vote for the generic interface to be called something
> >>   like
> >>
> >>   intel_aux_display_runtime_forbid/allow
> >
> > I changed to intel_aux_display_shutdown_forbid/allow, because we
> > forbid the shutdown, not the "runtime".
> 
> Just one quick comment: The idea was to be 100% consisten with the
> runtime pm api (I've dropped the pm_ part unfortunately), which is:
> 
> extern void pm_runtime_allow(struct device *dev);
> extern void pm_runtime_forbid(struct device *dev);

I've not seen those guys before, but they don't seem like something
we ought to be using. In fact it seems they're there solely for the
purpose of sysfs. They frob the runtime_auto boolean which has no ref
count protecting it, so someone writing to the sysfs "control" file
could undo what we just did from the driver, and vice versa. There's
still the usage_count though so the device should remain alive, but
then that's what _get()/_put() also modify.

I do see quite a few pm_runtime_allow/forbid calls from drivers though,
which is confusing to say the least. Either I'm confused about the
purpose of these functions, or there has been a serious problem with 
driver review.

-- 
Ville Syrjälä
Intel OTC