[RFC] drm: implement generic firmware eviction

[Rob Herring]

On Tue, Aug 30, 2016 at 2:30 PM, David Herrmann <dh.herrmann at gmail.com> wrote:
> Hi Rob
>
> On Fri, Aug 26, 2016 at 2:36 PM, Rob Herring <robh+dt at kernel.org> wrote:
>> On Thu, Aug 25, 2016 at 7:00 PM, David Herrmann <dh.herrmann at gmail.com> wrote:
>>> Provide a generic DRM helper that evicts all conflicting firmware
>>> framebuffers, devices, and drivers. The new helper is called
>>> drm_evict_firmware(), and takes a flagset controlling which firmware to
>>> kick out.
>>>
>>> This is meant to be used by drivers in their ->probe() callbacks of their
>>> parent bus, before calling into drm_dev_register().
>>>
>>> Signed-off-by: David Herrmann <dh.herrmann at gmail.com>
>>> ---
>>> Hey
>>>
>>> This is just compile-tested for now. I just want to get some comments on the
>>> design. I decided to drop the sysfb infrastructure and rather just use this
>>> generic helper. It keeps things simple and should work just fine for all
>>> reasonable use-cases.
>>>
>>> This will work with SimpleDRM out-of-the-box on x86.
>>>
>>> Architectures with dynamic simple-framebuffer devices are not supported yet. I
>>> actually have no idea what the strategy there is? Can the DeviceTree people come
>>> up with something? Am I supposed to call of_platform_depopulate()?
>>
>> If of_platform_populate was used to create the device, then yes call
>> of_platform_depopulate. In this case, I think it wasn't. If
>> of_platform_device_create was used, then platform_device_del.
>>
>>> Or
>>> of_detach_node()? Or what?
>>
>> No. Only the struct device and its resources should need to be
>> destroyed. The node should remain.
>>
>>> Looking at drivers/of/{platform,dynamic}.c, I cannot see how this is supposed to
>>> work at all. Who protects platform_device_del() from being called in parallel?
>>
>> Not sure. In parallel to what? On most systems, nodes never go away
>> and on those that do it is only a few things that get hotplugged.
>> That's changing with DT overlays now, so there certainly could be some
>> issues.
>
> Two tasks calling platform_device_del() in parallel on the same device
> will not work. Meaning, calling of_platform_device_destroy() in
> parallel does not work either. Same for of_platform_depopulate(). Same
> for of_node_detach().

Changes to DT nodes and struct device's are completely separate from a
DT core perspective ATM. A caller is responsible adding devices when
nodes are added, removing the devices, then removing the nodes. The
only overlays currently supported require a driver to load them and
handle any transitions.

DT is still far from dynamic in the sense that any random node can come and go.

> Sure, all those functions are not meant to be called in parallel by
> multiple tasks. They are rather meant to have a single holder which
> preferably is the one instantiating and destroying the
> node/device/foobar. But the framebuffer eviction code somehow needs to
> trigger the removal, and thus needs some hook, that can be called in
> parallel by any driver that is loaded.
>
> I can make sure the simple-framebuffer eviction code is locked
> properly. That'll work, for now. But if someone ends up providing
> simple-framebuffer devices via overlays or any other dynamic
> mechanism, they will race the removal.

No doubt someone will come up with some usecase wanting to do that,
but IMO that is not a supported usecase and should not be.
simple-framebuffer is for providing a firmware setup framebuffer.

> And it will be completely
> non-obvious to them. I really don't want to be responsible to have
> submitted that code. If anyone cares for proper device hand-over on
> ARM, please come up with an idea to fix it. Otherwise, I will just
> limit the simpledrm code to x86.
>
> IOW the device handover code somehow needs to know who was responsible
> for the instantiation of the simple-framebuffer device, so it can tell
> them to remove it again. On x86 there is only one place where those
> can be instantiated. But on OF-based systems, it can be dynamically
> instantiated in many places right now.

What do you mean by all over the place? It is only in simplefb_init
ATM. I haven't looked at what simpledrm is doing, but we can move the
device creation to of_platform_default_populate_init if we need a
single spot.

Rob