linux-kernel - Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc instead of synchronously

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20110930220136.GZ2859@phenom.ffwll.local>
Date:	Sat, 1 Oct 2011 00:01:36 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Keith Packard <keithp@...thp.com>
Cc:	Daniel Vetter <daniel@...ll.ch>, Dave Airlie <airlied@...hat.com>,
	intel-gfx@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
	dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 18/21] drm/i915: Disable eDP VDD in a delayed work proc
 instead of synchronously

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@...ll.ch> 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@...ll.ch
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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/