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
