[RFC] drm: implement generic firmware eviction

[David Herrmann]

Hey Rob!

On Wed, Aug 31, 2016 at 1:01 AM, Rob Herring  wrote:
> On Tue, Aug 30, 2016 at 4:12 PM, David Herrmann  
> wrote:
>> Currently I see at least 3 paths that might add such nodes:
>>
>>  - of_platform_populate()
>
> If we assume the node is only under chosen, then that would require a
> call to of_platform_populate with /chosen as the root which shouldn't
> happen. More generally the assumption is of_platform_populate is only
> called once from for the root node and only on sub-trees by the driver
> of sub-tree's root device.

I see.

>>  - of_node_attach() (via the notifier)
>
> A node getting attached would be harmless. Nothing really would happen
> until a handler calls of_platform_populate. So this is the same as the
> 1st case.

drivers/of/platform.c has a notifier callback and registers the
platform devices when added. It has a check that the parent is a
populated bus, which I guess is what you refer to?

>>  - simplefb_init()
>
> Actually, I'd prefer to move it out of there and into the core. I
> don't think drivers should look at /chosen and only the core and arch
> code should. Of course, the only enforcing of that is policy. For
> overlays though, there's been some discussion on limiting where
> overlays can be applied.
>
>> Should I just ignore anything but simplefb_init()? I understand that
>> it's the only one used by normal code-paths, but isn't it kinda ugly
>> to silently introduce race conditions if a node just happens to be
>> introduced via one of the other methods? Or are errors in the DT not
>> meant to be caught?
>
> In general there's no limit to how wrong a DT could be. I could write
> a DT that causes every DT enabled driver in the kernel to probe (that
> would be an interesting test case). The kernel is limited in knowing
> what is correct, and the whole point of DT is to move that information
> out of the kernel.  This is case is just one compatible string out of
> thousands and location in the tree is just one thing to check.
>
> This is a major reason why there is not yet a userspace interface for
> applying DT overlays as who knows what random crap could be in the DT.
> We're nervous about what could happen from frying h/w to creating
> security holes. Evidently the ACPI folks were not so nervous and added
> an interface.

I see. Thanks a lot for the clarifications! I will put the
of_platform_device_destroy() right next to the x86 handler, making
sure it is properly locked. I'm not so sure about the of_chosen
instantiation, though. x86 has the instantiation in
arch/x86/kernel/sysfb_simplefb.c, so maybe an equivalent for arm would
be fine?

Thanks
David

[RFC] drm: implement generic firmware eviction

[David Herrmann]

Hi

On Tue, Aug 30, 2016 at 10:58 PM, Rob Herring  wrote:
> On Tue, Aug 30, 2016 at 2:30 PM, David Herrmann  
> wrote:
>> 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.

Sure. Any sensible simple-framebuffer use would follow what we have
now. But it feels wrong to me that if some node is added that just
happens to have "simple-framebuffer" as name, suddenly things will go
wrong. I mean, yeah, DT is not a userspace API, but I still would like
the code to catch misuses rather than fail. It is an API after all. Or
is that being overly pedantic?

>> 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.

Currently I see at least 3 paths that might add such nodes:

 - of_platform_populate()
 - of_node_attach() (via the notifier)
 - simplefb_init()

Should I just ignore anything but simplefb_init()? I understand that
it's the only one used by normal code-paths, but isn't it kinda ugly
to silently introduce race conditions if a node just happens to be
introduced via one of the other methods? Or are errors in the DT not
meant to be caught?

Thanks
David

[RFC] drm: implement generic firmware eviction

[Maxime Ripard]

Hi David,

On Tue, Aug 30, 2016 at 09:30:44PM +0200, David Herrmann wrote:
> Hi Rob
> 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.

I don't think that's true. There's the assumption that the bootloader
will have set up the framebuffer and Linux just takes over. Even on
ARM, you'll need to at least reserve the memory, grab the clocks and
so on, and it needs to happen way before you can load overlays (for
the buffer even way before the simple-framebuffer driver is probed).

I don't see how we could use overlays with simple-framebuffer, unless
we apply those overlays before linux starts, but then Linux basically
doesn't care if it was an overlay or not, it's treated as a single DT.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[RFC] drm: implement generic firmware eviction

[David Herrmann]

Hi Rob

On Fri, Aug 26, 2016 at 2:36 PM, Rob Herring  wrote:
> On Thu, Aug 25, 2016 at 7:00 PM, David Herrmann  
> 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 
>> ---
>> 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().

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. 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.

Thanks
David

[RFC] drm: implement generic firmware eviction

[Rob Herring]

On Tue, Aug 30, 2016 at 4:12 PM, David Herrmann  
wrote:
> Hi
>
> On Tue, Aug 30, 2016 at 10:58 PM, Rob Herring  wrote:
>> On Tue, Aug 30, 2016 at 2:30 PM, David Herrmann  
>> wrote:
>>> 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.
>
> Sure. Any sensible simple-framebuffer use would follow what we have
> now. But it feels wrong to me that if some node is added that just
> happens to have "simple-framebuffer" as name, suddenly things will go
> wrong. I mean, yeah, DT is not a userspace API, but I still would like
> the code to catch misuses rather than fail. It is an API after all. Or
> is that being overly pedantic?

The kernel could blow up in all sorts of interesting ways if random
nodes were created. IMO, it is not the kernel's job to be a DT
validator. If it is, it is doing a horrible job. And don't get me
wrong, we do need something to do that. That is all orthogonal to
dynamic devices and the issues there.

>>> 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.
>
> Currently I see at least 3 paths that might add such nodes:
>
>  - of_platform_populate()

If we assume the node is only under chosen, then that would require a
call to of_platform_populate with /chosen as the root which shouldn't
happen. More generally the assumption is of_platform_populate is only
called once from for the root node and only on sub-trees by the driver
of sub-tree's root device.

>  - of_node_attach() (via the notifier)

A node getting attached would be harmless. Nothing really would happen
until a handler calls of_platform_populate. So this is the same as the
1st case.

>  - simplefb_init()

Actually, I'd prefer to move it out of there and into the core. I
don't think drivers should look at /chosen and only the core and arch
code should. Of course, the only enforcing of that is policy. For
overlays though, there's been some discussion on limiting where
overlays can be applied.

> Should I just ignore anything but simplefb_init()? I understand that
> it's the only one used by normal code-paths, but isn't it kinda ugly
> to silently introduce race conditions if a node just happens to be
> introduced via one of the other methods? Or are errors in the DT not
> meant to be caught?

In general there's no limit to how wrong a DT could be. I could write
a DT that causes every DT enabled driver in the kernel to probe (that
would be an interesting test case). The kernel is limited in knowing
what is correct, and the whole point of DT is to move that information
out of the kernel.  This is case is just one compatible string out of
thousands and location in the tree is just one thing to check.

This is a major reason why there is not yet a userspace interface for
applying DT overlays as who knows what random crap could be in the DT.
We're nervous about what could happen from frying h/w to creating
security holes. Evidently the ACPI folks were not so nervous and added
an interface.

Rob

[RFC] drm: implement generic firmware eviction

[Rob Herring]

On Tue, Aug 30, 2016 at 2:30 PM, David Herrmann  
wrote:
> Hi Rob
>
> On Fri, Aug 26, 2016 at 2:36 PM, Rob Herring  wrote:
>> On Thu, Aug 25, 2016 at 7:00 PM, David Herrmann  
>> 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 
>>> ---
>>> 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

[RFC] drm: implement generic firmware eviction

[Maxime Ripard]

On Fri, Aug 26, 2016 at 02:58:51PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 26-08-16 14:52, Maxime Ripard wrote:
> >On Fri, Aug 26, 2016 at 11:02:17AM +0200, Hans de Goede wrote:
> >>Hi,
> >>
> >>On 26-08-16 10:58, Maxime Ripard wrote:
> >>>Hi,
> >>>
> >>>On Fri, Aug 26, 2016 at 10:43:55AM +0200, Hans de Goede wrote:
> >>I'm not sure we would want to remove the device at all, we
> >>certainly should not be removing the dt_node from the devicetree
> >>IMHO. Having that around to see how the bootloader set things up
> >>is really useful for debugging and normally we should never modify
> >>the devicetree as set up by the bootloader.
> >>
> >>Why not just unbind the driver from the platform device? That
> >>should be enough.
> >
> >That will leave IORESOURCE_MEM around, causing conflicts if
> >re-used/claimed by other devices/drivers. Furthermore, it is really
> >fragile leaving the device around, without any control over
> >possible future driver probing.
> 
> Ah, good point. On ARM this currently typically is reserved by the 
> bootloader
> so never touched by the kernel at all, not even when the simplefb is no 
> longer
> used, actually returning this memory to the kernel after unbinding the 
> simplefb /
> destroying the simplefb platform-dev would be really good to do. We should
> probably figure out how that should be done before getting rid of
> remove_conflicting_framebuffers... (sorry).
> >>>
> >>>That would be rather easy to do. The firmware could generate a
> >>>reserved-memory node instead of passing a smaller memory size to the
> >>>kernel. That way, the kernel will know that it's actual ram that it
> >>>can reclaim.
> >>
> >>So when would the kernel reclaim the RAM then?
> >
> >When we kickout the framebuffer driver?
> 
> Yes that is when we _want_ it to reclaim the RAM, my question was when
> it will _actually_ happen ? I'm not familiar with the reserved-memory
> implementation. Does your answer mean that some driver must make an
> explicit call to get the memory reclaimed ?

The reserved-memory implementation is relying on memblock. I don't
think there is a function yet to remove a reserved memory region, but
its implementation would use memblock_free I guess.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[RFC] drm: implement generic firmware eviction

[Maxime Ripard]

Hi,

On Fri, Aug 26, 2016 at 02:00:56AM +0200, David Herrmann wrote:
> Also: Does any platform make use the of 'display:' entry in 
> 'simple-framebuffer'
> DT nodes? If yes, how do I get access to it? And why don't vc4/sun4i make use 
> of
> it, rather than falling back to remove_conflicting_framebuffers()?

For the sun4i part, I'd say because I wasn't aware of it :)

A better argument would be that most of our device tree don't have it
at the moment, and we might boot on older device trees. So we need a
fallback solution anyway.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[RFC] drm: implement generic firmware eviction

[Hans de Goede]

Hi,

On 26-08-16 14:52, Maxime Ripard wrote:
> On Fri, Aug 26, 2016 at 11:02:17AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 26-08-16 10:58, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Fri, Aug 26, 2016 at 10:43:55AM +0200, Hans de Goede wrote:
>> I'm not sure we would want to remove the device at all, we
>> certainly should not be removing the dt_node from the devicetree
>> IMHO. Having that around to see how the bootloader set things up
>> is really useful for debugging and normally we should never modify
>> the devicetree as set up by the bootloader.
>>
>> Why not just unbind the driver from the platform device? That
>> should be enough.
>
> That will leave IORESOURCE_MEM around, causing conflicts if
> re-used/claimed by other devices/drivers. Furthermore, it is really
> fragile leaving the device around, without any control over
> possible future driver probing.

 Ah, good point. On ARM this currently typically is reserved by the 
 bootloader
 so never touched by the kernel at all, not even when the simplefb is no 
 longer
 used, actually returning this memory to the kernel after unbinding the 
 simplefb /
 destroying the simplefb platform-dev would be really good to do. We should
 probably figure out how that should be done before getting rid of
 remove_conflicting_framebuffers... (sorry).
>>>
>>> That would be rather easy to do. The firmware could generate a
>>> reserved-memory node instead of passing a smaller memory size to the
>>> kernel. That way, the kernel will know that it's actual ram that it
>>> can reclaim.
>>
>> So when would the kernel reclaim the RAM then?
>
> When we kickout the framebuffer driver?

Yes that is when we _want_ it to reclaim the RAM, my question was when
it will _actually_ happen ? I'm not familiar with the reserved-memory
implementation. Does your answer mean that some driver must make an
explicit call to get the memory reclaimed ?

Regards,

Hans

[RFC] drm: implement generic firmware eviction

[Maxime Ripard]

On Fri, Aug 26, 2016 at 11:02:17AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 26-08-16 10:58, Maxime Ripard wrote:
> >Hi,
> >
> >On Fri, Aug 26, 2016 at 10:43:55AM +0200, Hans de Goede wrote:
> I'm not sure we would want to remove the device at all, we
> certainly should not be removing the dt_node from the devicetree
> IMHO. Having that around to see how the bootloader set things up
> is really useful for debugging and normally we should never modify
> the devicetree as set up by the bootloader.
> 
> Why not just unbind the driver from the platform device? That
> should be enough.
> >>>
> >>>That will leave IORESOURCE_MEM around, causing conflicts if
> >>>re-used/claimed by other devices/drivers. Furthermore, it is really
> >>>fragile leaving the device around, without any control over
> >>>possible future driver probing.
> >>
> >>Ah, good point. On ARM this currently typically is reserved by the 
> >>bootloader
> >>so never touched by the kernel at all, not even when the simplefb is no 
> >>longer
> >>used, actually returning this memory to the kernel after unbinding the 
> >>simplefb /
> >>destroying the simplefb platform-dev would be really good to do. We should
> >>probably figure out how that should be done before getting rid of
> >>remove_conflicting_framebuffers... (sorry).
> >
> >That would be rather easy to do. The firmware could generate a
> >reserved-memory node instead of passing a smaller memory size to the
> >kernel. That way, the kernel will know that it's actual ram that it
> >can reclaim.
> 
> So when would the kernel reclaim the RAM then?

When we kickout the framebuffer driver?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[RFC] drm: implement generic firmware eviction

[Jani Nikula]

On Fri, 26 Aug 2016, David Herrmann  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.

Reading the subject, I thought this was somehow about the firmware
themselves, not about their framebuffers... My bikeshed is to make that
clear in the function name too.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

[RFC] drm: implement generic firmware eviction

[Hans de Goede]

Hi,

On 26-08-16 10:58, Maxime Ripard wrote:
> Hi,
>
> On Fri, Aug 26, 2016 at 10:43:55AM +0200, Hans de Goede wrote:
 I'm not sure we would want to remove the device at all, we
 certainly should not be removing the dt_node from the devicetree
 IMHO. Having that around to see how the bootloader set things up
 is really useful for debugging and normally we should never modify
 the devicetree as set up by the bootloader.

 Why not just unbind the driver from the platform device? That
 should be enough.
>>>
>>> That will leave IORESOURCE_MEM around, causing conflicts if
>>> re-used/claimed by other devices/drivers. Furthermore, it is really
>>> fragile leaving the device around, without any control over
>>> possible future driver probing.
>>
>> Ah, good point. On ARM this currently typically is reserved by the bootloader
>> so never touched by the kernel at all, not even when the simplefb is no 
>> longer
>> used, actually returning this memory to the kernel after unbinding the 
>> simplefb /
>> destroying the simplefb platform-dev would be really good to do. We should
>> probably figure out how that should be done before getting rid of
>> remove_conflicting_framebuffers... (sorry).
>
> That would be rather easy to do. The firmware could generate a
> reserved-memory node instead of passing a smaller memory size to the
> kernel. That way, the kernel will know that it's actual ram that it
> can reclaim.

So when would the kernel reclaim the RAM then? We do not want it
to reclaim before loading the simplefb / simpledrm driver.

And the real drm driver will likely be a module, so if the kernel
reclaims just before executing /sbin/init that would be too
early, unless we can somehow manually make it do another reclaim
pass when the simplefb gets disabled ...

Regards,

Hans

[RFC] drm: implement generic firmware eviction

[Maxime Ripard]

Hi,

On Fri, Aug 26, 2016 at 10:43:55AM +0200, Hans de Goede wrote:
> >>I'm not sure we would want to remove the device at all, we
> >>certainly should not be removing the dt_node from the devicetree
> >>IMHO. Having that around to see how the bootloader set things up
> >>is really useful for debugging and normally we should never modify
> >>the devicetree as set up by the bootloader.
> >>
> >>Why not just unbind the driver from the platform device? That
> >>should be enough.
> >
> >That will leave IORESOURCE_MEM around, causing conflicts if
> >re-used/claimed by other devices/drivers. Furthermore, it is really
> >fragile leaving the device around, without any control over
> >possible future driver probing.
> 
> Ah, good point. On ARM this currently typically is reserved by the bootloader
> so never touched by the kernel at all, not even when the simplefb is no longer
> used, actually returning this memory to the kernel after unbinding the 
> simplefb /
> destroying the simplefb platform-dev would be really good to do. We should
> probably figure out how that should be done before getting rid of
> remove_conflicting_framebuffers... (sorry).

That would be rather easy to do. The firmware could generate a
reserved-memory node instead of passing a smaller memory size to the
kernel. That way, the kernel will know that it's actual ram that it
can reclaim.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: 

[RFC] drm: implement generic firmware eviction

[Hans de Goede]

Hi,

On 26-08-16 10:01, David Herrmann wrote:
> Hi
>
> On Fri, Aug 26, 2016 at 9:57 AM, Hans de Goede  wrote:
>> Hi,
>>
>> On 26-08-16 02:00, David Herrmann 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 
>>> ---
>>> 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()? Or
>>> of_detach_node()? Or what?
>>
>>
>> I'm not sure we would want to remove the device at all, we certainly should
>> not
>> be removing the dt_node from the devicetree IMHO. Having that around to see
>> how
>> the bootloader set things up is really useful for debugging and normally we
>> should never modify the devicetree as set up by the bootloader.
>>
>> Why not just unbind the driver from the platform device? That should be
>> enough.
>
> That will leave IORESOURCE_MEM around, causing conflicts if
> re-used/claimed by other devices/drivers. Furthermore, it is really
> fragile leaving the device around, without any control over possible
> future driver probing.

Ah, good point. On ARM this currently typically is reserved by the bootloader
so never touched by the kernel at all, not even when the simplefb is no longer
used, actually returning this memory to the kernel after unbinding the simplefb 
/
destroying the simplefb platform-dev would be really good to do. We should
probably figure out how that should be done before getting rid of
remove_conflicting_framebuffers... (sorry).

Regards,

Hans

[RFC] drm: implement generic firmware eviction

[David Herrmann]

Hi

On Fri, Aug 26, 2016 at 9:57 AM, Hans de Goede  wrote:
> Hi,
>
> On 26-08-16 02:00, David Herrmann 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 
>> ---
>> 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()? Or
>> of_detach_node()? Or what?
>
>
> I'm not sure we would want to remove the device at all, we certainly should
> not
> be removing the dt_node from the devicetree IMHO. Having that around to see
> how
> the bootloader set things up is really useful for debugging and normally we
> should never modify the devicetree as set up by the bootloader.
>
> Why not just unbind the driver from the platform device? That should be
> enough.

That will leave IORESOURCE_MEM around, causing conflicts if
re-used/claimed by other devices/drivers. Furthermore, it is really
fragile leaving the device around, without any control over possible
future driver probing.

Thanks
David

[RFC] drm: implement generic firmware eviction

[Hans de Goede]

Hi,

On 26-08-16 02:00, David Herrmann 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 
> ---
> 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()? Or
> of_detach_node()? Or what?

I'm not sure we would want to remove the device at all, we certainly should not
be removing the dt_node from the devicetree IMHO. Having that around to see how
the bootloader set things up is really useful for debugging and normally we
should never modify the devicetree as set up by the bootloader.

Why not just unbind the driver from the platform device? That should be enough.

Regards,

Hans




> 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?
> Also: Does any platform make use the of 'display:' entry in 
> 'simple-framebuffer'
> DT nodes? If yes, how do I get access to it? And why don't vc4/sun4i make use 
> of
> it, rather than falling back to remove_conflicting_framebuffers()?
>
> Thanks
> David
>
>  drivers/gpu/drm/drm_drv.c | 257 
> ++
>  include/drm/drmP.h|  13 +++
>  2 files changed, 270 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 0773547..581c342 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -26,12 +26,16 @@
>   * DEALINGS IN THE SOFTWARE.
>   */
>
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include "drm_crtc_internal.h"
>  #include "drm_legacy.h"
> @@ -881,6 +885,259 @@ void drm_dev_unregister(struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_dev_unregister);
>
> +struct drm_evict_ctx {
> + struct apertures_struct *ap;
> + unsigned int flags;
> +};
> +
> +static bool drm_evict_match_resource(struct drm_evict_ctx *ctx,
> +  struct resource *mem)
> +{
> + struct aperture *g;
> + unsigned int i;
> +
> + for (i = 0; i < ctx->ap->count; ++i) {
> + g = >ap->ranges[i];
> +
> + if (mem->start == g->base)
> + return true;
> + if (mem->start >= g->base && mem->end < g->base + g->size)
> + return true;
> + if ((ctx->flags & DRM_EVICT_VBE) && mem->start == 0xA)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static int drm_evict_platform_device(struct device *dev, void *userdata)
> +{
> + struct drm_evict_ctx *ctx = userdata;
> + struct platform_device *pdev = to_platform_device(dev);
> + struct resource *mem;
> +
> + /*
> +  * We are only interested in static devices here (i.e., they carry
> +  * information via a statically allocated platform data field). Any
> +  * dynamically allocated devices have separate ownership models and
> +  * must be handled via their respective management calls.
> +  */
> + if (!dev_get_platdata(>dev))
> + return 0;
> + if (!pdev->name)
> + return 0;
> +
> + if (!strcmp(pdev->name, "simple-framebuffer")) {
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!mem)
> + return 0;
> + if (!drm_evict_match_resource(ctx, mem))
> + return 0;
> +
> + platform_device_del(pdev);
> + }
> +
> + return 0;
> +}
> +
> +static int drm_evict_platform(struct drm_evict_ctx *ctx)
> +{
> + /*
> +  * Early-boot architecture setup and boot-loarders sometimes create
> +  * preliminary platform devices with a generic framebuffer setup. This
> +  * allows graphics access during boot-up, without a real graphics
> +  * driver loaded. However, once a real graphics driver takes over, we
> +  * have to destroy those platform devices. In the legacy fbdev case, we
> +  * just used to unload the fbdev driver. However, to make sure any kind
> +  * of driver is unloaded, the platform-eviction code here simply
> +  * removes any conflicting platform device 

[RFC] drm: implement generic firmware eviction

[Daniel Vetter]

On Fri, Aug 26, 2016 at 2:00 AM, David Herrmann  
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 
> ---
> 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()? Or
> of_detach_node()? Or what?
> 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?
> Also: Does any platform make use the of 'display:' entry in 
> 'simple-framebuffer'
> DT nodes? If yes, how do I get access to it? And why don't vc4/sun4i make use 
> of
> it, rather than falling back to remove_conflicting_framebuffers()?

If we revamp this I'd like to make it a lot more automatic. Atm all
drivers need to explicitly figure out what they need to kick (and as
you noticed, many seem to get it wrong). But in most cases it's the
platform code that would know this best. What about

drm_evict_firmware_pci(struct pci_device *pci);

which then:
- does the remove_conflicting_framebuffer dance with all the pci bars
- if the device class is VGA, also nuke vgacon (not just when it's the
currently decoding VGA device, since that might change later on)

Similar for other types of devices (i.e. drm_evict_firmware_platform
for arm/of). Or maybe we can just take a struct device *dev and cast
suitably within the helper to hide it all?

This is already what correct drivers are doing, and we can still
expose the internals if there's ever a case where things work
differently.

A few more comments below.

> Thanks
> David
>
>  drivers/gpu/drm/drm_drv.c | 257 
> ++
>  include/drm/drmP.h|  13 +++
>  2 files changed, 270 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 0773547..581c342 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -26,12 +26,16 @@
>   * DEALINGS IN THE SOFTWARE.
>   */
>
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include "drm_crtc_internal.h"
>  #include "drm_legacy.h"
> @@ -881,6 +885,259 @@ void drm_dev_unregister(struct drm_device *dev)
>  }
>  EXPORT_SYMBOL(drm_dev_unregister);
>
> +struct drm_evict_ctx {
> +   struct apertures_struct *ap;
> +   unsigned int flags;
> +};
> +
> +static bool drm_evict_match_resource(struct drm_evict_ctx *ctx,
> +struct resource *mem)
> +{
> +   struct aperture *g;
> +   unsigned int i;
> +
> +   for (i = 0; i < ctx->ap->count; ++i) {
> +   g = >ap->ranges[i];
> +
> +   if (mem->start == g->base)
> +   return true;
> +   if (mem->start >= g->base && mem->end < g->base + g->size)
> +   return true;
> +   if ((ctx->flags & DRM_EVICT_VBE) && mem->start == 0xA)
> +   return true;
> +   }
> +
> +   return false;
> +}
> +
> +static int drm_evict_platform_device(struct device *dev, void *userdata)
> +{
> +   struct drm_evict_ctx *ctx = userdata;
> +   struct platform_device *pdev = to_platform_device(dev);
> +   struct resource *mem;
> +
> +   /*
> +* We are only interested in static devices here (i.e., they carry
> +* information via a statically allocated platform data field). Any
> +* dynamically allocated devices have separate ownership models and
> +* must be handled via their respective management calls.
> +*/
> +   if (!dev_get_platdata(>dev))
> +   return 0;
> +   if (!pdev->name)
> +   return 0;
> +
> +   if (!strcmp(pdev->name, "simple-framebuffer")) {
> +   mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +   if (!mem)
> +   return 0;
> +   if (!drm_evict_match_resource(ctx, mem))
> +   return 0;
> +
> +   platform_device_del(pdev);
> +   }
> +
> +   return 0;
> +}

Shouldn't the device framework and iores allocator make sure that such
conflicts are handled? Assuming of course that 

[RFC] drm: implement generic firmware eviction

[Rob Herring]

On Thu, Aug 25, 2016 at 7:00 PM, David Herrmann  
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 
> ---
> 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.

> Also: Does any platform make use the of 'display:' entry in 
> 'simple-framebuffer'
> DT nodes? If yes, how do I get access to it? And why don't vc4/sun4i make use 
> of
> it, rather than falling back to remove_conflicting_framebuffers()?

No idea.

Rob

[RFC] drm: implement generic firmware eviction

[David Herrmann]

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 
---
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()? Or
of_detach_node()? Or what?
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?
Also: Does any platform make use the of 'display:' entry in 'simple-framebuffer'
DT nodes? If yes, how do I get access to it? And why don't vc4/sun4i make use of
it, rather than falling back to remove_conflicting_framebuffers()?

Thanks
David

 drivers/gpu/drm/drm_drv.c | 257 ++
 include/drm/drmP.h|  13 +++
 2 files changed, 270 insertions(+)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 0773547..581c342 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -26,12 +26,16 @@
  * DEALINGS IN THE SOFTWARE.
  */

+#include 
 #include 
+#include 
 #include 
 #include 
 #include 
 #include 
+#include 
 #include 
+#include 
 #include 
 #include "drm_crtc_internal.h"
 #include "drm_legacy.h"
@@ -881,6 +885,259 @@ void drm_dev_unregister(struct drm_device *dev)
 }
 EXPORT_SYMBOL(drm_dev_unregister);

+struct drm_evict_ctx {
+   struct apertures_struct *ap;
+   unsigned int flags;
+};
+
+static bool drm_evict_match_resource(struct drm_evict_ctx *ctx,
+struct resource *mem)
+{
+   struct aperture *g;
+   unsigned int i;
+
+   for (i = 0; i < ctx->ap->count; ++i) {
+   g = >ap->ranges[i];
+
+   if (mem->start == g->base)
+   return true;
+   if (mem->start >= g->base && mem->end < g->base + g->size)
+   return true;
+   if ((ctx->flags & DRM_EVICT_VBE) && mem->start == 0xA)
+   return true;
+   }
+
+   return false;
+}
+
+static int drm_evict_platform_device(struct device *dev, void *userdata)
+{
+   struct drm_evict_ctx *ctx = userdata;
+   struct platform_device *pdev = to_platform_device(dev);
+   struct resource *mem;
+
+   /*
+* We are only interested in static devices here (i.e., they carry
+* information via a statically allocated platform data field). Any
+* dynamically allocated devices have separate ownership models and
+* must be handled via their respective management calls.
+*/
+   if (!dev_get_platdata(>dev))
+   return 0;
+   if (!pdev->name)
+   return 0;
+
+   if (!strcmp(pdev->name, "simple-framebuffer")) {
+   mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   if (!mem)
+   return 0;
+   if (!drm_evict_match_resource(ctx, mem))
+   return 0;
+
+   platform_device_del(pdev);
+   }
+
+   return 0;
+}
+
+static int drm_evict_platform(struct drm_evict_ctx *ctx)
+{
+   /*
+* Early-boot architecture setup and boot-loarders sometimes create
+* preliminary platform devices with a generic framebuffer setup. This
+* allows graphics access during boot-up, without a real graphics
+* driver loaded. However, once a real graphics driver takes over, we
+* have to destroy those platform devices. In the legacy fbdev case, we
+* just used to unload the fbdev driver. However, to make sure any kind
+* of driver is unloaded, the platform-eviction code here simply
+* removes any conflicting platform device directly. This causes any
+* bound driver to be detached, and then removes the device entirely so
+* it cannot be bound to later on.
+*
+* Please note that any such platform device must be registered by
+* early architecture setup code. If they are registered after regular
+* DRM drivers, this will fail horribly.
+*/
+
+   static DEFINE_MUTEX(lock);
+   int ret;
+
+   /*
+* In case of static platform-devices, we must iterate the bus and
+* remove them manually. We know that we're the only code that might
+* remove them, so a