[PATCH libdrm 3/3] xf86drm: Add platform and host1x bus support

[Emil Velikov]

On 2 January 2017 at 13:53, Thierry Reding <thierry.reding at gmail.com> wrote:
> On Sat, Dec 24, 2016 at 05:00:27PM +0000, Emil Velikov wrote:
>> On 23 December 2016 at 17:49, Thierry Reding <thierry.reding at gmail.com> wrote:
>> > From: Thierry Reding <treding at nvidia.com>
>> >
>> > ARM SoCs usually have their DRM/KMS devices on the platform bus, so add
>> > support for that to enable these devices to be used with the drmDevice
>> > infrastructure.
>> >
>> > NVIDIA Tegra SoCs have an additional level in the hierarchy and DRM/KMS
>> > devices can also be on the host1x bus. This is mostly equivalent to the
>> > platform bus.
>> >
>> > Signed-off-by: Thierry Reding <treding at nvidia.com>
>> > ---
>> > Note that we could probably get away with treating host1x as platform
>> > bus. However, they are technically two different busses in the kernel
>> > and hence we may want to make use of that differentiation later on.
>> >
>> Admittedly there's no clear cut either way, but I'm inclined to have
>> host1x as platform.
>> I'm leaning towards that since there is no /sys/bus/host1x (afaict),
>
> Actually there is:
>
>         # find /sys/bus/host1x
>         /sys/bus/host1x
>         /sys/bus/host1x/drivers_probe
>         /sys/bus/host1x/devices
>         /sys/bus/host1x/devices/drm
>         /sys/bus/host1x/uevent
>         /sys/bus/host1x/drivers
>         /sys/bus/host1x/drivers/drm
>         /sys/bus/host1x/drivers/drm/bind
>         /sys/bus/host1x/drivers/drm/unbind
>         /sys/bus/host1x/drivers/drm/module
>         /sys/bus/host1x/drivers/drm/uevent
>         /sys/bus/host1x/drivers/drm/drm
>         /sys/bus/host1x/drivers_autoprobe
>
>> plus otherwise devices (like imx-vpu/vc4) will end with their own bus
>> type.
>
> I don't think that would happen. Tegra is somewhat special in this case
> because of the host1x device that is a parent for both display and
> capture devices. The host1x bus is architected to allow binding each of
> the DRM/KMS and V4L2 devices (the V4L2 driver hasn't been merged yet)
> to the host1x device.
>
> The reason for this is mostly historical. Back when Tegra support was
> added there was a strict rule against adding dummy device nodes to the
> device tree. For Tegra this wasn't much of a problem because the host1x
> device is the logical choice for a parent, so I just had to write some
> glue (the host1x bus) to instantiate a dummy device for each composite
> driver.
>
> Later on the component/master infrastructure was added and the rules
> regarding dummy device nodes were relaxed, so as far as I know all other
> devices end up anchored to a dummy device on the platform bus.
>
I see. Thanks for the correction and the educational comment.

>> > +static int drmParsePlatformBusInfo(int maj, int min, drmPlatformBusInfoPtr info)
>> > +{
>> > +    char path[PATH_MAX + 1], *name;
>> > +
>> > +    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device", maj, min);
>> > +
>> > +    name = sysfs_uevent_get(path, "OF_FULLNAME");
>> > +    strcpy(info->fullname, name);
>> > +    free(name);
>> > +
>> Let's be extra careful and ensure that we don't overrun (and properly
>> terminate) the fixed size fullname[].
>
> Okay, will do.
>
>> > +    return 0;
>> > +}
>> > +
>> > +static int drmParsePlatformDeviceInfo(int maj, int min,
>> > +                                      drmPlatformDeviceInfoPtr info)
>> > +{
>> > +    /* XXX fill in platform device info */
>> > +
>> Not 100% sure what exactly we should consider bus and what device
>> info, either way an empty struct is not going to go well.
>> As a food for thought, here is some info for imx/etna and vc4.
>>
>> cat /sys/dev/char/226\:*/device/uevent
>>
>> DRIVER=etnaviv
>> OF_NAME=gpu-subsystem
>> OF_FULLNAME=/gpu-subsystem
>> OF_COMPATIBLE_0=fsl,imx-gpu-subsystem
>> OF_COMPATIBLE_N=1
>> MODALIAS=of:Ngpu-subsystemT<NULL>Cfsl,imx-gpu-subsystem
>> DRIVER=imx-drm
>> OF_NAME=display-subsystem
>> OF_FULLNAME=/display-subsystem
>> OF_COMPATIBLE_0=fsl,imx-display-subsystem
>> OF_COMPATIBLE_N=1
>>
>> DRIVER=vc4-drm
>> OF_NAME=gpu
>> OF_FULLNAME=/soc/gpu
>> OF_COMPATIBLE_0=brcm,bcm2835-vc4
>> OF_COMPATIBLE_N=1
>> MODALIAS=of:NgpuT<NULL>Cbrcm,bcm2835-vc4
>
> I think the full name is appropriate for bus info because it's about the
> closest thing we have to address the device (the equivalent to domain,
> bus, device and function numbers).
>
> The list of compatible strings might be suitable for device information
> and might be useful for drivers to determine capabilities if the kernel
> doesn't have another means of communicating that to userspace.
>
Agreed. With your thorough explanation in mind I think your
patch/proposal is spot on.

So with the overflow check, this is safe to land.

Thanks
Emil