>> I'm a little fuzzy on the details, this part of the patch was carried >> over from Jesse's original work. I believe set_base is supposed to be >> called with the struct mutex held. >> >> > > We shouldn't be this sloppy about locking, and in particular we shouldn't > protect a callback with a lock without knowing why. A lock should protect > data, not serialize calls to functions, and it should ideally be documented > somewhere. Why does set_base need the _caller_ to take the struct_mutex > lock? Are there more driver callbacks that need the struct_mutex held? > > I mean if I implement a modesetting driver that takes care to use device > private locks not visible to the caller to avoid polluting the struct_mutex > with possible contention and lockdep errors as a result, I'd still find the > generic code taking the struct_mutex for me becase it assumes that I need > it? This can't be right and should go away.
Good point, I'll take a closer look and figure out what the locking should be like. >> Perhaps TTM bo read/write could use ioctls like the gem pwrite/pread >> ioctls? The only way we can get asynchronous notifications from the >> drm to userspace is through readable events on the drm fd. How were >> you planning to implement read and write from multiple bo's through >> one fd anyway? Reusing the fake offset we use for mmap? I think the >> ioctl approach is cleaner since you can just pass in the object handle >> and the offset into the object. Maybe even add src and dst stride >> arguments. > > It's a common misconception that the TTM bo offsets are fake offsets from > outer space. > The offsets aren't fake offsets but real offsets into the device address > space, so a logical consequence of mapping a part of that address space is > to be able to read and write to the same address space, That is read / write > implemented using syscalls as intended. > > That said, rectangular bo blit ioctls is probably a good idea as well, and > some drivers, like via, already have them. I don't know that we need a separate char device or that read/write was intended for accessing buffer data. We already have lots of different, unrelated ioclts on the fd - I see it more as a IPC mechanism between kernel and userspace, >>> A couple of years ago, any attempt to return anything else than 0 from >>> drm poll resulted in an X server error. >>> http://freedesktop.org/bugzilla/show_bug.cgi?id=1505. The fix mentioned >>> in the bug was actually to return 0 from drm poll, and a comment about >>> this is still present in drm.git. The above breaks drm for old X servers >>> and all drivers, which I think is against drm policy? >>> >> >> That can't be the real problem. The X server polls on a ton of file >> descriptors already; sockets from clients, dbus, input devices. They >> all have poll implementations that don't return 0... I mean, otherwise >> they wouldn't work. Look at evdev_poll() in drivers/input/evdev.c for >> the evdev poll implementation, for example. >> > > You're probably right, but we should probably find out what went wrong and > make sure it doesn't happen again with non-modesetting drivers + dri1 before > pushing this. I really don't think that's necessary. As I wrote in my reply to Dave, there's nothing in this patch that can cause select(2) to return EINVAL that isn't already present in other poll fops implementations. Like the evdev one, which we already select on - please compare that function with the poll implementation in my patch and tell me why the drm poll is cause for concern. I need a better, more specific reason why this is such a risk and why I should spend more time tracking this stuff down. And if select(2), for whatever reason, returns EINVAL because of the drm_poll() fops implementation, that's a bug in the kernel that needs to be fixed. cheers, Kristian ------------------------------------------------------------------------------ Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day trial. Simplify your report design, integration and deployment - and focus on what you do best, core application coding. Discover what's new with Crystal Reports now. http://p.sf.net/sfu/bobj-july -- _______________________________________________ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
