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

From: Paulo Zanoni <przanoni@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH 7/7] lib: add igt_draw
Date: Tue, 5 May 2015 18:30:50 -0300	[thread overview]
Message-ID: <CA+gsUGRQnFvC6kETLzRd2_rh1_Ddtg4EwRXkagovoL25fQVUyw@mail.gmail.com> (raw)
In-Reply-To: <20150407134443.GH6354@phenom.ffwll.local>

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.

Thanks for the reviews,
Paulo

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx