Re: [PATCH v3 05/16] drm/fb-helper: do a generic fb_setcmap helper in terms of crtc .gamma_set

[Peter Rosin]

On 2017-07-06 07:55, Daniel Vetter wrote:
> On Wed, Jul 5, 2017 at 7:50 PM, Peter Rosin <peda@xxxxxxxxxx> wrote:
>>>> +retry:
>>>> +    ret = drm_modeset_lock_all_ctx(dev, &ctx);
>>>
>>> With atomic you don't need to grab locks, this is done behind the scenes
>>> (as long as you handle the retry/backoff correctly). See the kerneldoc for
>>> the various drm_atomic_get_*_state functions.
>>
>> It doesn't work if I remove it. What is the disconnect?
> 
> Good question,

Duh, for symmetry I also removed the dropping of the locks. So the next
call of course had no chance of getting access. How silly of me...

>                didn't spot this at first, but your backoff/retry logic
> is proken. When typing drm_modeset_lock locking code please make sure
> you've enabled both CONFIG_PROVE_LOCKING and
> CONFIG_DEBUG_WW_MUTEX_SLOWPATH. Without these two it's really easy to
> get this wrong. Please also read
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#kms-locking
> carefully plus all the kernel-doc of the various hooks. This stuff is
> a really tricky locking scheme, it takes a while to understand it and
> implement it correctly. Which is why all the locking magic is in
> shared code and for normal drivers no need think about it. For the
> fundamental algorithm, you can also check out the docs for w/w mutexes
> at https://www.kernel.org/doc/Documentation/locking/ww-mutex-design.txt
> 
> Might also help to read a bunch of the other locking paths again, with
> my patches there's a few just in drm_fbdev_helper.c. I'll leave you
> with these snippets here since I think this is fun to learn, but when
> you're stuck I'm happy to help learn.

I'll take a long look at this before I send a cleaned up v4. Thanks for
the pointers...

Cheers,
peda