[PATCH v2 1/7] drm/exynos: add super device support
2014-04-08 0:40 GMT+09:00, Tomasz Figa :
> On 07.04.2014 17:16, Inki Dae wrote:
>> Hi Andrzej,
>>
>> 2014-04-07 23:18 GMT+09:00 Andrzej Hajda :
>>> Hi Inki and Tomasz,
>>>
>>> On 04/06/2014 05:15 AM, Inki Dae wrote:
>>>
>>> (...)
The code creating the list of components to wait for
(exynos_drm_add_components()) doesn't seem to consider which sub-drivers
are
actually enabled in kernel config.
Are you sure?
exynos_drm_add_components() will try to attach components *added to
component_lists. And these components will be added by only
corresponding sub drivers to the component_lists and
master->components.
So in this case, if users disabled HDMI support then a commponent
object for HDMI sub driver doesn't exists in the component_lists and
master->components. This means that a component object for HDMI sub
driver *cannot be attached to master object*.
As a result, component_bind_add() will ignor component_bind call for
HDMI sub driver so HDMI driver will not be bounded. The only
components added by sub drivers will be bound, component->ops->bind().
For more understanding, it seems like you need to look into below
codes,
static int exynos_drm_add_components(...)
{
...
for (i == 0;; i++) {
...
node = of_parse_phandle(np, "ports", i);
...
ret = component_master_add_child(m, compare_of, node);
...
}
}
>>>
>>> Hmm, In case HDMI driver is not present, HDMI device is not probed and
>>> HDMI component is not added, so component_master_add_child returns
>>> an error when it tries to add hdmi component and master is never brought
>>> up, am I correct?
>>>
>>
>> Ok, after that,
>>
>> ret = component_master_add(,..)
>> if (ret < 0)
>> DRM_DEBUG_KMS("re-tried by last sub driver probed later.\n");
>>
>> As you can see above, if component_master_add() is failed, return 0,
>> *not error*. And then component_add() called by other kms drivers,
>> late probing of hdmi or fimd probing - if there is any kms driver
>> enabled - will try to bring up master again by calling
>> try_to_bring_up_master() from beginning.
>>
>> And if component_list is empty then it means that there is no any kms
>> driver enabled.
>>
>> Do you still think in that case, exynos drm initialization will be
>> failed?
>
> There will be no HDMI driver to call component_add(), because HDMI sub
> driver will be disabled in kernel config and any previous
> component_master_add_child() for the phandle pointing to HDMI node will
> wail, because such component will never be registered.
>
> The complete failure path is as follows:
>
> exynos_drm_platform_probe()
> component_master_add()
> try_to_bring_up_master()
> exynos_drm_add_components()
> // phandle to HDMI node
> component_master_add_child()
> = -ENXIO
> = -ENXIO
> = 0 // but no call to master->ops->bind(master->dev);
> = 0
> = 0 // but Exynos DRM platform is not registered yet
>
> Now if any other late-probed sub-driver comes, the sequence will be as
> follows (let's take FIMD as an example):
>
> fimd_probe()
> component_add()
> try_to_bring_up_masters()
> try_to_bring_up_master()
> exynos_drm_add_components()
> // phandle to HDMI node
> component_master_add_child()
> = -ENXIO
> = -ENXIO
> = 0 // but no call to
> // master->ops->bind(master->dev);
> = 0
> = 0
> = 0 // but Exynos DRM platform still not registered
>
> And the same for any late-probed driver, unless HDMI drivers loads, but
> there is no HDMI driver, because it is disabled in kernel config and not
> compiled in.
>
Ah, right. I missed that it gets phandle to hdmi, and compares the
phandle to one in component_list.
Yes, in this case, exynos drm driver will be failed because
component_list never has a component object for hdmi.
So this shouldn't return as long as there is any component - in
component_list - that can be attached to master.
Hm.. the foot of the candle is dark.
For this, I fixed and tested it like below,
for (i = 0;; i++) {
node = of_parse_phandle(np, "ports", i);
...
ret = component_master_add_child(...);
...
if (!ret)
attached_cnt++;
}
if (!attached_cnt)
return -ENXIO;
...
And one more thing, it seems that we could resolve dt broken issue
easily by getting phandle to existing device node directly.
Thanks,
Inki Dae
>
[PATCH v2 1/7] drm/exynos: add super device support
Hi Andrzej,
2014-04-07 23:18 GMT+09:00 Andrzej Hajda :
> Hi Inki and Tomasz,
>
> On 04/06/2014 05:15 AM, Inki Dae wrote:
>
> (...)
>> The code creating the list of components to wait for
>> (exynos_drm_add_components()) doesn't seem to consider which sub-drivers are
>> actually enabled in kernel config.
>>
>> Are you sure?
>>
>> exynos_drm_add_components() will try to attach components *added to
>> component_lists. And these components will be added by only
>> corresponding sub drivers to the component_lists and
>> master->components.
>>
>> So in this case, if users disabled HDMI support then a commponent
>> object for HDMI sub driver doesn't exists in the component_lists and
>> master->components. This means that a component object for HDMI sub
>> driver *cannot be attached to master object*.
>>
>> As a result, component_bind_add() will ignor component_bind call for
>> HDMI sub driver so HDMI driver will not be bounded. The only
>> components added by sub drivers will be bound, component->ops->bind().
>>
>> For more understanding, it seems like you need to look into below codes,
>>
>> static int exynos_drm_add_components(...)
>> {
>> ...
>> for (i == 0;; i++) {
>> ...
>> node = of_parse_phandle(np, "ports", i);
>> ...
>> ret = component_master_add_child(m, compare_of, node);
>> ...
>> }
>> }
>
> Hmm, In case HDMI driver is not present, HDMI device is not probed and
> HDMI component is not added, so component_master_add_child returns
> an error when it tries to add hdmi component and master is never brought
> up, am I correct?
>
Ok, after that,
ret = component_master_add(,..)
if (ret < 0)
DRM_DEBUG_KMS("re-tried by last sub driver probed later.\n");
As you can see above, if component_master_add() is failed, return 0,
*not error*. And then component_add() called by other kms drivers,
late probing of hdmi or fimd probing - if there is any kms driver
enabled - will try to bring up master again by calling
try_to_bring_up_master() from beginning.
And if component_list is empty then it means that there is no any kms
driver enabled.
Do you still think in that case, exynos drm initialization will be failed?
Thanks,
Inki Dae
>>
>>
>> And below codes,
>>
>> int component_master_add_child(...)
>> {
>> list_for_each_entry(c, _list, node) {
>> if (c->master)
>> continue;
>>
>> if (compare(...)) {
>> component_attach_master(master, c);
>> ...
>> }
>> }
>> }
>>
>> And below codes,
>>
>> static void component_attach_master(master, c)
>> {
>> c->master = master;
>> list_add_tail(>master_node, >comonents);
>> }
>>
>>
>> As you can see above, the only components added to component_list can
>> be attached to master. And the important thing is that components can
>> be added by sub drivers to the component_list.
>>
>> And below codes that actually tries to bind each sub drivers,
>>
>> int component_bind_add(...)
>> {
>>
>> list_for_each_entry(c, >components, master_node) {
>>ret = component_bind(c, master, data);
>>...
>> }
>> ...
>> }
>>
>> The hdmi driver users disabled doesn't exist to master->components list.
>> How Exynos DRM cannot be initialized?
>>
>> Of course, there may be my missing point, but I think I see them
>> correctly. Anyway I will test them tomorrow.
>>
>> Thanks,
>> Inki Dae
>>
> register, which is completely wrong. Users should be able to select which
> drivers should be compiled into their kernels.
So users are be able to select drivers they want to use, and will be
compiled correctly. So no, the only thing users can disable is each
sub driver, not core module.
> 3) Such approach leads to complete integration of all Exynos DRM drivers,
> without possibility of loading some sub-drivers as modules. I know that
> current driver design doesn't support it either, but if this series is
No, current drm driver *must also be built* as one integrated single
drm driver without super device approach.
>>>
>>> As I wrote, I know that current design before this patch isn't modular
>>> either, but this is not my concern here. See below.
>>>
>>>
So the super device approach
*has no any effect on existing design*, and what the super device
approch tries to do is to resolve the probe order issue to sub drivers
*without some codes specific to Exynos drm*.
>>>
>>> My concern is that the supernode design is actually carving such broken
>>> non-modular design in stone. Remember that we are currently heading towards
>>> a fully multi-platform kernel where it is critical for such subsystems to be
>>> modular, because the same zImage is going to be running on completely
>>> different machines.
>>>
>>>
[PATCH v2 1/7] drm/exynos: add super device support
On 07.04.2014 17:16, Inki Dae wrote:
> Hi Andrzej,
>
> 2014-04-07 23:18 GMT+09:00 Andrzej Hajda :
>> Hi Inki and Tomasz,
>>
>> On 04/06/2014 05:15 AM, Inki Dae wrote:
>>
>> (...)
>>> The code creating the list of components to wait for
>>> (exynos_drm_add_components()) doesn't seem to consider which sub-drivers are
>>> actually enabled in kernel config.
>>>
>>> Are you sure?
>>>
>>> exynos_drm_add_components() will try to attach components *added to
>>> component_lists. And these components will be added by only
>>> corresponding sub drivers to the component_lists and
>>> master->components.
>>>
>>> So in this case, if users disabled HDMI support then a commponent
>>> object for HDMI sub driver doesn't exists in the component_lists and
>>> master->components. This means that a component object for HDMI sub
>>> driver *cannot be attached to master object*.
>>>
>>> As a result, component_bind_add() will ignor component_bind call for
>>> HDMI sub driver so HDMI driver will not be bounded. The only
>>> components added by sub drivers will be bound, component->ops->bind().
>>>
>>> For more understanding, it seems like you need to look into below codes,
>>>
>>> static int exynos_drm_add_components(...)
>>> {
>>> ...
>>> for (i == 0;; i++) {
>>> ...
>>> node = of_parse_phandle(np, "ports", i);
>>> ...
>>> ret = component_master_add_child(m, compare_of, node);
>>> ...
>>> }
>>> }
>>
>> Hmm, In case HDMI driver is not present, HDMI device is not probed and
>> HDMI component is not added, so component_master_add_child returns
>> an error when it tries to add hdmi component and master is never brought
>> up, am I correct?
>>
>
> Ok, after that,
>
> ret = component_master_add(,..)
> if (ret < 0)
> DRM_DEBUG_KMS("re-tried by last sub driver probed later.\n");
>
> As you can see above, if component_master_add() is failed, return 0,
> *not error*. And then component_add() called by other kms drivers,
> late probing of hdmi or fimd probing - if there is any kms driver
> enabled - will try to bring up master again by calling
> try_to_bring_up_master() from beginning.
>
> And if component_list is empty then it means that there is no any kms
> driver enabled.
>
> Do you still think in that case, exynos drm initialization will be failed?
There will be no HDMI driver to call component_add(), because HDMI sub
driver will be disabled in kernel config and any previous
component_master_add_child() for the phandle pointing to HDMI node will
wail, because such component will never be registered.
The complete failure path is as follows:
exynos_drm_platform_probe()
component_master_add()
try_to_bring_up_master()
exynos_drm_add_components()
// phandle to HDMI node
component_master_add_child()
= -ENXIO
= -ENXIO
= 0 // but no call to master->ops->bind(master->dev);
= 0
= 0 // but Exynos DRM platform is not registered yet
Now if any other late-probed sub-driver comes, the sequence will be as
follows (let's take FIMD as an example):
fimd_probe()
component_add()
try_to_bring_up_masters()
try_to_bring_up_master()
exynos_drm_add_components()
// phandle to HDMI node
component_master_add_child()
= -ENXIO
= -ENXIO
= 0 // but no call to
// master->ops->bind(master->dev);
= 0
= 0
= 0 // but Exynos DRM platform still not registered
And the same for any late-probed driver, unless HDMI drivers loads, but
there is no HDMI driver, because it is disabled in kernel config and not
compiled in.
Best regards,
Tomasz
[PATCH v2 1/7] drm/exynos: add super device support
Hi Andrzej,
On 07.04.2014 16:18, Andrzej Hajda wrote:
> Hi Inki and Tomasz,
>
> On 04/06/2014 05:15 AM, Inki Dae wrote:
>
> (...)
>> The code creating the list of components to wait for
>> (exynos_drm_add_components()) doesn't seem to consider which sub-drivers are
>> actually enabled in kernel config.
>>
>> Are you sure?
>>
>> exynos_drm_add_components() will try to attach components *added to
>> component_lists. And these components will be added by only
>> corresponding sub drivers to the component_lists and
>> master->components.
>>
>> So in this case, if users disabled HDMI support then a commponent
>> object for HDMI sub driver doesn't exists in the component_lists and
>> master->components. This means that a component object for HDMI sub
>> driver *cannot be attached to master object*.
>>
>> As a result, component_bind_add() will ignor component_bind call for
>> HDMI sub driver so HDMI driver will not be bounded. The only
>> components added by sub drivers will be bound, component->ops->bind().
>>
>> For more understanding, it seems like you need to look into below codes,
>>
>> static int exynos_drm_add_components(...)
>> {
>> ...
>> for (i == 0;; i++) {
>> ...
>> node = of_parse_phandle(np, "ports", i);
>> ...
>> ret = component_master_add_child(m, compare_of, node);
>> ...
>> }
>> }
>
> Hmm, In case HDMI driver is not present, HDMI device is not probed and
> HDMI component is not added, so component_master_add_child returns
> an error when it tries to add hdmi component and master is never brought
> up, am I correct?
>
This is my thought here as well.
>>
>>
>> And below codes,
>>
>> int component_master_add_child(...)
>> {
>> list_for_each_entry(c, _list, node) {
>> if (c->master)
>> continue;
>>
>> if (compare(...)) {
>> component_attach_master(master, c);
>> ...
>> }
>> }
>> }
>>
>> And below codes,
>>
>> static void component_attach_master(master, c)
>> {
>> c->master = master;
>> list_add_tail(>master_node, >comonents);
>> }
>>
>>
>> As you can see above, the only components added to component_list can
>> be attached to master. And the important thing is that components can
>> be added by sub drivers to the component_list.
>>
>> And below codes that actually tries to bind each sub drivers,
>>
>> int component_bind_add(...)
>> {
>>
>> list_for_each_entry(c, >components, master_node) {
>> ret = component_bind(c, master, data);
>> ...
>> }
>> ...
>> }
>>
>> The hdmi driver users disabled doesn't exist to master->components list.
>> How Exynos DRM cannot be initialized?
>>
>> Of course, there may be my missing point, but I think I see them
>> correctly. Anyway I will test them tomorrow.
>>
>> Thanks,
>> Inki Dae
>>
> register, which is completely wrong. Users should be able to select which
> drivers should be compiled into their kernels.
So users are be able to select drivers they want to use, and will be
compiled correctly. So no, the only thing users can disable is each
sub driver, not core module.
> 3) Such approach leads to complete integration of all Exynos DRM drivers,
> without possibility of loading some sub-drivers as modules. I know that
> current driver design doesn't support it either, but if this series is
No, current drm driver *must also be built* as one integrated single
drm driver without super device approach.
>>>
>>> As I wrote, I know that current design before this patch isn't modular
>>> either, but this is not my concern here. See below.
>>>
>>>
So the super device approach
*has no any effect on existing design*, and what the super device
approch tries to do is to resolve the probe order issue to sub drivers
*without some codes specific to Exynos drm*.
>>>
>>> My concern is that the supernode design is actually carving such broken
>>> non-modular design in stone. Remember that we are currently heading towards
>>> a fully multi-platform kernel where it is critical for such subsystems to be
>>> modular, because the same zImage is going to be running on completely
>>> different machines.
>>>
>>>
> claimed to improve things, it should really do so.
>
> 4) Exactly the same can be achieved without changing the DT bindings at
> all.
> In fact even without adding any new single property or node to DT. We
> discussed this with Andrzej and Marek today and came to a solution in
> which
> just by adding a little bit of code to Exynos DRM subdrivers, you could
> guarantee correct registration of Exynos DRM platform and also get rid of
> #ifdeffery in exynos_drm_drv.c. Andrzej will send an RFC after the
>
[PATCH v2 1/7] drm/exynos: add super device support
Hi Inki and Tomasz,
On 04/06/2014 05:15 AM, Inki Dae wrote:
(...)
> The code creating the list of components to wait for
> (exynos_drm_add_components()) doesn't seem to consider which sub-drivers are
> actually enabled in kernel config.
>
> Are you sure?
>
> exynos_drm_add_components() will try to attach components *added to
> component_lists. And these components will be added by only
> corresponding sub drivers to the component_lists and
> master->components.
>
> So in this case, if users disabled HDMI support then a commponent
> object for HDMI sub driver doesn't exists in the component_lists and
> master->components. This means that a component object for HDMI sub
> driver *cannot be attached to master object*.
>
> As a result, component_bind_add() will ignor component_bind call for
> HDMI sub driver so HDMI driver will not be bounded. The only
> components added by sub drivers will be bound, component->ops->bind().
>
> For more understanding, it seems like you need to look into below codes,
>
> static int exynos_drm_add_components(...)
> {
> ...
> for (i == 0;; i++) {
> ...
> node = of_parse_phandle(np, "ports", i);
> ...
> ret = component_master_add_child(m, compare_of, node);
> ...
> }
> }
Hmm, In case HDMI driver is not present, HDMI device is not probed and
HDMI component is not added, so component_master_add_child returns
an error when it tries to add hdmi component and master is never brought
up, am I correct?
>
>
> And below codes,
>
> int component_master_add_child(...)
> {
> list_for_each_entry(c, _list, node) {
> if (c->master)
> continue;
>
> if (compare(...)) {
> component_attach_master(master, c);
> ...
> }
> }
> }
>
> And below codes,
>
> static void component_attach_master(master, c)
> {
> c->master = master;
> list_add_tail(>master_node, >comonents);
> }
>
>
> As you can see above, the only components added to component_list can
> be attached to master. And the important thing is that components can
> be added by sub drivers to the component_list.
>
> And below codes that actually tries to bind each sub drivers,
>
> int component_bind_add(...)
> {
>
> list_for_each_entry(c, >components, master_node) {
>ret = component_bind(c, master, data);
>...
> }
> ...
> }
>
> The hdmi driver users disabled doesn't exist to master->components list.
> How Exynos DRM cannot be initialized?
>
> Of course, there may be my missing point, but I think I see them
> correctly. Anyway I will test them tomorrow.
>
> Thanks,
> Inki Dae
>
register, which is completely wrong. Users should be able to select which
drivers should be compiled into their kernels.
>>>
>>> So users are be able to select drivers they want to use, and will be
>>> compiled correctly. So no, the only thing users can disable is each
>>> sub driver, not core module.
>>>
3) Such approach leads to complete integration of all Exynos DRM drivers,
without possibility of loading some sub-drivers as modules. I know that
current driver design doesn't support it either, but if this series is
>>>
>>> No, current drm driver *must also be built* as one integrated single
>>> drm driver without super device approach.
>>
>> As I wrote, I know that current design before this patch isn't modular
>> either, but this is not my concern here. See below.
>>
>>
>>> So the super device approach
>>> *has no any effect on existing design*, and what the super device
>>> approch tries to do is to resolve the probe order issue to sub drivers
>>> *without some codes specific to Exynos drm*.
>>
>> My concern is that the supernode design is actually carving such broken
>> non-modular design in stone. Remember that we are currently heading towards
>> a fully multi-platform kernel where it is critical for such subsystems to be
>> modular, because the same zImage is going to be running on completely
>> different machines.
>>
>>
claimed to improve things, it should really do so.
4) Exactly the same can be achieved without changing the DT bindings at
all.
In fact even without adding any new single property or node to DT. We
discussed this with Andrzej and Marek today and came to a solution in
which
just by adding a little bit of code to Exynos DRM subdrivers, you could
guarantee correct registration of Exynos DRM platform and also get rid of
#ifdeffery in exynos_drm_drv.c. Andrzej will send an RFC after the
weekend.
>>>
>>> I'm not sure but I had implemented below prototype codes for that, see
>>> the below link,
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/commit/?h=exynos-bridge-test=2860ad02d736e300034ddeabce3da92614922b4e
>>>
>>> I
[PATCH v2 1/7] drm/exynos: add super device support
Hi Russell, 2014-04-06 17:47 GMT+09:00 Russell King - ARM Linux : > On Sun, Apr 06, 2014 at 01:21:24PM +0900, Inki Dae wrote: >> As you may know, there is exynos chip that has two display >> controllers. So it is possible to compose display pipe lines at same >> time like below, >> >> fimd0-bridges..--Panel >> fimd1-maybe bridges..-CPU Panel >> >> How we can care such pipelines without component? Of course, it would >> be possible somehow but I'm not sure there could be better and more >> generic way than super device. >> >> And super device will make existing dtb broken but this time it might >> be good opportunity to more generic way before more users use existing >> dt. > > There has been a discussion ongoing about how to best represent display > (and capture) stuff in DT using graphs, which you may wish to consider Right, there was my wrong comments. Actually, I had made a discussion about it. For this, you can refer to below link, http://www.spinics.net/lists/dri-devel/msg5.html My intension there was that let's use super device approach to resolve the probe order issue of drm drivers for embedded system, and the graphs to compose their display pipelines. And for this, we need generic common structure to handle various bridge devices that could be controlled by crtc and encoder/connector drivers. I'd be happy to get your opinions about it. > looking at. That discussion never reached any kind of conclusion before > the merge window, but it does need to reach a conclusion for the next > window. Yes. I still think the super device approach is a best solution, and I think the only problem here is backward compatibility. I have a feel that we might resolve that issue easier then we think. I think all we have to do for it is to implement some codes for backward compatibility on top of super device approach. This way, we could have a better design for the future drm drivers without being tied to the backward compatibility. Thanks, Inki Dae > > -- > FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly > improving, and getting towards what was expected from it. > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/7] drm/exynos: add super device support
2014-04-06 3:31 GMT+09:00, Tomasz Figa :
>
>
> On 05.04.2014 20:24, Russell King - ARM Linux wrote:
>> On Sat, Apr 05, 2014 at 07:32:50PM +0200, Tomasz Figa wrote:
>>> Not exactly. The approach we found does mostly the same as componentized
>>> subsystem framework but without _any_ extra data in Device Tree. Just
>>> based on the list of subsystem sub-drivers that is already available to
>>> the master driver.
>>
>> The existing approach is fundamentally broken. Yes, your solution may
>> work for the probing case, but have you tried unbinding any of your
>> sub-drivers?
>>
>> From what I can see, that causes a kernel oops for one very simple reason
>> -
>> you destroy stuff while it's still in use. Let's look at an example:
>>
>> struct platform_driver ipp_driver = {
>> .probe = ipp_probe,
>> .remove = ipp_remove,
>> .driver = {
>> .name = "exynos-drm-ipp",
>> .owner = THIS_MODULE,
>> .pm = _pm_ops,
>> },
>> };
>>
>> static int ipp_remove(struct platform_device *pdev)
>> {
>> struct ipp_context *ctx = platform_get_drvdata(pdev);
>>
>> /* unregister sub driver */
>> exynos_drm_subdrv_unregister(>subdrv);
>>
>> /* remove,destroy ipp idr */
>> idr_destroy(>ipp_idr);
>> idr_destroy(>prop_idr);
>>
>> mutex_destroy(>ipp_lock);
>> mutex_destroy(>prop_lock);
>>
>> /* destroy command, event work queue */
>> destroy_workqueue(ctx->cmd_workq);
>> destroy_workqueue(ctx->event_workq);
>>
>> return 0;
>> }
>>
>> int exynos_drm_subdrv_unregister(struct exynos_drm_subdrv *subdrv)
>> {
>> if (!subdrv)
>> return -EINVAL;
>>
>> list_del(>list);
>>
>> return 0;
>> }
>>
>> Oh dear, that destroys a whole pile of resources which could already
>> be in use without telling anything that it's about to do that.
>>
>> I'm sure if I continue looking at the exynos stuff, it'll show similar
>> crap all over the place.
>>
>> What you have now in mainline is not a solution. It's a crappy bodge.
>>
>
> Undoubtedly. Nobody here is trying to state the opposite.
>
> Maybe my words have been misinterpreted, but all I'm suggesting here is
> that there is no need to add any new data to DT to solve the same issue
> to the same extent as componentized subsystem framework, at least in
> Exynos case.
As you may know, there is exynos chip that has two display
controllers. So it is possible to compose display pipe lines at same
time like below,
fimd0-bridges..--Panel
fimd1-maybe bridges..-CPU Panel
How we can care such pipelines without component? Of course, it would
be possible somehow but I'm not sure there could be better and more
generic way than super device.
And super device will make existing dtb broken but this time it might
be good opportunity to more generic way before more users use existing
dt.
Thanks,
Inki Dae
>
> Best regards,
> Tomasz
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
[PATCH v2 1/7] drm/exynos: add super device support
I'd be really happy if you gave me such opinions before pull request.
Anyway, below is my comments.
2014-04-06 2:32 GMT+09:00 Tomasz Figa :
> [adding more people and MLs on Cc for further discussion]
>
>
> On 04.04.2014 17:44, Inki Dae wrote:
>>
>> 2014-04-04 22:55 GMT+09:00 Tomasz Figa :
>>>
>>> Hi Inki,
>>>
>>>
>>> On 01.04.2014 14:37, Inki Dae wrote:
This patch adds super device support to bind sub drivers
using device tree.
For this, you should add a super device node to each machine dt files
like belows,
In case of using MIPI-DSI,
display-subsystem {
compatible = "samsung,exynos-display-subsystem";
ports = <>, <>;
};
In case of using DisplayPort,
display-subsystem {
compatible = "samsung,exynos-display-subsystem";
ports = <>, <>;
};
In case of using Parallel panel,
display-subsystem {
compatible = "samsung,exynos-display-subsystem";
ports = <>;
};
And if you don't add connector device node to ports property,
default parallel panel driver, exynos_drm_dpi module, will be used.
ports property can have the following device nodes,
fimd, mixer, Image Enhancer, MIPI-DSI, eDP, LVDS Bridge, or
HDMI
With this patch, we can resolve the probing order issue without
some global lists. So this patch also removes the unnecessary lists and
stuff related to these lists.
>>>
>>>
>>>
>>> I can see several problems with this approach:
>>>
>>> 1) It breaks compatibility with existing DT. After this patch it is no
>>> longer possible to use old device trees and get a working DRM. However,
>>> in
>>> my opinion, this requirement can be relaxed if we make sure that any
>>> users
>>> are properly converted.
>>>
>>> 2) What happens if in Kconfig you disable a driver for a component that
>>> is
>>
>>
>> I'm not sure what you meant but there wouldn't be no way that users
>> *cannot disable* the driver for a component. The driver would be
>> exynos_drm_drv, not separated module, and would always be built as
>> long as users want to use *exynos drm driver*.
>
>
> I think you don't understand what I mean. Let me show you an example:
>
> You have a board with a DSI panel and also a HDMI output. So you have a
> supernode pointing to FIMD, DSI and HDMI.
>
> Now, an user finds that he doesn't need HDMI in his system, so he turns off
> CONFIG_DRM_EXYNOS_HDMI. The supernode still points at hdmi node and Exynos
> DRM core will register it as a component, but HDMI driver is not available
> and will never probe, leading the whole Exynos DRM to never initialize. Is
> this a desired behavior?
>
Ok, now I understood. Your comment was not enough to me.
First of all, if I see component codes correctly then it seems your
misunderstanding. See the below comments,
>
>>
>>> listed in supernode? If I'm reading the code correctly, Exynos DRM will
>>> not
>>
>>
>> And the only case a component isn't added to the list is when users
>> disabled sub driver.
>
>
> See above.
>
> The code creating the list of components to wait for
> (exynos_drm_add_components()) doesn't seem to consider which sub-drivers are
> actually enabled in kernel config.
>
Are you sure?
exynos_drm_add_components() will try to attach components *added to
component_lists. And these components will be added by only
corresponding sub drivers to the component_lists and
master->components.
So in this case, if users disabled HDMI support then a commponent
object for HDMI sub driver doesn't exists in the component_lists and
master->components. This means that a component object for HDMI sub
driver *cannot be attached to master object*.
As a result, component_bind_add() will ignor component_bind call for
HDMI sub driver so HDMI driver will not be bounded. The only
components added by sub drivers will be bound, component->ops->bind().
For more understanding, it seems like you need to look into below codes,
static int exynos_drm_add_components(...)
{
...
for (i == 0;; i++) {
...
node = of_parse_phandle(np, "ports", i);
...
ret = component_master_add_child(m, compare_of, node);
...
}
}
And below codes,
int component_master_add_child(...)
{
list_for_each_entry(c, _list, node) {
if (c->master)
continue;
if (compare(...)) {
component_attach_master(master, c);
...
}
}
}
And below codes,
static void component_attach_master(master, c)
{
c->master = master;
list_add_tail(>master_node, >comonents);
}
As you can see above, the only components added to component_list can
be attached to master.
[PATCH v2 1/7] drm/exynos: add super device support
On Sun, Apr 06, 2014 at 01:21:24PM +0900, Inki Dae wrote: > As you may know, there is exynos chip that has two display > controllers. So it is possible to compose display pipe lines at same > time like below, > > fimd0-bridges..--Panel > fimd1-maybe bridges..-CPU Panel > > How we can care such pipelines without component? Of course, it would > be possible somehow but I'm not sure there could be better and more > generic way than super device. > > And super device will make existing dtb broken but this time it might > be good opportunity to more generic way before more users use existing > dt. There has been a discussion ongoing about how to best represent display (and capture) stuff in DT using graphs, which you may wish to consider looking at. That discussion never reached any kind of conclusion before the merge window, but it does need to reach a conclusion for the next window. -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
[PATCH v2 1/7] drm/exynos: add super device support
On 05.04.2014 20:52, Russell King - ARM Linux wrote: > On Sat, Apr 05, 2014 at 08:31:15PM +0200, Tomasz Figa wrote: >> Maybe my words have been misinterpreted, but all I'm suggesting here is >> that there is no need to add any new data to DT to solve the same issue >> to the same extent as componentized subsystem framework, at least in >> Exynos case. > > Right, so we seem to have agreement that what exynos is currently doing > is fundamentally broken. > > So the next question is, how is it going to get fixed? > I believe this is a separate issue from what is being discussed in this thread, but anyway, I'd wait with this until Monday, so we could discuss this with Inki and Andrzej, who is the person responsible for Exynos DRM in my team. I was just passing by. ;) Best regards, Tomasz
[PATCH v2 1/7] drm/exynos: add super device support
On 05.04.2014 20:24, Russell King - ARM Linux wrote:
> On Sat, Apr 05, 2014 at 07:32:50PM +0200, Tomasz Figa wrote:
>> Not exactly. The approach we found does mostly the same as componentized
>> subsystem framework but without _any_ extra data in Device Tree. Just
>> based on the list of subsystem sub-drivers that is already available to
>> the master driver.
>
> The existing approach is fundamentally broken. Yes, your solution may
> work for the probing case, but have you tried unbinding any of your
> sub-drivers?
>
> From what I can see, that causes a kernel oops for one very simple reason -
> you destroy stuff while it's still in use. Let's look at an example:
>
> struct platform_driver ipp_driver = {
> .probe = ipp_probe,
> .remove = ipp_remove,
> .driver = {
> .name = "exynos-drm-ipp",
> .owner = THIS_MODULE,
> .pm = _pm_ops,
> },
> };
>
> static int ipp_remove(struct platform_device *pdev)
> {
> struct ipp_context *ctx = platform_get_drvdata(pdev);
>
> /* unregister sub driver */
> exynos_drm_subdrv_unregister(>subdrv);
>
> /* remove,destroy ipp idr */
> idr_destroy(>ipp_idr);
> idr_destroy(>prop_idr);
>
> mutex_destroy(>ipp_lock);
> mutex_destroy(>prop_lock);
>
> /* destroy command, event work queue */
> destroy_workqueue(ctx->cmd_workq);
> destroy_workqueue(ctx->event_workq);
>
> return 0;
> }
>
> int exynos_drm_subdrv_unregister(struct exynos_drm_subdrv *subdrv)
> {
> if (!subdrv)
> return -EINVAL;
>
> list_del(>list);
>
> return 0;
> }
>
> Oh dear, that destroys a whole pile of resources which could already
> be in use without telling anything that it's about to do that.
>
> I'm sure if I continue looking at the exynos stuff, it'll show similar
> crap all over the place.
>
> What you have now in mainline is not a solution. It's a crappy bodge.
>
Undoubtedly. Nobody here is trying to state the opposite.
Maybe my words have been misinterpreted, but all I'm suggesting here is
that there is no need to add any new data to DT to solve the same issue
to the same extent as componentized subsystem framework, at least in
Exynos case.
Best regards,
Tomasz
[PATCH v2 1/7] drm/exynos: add super device support
On Sat, Apr 05, 2014 at 08:31:15PM +0200, Tomasz Figa wrote: > Maybe my words have been misinterpreted, but all I'm suggesting here is > that there is no need to add any new data to DT to solve the same issue > to the same extent as componentized subsystem framework, at least in > Exynos case. Right, so we seem to have agreement that what exynos is currently doing is fundamentally broken. So the next question is, how is it going to get fixed? -- FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
[PATCH v2 1/7] drm/exynos: add super device support
[adding more people and MLs on Cc for further discussion]
On 04.04.2014 17:44, Inki Dae wrote:
> 2014-04-04 22:55 GMT+09:00 Tomasz Figa :
>> Hi Inki,
>>
>>
>> On 01.04.2014 14:37, Inki Dae wrote:
>>>
>>> This patch adds super device support to bind sub drivers
>>> using device tree.
>>>
>>> For this, you should add a super device node to each machine dt files
>>> like belows,
>>>
>>> In case of using MIPI-DSI,
>>> display-subsystem {
>>> compatible = "samsung,exynos-display-subsystem";
>>> ports = <>, <>;
>>> };
>>>
>>> In case of using DisplayPort,
>>> display-subsystem {
>>> compatible = "samsung,exynos-display-subsystem";
>>> ports = <>, <>;
>>> };
>>>
>>> In case of using Parallel panel,
>>> display-subsystem {
>>> compatible = "samsung,exynos-display-subsystem";
>>> ports = <>;
>>> };
>>>
>>> And if you don't add connector device node to ports property,
>>> default parallel panel driver, exynos_drm_dpi module, will be used.
>>>
>>> ports property can have the following device nodes,
>>> fimd, mixer, Image Enhancer, MIPI-DSI, eDP, LVDS Bridge, or HDMI
>>>
>>> With this patch, we can resolve the probing order issue without
>>> some global lists. So this patch also removes the unnecessary lists and
>>> stuff related to these lists.
>>
>>
>> I can see several problems with this approach:
>>
>> 1) It breaks compatibility with existing DT. After this patch it is no
>> longer possible to use old device trees and get a working DRM. However, in
>> my opinion, this requirement can be relaxed if we make sure that any users
>> are properly converted.
>>
>> 2) What happens if in Kconfig you disable a driver for a component that is
>
> I'm not sure what you meant but there wouldn't be no way that users
> *cannot disable* the driver for a component. The driver would be
> exynos_drm_drv, not separated module, and would always be built as
> long as users want to use *exynos drm driver*.
I think you don't understand what I mean. Let me show you an example:
You have a board with a DSI panel and also a HDMI output. So you have a
supernode pointing to FIMD, DSI and HDMI.
Now, an user finds that he doesn't need HDMI in his system, so he turns
off CONFIG_DRM_EXYNOS_HDMI. The supernode still points at hdmi node and
Exynos DRM core will register it as a component, but HDMI driver is not
available and will never probe, leading the whole Exynos DRM to never
initialize. Is this a desired behavior?
>
>> listed in supernode? If I'm reading the code correctly, Exynos DRM will not
>
> And the only case a component isn't added to the list is when users
> disabled sub driver.
See above.
The code creating the list of components to wait for
(exynos_drm_add_components()) doesn't seem to consider which sub-drivers
are actually enabled in kernel config.
>> register, which is completely wrong. Users should be able to select which
>> drivers should be compiled into their kernels.
>
> So users are be able to select drivers they want to use, and will be
> compiled correctly. So no, the only thing users can disable is each
> sub driver, not core module.
>
>>
>> 3) Such approach leads to complete integration of all Exynos DRM drivers,
>> without possibility of loading some sub-drivers as modules. I know that
>> current driver design doesn't support it either, but if this series is
>
> No, current drm driver *must also be built* as one integrated single
> drm driver without super device approach.
As I wrote, I know that current design before this patch isn't modular
either, but this is not my concern here. See below.
> So the super device approach
> *has no any effect on existing design*, and what the super device
> approch tries to do is to resolve the probe order issue to sub drivers
> *without some codes specific to Exynos drm*.
My concern is that the supernode design is actually carving such broken
non-modular design in stone. Remember that we are currently heading
towards a fully multi-platform kernel where it is critical for such
subsystems to be modular, because the same zImage is going to be running
on completely different machines.
>
>> claimed to improve things, it should really do so.
>>
>> 4) Exactly the same can be achieved without changing the DT bindings at all.
>> In fact even without adding any new single property or node to DT. We
>> discussed this with Andrzej and Marek today and came to a solution in which
>> just by adding a little bit of code to Exynos DRM subdrivers, you could
>> guarantee correct registration of Exynos DRM platform and also get rid of
>> #ifdeffery in exynos_drm_drv.c. Andrzej will send an RFC after the weekend.
>
> I'm not sure but I had implemented below prototype codes for that, see
> the below link,
>
[PATCH v2 1/7] drm/exynos: add super device support
On Sat, Apr 05, 2014 at 07:32:50PM +0200, Tomasz Figa wrote:
> Not exactly. The approach we found does mostly the same as componentized
> subsystem framework but without _any_ extra data in Device Tree. Just
> based on the list of subsystem sub-drivers that is already available to
> the master driver.
The existing approach is fundamentally broken. Yes, your solution may
work for the probing case, but have you tried unbinding any of your
sub-drivers?
>From what I can see, that causes a kernel oops for one very simple reason -
you destroy stuff while it's still in use. Let's look at an example:
struct platform_driver ipp_driver = {
.probe = ipp_probe,
.remove = ipp_remove,
.driver = {
.name = "exynos-drm-ipp",
.owner = THIS_MODULE,
.pm = _pm_ops,
},
};
static int ipp_remove(struct platform_device *pdev)
{
struct ipp_context *ctx = platform_get_drvdata(pdev);
/* unregister sub driver */
exynos_drm_subdrv_unregister(>subdrv);
/* remove,destroy ipp idr */
idr_destroy(>ipp_idr);
idr_destroy(>prop_idr);
mutex_destroy(>ipp_lock);
mutex_destroy(>prop_lock);
/* destroy command, event work queue */
destroy_workqueue(ctx->cmd_workq);
destroy_workqueue(ctx->event_workq);
return 0;
}
int exynos_drm_subdrv_unregister(struct exynos_drm_subdrv *subdrv)
{
if (!subdrv)
return -EINVAL;
list_del(>list);
return 0;
}
Oh dear, that destroys a whole pile of resources which could already
be in use without telling anything that it's about to do that.
I'm sure if I continue looking at the exynos stuff, it'll show similar
crap all over the place.
What you have now in mainline is not a solution. It's a crappy bodge.
--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
[PATCH v2 1/7] drm/exynos: add super device support
2014-04-04 22:55 GMT+09:00 Tomasz Figa :
> Hi Inki,
>
>
> On 01.04.2014 14:37, Inki Dae wrote:
>>
>> This patch adds super device support to bind sub drivers
>> using device tree.
>>
>> For this, you should add a super device node to each machine dt files
>> like belows,
>>
>> In case of using MIPI-DSI,
>> display-subsystem {
>> compatible = "samsung,exynos-display-subsystem";
>> ports = <>, <>;
>> };
>>
>> In case of using DisplayPort,
>> display-subsystem {
>> compatible = "samsung,exynos-display-subsystem";
>> ports = <>, <>;
>> };
>>
>> In case of using Parallel panel,
>> display-subsystem {
>> compatible = "samsung,exynos-display-subsystem";
>> ports = <>;
>> };
>>
>> And if you don't add connector device node to ports property,
>> default parallel panel driver, exynos_drm_dpi module, will be used.
>>
>> ports property can have the following device nodes,
>> fimd, mixer, Image Enhancer, MIPI-DSI, eDP, LVDS Bridge, or HDMI
>>
>> With this patch, we can resolve the probing order issue without
>> some global lists. So this patch also removes the unnecessary lists and
>> stuff related to these lists.
>
>
> I can see several problems with this approach:
>
> 1) It breaks compatibility with existing DT. After this patch it is no
> longer possible to use old device trees and get a working DRM. However, in
> my opinion, this requirement can be relaxed if we make sure that any users
> are properly converted.
>
> 2) What happens if in Kconfig you disable a driver for a component that is
I'm not sure what you meant but there wouldn't be no way that users
*cannot disable* the driver for a component. The driver would be
exynos_drm_drv, not separated module, and would always be built as
long as users want to use *exynos drm driver*.
> listed in supernode? If I'm reading the code correctly, Exynos DRM will not
And the only case a component isn't added to the list is when users
disabled sub driver.
> register, which is completely wrong. Users should be able to select which
> drivers should be compiled into their kernels.
So users are be able to select drivers they want to use, and will be
compiled correctly. So no, the only thing users can disable is each
sub driver, not core module.
>
> 3) Such approach leads to complete integration of all Exynos DRM drivers,
> without possibility of loading some sub-drivers as modules. I know that
> current driver design doesn't support it either, but if this series is
No, current drm driver *must also be built* as one integrated single
drm driver without super device approach. So the super device approach
*has no any effect on existing design*, and what the super device
approch tries to do is to resolve the probe order issue to sub drivers
*without some codes specific to Exynos drm*.
> claimed to improve things, it should really do so.
>
> 4) Exactly the same can be achieved without changing the DT bindings at all.
> In fact even without adding any new single property or node to DT. We
> discussed this with Andrzej and Marek today and came to a solution in which
> just by adding a little bit of code to Exynos DRM subdrivers, you could
> guarantee correct registration of Exynos DRM platform and also get rid of
> #ifdeffery in exynos_drm_drv.c. Andrzej will send an RFC after the weekend.
I'm not sure but I had implemented below prototype codes for that, see
the below link,
https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/commit/?h=exynos-bridge-test=2860ad02d736e300034ddeabce3da92614922b4e
I guess what you said would be similler approach.
And I still think the use of the component framework would be the best
solution and *Linux generic way* for resolving the probe order issue
without any specific codes. So I'm not advocating the compoent
framework but I tend not to want to use specific codes.
>
> 5) This series seems to break DPI display support with runtime PM enabled.
> Universal C210 just hangs on second FIMD probe, after first one fails with
> probe deferral. This needs more investigation, though.
For -next, I never expect that pm operations would be operated
perfactly. They are really not for product. Just adding new feature
and drivers to -next, and then fixing them while in RC. And RC process
would also be for it. Actually, I see pm interfaces of exynos_drm_dsi
driver leads to break a single driver model because pm operations
should be done at top level of exynos drm, and some codes Sean didn't
fix. So such things are in my fix-todo-list. I tend to accept new
features and drivers for -next as long as they have no big problem,
And that is my style I maintain.
Thanks,
Inki Dae
>
> Best regards,
> Tomasz
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/7] drm/exynos: add super device support
Hi Inki,
On 01.04.2014 14:37, Inki Dae wrote:
> This patch adds super device support to bind sub drivers
> using device tree.
>
> For this, you should add a super device node to each machine dt files
> like belows,
>
> In case of using MIPI-DSI,
> display-subsystem {
> compatible = "samsung,exynos-display-subsystem";
> ports = <>, <>;
> };
>
> In case of using DisplayPort,
> display-subsystem {
> compatible = "samsung,exynos-display-subsystem";
> ports = <>, <>;
> };
>
> In case of using Parallel panel,
> display-subsystem {
> compatible = "samsung,exynos-display-subsystem";
> ports = <>;
> };
>
> And if you don't add connector device node to ports property,
> default parallel panel driver, exynos_drm_dpi module, will be used.
>
> ports property can have the following device nodes,
> fimd, mixer, Image Enhancer, MIPI-DSI, eDP, LVDS Bridge, or HDMI
>
> With this patch, we can resolve the probing order issue without
> some global lists. So this patch also removes the unnecessary lists and
> stuff related to these lists.
I can see several problems with this approach:
1) It breaks compatibility with existing DT. After this patch it is no
longer possible to use old device trees and get a working DRM. However,
in my opinion, this requirement can be relaxed if we make sure that any
users are properly converted.
2) What happens if in Kconfig you disable a driver for a component that
is listed in supernode? If I'm reading the code correctly, Exynos DRM
will not register, which is completely wrong. Users should be able to
select which drivers should be compiled into their kernels.
3) Such approach leads to complete integration of all Exynos DRM
drivers, without possibility of loading some sub-drivers as modules. I
know that current driver design doesn't support it either, but if this
series is claimed to improve things, it should really do so.
4) Exactly the same can be achieved without changing the DT bindings at
all. In fact even without adding any new single property or node to DT.
We discussed this with Andrzej and Marek today and came to a solution in
which just by adding a little bit of code to Exynos DRM subdrivers, you
could guarantee correct registration of Exynos DRM platform and also get
rid of #ifdeffery in exynos_drm_drv.c. Andrzej will send an RFC after
the weekend.
5) This series seems to break DPI display support with runtime PM
enabled. Universal C210 just hangs on second FIMD probe, after first one
fails with probe deferral. This needs more investigation, though.
Best regards,
Tomasz
[PATCH v2 1/7] drm/exynos: add super device support
2014-04-02 23:06 GMT+09:00, Andrzej Hajda :
> On 04/01/2014 02:37 PM, Inki Dae wrote:
>> This patch adds super device support to bind sub drivers
>> using device tree.
>>
>> For this, you should add a super device node to each machine dt files
>> like belows,
>>
>> In case of using MIPI-DSI,
>> display-subsystem {
>> compatible = "samsung,exynos-display-subsystem";
>> ports = <>, <>;
>> };
>>
>> In case of using DisplayPort,
>> display-subsystem {
>> compatible = "samsung,exynos-display-subsystem";
>> ports = <>, <>;
>> };
>>
>> In case of using Parallel panel,
>> display-subsystem {
>> compatible = "samsung,exynos-display-subsystem";
>> ports = <>;
>> };
>>
>> And if you don't add connector device node to ports property,
>> default parallel panel driver, exynos_drm_dpi module, will be used.
>>
>> ports property can have the following device nodes,
>> fimd, mixer, Image Enhancer, MIPI-DSI, eDP, LVDS Bridge, or HDMI
>>
>> With this patch, we can resolve the probing order issue without
>> some global lists. So this patch also removes the unnecessary lists and
>> stuff related to these lists.
>>
>
> (...)
>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> index 40fd6cc..7ebfe15 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
>> @@ -19,10 +19,12 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include
>> #include
>> #include
>> +#include
>> #include
>>
>> #include "exynos_drm_drv.h"
>> @@ -144,12 +146,14 @@ static inline struct fimd_driver_data
>> *drm_fimd_get_driver_data(
>> }
>>
>> static int fimd_mgr_initialize(struct exynos_drm_manager *mgr,
>> -struct drm_device *drm_dev, int pipe)
>> +struct drm_device *drm_dev)
>> {
>> struct fimd_context *ctx = mgr->ctx;
>> +struct exynos_drm_private *priv;
>> +priv = drm_dev->dev_private;
>>
>> -ctx->drm_dev = drm_dev;
>> -ctx->pipe = pipe;
>> +mgr->drm_dev = ctx->drm_dev = drm_dev;
>> +mgr->pipe = ctx->pipe = priv->pipe++;
>>
>> /*
>> * enable drm irq mode.
>> @@ -803,8 +807,6 @@ static void fimd_dpms(struct exynos_drm_manager *mgr,
>> int mode)
>> }
>>
>> static struct exynos_drm_manager_ops fimd_manager_ops = {
>> -.initialize = fimd_mgr_initialize,
>> -.remove = fimd_mgr_remove,
>> .dpms = fimd_dpms,
>> .mode_fixup = fimd_mode_fixup,
>> .mode_set = fimd_mode_set,
>> @@ -849,9 +851,10 @@ out:
>> return IRQ_HANDLED;
>> }
>>
>> -static int fimd_probe(struct platform_device *pdev)
>> +static int fimd_bind(struct device *dev, struct device *master, void
>> *data)
>> {
>> -struct device *dev = >dev;
>> +struct platform_device *pdev = to_platform_device(dev);
>> +struct drm_device *drm_dev = data;
>> struct fimd_context *ctx;
>> struct resource *res;
>> int win;
>> @@ -910,11 +913,16 @@ static int fimd_probe(struct platform_device *pdev)
>> platform_set_drvdata(pdev, _manager);
>>
>> fimd_manager.ctx = ctx;
>> -exynos_drm_manager_register(_manager);
>> +fimd_mgr_initialize(_manager, drm_dev);
>>
>> -exynos_dpi_probe(ctx->dev);
>> +exynos_drm_crtc_create(_manager);
>>
>> -pm_runtime_enable(dev);
>> +/*
>> + * It should be called after exynos_drm_crtc_create call because
>> + * exynos_dpi_probe call will try to find same lcd type
>> + * of manager to setup possible_crtcs.
>> + */
>> +exynos_dpi_probe(drm_dev, dev);
>>
>> for (win = 0; win < WINDOWS_NR; win++)
>> fimd_clear_win(ctx, win);
>> @@ -922,18 +930,56 @@ static int fimd_probe(struct platform_device *pdev)
>> return 0;
>> }
>>
>> -static int fimd_remove(struct platform_device *pdev)
>> +static void fimd_unbind(struct device *dev, struct device *master,
>> +void *data)
>> {
>> -struct exynos_drm_manager *mgr = platform_get_drvdata(pdev);
>> +struct exynos_drm_manager *mgr = dev_get_drvdata(dev);
>> +struct drm_crtc *crtc = mgr->crtc;
>> +
>> +fimd_dpms(mgr, DRM_MODE_DPMS_OFF);
>>
>> -exynos_dpi_remove(>dev);
>> +exynos_dpi_remove(mgr->drm_dev, dev);
>>
>> -exynos_drm_manager_unregister(_manager);
>> +fimd_mgr_remove(mgr);
>>
>> -fimd_dpms(mgr, DRM_MODE_DPMS_OFF);
>> +crtc->funcs->destroy(crtc);
>> +}
>> +
>> +static const struct component_ops fimd_component_ops = {
>> +.bind = fimd_bind,
>> +.unbind = fimd_unbind,
>> +};
>>
>> +static int fimd_probe(struct platform_device *pdev)
>> +{
>> +struct device_node *dn;
>> +
>> +/* Check if fimd node has port node. */
>> +dn = exynos_dpi_of_find_panel_node(>dev);
>> +if (dn) {
>> +struct drm_panel *panel;
>> +
>> +/*
>> + * Do not bind if there is the port node but a drm_panel
>> +
[PATCH v2 1/7] drm/exynos: add super device support
On 04/01/2014 02:37 PM, Inki Dae wrote:
> This patch adds super device support to bind sub drivers
> using device tree.
>
> For this, you should add a super device node to each machine dt files
> like belows,
>
> In case of using MIPI-DSI,
> display-subsystem {
> compatible = "samsung,exynos-display-subsystem";
> ports = <>, <>;
> };
>
> In case of using DisplayPort,
> display-subsystem {
> compatible = "samsung,exynos-display-subsystem";
> ports = <>, <>;
> };
>
> In case of using Parallel panel,
> display-subsystem {
> compatible = "samsung,exynos-display-subsystem";
> ports = <>;
> };
>
> And if you don't add connector device node to ports property,
> default parallel panel driver, exynos_drm_dpi module, will be used.
>
> ports property can have the following device nodes,
> fimd, mixer, Image Enhancer, MIPI-DSI, eDP, LVDS Bridge, or HDMI
>
> With this patch, we can resolve the probing order issue without
> some global lists. So this patch also removes the unnecessary lists and
> stuff related to these lists.
>
(...)
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 40fd6cc..7ebfe15 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -19,10 +19,12 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> #include
> +#include
> #include
>
> #include "exynos_drm_drv.h"
> @@ -144,12 +146,14 @@ static inline struct fimd_driver_data
> *drm_fimd_get_driver_data(
> }
>
> static int fimd_mgr_initialize(struct exynos_drm_manager *mgr,
> - struct drm_device *drm_dev, int pipe)
> + struct drm_device *drm_dev)
> {
> struct fimd_context *ctx = mgr->ctx;
> + struct exynos_drm_private *priv;
> + priv = drm_dev->dev_private;
>
> - ctx->drm_dev = drm_dev;
> - ctx->pipe = pipe;
> + mgr->drm_dev = ctx->drm_dev = drm_dev;
> + mgr->pipe = ctx->pipe = priv->pipe++;
>
> /*
>* enable drm irq mode.
> @@ -803,8 +807,6 @@ static void fimd_dpms(struct exynos_drm_manager *mgr, int
> mode)
> }
>
> static struct exynos_drm_manager_ops fimd_manager_ops = {
> - .initialize = fimd_mgr_initialize,
> - .remove = fimd_mgr_remove,
> .dpms = fimd_dpms,
> .mode_fixup = fimd_mode_fixup,
> .mode_set = fimd_mode_set,
> @@ -849,9 +851,10 @@ out:
> return IRQ_HANDLED;
> }
>
> -static int fimd_probe(struct platform_device *pdev)
> +static int fimd_bind(struct device *dev, struct device *master, void *data)
> {
> - struct device *dev = >dev;
> + struct platform_device *pdev = to_platform_device(dev);
> + struct drm_device *drm_dev = data;
> struct fimd_context *ctx;
> struct resource *res;
> int win;
> @@ -910,11 +913,16 @@ static int fimd_probe(struct platform_device *pdev)
> platform_set_drvdata(pdev, _manager);
>
> fimd_manager.ctx = ctx;
> - exynos_drm_manager_register(_manager);
> + fimd_mgr_initialize(_manager, drm_dev);
>
> - exynos_dpi_probe(ctx->dev);
> + exynos_drm_crtc_create(_manager);
>
> - pm_runtime_enable(dev);
> + /*
> + * It should be called after exynos_drm_crtc_create call because
> + * exynos_dpi_probe call will try to find same lcd type
> + * of manager to setup possible_crtcs.
> + */
> + exynos_dpi_probe(drm_dev, dev);
>
> for (win = 0; win < WINDOWS_NR; win++)
> fimd_clear_win(ctx, win);
> @@ -922,18 +930,56 @@ static int fimd_probe(struct platform_device *pdev)
> return 0;
> }
>
> -static int fimd_remove(struct platform_device *pdev)
> +static void fimd_unbind(struct device *dev, struct device *master,
> + void *data)
> {
> - struct exynos_drm_manager *mgr = platform_get_drvdata(pdev);
> + struct exynos_drm_manager *mgr = dev_get_drvdata(dev);
> + struct drm_crtc *crtc = mgr->crtc;
> +
> + fimd_dpms(mgr, DRM_MODE_DPMS_OFF);
>
> - exynos_dpi_remove(>dev);
> + exynos_dpi_remove(mgr->drm_dev, dev);
>
> - exynos_drm_manager_unregister(_manager);
> + fimd_mgr_remove(mgr);
>
> - fimd_dpms(mgr, DRM_MODE_DPMS_OFF);
> + crtc->funcs->destroy(crtc);
> +}
> +
> +static const struct component_ops fimd_component_ops = {
> + .bind = fimd_bind,
> + .unbind = fimd_unbind,
> +};
>
> +static int fimd_probe(struct platform_device *pdev)
> +{
> + struct device_node *dn;
> +
> + /* Check if fimd node has port node. */
> + dn = exynos_dpi_of_find_panel_node(>dev);
> + if (dn) {
> + struct drm_panel *panel;
> +
> + /*
> + * Do not bind if there is the port node but a drm_panel
> + * isn't added to panel_list yet.
> + * In this case, fimd_probe will
[PATCH v2 1/7] drm/exynos: add super device support
This patch adds super device support to bind sub drivers
using device tree.
For this, you should add a super device node to each machine dt files
like belows,
In case of using MIPI-DSI,
display-subsystem {
compatible = "samsung,exynos-display-subsystem";
ports = <>, <>;
};
In case of using DisplayPort,
display-subsystem {
compatible = "samsung,exynos-display-subsystem";
ports = <>, <>;
};
In case of using Parallel panel,
display-subsystem {
compatible = "samsung,exynos-display-subsystem";
ports = <>;
};
And if you don't add connector device node to ports property,
default parallel panel driver, exynos_drm_dpi module, will be used.
ports property can have the following device nodes,
fimd, mixer, Image Enhancer, MIPI-DSI, eDP, LVDS Bridge, or HDMI
With this patch, we can resolve the probing order issue without
some global lists. So this patch also removes the unnecessary lists and
stuff related to these lists.
Previous RFC patch,
http://www.spinics.net/lists/dri-devel/msg54671.html
Changelog since RFC patch:
- Register sub drivers and probe them at load(). In case of non sub
drivers, sub driver probe is needed.
- Enable runtime pm at fimd_probe() instead of fimd_bind(). runtime pm
should be enabled before iommu device is attached to fimd device.
- Do not return an error with component_master_add fail.
- Remove super device support from mipi dsi driver which was in RFC.
- Add super device support to parallel driver.
Changelog v2:
- Add super device support to mipi dsi driver.
- Bind fimd driver only in case that a drm_panel for parallel panel driver
is added to panel_list of drm_panel module.
- Change super node name to 'display-subsystem'
. 'exynos-drm' is specific to Linux so change it to generic name.
- Change propery name of super node to 'ports'
. crtcs and connectors propery names are also specific to Linux so
change them to generic name.
Signed-off-by: Inki Dae
Signed-off-by: Kyungmin Park
---
drivers/gpu/drm/exynos/exynos_dp_core.c | 45 ---
drivers/gpu/drm/exynos/exynos_drm_core.c | 216 +-
drivers/gpu/drm/exynos/exynos_drm_crtc.c | 17 +++
drivers/gpu/drm/exynos/exynos_drm_crtc.h |4 +
drivers/gpu/drm/exynos/exynos_drm_dpi.c | 16 ++-
drivers/gpu/drm/exynos/exynos_drm_drv.c | 202 +++-
drivers/gpu/drm/exynos/exynos_drm_drv.h | 73 +-
drivers/gpu/drm/exynos/exynos_drm_dsi.c | 38 +-
drivers/gpu/drm/exynos/exynos_drm_fimd.c | 76 ---
drivers/gpu/drm/exynos/exynos_drm_vidi.c | 61 +++--
drivers/gpu/drm/exynos/exynos_hdmi.c | 45 ---
drivers/gpu/drm/exynos/exynos_mixer.c| 54 ++--
12 files changed, 442 insertions(+), 405 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
b/drivers/gpu/drm/exynos/exynos_dp_core.c
index a59bca9..cb9aa58 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -18,6 +18,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -969,16 +970,6 @@ static struct drm_connector_helper_funcs
exynos_dp_connector_helper_funcs = {
.best_encoder = exynos_dp_best_encoder,
};
-static int exynos_dp_initialize(struct exynos_drm_display *display,
- struct drm_device *drm_dev)
-{
- struct exynos_dp_device *dp = display->ctx;
-
- dp->drm_dev = drm_dev;
-
- return 0;
-}
-
static bool find_bridge(const char *compat, struct bridge_init *bridge)
{
bridge->client = NULL;
@@ -1106,7 +1097,6 @@ static void exynos_dp_dpms(struct exynos_drm_display
*display, int mode)
}
static struct exynos_drm_display_ops exynos_dp_display_ops = {
- .initialize = exynos_dp_initialize,
.create_connector = exynos_dp_create_connector,
.dpms = exynos_dp_dpms,
};
@@ -1230,8 +1220,10 @@ static int exynos_dp_dt_parse_panel(struct
exynos_dp_device *dp)
return 0;
}
-static int exynos_dp_probe(struct platform_device *pdev)
+static int exynos_dp_bind(struct device *dev, struct device *master, void
*data)
{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct drm_device *drm_dev = data;
struct resource *res;
struct exynos_dp_device *dp;
@@ -1293,21 +1285,40 @@ static int exynos_dp_probe(struct platform_device *pdev)
}
disable_irq(dp->irq);
+ dp->drm_dev = drm_dev;
exynos_dp_display.ctx = dp;
platform_set_drvdata(pdev, _dp_display);
- exynos_drm_display_register(_dp_display);
- return 0;
+ return exynos_drm_create_enc_conn(drm_dev, _dp_display);
}
-static int exynos_dp_remove(struct platform_device *pdev)
+static void exynos_dp_unbind(struct device *dev, struct device *master,
+ void *data)
{
