From: Sui Jingfeng <[email protected]>
There is a Vivante GC1000 (v5037) in LS2K1000 and LS7A1000, this GPU is a
PCI device, and it has 2D and 3D cores in the same core. This series is
trying to add PCI device driver support to drm/etnaviv.
v6:
* Fix build issue on system without CONFIG_PCI enabled
v7:
* Add a separate patch for the platform driver rearrangement (Bjorn)
* Switch to runtime check if the GPU is dma coherent or not (Lucas)
* Add ETNAVIV_PARAM_GPU_COHERENT to allow userspace to query (Lucas)
* Remove etnaviv_gpu.no_clk member (Lucas)
* Various Typos and coding style fixed (Bjorn)
v8:
* Fix typos and remove unnecessary header included (Bjorn).
* Add a dedicated function to create the virtual master platform
device.
v9:
* Use PCI_VDEVICE() macro (Bjorn)
* Add trivial stubs for the PCI driver (Bjorn)
* Remove a redundant dev_err() usage (Bjorn)
* Clean up etnaviv_pdev_probe() with etnaviv_of_first_available_node()
v10:
* Add one more cleanup patch
* Resolve the conflict with a patch from Rob
* Make the dummy PCI stub inlined
* Print only if the platform is dma-coherrent
Rob Herring (1):
drm/etnaviv: Replace of_platform.h with explicit includes
Sui Jingfeng (10):
drm/etnaviv: Add a dedicated function to register an irq handler
drm/etnaviv: Add a dedicated function to get various clocks
drm/etnaviv: Add dedicated functions to create and destroy platform
device
drm/etnaviv: Add helpers for private data construction and destruction
drm/etnaviv: Allow bypass component framework
drm/etnaviv: Add driver support for the PCI devices
drm/etnaviv: Add support for the dma coherent device
drm/etnaviv: Add a dedicated function to create the virtual master
drm/etnaviv: Clean up etnaviv_pdev_probe() function
drm/etnaviv: Keep the curly brace aligned
drivers/gpu/drm/etnaviv/Kconfig | 10 +
drivers/gpu/drm/etnaviv/Makefile | 2 +
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 322 ++++++++++++++------
drivers/gpu/drm/etnaviv/etnaviv_drv.h | 10 +
drivers/gpu/drm/etnaviv/etnaviv_gem.c | 22 +-
drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 7 +-
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 166 ++++++----
drivers/gpu/drm/etnaviv/etnaviv_gpu.h | 9 +
drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c | 75 +++++
drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h | 18 ++
include/uapi/drm/etnaviv_drm.h | 1 +
11 files changed, 481 insertions(+), 161 deletions(-)
create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.c
create mode 100644 drivers/gpu/drm/etnaviv/etnaviv_pci_drv.h
--
2.25.1
From: Sui Jingfeng <[email protected]>
Also rename the virtual master platform device as etnaviv_platform_device,
for better reflection that it is a platform device, not a DRM device.
Another benefit is that we no longer need to call of_node_put() for three
different cases, Instead, we only need to call it once.
Cc: Lucas Stach <[email protected]>
Cc: Christian Gmeiner <[email protected]>
Cc: Philipp Zabel <[email protected]>
Cc: Bjorn Helgaas <[email protected]>
Cc: Daniel Vetter <[email protected]>
Signed-off-by: Sui Jingfeng <[email protected]>
---
drivers/gpu/drm/etnaviv/etnaviv_drv.c | 56 +++++++++++++++++++--------
1 file changed, 39 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
index 31a7f59ccb49..cec005035d0e 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
@@ -656,12 +656,44 @@ static struct platform_driver etnaviv_platform_driver = {
},
};
-static struct platform_device *etnaviv_drm;
+static struct platform_device *etnaviv_platform_device;
-static int __init etnaviv_init(void)
+static int etnaviv_create_platform_device(const char *name,
+ struct platform_device **ppdev)
{
struct platform_device *pdev;
int ret;
+
+ pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE);
+ if (!pdev)
+ return -ENOMEM;
+
+ ret = platform_device_add(pdev);
+ if (ret) {
+ platform_device_put(pdev);
+ return ret;
+ }
+
+ *ppdev = pdev;
+
+ return 0;
+}
+
+static void etnaviv_destroy_platform_device(struct platform_device **ppdev)
+{
+ struct platform_device *pdev = *ppdev;
+
+ if (!pdev)
+ return;
+
+ platform_device_unregister(pdev);
+
+ *ppdev = NULL;
+}
+
+static int __init etnaviv_init(void)
+{
+ int ret;
struct device_node *np;
etnaviv_validate_init();
@@ -681,23 +713,13 @@ static int __init etnaviv_init(void)
for_each_compatible_node(np, NULL, "vivante,gc") {
if (!of_device_is_available(np))
continue;
+ of_node_put(np);
- pdev = platform_device_alloc("etnaviv", PLATFORM_DEVID_NONE);
- if (!pdev) {
- ret = -ENOMEM;
- of_node_put(np);
- goto unregister_platform_driver;
- }
-
- ret = platform_device_add(pdev);
- if (ret) {
- platform_device_put(pdev);
- of_node_put(np);
+ ret = etnaviv_create_platform_device("etnaviv",
+ &etnaviv_platform_device);
+ if (ret)
goto unregister_platform_driver;
- }
- etnaviv_drm = pdev;
- of_node_put(np);
break;
}
@@ -713,7 +735,7 @@ module_init(etnaviv_init);
static void __exit etnaviv_exit(void)
{
- platform_device_unregister(etnaviv_drm);
+ etnaviv_destroy_platform_device(&etnaviv_platform_device);
platform_driver_unregister(&etnaviv_platform_driver);
platform_driver_unregister(&etnaviv_gpu_driver);
}
--
2.25.1
Hi
On 2023/6/21 15:55, Christian Gmeiner wrote:
> Hi
>
>> From: Sui Jingfeng <[email protected]>
>>
>> There is a Vivante GC1000 (v5037) in LS2K1000 and LS7A1000, this GPU is a
>> PCI device, and it has 2D and 3D cores in the same core. This series is
>> trying to add PCI device driver support to drm/etnaviv.
>>
> Is it possible to get the lspci output for the GPU? Something like
> this: sudo lspci -vvv -s ...
Yes,
sudo lspci -vvvxxx -s 00:06.0
00:06.0 Multimedia video controller: Loongson Technology LLC Vivante GPU
(Graphics Processing Unit) (rev 01)
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop+ ParErr-
Stepping- SERR- FastB2B- DisINTx-
Status: Cap- 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Interrupt: pin A routed to IRQ 51
NUMA node: 0
Region 0: Memory at e0035200000 (64-bit, non-prefetchable) [size=256K]
Region 2: Memory at e0030000000 (64-bit, non-prefetchable) [size=64M]
Region 4: Memory at e0035240000 (64-bit, non-prefetchable) [size=64K]
Kernel driver in use: etnaviv
Kernel modules: etnaviv
00: 14 00 15 7a 27 00 00 00 01 00 00 04 00 00 80 00
10: 04 00 20 35 00 00 00 00 04 00 00 30 00 00 00 00
20: 04 00 24 35 00 00 00 00 00 00 00 00 00 00 00 00
30: 00 00 00 00 00 00 00 00 00 00 00 00 5d 01 00 00
40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> thanks
> --
> Christian Gmeiner, MSc
>
> https://christian-gmeiner.info/privacypolicy
--
Jingfeng
Hi,
Below is the gpu info cat from the debugfs,
I guess this is also what you want ?
[root@fedora 0]# cat gpu
0000:00:06.0 Status:
identity
model: 0x1000
revision: 0x5037
product_id: 0x0
customer_id: 0x0
eco_id: 0x0
features
major_features: 0xe0286eed
minor_features0: 0xe9799eff
minor_features1: 0xbe13b2d9
minor_features2: 0xca114080
minor_features3: 0x0e0100a1
minor_features4: 0x00000000
minor_features5: 0x00000000
minor_features6: 0x00000000
minor_features7: 0x00000000
minor_features8: 0x00000000
minor_features9: 0x00000000
minor_features10: 0x00000000
minor_features11: 0x00000000
specs
stream_count: 4
register_max: 64
thread_count: 512
vertex_cache_size: 8
shader_core_count: 2
nn_core_count: 0
pixel_pipes: 1
vertex_output_buffer_size: 512
buffer_size: 0
instruction_count: 256
num_constants: 576
varyings_count: 8
axi: 0x00000051
idle: 0x7ffffffe
FE is not idle
DMA is running
address 0: 0x00002ac0
address 1: 0x00002ac8
state 0: 0x00000800
state 1: 0x00000812
last fetch 64 bit word: 0x380000c8 0x00000701
On 2023/6/21 15:55, Christian Gmeiner wrote:
> Hi
>
>> From: Sui Jingfeng <[email protected]>
>>
>> There is a Vivante GC1000 (v5037) in LS2K1000 and LS7A1000, this GPU is a
>> PCI device, and it has 2D and 3D cores in the same core. This series is
>> trying to add PCI device driver support to drm/etnaviv.
>>
> Is it possible to get the lspci output for the GPU? Something like
> this: sudo lspci -vvv -s ...
>
>
> thanks
> --
> Christian Gmeiner, MSc
>
> https://christian-gmeiner.info/privacypolicy
--
Jingfeng
Hi
>
> From: Sui Jingfeng <[email protected]>
>
> There is a Vivante GC1000 (v5037) in LS2K1000 and LS7A1000, this GPU is a
> PCI device, and it has 2D and 3D cores in the same core. This series is
> trying to add PCI device driver support to drm/etnaviv.
>
Is it possible to get the lspci output for the GPU? Something like
this: sudo lspci -vvv -s ...
thanks
--
Christian Gmeiner, MSc
https://christian-gmeiner.info/privacypolicy
Am Dienstag, dem 20.06.2023 um 17:47 +0800 schrieb Sui Jingfeng:
> From: Sui Jingfeng <[email protected]>
>
> Also rename the virtual master platform device as etnaviv_platform_device,
> for better reflection that it is a platform device, not a DRM device.
>
> Another benefit is that we no longer need to call of_node_put() for three
> different cases, Instead, we only need to call it once.
>
> Cc: Lucas Stach <[email protected]>
> Cc: Christian Gmeiner <[email protected]>
> Cc: Philipp Zabel <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Signed-off-by: Sui Jingfeng <[email protected]>
> ---
> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 56 +++++++++++++++++++--------
> 1 file changed, 39 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index 31a7f59ccb49..cec005035d0e 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -656,12 +656,44 @@ static struct platform_driver etnaviv_platform_driver = {
> },
> };
>
> -static struct platform_device *etnaviv_drm;
> +static struct platform_device *etnaviv_platform_device;
>
> -static int __init etnaviv_init(void)
> +static int etnaviv_create_platform_device(const char *name,
> + struct platform_device **ppdev)
As the platform device is a global static variable, there is no need to
push it through the parameters of this function. Just use the global
variable directly in this function.
> {
> struct platform_device *pdev;
> int ret;
> +
> + pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE);
> + if (!pdev)
> + return -ENOMEM;
> +
> + ret = platform_device_add(pdev);
> + if (ret) {
> + platform_device_put(pdev);
> + return ret;
> + }
> +
> + *ppdev = pdev;
> +
> + return 0;
> +}
> +
> +static void etnaviv_destroy_platform_device(struct platform_device **ppdev)
> +{
> + struct platform_device *pdev = *ppdev;
Same here, just use the global variable directly.
Regards,
Lucas
> +
> + if (!pdev)
> + return;
> +
> + platform_device_unregister(pdev);
> +
> + *ppdev = NULL;
> +}
> +
> +static int __init etnaviv_init(void)
> +{
> + int ret;
> struct device_node *np;
>
> etnaviv_validate_init();
> @@ -681,23 +713,13 @@ static int __init etnaviv_init(void)
> for_each_compatible_node(np, NULL, "vivante,gc") {
> if (!of_device_is_available(np))
> continue;
> + of_node_put(np);
>
> - pdev = platform_device_alloc("etnaviv", PLATFORM_DEVID_NONE);
> - if (!pdev) {
> - ret = -ENOMEM;
> - of_node_put(np);
> - goto unregister_platform_driver;
> - }
> -
> - ret = platform_device_add(pdev);
> - if (ret) {
> - platform_device_put(pdev);
> - of_node_put(np);
> + ret = etnaviv_create_platform_device("etnaviv",
> + &etnaviv_platform_device);
> + if (ret)
> goto unregister_platform_driver;
> - }
>
> - etnaviv_drm = pdev;
> - of_node_put(np);
> break;
> }
>
> @@ -713,7 +735,7 @@ module_init(etnaviv_init);
>
> static void __exit etnaviv_exit(void)
> {
> - platform_device_unregister(etnaviv_drm);
> + etnaviv_destroy_platform_device(&etnaviv_platform_device);
> platform_driver_unregister(&etnaviv_platform_driver);
> platform_driver_unregister(&etnaviv_gpu_driver);
> }
Hi
On 2023/6/21 17:15, Lucas Stach wrote:
> Am Dienstag, dem 20.06.2023 um 17:47 +0800 schrieb Sui Jingfeng:
>> From: Sui Jingfeng <[email protected]>
>>
>> Also rename the virtual master platform device as etnaviv_platform_device,
>> for better reflection that it is a platform device, not a DRM device.
>>
>> Another benefit is that we no longer need to call of_node_put() for three
>> different cases, Instead, we only need to call it once.
>>
>> Cc: Lucas Stach <[email protected]>
>> Cc: Christian Gmeiner <[email protected]>
>> Cc: Philipp Zabel <[email protected]>
>> Cc: Bjorn Helgaas <[email protected]>
>> Cc: Daniel Vetter <[email protected]>
>> Signed-off-by: Sui Jingfeng <[email protected]>
>> ---
>> drivers/gpu/drm/etnaviv/etnaviv_drv.c | 56 +++++++++++++++++++--------
>> 1 file changed, 39 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> index 31a7f59ccb49..cec005035d0e 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>> @@ -656,12 +656,44 @@ static struct platform_driver etnaviv_platform_driver = {
>> },
>> };
>>
>> -static struct platform_device *etnaviv_drm;
>> +static struct platform_device *etnaviv_platform_device;
>>
>> -static int __init etnaviv_init(void)
>> +static int etnaviv_create_platform_device(const char *name,
>> + struct platform_device **ppdev)
> As the platform device is a global static variable, there is no need to
> push it through the parameters of this function. Just use the global
> variable directly in this function.
A function reference a global static variable is *NOT* a *pure* fucntion,
it degenerate as a procedure,
The function is perfect in the sense that it does not reference any
global variable.
etnaviv_create_platform_device() is NOT intended to used by one function,
a specific purpose only, but when create this function, I want to create other
platform device with this function.
Say, You want to create a dummy platform device, targeting to bind to the real master
(the single GPU core) . To verify the idea that we choose the first 3D gpu core as master,
other 2D or VG gpu core is not as important as the 3D one.
The should bind to the 3D GPU core (master).
While back to the question you ask, I want etnaviv_create_platform_device() to be generic,
can be used by multiple place for multiple purpose.
I have successfully copy this to a another drm driver by simply renaming.
The body of the function itself does not need to change.
>> {
>> struct platform_device *pdev;
>> int ret;
>> +
>> + pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE);
>> + if (!pdev)
>> + return -ENOMEM;
>> +
>> + ret = platform_device_add(pdev);
>> + if (ret) {
>> + platform_device_put(pdev);
>> + return ret;
>> + }
>> +
>> + *ppdev = pdev;
>> +
>> + return 0;
>> +}
>> +
>> +static void etnaviv_destroy_platform_device(struct platform_device **ppdev)
>> +{
>> + struct platform_device *pdev = *ppdev;
> Same here, just use the global variable directly.
>
> Regards,
> Lucas
>
>> +
>> + if (!pdev)
>> + return;
>> +
>> + platform_device_unregister(pdev);
>> +
>> + *ppdev = NULL;
>> +}
>> +
>> +static int __init etnaviv_init(void)
>> +{
>> + int ret;
>> struct device_node *np;
>>
>> etnaviv_validate_init();
>> @@ -681,23 +713,13 @@ static int __init etnaviv_init(void)
>> for_each_compatible_node(np, NULL, "vivante,gc") {
>> if (!of_device_is_available(np))
>> continue;
>> + of_node_put(np);
>>
>> - pdev = platform_device_alloc("etnaviv", PLATFORM_DEVID_NONE);
>> - if (!pdev) {
>> - ret = -ENOMEM;
>> - of_node_put(np);
>> - goto unregister_platform_driver;
>> - }
>> -
>> - ret = platform_device_add(pdev);
>> - if (ret) {
>> - platform_device_put(pdev);
>> - of_node_put(np);
>> + ret = etnaviv_create_platform_device("etnaviv",
>> + &etnaviv_platform_device);
>> + if (ret)
>> goto unregister_platform_driver;
>> - }
>>
>> - etnaviv_drm = pdev;
>> - of_node_put(np);
>> break;
>> }
>>
>> @@ -713,7 +735,7 @@ module_init(etnaviv_init);
>>
>> static void __exit etnaviv_exit(void)
>> {
>> - platform_device_unregister(etnaviv_drm);
>> + etnaviv_destroy_platform_device(&etnaviv_platform_device);
>> platform_driver_unregister(&etnaviv_platform_driver);
>> platform_driver_unregister(&etnaviv_gpu_driver);
>> }
--
Jingfeng
Am Mittwoch, dem 21.06.2023 um 17:49 +0800 schrieb Sui Jingfeng:
> Hi
>
> On 2023/6/21 17:15, Lucas Stach wrote:
> > Am Dienstag, dem 20.06.2023 um 17:47 +0800 schrieb Sui Jingfeng:
> > > From: Sui Jingfeng <[email protected]>
> > >
> > > Also rename the virtual master platform device as etnaviv_platform_device,
> > > for better reflection that it is a platform device, not a DRM device.
> > >
> > > Another benefit is that we no longer need to call of_node_put() for three
> > > different cases, Instead, we only need to call it once.
> > >
> > > Cc: Lucas Stach <[email protected]>
> > > Cc: Christian Gmeiner <[email protected]>
> > > Cc: Philipp Zabel <[email protected]>
> > > Cc: Bjorn Helgaas <[email protected]>
> > > Cc: Daniel Vetter <[email protected]>
> > > Signed-off-by: Sui Jingfeng <[email protected]>
> > > ---
> > > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 56 +++++++++++++++++++--------
> > > 1 file changed, 39 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > > index 31a7f59ccb49..cec005035d0e 100644
> > > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > > @@ -656,12 +656,44 @@ static struct platform_driver etnaviv_platform_driver = {
> > > },
> > > };
> > >
> > > -static struct platform_device *etnaviv_drm;
> > > +static struct platform_device *etnaviv_platform_device;
> > >
> > > -static int __init etnaviv_init(void)
> > > +static int etnaviv_create_platform_device(const char *name,
> > > + struct platform_device **ppdev)
> > As the platform device is a global static variable, there is no need to
> > push it through the parameters of this function. Just use the global
> > variable directly in this function.
>
> A function reference a global static variable is *NOT* a *pure* fucntion,
>
That's right, but all you do with those indirections through the
parameter list is move which of the functions is non-pure, in your case
it's etnaviv_init/etnaviv_exit, with the indirection dropped it's
etnaviv_create_platform_device/etnaviv_destroy_platform_device.
> it degenerate as a procedure,
>
>
> The function is perfect in the sense that it does not reference any
> global variable.
>
>
> etnaviv_create_platform_device() is NOT intended to used by one function,
>
> a specific purpose only, but when create this function, I want to create other
>
> platform device with this function.
>
> Say, You want to create a dummy platform device, targeting to bind to the real master
>
> (the single GPU core) . To verify the idea that we choose the first 3D gpu core as master,
>
> other 2D or VG gpu core is not as important as the 3D one.
>
> The should bind to the 3D GPU core (master).
>
Sorry, I'm not following what you are trying to tell me here. Could you
please rephrase?
>
> While back to the question you ask, I want etnaviv_create_platform_device() to be generic,
>
> can be used by multiple place for multiple purpose.
>
> I have successfully copy this to a another drm driver by simply renaming.
>
> The body of the function itself does not need to change.
But it isn't shared, in this compilation unit this function is specific
to the etnaviv driver and I don't see why we shouldn't have etnaviv
specifics in there if it makes the code of this driver easier to
follow.
Regards,
Lucas
>
> > > {
> > > struct platform_device *pdev;
> > > int ret;
> > > +
> > > + pdev = platform_device_alloc(name, PLATFORM_DEVID_NONE);
> > > + if (!pdev)
> > > + return -ENOMEM;
> > > +
> > > + ret = platform_device_add(pdev);
> > > + if (ret) {
> > > + platform_device_put(pdev);
> > > + return ret;
> > > + }
> > > +
> > > + *ppdev = pdev;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static void etnaviv_destroy_platform_device(struct platform_device **ppdev)
> > > +{
> > > + struct platform_device *pdev = *ppdev;
> > Same here, just use the global variable directly.
> >
> > Regards,
> > Lucas
> >
> > > +
> > > + if (!pdev)
> > > + return;
> > > +
> > > + platform_device_unregister(pdev);
> > > +
> > > + *ppdev = NULL;
> > > +}
> > > +
> > > +static int __init etnaviv_init(void)
> > > +{
> > > + int ret;
> > > struct device_node *np;
> > >
> > > etnaviv_validate_init();
> > > @@ -681,23 +713,13 @@ static int __init etnaviv_init(void)
> > > for_each_compatible_node(np, NULL, "vivante,gc") {
> > > if (!of_device_is_available(np))
> > > continue;
> > > + of_node_put(np);
> > >
> > > - pdev = platform_device_alloc("etnaviv", PLATFORM_DEVID_NONE);
> > > - if (!pdev) {
> > > - ret = -ENOMEM;
> > > - of_node_put(np);
> > > - goto unregister_platform_driver;
> > > - }
> > > -
> > > - ret = platform_device_add(pdev);
> > > - if (ret) {
> > > - platform_device_put(pdev);
> > > - of_node_put(np);
> > > + ret = etnaviv_create_platform_device("etnaviv",
> > > + &etnaviv_platform_device);
> > > + if (ret)
> > > goto unregister_platform_driver;
> > > - }
> > >
> > > - etnaviv_drm = pdev;
> > > - of_node_put(np);
> > > break;
> > > }
> > >
> > > @@ -713,7 +735,7 @@ module_init(etnaviv_init);
> > >
> > > static void __exit etnaviv_exit(void)
> > > {
> > > - platform_device_unregister(etnaviv_drm);
> > > + etnaviv_destroy_platform_device(&etnaviv_platform_device);
> > > platform_driver_unregister(&etnaviv_platform_driver);
> > > platform_driver_unregister(&etnaviv_gpu_driver);
> > > }
>
On 2023/6/21 18:23, Lucas Stach wrote:
>> While back to the question you ask, I want etnaviv_create_platform_device() to be generic,
>>
>> can be used by multiple place for multiple purpose.
>>
>> I have successfully copy this to a another drm driver by simply renaming.
>>
>> The body of the function itself does not need to change.
> But it isn't shared,
This can be shared for drm/etnaviv in the future,
currently, we just need an opportunity to use this function.
I want to create a dummy platform device,
let this dummy platform be bound to the single PCI GPU master.
etnaviv_create_platform_device("dummy", &dummy_device);
1) To verify the component code path on PCI case.
2) Possibly for create a device for some other tiny hardware logic
come with the platform
3) Revival component_compare_dev_name() function.
> in this compilation unit this function is specific
> to the etnaviv driver and I don't see why we shouldn't have etnaviv
> specifics in there if it makes the code of this driver easier to
> follow.
--
Jingfeng
Hi,
On 2023/6/21 18:23, Lucas Stach wrote:
> That's right, but all you do with those indirections through the
> parameter list is move which of the functions is non-pure, in your case
> it's etnaviv_init/etnaviv_exit,
But there is a difference, etnaviv_init() and etnaviv_exit() is
impossible to be shared
there are only get called once when the module is loaded.
They can never be reused anymore, except here.
And etnaviv_init() and etnaviv_exit() don't have a choice.
But for etnaviv_create_platform_device() function,
there is a possibility to be reused in the future.
> with the indirection dropped it's
> etnaviv_create_platform_device/etnaviv_destroy_platform_device.
--
Jingfeng
Am Mittwoch, dem 21.06.2023 um 21:31 +0800 schrieb Sui Jingfeng:
> On 2023/6/21 18:23, Lucas Stach wrote:
> > > While back to the question you ask, I want etnaviv_create_platform_device() to be generic,
> > >
> > > can be used by multiple place for multiple purpose.
> > >
> > > I have successfully copy this to a another drm driver by simply renaming.
> > >
> > > The body of the function itself does not need to change.
> > But it isn't shared,
>
> This can be shared for drm/etnaviv in the future,
>
> currently, we just need an opportunity to use this function.
>
I'm not convinced, yet.
> I want to create a dummy platform device,
>
> let this dummy platform be bound to the single PCI GPU master.
>
>
> etnaviv_create_platform_device("dummy", &dummy_device);
>
>
> 1) To verify the component code path on PCI case.
>
My favorite option would be to just always use the component path even
when the GPU is on a PCI device to keep both paths mostly aligned. One
could easily image both a 3D and a 2D core being made available though
the same PCI device.
> 2) Possibly for create a device for some other tiny hardware logic
> come with the platform
>
Do you have something in mind here? Until now I assumed that only the
GPU core is behind the PCI abstraction. Is there something else sharing
the MMIO space?
Regards,
Lucas
> 3) Revival component_compare_dev_name() function.
>
> > in this compilation unit this function is specific
> > to the etnaviv driver and I don't see why we shouldn't have etnaviv
> > specifics in there if it makes the code of this driver easier to
> > follow.
>
Hi,
On 2023/6/21 22:00, Lucas Stach wrote:
> Am Mittwoch, dem 21.06.2023 um 21:31 +0800 schrieb Sui Jingfeng:
>> On 2023/6/21 18:23, Lucas Stach wrote:
>>>> While back to the question you ask, I want etnaviv_create_platform_device() to be generic,
>>>>
>>>> can be used by multiple place for multiple purpose.
>>>>
>>>> I have successfully copy this to a another drm driver by simply renaming.
>>>>
>>>> The body of the function itself does not need to change.
>>> But it isn't shared,
>> This can be shared for drm/etnaviv in the future,
>>
>> currently, we just need an opportunity to use this function.
>>
> I'm not convinced, yet.
>
>> I want to create a dummy platform device,
>>
>> let this dummy platform be bound to the single PCI GPU master.
>>
>>
>> etnaviv_create_platform_device("dummy", &dummy_device);
>>
>>
>> 1) To verify the component code path on PCI case.
>>
> My favorite option would be to just always use the component path even
> when the GPU is on a PCI device to keep both paths mostly aligned. One
> could easily image both a 3D and a 2D core being made available though
> the same PCI device.
Component is for something that is possible not available. (or something
is optional)
Yes it provided flexibly, but don't forget, it rely on the DT.
But for the PCIe device, it always the case that all of the hardware is
available at the same time
when the device driver(kernel module) is loaded.
>> 2) Possibly for create a device for some other tiny hardware logic
>> come with the platform
>>
> Do you have something in mind here? Until now I assumed that only the
> GPU core is behind the PCI abstraction. Is there something else sharing
> the MMIO space?
A display controller, HDMI phy, vga encoder etc
I have a discrete PCIe GPU card from another vendor,
It integrated display controller and vivante GPU and unknown VPUs.
All of the hardware block mentioned above sharing the MMIO space.
There are available on the same time when you mount this discrete PCIe
GPU card on the mother board
>
> Regards,
> Lucas
>
>> 3) Revival component_compare_dev_name() function.
>>
>>> in this compilation unit this function is specific
>>> to the etnaviv driver and I don't see why we shouldn't have etnaviv
>>> specifics in there if it makes the code of this driver easier to
>>> follow.
--
Jingfeng
On 2023/6/21 22:35, Sui Jingfeng wrote:
> Yes it provided flexibly, but don't forget, it rely on the DT.
Yes it provided flexibility, it rely on the DT provide such flexibility.
--
Jingfeng
Am Mittwoch, dem 21.06.2023 um 22:35 +0800 schrieb Sui Jingfeng:
> Hi,
>
> On 2023/6/21 22:00, Lucas Stach wrote:
> > Am Mittwoch, dem 21.06.2023 um 21:31 +0800 schrieb Sui Jingfeng:
> > > On 2023/6/21 18:23, Lucas Stach wrote:
> > > > > While back to the question you ask, I want etnaviv_create_platform_device() to be generic,
> > > > >
> > > > > can be used by multiple place for multiple purpose.
> > > > >
> > > > > I have successfully copy this to a another drm driver by simply renaming.
> > > > >
> > > > > The body of the function itself does not need to change.
> > > > But it isn't shared,
> > > This can be shared for drm/etnaviv in the future,
> > >
> > > currently, we just need an opportunity to use this function.
> > >
> > I'm not convinced, yet.
> >
> > > I want to create a dummy platform device,
> > >
> > > let this dummy platform be bound to the single PCI GPU master.
> > >
> > >
> > > etnaviv_create_platform_device("dummy", &dummy_device);
> > >
> > >
> > > 1) To verify the component code path on PCI case.
> > >
> > My favorite option would be to just always use the component path even
> > when the GPU is on a PCI device to keep both paths mostly aligned. One
> > could easily image both a 3D and a 2D core being made available though
> > the same PCI device.
>
> Component is for something that is possible not available. (or something
> is optional)
>
> Yes it provided flexibly, but don't forget, it rely on the DT.
The component framework itself doesn't rely on DT in any way. By
providing a appropriate match function you can make it work with any
kind of device. In fact etnaviv supports platform devices instantiated
via board code today. They don't need to come from DT.
If we could make the PCI stuff work the same way, that would be my
preferred option.
>
>
> But for the PCIe device, it always the case that all of the hardware is
> available at the same time
>
> when the device driver(kernel module) is loaded.
That isn't the issue solved by the component framework. On the existing
SoCs all the hardware is available when the driver is probed. The
component framework just makes sure that we only expose the DRM device
after all GPU cores that should be managed by a single DRM device
instance are probed.
One could easily image a PCI device that containing a 2D and a 3D
Vivante GPU that should be made available through a single DRM device.
In that case you'll also need to use the component framework.
>
>
> > > 2) Possibly for create a device for some other tiny hardware logic
> > > come with the platform
> > >
> > Do you have something in mind here? Until now I assumed that only the
> > GPU core is behind the PCI abstraction. Is there something else sharing
> > the MMIO space?
>
> A display controller, HDMI phy, vga encoder etc
>
>
> I have a discrete PCIe GPU card from another vendor,
>
> It integrated display controller and vivante GPU and unknown VPUs.
>
> All of the hardware block mentioned above sharing the MMIO space.
>
> There are available on the same time when you mount this discrete PCIe
> GPU card on the mother board
>
But they surely should not all be made available through the etnaviv
driver. Etnaviv deals with the Vivante GPUs. If you have a PCI device
with multiple IP cores behind the shared MMIO space you should have a
PCI driver instantiating platform devices so the respective drivers for
those IP cores can bind to the platform device. Etnaviv is not that
driver.
Regards,
Lucas
> >
> > Regards,
> > Lucas
> >
> > > 3) Revival component_compare_dev_name() function.
> > >
> > > > in this compilation unit this function is specific
> > > > to the etnaviv driver and I don't see why we shouldn't have etnaviv
> > > > specifics in there if it makes the code of this driver easier to
> > > > follow.
>
Hi
On 2023/6/21 23:20, Lucas Stach wrote:
> Am Mittwoch, dem 21.06.2023 um 22:35 +0800 schrieb Sui Jingfeng:
>> Hi,
>>
>> On 2023/6/21 22:00, Lucas Stach wrote:
>>> Am Mittwoch, dem 21.06.2023 um 21:31 +0800 schrieb Sui Jingfeng:
>>>> On 2023/6/21 18:23, Lucas Stach wrote:
>>>>>> While back to the question you ask, I want etnaviv_create_platform_device() to be generic,
>>>>>>
>>>>>> can be used by multiple place for multiple purpose.
>>>>>>
>>>>>> I have successfully copy this to a another drm driver by simply renaming.
>>>>>>
>>>>>> The body of the function itself does not need to change.
>>>>> But it isn't shared,
>>>> This can be shared for drm/etnaviv in the future,
>>>>
>>>> currently, we just need an opportunity to use this function.
>>>>
>>> I'm not convinced, yet.
>>>
>>>> I want to create a dummy platform device,
>>>>
>>>> let this dummy platform be bound to the single PCI GPU master.
>>>>
>>>>
>>>> etnaviv_create_platform_device("dummy", &dummy_device);
>>>>
>>>>
>>>> 1) To verify the component code path on PCI case.
>>>>
>>> My favorite option would be to just always use the component path even
>>> when the GPU is on a PCI device to keep both paths mostly aligned. One
>>> could easily image both a 3D and a 2D core being made available though
>>> the same PCI device.
>> Component is for something that is possible not available. (or something
>> is optional)
>>
>> Yes it provided flexibly, but don't forget, it rely on the DT.
> The component framework itself doesn't rely on DT in any way.
Yes I know that, for example the HDMI audio stuff.
But *your implement* do rely on the DT, this is the point
> By
> providing a appropriate match function you can make it work with any
> kind of device.
Yes, you are right.
> In fact etnaviv supports platform devices instantiated
> via board code today.
Nice,
> They don't need to come from DT.
What about the various clock, sir?
> If we could make the PCI stuff work the same way, that would be my
> preferred option.
>
>>
>> But for the PCIe device, it always the case that all of the hardware is
>> available at the same time
>>
>> when the device driver(kernel module) is loaded.
> That isn't the issue solved by the component framework. On the existing
> SoCs all the hardware is available when the driver is probed. The
> component framework just makes sure that we only expose the DRM device
> after all GPU cores that should be managed by a single DRM device
> instance are probed.
>
> One could easily image a PCI device that containing a 2D and a 3D
> Vivante GPU that should be made available through a single DRM device.
> In that case you'll also need to use the component framework.
>
>>
>>>> 2) Possibly for create a device for some other tiny hardware logic
>>>> come with the platform
>>>>
>>> Do you have something in mind here? Until now I assumed that only the
>>> GPU core is behind the PCI abstraction. Is there something else sharing
>>> the MMIO space?
>> A display controller, HDMI phy, vga encoder etc
>>
>>
>> I have a discrete PCIe GPU card from another vendor,
>>
>> It integrated display controller and vivante GPU and unknown VPUs.
>>
>> All of the hardware block mentioned above sharing the MMIO space.
>>
>> There are available on the same time when you mount this discrete PCIe
>> GPU card on the mother board
>>
> But they surely should not all be made available through the etnaviv
> driver. Etnaviv deals with the Vivante GPUs. If you have a PCI device
> with multiple IP cores behind the shared MMIO space you should have a
> PCI driver instantiating platform devices so the respective drivers for
> those IP cores can bind to the platform device.
I have only one PCI device, let start from the simple case, OK?
I admire your fantastic idea.
let deal with it another patch in the future if such hardware emerged.
Accept the current implement, please ?
> Etnaviv is not that
> driver.
Yeah, but I notice that there is chipFeatures_DC defined in common.xml.h
I don't know how does this going to used, if a hardware marked it as true.
> Regards,
> Lucas
>
>>> Regards,
>>> Lucas
>>>
>>>> 3) Revival component_compare_dev_name() function.
>>>>
>>>>> in this compilation unit this function is specific
>>>>> to the etnaviv driver and I don't see why we shouldn't have etnaviv
>>>>> specifics in there if it makes the code of this driver easier to
>>>>> follow.
--
Jingfeng