<html> <head> <meta content="text/html; charset=utf-8" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> <div class="moz-cite-prefix">On 17.03.2015 16:19, Alex Deucher wrote: </div> <blockquote cite="mid:CADnq5_NeqyNb=XKxdWuEGiHbfGX+i5yqtFRyOnzg3EzyjzQmKg@mail.gmail.com" type="cite"> <pre wrap="">On Mon, Mar 16, 2015 at 11:48 PM, Michel Dänzer <a class="moz-txt-link-rfc2396E" href="mailto:michel@daenzer.net"><michel@daenzer.net></a> wrote: </pre> <blockquote type="cite"> <pre wrap="">On 17.03.2015 07:32, Alex Deucher wrote: </pre> <blockquote type="cite"> <pre wrap="">On Thu, Mar 12, 2015 at 10:55 PM, Michel Dänzer <a class="moz-txt-link-rfc2396E" href="mailto:michel@daenzer.net"><michel@daenzer.net></a> wrote: </pre> <blockquote type="cite"> <pre wrap="">On 12.03.2015 22:09, Alex Deucher wrote: </pre> <blockquote type="cite"> <pre wrap="">On Thu, Mar 12, 2015 at 5:23 AM, Christian König <a class="moz-txt-link-rfc2396E" href="mailto:deathsimple@vodafone.de"><deathsimple@vodafone.de></a> wrote: </pre> <blockquote type="cite"> <pre wrap="">On 12.03.2015 10:02, Michel Dänzer wrote: </pre> <blockquote type="cite"> <pre wrap=""> On 12.03.2015 06:14, Alex Deucher wrote: </pre> <blockquote type="cite"> <pre wrap=""> On Wed, Mar 11, 2015 at 4:51 PM, Alex Deucher <a class="moz-txt-link-rfc2396E" href="mailto:alexdeucher@gmail.com"><alexdeucher@gmail.com></a> wrote: </pre> <blockquote type="cite"> <pre wrap=""> On Wed, Mar 11, 2015 at 2:21 PM, Christian König <a class="moz-txt-link-rfc2396E" href="mailto:deathsimple@vodafone.de"><deathsimple@vodafone.de></a> wrote: </pre> <blockquote type="cite"> <pre wrap=""> On 11.03.2015 16:44, Alex Deucher wrote: </pre> <blockquote type="cite"> <pre wrap=""> radeon_bo_create() calls radeon_ttm_placement_from_domain() before ttm_bo_init() is called. radeon_ttm_placement_from_domain() uses the ttm bo size to determine when to select top down allocation but since the ttm bo is not initialized yet the check is always false. Noticed-by: Oded Gabbay <a class="moz-txt-link-rfc2396E" href="mailto:oded.gabbay@amd.com"><oded.gabbay@amd.com></a> Signed-off-by: Alex Deucher <a class="moz-txt-link-rfc2396E" href="mailto:alexander.deucher@amd.com"><alexander.deucher@amd.com></a> Cc: <a class="moz-txt-link-abbreviated" href="mailto:stable@vger.kernel.org">stable@vger.kernel.org</a> </pre> </blockquote> <pre wrap=""> And I was already wondering why the heck the BOs always made this ping/pong in memory after creation. Patch is Reviewed-by: Christian König <a class="moz-txt-link-rfc2396E" href="mailto:christian.koenig@amd.com"><christian.koenig@amd.com></a> </pre> </blockquote> <pre wrap=""> And fixing that promptly broke VCE due to vram location requirements. Updated patch attached. Thoughts? </pre> </blockquote> <pre wrap=""> And one more take to make things a bit more explicit for static kernel driver allocations. </pre> </blockquote> <pre wrap=""> struct ttm_place::lpfn is honoured even with TTM_PL_FLAG_TOPDOWN, so latter should work with RADEON_GEM_CPU_ACCESS. It sounds like the problem is really that some BOs are expected to be within a certain range from the beginning of VRAM, but lpfn isn't set accordingly. It would be better to fix that by setting lpfn directly than indirectly via RADEON_GEM_CPU_ACCESS. </pre> </blockquote> <pre wrap=""> Yeah, agree. We should probably try to find the root cause of this instead. As far as I know VCE has no documented limitation on where buffers are placed (unlike UVD). So this is a bit strange. Are you sure that it isn't UVD which breaks here? </pre> </blockquote> <pre wrap=""> It's definitely VCE, I don't know why UVD didn't have a problem. I considered using pin_restricted to make sure it got pinned in the CPU visible region, but that had two problems: 1. it would end up getting migrated when pinned, </pre> </blockquote> <pre wrap=""> Maybe something like radeon_uvd_force_into_uvd_segment() is needed for VCE as well? </pre> <blockquote type="cite"> <pre wrap="">2. it would end up at the top of the restricted region since the top down flag is set which would end up fragmenting vram. </pre> </blockquote> <pre wrap=""> If that's an issue (which outweighs the supposed benefit of TTM_PL_FLAG_TOPDOWN), then again the proper solution would be not to set TTM_PL_FLAG_TOPDOWN when rbo->placements[i].lpfn != 0 and smaller than the whole available region, instead of checking for VRAM and RADEON_GEM_CPU_ACCESS. </pre> </blockquote> <pre wrap=""> How about something like the attached patch? I'm not really sure about the restrictions for the UVD and VCE fw and stack/heap buffers, but this seems to work. It seems like the current UVD/VCE code works by accident since the check for TOPDOWN fails. </pre> </blockquote> <pre wrap=""> This patch is getting a bit messy, mixing several logically separate changes. Can you split it up accordingly? E.g. one patch just adding the new fpfn and lpfn function parameters but passing 0 for them (so no functional change), then one or several patches with the corresponding functional changes, and finally one patch adding the new size parameter (and thus making TTM_PL_FLAG_TOPDOWN actually used for newly allocated BOs). I think that would help for reviewing and generally understanding the changes. </pre> <blockquote type="cite"> <pre wrap="">@@ -105,14 +106,17 @@ void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32 domain) */ if ((rbo->flags & RADEON_GEM_NO_CPU_ACCESS) && rbo->rdev->mc.visible_vram_size < rbo->rdev->mc.real_vram_size) { - rbo->placements[c].fpfn = - rbo->rdev->mc.visible_vram_size >> PAGE_SHIFT; + if (fpfn > (rbo->rdev->mc.visible_vram_size >> PAGE_SHIFT)) + rbo->placements[c].fpfn = fpfn; + else + rbo->placements[c].fpfn = + rbo->rdev->mc.visible_vram_size >> PAGE_SHIFT; rbo->placements[c++].flags = TTM_PL_FLAG_WC | TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_VRAM; } </pre> </blockquote> <pre wrap=""> If (fpfn >= rbo->rdev->mc.visible_vram_size), this whole block can be skipped, since the next placement will be identical. OTOH, fpfn is currently always 0 anyway, so maybe it's better not to add that parameter in the first place. Other than that, looks good to me. </pre> </blockquote> <pre wrap=""> Broken out patches attached. Also available here: <a class="moz-txt-link-freetext" href="http://cgit.freedesktop.org/~agd5f/linux/log/?h=topdown-fixes">http://cgit.freedesktop.org/~agd5f/linux/log/?h=topdown-fixes</a></pre> </blockquote> Thinking more about it that approach is a NAK. For limiting a BO into visible VRAM we want the limit it to only apply to the VRAM domain entry, doing it this way it applies to GTT as well which is really bad for handling page faults. I would rather say let us completely nuke radeon_ttm_placement_from_domain for internal allocations and give radeon_bo_create a ttm_placement pointer to use. Driver internal allocations would then have a couple of predefined placements for their buffers. We might need to make a few ttm_placement pointers const for this, but I think that this is the better approach. Regards, Christian. <blockquote cite="mid:CADnq5_NeqyNb=XKxdWuEGiHbfGX+i5yqtFRyOnzg3EzyjzQmKg@mail.gmail.com" type="cite"> <pre wrap=""> Alex </pre> <fieldset class="mimeAttachmentHeader"></fieldset> <pre wrap="">_______________________________________________ dri-devel mailing list <a class="moz-txt-link-abbreviated" href="mailto:dri-devel@lists.freedesktop.org">dri-devel@lists.freedesktop.org</a> <a class="moz-txt-link-freetext" href="http://lists.freedesktop.org/mailman/listinfo/dri-devel">http://lists.freedesktop.org/mailman/listinfo/dri-devel</a> </pre> </blockquote> </body> </html>