[Intel-gfx] [PATCH 2/4] drm: Add dispatcher and driver identification for DRM

[Patrik Jakobsson]

On Sat, Jun 13, 2015 at 1:41 AM, Dmitry V. Levin <ldv at altlinux.org> wrote:
> On Thu, Jun 11, 2015 at 04:11:49PM +0200, Patrik Jakobsson wrote:
>> On Thu, Jun 11, 2015 at 02:26:59AM +0300, Dmitry V. Levin wrote:
>> > On Wed, Jun 10, 2015 at 01:52:33PM +0200, Patrik Jakobsson wrote:
>> > > On Wed, Jun 10, 2015 at 01:14:20AM +0300, Dmitry V. Levin wrote:
>> > > > On Tue, Jun 09, 2015 at 01:26:42PM +0200, Patrik Jakobsson wrote:
>> > [...]
>> > > > > +#define DRM_MAX_NAME_LEN 128
>> > > > > +
>> > > > > +inline int drm_is_priv(const unsigned int num)
>> > > > > +{
>> > > > > +     return (_IOC_NR(num) >= DRM_COMMAND_BASE &&
>> > > > > +             _IOC_NR(num) < DRM_COMMAND_END);
>> > > > > +}
>> > > > > +
>> > > > > +static int drm_get_driver_name(struct tcb *tcp, char *name, size_t bufsize)
>> > > > > +{
>> > > > > +     char path[PATH_MAX];
>> > > > > +     char link[PATH_MAX];
>> > > > > +     int ret;
>> > > > > +
>> > > > > +     ret = getfdpath(tcp, tcp->u_arg[0], path, PATH_MAX - 1);
>> > > > > +     if (!ret)
>> > > > > +             return ret;
>> > > > > +
>> > > > > +     snprintf(link, PATH_MAX, "/sys/class/drm/%s/device/driver",
>> > > > > +              basename(path));
>> > > > > +
>> > > > > +     ret = readlink(link, path, PATH_MAX - 1);
>> > > > > +     if (ret < 0)
>> > > > > +             return ret;
>> > > > > +
>> > > > > +     path[ret] = '\0';
>> > > > > +     strncpy(name, basename(path), bufsize);
>> > > > > +
>> > > > > +     return 0;
>> > > > > +}
>> > > > > +
>> > > > > +int drm_is_driver(struct tcb *tcp, const char *name)
>> > > > > +{
>> > > > > +     char drv[DRM_MAX_NAME_LEN];
>> > > > > +     int ret;
>> > > > > +
>> > > > > +     ret = drm_get_driver_name(tcp, drv, DRM_MAX_NAME_LEN);
>> > > > > +     if (ret)
>> > > > > +             return 0;
>> > > > > +
>> > > > > +     return strcmp(name, drv) == 0;
>> > > > > +}
>> > > >
>> > > > This interface will result to several getfdpath() calls per
>> > > > ioctl_decode().  If the only purpose of drm_is_driver() is to help finding
>> > > > the most appropriate function, let's create a table of pairs
>> > > > {driver name, function} and pass this table to a function that will do a
>> > > > single getfdpath() call, calculate the driver name, and choose the right
>> > > > function from the table.
>> > >
>> > > Yes I was thinking the same thing but it's a bit tricky. What I need is:
>> > > fd -> path -> driver name. And fd -> path could change between ioctls. It is not
>> > > a likely scenario but it's possible. I could get rid of the extra call in
>> > > drm_decode_number() if I put it back in drm_ioctl as in my RFC. I could also
>> > > optimize path -> driver name with a table but I don't know how expensive those
>> > > calls actually are. Not sure what would be the best solution here.
>> >
>> > drm_get_driver_name() is quite expensive as it does two readlink syscalls,
>> > so it should be called at most once per ioctl_decode().
>> >
>> > Another method to achieve this is to change drm_get_driver_name() to return
>> > basename(path) instead of return code, so that drm_ioctl() would call it
>> > once and pass the result to strcmp calls:
>> >
>> > int
>> > drm_ioctl(struct tcb *tcp, const unsigned int code, long arg)
>> > {
>> >     if (verbose(tcp) && drm_is_priv(code)) {
>> >             const char *driver = drm_get_driver_name(tcp);
>> >             if (!driver)
>> >                     return 0;
>> >             if (!strcmp(driver, "i915"))
>> >                     return drm_i915_ioctl(tcp, code, arg);
>> >     }
>> >     return 0;
>> > }
>>
>> I misunderstood you. As you say, we shouldn't do a drm_get_driver_name() more
>> than once (no matter how many drm drivers we are looking for) so your suggestion
>> above would fix that. I was thinking about how to get rid of the extra call in
>> drm_decode_number() (if we could somehow squash them together). But that would
>> make things rather ugly. If ok with you we could just have the same approach in
>> drm_decode_number() as above and live with the fact that we get two calls to
>> drm_get_driver_name() per DRM device specific ioctl. One for drm_decode_number()
>> and one for drm_ioctl().
>
> This way we would end up with three drm_get_driver_name() calls per ioctl:
> twice on entering syscall and once on exiting.  Maybe we could cache
> result of the first of these three calls somewhere?
>

How about adding a "void *private" field to struct tcb. That way any
syscall can store additional data across the life of the tcb.

>
> --
> ldv
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>