<html><body><div style="font-family: times new roman, new york, times, serif; font-size: 12pt; color: #000000"><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Apr 10, 2017 at 3:45 PM, Frediano Ziglio <span dir="ltr"><<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="m_2499721410444427071gmail-HOEnZb"><div class="m_2499721410444427071gmail-h5">><br>
> In case of VSync is active (for the driver this means it shall take<br>
> in account watchdog policy and ensure fast execution of PresentDisplayOnly<br>
> callback) allocate bitmaps for drawable objects using non-forced requests.<br>
> If immediate allocation is not possible, place entire bitmap into memory<br>
> chunk allocated from the OS.<br>
> If bitmap is allocated from device memory, but one of later<br>
> chunks can't be allocated, allocate this and further chunks from<br>
> OS memory. All these 'delayed' allocations placed into linked list<br>
> which root entry is part of QXLOutput structure.<br>
> From separate thread, before sending drawable objects down, review<br>
> the list of delayed chunks and allocate device memory (forced) to<br>
> all of them.<br>
> The cost of solution is 2 pointers added to each drawable or cursor<br>
> object.<br>
> Cursor commands currently do not use them; in future we have an option<br>
> to offload also cursor commands.<br>
><br>
> Signed-off-by: Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com" target="_blank">yuri.benditovich@daynix.com</a>><br>
> ---<br>
> qxldod/QxlDod.cpp | 108<br>
> ++++++++++++++++++++++++++++++++++++++++++++++++++----<br>
> qxldod/QxlDod.h | 3 ++<br>
> 2 files changed, 103 insertions(+), 8 deletions(-)<br>
><br>
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp<br>
> index c832c93..7ff903a 100755<br>
> --- a/qxldod/QxlDod.cpp<br>
> +++ b/qxldod/QxlDod.cpp<br>
> @@ -4224,6 +4224,13 @@ void QxlDevice::DrawableAddRes(QXLDrawable *drawable,<br>
> Resource *res)<br>
> AddRes(output, res);<br>
> }<br>
><br>
> +static FORCEINLINE PLIST_ENTRY DelayedList(QXLDrawable *pd)<br>
> +{<br>
> + QXLOutput *output;<br>
> + output = (QXLOutput *)((UINT8 *)pd - sizeof(QXLOutput));<br>
> + return &output->list;<br>
> +}<br>
> +<br><br></div></div>This hacky code should just not exist. This assumes that before<br>
every Drawable there is a QXLOutput.<br></blockquote><div><br></div><div>This is correct assumption, see creation of drawable.</div><div>DrawableAddRes does exactly the same</div></div></div></div></blockquote><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I know there are already similar code but would be better to<br>
use a structure that contains a QXLOutput and a QXLDrawable and<br>
use that.<br><span class="m_2499721410444427071gmail-"><br></span></blockquote><div><br></div><div>This would create more changes in the code than really needed.</div></div></div></div></blockquote><div>I agree for this series but would like to have a proper style document for this driver<br></div><div>in a near future.<br></div><div><br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_2499721410444427071gmail-">
> void QxlDevice::CursorCmdAddRes(QXLCursorCmd *cmd, Resource *res)<br>
> {<br>
> PAGED_CODE();<br>
> @@ -4331,6 +4338,7 @@ QXLDrawable *QxlDevice::Drawable(UINT8 type, CONST RECT<br>
> *area, CONST RECT *clip,<br>
> drawable->surfaces_dest[1] = - 1;<br>
> drawable->surfaces_dest[2] = -1;<br>
> CopyRect(&drawable->bbox, area);<br>
> + InitializeListHead(DelayedList(drawable));<br>
><br>
<br>
</span>Use a constructor?<br></blockquote><div><br></div><div>I did not plan to rework all the existing code.</div><div>The rule of thumb is as less changes as possible.</div></div></div></div></blockquote><div>Well... this is new code but on the other way calling the constructor at the moment<br></div><div>could take some more code so agreed for this series.<br></div><div><br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="m_2499721410444427071gmail-h5"><br>
> if (!SetClip(clip, drawable)) {<br>
> DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: set clip failed\n",<br>
> __FUNCTION__));<br>
> @@ -4425,7 +4433,7 @@ BOOLEAN QxlDevice::AttachNewBitmap(QXLDrawable<br>
> *drawable, UINT8 *src, UINT8 *src<br>
> Resource *image_res;<br>
> InternalImage *internal;<br>
> QXLDataChunk *chunk;<br>
> - PLIST_ENTRY pDelayedList = NULL;<br>
> + PLIST_ENTRY pDelayedList = bForce ? NULL : DelayedList(drawable);<br>
> UINT8* dest, *dest_end;<br>
><br>
> height = drawable->u.copy.src_area.bottom;<br>
> @@ -4494,9 +4502,12 @@ BOOLEAN QxlDevice::AttachNewBitmap(QXLDrawable<br>
> *drawable, UINT8 *src, UINT8 *src<br>
> }<br>
><br>
> for (; src != src_end; src -= pitch, alloc_size -= line_size) {<br>
> - BOOLEAN b = PutBytesAlign(&chunk, &dest, &dest_end, src, line_size,<br>
> alloc_size, pDelayedList);<br>
> - if (!b) {<br>
> - DbgPrint(TRACE_LEVEL_WARNING, ("%s: aborting copy of lines\n",<br>
> __FUNCTION__));<br>
> + if (!PutBytesAlign(&chunk, &dest, &dest_end, src, line_size,<br>
> alloc_size, pDelayedList)) {<br>
> + if (pitch < 0 && bForce) {<br>
> + DbgPrint(TRACE_LEVEL_WARNING, ("%s: aborting copy of lines<br>
> (forced)\n", __FUNCTION__));<br>
> + } else {<br>
> + DbgPrint(TRACE_LEVEL_WARNING, ("%s: unexpected aborting copy<br>
> of lines (force %d, pitch %d)\n", __FUNCTION__, bForce, pitch));<br>
> + }<br><br></div></div>Why the if on the pitch and not just this last version?<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_2499721410444427071gmail-"><br>
> return FALSE;<br>
> }<br>
> }<br>
> @@ -4551,7 +4562,13 @@ QXLDrawable *QxlDevice::PrepareBltBits (<br>
> UINT8* src_end = src - pSrc->Pitch;<br>
> src += pSrc->Pitch * (height - 1);<br>
><br>
> - if (!AttachNewBitmap(drawable, src, src_end, (INT)pSrc->Pitch, TRUE)) {<br>
> + if (!AttachNewBitmap(drawable, src, src_end, (INT)pSrc->Pitch,<br>
> !g_bSupportVSync)) {<br>
> + PLIST_ENTRY pDelayedList = DelayedList(drawable);<br>
> + // if some delayed chunks were allocated, free them<br>
> + while (!IsListEmpty(pDelayedList)) {<br>
> + QXL_DELAYED_CHUNK *pdc = (QXL_DELAYED_CHUNK<br>
> *)RemoveHeadList(pDelayedList);<br>
> + delete[] reinterpret_cast<BYTE*>(pdc);<br>
> + }<br>
<br>
</span>destructor?<br></blockquote><div><br></div><div>Lifecycle of drawable is in general larger than lifecycle of delayed chunks.</div><div>In main flow we need to free chunks allocated from OS earlier than we deallocate the drawable.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
> ReleaseOutput(drawable-><a href="http://release_info.id" rel="noreferrer" target="_blank">release_info.id</a>);<br><br>
Why ReleaseOutput is not releasing the linked list, which is<br>
in it?<br><div><div class="m_2499721410444427071gmail-h5"><br></div></div></blockquote><div><br></div><div>The driver in any flow must free each memory allocation from OS (this is checked by the driver verifier).</div><div>Objects allocated from device's memory are released when the ring is being processed, i.e. much later.</div><div>In corner cases we do not release drawable objects at all. So we need to free the memory allocated from OS before we send the drawable object down.</div></div></div></div></blockquote><div>I would add this to the style comments above.<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="m_2499721410444427071gmail-h5">
> drawable = NULL;<br>
> } else {<br>
> @@ -5179,11 +5196,76 @@ void QxlDevice::StopPresentThread()<br>
> }<br>
> }<br>
><br>
> +QXLDataChunk *QxlDevice::MakeChunk(QXL_DELAYED_CHUNK *pdc)<br>
> +{<br>
> + PAGED_CODE();<br>
> + QXLDataChunk *chunk = (QXLDataChunk *)AllocMem(MSPACE_TYPE_VRAM,<br>
> pdc->chunk.data_size + sizeof(QXLDataChunk), TRUE);<br>
> + if (chunk)<br>
> + {<br>
> + chunk->data_size = pdc->chunk.data_size;<br>
> + chunk->next_chunk = 0;<br>
> + RtlCopyMemory(chunk->data, pdc->chunk.data, chunk->data_size);<br>
> + }<br>
> + return chunk;<br>
> +}<br>
> +<br>
> +ULONG QxlDevice::PrepareDrawable(QXLDrawable*& drawable)<br>
> +{<br>
> + PAGED_CODE();<br>
> + ULONG n = 0;<br>
> + BOOLEAN bFail;<br>
> + PLIST_ENTRY pe = DelayedList(drawable);<br>
> + QXLDataChunk *chunk, *lastchunk = NULL;<br>
> +<br>
> + bFail = !m_bActive;<br>
> +<br>
> + while (!IsListEmpty(pe)) {<br>
> + QXL_DELAYED_CHUNK *pdc = (QXL_DELAYED_CHUNK *)RemoveHeadList(pe);<br>
> + if (!lastchunk) {<br>
> + lastchunk = (QXLDataChunk *)pdc->chunk.prev_chunk;<br>
> + }<br>
> + if (!bFail && !lastchunk) {<br>
> + // bitmap was not allocated, this is single delayed chunk<br>
> + QXL_ASSERT(IsListEmpty(pe));<br>
> +<br>
> + if (AttachNewBitmap(<br>
> + drawable,<br>
> + pdc->chunk.data,<br>
> + pdc->chunk.data + pdc->chunk.data_size,<br>
> + -(drawable->u.copy.src_area.right * 4),<br>
> + TRUE)) {<br>
> + ++n;<br>
> + } else {<br>
> + bFail = TRUE;<br>
> + }<br>
> + }<br>
> + if (!bFail && lastchunk) {<br>
> + // some chunks were not allocated<br>
> + chunk = MakeChunk(pdc);<br>
> + if (chunk) {<br>
> + chunk->prev_chunk = PA(lastchunk, m_SurfaceMemSlot);<br>
> + lastchunk->next_chunk = PA(chunk, m_SurfaceMemSlot);<br>
> + lastchunk = chunk;<br>
> + ++n;<br>
> + } else {<br>
> + bFail = TRUE;<br>
> + }<br>
> + }<br>
> + delete[] reinterpret_cast<BYTE*>(pdc);<br>
> + }<br>
> + if (bFail) {<br>
> + ReleaseOutput(drawable-><a href="http://release_info.id" rel="noreferrer" target="_blank">release_info.id</a>);<br><br></div></div>This will leak memory if is not the last chunk and some allocations<br>
will fail (in stop flow).<br></blockquote><div><br></div><div>I do not see any leak here. We leave the loop after we free all the allocations from OS.</div><div>If there is no last chunk, the list contains just one entry, there is entire bitmap allocated from OS (see ASSERT) and if some HW chunks were allocated from device memory during unsuccessful AttachNewBitmap, they will be freed by ReleaseOutput.</div></div></div></div></blockquote><div>Yes, you are right.<br></div><div>I was actually thinking about a break or exit condition.<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="m_2499721410444427071gmail-h5"><br><br>
> + drawable = NULL;<br>
> + }<br>
> + return n;<br>
> +}<br>
> +<br>
> void QxlDevice::PresentThreadRoutine()<br>
> {<br>
> PAGED_CODE();<br>
> int wait;<br>
> int notify;<br>
> + ULONG delayed = 0;<br>
> QXLDrawable** drawables;<br>
><br>
> DbgPrint(TRACE_LEVEL_INFORMATION, ("--->%s\n", __FUNCTION__));<br>
> @@ -5206,13 +5288,23 @@ void QxlDevice::PresentThreadRoutine()<br>
><br>
> if (drawables) {<br>
> for (UINT i = 0; drawables[i]; ++i)<br>
> - PushDrawable(drawables[i]);<br>
> + {<br>
> + ULONG n = PrepareDrawable(drawables[i]);<br>
> + // only reason why drawables[i] is zeroed is stop flow<br>
> + if (drawables[i]) {<br>
> + delayed += n;<br>
> + PushDrawable(drawables[i]);<br>
> + }<br>
> + }<br>
> delete[] drawables;<br>
> - }<br>
> - else {<br>
> + } else {<br>
> DbgPrint(TRACE_LEVEL_WARNING, ("%s is being terminated\n",<br>
> __FUNCTION__));<br>
> break;<br>
> }<br>
> + if (delayed) {<br>
> + DbgPrint(TRACE_LEVEL_WARNING, ("%s: %d delayed chunks\n",<br>
> __FUNCTION__, delayed));<br>
> + delayed = 0;<br><br></div></div>If you move the variable declaration/initialization inside the for<br>
this is not needed.<br></blockquote><div><br></div><div>This would create more printouts in case the array contains several/many drawables.</div></div></div></div></blockquote><div>No. But is just question of style.<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="m_2499721410444427071gmail-HOEnZb"><div class="m_2499721410444427071gmail-h5"><br>
> + }<br>
> }<br>
> }<br>
><br>
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h<br>
> index 28373b8..c35ec3e 100755<br>
> --- a/qxldod/QxlDod.h<br>
> +++ b/qxldod/QxlDod.h<br>
> @@ -474,6 +474,7 @@ ReleaseMutex(<br>
> #define MAX_OUTPUT_RES 6<br>
><br>
> typedef struct QXLOutput {<br>
> + LIST_ENTRY list;<br>
> UINT32 num_res;<br>
> #ifdef DBG<br>
> UINT32 type;</div></div></blockquote></div></div></div></blockquote><div>Not that I like much other pointers put in device memory but for the moment is not a regression<br></div><div>and is not causing possible security concern (host had access to guest memory).<br></div><blockquote style="border-left:2px solid #1010FF;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="m_2499721410444427071gmail-HOEnZb"><div class="m_2499721410444427071gmail-h5"><br>
> @@ -612,6 +613,8 @@ private:<br>
> BOOLEAN PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,<br>
> UINT8 **end_ptr, UINT8 *src, int size,<br>
> size_t alloc_size, PLIST_ENTRY pDelayed);<br>
> + QXLDataChunk *MakeChunk(QXL_DELAYED_CHUNK *pdc);<br>
> + ULONG PrepareDrawable(QXLDrawable*& drawable);<br>
> void AsyncIo(UCHAR Port, UCHAR Value);<br>
> void SyncIo(UCHAR Port, UCHAR Value);<br>
> NTSTATUS UpdateChildStatus(BOOLEAN connect);<br></div></div></blockquote></div></div></div></blockquote><div>So for the time being I'll accept this, beside the structure name that I would change (QXL_DELAYED_CHUNK).<br></div><div><br></div><div>Frediano<br></div><div><br></div></div></body></html>