Sam Ravnborg wrote:
On Thu, Aug 19, 2004 at 03:26:07PM -0700, Erdi Chen wrote:
This patch adds two new ioctl's to the VIA Unichrome/Pro DRM driver: DRM_IOCTL_VIA_DMA_INIT DRM_IOCTL_VIA_CMDBUFFER
The first call sets up an area in AGP memory that will be used as the ring buffer. The second call copies a command buffer from user space memory to the ring buffer.
A few comments to the coding style - nothing to the functionality. [inlining your patch would make it soo much easier to review and comment]
1) Defines are mixed casisng. Usual style is upper case only except if casing match definitions in datasheet.
Defines come from VIA's driver source. I didn't want to make change them.
2) via_dma.c - remove emacs tagging "-*- linux-c -*-
Sin of code copying.
Came from the i915_dma.c that I've used as a template. I have no idea what it is for. It seems to exist in many drm source files.
3) What's the purpose of "#define __NO_VERSION__"
Much of this code came from the DRI userspace code. I didn't want to make too much change so that the two code bases diff too much. It was much easier to debug in userspace. Once the kernel code works reliably, I will work on style cleanup.
4) viaCheckDma => via_check_dma
5) via_dma_cleanup always return 0. No need to return anything. - Also drop extern in prototype in .h file
6) SetRag2DAGP - no mixed casing
- why is vb casted to uint32_t* - vb is of correct type.
- Put the define close to the (sole) user
7) viaAlingBuffer - no mixed casing in name - use tabs for indention
8) via_cmdbuf_wait - no mixed casing in name - use tabs for indention
9) In general use tabs for indention
qwPadCount is the number of quad-words (inherited from windows driver code) to pad the buffer to meet alignment requirement.
10) What doeis this do? qwPadCount = (CMDBUF_ALIGNMENT_SIZE>>3) - ((((uint32_t)vb) & CMDBUF_ALIGNMENT_MASK) >> 3); There is _no_ guarantee a pointer will fir into uint_32 Above construction used in several places.
I will use dev_priv->dmaLow instead.
11) via_drm.h - Drop typedef of drm_via_dma_init - Linux style is to _not_ hide when you work with a struct - Same for drm_via_cmdbuffer_t
Will do.
12) Comment about typedef is also valid for via_drv.h
13) unsigned int dmaLow - I'm suspicious this is OK on 64 bit platforms?
dmaLow is an offset. It is not an address. 32 bit is good enough.
Sam
------------------------------------------------------- SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media 100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33 Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift. http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285 -- _______________________________________________ Dri-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/dri-devel
