On Wed, Feb 01, 2017 at 12:47:21PM +0000, Emil Velikov wrote:
On 30 January 2017 at 10:29, Thierry Reding thierry.reding@gmail.com wrote:
From: Thierry Reding treding@nvidia.com
The util_open() helper is used in a couple of test programs to open an appropriate device. It takes a device path and a module name, both are optional, as parameters. If a device path is specified, it will try to open the given device. Otherwise it will try all available devices. If only a specific subset is desired, the module parameter can be used as a filter. The function will use it to open only devices whose kernel driver matches the given module name.
Instead of relying on the legacy drmOpen() function to do this, convert util_open() to use the new drmDevice helpers. This gets it functionally much closer to what other DRM/KMS users, such as the X.Org Server or a Wayland server, do.
Signed-off-by: Thierry Reding treding@nvidia.com
tests/util/kms.c | 167 +++++++++++++++++++++++++++++++++++++++++-------------- tests/util/kms.h | 43 ++++++++++++++ 2 files changed, 168 insertions(+), 42 deletions(-)
diff --git a/tests/util/kms.c b/tests/util/kms.c index d866398237bb..c5d35ab616d1 100644 --- a/tests/util/kms.c +++ b/tests/util/kms.c
+int util_get_devices(drmDevicePtr **devicesp, uint32_t flags)
Personally I would not have bothered with this helper, but it's nice to have ;-)
Yeah, I added this after realizing that I was typing the same code sequence for the second time.
+{
- if (!devicesp)
return err;
- if (devicesp)
With the "if !devicesp" check above this should always be true, no ?
Yes, you're absolutely right. I added the above shortcut in a very late iteration and forgot to update the tail of the function.
+int util_open_with_module(const char *device, const char *module)
Name and thus distinction vs util_open is blurred - here we require non-null device... which we should check for.
Yes, I've added a check for that now.
Sadly I'm out of ideas for better name(s) :-(
I know this isn't ideal, but hopefully the doxygen in patch 3/3 will clarify somewhat how these are to be used.
{
- int fd;
- int fd, err = 0;
- if (module)
printf("trying to open `%s' with `%s'...", device, module);- else
printf("trying to open `%s'...", device);- fd = open(device, O_RDWR);
Not sure how much we should care here, but O_CLOEXEC would be nice.
Yes, added.
+int util_open(const char *device, const char *module) +{
- } else {
fd = util_open_with_module(device, module);Nit: one can drop a level of indentation/brackets - I have no strong preference either way.
int util_open(...) { if (device) return util_open_with_module(device, module);
... get_devices ... }
This looks better indeed. It's also nice how the implementation now very closely matches the description in the doxygen comment introduced in patch 3/3.
--- a/tests/util/kms.h +++ b/tests/util/kms.h
+static inline char *util_device_get_node(drmDevicePtr device,
unsigned int type)+{
- if (type >= DRM_NODE_MAX)
return NULL;IMHO it's better to honour the ::available_nodes bitmask here, since we already check if we're within range.
This was meant to be a very low-level accessor that can be used to implement the iterators rather than be used standalone. If you look at the util_device_for_each_{,available}_node iterators, they make a distinction, though it is admittedly somewhat questionable how useful the iterator over all nodes, available or not, really is.
It's probably best to just drop the util_device_for_each() iterator altogether.
This the above the series is Reviewed-by: Emil Velikov emil.l.velikov@gmail.com
Thanks, Thierry