On Wed, Jun 24, 2015 at 08:33:50AM +0100, Chris Wilson wrote: > On Tue, Jun 23, 2015 at 04:37:19PM -0700, Anuj Phogat wrote: > > On Mon, Jun 22, 2015 at 12:49 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: > > > On Mon, Jun 22, 2015 at 10:21:46AM -0700, Ben Widawsky wrote: > > >> On Fri, Jun 19, 2015 at 03:52:01PM -0700, Anuj Phogat wrote: > > >> > +Ben > > >> > > > >> > On Fri, Apr 10, 2015 at 5:20 PM, Anuj Phogat <anuj.phogat@xxxxxxxxx> wrote: > > >> > > Signed-off-by: Anuj Phogat <anuj.phogat@xxxxxxxxx> > > >> > > --- > > >> > > intel/intel_bufmgr_gem.c | 4 ++-- > > >> > > 1 file changed, 2 insertions(+), 2 deletions(-) > > >> > > > > >> > > diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c > > >> > > index 51d87ae..92701a5 100644 > > >> > > --- a/intel/intel_bufmgr_gem.c > > >> > > +++ b/intel/intel_bufmgr_gem.c > > >> > > @@ -459,7 +459,7 @@ drm_intel_add_validate_buffer(drm_intel_bo *bo) > > >> > > bufmgr_gem->exec_objects[index].handle = bo_gem->gem_handle; > > >> > > bufmgr_gem->exec_objects[index].relocation_count = bo_gem->reloc_count; > > >> > > bufmgr_gem->exec_objects[index].relocs_ptr = (uintptr_t) bo_gem->relocs; > > >> > > - bufmgr_gem->exec_objects[index].alignment = 0; > > >> > > + bufmgr_gem->exec_objects[index].alignment = bo->align; > > >> > > bufmgr_gem->exec_objects[index].offset = 0; > > >> > > bufmgr_gem->exec_bos[index] = bo; > > >> > > bufmgr_gem->exec_count++; > > >> > > >> I'm a bit hesitant about this hunk. We're never going to use this from a mesa > > >> that supports Yf/Ys - and going this path wouldn't be expected. Maybe add a > > >> warning if bo->align? (From your other patch I don't think it can ever happen, > > >> but just to future proof it. > > >> > > >> > > @@ -501,7 +501,7 @@ drm_intel_add_validate_buffer2(drm_intel_bo *bo, int need_fence) > > >> > > bufmgr_gem->exec2_objects[index].handle = bo_gem->gem_handle; > > >> > > bufmgr_gem->exec2_objects[index].relocation_count = bo_gem->reloc_count; > > >> > > bufmgr_gem->exec2_objects[index].relocs_ptr = (uintptr_t)bo_gem->relocs; > > >> > > - bufmgr_gem->exec2_objects[index].alignment = 0; > > >> > > + bufmgr_gem->exec2_objects[index].alignment = bo->align; > > >> > > bufmgr_gem->exec2_objects[index].offset = 0; > > >> > > bufmgr_gem->exec_bos[index] = bo; > > >> > > bufmgr_gem->exec2_objects[index].flags = 0; > > >> > > >> I was about to argue this should be part of patch 1, but nope, it should be a > > >> separate patch :-) > > >> > > >> I started digging into whether we have a reasonable way to determine if a bo > > >> alignment failed, and fall back to a softer restriction. It didn't seem doable > > >> with the current interfaces, but it's something to think about. > > > > > > On gen2/3 we have a lot more severe alignment problems and there the only > > > thing we've done is to take worst-case space loss for alignment into > > > account for the execbuf space estimation. But if that failed (and it did > > > every now and then) userspace just died. I don't think we need any of this > > > for new tiling layouts since they're all uniformly using a 64kb alignment. > > > The kernel should be able to pack this very well. > > Daniel, I don't know much about how kernel is handling the alignment > > constraints for new tiling layouts during relocation. I'm here mostly > > relying on the advice from kernel guys. These two patches are based on > > your comment on an earlier patch on intel-gfx: > > "[PATCH 4/5] Align YS tile base address to 64KB" > > > > Even if the kernel is already packing the new tiling layouts correctly, these > > patches are passing some valid alignment value in place of always zero. > > Isn't it a harmless change? > > I think Daniel is talking here about the estimated GTT usage for a > batch. mesa/libdrm tracks the estimated usage so that it can flush > before the batch can no longer fit into the GTT. There is a one > primitive back-off in mesa, but if the estimate is far, that may not be > enough to trim the batch sufficiently for us to be able to render it. To > accommodate that worry, you just want to tweak > drm_intel_bo_gem_set_in_aperture_size() to include the alignment as > overhead (and not assume anything about the ability of the kernel to > pack buffers together). Well actually I just replied to Ben's concern that alignment might fail stating that we have some tricks already to handle the much worse situation on gen2/3, like Chris explained. But also that I don't think we actually need to update them. But ofc you can update the aperture size estimate like Chris suggested: size + alignment is a good worst-case estimate of what we'll need in gtt space. With that change we _really_ should be covered. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
