Re: [PATCH v3 6/6] qxl-wddm-dod: Non-forced memory allocations with VSync

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> 
> In case of VSync is active (for the driver this means it shall take
> in account watchdog policy and ensure fast execution of PresentDisplayOnly
> callback) allocate bitmaps for drawable objects using non-forced requests.
> If immediate allocation is not possible, place entire bitmap into memory
> chunk allocated from the OS.
> If bitmap is allocated from device memory, but one of later
> chunks can't be allocated, allocate this and further chunks from
> OS memory. All these 'delayed' allocations placed into linked list
> which root entry is part of QXLOutput structure.
> From separate thread, before sending drawable objects down, review
> the list of delayed chunks and allocate device memory (forced) to
> all of them.
> The cost of solution is 2 pointers added to each drawable or cursor
> object.
> Cursor commands currently do not use them; in future we have an option
> to offload also cursor commands.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@xxxxxxxxxx>
> ---
>  qxldod/QxlDod.cpp | 108
>  ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  qxldod/QxlDod.h   |   3 ++
>  2 files changed, 103 insertions(+), 8 deletions(-)
> 
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index c832c93..7ff903a 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -4224,6 +4224,13 @@ void QxlDevice::DrawableAddRes(QXLDrawable *drawable,
> Resource *res)
>      AddRes(output, res);
>  }
>  
> +static FORCEINLINE PLIST_ENTRY DelayedList(QXLDrawable *pd)
> +{
> +    QXLOutput *output;
> +    output = (QXLOutput *)((UINT8 *)pd - sizeof(QXLOutput));
> +    return &output->list;
> +}
> +

This hacky code should just not exist. This assumes that before
every Drawable there is a QXLOutput.
I know there are already similar code but would be better to
use a structure that contains a QXLOutput and a QXLDrawable and
use that.

>  void QxlDevice::CursorCmdAddRes(QXLCursorCmd *cmd, Resource *res)
>  {
>      PAGED_CODE();
> @@ -4331,6 +4338,7 @@ QXLDrawable *QxlDevice::Drawable(UINT8 type, CONST RECT
> *area, CONST RECT *clip,
>      drawable->surfaces_dest[1] = - 1;
>      drawable->surfaces_dest[2] = -1;
>      CopyRect(&drawable->bbox, area);
> +    InitializeListHead(DelayedList(drawable));
>  

Use a constructor?

>      if (!SetClip(clip, drawable)) {
>          DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: set clip failed\n",
>          __FUNCTION__));
> @@ -4425,7 +4433,7 @@ BOOLEAN QxlDevice::AttachNewBitmap(QXLDrawable
> *drawable, UINT8 *src, UINT8 *src
>      Resource *image_res;
>      InternalImage *internal;
>      QXLDataChunk *chunk;
> -    PLIST_ENTRY pDelayedList = NULL;
> +    PLIST_ENTRY pDelayedList = bForce ? NULL : DelayedList(drawable);
>      UINT8* dest, *dest_end;
>  
>      height = drawable->u.copy.src_area.bottom;
> @@ -4494,9 +4502,12 @@ BOOLEAN QxlDevice::AttachNewBitmap(QXLDrawable
> *drawable, UINT8 *src, UINT8 *src
>      }
>  
>      for (; src != src_end; src -= pitch, alloc_size -= line_size) {
> -        BOOLEAN b = PutBytesAlign(&chunk, &dest, &dest_end, src, line_size,
> alloc_size, pDelayedList);
> -        if (!b) {
> -            DbgPrint(TRACE_LEVEL_WARNING, ("%s: aborting copy of lines\n",
> __FUNCTION__));
> +        if (!PutBytesAlign(&chunk, &dest, &dest_end, src, line_size,
> alloc_size, pDelayedList)) {
> +            if (pitch < 0 && bForce) {
> +                DbgPrint(TRACE_LEVEL_WARNING, ("%s: aborting copy of lines
> (forced)\n", __FUNCTION__));
> +            } else {
> +                DbgPrint(TRACE_LEVEL_WARNING, ("%s: unexpected aborting copy
> of lines (force %d, pitch %d)\n", __FUNCTION__, bForce, pitch));
> +            }

Why the if on the pitch and not just this last version?

>              return FALSE;
>          }
>      }
> @@ -4551,7 +4562,13 @@ QXLDrawable *QxlDevice::PrepareBltBits (
>      UINT8* src_end = src - pSrc->Pitch;
>      src += pSrc->Pitch * (height - 1);
>  
> -    if (!AttachNewBitmap(drawable, src, src_end, (INT)pSrc->Pitch, TRUE)) {
> +    if (!AttachNewBitmap(drawable, src, src_end, (INT)pSrc->Pitch,
> !g_bSupportVSync)) {
> +        PLIST_ENTRY pDelayedList = DelayedList(drawable);
> +        // if some delayed chunks were allocated, free them
> +        while (!IsListEmpty(pDelayedList)) {
> +            QXL_DELAYED_CHUNK *pdc = (QXL_DELAYED_CHUNK
> *)RemoveHeadList(pDelayedList);
> +            delete[] reinterpret_cast<BYTE*>(pdc);
> +        }

destructor?

>          ReleaseOutput(drawable->release_info.id);

Why ReleaseOutput is not releasing the linked list, which is
in it?

>          drawable = NULL;
>      } else {
> @@ -5179,11 +5196,76 @@ void QxlDevice::StopPresentThread()
>      }
>  }
>  
> +QXLDataChunk *QxlDevice::MakeChunk(QXL_DELAYED_CHUNK *pdc)
> +{
> +    PAGED_CODE();
> +    QXLDataChunk *chunk = (QXLDataChunk *)AllocMem(MSPACE_TYPE_VRAM,
> pdc->chunk.data_size + sizeof(QXLDataChunk), TRUE);
> +    if (chunk)
> +    {
> +        chunk->data_size = pdc->chunk.data_size;
> +        chunk->next_chunk = 0;
> +        RtlCopyMemory(chunk->data, pdc->chunk.data, chunk->data_size);
> +    }
> +    return chunk;
> +}
> +
> +ULONG QxlDevice::PrepareDrawable(QXLDrawable*& drawable)
> +{
> +    PAGED_CODE();
> +    ULONG n = 0;
> +    BOOLEAN bFail;
> +    PLIST_ENTRY pe = DelayedList(drawable);
> +    QXLDataChunk *chunk, *lastchunk = NULL;
> +
> +    bFail = !m_bActive;
> +
> +    while (!IsListEmpty(pe)) {
> +        QXL_DELAYED_CHUNK *pdc = (QXL_DELAYED_CHUNK *)RemoveHeadList(pe);
> +        if (!lastchunk) {
> +            lastchunk = (QXLDataChunk *)pdc->chunk.prev_chunk;
> +        }
> +        if (!bFail && !lastchunk) {
> +            // bitmap was not allocated, this is single delayed chunk
> +            QXL_ASSERT(IsListEmpty(pe));
> +
> +            if (AttachNewBitmap(
> +                drawable,
> +                pdc->chunk.data,
> +                pdc->chunk.data + pdc->chunk.data_size,
> +                -(drawable->u.copy.src_area.right * 4),
> +                TRUE)) {
> +                ++n;
> +            } else {
> +                bFail = TRUE;
> +            }
> +        }
> +        if (!bFail && lastchunk) {
> +            // some chunks were not allocated
> +            chunk = MakeChunk(pdc);
> +            if (chunk) {
> +                chunk->prev_chunk = PA(lastchunk, m_SurfaceMemSlot);
> +                lastchunk->next_chunk = PA(chunk, m_SurfaceMemSlot);
> +                lastchunk = chunk;
> +                ++n;
> +            } else {
> +                bFail = TRUE;
> +            }
> +        }
> +        delete[] reinterpret_cast<BYTE*>(pdc);
> +    }
> +    if (bFail) {
> +        ReleaseOutput(drawable->release_info.id);

This will leak memory if is not the last chunk and some allocations
will fail (in stop flow).


> +        drawable = NULL;
> +    }
> +    return n;
> +}
> +
>  void QxlDevice::PresentThreadRoutine()
>  {
>      PAGED_CODE();
>      int wait;
>      int notify;
> +    ULONG delayed = 0;
>      QXLDrawable** drawables;
>  
>      DbgPrint(TRACE_LEVEL_INFORMATION, ("--->%s\n", __FUNCTION__));
> @@ -5206,13 +5288,23 @@ void QxlDevice::PresentThreadRoutine()
>  
>          if (drawables) {
>              for (UINT i = 0; drawables[i]; ++i)
> -                PushDrawable(drawables[i]);
> +            {
> +                ULONG n = PrepareDrawable(drawables[i]);
> +                // only reason why drawables[i] is zeroed is stop flow
> +                if (drawables[i]) {
> +                    delayed += n;
> +                    PushDrawable(drawables[i]);
> +                }
> +            }
>              delete[] drawables;
> -        }
> -        else {
> +        } else {
>              DbgPrint(TRACE_LEVEL_WARNING, ("%s is being terminated\n",
>              __FUNCTION__));
>              break;
>          }
> +        if (delayed) {
> +            DbgPrint(TRACE_LEVEL_WARNING, ("%s: %d delayed chunks\n",
> __FUNCTION__, delayed));
> +            delayed = 0;

If you move the variable declaration/initialization inside the for
this is not needed.

> +        }
>      }
>  }
>  
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> index 28373b8..c35ec3e 100755
> --- a/qxldod/QxlDod.h
> +++ b/qxldod/QxlDod.h
> @@ -474,6 +474,7 @@ ReleaseMutex(
>  #define MAX_OUTPUT_RES 6
>  
>  typedef struct QXLOutput {
> +    LIST_ENTRY list;
>      UINT32 num_res;
>  #ifdef DBG
>      UINT32 type;
> @@ -612,6 +613,8 @@ private:
>      BOOLEAN PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
>                              UINT8 **end_ptr, UINT8 *src, int size,
>                              size_t alloc_size, PLIST_ENTRY pDelayed);
> +    QXLDataChunk *MakeChunk(QXL_DELAYED_CHUNK *pdc);
> +    ULONG PrepareDrawable(QXLDrawable*& drawable);
>      void AsyncIo(UCHAR  Port, UCHAR Value);
>      void SyncIo(UCHAR  Port, UCHAR Value);
>      NTSTATUS UpdateChildStatus(BOOLEAN connect);

Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]