Re: [PATCH 7/7] lib: add igt_draw

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 05, 2015 at 06:30:50PM -0300, Paulo Zanoni wrote:
> 2015-04-07 10:44 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Tue, Apr 07, 2015 at 11:12:09AM +0100, Chris Wilson wrote:
> >> On Tue, Apr 07, 2015 at 11:07:07AM +0200, Daniel Vetter wrote:
> >> > On Tue, Apr 07, 2015 at 09:36:37AM +0100, Chris Wilson wrote:
> >> > > On Tue, Apr 07, 2015 at 10:10:25AM +0200, Daniel Vetter wrote:
> >> > > > On Thu, Apr 02, 2015 at 12:15:13AM +0100, Chris Wilson wrote:
> >> > > > > On Wed, Apr 01, 2015 at 07:40:59PM -0300, Paulo Zanoni wrote:
> >> > > > > > +static void draw_rect_mmap_wc(int fd, struct buf_data *buf, struct rect *rect,
> >> > > > > > +                         uint32_t color)
> >> > > > > > +{
> >> > > > > > +   uint32_t *ptr;
> >> > > > > > +   uint32_t tiling, swizzle;
> >> > > > > > +
> >> > > > > > +   gem_get_tiling(fd, buf->handle, &tiling, &swizzle);
> >> > > > > > +
> >> > > > > > +   /* We didn't implement suport for the older tiling methods yet. */
> >> > > > > > +   if (tiling != I915_TILING_NONE)
> >> > > > > > +           igt_require(intel_gen(intel_get_drm_devid(fd)) >= 5);
> >> > > > >
> >> > > > > But you now do! You need something like:
> >> > > >
> >> > > > The problem is that the kernel hides bit17 swizzling. I chatted with Paulo
> >> > > > on irc about this and we decided just ignore them all is the simplest
> >> > > > approach.
> >> > >
> >> > > Urm, that was the whole point of GET_TILING v2. That small function is
> >> > > all you need to determine when bit17 is in effect and then you get to
> >> > > reuse all the direct CPU methods (as they are also used by userspace)
> >> > > for earlier gen.
> >> >
> >> > Oh right completely forgot that we've added this. But imo can be added on
> >> > top once we need it (it's not just gen2 but also some gen3 which need
> >> > different tile dimensions).
> >>
> >> Tile size is 2048 for gen2 only right. Tile width is the same for all
> >> gen3 for tiling X (and gen2), but tiling Y width depends on subgen.
> >> Right?
> >>
> >> Just need to check because I have code that depends on this...
> >
> > Oh right thought about Y tiling which changes on gen3. X tiling matches
> > your description afaik - I consider gem_tiled_pread the authoritative
> > source for this stuff.
> 
> Ok, so after all this discussion, what is the conclusion here? What is
> required before we can merge the patch?
> 
> Notice that with this patch we still won't have anybody using the
> library (except for the test added by the patch), and all the users I
> plan to add are gen5+. Also, I created VIZ-5495 to make sure we create
> intel_bo_fill() at some point (and, when we do it, I suggest we just
> use the implementation from igt_draw.c).
> 
> I really think the improvements discussed here can be done after we
> merge the patch because the it's not breaking anything, AFAICS. So,
> can I push this? Also, after the code is on igt, it will probably be
> easier to discuss the possible improvements since we'll be able to
> send patches.

I guess you could capture the discussion here in a TODO comment in
igt_draw? But yeah I thought we've discussed this on irc and agreed that
moving ahead is ok, and that extensions can be done later on.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx