<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;background-color:#FFFFFF;font-family:Calibri,Arial,Helvetica,sans-serif;"> Why not be consistent and add a DRM_ERROR on all paths that include an error? e.g. instead of if (r) goto error_free; Throw a DRM_ERROR("") in there. Tom <div style="color: rgb(0, 0, 0);"> <div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr">From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Christian König <deathsimple@vodafone.de> Sent: Wednesday, August 17, 2016 10:04 To: Zhou, David(ChunMing); amd-gfx@lists.freedesktop.org Subject: Re: [PATCH 0/8] shadow page table support V5 ---> shadow bo support <div> </div> </div> </div> <div class="PlainText">Patch #1: Could be that we need to add another module parameter to control this, but I think for now that should be sufficient. Patch is Reviewed-by: Christian König <christian.koenig@amd.com> Patch #2: > + if (direct_submit) { > + r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, > + NULL, NULL, fence); > + job->fence = fence_get(*fence); > + if (r) > + DRM_ERROR("Error scheduling IBs (%d)\n", r); > + amdgpu_job_free(job); When there is an error you should return the code as well. > + } else { > + r = amdgpu_job_submit(job, ring, &adev->mman.entity, > + AMDGPU_FENCE_OWNER_UNDEFINED, fence); > + if (r) > + goto error_free; > + } > > return 0; Just changing this to to "return r;" should be sufficient. With that fixed the patch is Reviewed-by: Christian König <christian.koenig@amd.com> as well. Patch #3: You mentioned that this isn't used during command submission but rather during GPU reset, correct? In this case I would advise not to use a member in the BO structure to note the backup direction and instead have one function for back and one for restoring the content (or note that as a parameter to the function). Otherwise we could run into trouble when the CS wants to backup something the GPU reset wants to restore at the same time. Patch #4: Was already reviewed by me. Patch #5: > + pt = params->shadow ? vm->page_tables[pt_idx].entry.robj->shadow : > + vm->page_tables[pt_idx].entry.robj; You need to double check here if shadows are actually allocated or not. Otherwise we will crash on an APU. > + /* double ndw, since need to update shadow pt bo as well */ > + ndw *= 2; > + We don't need to double the IB size, but only the number of commands in it (ncmds). The difference is when we want to write scattered GART entries. In this case I've added the PTEs to the end of the IB. Patch #6: > + spinlock_t shadow_list_lock; We might want to use a mutex here instead. Usually I would agree that a spin lock is better for a linked list, but during the restore process in a GPU reset we probably want to sleep here. Alternatively you could splice the list to a local version on the stack, grab references to the BOs and then drop the lock during the restore process. > @@ -541,6 +546,13 @@ void amdgpu_bo_unref(struct amdgpu_bo **bo) > if ((*bo) == NULL) > return; > > + /* shadow must be freed before bo itself */ > + if ((!(*bo)->shadow) && !list_empty(&(*bo)->shadow_list)) { > + spin_lock(&(*bo)->adev->shadow_list_lock); > + list_del_init(&(*bo)->shadow_list); > + spin_unlock(&(*bo)->adev->shadow_list_lock); > + > + } > tbo = &((*bo)->tbo); > ttm_bo_unref(&tbo); > if (tbo == NULL) That would trigger even when we take a temporary reference. I suggest to add a amdgpu_bo_unref_shadow() function instead, unreferencing both the shadow and the normal BO. This can then be used in the VM code to cleanup the shadow created. Going to skip patch #7 and #8 for now cause our team call begins in a moment. Regards, Christian. Am 17.08.2016 um 08:05 schrieb Chunming Zhou: > Since we cannot ensure VRAM is consistent after a GPU reset, page > table shadowing is necessary. Shadowed page tables are, in a sense, a > method to recover the consistent state of the page tables before the > reset occurred. > > We need to allocate GTT bo as the shadow of VRAM bo when creating page table, > and make them the same. After gpu reset, we will need to use SDMA to copy GTT bo > content to VRAM bo, then page table will be recoveried. > > > V2: > Shadow bo uses a shadow entity running on normal run queue, after gpu reset, > we need to wait for all shadow jobs finished first, then recovery page table from shadow. > > V3: > Addressed Christian comments for shadow bo part. > > V4: > Switch back to update page table twice (one of two is for shadow) > > V5: > make it be gerneic shadow bo support. Address Christian comments. > > Chunming Zhou (8): > drm/amdgpu: add need backup function V2 > drm/amdgpu: add direct submision option for copy_buffer > drm/amdgpu: sync bo and shadow V2 > drm/amdgpu: update pd shadow while updating pd > drm/amdgpu: update pt shadow while updating pt V2 > drm/amdgpu: link all shadow bo > drm/amdgpu: implement recovery vram bo from shadow > drm/amdgpu: recover vram bo from shadow after gpu reset > > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 9 ++- > drivers/gpu/drm/amd/amdgpu/amdgpu_benchmark.c | 3 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 39 ++++++++++- > drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 94 ++++++++++++++++++++++++++- > drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 9 +++ > drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 4 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 21 ++++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 69 ++++++++++++++------ > 8 files changed, 215 insertions(+), 33 deletions(-) > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx" id="LPlnk214970">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> <div id="LPBorder_GT_14714429737800.4406300387876183" style="margin-bottom: 20px; overflow: auto; width: 100%; text-indent: 0px;"> <table id="LPContainer_14714429737780.3993206472027586" cellspacing="0" style="width: 90%; position: relative; overflow: auto; padding-top: 20px; padding-bottom: 20px; margin-top: 20px; border-top: 1px dotted rgb(200, 200, 200); border-bottom: 1px dotted rgb(200, 200, 200); background-color: rgb(255, 255, 255);"> <tbody> <tr valign="top" style="border-spacing: 0px;"> <td id="TextCell_14714429737790.7705126490415855" colspan="2" style="vertical-align: top; position: relative; padding: 0px; display: table-cell;"> <div id="LPRemovePreviewContainer_14714429737790.7102716663188782"></div> <div id="LPTitle_14714429737790.7634184458877278" style="top: 0px; color: rgb(59, 87, 119); font-weight: normal; font-size: 21px; font-family: wf_segoe-ui_light, "Segoe UI Light", "Segoe WP Light", "Segoe UI", "Segoe WP", Tahoma, Arial, sans-serif; line-height: 21px;"> <a id="LPUrlAnchor_14714429737790.6135333253292945" href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx" target="_blank" style="text-decoration: none;">amd-gfx Info Page - lists.freedesktop.org</a></div> <div id="LPMetadata_14714429737790.9058058508163938" style="margin: 10px 0px 16px; color: rgb(102, 102, 102); font-weight: normal; font-family: wf_segoe-ui_normal, "Segoe UI", "Segoe WP", Tahoma, Arial, sans-serif; font-size: 14px; line-height: 14px;"> lists.freedesktop.org</div> <div id="LPDescription_14714429737800.03199008072574672" style="display: block; color: rgb(102, 102, 102); font-weight: normal; font-family: wf_segoe-ui_normal, "Segoe UI", "Segoe WP", Tahoma, Arial, sans-serif; font-size: 14px; line-height: 20px; max-height: 100px; overflow: hidden;"> To see the collection of prior postings to the list, visit the amd-gfx Archives. Using amd-gfx: To post a message to all the list members, send email ...</div> </td> </tr> </tbody> </table> </div> </div> </div> </div> </body> </html>