Re: [Intel-gfx] [PATCH 40/44] drm/cirrus: Don't use drm_device->dev_private

[Eric Anholt]

On Fri, Apr 3, 2020 at 6:59 AM Daniel Vetter  wrote:
>
> Upcasting using a container_of macro is more typesafe, faster and
> easier for the compiler to optimize.
>
> Signed-off-by: Daniel Vetter 
> Cc: Dave Airlie 
> Cc: Gerd Hoffmann 
> Cc: Daniel Vetter 
> Cc: "Noralf Trønnes" 
> Cc: Sam Ravnborg 

Acked-by: Eric Anholt 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/44] drm/v3d: Don't set drm_device->dev_private

[Eric Anholt]

On Fri, Apr 3, 2020 at 6:58 AM Daniel Vetter  wrote:
>
> And switch the helper over to container_of, which is a bunch faster
> than chasing a pointer. Plus allows gcc to see through this maze.
>
> Signed-off-by: Daniel Vetter 

Acked-by: Eric Anholt 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 24/44] drm/hx8357d: Use devm_drm_dev_alloc

[Eric Anholt]

On Fri, Apr 3, 2020 at 6:59 AM Daniel Vetter  wrote:
>
> Already using devm_drm_dev_init, so very simple replacment.
>
> Signed-off-by: Daniel Vetter 

Acked-by: Eric Anholt 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 12/44] drm/v3d: Use devm_drm_dev_alloc

[Eric Anholt]

On Fri, Apr 3, 2020 at 6:58 AM Daniel Vetter  wrote:
>
> Also allows us to simplify the unroll code since the drm_dev_put
> disappears.
>
> Signed-off-by: Daniel Vetter 

Acked-by: Eric Anholt 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 13/44] drm/v3d: Delete v3d_dev->dev

[Eric Anholt]

On Fri, Apr 3, 2020 at 6:58 AM Daniel Vetter  wrote:
>
> We already have it in v3d_dev->drm.dev with zero additional pointer
> chasing. Personally I don't like duplicated pointers like this
> because:
> - reviewers need to check whether the pointer is for the same or
>   different objects if there's multiple
> - compilers have an easier time too
>
> But also a bit a bikeshed, so feel free to ignore.
>
> Signed-off-by: Daniel Vetter 
> Cc: Eric Anholt 

a-b.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 14/44] drm/v3d: Delete v3d_dev->pdev

[Eric Anholt]

On Fri, Apr 3, 2020 at 6:58 AM Daniel Vetter  wrote:
>
> We already have it in v3d_dev->drm.dev with zero additional pointer
> chasing. Personally I don't like duplicated pointers like this
> because:
> - reviewers need to check whether the pointer is for the same or
> different objects if there's multiple
> - compilers have an easier time too
>
> To avoid having to pull in some big headers I implemented the casting
> function as a macro instead of a static inline. Typechecking thanks to
> container_of still assured.
>
> But also a bit a bikeshed, so feel free to ignore.
>
> Signed-off-by: Daniel Vetter 
> Cc: Eric Anholt 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [Mesa-dev] gitlab.fd.o financial situation and impact on services

[Eric Anholt]

On Fri, Feb 28, 2020 at 12:48 AM Dave Airlie  wrote:
>
> On Fri, 28 Feb 2020 at 18:18, Daniel Stone  wrote:
> >
> > On Fri, 28 Feb 2020 at 03:38, Dave Airlie  wrote:
> > > b) we probably need to take a large step back here.
> > >
> > > Look at this from a sponsor POV, why would I give X.org/fd.o
> > > sponsorship money that they are just giving straight to google to pay
> > > for hosting credits? Google are profiting in some minor way from these
> > > hosting credits being bought by us, and I assume we aren't getting any
> > > sort of discounts here. Having google sponsor the credits costs google
> > > substantially less than having any other company give us money to do
> > > it.
> >
> > The last I looked, Google GCP / Amazon AWS / Azure were all pretty
> > comparable in terms of what you get and what you pay for them.
> > Obviously providers like Packet and Digital Ocean who offer bare-metal
> > services are cheaper, but then you need to find someone who is going
> > to properly administer the various machines, install decent
> > monitoring, make sure that more storage is provisioned when we need
> > more storage (which is basically all the time), make sure that the
> > hardware is maintained in decent shape (pretty sure one of the fd.o
> > machines has had a drive in imminent-failure state for the last few
> > months), etc.
> >
> > Given the size of our service, that's a much better plan (IMO) than
> > relying on someone who a) isn't an admin by trade, b) has a million
> > other things to do, and c) hasn't wanted to do it for the past several
> > years. But as long as that's the resources we have, then we're paying
> > the cloud tradeoff, where we pay more money in exchange for fewer
> > problems.
>
> Admin for gitlab and CI is a full time role anyways. The system is
> definitely not self sustaining without time being put in by you and
> anholt still. If we have $75k to burn on credits, and it was diverted
> to just pay an admin to admin the real hw + gitlab/CI would that not
> be a better use of the money? I didn't know if we can afford $75k for
> an admin, but suddenly we can afford it for gitlab credits?

As I think about the time that I've spent at google in less than a
year on trying to keep the lights on for CI and optimize our
infrastructure in the current cloud environment, that's more than the
entire yearly budget you're talking about here.  Saying "let's just
pay for people to do more work instead of paying for full-service
cloud" is not a cost optimization.


> > Yes, we could federate everything back out so everyone runs their own
> > builds and executes those. Tinderbox did something really similar to
> > that IIRC; not sure if Buildbot does as well. Probably rules out
> > pre-merge testing, mind.
>
> Why? does gitlab not support the model? having builds done in parallel
> on runners closer to the test runners seems like it should be a thing.
> I guess artifact transfer would cost less then as a result.

Let's do some napkin math.  The biggest artifacts cost we have in Mesa
is probably meson-arm64/meson-arm (60MB zipped from meson-arm64,
downloaded by 4 freedreno and 6ish lava, about 100 pipelines/day,
makes ~1.8TB/month ($180 or so).  We could build a local storage next
to the lava dispatcher so that the artifacts didn't have to contain
the rootfs that came from the container (~2/3 of the insides of the
zip file), but that's another service to build and maintain.  Building
the drivers once locally and storing it would save downloading the
other ~1/3 of the inside of the zip file, but that requires a big
enough system to do builds in time.

I'm planning on doing a local filestore for google's lava lab, since I
need to be able to move our xml files off of the lava DUTs to get the
xml results we've become accustomed to, but this would not bubble up
to being a priority for my time if I wasn't doing it anyway.  If it
takes me a single day to set all this up (I estimate a couple of
weeks), that costs my employer a lot more than sponsoring the costs of
the inefficiencies of the system that has accumulated.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/52] drm/v3d: Use drmm_add_final_kfree

[Eric Anholt]

On Wed, Feb 19, 2020 at 2:21 AM Daniel Vetter  wrote:
>
> With this we can drop the final kfree from the release function.
>
> I also noticed that the unwind code is wrong, after drm_dev_init the
> drm_device owns the v3d allocation, so the kfree(v3d) is a double-free.
> Reorder the setup to fix this issue.
>
> After a bit more prep in drivers and drm core v3d should be able to
> switch over to devm_drm_dev_init, which should clean this up further.
>
> Signed-off-by: Daniel Vetter 
> Cc: Eric Anholt 

Acked-by: Eric Anholt 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 3/4] drm/msm: Use dma_resv locking wrappers

[Eric Anholt]

On Fri, Dec 13, 2019 at 12:08 PM Daniel Vetter  wrote:
>
> On Mon, Nov 25, 2019 at 10:43:55AM +0100, Daniel Vetter wrote:
> > I'll add more fancy logic to them soon, so everyone really has to use
> > them. Plus they already provide some nice additional debug
> > infrastructure on top of direct ww_mutex usage for the fences tracked
> > by dma_resv.
> >
> > Signed-off-by: Daniel Vetter 
> > Cc: Rob Clark 
> > Cc: Sean Paul 
> > Cc: linux-arm-...@vger.kernel.org
> > Cc: freedr...@lists.freedesktop.org
>
> Ping for some review/acks.
>
> Thanks, Daniel
>
> > ---
> >  drivers/gpu/drm/msm/msm_gem_submit.c | 10 +-
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c 
> > b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index 7d04c47d0023..385d4965a8d0 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -157,7 +157,7 @@ static void submit_unlock_unpin_bo(struct 
> > msm_gem_submit *submit,
> >   msm_gem_unpin_iova(_obj->base, submit->aspace);
> >
> >   if (submit->bos[i].flags & BO_LOCKED)
> > - ww_mutex_unlock(_obj->base.resv->lock);
> > + dma_resv_unlock(msm_obj->base.resv);
> >
> >   if (backoff && !(submit->bos[i].flags & BO_VALID))
> >   submit->bos[i].iova = 0;
> > @@ -180,8 +180,8 @@ static int submit_lock_objects(struct msm_gem_submit 
> > *submit)
> >   contended = i;
> >
> >   if (!(submit->bos[i].flags & BO_LOCKED)) {
> > - ret = 
> > ww_mutex_lock_interruptible(_obj->base.resv->lock,
> > - >ticket);
> > + ret = dma_resv_lock_interruptible(msm_obj->base.resv,
> > +   >ticket);
> >   if (ret)
> >   goto fail;
> >   submit->bos[i].flags |= BO_LOCKED;
> > @@ -202,8 +202,8 @@ static int submit_lock_objects(struct msm_gem_submit 
> > *submit)
> >   if (ret == -EDEADLK) {
> >   struct msm_gem_object *msm_obj = submit->bos[contended].obj;
> >   /* we lost out in a seqno race, lock and retry.. */
> > - ret = 
> > ww_mutex_lock_slow_interruptible(_obj->base.resv->lock,
> > - >ticket);
> > + ret = dma_resv_lock_slow_interruptible(msm_obj->base.resv,
> > +>ticket);
> >   if (!ret) {
> >   submit->bos[contended].flags |= BO_LOCKED;
> >   slow_locked = contended;
> > --
> > 2.24.0
> >

Reviewed-by: Eric Anholt 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/vc4: Use dma_resv locking wrappers

[Eric Anholt]

On Fri, Dec 13, 2019 at 12:10 PM Daniel Vetter  wrote:
>
> On Mon, Nov 25, 2019 at 10:43:56AM +0100, Daniel Vetter wrote:
> > I'll add more fancy logic to them soon, so everyone really has to use
> > them. Plus they already provide some nice additional debug
> > infrastructure on top of direct ww_mutex usage for the fences tracked
> > by dma_resv.
> >
> > Signed-off-by: Daniel Vetter 
>
> Ping for some review/acks.
>
> Thanks, Daniel
>
> > ---
> >  drivers/gpu/drm/vc4/vc4_gem.c | 11 +--
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> > index 7a06cb6e31c5..e1cfc3ccd05a 100644
> > --- a/drivers/gpu/drm/vc4/vc4_gem.c
> > +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> > @@ -568,7 +568,7 @@ vc4_unlock_bo_reservations(struct drm_device *dev,
> >   for (i = 0; i < exec->bo_count; i++) {
> >   struct drm_gem_object *bo = >bo[i]->base;
> >
> > - ww_mutex_unlock(>resv->lock);
> > + dma_resv_unlock(bo->resv);
> >   }
> >
> >   ww_acquire_fini(acquire_ctx);
> > @@ -595,8 +595,7 @@ vc4_lock_bo_reservations(struct drm_device *dev,
> >  retry:
> >   if (contended_lock != -1) {
> >   bo = >bo[contended_lock]->base;
> > - ret = ww_mutex_lock_slow_interruptible(>resv->lock,
> > -acquire_ctx);
> > + ret = dma_resv_lock_slow_interruptible(bo->resv, acquire_ctx);
> >   if (ret) {
> >   ww_acquire_done(acquire_ctx);
> >   return ret;
> > @@ -609,19 +608,19 @@ vc4_lock_bo_reservations(struct drm_device *dev,
> >
> >   bo = >bo[i]->base;
> >
> > - ret = ww_mutex_lock_interruptible(>resv->lock, 
> > acquire_ctx);
> > + ret = dma_resv_lock_interruptible(bo->resv, acquire_ctx);
> >   if (ret) {
> >   int j;
> >
> >   for (j = 0; j < i; j++) {
> >   bo = >bo[j]->base;
> > - ww_mutex_unlock(>resv->lock);
> > + dma_resv_unlock(bo->resv);
> >   }
> >
> >   if (contended_lock != -1 && contended_lock >= i) {
> >   bo = >bo[contended_lock]->base;
> >
> > - ww_mutex_unlock(>resv->lock);
> > + dma_resv_unlock(bo->resv);
> >   }
> >
> >   if (ret == -EDEADLK) {
> > --
> > 2.24.0
> >

Assuming they're supposed to be exactly equivalent currently,

Acked-by: Eric Anholt 

but we should really just be using drm_gem_lock_reservations()
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 3/3] drm/vgem: use normal cached mmap'ings

[Eric Anholt]

Rob Clark  writes:

> From: Rob Clark 
>
> Since there is no real device associated with VGEM, it is impossible to
> end up with appropriate dev->dma_ops, meaning that we have no way to
> invalidate the shmem pages allocated by VGEM.  So, at least on platforms
> without drm_cflush_pages(), we end up with corruption when cache lines
> from previous usage of VGEM bo pages get evicted to memory.
>
> The only sane option is to use cached mappings.

This may be an improvement, but...

pin/unpin is only on attaching/closing the dma-buf, right?  So, great,
you flushed the cached map once after exporting the vgem dma-buf to the
actual GPU device, but from then on you still have no interface for
getting coherent access through VGEM's mapping again, which still
exists.

I feel like this is papering over something that's really just broken,
and we should stop providing VGEM just because someone wants to write
dma-buf test code without driver-specific BO alloc ioctl code.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 1/3] drm/gem: don't force writecombine mmap'ing

[Eric Anholt]

Rob Clark  writes:

> From: Rob Clark 
>
> The driver should be in control of this.
>
> Signed-off-by: Rob Clark 
> ---
> It is possible that this was masking bugs (ie. not setting appropriate
> pgprot) in drivers.  I don't have a particularly good idea for tracking
> those down (since I don't have the hw for most drivers).  Unless someone
> has a better idea, maybe land this and let driver maintainers fix any
> potential fallout in their drivers?
>
> This is necessary for the last patch to fix VGEM brokenness on arm.

This will break at least v3d and panfrost, and it looks like cirrus as
well, since you're now promoting the mapping to cached by default and
drm_gem_shmem_helper now produces cached mappings.  That's all I could
find that would break, but don't trust me on that.



signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 01/59] drm/todo: Improve drm_gem_object funcs todo

[Eric Anholt]

Daniel Vetter  writes:

> We're kinda going in the wrong direction. Spotted while typing better
> gem/prime docs.
>
> Cc: Thomas Zimmermann 
> Cc: Gerd Hoffmann 
> Cc: Rob Herring 
> Cc: Noralf Trønnes 
> Signed-off-by: Daniel Vetter 

That's a big series, but a great cleanup.  I took a look at a lot of it.
Patch 1-2, 4-10, 41-47, 49-50, and all the gem_prime_import/export drop
patches are:

Reviewed-by: Eric Anholt 

I don't currently have a plan for reading the shuffle in patch 3.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 16/16] drm/vc4: Call drm_dev_register() after all setup is done

[Eric Anholt]

Noralf Trønnes  writes:

> drm_dev_register() initializes internal clients like bootsplash as the
> last thing it does, so all setup needs to be done at this point.
>
> Fix by calling vc4_kms_load() before registering.
> Also check the error code returned from that function.
>
> Cc: Eric Anholt 
> Signed-off-by: Noralf Trønnes 

Reviewed-by: Eric Anholt 


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 15/17] drm/vc4: Convert to using __drm_atomic_helper_crtc_reset() for reset.

[Eric Anholt]

Maarten Lankhorst  writes:

> Convert vc4 to using __drm_atomic_helper_crtc_reset(), instead of
> writing its own version. Instead of open coding destroy_state(),
> call it directly for freeing the old state.
>
> Signed-off-by: Maarten Lankhorst 
> Cc: Eric Anholt 
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index e7c04a9eb219..fdf21594b050 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -1041,12 +1041,13 @@ static void vc4_crtc_destroy_state(struct drm_crtc 
> *crtc,
>  static void
>  vc4_crtc_reset(struct drm_crtc *crtc)
>  {
> - if (crtc->state)
> - vc4_crtc_destroy_state(crtc->state);
> + struct vc4_crtc_state *crtc_state =
> + kzalloc(sizeof(*crtc_state), GFP_KERNEL);
>  
> - crtc->state = kzalloc(sizeof(struct vc4_crtc_state), GFP_KERNEL);
>   if (crtc->state)
> - crtc->state->crtc = crtc;
> + vc4_crtc_destroy_state(crtc, crtc->state);
> +
> + __drm_atomic_helper_crtc_reset(crtc, _state->base);

Wouldn't it be much easier if __drm_atomic_helper_crtc_reset took in a
new state and destroyed the old state for you?  It seems like hardly a
helper as is.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 01/17] drm/vc4: Fix memory leak during gpu reset.

[Eric Anholt]

Maarten Lankhorst  writes:

> __drm_atomic_helper_crtc_destroy_state does not free memory, it only
> cleans it up. Fix this by calling the functions own destroy function.

Reviewed-by: Eric Anholt 


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Eric Anholt]

Daniel Vetter  writes:
>
> Zooming out more looking at the big picture I'd say all your work in the
> past few years has enormously simplified drm for simple drivers already.
> If we can't resolve this one here right now that just means you "only"
> made drm 98% simpler instead of maybe 99%. It's still an epic win :-)

I'd like to second this.  So many of Noralf's cleanups I think "oof,
that's a lot of work for a little cleanup here".  But we've benefited
immensely from it accumulating over the years.  Thanks again!


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects

[Eric Anholt]

Noralf Trønnes  writes:

> Den 28.01.2019 21.57, skrev Rob Herring:
>> On Sun, Dec 2, 2018 at 9:59 AM Noralf Trønnes  wrote:
>>>
>>>
>>> Den 30.11.2018 00.58, skrev Eric Anholt:
>>>> Daniel Vetter  writes:
>>>>
>>>>> On Wed, Nov 28, 2018 at 01:52:56PM -0800, Eric Anholt wrote:
>>>>>> Daniel Vetter  writes:
>>>>>>
>>>>>>> On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
>>>>>>>> Daniel Vetter  writes:
>>>>>>>>
>>>>>>>>> On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
>>>>>>>>>> Noralf Trønnes  writes:
>>>>>>>>>>> +static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
>>>>>>>>>>> +{
>>>>>>>>>>> +  struct drm_gem_object *obj = vma->vm_private_data;
>>>>>>>>>>> +  struct drm_gem_shmem_object *shmem = 
>>>>>>>>>>> to_drm_gem_shmem_obj(obj);
>>>>>>>>>>> +
>>>>>>>>>>> +  drm_gem_shmem_put_pages(shmem);
>>>>>>>>>>> +  drm_gem_vm_close(vma);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +const struct vm_operations_struct drm_gem_shmem_vm_ops = {
>>>>>>>>>>> +  .fault = drm_gem_shmem_fault,
>>>>>>>>>>> +  .open = drm_gem_vm_open,
>>>>>>>>>>> +  .close = drm_gem_shmem_vm_close,
>>>>>>>>>>> +};
>>>>>>>>>>> +EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
>>>>>>>>>> I just saw a warning from drm_gem_shmem_put_pages() for
>>>>>>>>>> !shmem->pages_use_count -- I think drm_gem_vm_open() needs to
>>>>>>>>>> drm_gem_shmem_get_pages().
>>>>>>>>> Yeah we need a drm_gem_shmem_vm_open here.
>>>>>>>> Adding one of those fixed my refcounting issues, so I've sent out a v6
>>>>>>>> with it.
>>>>>>> Just realized that I've reviewed this patch already, spotted that vma
>>>>>>> management issue there too. Plus a pile of other things. From reading 
>>>>>>> that
>>>>>>> other thread discussion with Noralf concluded with "not yet ready for
>>>>>>> prime time" unfortunately :-/
>>>>>> I saw stuff about how it wasn't usable for SPI because SPI does weird
>>>>>> things with DMA mapping.  Was there something else?
>>>>> Looking through that mail it was a bunch of comments to improve the
>>>>> kerneldoc. Plus a note that buffer sharing/mmap is going to be all
>>>>> incoherent and horrible (but I guess for vkms we don't care that much).
>>>>> I'm just kinda vary of generic buffer handling that turns out to not be
>>>>> actually all that useful. We have lots of deadends and kinda-midlayers in
>>>>> this area already (but this one here definitely smells plenty better than
>>>>> lots of older ones).
>>>> FWIW, I really want shmem helpers for v3d.  The fault handling in
>>>> particular has magic I don't understand, and this is not my first fault
>>>> handler. :/
>>>
>>>
>>> If you can use it for a "real" hw driver like v3d, I think it makes a lot
>>> sense to have it as a helper. I believe udl and a future simpledrm can
>>> also make use of it.
>> 
>> FWIW, I think etnaviv at least could use this too.
>> 
>> I'm starting to look at panfrost and lima drivers and was trying to
>> figure out where to start with the GEM code. So I've been comparing
>> etnaviv, freedreno, and vgem implementations. They are all pretty
>> similar from what I see. The per driver GEM obj structs certainly are.
>> I can't bring myself to just copy etnaviv code over and do a
>> s/etnaviv/panfrost/. So searching around a bit, I ended up on this
>> thread. This seems to be what I need for panfrost (based on my brief
>> study).
>> 
>
> I gave up on this due to problems with SPI DMA.
> Eric tried to use it with vkms, but it failed. On his blog he speculates
> that it might be due to cached CPU mappings:
> https://anholt.github.io/twivc4/2018/12/03/twiv/
>
> For tinydrm I wanted cached mappings, but it might not work that well
> with shmem. Maybe that's why I had problems with SPI DMA.

Actually, for tinydrm buffers that are dma-buf exported through prime, I
really want tinydrm using WC mappings so that vc4 or v3d rendering (now
supported on Mesa master with kmsro) works.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] dma-buf: Enhance dma-fence tracing

[Eric Anholt]

Chris Wilson  writes:

> Quoting Koenig, Christian (2019-01-22 08:49:30)
>> Am 22.01.19 um 00:20 schrieb Chris Wilson:
>> > Rather than every backend and GPU driver reinventing the same wheel for
>> > user level debugging of HW execution, the common dma-fence framework
>> > should include the tracing infrastructure required for most client API
>> > level flow visualisation.
>> >
>> > With these common dma-fence level tracepoints, the userspace tools can
>> > establish a detailed view of the client <-> HW flow across different
>> > kernels. There is a strong ask to have this available, so that the
>> > userspace developer can effectively assess if they're doing a good job
>> > about feeding the beast of a GPU hardware.
>> >
>> > In the case of needing to look into more fine-grained details of how
>> > kernel internals work towards the goal of feeding the beast, the tools
>> > may optionally amend the dma-fence tracing information with the driver
>> > implementation specific. But for such cases, the tools should have a
>> > graceful degradation in case the expected extra tracepoints have
>> > changed or their format differs from the expected, as the kernel
>> > implementation internals are not expected to stay the same.
>> >
>> > It is important to distinguish between tracing for the purpose of client
>> > flow visualisation and tracing for the purpose of low-level kernel
>> > debugging. The latter is highly implementation specific, tied to
>> > a particular HW and driver, whereas the former addresses a common goal
>> > of user level tracing and likely a common set of userspace tools.
>> > Having made the distinction that these tracepoints will be consumed for
>> > client API tooling, we raise the spectre of tracepoint ABI stability. It
>> > is hoped that by defining a common set of dma-fence tracepoints, we avoid
>> > the pitfall of exposing low level details and so restrict ourselves only
>> > to the high level flow that is applicable to all drivers and hardware.
>> > Thus the reserved guarantee that this set of tracepoints will be stable
>> > (with the emphasis on depicting client <-> HW flow as opposed to
>> > driver <-> HW).
>> >
>> > In terms of specific changes to the dma-fence tracing, we remove the
>> > emission of the strings for every tracepoint (reserving them for
>> > dma_fence_init for cases where they have unique dma_fence_ops, and
>> > preferring to have descriptors for the whole fence context). strings do
>> > not pack as well into the ftrace ringbuffer and we would prefer to
>> > reduce the amount of indirect callbacks required for frequent tracepoint
>> > emission.
>> >
>> > Signed-off-by: Chris Wilson 
>> > Cc: Joonas Lahtinen 
>> > Cc: Tvrtko Ursulin 
>> > Cc: Alex Deucher 
>> > Cc: "Christian König" 
>> > Cc: Eric Anholt 
>> > Cc: Pierre-Loup Griffais 
>> > Cc: Michael Sartain 
>> > Cc: Steven Rostedt 
>> 
>> In general yes please! If possible please separate out the changes to 
>> the common dma_fence infrastructure from the i915 changes.
>
> Sure, I was just stressing the impact: remove some randomly placed
> internal debugging tracepoints, try to define useful ones instead :)
>
> On the list of things to do was to convert at least 2 other drivers
> (I was thinking nouveau/msm for simplicity, vc4 for a simpler
> introduction to drm_sched than amdgpu) over to be sure we have the right
> tracepoints.

v3d is using gpu-scheduler, and I'd love to see it using some shared
tracepoints -- I put in some of what we'd need for visualization, but I
haven't actually built visualization yet so I'm not sure it's good
enough.

vc4 isn't using gpu-scheduler yet.  I'm interested in it -- there's the
user qpu pipeline that we should expose, but supporting another pipeline
without the shared scheduler is no fun.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 2/3] drm: Add CRTC background color property (v3)

[Eric Anholt]

Brian Starkey  writes:

> Hi Matt,
>
> On Thu, Nov 15, 2018 at 02:13:45PM -0800, Matt Roper wrote:
>>Some display controllers can be programmed to present non-black colors
>>for pixels not covered by any plane (or pixels covered by the
>>transparent regions of higher planes).  Compositors that want a UI with
>>a solid color background can potentially save memory bandwidth by
>>setting the CRTC background property and using smaller planes to display
>>the rest of the content.
>>
>>To avoid confusion between different ways of encoding RGB data, we
>>define a standard 64-bit format that should be used for this property's
>>value.  Helper functions and macros are provided to generate and dissect
>>values in this standard format with varying component precision values.
>>
>>v2:
>> - Swap internal representation's blue and red bits to make it easier
>>   to read if printed out.  (Ville)
>> - Document bgcolor property in drm_blend.c.  (Sean Paul)
>> - s/background_color/bgcolor/ for consistency between property name and
>>   value storage field.  (Sean Paul)
>> - Add a convenience function to attach property to a given crtc.
>>
>>v3:
>> - Restructure ARGB component extraction macros to be easier to
>>   understand and enclose the parameters in () to avoid calculations
>>   if expressions are passed.  (Sean Paul)
>> - s/rgba/argb/ in helper function/macro names.  Even though the idea is
>>   to not worry about the internal representation of the u64, it can
>>   still be confusing to look at code that uses 'rgba' terminology, but
>>   stores values with argb ordering.  (Ville)
>>
>>Cc: dri-de...@lists.freedesktop.org
>>Cc: wei.c...@intel.com
>>Cc: harish.krupo@intel.com
>>Cc: Ville Syrjälä 
>>Cc: Sean Paul 
>>Signed-off-by: Matt Roper 
>>Reviewed-by(v2): Sean Paul 
>>---

>
>>+ *   Background color is setup with drm_crtc_add_bgcolor_property().  It
>>+ *   controls the RGB color of a full-screen, fully-opaque layer that exists
>>+ *   below all planes.  This color will be used for pixels not covered by
>>+ *   any plane and may also be blended with plane contents as allowed by a
>>+ *   plane's alpha values.  The background color defaults to black, and is
>>+ *   assumed to be black for drivers that do not expose this property.
>
> Might be worth explicitly mentioning that this value is used as-is,
> without any gamma/gamut conversion before blending, i.e. it's assumed
> to already be in whatever blend-space the CRTC is using (at least I
> assume that's the case?). As we start making the color pipe more
> complicated/explicit, the details matter more.

Raspberry Pi should be able to do background color as well, but the
question of where this property is in the pipeline (particularly for CTM
and gamma) also came up for me.  My background color would apply at the
same stage as plane composition, so before gamma and CTM.

FWIW, my background color on the writeback connector (the only one where
the value would be relevant) can have alpha.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects

[Eric Anholt]

Daniel Vetter  writes:

> On Wed, Nov 28, 2018 at 01:52:56PM -0800, Eric Anholt wrote:
>> Daniel Vetter  writes:
>> 
>> > On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
>> >> Daniel Vetter  writes:
>> >> 
>> >> > On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
>> >> >> Noralf Trønnes  writes:
>> >> >> > +static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
>> >> >> > +{
>> >> >> > +struct drm_gem_object *obj = vma->vm_private_data;
>> >> >> > +struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>> >> >> > +
>> >> >> > +drm_gem_shmem_put_pages(shmem);
>> >> >> > +drm_gem_vm_close(vma);
>> >> >> > +}
>> >> >> > +
>> >> >> > +const struct vm_operations_struct drm_gem_shmem_vm_ops = {
>> >> >> > +.fault = drm_gem_shmem_fault,
>> >> >> > +.open = drm_gem_vm_open,
>> >> >> > +.close = drm_gem_shmem_vm_close,
>> >> >> > +};
>> >> >> > +EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
>> >> >> 
>> >> >> I just saw a warning from drm_gem_shmem_put_pages() for
>> >> >> !shmem->pages_use_count -- I think drm_gem_vm_open() needs to
>> >> >> drm_gem_shmem_get_pages().
>> >> >
>> >> > Yeah we need a drm_gem_shmem_vm_open here.
>> >> 
>> >> Adding one of those fixed my refcounting issues, so I've sent out a v6
>> >> with it.
>> >
>> > Just realized that I've reviewed this patch already, spotted that vma
>> > management issue there too. Plus a pile of other things. From reading that
>> > other thread discussion with Noralf concluded with "not yet ready for
>> > prime time" unfortunately :-/
>> 
>> I saw stuff about how it wasn't usable for SPI because SPI does weird
>> things with DMA mapping.  Was there something else?
>
> Looking through that mail it was a bunch of comments to improve the
> kerneldoc. Plus a note that buffer sharing/mmap is going to be all
> incoherent and horrible (but I guess for vkms we don't care that much).
> I'm just kinda vary of generic buffer handling that turns out to not be
> actually all that useful. We have lots of deadends and kinda-midlayers in
> this area already (but this one here definitely smells plenty better than
> lots of older ones).

FWIW, I really want shmem helpers for v3d.  The fault handling in
particular has magic I don't understand, and this is not my first fault
handler. :/


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects

[Eric Anholt]

Daniel Vetter  writes:

> On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
>> Daniel Vetter  writes:
>> 
>> > On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
>> >> Noralf Trønnes  writes:
>> >> > +static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
>> >> > +{
>> >> > +   struct drm_gem_object *obj = vma->vm_private_data;
>> >> > +   struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>> >> > +
>> >> > +   drm_gem_shmem_put_pages(shmem);
>> >> > +   drm_gem_vm_close(vma);
>> >> > +}
>> >> > +
>> >> > +const struct vm_operations_struct drm_gem_shmem_vm_ops = {
>> >> > +   .fault = drm_gem_shmem_fault,
>> >> > +   .open = drm_gem_vm_open,
>> >> > +   .close = drm_gem_shmem_vm_close,
>> >> > +};
>> >> > +EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
>> >> 
>> >> I just saw a warning from drm_gem_shmem_put_pages() for
>> >> !shmem->pages_use_count -- I think drm_gem_vm_open() needs to
>> >> drm_gem_shmem_get_pages().
>> >
>> > Yeah we need a drm_gem_shmem_vm_open here.
>> 
>> Adding one of those fixed my refcounting issues, so I've sent out a v6
>> with it.
>
> Just realized that I've reviewed this patch already, spotted that vma
> management issue there too. Plus a pile of other things. From reading that
> other thread discussion with Noralf concluded with "not yet ready for
> prime time" unfortunately :-/

I saw stuff about how it wasn't usable for SPI because SPI does weird
things with DMA mapping.  Was there something else?


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/4] drm/edid: Add display_info.rgb_quant_range_selectable

[Eric Anholt]

Ville Syrjala  writes:

> From: Ville Syrjälä 
>
> Move the CEA-861 QS bit handling entirely into the edid code. No
> need to bother the drivers with this.
>
> Cc: Alex Deucher 
> Cc: "Christian König" 
> Cc: "David (ChunMing) Zhou" 
> Cc: amd-...@lists.freedesktop.org
> Cc: Eric Anholt  (supporter:DRM DRIVERS FOR VC4)
> Signed-off-by: Ville Syrjälä 

For vc4,
Acked-by: Eric Anholt 

Looks like a nice cleanup!


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 4/4] igt/v3d_*: Add new tests for the V3D UABI.

[Eric Anholt]

Petri Latvala  writes:

> On Mon, Nov 26, 2018 at 12:46:46PM -0800, Eric Anholt wrote:
>> Petri Latvala  writes:
>> 
>> > On Wed, Nov 14, 2018 at 02:28:32PM -0800, Eric Anholt wrote:
>> >> These are basic non-rendering tests of the UABI.
>> >> 
>> >> Signed-off-by: Eric Anholt 
>> >> ---
>> >>  lib/igt_v3d.c |  4 --
>> >>  tests/Makefile.am |  2 +
>> >>  tests/Makefile.sources|  6 +++
>> >>  tests/meson.build |  3 ++
>> >>  tests/v3d_ci/README   | 26 +
>> >>  tests/v3d_ci/v3d.testlist |  6 +++
>> >>  tests/v3d_get_bo_offset.c | 78 ++
>> >>  tests/v3d_get_param.c | 80 +++
>> >>  tests/v3d_mmap.c  | 55 +++
>> >
>> >
>> > Do you need a separate directory for v3d or can you use the vc4
>> > directory for v3d as well? Renamed to 'broadcom-ci' maybe?
>> 
>> Do we really need to introduce the vendor name separate from the driver
>> name?  I guess intel_ci makes some sense if it's used by Intel's CI, but
>> these are just the test lists for anyone that happens to use igt for
>> this driver.
>
> Fair enough, I was assuming there's an active CI somewhere that used
> both at the same time.
>
>
>
> Acked-by: Petri Latvala 

Thanks for taking a look at these and acking them!


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects

[Eric Anholt]

Daniel Vetter  writes:

> On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
>> Noralf Trønnes  writes:
>> > +static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
>> > +{
>> > +  struct drm_gem_object *obj = vma->vm_private_data;
>> > +  struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>> > +
>> > +  drm_gem_shmem_put_pages(shmem);
>> > +  drm_gem_vm_close(vma);
>> > +}
>> > +
>> > +const struct vm_operations_struct drm_gem_shmem_vm_ops = {
>> > +  .fault = drm_gem_shmem_fault,
>> > +  .open = drm_gem_vm_open,
>> > +  .close = drm_gem_shmem_vm_close,
>> > +};
>> > +EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
>> 
>> I just saw a warning from drm_gem_shmem_put_pages() for
>> !shmem->pages_use_count -- I think drm_gem_vm_open() needs to
>> drm_gem_shmem_get_pages().
>
> Yeah we need a drm_gem_shmem_vm_open here.

Adding one of those fixed my refcounting issues, so I've sent out a v6
with it.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects

[Eric Anholt]

Noralf Trønnes  writes:
> +static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
> +{
> + struct drm_gem_object *obj = vma->vm_private_data;
> + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> +
> + drm_gem_shmem_put_pages(shmem);
> + drm_gem_vm_close(vma);
> +}
> +
> +const struct vm_operations_struct drm_gem_shmem_vm_ops = {
> + .fault = drm_gem_shmem_fault,
> + .open = drm_gem_vm_open,
> + .close = drm_gem_shmem_vm_close,
> +};
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);

I just saw a warning from drm_gem_shmem_put_pages() for
!shmem->pages_use_count -- I think drm_gem_vm_open() needs to
drm_gem_shmem_get_pages().


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 4/4] igt/v3d_*: Add new tests for the V3D UABI.

[Eric Anholt]

Petri Latvala  writes:

> On Wed, Nov 14, 2018 at 02:28:32PM -0800, Eric Anholt wrote:
>> These are basic non-rendering tests of the UABI.
>> 
>> Signed-off-by: Eric Anholt 
>> ---
>>  lib/igt_v3d.c |  4 --
>>  tests/Makefile.am |  2 +
>>  tests/Makefile.sources|  6 +++
>>  tests/meson.build |  3 ++
>>  tests/v3d_ci/README   | 26 +
>>  tests/v3d_ci/v3d.testlist |  6 +++
>>  tests/v3d_get_bo_offset.c | 78 ++
>>  tests/v3d_get_param.c | 80 +++
>>  tests/v3d_mmap.c  | 55 +++
>
>
> Do you need a separate directory for v3d or can you use the vc4
> directory for v3d as well? Renamed to 'broadcom-ci' maybe?

Do we really need to introduce the vendor name separate from the driver
name?  I guess intel_ci makes some sense if it's used by Intel's CI, but
these are just the test lists for anyone that happens to use igt for
this driver.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH RFC 4/5] drm/amdgpu: Add accounting of command submission via DRM cgroup

[Eric Anholt]

Christian König  writes:

> Am 20.11.18 um 21:57 schrieb Eric Anholt:
>> Kenny Ho  writes:
>>
>>> Account for the number of command submitted to amdgpu by type on a per
>>> cgroup basis, for the purpose of profiling/monitoring applications.
>> For profiling other drivers, I've used perf tracepoints, which let you
>> get useful timelines of multiple events in the driver.  Have you made
>> use of this stat for productive profiling?
>
> Yes, but this is not related to profiling at all.
>
> What we want to do is to limit the resource usage of processes.

That sounds great, and something I'd be interested in for vc4.  However,
as far as I saw explained here, this patch doesn't let you limit
resource usage of a process and is only useful for
"profiling/monitoring" so I'm wondering how it is useful for that
purpose.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH RFC 4/5] drm/amdgpu: Add accounting of command submission via DRM cgroup

[Eric Anholt]

Kenny Ho  writes:

> Account for the number of command submitted to amdgpu by type on a per
> cgroup basis, for the purpose of profiling/monitoring applications.

For profiling other drivers, I've used perf tracepoints, which let you
get useful timelines of multiple events in the driver.  Have you made
use of this stat for productive profiling?


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH RFC 5/5] drm/amdgpu: Add accounting of buffer object creation request via DRM cgroup

[Eric Anholt]

Kenny Ho  writes:

> Account for the total size of buffer object requested to amdgpu by
> buffer type on a per cgroup basis.
>
> x prefix in the control file name x.bo_requested.amd.stat signify
> experimental.

Why is a counting of the size of buffer objects ever allocated useful,
as opposed to the current size of buffer objects allocated?

And, really, why is this stat in cgroups, instead of a debugfs entry?


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 4/5] drm/cma-helper: Add DRM_GEM_CMA_VMAP_DRIVER_OPS

[Eric Anholt]

Noralf Trønnes  writes:

> This adds functionality to the CMA helper which ensures that the kernel
> virtual address is set on the CMA GEM object also for imported buffers.
>
> The drivers have been audited to ensure that none set ->vaddr on imported
> buffers, making the conditional dma_buf_vunmap() call in
> drm_gem_cma_free_object() safe.
>
> Signed-off-by: Noralf Trønnes 

I didn't look through 1-3 much since they had acks, but 4/5 get my:

Reviewed-by: Eric Anholt 


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 1/4] kms_content_protection: Fix log bug on 32-bit platforms.

[Eric Anholt]

Ville Syrjälä  writes:

> On Wed, Nov 14, 2018 at 02:28:29PM -0800, Eric Anholt wrote:
>> long is different between 32 and 64 and should basically never be
>> used.  Fixes compiler warning about passing the wrong type.
>> 
>> Signed-off-by: Eric Anholt 
>> ---
>>  tests/kms_content_protection.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tests/kms_content_protection.c b/tests/kms_content_protection.c
>> index 801eff66c272..bb9ecd3f4cde 100644
>> --- a/tests/kms_content_protection.c
>> +++ b/tests/kms_content_protection.c
>> @@ -89,7 +89,8 @@ wait_for_prop_value(igt_output_t *output, uint64_t 
>> expected,
>>  return true;
>>  usleep(1000);
>>  }
>> -igt_info("prop_value mismatch %ld != %ld\n", val, expected);
>> +igt_info("prop_value mismatch %lld != %lld\n",
>> + (long long)val, (long long)expected);
>
> We use the ugly PRId64 & co. elsewhere for this.

My experience with those ugly macros is that people have a flinch when
trying to remember how they work and just ignore the issue instead,
leaving it for those that have to compile for 32.  I'll switch it,
though.

Hopefully i-g-t will get cross-compiling CI and merge requests at some
point so that these bugs can just never land in the first place.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t 3/4] Add v3d helper library

[Eric Anholt]

Just a few little ioctl wrappers that v3d tests will use.

Signed-off-by: Eric Anholt 
---
 lib/Makefile.sources |   2 +
 lib/drmtest.c|   3 +
 lib/drmtest.h|   1 +
 lib/igt_v3d.c| 130 +++
 lib/igt_v3d.h|  46 +++
 lib/meson.build  |   1 +
 6 files changed, 183 insertions(+)
 create mode 100644 lib/igt_v3d.c
 create mode 100644 lib/igt_v3d.h

diff --git a/lib/Makefile.sources b/lib/Makefile.sources
index e98989ff8ed9..808b9617eca2 100644
--- a/lib/Makefile.sources
+++ b/lib/Makefile.sources
@@ -107,6 +107,8 @@ lib_source_list =   \
igt_syncobj.h   \
igt_psr.c   \
igt_psr.h   \
+   igt_v3d.c   \
+   igt_v3d.h   \
$(NULL)
 
 .PHONY: version.h.tmp
diff --git a/lib/drmtest.c b/lib/drmtest.c
index fee9d33ad2a5..d2aa1c19154f 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -202,6 +202,7 @@ static const struct module {
 } modules[] = {
{ DRIVER_AMDGPU, "amdgpu" },
{ DRIVER_INTEL, "i915", modprobe_i915 },
+   { DRIVER_V3D, "v3d" },
{ DRIVER_VC4, "vc4" },
{ DRIVER_VGEM, "vgem" },
{ DRIVER_VIRTIO, "virtio-gpu" },
@@ -340,6 +341,8 @@ static const char *chipset_to_str(int chipset)
switch (chipset) {
case DRIVER_INTEL:
return "intel";
+   case DRIVER_V3D:
+   return "v3d";
case DRIVER_VC4:
return "vc4";
case DRIVER_VGEM:
diff --git a/lib/drmtest.h b/lib/drmtest.h
index 949865ee54dd..96ee517e2ec1 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -43,6 +43,7 @@
 #define DRIVER_VGEM(1 << 2)
 #define DRIVER_VIRTIO  (1 << 3)
 #define DRIVER_AMDGPU  (1 << 4)
+#define DRIVER_V3D (1 << 5)
 /*
  * Exclude DRVER_VGEM from DRIVER_ANY since if you run on a system
  * with vgem as well as a supported driver, you can end up with a
diff --git a/lib/igt_v3d.c b/lib/igt_v3d.c
new file mode 100644
index ..1a5ede1bd5fc
--- /dev/null
+++ b/lib/igt_v3d.c
@@ -0,0 +1,130 @@
+/*
+ * Copyright © 2016 Broadcom
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "drmtest.h"
+#include "igt_aux.h"
+#include "igt_core.h"
+#include "igt_v3d.h"
+#include "ioctl_wrappers.h"
+#include "intel_reg.h"
+#include "intel_chipset.h"
+#include "v3d_drm.h"
+
+#if NEW_CONTEXT_PARAM_NO_ERROR_CAPTURE_API
+#define LOCAL_CONTEXT_PARAM_NO_ERROR_CAPTURE 0x4
+#endif
+
+/**
+ * SECTION:igt_v3d
+ * @short_description: V3D support library
+ * @title: V3D
+ * @include: igt.h
+ *
+ * This library provides various auxiliary helper functions for writing V3D
+ * tests.
+ */
+
+struct v3d_bo *
+igt_v3d_create_bo(int fd, size_t size)
+{
+   struct v3d_bo *bo = calloc(1, sizeof(*bo));
+
+   struct drm_v3d_create_bo create = {
+   .size = size,
+   };
+
+   do_ioctl(fd, DRM_IOCTL_V3D_CREATE_BO, );
+
+   bo->handle = create.handle;
+   bo->offset = create.offset;
+   bo->size = size;
+
+   return bo;
+}
+
+void
+igt_v3d_free_bo(int fd, struct v3d_bo *bo)
+{
+   if (bo->map)
+   munmap(bo->map, bo->size);
+   gem_close(fd, bo->handle);
+   free(bo);
+}
+
+uint32_t
+igt_v3d_get_bo_offset(int fd, uint32_t handle)
+{
+   struct drm_v3d_get_bo_offset get = {
+   .handle = handle,
+   };
+
+   do_ioctl(fd, DRM_IOCTL_V3D_GET_BO_OFFSET, );
+
+   return get.offset;
+}
+
+uint32_t
+igt_v3d_get_param(int fd, enum drm_v3d_param param)
+{
+   str

[Intel-gfx] [PATCH i-g-t 4/4] igt/v3d_*: Add new tests for the V3D UABI.

[Eric Anholt]

These are basic non-rendering tests of the UABI.

Signed-off-by: Eric Anholt 
---
 lib/igt_v3d.c |  4 --
 tests/Makefile.am |  2 +
 tests/Makefile.sources|  6 +++
 tests/meson.build |  3 ++
 tests/v3d_ci/README   | 26 +
 tests/v3d_ci/v3d.testlist |  6 +++
 tests/v3d_get_bo_offset.c | 78 ++
 tests/v3d_get_param.c | 80 +++
 tests/v3d_mmap.c  | 55 +++
 9 files changed, 256 insertions(+), 4 deletions(-)
 create mode 100644 tests/v3d_ci/README
 create mode 100644 tests/v3d_ci/v3d.testlist
 create mode 100644 tests/v3d_get_bo_offset.c
 create mode 100644 tests/v3d_get_param.c
 create mode 100644 tests/v3d_mmap.c

diff --git a/lib/igt_v3d.c b/lib/igt_v3d.c
index 1a5ede1bd5fc..619c072c0e47 100644
--- a/lib/igt_v3d.c
+++ b/lib/igt_v3d.c
@@ -40,10 +40,6 @@
 #include "intel_chipset.h"
 #include "v3d_drm.h"
 
-#if NEW_CONTEXT_PARAM_NO_ERROR_CAPTURE_API
-#define LOCAL_CONTEXT_PARAM_NO_ERROR_CAPTURE 0x4
-#endif
-
 /**
  * SECTION:igt_v3d
  * @short_description: V3D support library
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 3d1ce0bc1af8..a6b2ba51ea4f 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -14,6 +14,8 @@ if BUILD_VC4
 TESTS_progs += $(VC4_TESTS)
 endif
 
+TESTS_progs += $(V3D_TESTS)
+
 if HAVE_CHAMELIUM
 TESTS_progs += \
kms_chamelium \
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index d007ebc74ab9..3ed60e7c30c7 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -15,6 +15,12 @@ VC4_TESTS = \
vc4_wait_seqno \
$(NULL)
 
+V3D_TESTS = \
+   v3d_get_bo_offset \
+   v3d_get_param \
+   v3d_mmap \
+   $(NULL)
+
 AMDGPU_TESTS = \
amdgpu/amd_basic \
amdgpu/amd_cs_nop \
diff --git a/tests/meson.build b/tests/meson.build
index 3020f7984d7a..4472536aef65 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -85,6 +85,9 @@ test_progs = [
'syncobj_wait',
'template',
'tools_test',
+   'v3d_get_bo_offset',
+   'v3d_get_param',
+   'v3d_mmap',
'vc4_create_bo',
'vc4_dmabuf_poll',
'vc4_label_bo',
diff --git a/tests/v3d_ci/README b/tests/v3d_ci/README
new file mode 100644
index ..e03c552fb972
--- /dev/null
+++ b/tests/v3d_ci/README
@@ -0,0 +1,26 @@
+This directory contains test lists to be used for v3d's DRM support. The files
+are passed to piglit with the --test-list parameter directly.
+
+The test lists are contained in the IGT repository for several
+reasons:
+
+- The lists stay synchronized with the IGT codebase.
+- Public availability. Kernel developers can see what tests are run,
+  and can see what changes are done to the set, when, and why.
+- Explicit test lists in general make it possible to implement a new
+  test without having it run by everyone else before the tests and / or setup
+  are ready for it.
+
+Changing the test lists should only happen with approval from the v3d
+maintainer, Eric Anholt (e...@anholt.net).
+
+
+v3d.testlist
+
+
+This test list is meant as a general test suite without any time
+restriction for the v3d DRM driver, combining generic DRM and KMS
+tests.  As a reminder, you can run this with the meson build using:
+
+./build/runner/igt_runner --test-list tests/v3d_ci/v3d.testlist \
+  build/tests -o results
diff --git a/tests/v3d_ci/v3d.testlist b/tests/v3d_ci/v3d.testlist
new file mode 100644
index ..b55e8e571d7d
--- /dev/null
+++ b/tests/v3d_ci/v3d.testlist
@@ -0,0 +1,6 @@
+igt@v3d_get_bo_offset@create-get-offsets
+igt@v3d_get_bo_offset@get-bad-handle
+igt@v3d_get_param@base-params
+igt@v3d_get_param@get-bad-param
+igt@v3d_get_param@get-bad-flags
+igt@v3d_mmap@mmap-bad-handle
diff --git a/tests/v3d_get_bo_offset.c b/tests/v3d_get_bo_offset.c
new file mode 100644
index ..0923dc85f0d0
--- /dev/null
+++ b/tests/v3d_get_bo_offset.c
@@ -0,0 +1,78 @@
+/*
+ * Copyright © 2017 Broadcom
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT 

[Intel-gfx] [PATCH i-g-t 1/4] kms_content_protection: Fix log bug on 32-bit platforms.

[Eric Anholt]

long is different between 32 and 64 and should basically never be
used.  Fixes compiler warning about passing the wrong type.

Signed-off-by: Eric Anholt 
---
 tests/kms_content_protection.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/kms_content_protection.c b/tests/kms_content_protection.c
index 801eff66c272..bb9ecd3f4cde 100644
--- a/tests/kms_content_protection.c
+++ b/tests/kms_content_protection.c
@@ -89,7 +89,8 @@ wait_for_prop_value(igt_output_t *output, uint64_t expected,
return true;
usleep(1000);
}
-   igt_info("prop_value mismatch %ld != %ld\n", val, expected);
+   igt_info("prop_value mismatch %lld != %lld\n",
+(long long)val, (long long)expected);
 
return false;
 }
-- 
2.19.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t 2/4] v3d: Import the V3D DRM UAPI header.

[Eric Anholt]

Copied from make headers_install at drm-misc-next 783195ec1cad
("drm/syncobj: disable the timeline UAPI for now v2")

Signed-off-by: Eric Anholt 
---
 include/drm-uapi/v3d_drm.h | 204 +
 1 file changed, 204 insertions(+)
 create mode 100644 include/drm-uapi/v3d_drm.h

diff --git a/include/drm-uapi/v3d_drm.h b/include/drm-uapi/v3d_drm.h
new file mode 100644
index ..f446656d00b1
--- /dev/null
+++ b/include/drm-uapi/v3d_drm.h
@@ -0,0 +1,204 @@
+/*
+ * Copyright © 2014-2018 Broadcom
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef _V3D_DRM_H_
+#define _V3D_DRM_H_
+
+#include "drm.h"
+
+#if defined(__cplusplus)
+extern "C" {
+#endif
+
+#define DRM_V3D_SUBMIT_CL 0x00
+#define DRM_V3D_WAIT_BO   0x01
+#define DRM_V3D_CREATE_BO 0x02
+#define DRM_V3D_MMAP_BO   0x03
+#define DRM_V3D_GET_PARAM 0x04
+#define DRM_V3D_GET_BO_OFFSET 0x05
+
+#define DRM_IOCTL_V3D_SUBMIT_CL   DRM_IOWR(DRM_COMMAND_BASE + 
DRM_V3D_SUBMIT_CL, struct drm_v3d_submit_cl)
+#define DRM_IOCTL_V3D_WAIT_BO DRM_IOWR(DRM_COMMAND_BASE + 
DRM_V3D_WAIT_BO, struct drm_v3d_wait_bo)
+#define DRM_IOCTL_V3D_CREATE_BO   DRM_IOWR(DRM_COMMAND_BASE + 
DRM_V3D_CREATE_BO, struct drm_v3d_create_bo)
+#define DRM_IOCTL_V3D_MMAP_BO DRM_IOWR(DRM_COMMAND_BASE + 
DRM_V3D_MMAP_BO, struct drm_v3d_mmap_bo)
+#define DRM_IOCTL_V3D_GET_PARAM   DRM_IOWR(DRM_COMMAND_BASE + 
DRM_V3D_GET_PARAM, struct drm_v3d_get_param)
+#define DRM_IOCTL_V3D_GET_BO_OFFSET   DRM_IOWR(DRM_COMMAND_BASE + 
DRM_V3D_GET_BO_OFFSET, struct drm_v3d_get_bo_offset)
+
+/**
+ * struct drm_v3d_submit_cl - ioctl argument for submitting commands to the 3D
+ * engine.
+ *
+ * This asks the kernel to have the GPU execute an optional binner
+ * command list, and a render command list.
+ */
+struct drm_v3d_submit_cl {
+   /* Pointer to the binner command list.
+*
+* This is the first set of commands executed, which runs the
+* coordinate shader to determine where primitives land on the screen,
+* then writes out the state updates and draw calls necessary per tile
+* to the tile allocation BO.
+*
+* This BCL will block on any previous BCL submitted on the
+* same FD, but not on any RCL or BCLs submitted by other
+* clients -- that is left up to the submitter to control
+* using in_sync_bcl if necessary.
+*/
+   __u32 bcl_start;
+
+/** End address of the BCL (first byte after the BCL) */
+   __u32 bcl_end;
+
+   /* Offset of the render command list.
+*
+* This is the second set of commands executed, which will either
+* execute the tiles that have been set up by the BCL, or a fixed set
+* of tiles (in the case of RCL-only blits).
+*
+* This RCL will block on this submit's BCL, and any previous
+* RCL submitted on the same FD, but not on any RCL or BCLs
+* submitted by other clients -- that is left up to the
+* submitter to control using in_sync_rcl if necessary.
+*/
+   __u32 rcl_start;
+
+/** End address of the RCL (first byte after the RCL) */
+   __u32 rcl_end;
+
+   /** An optional sync object to wait on before starting the BCL. */
+   __u32 in_sync_bcl;
+   /** An optional sync object to wait on before starting the RCL. */
+   __u32 in_sync_rcl;
+   /** An optional sync object to place the completion fence in. */
+   __u32 out_sync;
+
+   /* Offset of the tile alloc memory
+*
+* This is optional on V3D 3.3 (where the CL can set the value) but
+* required on V3D 4.1.
+*/
+   __u3

Re: [Intel-gfx] [PATCH v4 1/2] drm: Add drm_any_plane_has_format()

[Eric Anholt]

Ville Syrjala  writes:

> From: Ville Syrjälä 
>
> Add a function to check whether there is at least one plane that
> supports a specific format and modifier combination. Drivers can
> use this to reject unsupported formats/modifiers in .fb_create().
>
> v2: Accept anyformat if the driver doesn't do planes (Eric)
> s/planes_have_format/any_plane_has_format/ (Eric)
> Check the modifier as well since we already have a function
> that does both
> v3: Don't do the check in the core since we may not know the
> modifier yet, instead export the function and let drivers
> call it themselves
>
> Cc: Eric Anholt 
> Cc: Dhinakaran Pandiyan 
> Signed-off-by: Ville Syrjälä 
> Reviewed-by: Dhinakaran Pandiyan 

I don't particularly see the point in having FB creation duplicate the
validation that atomic check will eventually do, and it means that FB
creation cost scales with plane count, but if i915's going to do this,
it seems reasonable for them.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [igt-dev] [PATCH] RFC: Make igts for cross-driver stuff mandatory?

[Eric Anholt]

Sean Paul  writes:

> On Fri, Oct 19, 2018 at 10:50:49AM +0200, Daniel Vetter wrote:
>> Hi all,
>> 
>> This is just to collect feedback on this idea, and see whether the
>> overall dri-devel community stands on all this. I think the past few
>> cross-vendor uapi extensions all came with igts attached, and
>> personally I think there's lots of value in having them: A
>> cross-vendor interface isn't useful if every driver implements it
>> slightly differently.
>> 
>> I think there's 2 questions here:
>> 
>> - Do we want to make such testcases mandatory?
>> 
>
> Yes, more testing == better code.
>
>
>> - If yes, are we there yet, or is there something crucially missing
>>   still?
>
> In my experience, no. Last week while trying to replicate an intel-gfx CI
> failure, I tried compiling igt for one of my (intel) chromebooks. It seems 
> like
> cross-compilation (or, in my case, just specifying
> prefix/ld_library_path/sbin_path) is broken on igt. If we want to impose
> restrictions across the entire subsystem, we need to make sure that everyone 
> can
> build and deploy igt easily.
>
> I managed to hack around everything and get it working, but I still haven't
> tried switching out the toolchain. Once we have some GitLab CI to validate
> cross-compilation, then we can consider making IGT mandatory.
>
> It's possible that I'm just a meson n00b and didn't use the right incantation,
> so maybe it already works, but then we need better documentation.
>
> I've pasted my horrible hacks below, I also didn't have libunwind, so removed
> its usage.

I've also had to cut out libunwind for cross-compiling on many
occasions.  Worst library.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 13/18] drm/vc4: Use drm_atomic_helper_shutdown

[Eric Anholt]

Daniel Vetter  writes:

> drm_plane_helper_disable is a non-atomic drivers only function, and
> will blow up (since no one passes the locking context it needs).
>
> Atomic drivers which want to quiescent their hw on unload should
> use drm_atomic_helper_shutdown() instead.
>
> v2: Rebase.

I've definitely never tested unload.

Looks like we could drop vc4_plane_destroy() entirely?  Regardless,
acked-by.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 09/10] drm/vc4: Use drm_crtc_mask()

[Eric Anholt]

Ville Syrjala  writes:

> From: Ville Syrjälä 
>
> Use drm_crtc_mask() where appropriate.

Reviewed-by: Eric Anholt 


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 12/13] drm/vc4: Stop updating plane->fb/crtc

[Eric Anholt]

Ville Syrjala  writes:

> From: Ville Syrjälä 
>
> We want to get rid of plane->fb/crtc on atomic drivers. Stop setting
> them.
>
> Cc: Eric Anholt 
> Signed-off-by: Ville Syrjälä 
> Reviewed-by: Maarten Lankhorst 
> Reviewed-by: Daniel Vetter 

Reviewed-by: Eric Anholt 


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/9] drm/pl111: Set .gem_prime_vmap and .gem_prime_mmap

[Eric Anholt]

Noralf Trønnes  writes:

> These are needed for pl111 to use the generic fbdev emulation.

Reviewed-by: Eric Anholt 


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Promote .format_mod_supported() to the lead role

[Eric Anholt]

Ville Syrjälä  writes:

> On Mon, May 21, 2018 at 12:21:01PM -0700, Eric Anholt wrote:
>> Ville Syrjala  writes:
>> 
>> > From: Ville Syrjälä 
>> >
>> > Up to now we've used the plane's modifier list as the primary
>> > source of information for which modifiers are supported by a
>> > given plane. In order to allow auxiliary metadata to be embedded
>> > within the bits of the modifier we need to stop doing that.
>> >
>> > Thus we have to make .format_mod_supported() aware of the plane's
>> > capabilities and gracefully deal with any modifier being passed
>> > in directly from userspace.
>> 
>> This seems like it would be a lot shorter if you just had a helper to
>> check if your format and modifier was in drm_plane->format_types and
>> drm_plane->modifiers, since then you wouldn't be duplicating your tables
>> and you wouldn't need has_ccs either.
>
> I suppose. And I guess that's where I started originally :/
>
> But I'm not sure if it's better go that route or the other route of
> reducing the arrays to some simple supersets and also utilize
> .format_mod_supported() in plane init to filter out the unsupported
> formats when populating the plane's format list. Probably best not
> dwell on this too much for now so that we can at least make some
> progress :)
>
>> 
>> However, it's not my driver and it unblocks vc4's patch, so:
>> 
>> Reviewed-by: Eric Anholt 
>
> Thanks. I'm guessing we should push this into drm-misc-next so
> that you can pile your core/sand bits on top?

That would be wonderful.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915: Promote .format_mod_supported() to the lead role

[Eric Anholt]

Ville Syrjala <ville.syrj...@linux.intel.com> writes:

> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
> Up to now we've used the plane's modifier list as the primary
> source of information for which modifiers are supported by a
> given plane. In order to allow auxiliary metadata to be embedded
> within the bits of the modifier we need to stop doing that.
>
> Thus we have to make .format_mod_supported() aware of the plane's
> capabilities and gracefully deal with any modifier being passed
> in directly from userspace.

This seems like it would be a lot shorter if you just had a helper to
check if your format and modifier was in drm_plane->format_types and
drm_plane->modifiers, since then you wouldn't be duplicating your tables
and you wouldn't need has_ccs either.

However, it's not my driver and it unblocks vc4's patch, so:

Reviewed-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PULL] drm-misc-next

[Eric Anholt]

Maarten Lankhorst  writes:

> Hey,
>
> Another pull request for drm-misc-next. Previous one was not applied yet,
> but only sending delta since last request:
> https://lists.freedesktop.org/archives/dri-devel/2018-May/175722.html

Note, I think this PR has a UABI regression in it:

https://patchwork.freedesktop.org/patch/221618/


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] dma-fence: remove fill_driver_data callback

[Eric Anholt]

Daniel Vetter <daniel.vet...@ffwll.ch> writes:

> Noticed while I was typing docs. Entirely unused.
>
> v2: Remove reference in @timeline_value_str too. While at it clarify
> why timeline_value_str has a fence parameter - we don't have an
> explicit timeline structure unfortunately.
>
> Cc: Eric Anholt <e...@anholt.net>

Reviewed-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 16/17] drm/virtio: Remove unecessary dma_fence_ops

[Eric Anholt]

Daniel Vetter <daniel.vet...@ffwll.ch> writes:

> dma_fence_default_wait is the default now, same for the trivial
> enable_signaling implementation.

Reviewed-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 12/17] drm/qxl: Remove unecessary dma_fence_ops

[Eric Anholt]

Daniel Vetter <daniel.vet...@ffwll.ch> writes:

> dma_fence_default_wait is the default now, same for the trivial
> enable_signaling implementation.

Drop the mention of dma_fence_default_wait, since this one doesn't use
that?  Other than that,

Reviewed-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 07/17] drm: Remove unecessary dma_fence_ops

[Eric Anholt]

Daniel Vetter <daniel.vet...@ffwll.ch> writes:

> dma_fence_default_wait is the default now, same for the trivial
> enable_signaling implementation.
>
> Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>

Reviewed-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 14/17] drm/vc4: Remove unecessary dma_fence_ops

[Eric Anholt]

Daniel Vetter <daniel.vet...@ffwll.ch> writes:

> dma_fence_default_wait is the default now, same for the trivial
> enable_signaling implementation.
>
> Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>

Reviewed-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 02/17] dma-fence: remove fill_driver_data callback

[Eric Anholt]

Daniel Vetter  writes:

> Noticed while I was typing docs. Entirely unused.
>
> Signed-off-by: Daniel Vetter 
> ---
>  include/linux/dma-fence.h | 10 --
>  1 file changed, 10 deletions(-)
>
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 9d6f39bf2111..f9a6848f8558 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -217,16 +217,6 @@ struct dma_fence_ops {
>*/
>   void (*release)(struct dma_fence *fence);
>  
> - /**
> -  * @fill_driver_data:
> -  *
> -  * Callback to fill in free-form debug info Returns amount of bytes
> -  * filled, or negative error on failure.
> -  *
> -  * This callback is optional.
> -  */
> - int (*fill_driver_data)(struct dma_fence *fence, void *data, int size);
> -

There's a reference to this one from timeline_value_str() as well.
Could you fix that up?


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 01/17] dma-fence: Some kerneldoc polish for dma-fence.h

[Eric Anholt]

Daniel Vetter <daniel.vet...@ffwll.ch> writes:
> + /**
> +  * @fill_driver_data:
> +  *
> +  * Callback to fill in free-form debug info Returns amount of bytes
> +  * filled, or negative error on failure.

Maybe this "Returns" should be on a new line?  Or at least a '.' in
between.

Other than that,

Reviewed-by: Eric Anholt <e...@anholt.net>

Thanks!


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [Freedreno] [PATCH 01/10] include: Move ascii85 functions from i915 to linux/ascii85.h

[Eric Anholt]

Chris Wilson  writes:

> Quoting Jordan Crouse (2018-04-05 23:06:53)
>> On Thu, Apr 05, 2018 at 04:00:47PM -0600, Jordan Crouse wrote:
>> > The i915 DRM driver very cleverly used ascii85 encoding for their
>> > GPU state file. Move the encode functions to a general header file to
>> > support other drivers that might be interested in the same
>> > functionality.
>> 
>> In a previous version of this patch, Chris asked what tree I wanted this 
>> applied
>> to, and the answer is: I'm not sure?  I'm hoping that Rob will be cool with
>> picking the rest up for msm-next for 4.18 but I'm okay with putting this
>> particular patch wherever it is easiest for the maintainers.
>
> We are a bit late to sneak it into the 4.17 drm base via i915. I don't
> anticipate any problems (for i915) with this patch going in through
> msm-next, so happy to have it land there and percolate back to i915 3
> months later.

I'd love to have it in drm-misc-next, so I can build a similar hang dump
interface for vc5.  But most importantly, I'd like to have it somewhere
soon.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Promote .format_mod_supported() to the lead role

[Eric Anholt]

Ville Syrjala <ville.syrj...@linux.intel.com> writes:

> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
> Up to now we've used the plane's modifier list as the primary
> source of information for which modifiers are supported by a
> given plane. In order to allow auxiliary metadata to be embedded
> within the bits of the modifier we need to stop doing that.
>
> Thus we have to make .format_mod_supported() aware of the plane's
> capabilities and gracefully deal with any modifier being passed
> in directly from userspace.

I took a look, and I think you have the chance to delete a whole ton of
code if you keep the assumption that the core will check that the format
is one of plane->format_types.

>
> Cc: Eric Anholt <e...@anholt.net>
> References: 
> https://lists.freedesktop.org/archives/dri-devel/2018-March/169782.html
> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 147 +++---
>  drivers/gpu/drm/i915/intel_drv.h |   1 +
>  drivers/gpu/drm/i915/intel_sprite.c  | 194 
> ++-
>  3 files changed, 210 insertions(+), 132 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 3e7ab75e1b41..d717004be0b8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -88,15 +88,7 @@ static const uint32_t skl_primary_formats[] = {
>   DRM_FORMAT_VYUY,
>  };
>  
> -static const uint64_t skl_format_modifiers_noccs[] = {
> - I915_FORMAT_MOD_Yf_TILED,
> - I915_FORMAT_MOD_Y_TILED,
> - I915_FORMAT_MOD_X_TILED,
> - DRM_FORMAT_MOD_LINEAR,
> - DRM_FORMAT_MOD_INVALID
> -};
> -
> -static const uint64_t skl_format_modifiers_ccs[] = {
> +static const uint64_t skl_format_modifiers[] = {
>   I915_FORMAT_MOD_Yf_TILED_CCS,
>   I915_FORMAT_MOD_Y_TILED_CCS,
>   I915_FORMAT_MOD_Yf_TILED,
> @@ -12997,8 +12989,17 @@ void intel_plane_destroy(struct drm_plane *plane)
>   kfree(to_intel_plane(plane));
>  }
>  
> -static bool i8xx_mod_supported(uint32_t format, uint64_t modifier)
> +static bool i8xx_plane_format_mod_supported(struct drm_plane *_plane,
> + u32 format, u64 modifier)
>  {
> + switch (modifier) {
> + case DRM_FORMAT_MOD_LINEAR:
> + case I915_FORMAT_MOD_X_TILED:
> + break;
> + default:
> + return false;
> + }
> +

I think you could just remove the format-dependent switch below in favor
of s/break/return true/.  It's just a list of all the formats in
i8xx_primary_formats[].

>   switch (format) {
>   case DRM_FORMAT_C8:
>   case DRM_FORMAT_RGB565:
> @@ -13011,8 +13012,17 @@ static bool i8xx_mod_supported(uint32_t format, 
> uint64_t modifier)
>   }
>  }
>  
> -static bool i965_mod_supported(uint32_t format, uint64_t modifier)
> +static bool i965_plane_format_mod_supported(struct drm_plane *_plane,
> + u32 format, u64 modifier)
>  {
> + switch (modifier) {
> + case DRM_FORMAT_MOD_LINEAR:
> + case I915_FORMAT_MOD_X_TILED:
> + break;
> + default:
> + return false;
> + }

Again, there's no format dependence, so drop the switch statement, and
probably just reuse the mod_supported func from 8xx.

> +
>   switch (format) {
>   case DRM_FORMAT_C8:
>   case DRM_FORMAT_RGB565:
> @@ -13027,17 +13037,37 @@ static bool i965_mod_supported(uint32_t format, 
> uint64_t modifier)
>   }
>  }
>  
> -static bool skl_mod_supported(uint32_t format, uint64_t modifier)
> +static bool skl_plane_format_mod_supported(struct drm_plane *_plane,
> +u32 format, u64 modifier)
>  {
> + struct intel_plane *plane = to_intel_plane(_plane);
> +
> + switch (modifier) {
> + case DRM_FORMAT_MOD_LINEAR:
> + case I915_FORMAT_MOD_X_TILED:
> + case I915_FORMAT_MOD_Y_TILED:
> + case I915_FORMAT_MOD_Yf_TILED:
> + break;
> + case I915_FORMAT_MOD_Y_TILED_CCS:
> + case I915_FORMAT_MOD_Yf_TILED_CCS:
> + if (!plane->has_ccs)
> + return false;
> + break;
> + default:
> + return false;
> + }
> +
>   switch (format) {
>   case DRM_FORMAT_XRGB:
>   case DRM_FORMAT_XBGR:
>   case DRM_FORMAT_ARGB:
>   case DRM_FORMAT_ABGR:
> - if (modifier == I915_FORMAT_MOD_Yf_TILED_CCS ||
> - modifier == I915_FORMAT_MOD_Y_TILED_CCS)
> - re

Re: [Intel-gfx] [RFC PATCH i-g-t 0/3] Test the plane formats on the Chamelium

[Eric Anholt]

Maxime Ripard  writes:

> [ Unknown signature status ]
> Hi,
>
> On Mon, Mar 05, 2018 at 03:21:26PM +0100, Maxime Ripard wrote:
>> Here is an RFC at starting to test the plane formats using the
>> Chamelium over the HDMI. This was tested using the vc4 DRM driver
>> found on the RaspberryPi.
>> 
>> This is still pretty rough around the edges at this point, but I'd
>> like to get feedback on a few issues before getting any further.
>> 
>>   * I've used pixman for now to convert back and forth the pattern and
>> the captured frame. While this worked quite well for the RGB
>> formats since pixman supports most (but not all) of them. However,
>> the long term plan is to also test YUV and more exotic (like
>> vendor specific) formats that pixman has 0 support for. So I
>> really wonder whether this is the right approach compared to:
>> - Using something else (but what?)?
>> - Rolling our own format conversion library?

Let's start with pixman and either extend pixman if we have formats we
need (they should be pretty amenable for non-yuv channel layouts), or
roll our own YUV bits.  For tiling, I think we can just take
pixman-generated linear image content and do the tiling in igt.

>>   * I've so far had a single big test that will test all the formats
>> exposed by the planes that have a pixman representation. I wonder
>> whether this is preferrable, or if we want to have a subtest per
>> format. I guess the latter will be slightly better since we would
>> be able to catch regressions in the number of formats exposed that
>> we wouldn't be able to with the former.

Yeah, exposing the formats as subtests is probably a good idea.

>>   * Kind of related, I'm not sure what is the policy when it comes to
>> tests, and whether I should merge this tests with kms_chamelium or
>> leave it as a separate file.

I'll leave this up to the original test author.

>>   * One of the biggest challenge of the serie is to support formats
>> that have less bits than the reference frame. Indeed, the flow of
>> patterns is this one: the pattern will first be generated in
>> ARGB. It will then be converted to whatever format we want to
>> test, be fed into the display engine, that will output it, and the
>> Chamelium will capture it in ARGB.
>> However, when the plane format has less than 8 bits per color,
>> some upsampling will happen, where the less significant bits will
>> be filled with values that probably depend on the display
>> engine. Another side effect is that the CRC used in the Chamelium
>> tests cannot be used anymore.
>> The way I'm testing currently is that I'm retrieving the frame,
>> and then compare each pixels on their most significant bits. This
>> sounds inefficient, and it is, especially on the RPi that doesn't
>> have the best networking throughput out there.
>> I guess we could also generate a CRC for both an upsampling with
>> the lowest bits set to 1, and one for the lowest bits set to 0,
>> and try to see if one of them match. I guess this should cover
>> most of the situation.

I still think that we should expect the top bits to be replicated into
the low bits, until we find hardware that just can't do that.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/3] drm: Make sure at least one plane supports the fb format

[Eric Anholt]

Ville Syrjälä <ville.syrj...@linux.intel.com> writes:

> On Thu, Mar 15, 2018 at 08:03:44PM +0200, Ville Syrjälä wrote:
>> On Thu, Mar 15, 2018 at 07:48:02PM +0200, Ville Syrjälä wrote:
>> > On Thu, Mar 15, 2018 at 10:42:17AM -0700, Eric Anholt wrote:
>> > > Ville Syrjala <ville.syrj...@linux.intel.com> writes:
>> > > 
>> > > > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>> > > >
>> > > > To make life easier for drivers, let's have the core check that the
>> > > > requested pixel format is supported by at least one plane when creating
>> > > > a new framebuffer.
>> > > >
>> > > > This eases the burden on drivers by making sure they'll never get
>> > > > requests to create fbs with unsupported pixel formats. Thanks to the
>> > > > new .fb_modifier() hook this check can now be done whether the request
>> > > > specifies the modifier directly or driver has to deduce it from the
>> > > > gem bo tiling (or via any other method really).
>> > > >
>> > > > v0: Accept anyformat if the driver doesn't do planes (Eric)
>> > > > s/planes_have_format/any_plane_has_format/ (Eric)
>> > > > Check the modifier as well since we already have a function
>> > > > that does both
>> > > > v3: Don't do the check in the core since we may not know the
>> > > > modifier yet, instead export the function and let drivers
>> > > > call it themselves
>> > > > v4: Unexport the functiona and put the format_default check back
>> > > > since this will again be called by the core, ie. undo v3 ;)
>> > > >
>> > > > Cc: Eric Anholt <e...@anholt.net>
>> > > > Testcase: igt/kms_addfb_basic/expected-formats
>> > > > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
>> > > > ---
>> > > >  drivers/gpu/drm/drm_framebuffer.c | 30 ++
>> > > >  1 file changed, 30 insertions(+)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c 
>> > > > b/drivers/gpu/drm/drm_framebuffer.c
>> > > > index 21d3d51eb261..e618a6b728d4 100644
>> > > > --- a/drivers/gpu/drm/drm_framebuffer.c
>> > > > +++ b/drivers/gpu/drm/drm_framebuffer.c
>> > > > @@ -152,6 +152,26 @@ static int fb_plane_height(int height,
>> > > >return DIV_ROUND_UP(height, format->vsub);
>> > > >  }
>> > > >  
>> > > > +static bool any_plane_has_format(struct drm_device *dev,
>> > > > +   u32 format, u64 modifier)
>> > > > +{
>> > > > +  struct drm_plane *plane;
>> > > > +
>> > > > +  drm_for_each_plane(plane, dev) {
>> > > > +  /*
>> > > > +   * In case the driver doesn't really do
>> > > > +   * planes we have to accept any format here.
>> > > > +   */
>> > > > +  if (plane->format_default)
>> > > > +  return true;
>> > > > +
>> > > > +  if (drm_plane_check_pixel_format(plane, format, 
>> > > > modifier) == 0)
>> > > > +  return true;
>> > > > +  }
>> > > > +
>> > > > +  return false;
>> > > > +}
>> > > 
>> > > This drm_plane_check_pixel_format() will always fail for VC4's SAND
>> > > modifiers or VC5's UIF modifiers, where we're using the middle 48 bits
>> > > as a bit of metadata (like how we have horizontal stride passed outside
>> > > of the modifier) and you can't list all of the possible values in an
>> > > array on the plane.
>> > 
>> > Hmm. drm_atomic_plane_check() etc. call this thing as well. How does
>> > that manage to work currently?
>> 
>> Maybe it doesn't. I added the modifier checks in
>> 
>> commit 23163a7d4b032489d375099d56571371c0456980
>> Author: Ville Syrjälä <ville.syrj...@linux.intel.com>
>> AuthorDate: Fri Dec 22 21:22:30 2017 +0200
>> Commit: Ville Syrjälä <ville.syrj...@linux.intel.com>
>> CommitDate: Mon Feb 26 16:29:47 2018 +0200
>> 
>> drm: Check that the plane supports the request format+modifier combo
>> 
>> M

Re: [Intel-gfx] [PATCH 2/3] drm: Make sure at least one plane supports the fb format

[Eric Anholt]

Ville Syrjala <ville.syrj...@linux.intel.com> writes:

> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
> To make life easier for drivers, let's have the core check that the
> requested pixel format is supported by at least one plane when creating
> a new framebuffer.
>
> This eases the burden on drivers by making sure they'll never get
> requests to create fbs with unsupported pixel formats. Thanks to the
> new .fb_modifier() hook this check can now be done whether the request
> specifies the modifier directly or driver has to deduce it from the
> gem bo tiling (or via any other method really).
>
> v0: Accept anyformat if the driver doesn't do planes (Eric)
> s/planes_have_format/any_plane_has_format/ (Eric)
> Check the modifier as well since we already have a function
> that does both
> v3: Don't do the check in the core since we may not know the
> modifier yet, instead export the function and let drivers
> call it themselves
> v4: Unexport the functiona and put the format_default check back
>     since this will again be called by the core, ie. undo v3 ;)
>
> Cc: Eric Anholt <e...@anholt.net>
> Testcase: igt/kms_addfb_basic/expected-formats
> Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 30 ++
>  1 file changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_framebuffer.c 
> b/drivers/gpu/drm/drm_framebuffer.c
> index 21d3d51eb261..e618a6b728d4 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -152,6 +152,26 @@ static int fb_plane_height(int height,
>   return DIV_ROUND_UP(height, format->vsub);
>  }
>  
> +static bool any_plane_has_format(struct drm_device *dev,
> +  u32 format, u64 modifier)
> +{
> + struct drm_plane *plane;
> +
> + drm_for_each_plane(plane, dev) {
> + /*
> +  * In case the driver doesn't really do
> +  * planes we have to accept any format here.
> +  */
> + if (plane->format_default)
> + return true;
> +
> + if (drm_plane_check_pixel_format(plane, format, modifier) == 0)
> + return true;
> + }
> +
> + return false;
> +}

This drm_plane_check_pixel_format() will always fail for VC4's SAND
modifiers or VC5's UIF modifiers, where we're using the middle 48 bits
as a bit of metadata (like how we have horizontal stride passed outside
of the modifier) and you can't list all of the possible values in an
array on the plane.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC PATCH i-g-t 2/3] tests/chamelium: Add test case for plane formats

[Eric Anholt]

Maxime Ripard  writes:

> KMS can support a lot of different plane formats that are not being tested
> by the current chamelium tests.
>
> Add some preliminary tests to exert the RGB formats exposed by the KMS
> planes.

I'm really excited for this test.  A few comments...

> ---
>  tests/Makefile.am |   1 +
>  tests/Makefile.sources|   5 +
>  tests/kms_chamelium_formats.c | 305 
> ++
>  tests/meson.build |   1 +
>  4 files changed, 312 insertions(+)
>  create mode 100644 tests/kms_chamelium_formats.c
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 8472a6bf0a73..becc23de895b 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -17,6 +17,7 @@ endif
>  if HAVE_CHAMELIUM
>  TESTS_progs += \
>   kms_chamelium \
> + kms_chamelium_formats \
>   $(NULL)
>  endif
>  
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index c27226fc96c9..8476b63a245c 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -280,6 +280,11 @@ kms_chamelium_SOURCES = \
>   helpers_chamelium.h \
>   helpers_chamelium.c
>  
> +kms_chamelium_formats_SOURCES = \
> + kms_chamelium_formats.c \
> + helpers_chamelium.h \
> + helpers_chamelium.c
> +
>  testdisplay_SOURCES = \
>   testdisplay.c \
>   testdisplay.h \
> diff --git a/tests/kms_chamelium_formats.c b/tests/kms_chamelium_formats.c
> new file mode 100644
> index ..6d61f2fa34d8
> --- /dev/null
> +++ b/tests/kms_chamelium_formats.c
> @@ -0,0 +1,305 @@
> +/*
> + * Copyright © 2016 Red Hat Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
> DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *Lyude Paul 
> + */
> +
> +#include "config.h"
> +#include "helpers_chamelium.h"
> +#include "igt.h"
> +
> +#include 
> +#include 
> +#include 
> +
> +struct formats {
> + uint32_tdrm_fmt;
> + pixman_format_code_tpixman_fmt;
> +} formats_map[] = {
> + { DRM_FORMAT_XRGB, PIXMAN_x8r8g8b8 },
> + { DRM_FORMAT_ARGB, PIXMAN_a8r8g8b8 },
> + { DRM_FORMAT_ABGR, PIXMAN_a8b8g8r8 },
> + { DRM_FORMAT_RGB565, PIXMAN_r5g6b5 },
> + { DRM_FORMAT_BGR565, PIXMAN_b5g6r5 },
> + { DRM_FORMAT_ARGB1555, PIXMAN_a1r5g5b5 },
> + { DRM_FORMAT_XRGB1555, PIXMAN_x1r5g5b5 },
> +};
> +
> +static pixman_image_t *paint_ar24_pattern(size_t width, size_t height)
> +{
> + uint32_t colors[] = { 0xff00,
> +   0x,
> +   0xff00ff00,
> +   0xffff,
> +   0x };
> + unsigned i, j;
> + uint32_t *data;
> +
> + data = malloc(width * height * sizeof(*data));
> + igt_assert(data);
> +
> + for (i = 0; i < height; i++)
> + for (j = 0; j < width; j++)
> + *(data + i * width + j) = colors[((j / 64) + (i / 64)) 
> % 5];
> +
> + return pixman_image_create_bits(PIXMAN_a8r8g8b8, width, height,
> + data, width * 4);
> +}
> +
> +static void free_pattern(pixman_image_t *pattern)
> +{
> + void *data = pixman_image_get_data(pattern);
> +
> + pixman_image_unref(pattern);
> + free(data);
> +}
> +
> +static pixman_image_t *pattern_to_fb(pixman_image_t *pattern, struct igt_fb 
> *fb,
> +  pixman_format_code_t pixman_fmt)
> +{
> + pixman_image_t *converted;
> + void *ptr;
> +
> + igt_assert(fb->is_dumb);
> +
> + ptr = kmstest_dumb_map_buffer(fb->fd, fb->gem_handle, fb->size,
> +   PROT_READ | PROT_WRITE);
> + igt_assert(ptr);
> +
> + converted = pixman_image_create_bits(pixman_fmt, fb->width, fb->height,
> +  ptr, fb->stride);
> + 

Re: [Intel-gfx] [PATCH 4/4] drm/vc4: Validate framebuffer pixel format/modifier

[Eric Anholt]

Ville Syrjala  writes:

> From: Ville Syrjälä 
>
> Only create framebuffers with supported format/modifier combinations by
> checking that at least one plane supports the requested combination.
>
> Using drm_any_plane_has_format() is somewhat suboptimal for vc4 since
> the planes have (mostly) uniform capabilities. But I was lazy and
> didn't feel like exporting drm_plane_format_check() and hand rolling
> anything better. Also I really just wanted to come up with another
> user for drm_any_plane_has_format() ;)

I'm not excited about vc4 having error-return behavior that other
drivers don't have.  Actually, I don't really see why we should be doing
this check in fb create at all, given that you have to do so again at
atomic_check time with the specific plane.

Could you just delete the i915 fb format check code?


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm: Make sure at least one plane supports the fb format

[Eric Anholt]

Ville Syrjälä <ville.syrj...@linux.intel.com> writes:

> On Mon, Mar 05, 2018 at 12:59:00PM -0800, Eric Anholt wrote:
>> Ville Syrjala <ville.syrj...@linux.intel.com> writes:
>> 
>> > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>> >
>> > To make life easier for drivers, let's have the core check that the
>> > requested pixel format is supported by at least one plane when creating
>> > a new framebuffer.
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/drm_framebuffer.c | 26 ++
>> >  1 file changed, 26 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_framebuffer.c 
>> > b/drivers/gpu/drm/drm_framebuffer.c
>> > index c0530a1af5e3..155b21e579c4 100644
>> > --- a/drivers/gpu/drm/drm_framebuffer.c
>> > +++ b/drivers/gpu/drm/drm_framebuffer.c
>> > @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
>> >return DIV_ROUND_UP(height, format->vsub);
>> >  }
>> >  
>> > +static bool planes_have_format(struct drm_device *dev, u32 format)
>> > +{
>> > +  struct drm_plane *plane;
>> > +
>> > +  /* TODO: maybe maintain a device level format list? */
>> > +  drm_for_each_plane(plane, dev) {
>> > +  int i;
>> > +
>> > +  for (i = 0; i < plane->format_count; i++) {
>> > +  if (plane->format_types[i] == format)
>> > +  return true;
>> > +  }
>> > +  }
>> > +
>> > +  return false;
>> > +}
>> > +
>> >  static int framebuffer_check(struct drm_device *dev,
>> > const struct drm_mode_fb_cmd2 *r)
>> >  {
>> > @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
>> >return -EINVAL;
>> >}
>> >  
>> > +  if (!planes_have_format(dev, r->pixel_format)) {
>> > +  struct drm_format_name_buf format_name;
>> > +
>> > +  DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
>> > +drm_get_format_name(r->pixel_format,
>> > +_name));
>> > +  return -EINVAL;
>> > +  }
>> > +
>> 
>> Won't this break KMS on things like the radeon driver, which doesn't do
>> planes?  Maybe check if any universal planes have been registered and
>> only do the check in that case?
>
> Hmm. I thought we add the implicit planes always. Apparently
> drm_crtc_init() adds a primary with X/ARGB, but no more. So
> this would break all other formats, which is probably a bit too
> aggressive.
>
> I guess I could just skip the check in case any plane has
> plane->format_default set. That should be indicating that the driver
> doesn't do planes fully. Oh, why exactly is amggpu setting that flag?
> Harry?
>
>> 
>> Also, "any_planes_have_format()" might be slightly more descriptive.
>
> Or any_plane_has_format()? Is that more englishy? :)

I like it.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm: Make sure at least one plane supports the fb format

[Eric Anholt]

Ville Syrjala  writes:

> From: Ville Syrjälä 
>
> To make life easier for drivers, let's have the core check that the
> requested pixel format is supported by at least one plane when creating
> a new framebuffer.
>
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/drm_framebuffer.c | 26 ++
>  1 file changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_framebuffer.c 
> b/drivers/gpu/drm/drm_framebuffer.c
> index c0530a1af5e3..155b21e579c4 100644
> --- a/drivers/gpu/drm/drm_framebuffer.c
> +++ b/drivers/gpu/drm/drm_framebuffer.c
> @@ -152,6 +152,23 @@ static int fb_plane_height(int height,
>   return DIV_ROUND_UP(height, format->vsub);
>  }
>  
> +static bool planes_have_format(struct drm_device *dev, u32 format)
> +{
> + struct drm_plane *plane;
> +
> + /* TODO: maybe maintain a device level format list? */
> + drm_for_each_plane(plane, dev) {
> + int i;
> +
> + for (i = 0; i < plane->format_count; i++) {
> + if (plane->format_types[i] == format)
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
>  static int framebuffer_check(struct drm_device *dev,
>const struct drm_mode_fb_cmd2 *r)
>  {
> @@ -168,6 +185,15 @@ static int framebuffer_check(struct drm_device *dev,
>   return -EINVAL;
>   }
>  
> + if (!planes_have_format(dev, r->pixel_format)) {
> + struct drm_format_name_buf format_name;
> +
> + DRM_DEBUG_KMS("unsupported framebuffer format %s\n",
> +   drm_get_format_name(r->pixel_format,
> +   _name));
> + return -EINVAL;
> + }
> +

Won't this break KMS on things like the radeon driver, which doesn't do
planes?  Maybe check if any universal planes have been registered and
only do the check in that case?

Also, "any_planes_have_format()" might be slightly more descriptive.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 2/3] meson: gtkdoc support

[Eric Anholt]

Daniel Vetter  writes:

> Bunch of neat improvements:
>
> - xml generates correctly depend upon the test binaries
> - no need to re-run autogen.sh when new chapters/functions get added,
>   all handed by meson

I just rebased on top of this commit, and now my cross build takes 5
minutes as my skylake build host slowly churns through executing the ARM
binaries under QEMU.

Is there anything we could do to avoid executing the built binaries
during the build process?


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 2/2] automake: include drm-uapi headers in EXTRA_DIST

[Eric Anholt]

Petri Latvala  writes:

> Series is reviewed and tested and pushed, thanks.

Thanks, sorry for the fallout!


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t v2 3/3] igt: Add VC4 purgeable BO tests

[Eric Anholt]

Boris Brezillon  writes:

> Signed-off-by: Boris Brezillon 
> ---
> Changes in v2:
> - Add a rule to build vc4 purgeable tests when using Meson (suggested by
>   Petri)
> - Rework the subtests to avoid uncertainty (suggested by Chris)
> - Add a subtest to make sure new BOs have their content retained by
>   default (suggested by Chris)
> - Make sure we are in a known state before starting a subtest even if
>   the previous subtest failed (suggested by Chris)
> - Reworked the tests to adjust to the new MADV behavior (WILLNEED no
>   longer re-allocated the BO if it's been purged)
> - Make the purgeable test dependent on the MADVISE feature (checked
>   with the GET_PARAM ioctl)

I've pushed a rebased version of your series as of
f9b0d7ac84b6579c5248357ab45b0a65b27c6e54 up to the "purgeable" branch of
my tree.  Something I noticed as I was reviewing, though...

> ---
>  tests/Makefile.sources   |   1 +
>  tests/meson.build|   1 +
>  tests/vc4_purgeable_bo.c | 265 
> +++
>  3 files changed, 267 insertions(+)
>  create mode 100644 tests/vc4_purgeable_bo.c
>
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 0adc28a014d2..c78ac9d27921 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -8,6 +8,7 @@ VC4_TESTS = \
>   vc4_create_bo \
>   vc4_dmabuf_poll \
>   vc4_lookup_fail \
> + vc4_purgeable_bo \
>   vc4_wait_bo \
>   vc4_wait_seqno \
>   $(NULL)
> diff --git a/tests/meson.build b/tests/meson.build
> index 1c619179fe6a..8f94e3c4385b 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -243,6 +243,7 @@ if libdrm_vc4.found()
>   'vc4_create_bo',
>   'vc4_dmabuf_poll',
>   'vc4_lookup_fail',
> + 'vc4_purgeable_bo',
>   'vc4_wait_bo',
>   'vc4_wait_seqno',
>   ]
> diff --git a/tests/vc4_purgeable_bo.c b/tests/vc4_purgeable_bo.c
> new file mode 100644
> index ..b25581cc532e
> --- /dev/null
> +++ b/tests/vc4_purgeable_bo.c
> @@ -0,0 +1,265 @@
> +/*
> + * Copyright © 2017 Broadcom
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "igt.h"
> +#include "igt_vc4.h"
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "vc4_drm.h"
> +
> +struct igt_vc4_bo {
> + struct igt_list node;
> + int handle;
> + void *map;
> + size_t size;
> +};
> +
> +static jmp_buf jmp;
> +
> +static void __attribute__((noreturn)) sigtrap(int sig)
> +{
> + longjmp(jmp, sig);
> +}
> +
> +static void igt_vc4_alloc_mmap_max_bo(int fd, struct igt_list *list,
> +   size_t size)
> +{
> + struct igt_vc4_bo *bo;
> + struct drm_vc4_create_bo create = {
> + .size = size,
> + };
> +
> + while (true) {
> + if (igt_ioctl(fd, DRM_IOCTL_VC4_CREATE_BO, ))
> + break;
> +
> + bo = malloc(sizeof(*bo));
> + igt_assert(bo);
> + bo->handle = create.handle;
> + bo->size = create.size;
> + bo->map = igt_vc4_mmap_bo(fd, bo->handle, bo->size,
> +   PROT_READ | PROT_WRITE);
> + igt_list_add_tail(>node, list);
> + }
> +}
> +
> +static void igt_vc4_unmap_free_bo_pool(int fd, struct igt_list *list)
> +{
> + struct igt_vc4_bo *bo;
> +
> + while (!igt_list_empty(list)) {
> + bo = igt_list_first_entry(list, bo, node);
> + igt_assert(bo);
> + igt_list_del(>node);
> + munmap(bo->map, bo->size);
> + gem_close(fd, bo->handle);
> + free(bo);
> + }
> +}
> +
> +static void igt_vc4_trigger_purge(int 

Re: [Intel-gfx] [PATCH i-g-t 0/5] Import drm UAPI headers.

[Eric Anholt]

Petri Latvala  writes:

> On Sat, Nov 11, 2017 at 12:27:15AM +, Lionel Landwerlin wrote:
>> Hey Eric,
>> 
>> Like it did for Mesa I think this makes developers' lives easier.
>> Not having to update libdrm and then compile against the right version just
>> for the kernel headers you need is a win (in my opinion).
>
> Yes indeed. Would be nice to be able to build IGT on Debian stable
> again.
>
> Series is
>
> Acked-by: Petri Latvala 
>
> Some more acks (or objections, if any) for the overall idea would be
> nice to have, after that we can merge this and start cleaning up the
> LOCAL_* crud.

Is there anyone else who is likely to give feedback on this?


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 2/5] tests: Convert to using the imported drm-uapi headers.

[Eric Anholt]

Lionel Landwerlin <lionel.g.landwer...@intel.com> writes:

> On 10/11/17 21:26, Eric Anholt wrote:
>> Tested by dropping garbage in my libdrm's headers and rebuilding.
>>
>> Signed-off-by: Eric Anholt <e...@anholt.net>
>> ---
>>   lib/Makefile.am   | 5 -
>>   meson.build   | 2 +-
>>   tests/Makefile.am | 1 +
>>   3 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/Makefile.am b/lib/Makefile.am
>> index 30ddb92bd0bc..7b3d87780db9 100644
>> --- a/lib/Makefile.am
>> +++ b/lib/Makefile.am
>> @@ -46,7 +46,10 @@ lib_source_list +=\
>>  $(NULL)
>>   endif
>>   
>> -AM_CPPFLAGS = -I$(top_srcdir)
>> +AM_CPPFLAGS = \
>> +-I$(top_srcdir)/include/drm-uapi \
>> +-I$(top_srcdir)
>> +
>>   AM_CFLAGS = \
>>  $(CWARNFLAGS) \
>>  $(DRM_CFLAGS) \
>> diff --git a/meson.build b/meson.build
>> index fb81c4dbbd55..b14617a5f5d0 100644
>> --- a/meson.build
>> +++ b/meson.build
>> @@ -26,7 +26,7 @@ foreach cc_arg : cc_args
>> endif
>>   endforeach
>>   
>> -inc = include_directories('lib', '.')
>> +inc = include_directories('include/drm-uapi', 'lib', '.')
>>   
>>   config = configuration_data()
>>   
>> diff --git a/tests/Makefile.am b/tests/Makefile.am
>> index 89a970153992..ba5acefa68b4 100644
>> --- a/tests/Makefile.am
>> +++ b/tests/Makefile.am
>> @@ -85,6 +85,7 @@ EXTRA_DIST = \
>>   CLEANFILES = $(EXTRA_PROGRAMS) test-list.txt test-list-full.txt .gitignore
>>   
>>   AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) -Wno-unused-result $(DEBUG_CFLAGS)\
>> +-I$(top_srcdir)/include/drm-uapi \
>
> That seems to put include/drm-uapi after $(DRM_CFLAGS) which probably 
> means you're picking up the system headers first.
> I guess you want to have it the other way?

Oh, that had accidentally ended up in patch 5.  Thanks!


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t 2/5] tests: Convert to using the imported drm-uapi headers.

[Eric Anholt]

Tested by dropping garbage in my libdrm's headers and rebuilding.

Signed-off-by: Eric Anholt <e...@anholt.net>
---
 lib/Makefile.am   | 5 -
 meson.build   | 2 +-
 tests/Makefile.am | 1 +
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/Makefile.am b/lib/Makefile.am
index 30ddb92bd0bc..7b3d87780db9 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -46,7 +46,10 @@ lib_source_list +=   \
$(NULL)
 endif
 
-AM_CPPFLAGS = -I$(top_srcdir)
+AM_CPPFLAGS = \
+   -I$(top_srcdir)/include/drm-uapi \
+   -I$(top_srcdir)
+
 AM_CFLAGS = \
$(CWARNFLAGS) \
$(DRM_CFLAGS) \
diff --git a/meson.build b/meson.build
index fb81c4dbbd55..b14617a5f5d0 100644
--- a/meson.build
+++ b/meson.build
@@ -26,7 +26,7 @@ foreach cc_arg : cc_args
   endif
 endforeach
 
-inc = include_directories('lib', '.')
+inc = include_directories('include/drm-uapi', 'lib', '.')
 
 config = configuration_data()
 
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 89a970153992..ba5acefa68b4 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -85,6 +85,7 @@ EXTRA_DIST = \
 CLEANFILES = $(EXTRA_PROGRAMS) test-list.txt test-list-full.txt .gitignore
 
 AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) -Wno-unused-result $(DEBUG_CFLAGS)\
+   -I$(top_srcdir)/include/drm-uapi \
-I$(srcdir)/.. \
-I$(srcdir)/../lib \
-include "$(srcdir)/../lib/check-ndebug.h" \
-- 
2.15.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t 5/5] lib: Use drm-uapi/i915_drm.h instead of local defines.

[Eric Anholt]

The MMAP_V2 is replaced by just using MMAP, since the official header
has the updated struct.  The gem_create_v2 and gem_get_aperture are
left as is, because they seem to not be reflected in the UABI header!

Signed-off-by: Eric Anholt <e...@anholt.net>
---
 benchmarks/Makefile.am |  6 -
 benchmarks/gem_wsim.c  |  6 ++---
 lib/i915/gem_context.c | 46 --
 lib/i915/gem_context.h | 19 +++---
 lib/igt_gt.c   | 38 ++--
 lib/ioctl_wrappers.c   | 67 ++
 tests/Makefile.am  |  3 ++-
 tests/gem_ctx_param.c  | 38 ++--
 8 files changed, 92 insertions(+), 131 deletions(-)

diff --git a/benchmarks/Makefile.am b/benchmarks/Makefile.am
index d066112a32a2..9f2671be192b 100644
--- a/benchmarks/Makefile.am
+++ b/benchmarks/Makefile.am
@@ -8,7 +8,11 @@ if HAVE_LIBDRM_INTEL
 benchmarks_PROGRAMS += $(LIBDRM_INTEL_BENCHMARKS)
 endif
 
-AM_CPPFLAGS = -I$(top_srcdir) -I$(top_srcdir)/lib
+AM_CPPFLAGS = \
+   -I$(top_srcdir) \
+   -I$(top_srcdir)/include/drm-uapi \
+   -I$(top_srcdir)/lib
+
 AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) $(CAIRO_CFLAGS) $(LIBUNWIND_CFLAGS) \
$(WERROR_CFLAGS) -D_GNU_SOURCE
 LDADD = $(top_builddir)/lib/libintel_tools.la
diff --git a/benchmarks/gem_wsim.c b/benchmarks/gem_wsim.c
index 82fe6ba9ec5f..063f3ca37f00 100644
--- a/benchmarks/gem_wsim.c
+++ b/benchmarks/gem_wsim.c
@@ -893,9 +893,9 @@ prepare_workload(unsigned int id, struct workload *wrk, 
unsigned int flags)
}
 
if (wrk->prio) {
-   struct local_i915_gem_context_param param = {
-   .context = arg.ctx_id,
-   .param = 0x6,
+   struct drm_i915_gem_context_param param = {
+   .ctx_id = arg.ctx_id,
+   .param = I915_CONTEXT_PARAM_PRIORITY,
.value = wrk->prio,
};
gem_context_set_param(fd, );
diff --git a/lib/i915/gem_context.c b/lib/i915/gem_context.c
index 6d9edf5e3263..8f383a9726ec 100644
--- a/lib/i915/gem_context.c
+++ b/lib/i915/gem_context.c
@@ -100,11 +100,9 @@ void gem_context_destroy(int fd, uint32_t ctx_id)
do_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_DESTROY, );
 }
 
-int __gem_context_get_param(int fd, struct local_i915_gem_context_param *p)
+int __gem_context_get_param(int fd, struct drm_i915_gem_context_param *p)
 {
-#define LOCAL_I915_GEM_CONTEXT_GETPARAM   0x34
-#define LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM DRM_IOWR (DRM_COMMAND_BASE + 
LOCAL_I915_GEM_CONTEXT_GETPARAM, struct local_i915_gem_context_param)
-   if (igt_ioctl(fd, LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM, p))
+   if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM, p))
return -errno;
 
errno = 0;
@@ -119,17 +117,15 @@ int __gem_context_get_param(int fd, struct 
local_i915_gem_context_param *p)
  * This wraps the CONTEXT_GET_PARAM ioctl, which is used to get a context
  * parameter.
  */
-void gem_context_get_param(int fd, struct local_i915_gem_context_param *p)
+void gem_context_get_param(int fd, struct drm_i915_gem_context_param *p)
 {
igt_assert(__gem_context_get_param(fd, p) == 0);
 }
 
 
-int __gem_context_set_param(int fd, struct local_i915_gem_context_param *p)
+int __gem_context_set_param(int fd, struct drm_i915_gem_context_param *p)
 {
-#define LOCAL_I915_GEM_CONTEXT_SETPARAM   0x35
-#define LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM DRM_IOWR (DRM_COMMAND_BASE + 
LOCAL_I915_GEM_CONTEXT_SETPARAM, struct local_i915_gem_context_param)
-   if (igt_ioctl(fd, LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM, p))
+   if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, p))
return -errno;
 
errno = 0;
@@ -143,7 +139,7 @@ int __gem_context_set_param(int fd, struct 
local_i915_gem_context_param *p)
  * This wraps the CONTEXT_SET_PARAM ioctl, which is used to set a context
  * parameter.
  */
-void gem_context_set_param(int fd, struct local_i915_gem_context_param *p)
+void gem_context_set_param(int fd, struct drm_i915_gem_context_param *p)
 {
igt_assert(__gem_context_set_param(fd, p) == 0);
 }
@@ -158,14 +154,14 @@ void gem_context_set_param(int fd, struct 
local_i915_gem_context_param *p)
  */
 void gem_context_require_param(int fd, uint64_t param)
 {
-   struct local_i915_gem_context_param p;
+   struct drm_i915_gem_context_param p;
 
-   p.context = 0;
+   p.ctx_id = 0;
p.param = param;
p.value = 0;
p.size = 0;
 
-   igt_require(igt_ioctl(fd, LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM, ) == 
0);
+   igt_require(igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM, ) == 
0);
 }
 
 void gem_context_require_bannable(int fd)
@@ -17

[Intel-gfx] [PATCH i-g-t 4/5] lib: Use the imported uapi's addfb2 defines.

[Eric Anholt]

Signed-off-by: Eric Anholt <e...@anholt.net>
---
 lib/ioctl_wrappers.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 7ad2b7b007c4..d98e7660a96f 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -1663,7 +1663,7 @@ void igt_require_fb_modifiers(int fd)
uint64_t cap_modifiers;
int ret;
 
-   ret = drmGetCap(fd, LOCAL_DRM_CAP_ADDFB2_MODIFIERS, 
_modifiers);
+   ret = drmGetCap(fd, DRM_CAP_ADDFB2_MODIFIERS, _modifiers);
igt_assert(ret == 0 || errno == EINVAL);
has_modifiers = ret == 0 && cap_modifiers == 1;
cap_modifiers_tested = true;
@@ -1691,7 +1691,7 @@ int __kms_addfb(int fd, uint32_t handle, uint32_t width, 
uint32_t height,
f.pitches[0] = stride;
f.modifier[0] = modifier;
 
-   ret = igt_ioctl(fd, LOCAL_DRM_IOCTL_MODE_ADDFB2, );
+   ret = igt_ioctl(fd, DRM_IOCTL_MODE_ADDFB2, );
 
*buf_id = f.fb_id;
 
-- 
2.15.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t 3/5] tests: Remove libdrm_vc4 dependency.

[Eric Anholt]

The autotools build retains the configure.ac option, while meson folds
vc4 into the default build since we don't have any meson_options.txt
to control parts of the build.

Signed-off-by: Eric Anholt <e...@anholt.net>
---
 configure.ac  | 12 
 lib/Makefile.am   |  2 +-
 lib/meson.build   |  6 ++
 meson.build   |  1 -
 tests/Makefile.am | 12 +---
 tests/meson.build | 16 +---
 6 files changed, 13 insertions(+), 36 deletions(-)

diff --git a/configure.ac b/configure.ac
index 53ef704e4a0e..adb5998142ef 100644
--- a/configure.ac
+++ b/configure.ac
@@ -288,16 +288,12 @@ fi
 AM_CONDITIONAL(HAVE_LIBDRM_NOUVEAU, [test "x$NOUVEAU" = xyes])
 
 AC_ARG_ENABLE(vc4, AS_HELP_STRING([--disable-vc4],
- [Enable building of vc4 tests (default: auto)]),
- [VC4=$enableval], [VC4=auto])
-if test "x$VC4" = xauto; then
-   PKG_CHECK_EXISTS([libdrm_vc4], [VC4=yes], [VC4=no])
-fi
+ [Enable building of vc4 tests (default: yes)]),
+ [VC4=$enableval], [VC4=yes])
 if test "x$VC4" = xyes; then
-   PKG_CHECK_MODULES(DRM_VC4, [libdrm_vc4])
-   AC_DEFINE(HAVE_LIBDRM_VC4, 1, [Have vc4 support])
+   AC_DEFINE(BUILD_VC4, 1, [Have vc4 support])
 fi
-AM_CONDITIONAL(HAVE_LIBDRM_VC4, [test "x$VC4" = xyes])
+AM_CONDITIONAL(BUILD_VC4, [test "x$VC4" = xyes])
 
 # Define a configure option for the shader debugger
 AC_ARG_ENABLE(shader-debugger, AS_HELP_STRING([--enable-shader-debugger],
diff --git a/lib/Makefile.am b/lib/Makefile.am
index 7b3d87780db9..9c511dc0a8f9 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -10,7 +10,7 @@ libintel_tools_la_SOURCES = $(lib_source_list)
 noinst_LTLIBRARIES = libintel_tools.la
 noinst_HEADERS = check-ndebug.h
 
-if HAVE_LIBDRM_VC4
+if BUILD_VC4
 libintel_tools_la_SOURCES +=   \
 igt_vc4.c  \
 igt_vc4.h
diff --git a/lib/meson.build b/lib/meson.build
index ddf93ec6e350..253548dca9ef 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -47,6 +47,7 @@ lib_headers = [
'igt_dummyload.h',
'uwildmat/uwildmat.h',
'igt_kmod.h',
+   'igt_vc4.h',
 ]
 
 lib_sources = [
@@ -95,6 +96,7 @@ lib_sources = [
'igt_dummyload.c',
'uwildmat/uwildmat.c',
'igt_kmod.c',
+   'igt_vc4.c',
 ]
 
 lib_deps = [
@@ -118,10 +120,6 @@ else
lib_sources += 'stubs/drm/intel_bufmgr.c'
 endif
 
-if libdrm_vc4.found()
-   lib_headers += 'igt_vc4.h'
-   lib_sources += 'igt_vc4.c'
-endif
 if valgrind.found()
lib_deps += valgrind
 endif
diff --git a/meson.build b/meson.build
index b14617a5f5d0..2361866b32d2 100644
--- a/meson.build
+++ b/meson.build
@@ -32,7 +32,6 @@ config = configuration_data()
 
 libdrm = dependency('libdrm', version : '>=2.4.82')
 libdrm_intel = dependency('libdrm_intel', required : false)
-libdrm_vc4 = dependency('libdrm_vc4', required : false)
 libdrm_nouveau = dependency('libdrm_nouveau', required : false)
 libdrm_amdgpu = dependency('libdrm_amdgpu', required : false)
 
diff --git a/tests/Makefile.am b/tests/Makefile.am
index ba5acefa68b4..27acfb358582 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -10,7 +10,7 @@ if HAVE_LIBDRM_NOUVEAU
 TESTS_progs += $(NOUVEAU_TESTS)
 endif
 
-if HAVE_LIBDRM_VC4
+if BUILD_VC4
 TESTS_progs += $(VC4_TESTS)
 endif
 
@@ -143,16 +143,6 @@ prime_nv_api_CFLAGS = $(AM_CFLAGS) $(DRM_NOUVEAU_CFLAGS)
 prime_nv_api_LDADD = $(LDADD) $(DRM_NOUVEAU_LIBS)
 prime_nv_pcopy_CFLAGS = $(AM_CFLAGS) $(DRM_NOUVEAU_CFLAGS)
 prime_nv_pcopy_LDADD = $(LDADD) $(DRM_NOUVEAU_LIBS)
-vc4_create_bo_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
-vc4_create_bo_LDADD = $(LDADD) $(DRM_VC4_LIBS)
-vc4_lookup_fail_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
-vc4_lookup_fail_LDADD = $(LDADD) $(DRM_VC4_LIBS)
-vc4_dmabuf_poll_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
-vc4_dmabuf_poll_LDADD = $(LDADD) $(DRM_VC4_LIBS)
-vc4_wait_bo_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
-vc4_wait_bo_LDADD = $(LDADD) $(DRM_VC4_LIBS)
-vc4_wait_seqno_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
-vc4_wait_seqno_LDADD = $(LDADD) $(DRM_VC4_LIBS)
 
 chamelium_CFLAGS = $(AM_CFLAGS) $(XMLRPC_CFLAGS) $(LIBUDEV_CFLAGS)
 chamelium_LDADD = $(LDADD) $(XMLRPC_LIBS) $(LIBUDEV_LIBS)
diff --git a/tests/meson.build b/tests/meson.build
index c3d5372f78ac..3b523593af00 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -213,6 +213,11 @@ test_progs = [
'syncobj_wait',
'template',
'tools_test',
+   'vc4_create_bo',
+   'vc4_dmabuf_poll',
+   'vc4_lookup_fail',
+   'vc4_wait_bo',
+   'vc4_wait_seqno',
'vgem_basic',
'vgem_slow',
 ]
@@ -238,17 +243,6 @@ if libdrm_nouveau.found()
test_deps += libdrm_nouveau
 endif
 
-if libdrm_vc4.found()
-   test_progs += [
-   'vc4_create_bo',
-   'vc4_dmabuf_poll',
-   'vc4_lookup_fail',
-   'vc4_wait_bo',
-   'vc4_wa

[Intel-gfx] [PATCH i-g-t 0/5] Import drm UAPI headers.

[Eric Anholt]

This series imports the UAPI headers from Linux, like we've been doing
in Mesa for vc4 and i965.  The advantage is that it lets you build new
kernel UAPI and testcases together, without needing libdrm releases,
and has a simple rule for producing header updates (avoiding the need
for the LOCAL_* define hacks that have been proliferating in the tree)

I started on converting i915 to not use LOCAL_*, but it's more than
I'm willing to complete.

Eric Anholt (5):
  headers: Import drm-next uapi headers.
  tests: Convert to using the imported drm-uapi headers.
  tests: Remove libdrm_vc4 dependency.
  lib: Use the imported uapi's addfb2 defines.
  lib: Use drm-uapi/i915_drm.h instead of local defines.

 README |8 +
 benchmarks/Makefile.am |6 +-
 benchmarks/gem_wsim.c  |6 +-
 configure.ac   |   12 +-
 include/drm-uapi/amdgpu_drm.h  |  930 
 include/drm-uapi/armada_drm.h  |   55 ++
 include/drm-uapi/drm.h |  988 +
 include/drm-uapi/drm_fourcc.h  |  410 +++
 include/drm-uapi/drm_mode.h|  855 ++
 include/drm-uapi/drm_sarea.h   |   92 +++
 include/drm-uapi/etnaviv_drm.h |  282 
 include/drm-uapi/exynos_drm.h  |  373 ++
 include/drm-uapi/i810_drm.h|  291 
 include/drm-uapi/i915_drm.h| 1542 
 include/drm-uapi/mga_drm.h |  427 +++
 include/drm-uapi/msm_drm.h |  306 
 include/drm-uapi/nouveau_drm.h |  152 
 include/drm-uapi/omap_drm.h|  125 
 include/drm-uapi/qxl_drm.h |  158 
 include/drm-uapi/r128_drm.h|  336 +
 include/drm-uapi/radeon_drm.h  | 1078 
 include/drm-uapi/savage_drm.h  |  220 ++
 include/drm-uapi/sis_drm.h |   77 ++
 include/drm-uapi/tegra_drm.h   |  209 ++
 include/drm-uapi/vc4_drm.h |  359 ++
 include/drm-uapi/vgem_drm.h|   62 ++
 include/drm-uapi/via_drm.h |  282 
 include/drm-uapi/virtgpu_drm.h |  174 +
 include/drm-uapi/vmwgfx_drm.h  | 1128 +
 lib/Makefile.am|7 +-
 lib/i915/gem_context.c |   46 +-
 lib/i915/gem_context.h |   19 +-
 lib/igt_gt.c   |   38 +-
 lib/ioctl_wrappers.c   |   71 +-
 lib/meson.build|6 +-
 meson.build|3 +-
 tests/Makefile.am  |   16 +-
 tests/gem_ctx_param.c  |   38 +-
 tests/meson.build  |   16 +-
 39 files changed, 11032 insertions(+), 171 deletions(-)
 create mode 100644 include/drm-uapi/amdgpu_drm.h
 create mode 100644 include/drm-uapi/armada_drm.h
 create mode 100644 include/drm-uapi/drm.h
 create mode 100644 include/drm-uapi/drm_fourcc.h
 create mode 100644 include/drm-uapi/drm_mode.h
 create mode 100644 include/drm-uapi/drm_sarea.h
 create mode 100644 include/drm-uapi/etnaviv_drm.h
 create mode 100644 include/drm-uapi/exynos_drm.h
 create mode 100644 include/drm-uapi/i810_drm.h
 create mode 100644 include/drm-uapi/i915_drm.h
 create mode 100644 include/drm-uapi/mga_drm.h
 create mode 100644 include/drm-uapi/msm_drm.h
 create mode 100644 include/drm-uapi/nouveau_drm.h
 create mode 100644 include/drm-uapi/omap_drm.h
 create mode 100644 include/drm-uapi/qxl_drm.h
 create mode 100644 include/drm-uapi/r128_drm.h
 create mode 100644 include/drm-uapi/radeon_drm.h
 create mode 100644 include/drm-uapi/savage_drm.h
 create mode 100644 include/drm-uapi/sis_drm.h
 create mode 100644 include/drm-uapi/tegra_drm.h
 create mode 100644 include/drm-uapi/vc4_drm.h
 create mode 100644 include/drm-uapi/vgem_drm.h
 create mode 100644 include/drm-uapi/via_drm.h
 create mode 100644 include/drm-uapi/virtgpu_drm.h
 create mode 100644 include/drm-uapi/vmwgfx_drm.h

-- 
2.15.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/edid: Don't send non-zero YQ in AVI infoframe for HDMI 1.x sinks

[Eric Anholt]

Daniel Vetter <dan...@ffwll.ch> writes:

> On Wed, Nov 08, 2017 at 02:21:08PM -0800, Eric Anholt wrote:
>> Ville Syrjälä <ville.syrj...@linux.intel.com> writes:
>> 
>> > On Wed, Nov 08, 2017 at 12:17:28PM -0800, Eric Anholt wrote:
>> >> Ville Syrjala <ville.syrj...@linux.intel.com> writes:
>> >> 
>> >> > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>> >> >
>> >> > Apparently some sinks look at the YQ bits even when receiving RGB,
>> >> > and they get somehow confused when they see a non-zero YQ value.
>> >> > So we can't just blindly follow CEA-861-F and set YQ to match the
>> >> > RGB range.
>> >> >
>> >> > Unfortunately there is no good way to tell whether the sink
>> >> > designer claims to have read CEA-861-F. The CEA extension block
>> >> > revision number has generally been stuck at 3 since forever,
>> >> > and even a very recently manufactured sink might be based on
>> >> > an old design so the manufacturing date doesn't seem like
>> >> > something we can use. In lieu of better information let's
>> >> > follow CEA-861-F only for HDMI 2.0 sinks, since HDMI 2.0 is
>> >> > based on CEA-861-F. For HDMI 1.x sinks we'll always set YQ=0.
>> >> >
>> >> > The alternative would of course be to always set YQ=0. And if
>> >> > we ever encounter a HDMI 2.0+ sink with this bug that's what
>> >> > we'll probably have to do.
>> >> 
>> >> Should vc4 be doing anything special for HDMI2 sinks, if it's an HDMI1.4
>> >> source?
>> >
>> > As long as you stick to < 340 MHz modes you shouldn't have to do
>> > anything. For >=340 MHz you'd need to use some new HDMI 2.0 features.
>> >
>> > Looks like vc4 crtc .mode_valid() doesn't do much. I presume it's up
>> > to bridges/encoders to filter out most things that aren't supported?
>> 
>> I had a patch for that at
>> https://patchwork.freedesktop.org/series/30680/ -- fedora folks had run
>> into trouble with 4k monitors.
>
> Ack on the clock limiting patch, silly that it's stuck. No idea about CEC,
> better for Hans/Boris I guess.

Thanks!


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/edid: Don't send non-zero YQ in AVI infoframe for HDMI 1.x sinks

[Eric Anholt]

Ville Syrjälä <ville.syrj...@linux.intel.com> writes:

> On Wed, Nov 08, 2017 at 12:17:28PM -0800, Eric Anholt wrote:
>> Ville Syrjala <ville.syrj...@linux.intel.com> writes:
>> 
>> > From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>> >
>> > Apparently some sinks look at the YQ bits even when receiving RGB,
>> > and they get somehow confused when they see a non-zero YQ value.
>> > So we can't just blindly follow CEA-861-F and set YQ to match the
>> > RGB range.
>> >
>> > Unfortunately there is no good way to tell whether the sink
>> > designer claims to have read CEA-861-F. The CEA extension block
>> > revision number has generally been stuck at 3 since forever,
>> > and even a very recently manufactured sink might be based on
>> > an old design so the manufacturing date doesn't seem like
>> > something we can use. In lieu of better information let's
>> > follow CEA-861-F only for HDMI 2.0 sinks, since HDMI 2.0 is
>> > based on CEA-861-F. For HDMI 1.x sinks we'll always set YQ=0.
>> >
>> > The alternative would of course be to always set YQ=0. And if
>> > we ever encounter a HDMI 2.0+ sink with this bug that's what
>> > we'll probably have to do.
>> 
>> Should vc4 be doing anything special for HDMI2 sinks, if it's an HDMI1.4
>> source?
>
> As long as you stick to < 340 MHz modes you shouldn't have to do
> anything. For >=340 MHz you'd need to use some new HDMI 2.0 features.
>
> Looks like vc4 crtc .mode_valid() doesn't do much. I presume it's up
> to bridges/encoders to filter out most things that aren't supported?

I had a patch for that at
https://patchwork.freedesktop.org/series/30680/ -- fedora folks had run
into trouble with 4k monitors.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/edid: Don't send non-zero YQ in AVI infoframe for HDMI 1.x sinks

[Eric Anholt]

Ville Syrjala <ville.syrj...@linux.intel.com> writes:

> From: Ville Syrjälä <ville.syrj...@linux.intel.com>
>
> Apparently some sinks look at the YQ bits even when receiving RGB,
> and they get somehow confused when they see a non-zero YQ value.
> So we can't just blindly follow CEA-861-F and set YQ to match the
> RGB range.
>
> Unfortunately there is no good way to tell whether the sink
> designer claims to have read CEA-861-F. The CEA extension block
> revision number has generally been stuck at 3 since forever,
> and even a very recently manufactured sink might be based on
> an old design so the manufacturing date doesn't seem like
> something we can use. In lieu of better information let's
> follow CEA-861-F only for HDMI 2.0 sinks, since HDMI 2.0 is
> based on CEA-861-F. For HDMI 1.x sinks we'll always set YQ=0.
>
> The alternative would of course be to always set YQ=0. And if
> we ever encounter a HDMI 2.0+ sink with this bug that's what
> we'll probably have to do.

Should vc4 be doing anything special for HDMI2 sinks, if it's an HDMI1.4
source?

That said, as far as vc4, this patch is

Acked-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [maintainer-tools PATCH] dim: Sign commits in addition to tags

[Eric Anholt]

Sean Paul  writes:

> On Wed, Nov 1, 2017 at 7:12 AM, Gustavo Padovan  wrote:
>> 2017-10-31 Sean Paul :
>>
>>> On Tue, Oct 31, 2017 at 1:31 PM, Daniel Vetter  wrote:
>>> > On Tue, Oct 31, 2017 at 5:14 PM, Sean Paul  wrote:
>>> >> On Tue, Oct 31, 2017 at 4:27 AM, Jani Nikula
>>> >>  wrote:
>>> >>>
>>> >>> Reminder, we have this new list dim-to...@lists.freedesktop.org for
>>> >>> maintainer tools patches. Cc'd.
>>> >>>
>>> >>
>>> >> Ahh, cool. I didn't realize dim grew up!
>>> >>
>>> >>> On Mon, 30 Oct 2017, Sean Paul  wrote:
>>>  Expanding on Jani's work to sign tags, this patch adds signing for git
>>>  commit/am.
>>> >>>
>>> >>> I guess I'd like more rationale here. Is this something we should be
>>> >>> doing? Is anyone else doing this?
>>> >>>
>>> >>
>>> >> Sure thing. Signing commits allows Dave to use --verify-signatures
>>> >> when pulling. If something is not signed, we'll know it was either not
>>> >> applied with dim, or was altered on fdo (both warrant investigation).
>>> >>
>>> >> I suspect no one else is doing this since most trees are single
>>> >> maintainer, and it's not possible to sign commits via git send-email.
>>> >> Since we have the committer model, and a bunch of people with access
>>> >> to fdo and the tree, I think it's important to add this. Especially
>>> >> since we can do it in dim without overhead.
>>> >>
>>>  Signed-off-by: Sean Paul 
>>>  ---
>>> 
>>>  This has been lightly tested with dim apply-branch/dim push-branch.
>>> 
>>>  Sean
>>> 
>>>   dim | 78 
>>>  +
>>>   1 file changed, 51 insertions(+), 27 deletions(-)
>>> 
>>>  diff --git a/dim b/dim
>>>  index 527989aff9ad..cd5e41f89a3a 100755
>>>  --- a/dim
>>>  +++ b/dim
>>>  @@ -67,9 +67,6 @@ 
>>>  DIM_TEMPLATE_SIGNATURE=${DIM_TEMPLATE_SIGNATURE:-$HOME/.dim.template.signature}
>>>   # dim pull-request tag summary template
>>>   
>>>  DIM_TEMPLATE_TAG_SUMMARY=${DIM_TEMPLATE_TAG_SUMMARY:-$HOME/.dim.template.tagsummary}
>>> 
>>>  -# GPG key id for signing tags. If unset, don't sign.
>>>  -DIM_GPG_KEYID=${DIM_GPG_KEYID:+-u $DIM_GPG_KEYID}
>>>  -
>>>   #
>>>   # Internal configuration.
>>>   #
>>>  @@ -104,6 +101,20 @@ test_request_recipients=(
>>>   # integration configuration
>>>   integration_config=nightly.conf
>>> 
>>>  +# GPG key id for signing tags. If unset, don't sign.
>>>  +function gpg_keyid_for_tag
>>>  +{
>>>  + echo "${DIM_GPG_KEYID:+-u $DIM_GPG_KEYID}"
>>>  + return 0
>>>  +}
>>>  +
>>>  +# GPG key id for committing (git commit/am). If unset, don't sign.
>>>  +function gpg_keyid_for_commit
>>>  +{
>>>  + echo "${DIM_GPG_KEYID:+-S$DIM_GPG_KEYID}"
>>>  + return 0
>>>  +}
>>> >>>
>>> >>> This seems like an overly complicated way to achieve what you want.
>>> >>>
>>> >>> Just put these under "Internal configuration." instead:
>>> >>>
>>> >>> dim_gpg_sign_tag=${DIM_GPG_KEYID:+-u $DIM_GPG_KEYID}
>>> >>> dim_gpg_sign_commit=${DIM_GPG_KEYID:+-S$DIM_GPG_KEYID}
>>> >>>
>>> >>> And use directly in git tag and commit, respectively?
>>> >>>
>>> >>
>>> >> Yep, sounds good.
>>> >>
>>> >>> Although... perhaps starting to sign tags should not force signing
>>> >>> commits?
>>> >>>
>>> >>
>>> >> Why would it be desirable to *not* sign tags?
>>> >
>>> > Again, what's the threat model you're trying to defend against? Atm
>>> > anyone with commit rights to fd.o can push whatever they want to. If
>>> > they want to be evil, they can also push whatever kind of garbage they
>>> > want to, including commit signature and and fake Link: and review
>>> > tags. With pull requests/tags signing them prevents a
>>> > man-in-the-midddle attack of the unprotected pull request in the mail,
>>> > but I still don't see what signing commits protects against.
>>>
>>> This is protecting against a bad actor (either through a committer's
>>> account, or some other fdo account) gaining access to the tree on fdo
>>> and either adding a malicious commit, or altering an existing commit.
>>
>> Yeah, but then we need to enforce it for all committer
>
> My hope is that dim makes it easy enough to get everyone on board
> eventually. In the interim, the people with signing commits will be
> able to attest that those commits were applied by them.
>
>> and we also need
>> a signing party to sign each others keys.
>
> I feel like most of us see each other often enough to make this
> possible. Even without a signing party, we still get *some* amount of
> coverage by virtue of TOFU [1].
>
> Is anyone against the idea of signing commits? Is there a reason that
> we shouldn't?

We've used GPG a bunch in fdo infrastructure, and 

Re: [Intel-gfx] [PATCH 6/7] drm/drivers: drop redundant drm_edid_to_eld() calls

[Eric Anholt]

Jani Nikula <jani.nik...@intel.com> writes:

> drm_add_edid_modes() now fills in the ELD automatically, so the calls to
> drm_edid_to_eld() are redundant. Remove them.
>
> All the other places are obvious, but nv50 has detached
> drm_edid_to_eld() from the drm_add_edid_modes() call.

Nice!  For vc4,

Acked-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t v2 3/3] igt: Add VC4 purgeable BO tests

[Eric Anholt]

Boris Brezillon  writes:

> Signed-off-by: Boris Brezillon 

I got a build error without including .  Rebased branch with
that changed is "purgeable" in my tree.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t 1/3] Fix rlim_cur compiler warnings when building on ARM.

[Eric Anholt]

Signed-off-by: Eric Anholt <e...@anholt.net>
---
 benchmarks/prime_lookup.c | 2 +-
 tests/gem_exec_reuse.c| 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/benchmarks/prime_lookup.c b/benchmarks/prime_lookup.c
index e995b766a173..d6c397299fcb 100644
--- a/benchmarks/prime_lookup.c
+++ b/benchmarks/prime_lookup.c
@@ -143,7 +143,7 @@ static bool allow_files(unsigned min)
return false;
 
igt_info("Current file limit is %ld, estimated we need %d\n",
-rlim.rlim_cur, min);
+(long)rlim.rlim_cur, min);
 
if (rlim.rlim_cur > min)
return true;
diff --git a/tests/gem_exec_reuse.c b/tests/gem_exec_reuse.c
index cdfa9783f5b7..4e3907cf7b52 100644
--- a/tests/gem_exec_reuse.c
+++ b/tests/gem_exec_reuse.c
@@ -122,7 +122,8 @@ static uint64_t max_open_files(void)
if (getrlimit(RLIMIT_NOFILE, ))
rlim.rlim_cur = 64 << 10;
 
-   igt_info("Process limit for file descriptors is %lu\n", rlim.rlim_cur);
+   igt_info("Process limit for file descriptors is %lu\n",
+(long)rlim.rlim_cur);
return rlim.rlim_cur;
 }
 
-- 
2.14.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t 2/3] meson: Fix build of igt_x86-using tests on non-x86 platforms.

[Eric Anholt]

Just stub out the features return value, and return an empty string.

Signed-off-by: Eric Anholt <e...@anholt.net>
---
 lib/igt_x86.h   | 12 
 lib/meson.build |  5 -
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/lib/igt_x86.h b/lib/igt_x86.h
index 589d224bec62..d6dcfa108331 100644
--- a/lib/igt_x86.h
+++ b/lib/igt_x86.h
@@ -40,7 +40,19 @@
 #define AVX0x80
 #define AVX2   0x100
 
+#if defined(__x86_64__)
 unsigned igt_x86_features(void);
 char *igt_x86_features_to_string(unsigned features, char *line);
+#else
+static inline unsigned igt_x86_features(void)
+{
+   return 0;
+}
+static inline char *igt_x86_features_to_string(unsigned features, char *line)
+{
+   line[0] = 0;
+   return line;
+}
+#endif
 
 #endif /* IGT_X86_H */
diff --git a/lib/meson.build b/lib/meson.build
index 203be520fd3f..df1dc465e310 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -54,7 +54,6 @@ lib_sources = [
'igt_rand.c',
'igt_stats.c',
'igt_sysfs.c',
-   'igt_x86.c',
'igt_vgem.c',
'instdone.c',
'intel_batchbuffer.c',
@@ -88,6 +87,10 @@ lib_sources = [
'igt_kmod.c',
 ]
 
+if ['x86', 'x86_64'].contains(host_machine.cpu_family())
+lib_sources += 'igt_x86.c'
+endif
+
 lib_deps = [
cairo,
glib,
-- 
2.14.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t 3/3] meson: Disable the intel overlay on non-x86 builds.

[Eric Anholt]

It's got calls to rmb/wmb that end up not linking successfully.

Signed-off-by: Eric Anholt <e...@anholt.net>
---
 meson.build | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 1cc501f3062f..7a09228292fd 100644
--- a/meson.build
+++ b/meson.build
@@ -121,6 +121,8 @@ subdir('benchmarks')
 subdir('tools')
 if libdrm_intel.found()
subdir('assembler')
-   subdir('overlay')
+   if ['x86', 'x86_64'].contains(host_machine.cpu_family())
+   subdir('overlay')
+   endif
 endif
 subdir('man')
-- 
2.14.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 2/2] igt: Add VC4 purgeable BO tests

[Eric Anholt]

Boris Brezillon  writes:

> Signed-off-by: Boris Brezillon 

Another test we should have: Queue up a big rendering job (Copy a
2048x2048@32bpp BO?), mark the source purgeable, force the purge, wait
for rendering, make sure we correctly rendered, and maybe have some
sanity-checking of purgeable state of the BO.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 2/2] igt: Add VC4 purgeable BO tests

[Eric Anholt]

Boris Brezillon  writes:

> On Wed, 27 Sep 2017 13:50:30 +0100
> Chris Wilson  wrote:
>
>> Quoting Boris Brezillon (2017-09-27 13:41:41)
>> > Hi Chris,
>> > 
>> > On Wed, 27 Sep 2017 13:07:28 +0100
>> > Chris Wilson  wrote:
>> >   
>> > > Quoting Boris Brezillon (2017-09-27 12:51:18)  
>> > > > +static void igt_vc4_trigger_purge(int fd)
>> > > > +{
>> > > 
>> > > May I suggest a /proc/sys/vm/drop_caches-esque interface?
>> > > For when you want to explicitly control reclaim.  
>> > 
>> > Eric suggested to add a debugfs entry to control the purge, I just
>> > thought I didn't really need it since I had a way to trigger this
>> > mechanism without adding yet another userspace -> kernel interface that
>> > will become part of the ABI and will have to be maintained forever.
>> > 
>> > If you think this is preferable, I'll go for the debugfs hook.  
>> 
>> I think you will find it useful in future. i915's drop-caches also has
>> options to make sure the GPU is idle, delayed frees are flushed, etc.
>> One thing we found useful is that through a debugfs interface, we can
>> pretend to be the shrinker/in-reclaim, setting
>> fs_reclaim_acquire(GFP_KERNEL) around the operation. That gives us
>> better lockdep coverage without having to trigger the shrinker.
>> 
>> Our experience says that you will make good use of a drop-caches
>> interface, it won't just be a one test wonder. :)
>
> Just had a look at i915_drop_caches_fops [1] and it seems
> over-complicated given what I can do in the VC4 driver: flush memory of
> BOs that are marked purgeable.
>
> Right now there's no shrinker object registered to the MM layer to help
> the system release memory. The only one who can trigger a purge is the
> VC4 BO allocator when it fails to allocate CMA memory.
> Also note that all VC4 BOs are backed by CMA mem, so I'm not sure
> plugging the BO purge system to the MM shrinker logic makes a lot of
> sense (is the MM core expecting shrinkers to release memory coming from
> the CMA pool?)

Given that general page cache stuff can live in CMA, freeing CMA memory
from your shrinker callback should be good for MM.  So, yeah, it would
be great if (in a later patchset) the mesa BO cache gets purged when the
system is busy doing non-graphics tasks and wants the memory back.

Also, I just landed the userspace side of BO labeling, so
/debug/dri/0/bo_stats will have a lot more useful information in it.  We
should probably have the mark-purgeable path in the kernel label the BO
as purgeable (erasing whatever previous label the BO had).  Then, maybe
after we can make it so that most allocations label their BOs, not just
debug Mesa builds.  Need to do some performance testing there.

> All this to say I'm not comfortable with designing a generic
> "drop_caches" debugfs hook that would take various options to delimit
> the scope of the cache-flush request. I'd prefer to have a simple
> "purge_purgeable_bos" file that does not care about the input value and
> flushes everything as soon as someone writes to it.
> But let's wait for Eric's feedback, maybe he has other plans and a
> better vision of what will be needed after this simple "purgeable-bo"
> implementation I'm about to post.

I thought your use of allocations to force purging was pretty elegant.
Also, it means that we'll be driving the purging code from the same
codepath as Mesa will be (BO allocation) rather than something slightly
different.

I think we'll want debugfs to test the shrinker path, since I don't know
of another good way for userspace to trigger it reliably without also
destabilizing the testing environment.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t 05/12] tests/gem_spin_batch: Fix warning

[Eric Anholt]

Daniel Vetter <daniel.vet...@ffwll.ch> writes:

> Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch>

1-5 are:

Reviewed-by: Eric Anholt <e...@anholt.net>

My meson branch has some stuff you probably want to squash into your
meson build system commit.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t 1/3] Use PATH_MAX to fix some sprintf-into-short-buffers warnings.

[Eric Anholt]

Signed-off-by: Eric Anholt <e...@anholt.net>
---

This little series cleans up many compiler warnings I saw when testing
danvet's meson branch.

 lib/igt_debugfs.c   | 4 ++--
 lib/igt_sysfs.c | 4 ++--
 tests/kms_hdmi_inject.c | 2 +-
 tests/pm_rpm.c  | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
index 090b56e03555..63183e57229b 100644
--- a/lib/igt_debugfs.c
+++ b/lib/igt_debugfs.c
@@ -491,7 +491,7 @@ static void igt_pipe_crc_reset(int drm_fd)
}
 
while ((dirent = readdir(dir))) {
-   char buf[128];
+   char buf[PATH_MAX];
 
if (strcmp(dirent->d_name, "crtc-") != 0)
continue;
@@ -525,7 +525,7 @@ static void igt_pipe_crc_reset(int drm_fd)
 static void pipe_crc_exit_handler(int sig)
 {
struct dirent *dirent;
-   char buf[128];
+   char buf[PATH_MAX];
DIR *dir;
int fd;
 
diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
index 15ed34be0088..9227e374bf44 100644
--- a/lib/igt_sysfs.c
+++ b/lib/igt_sysfs.c
@@ -192,7 +192,7 @@ int igt_sysfs_open_parameters(int device)
if (params < 0) { /* builtin? */
drm_version_t version;
char name[32] = "";
-   char path[128];
+   char path[PATH_MAX];
 
memset(, 0, sizeof(version));
version.name_len = sizeof(name);
@@ -499,7 +499,7 @@ void kick_fbcon(bool enable)
return;
 
while ((de = readdir(dir))) {
-   char buf[128];
+   char buf[PATH_MAX];
int fd, len;
 
if (strncmp(de->d_name, "vtcon", 5))
diff --git a/tests/kms_hdmi_inject.c b/tests/kms_hdmi_inject.c
index cb916acec3c1..22570a4b637a 100644
--- a/tests/kms_hdmi_inject.c
+++ b/tests/kms_hdmi_inject.c
@@ -170,7 +170,7 @@ eld_is_valid(void)
continue;
 
while ((snd_hda = readdir(dir))) {
-   char fpath[128];
+   char fpath[PATH_MAX];
 
if (*snd_hda->d_name == '.' ||
strstr(snd_hda->d_name, "eld") == 0)
diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 47c9f1143484..9e8cf79b5128 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -594,14 +594,14 @@ static int count_i2c_valid_edids(void)
DIR *dir;
 
struct dirent *dirent;
-   char full_name[32];
+   char full_name[PATH_MAX];
 
dir = opendir("/dev/");
igt_assert(dir);
 
while ((dirent = readdir(dir))) {
if (strncmp(dirent->d_name, "i2c-", 4) == 0) {
-   snprintf(full_name, 32, "/dev/%s", dirent->d_name);
+   sprintf(full_name, "/dev/%s", dirent->d_name);
fd = open(full_name, O_RDWR);
igt_assert_neq(fd, -1);
if (i2c_edid_is_valid(fd))
-- 
2.14.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t 3/3] intel_display_poller: Fix truncation of a test name.

[Eric Anholt]

Signed-off-by: Eric Anholt <e...@anholt.net>
---
 tools/intel_display_poller.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/intel_display_poller.c b/tools/intel_display_poller.c
index c501c79d6367..828ca52b35ee 100644
--- a/tools/intel_display_poller.c
+++ b/tools/intel_display_poller.c
@@ -901,7 +901,7 @@ static void poll_dsl_field(int pipe, uint32_t *min, 
uint32_t *max, const int cou
 
 static const char *test_name(enum test test, int pipe, int bit, bool 
test_pixel_count)
 {
-   static char str[32];
+   static char str[64];
const char *type = test_pixel_count ? "pixel" : "dsl";
 
switch (test) {
-- 
2.14.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t 2/3] intel_watermark: Fix a warning about "const char" return being silly.

[Eric Anholt]

Signed-off-by: Eric Anholt <e...@anholt.net>
---
 tools/intel_watermark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/intel_watermark.c b/tools/intel_watermark.c
index d98ef19b0abd..d8c784802c5b 100644
--- a/tools/intel_watermark.c
+++ b/tools/intel_watermark.c
@@ -110,7 +110,7 @@ struct ilk_wm {
lo) >> (shift_lo)) & MASK(size_lo)) | \
 hi) >> (shift_hi)) & MASK(size_hi)) << (size_lo)))
 
-static const char pipe_name(int pipe)
+static char pipe_name(int pipe)
 {
return 'A' + pipe;
 }
-- 
2.14.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] CONTRIBUTING: formalize review rules

[Eric Anholt]

Daniel Vetter <daniel.vet...@ffwll.ch> writes:

> There's a bunch of reasons why I think we should formalize and enforce
> our review rules for igt patches:
>
> - We have a lot of new engineers joining and review helps enormously
>   with mentoring and learning. But right now only patches from
>   contributors without commit rights are consistently subjected to
>   review, which makes this imbalanced and removes senior contributors
>   from the review pool.
>
> - We have a much bigger team and we need to make sure we're aligned on
>   where igt as a tool and testsuite needs to head towards. Getting
>   that alignment happens through reviewing each other's submission.
>   Pushing a contentious patch and then dealing with a heated irc
>   discussion is much less effective.
>
> - Finally igt becomes ever more important for our testing, making sure
>   the code quality is high is important. Review helps with that.
>
> v2: Improve wording a bit (Imre).
>
> Acked-by: Daniel Stone <dani...@collabora.com>
> Acked-by: Jani Nikula <jani.nik...@intel.com>
> Acked-by: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> Acked-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> Acked-by: Petri Latvala <petri.latv...@intel.com>
> Acked-by: Imre Deak <imre.d...@intel.com>
> Acked-by: Robert Foss <robert.f...@collabora.com>
> Acked-by: Ben Widawsky <b...@bwidawsk.net>
> Acked-by: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> Acked-by: Mika Kuoppala <mika.kuopp...@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
> ---
>  CONTRIBUTING | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/CONTRIBUTING b/CONTRIBUTING
> index d2adcf03b7c3..561c5dd80bba 100644
> --- a/CONTRIBUTING
> +++ b/CONTRIBUTING
> @@ -26,10 +26,11 @@ A short list of contribution guidelines:
>convenience macros provided by the igt library. The semantic patch 
> lib/igt.cocci
>can help with the more automatic conversions.
>  
> -- There is no formal review requirement and regular contributors with commit
> -  access can push patches right after submitting them to the mailing lists. 
> But
> -  invasive changes, new helper libraries and contributions from newcomers 
> should
> -  go through a proper review to ensure overall consistency in the codebase.
> +- Patches need to be reviewed on the mailing list. Exceptions only apply for
> +  testcases and tooling for drivers with just a single contributor (e.g. 
> vc4).
> +  In this case patches must still be submitted to the mailing list first.
> +  Testcase should preferrably be cross-reviewed by the same people who write 
> and
> +  review the kernel feature itself.

Thanks for considering my case here :)

Acked-by: Eric Anholt <e...@anholt.net>


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t] vc4: Test setting labels of BOs.

[Eric Anholt]

So far this test is basically making sure that we throw appropriate
errors, and don't oops the kernel with silly inputs.

Signed-off-by: Eric Anholt <e...@anholt.net>
---
 tests/Makefile.am  |  2 ++
 tests/Makefile.sources |  1 +
 tests/vc4_label_bo.c   | 95 ++
 3 files changed, 98 insertions(+)
 create mode 100644 tests/vc4_label_bo.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index a5cfcab8ed7e..eca2aeca5d0c 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -122,6 +122,8 @@ prime_nv_pcopy_CFLAGS = $(AM_CFLAGS) $(DRM_NOUVEAU_CFLAGS)
 prime_nv_pcopy_LDADD = $(LDADD) $(DRM_NOUVEAU_LIBS)
 vc4_create_bo_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
 vc4_create_bo_LDADD = $(LDADD) $(DRM_VC4_LIBS)
+vc4_label_bo_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
+vc4_label_bo_LDADD = $(LDADD) $(DRM_VC4_LIBS)
 vc4_lookup_fail_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
 vc4_lookup_fail_LDADD = $(LDADD) $(DRM_VC4_LIBS)
 vc4_dmabuf_poll_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 700914a8d8c9..592418b54c68 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -8,6 +8,7 @@ VC4_TESTS = \
vc4_create_bo \
vc4_dmabuf_poll \
vc4_lookup_fail \
+   vc4_label_bo \
vc4_tiling \
vc4_wait_bo \
vc4_wait_seqno \
diff --git a/tests/vc4_label_bo.c b/tests/vc4_label_bo.c
new file mode 100644
index ..7510f87d4676
--- /dev/null
+++ b/tests/vc4_label_bo.c
@@ -0,0 +1,95 @@
+/*
+ * Copyright © 2017 Broadcom
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "igt.h"
+#include "igt_vc4.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "vc4_drm.h"
+
+static void
+set_label(int fd, int handle, const char *name, int err)
+{
+   struct drm_vc4_label_bo label = {
+   .handle = handle,
+   .len = strlen(name),
+   .name = (uintptr_t)name,
+   };
+
+   if (err)
+   do_ioctl_err(fd, DRM_IOCTL_VC4_LABEL_BO, , err);
+   else
+   do_ioctl(fd, DRM_IOCTL_VC4_LABEL_BO, );
+}
+
+igt_main
+{
+   int fd;
+
+   igt_fixture
+   fd = drm_open_driver(DRIVER_VC4);
+
+   igt_subtest("set-label") {
+   int handle = igt_vc4_create_bo(fd, 4096);
+   set_label(fd, handle, "a test label", 0);
+   set_label(fd, handle, "a new test label", 0);
+   gem_close(fd, handle);
+   }
+
+   igt_subtest("set-bad-handle") {
+   set_label(fd, 0xd0d0d0d0, "bad handle", ENOENT);
+   }
+
+   igt_subtest("set-bad-name") {
+   int handle = igt_vc4_create_bo(fd, 4096);
+
+   struct drm_vc4_label_bo label = {
+   .handle = handle,
+   .len = 1000,
+   .name = 0,
+   };
+
+   do_ioctl_err(fd, DRM_IOCTL_VC4_LABEL_BO, , EFAULT);
+
+   gem_close(fd, handle);
+   }
+
+   igt_subtest("set-kernel-name") {
+   int handle = igt_vc4_create_bo(fd, 4096);
+   set_label(fd, handle, "BCL", 0);
+   set_label(fd, handle, "a test label", 0);
+   set_label(fd, handle, "BCL", 0);
+   gem_close(fd, handle);
+   }
+
+   igt_fixture
+   close(fd);
+}
-- 
2.11.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2 6/6 squash] drm/vc4: Fixup for the drm core async changes.

[Eric Anholt]

With this squashed in, the vc4 patch is:
Reviewed-by: Eric Anholt <e...@anholt.net>

Signed-off-by: Eric Anholt <e...@anholt.net>
---

Without this, the cursor never moved :)

 drivers/gpu/drm/vc4/vc4_plane.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index aac3a31a9260..59f7ab6b1831 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -792,6 +792,9 @@ static void vc4_plane_atomic_async_update(struct drm_plane 
*plane,
if (plane->state->fb != new_state->fb)
vc4_plane_async_set_fb(plane, new_state->fb);
 
+   /* Update the display list based on the new crtc_x/y. */
+   vc4_plane_atomic_check(plane, plane->state);
+
/* Note that we can't just call vc4_plane_write_dlist()
 * because that would smash the context data that the HVS is
 * currently using.
-- 
2.11.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t] Make autogen.sh set the default format.subjectPrefix

[Eric Anholt]

CONTRIBUTING requests that people do this, but it's a lot easier if we
just set it up by default for them.

Signed-off-by: Eric Anholt <e...@anholt.net>
---

I missed this step on my previous two patches, so let's just prevent
that in the future.  :(

 autogen.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/autogen.sh b/autogen.sh
index ec633f3efefd..53df7525a976 100755
--- a/autogen.sh
+++ b/autogen.sh
@@ -16,6 +16,9 @@ fi
 autoreconf -v --install || exit 1
 cd $ORIGDIR || exit $?
 
+git config --local --get format.subjectPrefix >/dev/null 2>&1 ||
+git config --local format.subjectPrefix "PATCH i-g-t"
+
 if test -z "$NOCONFIGURE"; then
 $srcdir/configure "$@"
 fi
-- 
2.11.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/2] igt/vc4_tiling: Test vc4's new set/get_tiling ioctls.

[Eric Anholt]

This just checks that the appropriate errors get thrown, and that the
modifier can be set/get successfully, and that the modifier doesn't
leak to other BO allocations.  Testing of scanout will be done with
the writeback support that Boris is building.  The modifier has no
effect on V3D rendering, so no need to test that.

Signed-off-by: Eric Anholt <e...@anholt.net>
---
 lib/igt_vc4.c  |  21 
 lib/igt_vc4.h  |   3 ++
 tests/Makefile.am  |   2 +
 tests/Makefile.sources |   1 +
 tests/vc4_tiling.c | 137 +
 5 files changed, 164 insertions(+)
 create mode 100644 tests/vc4_tiling.c

diff --git a/lib/igt_vc4.c b/lib/igt_vc4.c
index c4682f5688f1..5347af8df400 100644
--- a/lib/igt_vc4.c
+++ b/lib/igt_vc4.c
@@ -128,3 +128,24 @@ igt_vc4_mmap_bo(int fd, uint32_t handle, uint32_t size, 
unsigned prot)
else
return ptr;
 }
+
+void igt_vc4_set_tiling(int fd, uint32_t handle, uint64_t modifier)
+{
+   struct drm_vc4_set_tiling set = {
+   .handle = handle,
+   .modifier = modifier,
+   };
+
+   do_ioctl(fd, DRM_IOCTL_VC4_SET_TILING, );
+}
+
+uint64_t igt_vc4_get_tiling(int fd, uint32_t handle)
+{
+   struct drm_vc4_get_tiling get = {
+   .handle = handle,
+   };
+
+   do_ioctl(fd, DRM_IOCTL_VC4_GET_TILING, );
+
+   return get.modifier;
+}
diff --git a/lib/igt_vc4.h b/lib/igt_vc4.h
index e9252461c951..d1740b93f1fc 100644
--- a/lib/igt_vc4.h
+++ b/lib/igt_vc4.h
@@ -28,4 +28,7 @@ uint32_t igt_vc4_get_cleared_bo(int fd, size_t size, uint32_t 
clearval);
 int igt_vc4_create_bo(int fd, size_t size);
 void *igt_vc4_mmap_bo(int fd, uint32_t handle, uint32_t size, unsigned prot);
 
+void igt_vc4_set_tiling(int fd, uint32_t handle, uint64_t modifier);
+uint64_t igt_vc4_get_tiling(int fd, uint32_t handle);
+
 #endif /* IGT_VC4_H */
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 12cc6dc48c12..0de12e204080 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -126,6 +126,8 @@ vc4_lookup_fail_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
 vc4_lookup_fail_LDADD = $(LDADD) $(DRM_VC4_LIBS)
 vc4_dmabuf_poll_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
 vc4_dmabuf_poll_LDADD = $(LDADD) $(DRM_VC4_LIBS)
+vc4_tiling_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
+vc4_tiling_LDADD = $(LDADD) $(DRM_VC4_LIBS)
 vc4_wait_bo_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
 vc4_wait_bo_LDADD = $(LDADD) $(DRM_VC4_LIBS)
 vc4_wait_seqno_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index b3a680e9d75e..6d3c539b4041 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -8,6 +8,7 @@ VC4_TESTS_M = \
vc4_create_bo \
vc4_dmabuf_poll \
vc4_lookup_fail \
+   vc4_tiling \
vc4_wait_bo \
vc4_wait_seqno \
$(NULL)
diff --git a/tests/vc4_tiling.c b/tests/vc4_tiling.c
new file mode 100644
index ..00c7c6b84d60
--- /dev/null
+++ b/tests/vc4_tiling.c
@@ -0,0 +1,137 @@
+/*
+ * Copyright © 2017 Broadcom
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "igt.h"
+#include "igt_vc4.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "vc4_drm.h"
+
+igt_main
+{
+   int fd;
+
+   igt_fixture
+   fd = drm_open_driver(DRIVER_VC4);
+
+   igt_subtest("get-bad-handle") {
+   struct drm_vc4_get_tiling get = {
+   .handle = 0xd0d0d0d0,
+   };
+   do_ioctl_err(fd, DRM_IOCTL_VC4_GET_TILING, , ENOENT);
+   }
+
+   igt_subtest("set-bad-handle") {
+   struct drm_vc4_set_tiling set = {
+   .handle = 0xd0d0d0d0,
+   .modifier = DRM_FORMAT_MOD_NONE,

[Intel-gfx] [PATCH 1/2] Add an editorconfig file for the basics of igt style.

[Eric Anholt]

This makes my emacs default to consistent indentation for the project.

Signed-off-by: Eric Anholt <e...@anholt.net>
---
 .editorconfig | 9 +
 1 file changed, 9 insertions(+)
 create mode 100644 .editorconfig

diff --git a/.editorconfig b/.editorconfig
new file mode 100644
index ..bdfebacaf4cd
--- /dev/null
+++ b/.editorconfig
@@ -0,0 +1,9 @@
+[*]
+tab_width = 8
+
+[{Makefile*,*.mk}]
+indent_style = tab
+
+[*.{c,h,cpp,hpp,cc,hh}]
+indent_style = tab
+indent_size = 8
-- 
2.11.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH igt] igt/vc4_dmabuf_poll: Add a test for polling to wait for dmabuf fences.

[Eric Anholt]

Daniel Vetter <dan...@ffwll.ch> writes:

> On Mon, Apr 10, 2017 at 06:24:32PM -0700, Eric Anholt wrote:
>> This successfully catches vc4's lack of dmabuf fencing.
>> 
>> Signed-off-by: Eric Anholt <e...@anholt.net>
>> ---
>> 
>> Has anyone looked into shared infrastructure for tests to do
>> KMS/dmabuf/etc. things with a generic "get a BO that's being rendered
>> to for this driver" call?
>
> We have some helpers for i915 to make a bo busy with an explicit release
> (we create a looping batch which we break with a cpu write), for extremely
> well-control busy tests. Not sure how well that's portable, and without
> full control it's hard to make busy tests reliable.

I think I would need a new kernel ioctl for that.

I can queue up 32MB read/write jobs, and with ~1GB/s of memory
bandwidth, that's been plenty of time to write tests that catch at least
some bugs in synchronization.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH RESEND 1/6] drm/atomic: initial support for asynchronous plane update

[Eric Anholt]

Gustavo Padovan <gust...@padovan.org> writes:

> From: Gustavo Padovan <gustavo.pado...@collabora.com>
>
> In some cases, like cursor updates, it is interesting to update the
> plane in an asynchronous fashion to avoid big delays. The current queued
> update could be still waiting for a fence to signal and thus block any
> subsequent update until its scan out. In cases like this if we update the
> cursor synchronously through the atomic API it will cause significant
> delays that would even be noticed by the final user.
>
> This patch creates a fast path to jump ahead the current queued state and
> do single planes updates without going through all atomic steps in
> drm_atomic_helper_commit(). We take this path for legacy cursor updates.
>
> For now only single plane updates are supported, but we plan to support
> multiple planes updates and async PageFlips through this interface as well
> in the near future.

Sorry for taking so long to do some review for this.

> ---
>  drivers/gpu/drm/drm_atomic.c | 65 
> 
>  drivers/gpu/drm/drm_atomic_helper.c  | 35 +
>  include/drm/drm_atomic.h |  2 +
>  include/drm/drm_atomic_helper.h  |  2 +
>  include/drm/drm_modeset_helper_vtables.h | 48 +++
>  5 files changed, 152 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index be62774..2a0c297 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -631,6 +631,68 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
>   return 0;
>  }
>  
> +static bool drm_atomic_async_check(struct drm_atomic_state *state)
> +{
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> + struct drm_crtc_commit *commit;
> + struct drm_plane *__plane, *plane = NULL;
> + struct drm_plane_state *__plane_state, *plane_state = NULL;
> + const struct drm_plane_helper_funcs *funcs;
> + int i, j, n_planes = 0;
> +
> + for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> + if (drm_atomic_crtc_needs_modeset(crtc_state))
> + return false;
> + }
> +
> + for_each_new_plane_in_state(state, __plane, __plane_state, i) {
> + n_planes++;
> + plane = __plane;
> + plane_state = __plane_state;
> + }
> +
> + /* FIXME: we support only single plane updates for now */
> + if (!plane || n_planes != 1)
> + return false;
> +
> + if (!plane_state->crtc)
> + return false;
> +
> + funcs = plane->helper_private;
> + if (!funcs->atomic_async_update)
> + return false;
> +
> + if (plane_state->fence)
> + return false;
> +

Could you add a comment here like:

/* Don't do an async update if there is an outstanding commit modifying
 * the plane.  This prevents our async update's changes from getting
 * overridden by a previous synchronous update's state.
 */

(assuming I understand its intent correctly)

I don't understand KMS locking enough to say if this loop is actually
safely guaranteeing that.  If you're convinced of that, then with my
little cleanups this patch will be:

Acked-by: Eric Anholt <e...@anholt.net>

> + for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> + if (plane->crtc != crtc)
> + continue;
> +
> + spin_lock(>commit_lock);
> + commit = list_first_entry_or_null(>commit_list,
> +   struct drm_crtc_commit,
> +   commit_entry);
> + if (!commit) {
> + spin_unlock(>commit_lock);
> + continue;
> + }
> + spin_unlock(>commit_lock);
> +
> + if (!crtc->state->state)
> + continue;
> +
> + for_each_plane_in_state(crtc->state->state, __plane,
> + __plane_state, j) {
> + if (__plane == plane)
> + return false;
> + }
> + }
> +
> + return !funcs->atomic_async_check(plane, plane_state);
> +}
> +
>  static void drm_atomic_crtc_print_state(struct drm_printer *p,
>   const struct drm_crtc_state *state)
>  {
> @@ -1591,6 +1653,9 @@ int drm_atomic_check_only(struct drm_atomic_state 
> *state)
>   }
>   }
>  
> + if (state->legacy_cursor_update)
> + state->async_update = drm_a

Re: [Intel-gfx] [PATCH RESEND 6/6] drm/vc4: update cursors asynchronously through atomic

[Eric Anholt]

Gustavo Padovan <gust...@padovan.org> writes:

> From: Gustavo Padovan <gustavo.pado...@collabora.com>
>
> Add support to async updates of cursors by using the new atomic

"Add support for"

> interface for that. Basically what this commit does is do what
> vc4_update_plane() did but through atomic.
>
> v3: move size checks back to drivers (Ville Syrjälä)
>
> v2: move fb setting to core and use new state (Eric Anholt)

Given that vc4 isn't using drm_atomic_helper_commit(), isn't this
effectively disabling async cursor updates on vc4?


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Deal with upside-down mounted LCD panels

[Eric Anholt]

Hans de Goede  writes:

> HI,
>
> On 08-05-17 14:27, Chris Wilson wrote:
>> On Sun, May 07, 2017 at 11:10:56AM +0200, Hans de Goede wrote:
>>> On some (Bay Trail) devices the LCD panel is mounted upside-down.
>>>
>>> This commit uses the code to read back the initial rotation of the
>>> primary plane in get_initial_plane_config from Ville Syrjala's
>>> "drm/fb-helper: Inherit rotation wip" patch and when re-using the
>>> initial fb it stores that in intel_crtc.initial_rotation.
>>>
>>> It adds an intel_plane_get_rotation helper which combines this
>>> initial_rotation with any rotation requested by userspace and
>>> uses this in all places which look at a planes rotation, thus
>>> transparently dealing with upside-down LCD panels without requiring
>>> any user-space or fbcon changes.
>>>
>>> This fixes the kernel boot messages switching from being shown the right
>>> way up in efifb to being shown upside down as soon as a native kms driver
>>> loads, as well as any graphics displayed by userspace being upside-down.
>>>
>>> Note this only deals with upside-down LCD panels / 180 degrees
>>> rotation as the hardware in question only supports 180 degrees
>>> rotation in hardware. The one model known which has a panel mounted
>>> with 90/270 degrees rotation will need to be fully dealt with in
>>> userspace, even the firmware boot screen / menus are rotated 90 degrees
>>> on this one and there simply is nothing the kernel can do about this.
>> 
>> I shall just mention a concern with hiding the transformation on the
>> connection, we do expose the subpixel layout to userspace and that
>> should be adjusted based on this lie. There are probably other places
>> where the orientation of the output leaks through the current interface.
>> 
>> The commit message fails to explain how userspace, which should already
>> have the tools available to it, is unable to reapply the existing
>> rotation - i.e. why this is a kernel problem,
>
> This is a kernel problem because one of the things the kernel does
> is hardware abstraction, as I mentioned in my reply to Ville, there is
> not single userspace to fix here, there is a large supply of userspace
> consumers of the kms API which need fixing to fix this in userspace.
>
> For touchscreens which are mounted with a wrong orientation the
> input layer fixes things up, I don't see how this is any different
> really. Now if it was impossible or very complicated to fix this
> in the kernel that would be a different story, but as this patch
> shows it is quite doable to fix this in the kernel.

FWIW, Raspberry Pi has a similar problem: The panel has an appropriate
orientation due to its viewing angle, but the cabling is such that many
people mount it upside down and use a firmware configuration to have
everything flipped.  It would be nice if I could do readback of the
panel's orientation reg (I haven't tested) and retain automatically that
after boot, as a default policy.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: Document code of conduct

[Eric Anholt]

Daniel Vetter <daniel.vet...@ffwll.ch> writes:

> freedesktop.org has adopted a formal code of conduct:
>
> https://www.fooishbar.org/blog/fdo-contributor-covenant/
> https://www.freedesktop.org/wiki/CodeOfConduct/
>
> Besides formalizing things a bit more I don't think this changes
> anything for us, we've already peer-enforced respectful and
> constructive interactions since a long time. But it's good to document
> things properly.
>
> Note: As Daniel Stone mentioned in the announcement fd.o admins
> started chatting with the communities their hosting, which includs the
> X.org foundation board, to figure out how to fan out enforcement and
> allow projects to run things on their own (with fd.o still as the
> fallback).  So the details of enforcement (and appealing decisions)
> might still change, but since this involves the board and lots more
> people it'll take a while to get there. For now this is good enough I
> think.
>
> For the text itself I went with the same blurb as the Wayland project,
> didn't feel creative yet this early in the morning:
>
> https://cgit.freedesktop.org/wayland/wayland/commit/?id=0eefe99fe0683ae409b665a8b18cc7eb648c6c0c

With the other wording nitpicks fixed,

Reviewed-by: Eric Anholt <e...@anholt.net>

I'm pleased to be part of a community that's working on building an
inclusive, welcoming, productive environment.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH igt] igt/vc4_dmabuf_poll: Add a test for polling to wait for dmabuf fences.

[Eric Anholt]

This successfully catches vc4's lack of dmabuf fencing.

Signed-off-by: Eric Anholt <e...@anholt.net>
---

Has anyone looked into shared infrastructure for tests to do
KMS/dmabuf/etc. things with a generic "get a BO that's being rendered
to for this driver" call?

 tests/Makefile.am   |  2 ++
 tests/Makefile.sources  |  1 +
 tests/vc4_dmabuf_poll.c | 85 +
 3 files changed, 88 insertions(+)
 create mode 100644 tests/vc4_dmabuf_poll.c

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 8930c245043d..7c40c9ea8276 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -114,6 +114,8 @@ vc4_create_bo_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
 vc4_create_bo_LDADD = $(LDADD) $(DRM_VC4_LIBS)
 vc4_lookup_fail_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
 vc4_lookup_fail_LDADD = $(LDADD) $(DRM_VC4_LIBS)
+vc4_dmabuf_poll_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
+vc4_dmabuf_poll_LDADD = $(LDADD) $(DRM_VC4_LIBS)
 vc4_wait_bo_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
 vc4_wait_bo_LDADD = $(LDADD) $(DRM_VC4_LIBS)
 vc4_wait_seqno_CFLAGS = $(AM_CFLAGS) $(DRM_VC4_CFLAGS)
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 6e07d9387f83..b579e28c6669 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -6,6 +6,7 @@ NOUVEAU_TESTS_M = \
 
 VC4_TESTS_M = \
vc4_create_bo \
+   vc4_dmabuf_poll \
vc4_lookup_fail \
vc4_wait_bo \
vc4_wait_seqno \
diff --git a/tests/vc4_dmabuf_poll.c b/tests/vc4_dmabuf_poll.c
new file mode 100644
index ..ac44d4cf80c7
--- /dev/null
+++ b/tests/vc4_dmabuf_poll.c
@@ -0,0 +1,85 @@
+/*
+ * Copyright © 2017 Broadcom
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "igt.h"
+#include "igt_vc4.h"
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "vc4_drm.h"
+
+static void
+poll_write_bo_test(int fd, int poll_flag)
+{
+   size_t size = 1024 * 1024 * 4;
+   uint32_t clearval = 0xaabbccdd;
+   /* Get a BO that's being rendered to. */
+   int handle = igt_vc4_get_cleared_bo(fd, size, clearval);
+   int dmabuf_fd = prime_handle_to_fd(fd, handle);
+   struct pollfd p = {
+   .fd = dmabuf_fd,
+   .events = POLLIN,
+   };
+   struct drm_vc4_wait_bo wait = {
+   .handle = handle,
+   .timeout_ns = 0,
+   };
+
+   /* Block for a couple of minutes waiting for rendering to complete. */
+   int poll_ret = poll(, 1, 120 * 1000);
+   igt_assert(poll_ret == 1);
+
+   /* Now that we've waited for idle, a nonblocking wait for the
+* BO should pass.
+*/
+   do_ioctl(fd, DRM_IOCTL_VC4_WAIT_BO, );
+
+   close(dmabuf_fd);
+   gem_close(fd, handle);
+}
+
+igt_main
+{
+   int fd;
+
+   igt_fixture
+   fd = drm_open_driver(DRIVER_VC4);
+
+   igt_subtest("poll-write-waits-until-write-done") {
+   poll_write_bo_test(fd, POLLOUT);
+   }
+
+   igt_subtest("poll-read-waits-until-write-done") {
+   poll_write_bo_test(fd, POLLIN);
+   }
+
+   igt_fixture
+   close(fd);
+}
-- 
2.11.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 12/15] drm: Add acquire ctx to ->gamma_set hook

[Eric Anholt]

Daniel Vetter  writes:

> Atomic helpers really want this instead of the hacked-up legacy
> backoff trick, which unfortunately prevents drivers from using their
> own private drm_modeset_locks.
>
> Aside: There's a few atomic drivers (nv50, vc4, soon vmwgfx) which
> don't yet use the new atomic color mgmt/gamma table stuff. Would be
> nice if they could switch over and just hook up
> drm_atomic_helper_legacy_gamma_set() instead.

For notes like this, it would be helpful to have a pointer to a driver
doing it cleanly the current way.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/5] drm/edid: Introduce drm_default_rgb_quant_range()

[Eric Anholt]

ville.syrj...@linux.intel.com writes:

> From: Ville Syrjälä 
>
> Make the code selecting the RGB quantization range a little less magicy
> by wrapping it up in a small helper.

This series seems good.  I won't have the ability to test it on vc4
within a reasonable time frame, so you can add an Acked-by from me if
you'd like.  Just one comment that you can take or leave:

> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/drm_edid.c| 18 ++
>  drivers/gpu/drm/i915/intel_dp.c   |  4 +++-
>  drivers/gpu/drm/i915/intel_hdmi.c |  3 ++-
>  drivers/gpu/drm/vc4/vc4_hdmi.c|  4 +++-
>  include/drm/drm_edid.h|  2 ++
>  5 files changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 4ff04aa84dd0..304c583b8000 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3768,6 +3768,24 @@ bool drm_rgb_quant_range_selectable(struct edid *edid)
>  }
>  EXPORT_SYMBOL(drm_rgb_quant_range_selectable);
>  
> +/**
> + * drm_default_rgb_quant_range - default RGB quantization range
> + * @mode: display mode
> + *
> + * Determine the default RGB quantization range for the mode,
> + * as specified in CEA-861.
> + *
> + * Return: The default RGB quantization range for the mode
> + */
> +enum hdmi_quantization_range
> +drm_default_rgb_quant_range(const struct drm_display_mode *mode)
> +{
> + return drm_match_cea_mode(mode) > 1 ?
> + HDMI_QUANTIZATION_RANGE_LIMITED :
> + HDMI_QUANTIZATION_RANGE_FULL;

It might be nice to add a comment here like

/* All CEA modes other than VIC 1 use limited quantization range. */

When I first had to put this logic in vc4, I was surprised by it and
figured that it was an off-by-one bug in code that was trying to say "if
it's any CEA mode then it should be limited range"


> +}
> +EXPORT_SYMBOL(drm_default_rgb_quant_range);


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx