[Intel-gfx] [PATCH] Use ALIGN macro instead of open coding it.

Matt Turner mattst88 at gmail.com
Fri Mar 12 06:36:09 CET 2010


This patch exposes some WTFs, as noted inline.

On Fri, Mar 12, 2010 at 12:30 AM, Matt Turner <mattst88 at gmail.com> wrote:
> Also, kill off ROUND_TO, ROUND_DOWN, ROUND_TO_PAGE, and ROUND_TO_MB
> macros. ROUND_{TO,DOWN} are only useful if rounding to a non-power of
> two, which they always were, and ROUND_TO_PAGE and ROUND_TO_MB
> inherently round to a power of two, so just use ALIGN.
>
> Also, kill off i830_pad_drawable_width.
>
> CC: Eric Anholt <eric at anholt.net>
> CC: Carl Worth <cworth at cworth.org>
> Signed-off-by: Matt Turner <mattst88 at gmail.com>
> ---
>  src/common.h          |    4 ----
>  src/drmmode_display.c |    7 +++----
>  src/i810_accel.c      |    2 +-
>  src/i810_dri.c        |    7 +++----
>  src/i810_driver.c     |    3 +--
>  src/i810_video.c      |   18 +++++++++---------
>  src/i830.h            |    2 --
>  src/i830_driver.c     |   13 ++-----------
>  src/i830_memory.c     |    4 ++--
>  src/i830_uxa.c        |    4 ++--
>  src/i830_video.c      |   38 +++++++++++++++++---------------------
>  11 files changed, 40 insertions(+), 62 deletions(-)
>
> diff --git a/src/common.h b/src/common.h
> index bc5d722..05041a3 100644
> --- a/src/common.h
> +++ b/src/common.h
> @@ -395,10 +395,6 @@ extern int I810_DEBUG;
>  #define SUPPORTS_YTILING(pI810) (IS_I965G(intel))
>
>  #define GTT_PAGE_SIZE                  KB(4)
> -#define ROUND_TO(x, y)                 (((x) + (y) - 1) / (y) * (y))
> -#define ROUND_DOWN_TO(x, y)            ((x) / (y) * (y))
> -#define ROUND_TO_PAGE(x)               ROUND_TO((x), GTT_PAGE_SIZE)
> -#define ROUND_TO_MB(x)                 ROUND_TO((x), MB(1))
>  #define PRIMARY_RINGBUFFER_SIZE                KB(128)
>  #define MIN_SCRATCH_BUFFER_SIZE                KB(16)
>  #define MAX_SCRATCH_BUFFER_SIZE                KB(64)
> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 22c967a..a8b5df2 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -476,7 +476,7 @@ drmmode_crtc_shadow_allocate(xf86CrtcPtr crtc, int width, int height)
>        int size, ret;
>        unsigned long rotate_pitch;
>
> -       width = i830_pad_drawable_width(width, drmmode->cpp);
> +       width = ALIGN(width, 64);
>        rotate_pitch = width * drmmode->cpp;
>        size = rotate_pitch * height;
>
> @@ -523,8 +523,7 @@ drmmode_crtc_shadow_create(xf86CrtcPtr crtc, void *data, int width, int height)
>                }
>        }
>
> -       rotate_pitch =
> -               i830_pad_drawable_width(width, drmmode->cpp) * drmmode->cpp;
> +       rotate_pitch = ALIGN(width, 64) * drmmode->cpp;
>        rotate_pixmap = GetScratchPixmapHeader(scrn->pScreen,
>                                               width, height,
>                                               scrn->depth,
> @@ -1261,7 +1260,7 @@ drmmode_xf86crtc_resize (ScrnInfoPtr scrn, int width, int height)
>        if (scrn->virtualX == width && scrn->virtualY == height)
>                return TRUE;
>
> -       pitch = i830_pad_drawable_width(width, intel->cpp);
> +       pitch = ALIGN(width, 64);
>        xf86DrvMsg(scrn->scrnIndex, X_INFO,
>                   "Allocate new frame buffer %dx%d stride %d\n",
>                   width, height, pitch);
> diff --git a/src/i810_accel.c b/src/i810_accel.c
> index ae4a654..9aa3e42 100644
> --- a/src/i810_accel.c
> +++ b/src/i810_accel.c
> @@ -129,7 +129,7 @@ I810AccelInit(ScreenPtr pScreen)
>     */
>    if (pI810->Scratch.Size != 0) {
>       int i;
> -      int width = ((pScrn->displayWidth + 31) & ~31) / 8;
> +      int width = ALIGN(pScrn->displayWidth, 32) / 8;
>       int nr_buffers = pI810->Scratch.Size / width;
>       unsigned char *ptr = pI810->FbBase + pI810->Scratch.Start;
>
> diff --git a/src/i810_dri.c b/src/i810_dri.c
> index e566acf..cd73867 100644
> --- a/src/i810_dri.c
> +++ b/src/i810_dri.c
> @@ -348,9 +348,8 @@ I810DRIScreenInit(ScreenPtr pScreen)
>    pDRIInfo->ddxDriverMinorVersion = I810_MINOR_VERSION;
>    pDRIInfo->ddxDriverPatchVersion = I810_PATCHLEVEL;
>    pDRIInfo->frameBufferPhysicalAddress = (pointer) pI810->LinearAddr;
> -   pDRIInfo->frameBufferSize = (((pScrn->displayWidth *
> -                                 pScrn->virtualY * pI810->cpp) +
> -                                4096 - 1) / 4096) * 4096;
> +   pDRIInfo->frameBufferSize = ALIGN(pScrn->displayWidth * pScrn->virtualY *
> +                                       pI810->cpp, 4096);
>
>    pDRIInfo->frameBufferStride = pScrn->displayWidth * pI810->cpp;
>    pDRIInfo->ddxDrawableTableEntry = I810_MAX_DRAWABLES;
> @@ -537,7 +536,7 @@ I810DRIScreenInit(ScreenPtr pScreen)
>        *  - airlied */
>       int lines = (pScrn->virtualY + 15) / 16 * 16;
>       back_size = i810_pitches[pitch_idx] * lines;
> -      back_size = ((back_size + 4096 - 1) / 4096) * 4096;
> +      back_size = ALIGN(back_size, 4096);
>    }
>
>    sysmem_size = pScrn->videoRam * 1024;
> diff --git a/src/i810_driver.c b/src/i810_driver.c
> index 3109834..34a4413 100644
> --- a/src/i810_driver.c
> +++ b/src/i810_driver.c
> @@ -1838,8 +1838,7 @@ I810AllocateFront(ScrnInfoPtr pScrn)
>
>    if (!I810AllocLow(&(pI810->FrontBuffer),
>                     &(pI810->SysMem),
> -                    ((pI810->FbMemBox.x2 *
> -                      pI810->FbMemBox.y2 * pI810->cpp) + 4095) & ~4095)) {
> +                    ALIGN(pI810->FbMemBox.x2 * pI810->FbMemBox.y2 * pI810->cpp, 4096))) {
>       xf86DrvMsg(pScrn->scrnIndex,
>                 X_WARNING, "Framebuffer allocation failed\n");
>       return FALSE;
> diff --git a/src/i810_video.c b/src/i810_video.c
> index 9bb9870..5f82392 100644
> --- a/src/i810_video.c
> +++ b/src/i810_video.c
> @@ -750,14 +750,14 @@ I810DisplayVideo(
>     switch(id) {
>     case FOURCC_YV12:
>     case FOURCC_I420:
> -       swidth = (width + 7) & ~7;
> +       swidth = ALIGN(width, 8);
>        overlay->SWID = (swidth << 15) | swidth;
>        overlay->SWIDQW = (swidth << 12) | (swidth >> 3);
>        break;
>     case FOURCC_UYVY:
>     case FOURCC_YUY2:
>     default:
> -       swidth = ((width + 3) & ~3) << 1;
> +       swidth = ALIGN(width, 4) << 1;
>        overlay->SWID = swidth;
>        overlay->SWIDQW = swidth >> 3;
>        break;
> @@ -1013,15 +1013,15 @@ I810PutImage(
>     switch(id) {
>     case FOURCC_YV12:
>     case FOURCC_I420:
> -        srcPitch = (width + 3) & ~3;
> -        dstPitch = ((width >> 1) + 7) & ~7;  /* of chroma */
> +        srcPitch = ALIGN(width, 4);
> +        dstPitch = ALIGN(width >> 1, 8);  /* of chroma */
>         size =  dstPitch * height * 3;
>         break;
>     case FOURCC_UYVY:
>     case FOURCC_YUY2:
>     default:
>         srcPitch = (width << 1);
> -        dstPitch = (srcPitch + 7) & ~7;
> +        dstPitch = ALIGN(srcPitch, 8);
>         size = dstPitch * height;
>         break;
>     }
> @@ -1062,13 +1062,13 @@ I810PutImage(
>     /* copy data */
>     top = y1 >> 16;
>     left = (x1 >> 16) & ~1;
> -    npixels = ((((x2 + 0xffff) >> 16) + 1) & ~1) - left;
> +    npixels = (ALIGN((x2 + 0xffff) >> 16, 2)) - left;
>
>     switch(id) {
>     case FOURCC_YV12:
>     case FOURCC_I420:
>        top &= ~1;
> -       nlines = ((((y2 + 0xffff) >> 16) + 1) & ~1) - top;
> +       nlines = (ALIGN((y2 + 0xffff) >> 16, 2)) - top;
>        I810CopyPlanarData(pScrn, buf, srcPitch, dstPitch,  height, top, left,
>                            nlines, npixels, id);
>        break;
> @@ -1213,8 +1213,8 @@ I810AllocateSurface(
>     if((w > 1024) || (h > 1024))
>        return BadAlloc;
>
> -    w = (w + 1) & ~1;
> -    pitch = ((w << 1) + 15) & ~15;
> +    w = ALIGN(w, 2);
> +    pitch = ALIGN((w << 1), 16);
>     bpp = pScrn->bitsPerPixel >> 3;
>     size = ((pitch * h) + bpp - 1) / bpp;
>
> diff --git a/src/i830.h b/src/i830.h
> index 7593cde..5c76c0a 100644
> --- a/src/i830.h
> +++ b/src/i830.h
> @@ -429,8 +429,6 @@ void i830_init_bufmgr(ScrnInfoPtr scrn);
>
>  Bool i830_tiled_width(intel_screen_private *intel, int *width, int cpp);
>
> -int i830_pad_drawable_width(int width, int cpp);
> -
>  /* i830_memory.c */
>  unsigned long i830_get_fence_size(intel_screen_private *intel, unsigned long size);
>  unsigned long i830_get_fence_pitch(intel_screen_private *intel, unsigned long pitch,
> diff --git a/src/i830_driver.c b/src/i830_driver.c
> index 53d8663..d9f2d2b 100644
> --- a/src/i830_driver.c
> +++ b/src/i830_driver.c
> @@ -172,7 +172,7 @@ typedef enum {
>    OPTION_DEBUG_FLUSH_BATCHES,
>    OPTION_DEBUG_FLUSH_CACHES,
>    OPTION_DEBUG_WAIT,
> -} I830Opts;
> +}I830Opts;
>
>  static OptionInfoRec I830Options[] = {
>    {OPTION_DRI,                "DRI",          OPTV_BOOLEAN,   {0},    TRUE},
> @@ -389,14 +389,6 @@ Bool i830_tiled_width(intel_screen_private *intel, int *width, int cpp)
>  }
>
>  /*
> - * Pad to accelerator requirement
> - */
> -int i830_pad_drawable_width(int width, int cpp)
> -{
> -       return (width + 63) & ~63;
> -}

Why did this function take two arguments and then only use one? What
is cpp? Should it actually be aligning to cpp? I've replaced calls to
this function with ALIGN(width, 64) throughout the code to stay
consistent.

> -
> -/*
>  * DRM mode setting Linux only at this point... later on we could
>  * add a wrapper here.
>  */
> @@ -1100,8 +1092,7 @@ I830ScreenInit(int scrnIndex, ScreenPtr screen, int argc, char **argv)
>        struct pci_device *const device = intel->PciInfo;
>        int fb_bar = IS_I9XX(intel) ? 2 : 0;
>
> -       scrn->displayWidth =
> -           i830_pad_drawable_width(scrn->virtualX, intel->cpp);
> +       scrn->displayWidth = ALIGN(scrn->virtualX, intel->cpp);
>
>        /*
>         * The "VideoRam" config file parameter specifies the maximum amount of
> diff --git a/src/i830_memory.c b/src/i830_memory.c
> index a21f29b..ba47714 100644
> --- a/src/i830_memory.c
> +++ b/src/i830_memory.c
> @@ -129,7 +129,7 @@ i830_get_fence_pitch(intel_screen_private *intel, unsigned long pitch,
>
>        /* 965 is flexible */
>        if (IS_I965G(intel))
> -               return ROUND_TO(pitch, tile_width);
> +               return ALIGN(pitch, tile_width);
>
>        /* Pre-965 needs power of two tile width */
>        for (i = tile_width; i < pitch; i <<= 1) ;
> @@ -232,7 +232,7 @@ drm_intel_bo *i830_allocate_framebuffer(ScrnInfoPtr scrn)
>         */
>        fb_height = scrn->virtualY;
>
> -       size = ROUND_TO_PAGE(pitch * fb_height);
> +       size = ALIGN(pitch * fb_height, GTT_PAGE_SIZE);
>
>        if (intel->tiling && IsTileable(scrn, pitch))
>                tiling_mode = I915_TILING_X;
> diff --git a/src/i830_uxa.c b/src/i830_uxa.c
> index 9904311..8fc9cc9 100644
> --- a/src/i830_uxa.c
> +++ b/src/i830_uxa.c
> @@ -133,7 +133,7 @@ i830_uxa_pixmap_compute_size(PixmapPtr pixmap,
>        if (*tiling != I915_TILING_NONE) {
>                /* First check whether tiling is necessary. */
>                pitch_align = intel->accel_pixmap_pitch_alignment;
> -               size = ROUND_TO((w * pixmap->drawable.bitsPerPixel + 7) / 8,
> +               size = ALIGN((w * pixmap->drawable.bitsPerPixel + 7) / 8,
>                                pitch_align) * ALIGN (h, 2);
>                if (size < 4096)
>                        *tiling = I915_TILING_NONE;
> @@ -146,7 +146,7 @@ i830_uxa_pixmap_compute_size(PixmapPtr pixmap,
>                pitch_align = 512;
>        }
>
> -       *stride = ROUND_TO((w * pixmap->drawable.bitsPerPixel + 7) / 8,
> +       *stride = ALIGN((w * pixmap->drawable.bitsPerPixel + 7) / 8,
>                           pitch_align);
>
>        if (*tiling == I915_TILING_NONE) {
> diff --git a/src/i830_video.c b/src/i830_video.c
> index db72863..0c76908 100644
> --- a/src/i830_video.c
> +++ b/src/i830_video.c
> @@ -1271,10 +1271,10 @@ i830_clip_video_helper(ScrnInfoPtr scrn,
>
>        *top = y1 >> 16;
>        *left = (x1 >> 16) & ~1;
> -       *npixels = ((((x2 + 0xffff) >> 16) + 1) & ~1) - *left;
> +       *npixels = ALIGN((x2 + 0xffff) >> 16, 2) - *left;
>        if (is_planar_fourcc(id)) {
>                *top &= ~1;
> -               *nlines = ((((y2 + 0xffff) >> 16) + 1) & ~1) - *top;
> +               *nlines = ALIGN((y2 + 0xffff) >> 16, 2) - *top;
>        } else
>                *nlines = ((y2 + 0xffff) >> 16) - *top;
>
> @@ -1351,24 +1351,24 @@ i830_setup_dst_params(ScrnInfoPtr scrn, intel_adaptor_private *adaptor_priv, sho
>                        int id)
>  {
>        intel_screen_private *intel = intel_get_screen_private(scrn);
> -       int pitchAlignMask;
> +       int pitchAlign;
>
>        /* Only needs to be DWORD-aligned for textured on i915, but overlay has
>         * stricter requirements.
>         */
>        if (adaptor_priv->textured) {
> -               pitchAlignMask = 3;
> +               pitchAlign = 4;
>        } else {
>                if (IS_I965G(intel))
> -                       pitchAlignMask = 255;
> +                       pitchAlign = 256;
>                else
> -                       pitchAlignMask = 255;
> +                       pitchAlign = 256;
>        }

Why do we have two branches that do the same thing? Was one supposed
to actually do something different?

>
>  #if INTEL_XVMC
>        /* for i915 xvmc, hw requires 1kb aligned surfaces */
>        if ((id == FOURCC_XVMC) && IS_I915(intel))
> -               pitchAlignMask = 0x3ff;
> +               pitchAlign = 1024;
>  #endif
>
>        /* Determine the desired destination pitch (representing the chroma's pitch,
> @@ -1377,25 +1377,25 @@ i830_setup_dst_params(ScrnInfoPtr scrn, intel_adaptor_private *adaptor_priv, sho
>        if (is_planar_fourcc(id)) {
>                if (adaptor_priv->rotation & (RR_Rotate_90 | RR_Rotate_270)) {
>                        *dstPitch =
> -                           ((height / 2) + pitchAlignMask) & ~pitchAlignMask;
> +                           ALIGN(height / 2, pitchAlign);
>                        *dstPitch2 =
> -                           (height + pitchAlignMask) & ~pitchAlignMask;
> +                           ALIGN(height, pitchAlign);
>                        *size = *dstPitch * width * 3;
>                } else {
>                        *dstPitch =
> -                           ((width / 2) + pitchAlignMask) & ~pitchAlignMask;
> +                           ALIGN(width / 2, pitchAlign);
>                        *dstPitch2 =
> -                           (width + pitchAlignMask) & ~pitchAlignMask;
> +                           ALIGN(width, pitchAlign);
>                        *size = *dstPitch * height * 3;
>                }
>        } else {
>                if (adaptor_priv->rotation & (RR_Rotate_90 | RR_Rotate_270)) {
>                        *dstPitch =
> -                           ((height << 1) + pitchAlignMask) & ~pitchAlignMask;
> +                           ALIGN(height << 1, pitchAlign);
>                        *size = *dstPitch * width;
>                } else {
>                        *dstPitch =
> -                           ((width << 1) + pitchAlignMask) & ~pitchAlignMask;
> +                           ALIGN(width << 1, pitchAlign);
>                        *size = *dstPitch * height;
>                }
>                *dstPitch2 = 0;
> @@ -1426,16 +1426,9 @@ i830_copy_video_data(ScrnInfoPtr scrn, intel_adaptor_private *adaptor_priv,
>                     int top, int left, int npixels, int nlines,
>                     int id, unsigned char *buf)
>  {
> -       int srcPitch = 0, srcPitch2 = 0;
> +       int srcPitch, srcPitch2;

Just as clarification, these variables don't need to be initialized
since they're not read before being written to. I imagine gcc spit out
a warning because (1) they're written in branches, and then (2) read
in branches. Apparently what it couldn't tell was that the conditions
for those branches were identical. So I just merged them together.
Cleaner, simpler, and doesn't confuse gcc.

>        int size;
>
> -       if (is_planar_fourcc(id)) {
> -               srcPitch = (width + 0x3) & ~0x3;
> -               srcPitch2 = ((width >> 1) + 0x3) & ~0x3;
> -       } else {
> -               srcPitch = width << 1;
> -       }
> -
>        i830_setup_dst_params(scrn, adaptor_priv, width, height, dstPitch,
>                                dstPitch2, &size, id);
>
> @@ -1444,11 +1437,14 @@ i830_copy_video_data(ScrnInfoPtr scrn, intel_adaptor_private *adaptor_priv,
>
>        /* copy data */
>        if (is_planar_fourcc(id)) {
> +               srcPitch = ALIGN(width, 0x4);
> +               srcPitch2 = ALIGN(width >> 1, 0x4);
>                I830CopyPlanarData(adaptor_priv, buf, srcPitch, srcPitch2,
>                                   *dstPitch, *dstPitch2,
>                                   height, top, left, nlines,
>                                   npixels, id);
>        } else {
> +               srcPitch = width << 1;
>                I830CopyPackedData(adaptor_priv, buf, srcPitch, *dstPitch, top, left,
>                                   nlines, npixels);
>        }
> --
> 1.6.4.4



More information about the Intel-gfx mailing list