| Message ID | 1484747961-5418-1-git-send-email-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 18, 2017 at 02:59:21PM +0100, Neil Armstrong wrote: > Add support for Amlogic Meson DRM driver merged for Linux 4.10. > > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > tests/util/kms.c | 1 + > 1 file changed, 1 insertion(+) Applied, thanks. Thierry
On 19 January 2017 at 09:19, Thierry Reding <treding@nvidia.com> wrote: > On Wed, Jan 18, 2017 at 02:59:21PM +0100, Neil Armstrong wrote: >> Add support for Amlogic Meson DRM driver merged for Linux 4.10. >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> tests/util/kms.c | 1 + >> 1 file changed, 1 insertion(+) > > Applied, thanks. > Was going to say "NACK use Thierry's helpers" but you've beat me to it. As a Tl;DR: we _really_ want to stop using drmOpen* for anything that's KMS. -Emil
On Fri, Jan 20, 2017 at 02:13:00PM +0000, Emil Velikov wrote: > On 19 January 2017 at 09:19, Thierry Reding <treding@nvidia.com> wrote: > > On Wed, Jan 18, 2017 at 02:59:21PM +0100, Neil Armstrong wrote: > >> Add support for Amlogic Meson DRM driver merged for Linux 4.10. > >> > >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > >> --- > >> tests/util/kms.c | 1 + > >> 1 file changed, 1 insertion(+) > > > > Applied, thanks. > > > Was going to say "NACK use Thierry's helpers" but you've beat me to it. > As a Tl;DR: we _really_ want to stop using drmOpen* for anything that's KMS. I think it's still useful to have this helper to iterate over all supported driver because it enables easy testing with just a simple modetest. That said, for (almost) as long as I remember I've been using the -M option to prevent modetest from iterating over the list, which can take fairly long if you've got DRM_DEBUG messages enabled. I'm not sure I understand exactly what you're suggesting by "use Thierry's helpers". modetest and other tests use util_open() internally now. That in turn uses drmOpen(), though we're of course open to change that. Are you suggesting we somehow use drmDevice to locate existing devices? We could implement some heuristic that finds the first device with a primary node, but what if we want to support the -M option? There is currently no way of getting the driver from drmDevice. Maybe that's something we should add anyway. Thierry
On 20 January 2017 at 16:17, Thierry Reding <treding@nvidia.com> wrote: > On Fri, Jan 20, 2017 at 02:13:00PM +0000, Emil Velikov wrote: >> On 19 January 2017 at 09:19, Thierry Reding <treding@nvidia.com> wrote: >> > On Wed, Jan 18, 2017 at 02:59:21PM +0100, Neil Armstrong wrote: >> >> Add support for Amlogic Meson DRM driver merged for Linux 4.10. >> >> >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> >> --- >> >> tests/util/kms.c | 1 + >> >> 1 file changed, 1 insertion(+) >> > >> > Applied, thanks. >> > >> Was going to say "NACK use Thierry's helpers" but you've beat me to it. >> As a Tl;DR: we _really_ want to stop using drmOpen* for anything that's KMS. > > I think it's still useful to have this helper to iterate over all > supported driver because it enables easy testing with just a simple > modetest. > > That said, for (almost) as long as I remember I've been using the -M > option to prevent modetest from iterating over the list, which can take > fairly long if you've got DRM_DEBUG messages enabled. > > I'm not sure I understand exactly what you're suggesting by "use > Thierry's helpers". modetest and other tests use util_open() internally > now. That in turn uses drmOpen(), though we're of course open to change > that. Are you suggesting we somehow use drmDevice to locate existing > devices? > Yes using drmDevice is what I had in mind. The only thing that was stopping me from doing that is the lack of platform devices support. With that in we can start purging _everything_ that uses drmOpen*. > We could implement some heuristic that finds the first device with a > primary node, but what if we want to support the -M option? There is > currently no way of getting the driver from drmDevice. Maybe that's > something we should add anyway. > The -M (kernel module name as given by drmGetVersion) is another interesting topic. Feel free to skip to the Tl;Dr below. If doing the ioctl implicitly via drmOpen or drmDevice wakes up the device, keeping in mind that: - it can take some time, and - you don't always need the info A couple of elaborate workarounds include: - use only on demand - add DRM_DEVICE_GET_MODULE_NAME or alike to the drmDevice2 API Needs a big "this can be VERY slow" warning in the documentation/man pages... first we need actual man pages for libdrm ;-) - deprecate the whole thing and use the compat strings - how do we handle the PCI devices ? - use the kernel module name (foo.ko) - kind of making it an ABI, but iirc we've already have users which depend on it Neither PCI devices nor platform ones are consistent in their naming - foo.ko vs the value returned by the IOCTL. We might be able to combine the latter two ... need to double-check. Tl;Dr: For the moment I'd leave it to the user to call drmGetVersion. Thanks Emil
On Fri, Jan 20, 2017 at 06:28:39PM +0000, Emil Velikov wrote: > On 20 January 2017 at 16:17, Thierry Reding <treding@nvidia.com> wrote: > > On Fri, Jan 20, 2017 at 02:13:00PM +0000, Emil Velikov wrote: > >> On 19 January 2017 at 09:19, Thierry Reding <treding@nvidia.com> wrote: > >> > On Wed, Jan 18, 2017 at 02:59:21PM +0100, Neil Armstrong wrote: > >> >> Add support for Amlogic Meson DRM driver merged for Linux 4.10. > >> >> > >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > >> >> --- > >> >> tests/util/kms.c | 1 + > >> >> 1 file changed, 1 insertion(+) > >> > > >> > Applied, thanks. > >> > > >> Was going to say "NACK use Thierry's helpers" but you've beat me to it. > >> As a Tl;DR: we _really_ want to stop using drmOpen* for anything that's KMS. > > > > I think it's still useful to have this helper to iterate over all > > supported driver because it enables easy testing with just a simple > > modetest. > > > > That said, for (almost) as long as I remember I've been using the -M > > option to prevent modetest from iterating over the list, which can take > > fairly long if you've got DRM_DEBUG messages enabled. > > > > I'm not sure I understand exactly what you're suggesting by "use > > Thierry's helpers". modetest and other tests use util_open() internally > > now. That in turn uses drmOpen(), though we're of course open to change > > that. Are you suggesting we somehow use drmDevice to locate existing > > devices? > > > Yes using drmDevice is what I had in mind. The only thing that was > stopping me from doing that is the lack of platform devices support. > With that in we can start purging _everything_ that uses drmOpen*. > > > We could implement some heuristic that finds the first device with a > > primary node, but what if we want to support the -M option? There is > > currently no way of getting the driver from drmDevice. Maybe that's > > something we should add anyway. > > > The -M (kernel module name as given by drmGetVersion) is another > interesting topic. Feel free to skip to the Tl;Dr below. > > If doing the ioctl implicitly via drmOpen or drmDevice wakes up the > device, keeping in mind that: > - it can take some time, and > - you don't always need the info > > A couple of elaborate workarounds include: > - use only on demand - add DRM_DEVICE_GET_MODULE_NAME or alike to the > drmDevice2 API > Needs a big "this can be VERY slow" warning in the documentation/man > pages... first we need actual man pages for libdrm ;-) > - deprecate the whole thing and use the compat strings - how do we > handle the PCI devices ? > - use the kernel module name (foo.ko) - kind of making it an ABI, but > iirc we've already have users which depend on it > Neither PCI devices nor platform ones are consistent in their naming - > foo.ko vs the value returned by the IOCTL. > > We might be able to combine the latter two ... need to double-check. > > Tl;Dr: For the moment I'd leave it to the user to call drmGetVersion. Okay, sounds like a plan. The good thing is we can keep adding new module names to the util_open() helper irrespective of when we change the implementation to use drmDevice instead of drmOpen() since it is conveniently encapsulated in a single function across libdrm. Thierry
On 26 January 2017 at 15:49, Thierry Reding <treding@nvidia.com> wrote: > On Fri, Jan 20, 2017 at 06:28:39PM +0000, Emil Velikov wrote: >> On 20 January 2017 at 16:17, Thierry Reding <treding@nvidia.com> wrote: >> > On Fri, Jan 20, 2017 at 02:13:00PM +0000, Emil Velikov wrote: >> >> On 19 January 2017 at 09:19, Thierry Reding <treding@nvidia.com> wrote: >> >> > On Wed, Jan 18, 2017 at 02:59:21PM +0100, Neil Armstrong wrote: >> >> >> Add support for Amlogic Meson DRM driver merged for Linux 4.10. >> >> >> >> >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> >> >> --- >> >> >> tests/util/kms.c | 1 + >> >> >> 1 file changed, 1 insertion(+) >> >> > >> >> > Applied, thanks. >> >> > >> >> Was going to say "NACK use Thierry's helpers" but you've beat me to it. >> >> As a Tl;DR: we _really_ want to stop using drmOpen* for anything that's KMS. >> > >> > I think it's still useful to have this helper to iterate over all >> > supported driver because it enables easy testing with just a simple >> > modetest. >> > >> > That said, for (almost) as long as I remember I've been using the -M >> > option to prevent modetest from iterating over the list, which can take >> > fairly long if you've got DRM_DEBUG messages enabled. >> > >> > I'm not sure I understand exactly what you're suggesting by "use >> > Thierry's helpers". modetest and other tests use util_open() internally >> > now. That in turn uses drmOpen(), though we're of course open to change >> > that. Are you suggesting we somehow use drmDevice to locate existing >> > devices? >> > >> Yes using drmDevice is what I had in mind. The only thing that was >> stopping me from doing that is the lack of platform devices support. >> With that in we can start purging _everything_ that uses drmOpen*. >> >> > We could implement some heuristic that finds the first device with a >> > primary node, but what if we want to support the -M option? There is >> > currently no way of getting the driver from drmDevice. Maybe that's >> > something we should add anyway. >> > >> The -M (kernel module name as given by drmGetVersion) is another >> interesting topic. Feel free to skip to the Tl;Dr below. >> >> If doing the ioctl implicitly via drmOpen or drmDevice wakes up the >> device, keeping in mind that: >> - it can take some time, and >> - you don't always need the info >> >> A couple of elaborate workarounds include: >> - use only on demand - add DRM_DEVICE_GET_MODULE_NAME or alike to the >> drmDevice2 API >> Needs a big "this can be VERY slow" warning in the documentation/man >> pages... first we need actual man pages for libdrm ;-) >> - deprecate the whole thing and use the compat strings - how do we >> handle the PCI devices ? >> - use the kernel module name (foo.ko) - kind of making it an ABI, but >> iirc we've already have users which depend on it >> Neither PCI devices nor platform ones are consistent in their naming - >> foo.ko vs the value returned by the IOCTL. >> >> We might be able to combine the latter two ... need to double-check. >> >> Tl;Dr: For the moment I'd leave it to the user to call drmGetVersion. > > Okay, sounds like a plan. The good thing is we can keep adding new > module names to the util_open() helper The key point is that you do _not_ need any list. Any new and existing drivers should just work. If they don't then they're [highly likely] broken and should be fixed ;-) -Emil
On Thu, Jan 26, 2017 at 04:14:56PM +0000, Emil Velikov wrote: > On 26 January 2017 at 15:49, Thierry Reding <treding@nvidia.com> wrote: > > On Fri, Jan 20, 2017 at 06:28:39PM +0000, Emil Velikov wrote: > >> On 20 January 2017 at 16:17, Thierry Reding <treding@nvidia.com> wrote: > >> > On Fri, Jan 20, 2017 at 02:13:00PM +0000, Emil Velikov wrote: > >> >> On 19 January 2017 at 09:19, Thierry Reding <treding@nvidia.com> wrote: > >> >> > On Wed, Jan 18, 2017 at 02:59:21PM +0100, Neil Armstrong wrote: > >> >> >> Add support for Amlogic Meson DRM driver merged for Linux 4.10. > >> >> >> > >> >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > >> >> >> --- > >> >> >> tests/util/kms.c | 1 + > >> >> >> 1 file changed, 1 insertion(+) > >> >> > > >> >> > Applied, thanks. > >> >> > > >> >> Was going to say "NACK use Thierry's helpers" but you've beat me to it. > >> >> As a Tl;DR: we _really_ want to stop using drmOpen* for anything that's KMS. > >> > > >> > I think it's still useful to have this helper to iterate over all > >> > supported driver because it enables easy testing with just a simple > >> > modetest. > >> > > >> > That said, for (almost) as long as I remember I've been using the -M > >> > option to prevent modetest from iterating over the list, which can take > >> > fairly long if you've got DRM_DEBUG messages enabled. > >> > > >> > I'm not sure I understand exactly what you're suggesting by "use > >> > Thierry's helpers". modetest and other tests use util_open() internally > >> > now. That in turn uses drmOpen(), though we're of course open to change > >> > that. Are you suggesting we somehow use drmDevice to locate existing > >> > devices? > >> > > >> Yes using drmDevice is what I had in mind. The only thing that was > >> stopping me from doing that is the lack of platform devices support. > >> With that in we can start purging _everything_ that uses drmOpen*. > >> > >> > We could implement some heuristic that finds the first device with a > >> > primary node, but what if we want to support the -M option? There is > >> > currently no way of getting the driver from drmDevice. Maybe that's > >> > something we should add anyway. > >> > > >> The -M (kernel module name as given by drmGetVersion) is another > >> interesting topic. Feel free to skip to the Tl;Dr below. > >> > >> If doing the ioctl implicitly via drmOpen or drmDevice wakes up the > >> device, keeping in mind that: > >> - it can take some time, and > >> - you don't always need the info > >> > >> A couple of elaborate workarounds include: > >> - use only on demand - add DRM_DEVICE_GET_MODULE_NAME or alike to the > >> drmDevice2 API > >> Needs a big "this can be VERY slow" warning in the documentation/man > >> pages... first we need actual man pages for libdrm ;-) > >> - deprecate the whole thing and use the compat strings - how do we > >> handle the PCI devices ? > >> - use the kernel module name (foo.ko) - kind of making it an ABI, but > >> iirc we've already have users which depend on it > >> Neither PCI devices nor platform ones are consistent in their naming - > >> foo.ko vs the value returned by the IOCTL. > >> > >> We might be able to combine the latter two ... need to double-check. > >> > >> Tl;Dr: For the moment I'd leave it to the user to call drmGetVersion. > > > > Okay, sounds like a plan. The good thing is we can keep adding new > > module names to the util_open() helper > The key point is that you do _not_ need any list. Any new and existing > drivers should just work. If they don't then they're [highly likely] > broken and should be fixed ;-) Yeah, what I was trying to say is that the implementation is now centralized, so adding module names until we replace the implementation isn't going to hurt much, while at the same time getting people the support that they want. Once we transition the implementation to using drmDevice, the list will simply disappear and everyone will be using the new code automatically. That said, I have a couple of things on my plate for today, but I'll go reimplement util_open() when I'm done with those. Thierry
On 27 January 2017 at 07:47, Thierry Reding <treding@nvidia.com> wrote: > On Thu, Jan 26, 2017 at 04:14:56PM +0000, Emil Velikov wrote: >> On 26 January 2017 at 15:49, Thierry Reding <treding@nvidia.com> wrote: >> > On Fri, Jan 20, 2017 at 06:28:39PM +0000, Emil Velikov wrote: >> >> On 20 January 2017 at 16:17, Thierry Reding <treding@nvidia.com> wrote: >> >> > On Fri, Jan 20, 2017 at 02:13:00PM +0000, Emil Velikov wrote: >> >> >> On 19 January 2017 at 09:19, Thierry Reding <treding@nvidia.com> wrote: >> >> >> > On Wed, Jan 18, 2017 at 02:59:21PM +0100, Neil Armstrong wrote: >> >> >> >> Add support for Amlogic Meson DRM driver merged for Linux 4.10. >> >> >> >> >> >> >> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> >> >> >> --- >> >> >> >> tests/util/kms.c | 1 + >> >> >> >> 1 file changed, 1 insertion(+) >> >> >> > >> >> >> > Applied, thanks. >> >> >> > >> >> >> Was going to say "NACK use Thierry's helpers" but you've beat me to it. >> >> >> As a Tl;DR: we _really_ want to stop using drmOpen* for anything that's KMS. >> >> > >> >> > I think it's still useful to have this helper to iterate over all >> >> > supported driver because it enables easy testing with just a simple >> >> > modetest. >> >> > >> >> > That said, for (almost) as long as I remember I've been using the -M >> >> > option to prevent modetest from iterating over the list, which can take >> >> > fairly long if you've got DRM_DEBUG messages enabled. >> >> > >> >> > I'm not sure I understand exactly what you're suggesting by "use >> >> > Thierry's helpers". modetest and other tests use util_open() internally >> >> > now. That in turn uses drmOpen(), though we're of course open to change >> >> > that. Are you suggesting we somehow use drmDevice to locate existing >> >> > devices? >> >> > >> >> Yes using drmDevice is what I had in mind. The only thing that was >> >> stopping me from doing that is the lack of platform devices support. >> >> With that in we can start purging _everything_ that uses drmOpen*. >> >> >> >> > We could implement some heuristic that finds the first device with a >> >> > primary node, but what if we want to support the -M option? There is >> >> > currently no way of getting the driver from drmDevice. Maybe that's >> >> > something we should add anyway. >> >> > >> >> The -M (kernel module name as given by drmGetVersion) is another >> >> interesting topic. Feel free to skip to the Tl;Dr below. >> >> >> >> If doing the ioctl implicitly via drmOpen or drmDevice wakes up the >> >> device, keeping in mind that: >> >> - it can take some time, and >> >> - you don't always need the info >> >> >> >> A couple of elaborate workarounds include: >> >> - use only on demand - add DRM_DEVICE_GET_MODULE_NAME or alike to the >> >> drmDevice2 API >> >> Needs a big "this can be VERY slow" warning in the documentation/man >> >> pages... first we need actual man pages for libdrm ;-) >> >> - deprecate the whole thing and use the compat strings - how do we >> >> handle the PCI devices ? >> >> - use the kernel module name (foo.ko) - kind of making it an ABI, but >> >> iirc we've already have users which depend on it >> >> Neither PCI devices nor platform ones are consistent in their naming - >> >> foo.ko vs the value returned by the IOCTL. >> >> >> >> We might be able to combine the latter two ... need to double-check. >> >> >> >> Tl;Dr: For the moment I'd leave it to the user to call drmGetVersion. >> > >> > Okay, sounds like a plan. The good thing is we can keep adding new >> > module names to the util_open() helper >> The key point is that you do _not_ need any list. Any new and existing >> drivers should just work. If they don't then they're [highly likely] >> broken and should be fixed ;-) > > Yeah, what I was trying to say is that the implementation is now > centralized, so adding module names until we replace the implementation > isn't going to hurt much, while at the same time getting people the > support that they want. > > Once we transition the implementation to using drmDevice, the list will > simply disappear and everyone will be using the new code automatically. > > That said, I have a couple of things on my plate for today, but I'll go > reimplement util_open() when I'm done with those. > Seems like I completely misunderstood you there :-\ Pardon if I came too picky earlier. Thanks Emil
diff --git a/tests/util/kms.c b/tests/util/kms.c index 4d89ac8..959b688 100644 --- a/tests/util/kms.c +++ b/tests/util/kms.c @@ -144,6 +144,7 @@ static const char * const modules[] = { "vc4", "virtio_gpu", "mediatek", + "meson", }; int util_open(const char *device, const char *module)
Add support for Amlogic Meson DRM driver merged for Linux 4.10. Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> --- tests/util/kms.c | 1 + 1 file changed, 1 insertion(+)