Re: [PATCH v10 03/11] drm/etnaviv: Add dedicated functions to create and destroy platform device

[Sui Jingfeng <suijingfeng@loongson.cn>]

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