Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work procinstead of synchronously

[Daniel Vetter]

On Fri, Sep 30, 2011 at 01:56:09PM -0700, Keith Packard wrote:
> On Fri, 30 Sep 2011 20:47:00 +0200, Daniel Vetter <daniel@xxxxxxxx> wrote:
> 
> > A cancel_work might be good here, not point in waking up the cpu for no
> > reason. And can you add "panel off timer: " to the deboug output?
> 
> No, that's not correct, this is run before turning the panel back on and
> must check that enough time has elapsed since it was turned off, which
> may have happened in the work proc.

You're right, I've got a bit confused between wait_panel_off and
vdd_panel_off.

> However, you are right in saying that I could call the cancel work
> function when the panel is forced off synchronously. I'll add that.

Sounds good.

> > Like Chris already mentioned, s/struct_mutex/mode_config.mutex/ Maybe also
> > move the intel_dp->want_panel_vdd check in here - we need it only here to
> > avoid a race with vdd_on. I've almost overlooked it and already started to
> > write the complaint about the race ;-)
> 
> With the right mutex held, there won't be a race, we just need to check the
> value somewhere. Do you still see a race condition that needs to be avoided?

Nope, with the right mutex protection the race is gone. And judging by
your other response my 2nd trial at formulating the issue I see seems to
have come out much better ;-)

> > > @@ -1111,10 +1175,10 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
> > >  			intel_dp_start_link_train(intel_dp);
> > >  			if (is_edp(intel_dp))
> > >  				ironlake_edp_panel_on(intel_dp);
> > > -			ironlake_edp_panel_vdd_off(intel_dp);
> > > +			ironlake_edp_panel_vdd_off(intel_dp, true);
> > >  			intel_dp_complete_link_train(intel_dp);
> > >  		} else
> > > -			ironlake_edp_panel_vdd_off(intel_dp);
> > > +			ironlake_edp_panel_vdd_off(intel_dp, false);
> > 
> > Any reason why one vdd_off is sync while the other isn't?
> 
> After the panel is turned on, I want to synchronously turn off the VDD
> AUX bits, as that's what the driver did before. That's 'free' because
> the panel is on. In the other (failure) path, I don't want to impose a
> delay if the application wants to try the mode set again.

Ok, makes sense. Maybe add a comment explaining why the sync case is free
for dummies like me?
-Daniel
-- 
Daniel Vetter
Mail: daniel@xxxxxxxx
Mobile: +41 (0)79 365 57 48
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/