[GATOS]Re: [Dri-devel] Radeon DRM memory layout transition

[Michel <mi...@da...>]

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