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

[Sui Jingfeng]

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