On Mon, 2003-11-24 at 00:31, Alex Deucher wrote:
> The attached patch converts radeon_video.c to use the CP rather than
> MMIO if direct rendering is enabled. I wanted to get some feedback
> before committing to DRI cvs. I doubt this will affect overall
> performance much, but it will reduce the overhead involved in stalling
> the CP when switching between MMIO and the CP.
I still don't really see the point, but I do see problems:
> @@ -255,6 +260,11 @@
> CARD32 dwOvGCb, dwOvGCr;
> CARD32 dwOvBCb, dwOvBCr;
>
> +#ifdef ACCEL_CP
> + if (info->directRenderingEnabled)
> + RING_LOCALS;
> +#endif
> +
> if (ref >= 2)
> return;
>
ACCEL_CP is only defined in radeon_accel.c for radeon_accelfuncs.c; you
basically added commented out code, you'd have to use #ifdef XF86DRI
instead. I'm not sure using RING_LOCALS like this would even build
though, much less work as intended. I must say it's a bit worrying that
you don't seem to give even the most basic testing (does it get compiled
in at all?) to new code.
> @@ -370,9 +400,20 @@
> min = (r << 16) | (g << 8) | (b);
> max = (0xff << 24) | (r << 16) | (g << 8) | (b);
>
> - RADEONWaitForFifo(pScrn, 2);
> - OUTREG(RADEON_OV0_GRAPHICS_KEY_CLR_HIGH, max);
> - OUTREG(RADEON_OV0_GRAPHICS_KEY_CLR_LOW, min);
> +#ifdef ACCEL_CP
> + if (info->directRenderingEnabled) {
> + BEGIN_RING(2);
> + OUT_RING_REG(RADEON_OV0_GRAPHICS_KEY_CLR_HIGH, max);
> + OUT_RING_REG(RADEON_OV0_GRAPHICS_KEY_CLR_LOW, min);
> + ADVANCE_RING();
> + } else {
> +#endif
> + RADEONWaitForFifo(pScrn, 2);
> + OUTREG(RADEON_OV0_GRAPHICS_KEY_CLR_HIGH, max);
> + OUTREG(RADEON_OV0_GRAPHICS_KEY_CLR_LOW, min);
> +#ifdef ACCEL_CP
> + }
> +#endif
> }
>
> void
OUT_RING_REG() writes two words to the ring, so BEGIN_RING() needs twice
the number of register writes as an argument.
> @@ -981,14 +1085,26 @@
>
> left = (left >> 16) & 7;
>
> - RADEONWaitForFifo(pScrn, 2);
> - OUTREG(RADEON_OV0_REG_LOAD_CNTL, 1);
> - if (info->accelOn) info->accel->Sync(pScrn);
> - while(!(INREG(RADEON_OV0_REG_LOAD_CNTL) & (1 << 3)));
> -
> - RADEONWaitForFifo(pScrn, 14);
> - OUTREG(RADEON_OV0_H_INC, h_inc | ((h_inc >> 1) << 16));
> - OUTREG(RADEON_OV0_STEP_BY, step_by | (step_by << 8));
> +#ifdef ACCEL_CP
> + if (info->directRenderingEnabled) {
> + BEGIN_RING(3);
> + OUT_RING_REG(RADEON_OV0_REG_LOAD_CNTL, 1);
> + OUT_RING_REG(RADEON_OV0_H_INC, h_inc | ((h_inc >> 1) << 16));
> + OUT_RING_REG(RADEON_OV0_STEP_BY, step_by | (step_by << 8));
> + ADVANCE_RING();
> + } else {
> +#endif
> + RADEONWaitForFifo(pScrn, 2);
> + OUTREG(RADEON_OV0_REG_LOAD_CNTL, 1);
> + if (info->accelOn) info->accel->Sync(pScrn);
> + while(!(INREG(RADEON_OV0_REG_LOAD_CNTL) & (1 << 3)));
> +
> + RADEONWaitForFifo(pScrn, 14);
> + OUTREG(RADEON_OV0_H_INC, h_inc | ((h_inc >> 1) << 16));
> + OUTREG(RADEON_OV0_STEP_BY, step_by | (step_by << 8));
> +#ifdef ACCEL_CP
> + }
> +#endif
>
> x_off = 8;
> y_off = 0;
I'm not sure you can just get rid of the register read like this, I
think it's there to make sure that double buffering works correctly.
Even ignoring these problems, it's kind of ugly in this form; some kind
of unification scheme like in radeon_accelfuncs.c would be better. So
it's back to the drawing board I'm afraid.
--
Earthling Michel DÃnzer | Debian (powerpc), X and DRI developer
Software libre enthusiast | http://svcs.affero.net/rm.php?r=daenzer
_______________________________________________
Devel mailing list
[EMAIL PROTECTED]
http://XFree86.Org/mailman/listinfo/devel
