On Tue, 18 Feb 2003, [iso-8859-15] José Fonseca wrote:
> On Mon, Feb 17, 2003 at 08:15:02PM -0500, Leif Delgass wrote:
> > As I was doing some minor cleanups in the mach64 drm in the new branch, I
> > made some additional search and replace conversions of the mach64 DRM to
> > the os independence macros (I couldn't restrain myself ;) ). However, I
> > want to share what I've done so far and get some feedback, since there are
> > a couple of issues here:
> >
> > 1. We are currently using access_ok() to check the vertex buffers
> > submitted by the client before copying them to (what will be) private
> > kernel buffers. There was only a macro in drm_os_linux.h for
> > DRM_VERIFYAREA_READ(), so I added DRM_ACCESSOK_READ(). I don't really
> > know the details of what the difference is between verify_area() and
> > access_ok() (Jose added that code based on a suggestion from Linus, I
> > think), but I believe access_ok() is intended as a security check, which
> > is the reason for the copy. It seems that this all maps to the same thing
> > for *BSD at the moment -- i.e. the unchecked macros aren't implemented
> > differently from the checked ones, right?
>
> They seem to be equivalent, onyle the semantics of the return value
> differ. Here is the definition of verify_area in my distribution kernel
> sources (linux-2.4.20-gentoo-r1), in include/asm-i386/uaccess.h:
>
> static inline int verify_area(int type, const void * addr, unsigned long size)
> {
> return access_ok(type,addr,size) ? 0 : -EFAULT;
> }
>
> So perhaps we should converge on one.
OK, I've changed this to use DRM_VERIFYAREA_READ().
I also made some other changes to the copy/verify:
I added a check to the ioctl handler (mach64_dma_vertex) to check that the
vertex buffer has an integral number of dwords (in addition to the check
against MACH64_BUFFER_SIZE). I also changed copy_and_verify_from_user to
return an error code instead of the number of bytes copied. I don't see
any reason to dispatch a buffer unless all checks succeed (in fact it can
potentially cause lockups if we submit a partial buffer), so it's either
all or nothing. If it succeeds then we just copy buf->used to the private
buffer struct. This also allows us to return a more meaningful error
code, e.g. EFAULT for failed verify_area/copy_from_user, EINVAL for bad
command counts or buffer size, and EACCES for disallowed register commands
in the buffer. It also lets us get rid of the 'copied' local var and a
couple of adds inside the loop. A couple of other minor tweaks I made
were copying the byte length parameter to a local var and declaring the
function as inline. It also now will generate an error if there is a
register command without at least one data dword following it (in addition
to checking for a command count that overflows buf->used). That check is
now the new loop condition (n > 1 instead of n (!=0) ), so it doesn't add
anything inside the loop. There is one extra conditional in the loop now
to check the return of copy_from_user. All of this may or may not have
much of a performance effect, but it is a pretty critical code path, I
think.
> > 2. The Mach64 driver makes heavy use of the list struct and macros from
> > linux/list.h. I moved the define for list_for_each_safe() (needed for
> > older 2.4 Linux kernels) from mach64_drv.h to drmP.h, since that has
> > already been added in XFree86 CVS (I think the i8x0 drm uses it now also).
> > I also removed the include of linux/list.h from the mach64 driver, since
> > it already gets included (indirectly?) through the drm headers. However,
> > it looks like an analogue of linux/list.h might need to be added to the
> > BSD drm headers. The only wrinkle there is that it also uses
> > linux/prefetch.h.
FYI Eric, here's what we're currently using from linux/list.h:
struct list_head
INIT_LIST_HEAD()
list_entry()
list_for_each()
list_for_each_safe()
list_del()
list_add_tail()
list_empty()
> > 3. We still need to work out the wrapper/alternative to
> > pci_alloc_consistent() and friends.
>
> I'm still reading Ian texmem-2 proposal and looking to the source code
> to get a hold of this.
Something I have in my old tree that I haven't merged to the new branch
yet is setting up the ring in AGP when available. That will get rid of
the use of pci_alloc_consistent for AGP cards. However we still can't do
private buffers in AGP without multiple buffer lists. However, it might
be enough to allow porting/testing on *BSD to continue with the PCI mode
setup in mach64_dma.c disabled (with the current temporary code that
uses client buffers instead of secure buffers).
Eric, do you have hardware to test on?
> > 4. As I mentioned before, the interrupt code is not converted, but it's
> > currently unused.
>
> My memory is failing: this might still be usefull for Xv, isn't it?
Maybe, maybe not. DMA for XVideo seems to be of questionable value,
judging from the Rage 128 driver. Plus the fact that you have to
wait/sync when switching between GUI masters and system masters. I've
used interrupts to test page flipping with vsync. However, this is also
of questionable utility. Page flipping without vsync doesn't really buy
anything, since it has to be done synchronously (block until the vertical
counter resets). In either case you have to wait for idle before
dispatching the flip. With vsync, I had to modify the DRM to buffer up
commands in the ring after dispatching the flip, and only commit them when
the vblank arrives. Otherwise you get flickering/tearing since mach64 can
only process the flip at vblank (unlike r128/radeon that can flip
asynchronously). One advantage is that it seems to smooth out some of the
stutters in rendering as well as eliminating tearing, but at the cost of
reducing the framerate if the app can't keep up with the vertical refresh.
Most apps on mach64 (other than gears) don't fall into that category. :(
--
Leif Delgass
http://www.retinalburn.net
-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf
_______________________________________________
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel
