On Tue, 2003-10-21 at 11:06, Keith Whitwell wrote: > > In r200_screen.c, you check for drmMinor >= 10 before issuing the FB_LOCATION > ioctl, but it's not clear what happens if drmMinor < 10 -- will the driver > function correctly? [...]
Yes. The driver determines the memory layout from the chip registers, the ioctl just improves the DRM's ability to check and fix up state. > Also, it seems this patch does two things, one is the fblocation stuff, but > there's also some chipset_id changes relating to the R200_CHIPSET_RS300. I'd > be happier to see these in separate patches. I can commit these parts separately once everybody is happy, but I'll keep them together for now due to http://bugs.xfree86.org/show_bug.cgi?id=314 . > In radeon_state.c, the tests in radeon_emit_packets() are just ugly. The > normal usage for this code on a properly installed system is that > (filp_priv->fboffset == 0), right? So all those tests are going to result in > a noop? Could the tests be pushed into their own functions and shortcircuited > witha single test on (fboffset == 0) ? So I thought first, but then it occurred to me that there's no guarantee that clients use sensible memory offsets at all, so I think it's a good idea always to check them (even for the older ioctls, on second though) to prevent bad things from happening. > Additionally, in those tests, it looks like you are reading back and fixing up > the data on the ring -- ie from uncached memory! Especially when (fboffset == > 0) this is a very wasteful noop... Well, I haven't noticed any significant performance impact, but I can change that I guess. > Finally, and just being picky -- it'd be nice to keep the number of coding > styles fairly minimal. It just looks a little odd to start seeing the spaces > in code like 'tmp[ 0 ]', while the rest of the module is 'tmp[0]' Point taken. It's just that I've grown to like the spaces a lot (partly because of you ;), but they're certainly less important for square brackets. > Would you consider creating a DRM_RADEON_SETPARAM ioctl instead of > DRM_RADEON_FB_LOCATION, with an ioctl struct like: > > #define RADEON_SETPARAM_FB_LOCATION 1 > > typedef struct drm_radeon_setparam { > int param; > int value; > } drm_radeon_setparam_t; > > > There are other int-valued parameters that I can imagine being set in this way. Good idea. Thanks for your suggestions, I'll integrate them and post an updated patch ASAP. -- Earthling Michel Dänzer \ Debian (powerpc), XFree86 and DRI developer Software libre enthusiast \ http://svcs.affero.net/rm.php?r=daenzer ------------------------------------------------------- This SF.net email is sponsored by OSDN developer relations Here's your chance to show off your extensive product knowledge We want to know what you know. Tell us and you have a chance to win $100 http://www.zoomerang.com/survey.zgi?HRPT1X3RYQNC5V4MLNSV3E54 _______________________________________________ Dri-devel mailing list [EMAIL PROTECTED] https://lists.sourceforge.net/lists/listinfo/dri-devel
