[Dri-devel] Re: [PATCH] convert radeon_video.c to use the CP
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
---
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive? Does it
help you create better code? SHARE THE LOVE, and help us help
YOU! Click Here: http://sourceforge.net/donate/
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel
[Dri-devel] Re: [PATCH] convert radeon_video.c to use the CP
I would never commit any thing non-trivial with out some validation
first. that's why I posted it here. Thanks for your comments.
Alex
--- Michel D�nzer <[EMAIL PROTECTED]> wrote:
> 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.
>
>
__
Do you Yahoo!?
Free Pop-Up Blocker - Get it now
http://companion.yahoo.com/
---
This SF.net email is sponsored by: SF.net Giveback Program.
Does SourceForge.net help you be more productive? Does it
help you create better code? SHARE THE LOVE, and help us help
YOU! Click Here: http://sourceforge.net/donate/
___
Dri-devel mailing list
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/dri-devel
