| Message ID | 20161223174917.29267-4-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23 December 2016 at 17:49, Thierry Reding <thierry.reding@gmail.com> wrote: > From: Thierry Reding <treding@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@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), plus otherwise devices (like imx-vpu/vc4) will end with their own bus type. > +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[]. > + 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 Thanks Emil
On Sat, Dec 24, 2016 at 05:00:27PM +0000, Emil Velikov wrote: > On 23 December 2016 at 17:49, Thierry Reding <thierry.reding@gmail.com> wrote: > > From: Thierry Reding <treding@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@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. > > +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. Thierry
On 2 January 2017 at 13:53, Thierry Reding <thierry.reding@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@gmail.com> wrote: >> > From: Thierry Reding <treding@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@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
diff --git a/xf86drm.c b/xf86drm.c index f0171c34c958..69887accb3cb 100644 --- a/xf86drm.c +++ b/xf86drm.c @@ -2905,6 +2905,12 @@ static int drmParseSubsystemType(int maj, int min) if (strncmp(name, "/usb", 4) == 0) return DRM_BUS_USB; + if (strncmp(name, "/platform", 9) == 0) + return DRM_BUS_PLATFORM; + + if (strncmp(name, "/host1x", 7) == 0) + return DRM_BUS_HOST1X; + return -EINVAL; #elif defined(__OpenBSD__) return DRM_BUS_PCI; @@ -2995,6 +3001,12 @@ static int drmCompareBusInfo(drmDevicePtr a, drmDevicePtr b) case DRM_BUS_USB: return memcmp(a->businfo.usb, b->businfo.usb, sizeof(drmUsbBusInfo)); + case DRM_BUS_PLATFORM: + return memcmp(a->businfo.platform, b->businfo.platform, sizeof(drmPlatformBusInfo)); + + case DRM_BUS_HOST1X: + return memcmp(a->businfo.host1x, b->businfo.host1x, sizeof(drmHost1xBusInfo)); + default: break; } @@ -3357,6 +3369,128 @@ free_device: return ret; } +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); + + return 0; +} + +static int drmParsePlatformDeviceInfo(int maj, int min, + drmPlatformDeviceInfoPtr info) +{ + /* XXX fill in platform device info */ + + return 0; +} + +static int drmProcessPlatformDevice(drmDevicePtr *device, + const char *node, int node_type, + int maj, int min, bool fetch_deviceinfo, + uint32_t flags) +{ + drmDevicePtr dev; + char *ptr; + int ret; + + dev = drmDeviceAlloc(node_type, node, sizeof(drmPlatformBusInfo), + sizeof(drmPlatformDeviceInfo), &ptr); + if (!dev) + return -ENOMEM; + + dev->bustype = DRM_BUS_PLATFORM; + + dev->businfo.platform = (drmPlatformBusInfoPtr)ptr; + + ret = drmParsePlatformBusInfo(maj, min, dev->businfo.platform); + if (ret < 0) + goto free_device; + + if (fetch_deviceinfo) { + ptr += sizeof(drmPlatformBusInfo); + dev->deviceinfo.platform = (drmPlatformDeviceInfoPtr)ptr; + + ret = drmParsePlatformDeviceInfo(maj, min, dev->deviceinfo.platform); + if (ret < 0) + goto free_device; + } + + *device = dev; + + return 0; + +free_device: + free(dev); + return ret; +} + +static int drmParseHost1xBusInfo(int maj, int min, drmHost1xBusInfoPtr 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); + + return 0; +} + +static int drmParseHost1xDeviceInfo(int maj, int min, + drmHost1xDeviceInfoPtr info) +{ + /* XXX fill in host1x device info */ + + return 0; +} + +static int drmProcessHost1xDevice(drmDevicePtr *device, + const char *node, int node_type, + int maj, int min, bool fetch_deviceinfo, + uint32_t flags) +{ + drmDevicePtr dev; + char *ptr; + int ret; + + dev = drmDeviceAlloc(node_type, node, sizeof(drmHost1xBusInfo), + sizeof(drmHost1xDeviceInfo), &ptr); + if (!dev) + return -ENOMEM; + + dev->bustype = DRM_BUS_HOST1X; + + dev->businfo.host1x = (drmHost1xBusInfoPtr)ptr; + + ret = drmParseHost1xBusInfo(maj, min, dev->businfo.host1x); + if (ret < 0) + goto free_device; + + if (fetch_deviceinfo) { + ptr += sizeof(drmHost1xBusInfo); + dev->deviceinfo.host1x = (drmHost1xDeviceInfoPtr)ptr; + + ret = drmParseHost1xDeviceInfo(maj, min, dev->deviceinfo.host1x); + if (ret < 0) + goto free_device; + } + + *device = dev; + + return 0; + +free_device: + free(dev); + return ret; +} + /* Consider devices located on the same bus as duplicate and fold the respective * entries into a single one. * @@ -3536,6 +3670,20 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device) break; + case DRM_BUS_PLATFORM: + ret = drmProcessPlatformDevice(&d, node, node_type, maj, min, true, flags); + if (ret) + goto free_devices; + + break; + + case DRM_BUS_HOST1X: + ret = drmProcessHost1xDevice(&d, node, node_type, maj, min, true, flags); + if (ret) + goto free_devices; + + break; + default: continue; } @@ -3676,6 +3824,22 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices) break; + case DRM_BUS_PLATFORM: + ret = drmProcessPlatformDevice(&device, node, node_type, maj, min, + devices != NULL, flags); + if (ret) + goto free_devices; + + break; + + case DRM_BUS_HOST1X: + ret = drmProcessHost1xDevice(&device, node, node_type, maj, min, + devices != NULL, flags); + if (ret) + goto free_devices; + + break; + default: continue; } diff --git a/xf86drm.h b/xf86drm.h index 65d5321950fc..eeb19e5e187e 100644 --- a/xf86drm.h +++ b/xf86drm.h @@ -766,8 +766,10 @@ extern int drmPrimeFDToHandle(int fd, int prime_fd, uint32_t *handle); extern char *drmGetPrimaryDeviceNameFromFd(int fd); extern char *drmGetRenderDeviceNameFromFd(int fd); -#define DRM_BUS_PCI 0 -#define DRM_BUS_USB 1 +#define DRM_BUS_PCI 0 +#define DRM_BUS_USB 1 +#define DRM_BUS_PLATFORM 2 +#define DRM_BUS_HOST1X 3 typedef struct _drmPciBusInfo { uint16_t domain; @@ -794,6 +796,20 @@ typedef struct _drmUsbDeviceInfo { uint16_t product; } drmUsbDeviceInfo, *drmUsbDeviceInfoPtr; +typedef struct _drmPlatformBusInfo { + char fullname[512]; +} drmPlatformBusInfo, *drmPlatformBusInfoPtr; + +typedef struct _drmPlatformDeviceInfo { +} drmPlatformDeviceInfo, *drmPlatformDeviceInfoPtr; + +typedef struct _drmHost1xBusInfo { + char fullname[512]; +} drmHost1xBusInfo, *drmHost1xBusInfoPtr; + +typedef struct _drmHost1xDeviceInfo { +} drmHost1xDeviceInfo, *drmHost1xDeviceInfoPtr; + typedef struct _drmDevice { char **nodes; /* DRM_NODE_MAX sized array */ int available_nodes; /* DRM_NODE_* bitmask */ @@ -801,10 +817,14 @@ typedef struct _drmDevice { union { drmPciBusInfoPtr pci; drmUsbBusInfoPtr usb; + drmPlatformBusInfoPtr platform; + drmHost1xBusInfoPtr host1x; } businfo; union { drmPciDeviceInfoPtr pci; drmUsbDeviceInfoPtr usb; + drmPlatformDeviceInfoPtr platform; + drmHost1xDeviceInfoPtr host1x; } deviceinfo; } drmDevice, *drmDevicePtr;