DPCD/AUX locking

DPCD/AUX locking

[Dave Airlie]

So now I've been playing with MST I think get the feeling I might need
some explicit locking on the AUX channel, I think at the moment the
mode_config mutex implicitly defends the aux channel as the only real
paths into it are

a) from userspace connector probing,
b) from HPD irqs,

currently both of these on i915 at least take mode config,

however with MST I can't use mode_config for this, so I'm wondering if
I should be adding some explicit locking in the helpers or make it the
drivers problem to lock around helper access?

Any ideas, it would most likely have to be a mutex since DPCD can in
theory sleep.

Dave.

Re: DPCD/AUX locking

[Ben Skeggs]

On Wed, May 7, 2014 at 9:20 AM, Dave Airlie <airlied@gmail.com> wrote:
> So now I've been playing with MST I think get the feeling I might need
> some explicit locking on the AUX channel, I think at the moment the
> mode_config mutex implicitly defends the aux channel as the only real
> paths into it are
>
> a) from userspace connector probing,
> b) from HPD irqs,
>
> currently both of these on i915 at least take mode config,
>
> however with MST I can't use mode_config for this, so I'm wondering if
> I should be adding some explicit locking in the helpers or make it the
> drivers problem to lock around helper access?
Without yet being clear on what you're locking against exactly, my
vote would be on making this the driver's problem.  We (should be, but
don't yet) need to take locks around AUX access anyway as the pads are
shared between aux/ddc channels in a lot of cases.

Ben.

>
> Any ideas, it would most likely have to be a mutex since DPCD can in
> theory sleep.
>
> Dave.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DPCD/AUX locking

[Dave Airlie]

On 7 May 2014 15:26, Ben Skeggs <skeggsb@gmail.com> wrote:
> On Wed, May 7, 2014 at 9:20 AM, Dave Airlie <airlied@gmail.com> wrote:
>> So now I've been playing with MST I think get the feeling I might need
>> some explicit locking on the AUX channel, I think at the moment the
>> mode_config mutex implicitly defends the aux channel as the only real
>> paths into it are
>>
>> a) from userspace connector probing,
>> b) from HPD irqs,
>>
>> currently both of these on i915 at least take mode config,
>>
>> however with MST I can't use mode_config for this, so I'm wondering if
>> I should be adding some explicit locking in the helpers or make it the
>> drivers problem to lock around helper access?
> Without yet being clear on what you're locking against exactly, my
> vote would be on making this the driver's problem.  We (should be, but
> don't yet) need to take locks around AUX access anyway as the pads are
> shared between aux/ddc channels in a lot of cases.

locking against concurrent access,

currently if I get a HPD irq that requires reading the DPCD status,
and I get a connector detect from userspace that reads i2c over aux
they would collide,

at the moment mode config lock seems to stop that in i915 but taking mode config
for the DP HPD irq is very wrong for MST.

Dave.

Re: DPCD/AUX locking

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2516 bytes --]

On Wed, May 07, 2014 at 05:12:54PM +1000, Dave Airlie wrote:
> On 7 May 2014 15:26, Ben Skeggs <skeggsb@gmail.com> wrote:
> > On Wed, May 7, 2014 at 9:20 AM, Dave Airlie <airlied@gmail.com> wrote:
> >> So now I've been playing with MST I think get the feeling I might need
> >> some explicit locking on the AUX channel, I think at the moment the
> >> mode_config mutex implicitly defends the aux channel as the only real
> >> paths into it are
> >>
> >> a) from userspace connector probing,
> >> b) from HPD irqs,
> >>
> >> currently both of these on i915 at least take mode config,
> >>
> >> however with MST I can't use mode_config for this, so I'm wondering if
> >> I should be adding some explicit locking in the helpers or make it the
> >> drivers problem to lock around helper access?
> > Without yet being clear on what you're locking against exactly, my
> > vote would be on making this the driver's problem.  We (should be, but
> > don't yet) need to take locks around AUX access anyway as the pads are
> > shared between aux/ddc channels in a lot of cases.
> 
> locking against concurrent access,
> 
> currently if I get a HPD irq that requires reading the DPCD status,
> and I get a connector detect from userspace that reads i2c over aux
> they would collide,
> 
> at the moment mode config lock seems to stop that in i915 but taking mode config
> for the DP HPD irq is very wrong for MST.

I think that if concurrent access is all that you're worried about, then
having a lock in each driver's drm_dp_aux implementation should work. If
we end up doing that for every driver then it probably makes sense to
move it into drm_dp_aux directly and handle locking within the helpers.
That has the disadvantage that somebody could for some reason decide to
call into the driver's ->transfer function directly, in which case that
code will have to make sure to do proper locking itself.

Depending on what you want to do I guess it would make sense to
introduce two levels of locking. For example the DPCD and I2C-over-AUX
helpers have retry logic built in, so it might make sense to lock around
the loops as well in order for concurrent accesses not to be interleaved
with the subsequent retries. If the locking happens within the driver's
->transfer implementation it would still be possible for other DPCD
accesses to happen in between retries. I'm not sure if that's actually a
problem, but I can imagine that it could mess up the whole retry logic.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: DPCD/AUX locking

[Daniel Vetter]

On Wed, May 07, 2014 at 09:20:41AM +1000, Dave Airlie wrote:
> So now I've been playing with MST I think get the feeling I might need
> some explicit locking on the AUX channel, I think at the moment the
> mode_config mutex implicitly defends the aux channel as the only real
> paths into it are
> 
> a) from userspace connector probing,
> b) from HPD irqs,

c) Directly through the i2c-over-dp_aux /dev nodes with e.g. ddc tools.

> currently both of these on i915 at least take mode config,
> 
> however with MST I can't use mode_config for this, so I'm wondering if
> I should be adding some explicit locking in the helpers or make it the
> drivers problem to lock around helper access?
> 
> Any ideas, it would most likely have to be a mutex since DPCD can in
> theory sleep.

Definitely needs to be a mutex since at leas i915 does sleep doing dp_aux
transactions.

With gmbus we've had a similar issue since that one's shared and there's
the direct i2c access path from userspace (or other kernel drivers, e.g.
google's pixie has a touchpad connected to the i915 gmbus controller
iirc).

Like Ben says that kind of locking must be done in the driver since. But
on hw which doesn't have shared dp aux pads I think the locking for
concurrent access from a)-c) should be done in the helper. I think we
actually need 2 locks:

1. Low-level hardware lock which is just held around calls to the drivers
aux.transfer callback. This avoids races between a) and b). I guess we
could debate whether this should be in drivers or not, but doing it in the
helper should restrict us and prevents stupid bugs. Drivers should have a
locking assert in their low-level dp aux functions to make sure other
(driver-internal callers) also obey this rule.

2. High-level dp aux interface lock. We need this to prevent races between
a) and c), which is currently completely broken. And maybe we'll add a raw
dp aux interface eventually too. All dp aux helpers which are exposed to
drivers and the i2c transfer functions should grab this except the single
entry point used to handle dp hpd events. Otherwise b) will deadlock
against a)&c).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch