Re: [PATCH] drm/panthor: Kill the faulty_slots variable in panthor_sched_suspend()

[Erik Faye-Lund]

On Thu, 2024-04-25 at 12:39 +0200, Boris Brezillon wrote:
> We can use upd_ctx.timedout_mask directly, and the faulty_slots
> update
> in the flush_caches_failed situation is never used.
> 
> Suggested-by: Suggested-by: Steven Price 

Whoops? :)

> Signed-off-by: Boris Brezillon 
> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 10 +++---
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c
> b/drivers/gpu/drm/panthor/panthor_sched.c
> index fad4678ca4c8..fed28c16d5d1 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -2584,8 +2584,8 @@ void panthor_sched_suspend(struct
> panthor_device *ptdev)
>  {
>   struct panthor_scheduler *sched = ptdev->scheduler;
>   struct panthor_csg_slots_upd_ctx upd_ctx;
> - u32 suspended_slots, faulty_slots;
>   struct panthor_group *group;
> + u32 suspended_slots;
>   u32 i;
>  
>   mutex_lock(>lock);
> @@ -2605,10 +2605,9 @@ void panthor_sched_suspend(struct
> panthor_device *ptdev)
>  
>   csgs_upd_ctx_apply_locked(ptdev, _ctx);
>   suspended_slots &= ~upd_ctx.timedout_mask;
> - faulty_slots = upd_ctx.timedout_mask;
>  
> - if (faulty_slots) {
> - u32 slot_mask = faulty_slots;
> + if (upd_ctx.timedout_mask) {
> + u32 slot_mask = upd_ctx.timedout_mask;
>  
>   drm_err(>base, "CSG suspend failed,
> escalating to termination");
>   csgs_upd_ctx_init(_ctx);
> @@ -2659,9 +2658,6 @@ void panthor_sched_suspend(struct
> panthor_device *ptdev)
>  
>   slot_mask &= ~BIT(csg_id);
>   }
> -
> - if (flush_caches_failed)
> - faulty_slots |= suspended_slots;
>   }
>  
>   for (i = 0; i < sched->csg_slot_count; i++) {

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

[Erik Faye-Lund]

On Fri, 2020-02-28 at 10:43 +, Daniel Stone wrote:
> On Fri, 28 Feb 2020 at 10:06, Erik Faye-Lund
>  wrote:
> > On Fri, 2020-02-28 at 11:40 +0200, Lionel Landwerlin wrote:
> > > Yeah, changes on vulkan drivers or backend compilers should be
> > > fairly
> > > sandboxed.
> > > 
> > > We also have tools that only work for intel stuff, that should
> > > never
> > > trigger anything on other people's HW.
> > > 
> > > Could something be worked out using the tags?
> > 
> > I think so! We have the pre-defined environment variable
> > CI_MERGE_REQUEST_LABELS, and we can do variable conditions:
> > 
> > https://docs.gitlab.com/ee/ci/yaml/#onlyvariablesexceptvariables
> > 
> > That sounds like a pretty neat middle-ground to me. I just hope
> > that
> > new pipelines are triggered if new labels are added, because not
> > everyone is allowed to set labels, and sometimes people forget...
> 
> There's also this which is somewhat more robust:
> https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2569
> 
> 

I'm not sure it's more robust, but yeah that a useful tool too.

The reason I'm skeptical about the robustness is that we'll miss
testing if this misses a path. That can of course be fixed by testing
everything once things are in master, and fixing up that list when
something breaks on master.

The person who wrote a change knows more about the intricacies of the
changes than a computer will ever do. But humans are also good at
making mistakes, so I'm not sure which one is better. Maybe the union
of both?

As long as we have both rigorous testing after something landed in
master (doesn't nessecarily need to happen right after, but for now
that's probably fine), as well as a reasonable heuristic for what
testing is needed pre-merge, I think we're good.

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Erik Faye-Lund]

On Fri, 2020-02-28 at 10:47 +0100, Daniel Vetter wrote:
> On Fri, Feb 28, 2020 at 10:29 AM Erik Faye-Lund
>  wrote:
> > On Fri, 2020-02-28 at 13:37 +1000, Dave Airlie wrote:
> > > On Fri, 28 Feb 2020 at 07:27, Daniel Vetter <
> > > daniel.vet...@ffwll.ch>
> > > wrote:
> > > > Hi all,
> > > > 
> > > > You might have read the short take in the X.org board meeting
> > > > minutes
> > > > already, here's the long version.
> > > > 
> > > > The good news: gitlab.fd.o has become very popular with our
> > > > communities, and is used extensively. This especially includes
> > > > all
> > > > the
> > > > CI integration. Modern development process and tooling, yay!
> > > > 
> > > > The bad news: The cost in growth has also been tremendous, and
> > > > it's
> > > > breaking our bank account. With reasonable estimates for
> > > > continued
> > > > growth we're expecting hosting expenses totalling 75k USD this
> > > > year,
> > > > and 90k USD next year. With the current sponsors we've set up
> > > > we
> > > > can't
> > > > sustain that. We estimate that hosting expenses for gitlab.fd.o
> > > > without any of the CI features enabled would total 30k USD,
> > > > which
> > > > is
> > > > within X.org's ability to support through various sponsorships,
> > > > mostly
> > > > through XDC.
> > > > 
> > > > Note that X.org does no longer sponsor any CI runners
> > > > themselves,
> > > > we've stopped that. The huge additional expenses are all just
> > > > in
> > > > storing and serving build artifacts and images to outside CI
> > > > runners
> > > > sponsored by various companies. A related topic is that with
> > > > the
> > > > growth in fd.o it's becoming infeasible to maintain it all on
> > > > volunteer admin time. X.org is therefore also looking for admin
> > > > sponsorship, at least medium term.
> > > > 
> > > > Assuming that we want cash flow reserves for one year of
> > > > gitlab.fd.o
> > > > (without CI support) and a trimmed XDC and assuming no sponsor
> > > > payment
> > > > meanwhile, we'd have to cut CI services somewhere between May
> > > > and
> > > > June
> > > > this year. The board is of course working on acquiring
> > > > sponsors,
> > > > but
> > > > filling a shortfall of this magnitude is neither easy nor quick
> > > > work,
> > > > and we therefore decided to give an early warning as soon as
> > > > possible.
> > > > Any help in finding sponsors for fd.o is very much appreciated.
> > > 
> > > a) Ouch.
> > > 
> > > b) we probably need to take a large step back here.
> > > 
> > 
> > I kinda agree, but maybe the step doesn't have to be *too* large?
> > 
> > I wonder if we could solve this by restructuring the project a bit.
> > I'm
> > talking purely from a Mesa point of view here, so it might not
> > solve
> > the full problem, but:
> > 
> > 1. It feels silly that we need to test changes to e.g the i965
> > driver
> > on dragonboards. We only have a big "do not run CI at all" escape-
> > hatch.
> > 
> > 2. A lot of us are working for a company that can probably pay for
> > their own needs in terms of CI. Perhaps moving some costs "up
> > front" to
> > the company that needs it can make the future of CI for those who
> > can't
> > do this
> > 
> > 3. I think we need a much more detailed break-down of the cost to
> > make
> > educated changes. For instance, how expensive is Docker image
> > uploads/downloads (e.g intermediary artifacts) compared to build
> > logs
> > and final test-results? What kind of artifacts?
> 
> We have logs somewhere, but no one yet got around to analyzing that.
> Which will be quite a bit of work to do since the cloud storage is
> totally disconnected from the gitlab front-end, making the connection
> to which project or CI job caused something is going to require
> scripting. Volunteers definitely very much welcome I think.
> 

Fair enough, but just keep in mind that the same thing as optimizing
software applies here; working blindly reduces the impact. So if we
want to fix the current situation, this is going to have to be a
p

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

[Erik Faye-Lund]

On Fri, 2020-02-28 at 13:37 +1000, Dave Airlie wrote:
> On Fri, 28 Feb 2020 at 07:27, Daniel Vetter 
> wrote:
> > Hi all,
> > 
> > You might have read the short take in the X.org board meeting
> > minutes
> > already, here's the long version.
> > 
> > The good news: gitlab.fd.o has become very popular with our
> > communities, and is used extensively. This especially includes all
> > the
> > CI integration. Modern development process and tooling, yay!
> > 
> > The bad news: The cost in growth has also been tremendous, and it's
> > breaking our bank account. With reasonable estimates for continued
> > growth we're expecting hosting expenses totalling 75k USD this
> > year,
> > and 90k USD next year. With the current sponsors we've set up we
> > can't
> > sustain that. We estimate that hosting expenses for gitlab.fd.o
> > without any of the CI features enabled would total 30k USD, which
> > is
> > within X.org's ability to support through various sponsorships,
> > mostly
> > through XDC.
> > 
> > Note that X.org does no longer sponsor any CI runners themselves,
> > we've stopped that. The huge additional expenses are all just in
> > storing and serving build artifacts and images to outside CI
> > runners
> > sponsored by various companies. A related topic is that with the
> > growth in fd.o it's becoming infeasible to maintain it all on
> > volunteer admin time. X.org is therefore also looking for admin
> > sponsorship, at least medium term.
> > 
> > Assuming that we want cash flow reserves for one year of
> > gitlab.fd.o
> > (without CI support) and a trimmed XDC and assuming no sponsor
> > payment
> > meanwhile, we'd have to cut CI services somewhere between May and
> > June
> > this year. The board is of course working on acquiring sponsors,
> > but
> > filling a shortfall of this magnitude is neither easy nor quick
> > work,
> > and we therefore decided to give an early warning as soon as
> > possible.
> > Any help in finding sponsors for fd.o is very much appreciated.
> 
> a) Ouch.
> 
> b) we probably need to take a large step back here.
> 

I kinda agree, but maybe the step doesn't have to be *too* large?

I wonder if we could solve this by restructuring the project a bit. I'm
talking purely from a Mesa point of view here, so it might not solve
the full problem, but:

1. It feels silly that we need to test changes to e.g the i965 driver
on dragonboards. We only have a big "do not run CI at all" escape-
hatch.

2. A lot of us are working for a company that can probably pay for
their own needs in terms of CI. Perhaps moving some costs "up front" to
the company that needs it can make the future of CI for those who can't
do this 

3. I think we need a much more detailed break-down of the cost to make
educated changes. For instance, how expensive is Docker image
uploads/downloads (e.g intermediary artifacts) compared to build logs
and final test-results? What kind of artifacts?

One suggestion would be to do something more similar to what the kernel
does, and separate into different repos for different subsystems. This
could allow us to have separate testing-pipelines for these repos,
which would mean that for instance a change to RADV didn't trigger a
full Panfrost test-run.

This would probably require us to accept using a more branch-heavy
work-flow. I don't personally think that would be a bad thing.

But this is all kinda based on an assumption that running hardware-
testing is the expensive part. I think that's quite possibly the case,
but some more numbers would be helpful. I mean, it might turn out that
just throwing up a Docker cache inside the organizations that host
runners might be sufficient for all I know...

We could also do stuff like reducing the amount of tests we run on each
commit, and punt some testing to a per-weekend test-run or someting
like that. We don't *need* to know about every problem up front, just
the stuff that's about to be released, really. The other stuff is just
nice to have. If it's too expensive, I would say drop it.

I would really hope that we can consider approaches like this before we
throw out the baby with the bathwater...

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Erik Faye-Lund]

On Fri, 2020-02-28 at 11:40 +0200, Lionel Landwerlin wrote:
> On 28/02/2020 11:28, Erik Faye-Lund wrote:
> > On Fri, 2020-02-28 at 13:37 +1000, Dave Airlie wrote:
> > > On Fri, 28 Feb 2020 at 07:27, Daniel Vetter <
> > > daniel.vet...@ffwll.ch>
> > > wrote:
> > > > Hi all,
> > > > 
> > > > You might have read the short take in the X.org board meeting
> > > > minutes
> > > > already, here's the long version.
> > > > 
> > > > The good news: gitlab.fd.o has become very popular with our
> > > > communities, and is used extensively. This especially includes
> > > > all
> > > > the
> > > > CI integration. Modern development process and tooling, yay!
> > > > 
> > > > The bad news: The cost in growth has also been tremendous, and
> > > > it's
> > > > breaking our bank account. With reasonable estimates for
> > > > continued
> > > > growth we're expecting hosting expenses totalling 75k USD this
> > > > year,
> > > > and 90k USD next year. With the current sponsors we've set up
> > > > we
> > > > can't
> > > > sustain that. We estimate that hosting expenses for gitlab.fd.o
> > > > without any of the CI features enabled would total 30k USD,
> > > > which
> > > > is
> > > > within X.org's ability to support through various sponsorships,
> > > > mostly
> > > > through XDC.
> > > > 
> > > > Note that X.org does no longer sponsor any CI runners
> > > > themselves,
> > > > we've stopped that. The huge additional expenses are all just
> > > > in
> > > > storing and serving build artifacts and images to outside CI
> > > > runners
> > > > sponsored by various companies. A related topic is that with
> > > > the
> > > > growth in fd.o it's becoming infeasible to maintain it all on
> > > > volunteer admin time. X.org is therefore also looking for admin
> > > > sponsorship, at least medium term.
> > > > 
> > > > Assuming that we want cash flow reserves for one year of
> > > > gitlab.fd.o
> > > > (without CI support) and a trimmed XDC and assuming no sponsor
> > > > payment
> > > > meanwhile, we'd have to cut CI services somewhere between May
> > > > and
> > > > June
> > > > this year. The board is of course working on acquiring
> > > > sponsors,
> > > > but
> > > > filling a shortfall of this magnitude is neither easy nor quick
> > > > work,
> > > > and we therefore decided to give an early warning as soon as
> > > > possible.
> > > > Any help in finding sponsors for fd.o is very much appreciated.
> > > a) Ouch.
> > > 
> > > b) we probably need to take a large step back here.
> > > 
> > I kinda agree, but maybe the step doesn't have to be *too* large?
> > 
> > I wonder if we could solve this by restructuring the project a bit.
> > I'm
> > talking purely from a Mesa point of view here, so it might not
> > solve
> > the full problem, but:
> > 
> > 1. It feels silly that we need to test changes to e.g the i965
> > driver
> > on dragonboards. We only have a big "do not run CI at all" escape-
> > hatch.
> 
> Yeah, changes on vulkan drivers or backend compilers should be
> fairly 
> sandboxed.
> 
> We also have tools that only work for intel stuff, that should never 
> trigger anything on other people's HW.
> 
> Could something be worked out using the tags?
> 

I think so! We have the pre-defined environment variable
CI_MERGE_REQUEST_LABELS, and we can do variable conditions:

https://docs.gitlab.com/ee/ci/yaml/#onlyvariablesexceptvariables

That sounds like a pretty neat middle-ground to me. I just hope that
new pipelines are triggered if new labels are added, because not
everyone is allowed to set labels, and sometimes people forget...

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 22/22] gpu: host1x: At first try a non-blocking allocation for the gather copy

[Erik Faye-Lund]

On Wed, Jun 14, 2017 at 10:32 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
> On 14.06.2017 10:56, Erik Faye-Lund wrote:
>> On Wed, Jun 14, 2017 at 1:16 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
>>> The blocking gather copy allocation is a major performance downside of the
>>> Host1x firewall, it may take hundreds milliseconds which is unacceptable
>>> for the real-time graphics operations. Let's try a non-blocking allocation
>>> first as a least invasive solution, it makes opentegra (Xorg driver)
>>> performance indistinguishable with/without the firewall.
>>>
>>> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
>>> ---
>>>
>>> The other much more invasive solution could be: to have a memory pool
>>> reserved for the gather copy and performing the firewall checks (and the
>>> gather copying) on the actual job submission to the CDMA, not on the job
>>> allocation like it is done now. This solution reduces the overhead of a
>>> gather copy allocations.
>>>
>>>  drivers/gpu/host1x/job.c | 16 
>>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
>>> index fc194c676d91..ed0575f14093 100644
>>> --- a/drivers/gpu/host1x/job.c
>>> +++ b/drivers/gpu/host1x/job.c
>>> @@ -594,12 +594,20 @@ static inline int copy_gathers(struct host1x_job 
>>> *job, struct device *dev)
>>> size += g->words * sizeof(u32);
>>> }
>>>
>>> +   /*
>>> +* Try a non-blocking allocation from a higher priority pools first,
>>> +* as awaiting for the allocation here is a major performance hit.
>>> +*/
>>> job->gather_copy_mapped = dma_alloc_wc(dev, size, >gather_copy,
>>> -  GFP_KERNEL);
>>> -   if (!job->gather_copy_mapped) {
>>> -   job->gather_copy_mapped = NULL;
>>> +  GFP_NOWAIT);
>>> +
>>> +   /* the higher priority allocation failed, try the generic-blocking 
>>> */
>>> +   if (!job->gather_copy_mapped)
>>> +   job->gather_copy_mapped = dma_alloc_wc(dev, size,
>>> +  >gather_copy,
>>> +  GFP_KERNEL);
>>> +   if (!job->gather_copy_mapped)
>>> return -ENOMEM;
>>> -   }
>>>
>>> job->gather_copy_size = size;
>>
>> Can't we just have a static fixed-size buffer, and validate chunks at
>> the time? Or why can't we validate directly on the mapped pointers?
>>
>> It feels pretty silly to have to allocate memory in the first place here...
>>
>
> We can't validate the mapped pointers because userspace may write to the 
> buffers
> memory at any time. Maybe it should be possible to write-protect cmdbuffers
> memory for the time of its submission and execution, but I'm not sure whether
> it's worth the effort. The current-proposed solution is efficient and least
> invasive.

Right, good point.

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 22/22] gpu: host1x: At first try a non-blocking allocation for the gather copy

[Erik Faye-Lund]

On Wed, Jun 14, 2017 at 1:16 AM, Dmitry Osipenko  wrote:
> The blocking gather copy allocation is a major performance downside of the
> Host1x firewall, it may take hundreds milliseconds which is unacceptable
> for the real-time graphics operations. Let's try a non-blocking allocation
> first as a least invasive solution, it makes opentegra (Xorg driver)
> performance indistinguishable with/without the firewall.
>
> Signed-off-by: Dmitry Osipenko 
> ---
>
> The other much more invasive solution could be: to have a memory pool
> reserved for the gather copy and performing the firewall checks (and the
> gather copying) on the actual job submission to the CDMA, not on the job
> allocation like it is done now. This solution reduces the overhead of a
> gather copy allocations.
>
>  drivers/gpu/host1x/job.c | 16 
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index fc194c676d91..ed0575f14093 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -594,12 +594,20 @@ static inline int copy_gathers(struct host1x_job *job, 
> struct device *dev)
> size += g->words * sizeof(u32);
> }
>
> +   /*
> +* Try a non-blocking allocation from a higher priority pools first,
> +* as awaiting for the allocation here is a major performance hit.
> +*/
> job->gather_copy_mapped = dma_alloc_wc(dev, size, >gather_copy,
> -  GFP_KERNEL);
> -   if (!job->gather_copy_mapped) {
> -   job->gather_copy_mapped = NULL;
> +  GFP_NOWAIT);
> +
> +   /* the higher priority allocation failed, try the generic-blocking */
> +   if (!job->gather_copy_mapped)
> +   job->gather_copy_mapped = dma_alloc_wc(dev, size,
> +  >gather_copy,
> +  GFP_KERNEL);
> +   if (!job->gather_copy_mapped)
> return -ENOMEM;
> -   }
>
> job->gather_copy_size = size;

Can't we just have a static fixed-size buffer, and validate chunks at
the time? Or why can't we validate directly on the mapped pointers?

It feels pretty silly to have to allocate memory in the first place here...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 18/22] gpu: host1x: Check waits in the firewall

[Erik Faye-Lund]

On Wed, Jun 14, 2017 at 1:15 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
> Check waits in the firewall in a way it is done for relocations.
>
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> Reviewed-by: Mikko Perttunen <mperttu...@nvidia.com>

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 11/22] gpu: host1x: Initialize firewall class to the jobs one

[Erik Faye-Lund]

On Wed, Jun 14, 2017 at 1:15 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
> The commands stream is prepended by the jobs class on the CDMA submission,
> so that explicitly setting a module class in the commands stream isn't
> necessary. The firewall initializes its class to 0 and the command stream
> that doesn't explicitly specify the class effectively bypasses the firewall.
>
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> Reviewed-by: Mikko Perttunen <mperttu...@nvidia.com>

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 09/22] drm/tegra: Don't use IOMMU on Tegra20

[Erik Faye-Lund]

On Wed, Jun 14, 2017 at 1:15 AM, Dmitry Osipenko  wrote:
> There is no IOMMU on Tegra20, instead a GART would be picked as an IOMMU
> provider.
>
> Signed-off-by: Dmitry Osipenko 
> Acked-by: Joerg Roedel 
> ---
>  drivers/gpu/drm/tegra/drm.c | 3 ++-
>  drivers/gpu/host1x/dev.c| 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index e999391aedc9..aa7988dcc28f 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -131,7 +131,8 @@ static int tegra_drm_load(struct drm_device *drm, 
> unsigned long flags)
> if (!tegra)
> return -ENOMEM;
>
> -   if (iommu_present(_bus_type)) {
> +   if (iommu_present(_bus_type) &&
> +   !of_machine_is_compatible("nvidia,tegra20")) {
> u64 carveout_start, carveout_end, gem_start, gem_end;
> struct iommu_domain_geometry *geometry;
> unsigned long order;
> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
> index f05ebb14fa63..6a805ed908c3 100644
> --- a/drivers/gpu/host1x/dev.c
> +++ b/drivers/gpu/host1x/dev.c
> @@ -177,7 +177,8 @@ static int host1x_probe(struct platform_device *pdev)
> return err;
> }
>
> -   if (iommu_present(_bus_type)) {
> +   if (iommu_present(_bus_type) &&
> +   !of_machine_is_compatible("nvidia,tegra20")) {
> struct iommu_domain_geometry *geometry;
> unsigned long order;
>

This doesn't feel great... The commit message says there's no IOMMU,
but iommu_present says otherwise. I know there's some more subtleties
here, and the commit message does hint at this. But...

If we don't want to treat the GART as an IOMMU, shouldn't we somehow
make sure iommu_present() doesn't return true in these cases (or
perhaps make something like tegra_use_iommu(), with a comment
explaining why we don't want to allow the GART to be treated like a
proper IOMMU)? These seems to be the only Tegra-specific calls to
iommu_present()...

That being said, the patch seems to have the right effect...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 08/22] drm/tegra: dc: Disable plane if it is invisible

[Erik Faye-Lund]

On Wed, Jun 14, 2017 at 1:15 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
> On Tegra20 if plane has width or height equal to 0, it will be infinitely
> wide or tall. Let's disable the plane if it is invisible on atomic state
> committing to fix the issue. The Rockchip DRM driver does the same.
>
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>

Looks good as far as I can tell

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 07/22] drm/tegra: dc: Apply clipping to the plane

[Erik Faye-Lund]

On Wed, Jun 14, 2017 at 1:15 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
> On Tegra20 an overlay plane should be clipped, otherwise its output is
> distorted once plane crosses display boundary.
>
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> ---
>  drivers/gpu/drm/tegra/dc.c | 29 +
>  1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 98ee6abb056c..a7a7cce1afd0 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -486,12 +486,25 @@ static int tegra_plane_state_add(struct tegra_plane 
> *plane,
>  {
> struct drm_crtc_state *crtc_state;
> struct tegra_dc_state *tegra;
> +   struct drm_rect clip;
> +   int err;
>
> /* Propagate errors from allocation or locking failures. */
> crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc);
> if (IS_ERR(crtc_state))
> return PTR_ERR(crtc_state);
>
> +   clip.x1 = 0;
> +   clip.y1 = 0;
> +   clip.x2 = crtc_state->mode.hdisplay;
> +   clip.y2 = crtc_state->mode.vdisplay;
> +
> +   /* Check plane state for visibility and calculate clipping bounds */
> +   err = drm_plane_helper_check_state(state, , 0, INT_MAX,
> +  true, true);
> +   if (err < 0)
> +   return err;
> +
> tegra = to_dc_state(crtc_state);
>
> tegra->planes |= WIN_A_ACT_REQ << plane->index;
> @@ -561,14 +574,14 @@ static void tegra_plane_atomic_update(struct drm_plane 
> *plane,
> return;
>
> memset(, 0, sizeof(window));
> -   window.src.x = plane->state->src_x >> 16;
> -   window.src.y = plane->state->src_y >> 16;
> -   window.src.w = plane->state->src_w >> 16;
> -   window.src.h = plane->state->src_h >> 16;
> -   window.dst.x = plane->state->crtc_x;
> -   window.dst.y = plane->state->crtc_y;
> -   window.dst.w = plane->state->crtc_w;
> -   window.dst.h = plane->state->crtc_h;
> +   window.src.x = plane->state->src.x1 >> 16;
> +   window.src.y = plane->state->src.y1 >> 16;
> +   window.src.w = drm_rect_width(>state->src) >> 16;
> +   window.src.h = drm_rect_height(>state->src) >> 16;
> +   window.dst.x = plane->state->dst.x1;
> +   window.dst.y = plane->state->dst.y1;
> +   window.dst.w = drm_rect_width(>state->dst);
> +   window.dst.h = drm_rect_height(>state->dst);
> window.bits_per_pixel = fb->format->cpp[0] * 8;
> window.bottom_up = tegra_fb_is_bottom_up(fb);
>

Looks good as far as I can tell.

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 06/22] drm/tegra: dc: Avoid reset asserts on Tegra20

[Erik Faye-Lund]

On Wed, Jun 14, 2017 at 1:15 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
> Commit 33a8eb8 ("Implement runtime PM") introduced HW reset control. It
> causes a hang on Tegra20 if both display controllers are utilized (RGB
> panel and HDMI). The TRM suggests that each display controller has its own
> reset control, apparently it is not correct.
>
> Fixes: 33a8eb8d40ee ("drm/tegra: dc: Implement runtime PM")
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 01/22] drm/tegra: Fix lockup on a use of staging API

[Erik Faye-Lund]

On Wed, Jun 14, 2017 at 1:15 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
> Commit bdd2f9cd ("Don't leak kernel pointer to userspace") added a mutex
> around staging IOCTL's, some of those mutexes are taken twice.
>
> Fixes: bdd2f9cd10eb ("drm/tegra: Don't leak kernel pointer to userspace")
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> Reviewed-by: Mikko Perttunen <mperttu...@nvidia.com>

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 19/22] gpu: host1x: Remove unused host1x_cdma_stop() definition

[Erik Faye-Lund]

On Tue, May 23, 2017 at 2:14 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
> There is no host1x_cdma_stop() in the code, let's remove its definition
> from the header file.
>
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> ---
>  drivers/gpu/host1x/cdma.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/host1x/cdma.h b/drivers/gpu/host1x/cdma.h
> index ec170a78f4e1..286d49386be9 100644
> --- a/drivers/gpu/host1x/cdma.h
> +++ b/drivers/gpu/host1x/cdma.h
> @@ -88,7 +88,6 @@ struct host1x_cdma {
>
>  int host1x_cdma_init(struct host1x_cdma *cdma);
>  int host1x_cdma_deinit(struct host1x_cdma *cdma);
> -void host1x_cdma_stop(struct host1x_cdma *cdma);
>  int host1x_cdma_begin(struct host1x_cdma *cdma, struct host1x_job *job);
>  void host1x_cdma_push(struct host1x_cdma *cdma, u32 op1, u32 op2);
>  void host1x_cdma_end(struct host1x_cdma *cdma, struct host1x_job *job);
> --
> 2.13.0
>

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 18/22] gpu: host1x: Remove unused 'struct host1x_cmdbuf'

[Erik Faye-Lund]

On Tue, May 23, 2017 at 2:14 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
> The struct host1x_cmdbuf is unused, let's remove it.
>
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> ---
>  drivers/gpu/host1x/job.h | 7 ---
>  1 file changed, 7 deletions(-)
>
> diff --git a/drivers/gpu/host1x/job.h b/drivers/gpu/host1x/job.h
> index 0debd93a1849..4bda51d503ec 100644
> --- a/drivers/gpu/host1x/job.h
> +++ b/drivers/gpu/host1x/job.h
> @@ -27,13 +27,6 @@ struct host1x_job_gather {
> bool handled;
>  };
>
> -struct host1x_cmdbuf {
> -   u32 handle;
> -   u32 offset;
> -   u32 words;
> -   u32 pad;
> -};
> -
>  struct host1x_job_unpin_data {
> struct host1x_bo *bo;
> struct sg_table *sgt;
> --
> 2.13.0
>

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 16/22] gpu: host1x: Forbid unrelated SETCLASS opcode in the firewall

[Erik Faye-Lund]

lass = word >> 6 & 0x3ff;
> fw->mask = word & 0x3f;
> fw->reg = word >> 16 & 0xfff;
> -   err = check_mask(fw);
> +   err = check_class(fw, job_class);
> +   if (!err)
> +   err = check_mask(fw);
> if (err)
> goto out;
> break;
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index aa323e43ae4e..561d6bb6580d 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -233,7 +233,10 @@ struct host1x_job {
> u8 *gather_copy_mapped;
>
> /* Check if register is marked as an address reg */
> -   int (*is_addr_reg)(struct device *dev, u32 reg, u32 class);
> +   int (*is_addr_reg)(struct device *dev, u32 class, u32 reg);

This seems like an unrelated fix, you might want to mention it in the
commit message at least.

> +
> +   /* Check if class belongs to the unit */
> +   int (*is_valid_class)(u32 class);
>
> /* Request a SETCLASS to this class */
> u32 class;
> --
> 2.13.0
>

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 14/22] gpu: host1x: Forbid relocation address shifting in the firewall

[Erik Faye-Lund]

On Tue, May 23, 2017 at 2:14 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
> Incorrectly shifted relocation address will cause a lower memory corruption
> and likely a hang on a write or a read of an arbitrary data in case of IOMMU
> absent. As of now there is no use for the address shifting (at least on
> Tegra20) and adding a proper shifting / sizes validation is much more work.
> Let's forbid it in the firewall till a proper validation is implemented.
>
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> ---
>  drivers/gpu/host1x/job.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index 190353944d23..1a1568e64ba8 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -332,6 +332,10 @@ static bool check_reloc(struct host1x_reloc *reloc, 
> struct host1x_bo *cmdbuf,
> if (reloc->cmdbuf.bo != cmdbuf || reloc->cmdbuf.offset != offset)
> return false;
>
> +   /* relocation shift value validation isn't implemented yet */
> +   if (reloc->shift)
> +   return false;
> +
> return true;
>  }
>
> --
> 2.13.0
>

Good call.


Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 12/22] gpu: host1x: Correct host1x_job_pin() error handling

[Erik Faye-Lund]

On Tue, May 23, 2017 at 2:14 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
> In case of relocations / waitchecks patching failure the jobs pins stay
> referenced till DRM file get closed, wasting memory. Add the missed
> unpinning.
>
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> ---
>  drivers/gpu/host1x/job.c | 16 +++-
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index d9933828fe87..14f3f957ffab 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -592,22 +592,20 @@ int host1x_job_pin(struct host1x_job *job, struct 
> device *dev)
>
> err = do_relocs(job, g->bo);
> if (err)
> -   break;
> +   goto out;
>
> err = do_waitchks(job, host, g->bo);
> if (err)
> -   break;
> +   goto out;
> }
>
> -   if (IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && !err) {
> -   err = copy_gathers(job, dev);
> -   if (err) {
> -   host1x_job_unpin(job);
> -   return err;
> -   }
> -   }
> +   if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL))
> +   goto out;
>
> +   err = copy_gathers(job, dev);
>  out:
> +   if (err)
> +   host1x_job_unpin(job);
> wmb();
>
> return err;
> --
> 2.13.0
>

One subtle undocumented change here, is that wmb() now gets called in
the copy_gathers()-error case (just like it already does for the other
error-cases). That's seems like an OK change, so:

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/22] drm/tegra: Check whether page belongs to BO in tegra_bo_kmap()

[Erik Faye-Lund]

On Tue, May 23, 2017 at 2:14 AM, Dmitry Osipenko <dig...@gmail.com> wrote:
> This fixes an OOPS in case of out-of-bounds accessing of a kmap'ed cmdbuf
> (non-IOMMU allocation) while patching the relocations in do_relocs().
>
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
> ---
>  drivers/gpu/drm/tegra/gem.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> index 424569b53e57..ca0d4439e97b 100644
> --- a/drivers/gpu/drm/tegra/gem.c
> +++ b/drivers/gpu/drm/tegra/gem.c
> @@ -74,6 +74,9 @@ static void *tegra_bo_kmap(struct host1x_bo *bo, unsigned 
> int page)
>  {
> struct tegra_bo *obj = host1x_to_tegra_bo(bo);
>
> +   if (page * PAGE_SIZE >= obj->gem.size)
> +   return NULL;
> +
> if (obj->vaddr)
> return obj->vaddr + page * PAGE_SIZE;
> else if (obj->gem.import_attach)
> --
> 2.13.0
>

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/tegra: Check offsets of a submitted command buffer and of relocations

[Erik Faye-Lund]

On Tue, May 16, 2017 at 8:56 AM, Mikko Perttunen <cyn...@kapsi.fi> wrote:
> On 14.05.2017 23:47, Dmitry Osipenko wrote:
>>
>> If commands buffer claims a number of words that is higher than its BO can
>> fit, a kernel OOPS will be fired on the out-of-bounds BO access. This was
>> triggered by an opentegra Xorg driver that erroneously pushed too many
>> commands to the pushbuf. The CMDA commands buffer address is 4 bytes
>> aligned, so check the alignment as well.
>>
>> Add a sanity check for the relocations in a same way.
>>
>> [   46.829393] Unable to handle kernel paging request at virtual address
>> f09b2000
>> ...
>> [] (host1x_job_pin) from []
>> (tegra_drm_submit+0x474/0x510)
>> [] (tegra_drm_submit) from [] (tegra_submit+0x50/0x6c)
>> [] (tegra_submit) from [] (drm_ioctl+0x1e4/0x3ec)
>> [] (drm_ioctl) from [] (do_vfs_ioctl+0x9c/0x8e4)
>> [] (do_vfs_ioctl) from [] (SyS_ioctl+0x34/0x5c)
>> [] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x3c)
>>
>> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>
>> Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
>> ---
>>  drivers/gpu/drm/tegra/drm.c | 30 ++
>>  drivers/gpu/drm/tegra/gem.c |  5 -
>>  drivers/gpu/drm/tegra/gem.h |  5 +
>>  3 files changed, 35 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
>> index 768750226452..c5844a065681 100644
>> --- a/drivers/gpu/drm/tegra/drm.c
>> +++ b/drivers/gpu/drm/tegra/drm.c
>> @@ -362,6 +362,8 @@ int tegra_drm_submit(struct tegra_drm_context
>> *context,
>> while (num_cmdbufs) {
>> struct drm_tegra_cmdbuf cmdbuf;
>> struct host1x_bo *bo;
>> +   struct tegra_bo *obj;
>> +   u64 offset;
>>
>> if (copy_from_user(, cmdbufs, sizeof(cmdbuf))) {
>> err = -EFAULT;
>> @@ -374,6 +376,14 @@ int tegra_drm_submit(struct tegra_drm_context
>> *context,
>> goto fail;
>> }
>>
>> +   offset = (u64)cmdbuf.offset + (u64)cmdbuf.words *
>> sizeof(u32);
>> +   obj = host1x_to_tegra_bo(bo);
>> +
>> +   if (offset & 3 || offset > obj->gem.size) {
>> +   err = -EINVAL;
>> +   goto fail;
>> +   }
>> +
>> host1x_job_add_gather(job, bo, cmdbuf.words,
>> cmdbuf.offset);
>> num_cmdbufs--;
>> cmdbufs++;
>> @@ -381,11 +391,31 @@ int tegra_drm_submit(struct tegra_drm_context
>> *context,
>>
>> /* copy and resolve relocations from submit */
>> while (num_relocs--) {
>> +   struct host1x_reloc *reloc;
>> +   struct tegra_bo *obj;
>> +
>> err =
>> host1x_reloc_copy_from_user(>relocarray[num_relocs],
>>   [num_relocs],
>> drm,
>>   file);
>> if (err < 0)
>> goto fail;
>> +
>> +   reloc = >relocarray[num_relocs];
>> +   obj = host1x_to_tegra_bo(reloc->cmdbuf.bo);
>> +
>> +   if (reloc->cmdbuf.offset & 3 ||
>> +   reloc->cmdbuf.offset > obj->gem.size) {
>
>
> This could still fail if the bo's size is not divisible by 4, even with >=
> comparison (we would overwrite the buffer by 1 to 3 bytes). I would do the
> same as in the gather case, i.e. find out the address immediately after the
> write and compare using >. Perhaps add a helper function if it makes sense.
> I also don't think the "& 3" checks are needed.

The bo-size is always a multiple of PAGE_SIZE, due to the rounding in
tegra_bo_alloc_object(), so I don't think this actually can fail. But
maybe we want to future-proof this code for a potential future where
this is not the case?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] gpu: host1x: Forbid RESTART opcode in the firewall

[Erik Faye-Lund]

On Mon, May 15, 2017 at 2:02 PM, Dmitry Osipenko <dig...@gmail.com> wrote:
> The RESTART opcode terminates the gather and restarts the CDMA fetch from
> a specified word << 2 relative to the CDMA start address. That shouldn't
> be allowed to be done by userspace.
>
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>

Outch, yeah.

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/tegra: Check size of a submitted command buffer

[Erik Faye-Lund]

On Fri, May 12, 2017 at 9:29 PM, Dmitry Osipenko <dig...@gmail.com> wrote:
> If command buffer claims a number of words that is higher than its BO can
> fit and a relocation lays past the BO, a kernel OOPS will be fired on that
> relocation address patching. This was triggered by an opentegra Xorg driver
> that erroneously pushed too many commands to the pushbuf.
>
> [   46.829393] Unable to handle kernel paging request at virtual address 
> f09b2000
> ...
> [] (host1x_job_pin) from [] (tegra_drm_submit+0x474/0x510)
> [] (tegra_drm_submit) from [] (tegra_submit+0x50/0x6c)
> [] (tegra_submit) from [] (drm_ioctl+0x1e4/0x3ec)
> [] (drm_ioctl) from [] (do_vfs_ioctl+0x9c/0x8e4)
> [] (do_vfs_ioctl) from [] (SyS_ioctl+0x34/0x5c)
> [] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x3c)
>
> Signed-off-by: Dmitry Osipenko <dig...@gmail.com>

Looks good, good catch!

Reviewed-by: Erik Faye-Lund <kusmab...@gmail.com>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/tegra: Check size of a submitted command buffer

[Erik Faye-Lund]

On Fri, May 12, 2017 at 9:02 PM, Dmitry Osipenko  wrote:
> If command buffer claims a number of words that is higher than its BO can
> fit and a relocation lays past the BO, a kernel OOPS will be fired on that
> relocation address patching. This was triggered by an opentegra Xorg driver
> that erroneously pushed too many commands to the pushbuf.
>
> [   46.829393] Unable to handle kernel paging request at virtual address 
> f09b2000
> ...
> [] (host1x_job_pin) from [] (tegra_drm_submit+0x474/0x510)
> [] (tegra_drm_submit) from [] (tegra_submit+0x50/0x6c)
> [] (tegra_submit) from [] (drm_ioctl+0x1e4/0x3ec)
> [] (drm_ioctl) from [] (do_vfs_ioctl+0x9c/0x8e4)
> [] (do_vfs_ioctl) from [] (SyS_ioctl+0x34/0x5c)
> [] (SyS_ioctl) from [] (ret_fast_syscall+0x0/0x3c)
>
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/gpu/drm/tegra/drm.c | 18 ++
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 732c8d98044f..e9c74a7780e7 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -361,20 +361,30 @@ int tegra_drm_submit(struct tegra_drm_context *context,
>
> while (num_cmdbufs) {
> struct drm_tegra_cmdbuf cmdbuf;
> -   struct host1x_bo *bo;
> +   struct drm_gem_object *gem;
> +   struct tegra_bo *bo;
>
> if (copy_from_user(, cmdbufs, sizeof(cmdbuf))) {
> err = -EFAULT;
> goto fail;
> }
>
> -   bo = host1x_bo_lookup(file, cmdbuf.handle);
> -   if (!bo) {
> +   gem = drm_gem_object_lookup(file, cmdbuf.handle);
> +   if (!gem) {
> err = -ENOENT;
> goto fail;
> }
>
> -   host1x_job_add_gather(job, bo, cmdbuf.words, cmdbuf.offset);
> +   drm_gem_object_unreference_unlocked(gem);
> +
> +   if (cmdbuf.words * 4 > gem->size) {

Shouldn't this be "cmdbuf.offset + cmdbuf.words * 4 > gem->size"?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 2/2] tegra: update symbol-check

[Erik Faye-Lund]

On Mon, Apr 3, 2017 at 6:59 PM, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> On 29 March 2017 at 23:24, Erik Faye-Lund <kusmab...@gmail.com> wrote:
>> I get a few more symbols in my build tegra-libraries, so let's
>> include these in the whitelist as well.
>>
>> While we're at it, update the comment at the top.
>>
>> Signed-off-by: Erik Faye-Lund <kusmab...@gmail.com>
> R-B and pushed to master.
>
> Out of curiosity:
> What platform are you using that introduces these - musl, newer glibc, other ?

I'm using glibc 2.25 (in particular, 2.25-1 from Arch Linux).

> Would be great if we can strip platform specific symbols from the list
> - do you have any ideas how we can do that ?
> We could omit any symbols that start with __ but that does not sound too 
> robust.

I think ignoring anything that starts with both single and double
underscore would make sense, as those symbols are reserved by the
compiler and "implementation" (typically libc etc). Our API shouldn't
export these symbols in the first place, and I suspect code-review is
sufficient to ensure this.

But either way, I'd prefer to just get the test working again first.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 2/2] tegra: update symbol-check

[Erik Faye-Lund]

I get a few more symbols in my build tegra-libraries, so let's
include these in the whitelist as well.

While we're at it, update the comment at the top.

Signed-off-by: Erik Faye-Lund <kusmab...@gmail.com>
---
 tegra/tegra-symbol-check | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tegra/tegra-symbol-check b/tegra/tegra-symbol-check
index 40208311..420469f4 100755
--- a/tegra/tegra-symbol-check
+++ b/tegra/tegra-symbol-check
@@ -1,11 +1,14 @@
 #!/bin/bash
 
-# The following symbols (past the first five) are taken from the public 
headers.
-# A list of the latter should be available 
Makefile.sources/LIBDRM_FREEDRENO_H_FILES
+# The following symbols (past the first nine) are taken from tegra.h.
 
 FUNCS=$(nm -D --format=bsd --defined-only ${1-.libs/libdrm_tegra.so} | awk 
'{print $3}'| while read func; do
 ( grep -q "^$func$" || echo $func )  <https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 1/2] tests/tegra: add openclose test to check-target

[Erik Faye-Lund]

This makes the test run under the 'make check'-taget.

Signed-off-by: Erik Faye-Lund <kusmab...@gmail.com>
---
 tests/tegra/Makefile.am | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
index 8e625c8f..f8e2a146 100644
--- a/tests/tegra/Makefile.am
+++ b/tests/tegra/Makefile.am
@@ -9,5 +9,7 @@ LDADD = \
../../tegra/libdrm_tegra.la \
../../libdrm.la
 
-noinst_PROGRAMS = \
+TESTS = \
openclose
+
+check_PROGRAMS = $(TESTS)
-- 
2.12.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/tegra: add tiling FB modifiers

[Erik Faye-Lund]

On Tue, Nov 8, 2016 at 8:50 AM, Alexandre Courbot  
wrote:
> Add FB modifiers to allow user-space to specify that a surface is in one
> of the two tiling formats supported by Tegra chips, and add support in
> the tegradrm driver to handle them properly. This is necessary for the
> display controller to directly display buffers generated by the GPU.
>
> This feature is intended to replace the dedicated IOCTL enabled
> by TEGRA_STAGING and to provide a non-staging alternative to that
> solution.
>
> Signed-off-by: Alexandre Courbot 
> ---
>  drivers/gpu/drm/tegra/drm.c   |  2 ++
>  drivers/gpu/drm/tegra/fb.c| 23 +++---
>  include/uapi/drm/drm_fourcc.h | 45 
> +++
>  3 files changed, 67 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index a9630c2d6cb3..36b4b30a5164 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -161,6 +161,8 @@ static int tegra_drm_load(struct drm_device *drm, 
> unsigned long flags)
> drm->mode_config.max_width = 4096;
> drm->mode_config.max_height = 4096;
>
> +   drm->mode_config.allow_fb_modifiers = true;
> +
> drm->mode_config.funcs = _drm_mode_funcs;
>
> err = tegra_drm_fb_prepare(drm);
> diff --git a/drivers/gpu/drm/tegra/fb.c b/drivers/gpu/drm/tegra/fb.c
> index e6d71fa4028e..2fded58b2ca5 100644
> --- a/drivers/gpu/drm/tegra/fb.c
> +++ b/drivers/gpu/drm/tegra/fb.c
> @@ -52,9 +52,26 @@ int tegra_fb_get_tiling(struct drm_framebuffer 
> *framebuffer,
> struct tegra_bo_tiling *tiling)
>  {
> struct tegra_fb *fb = to_tegra_fb(framebuffer);
> -
> -   /* TODO: handle YUV formats? */
> -   *tiling = fb->planes[0]->tiling;
> +   uint64_t modifier = fb->base.modifier[0];
> +
> +   switch (fourcc_mod_tegra_mod(modifier)) {
> +   case NV_FORMAT_MOD_TEGRA_TILED:
> +   tiling->mode = TEGRA_BO_TILING_MODE_TILED;
> +   tiling->value = 0;
> +   break;
> +
> +   case NV_FORMAT_MOD_TEGRA_16BX2_BLOCK(0):
> +   tiling->mode = TEGRA_BO_TILING_MODE_BLOCK;
> +   tiling->value = fourcc_mod_tegra_param(modifier);
> +   if (tiling->value > 5)
> +   return -EINVAL;

Shouldn't this contain some hardware-check for the support? AFAIK, not
all Tegras support all block-heights (if even this mode at all?)...

[PATCH v2] drm/tegra: Add tegra_gem_mmap2 to fix 64-bit offsets

[Erik Faye-Lund]

On Fri, Jan 30, 2015 at 10:49 AM, Thierry Reding
 wrote:
> On Thu, Jan 29, 2015 at 02:18:41PM -0500, Sean Paul wrote:
>> On 64-bit targets, tegra_gem_mmap doesn't return the
>> offset to userspace. As such, subsequent calls to mmap(2)
>> fail. Add a new tegra_gem_mmap2 ioctl to fix this.
>>
>> Signed-off-by: Sean Paul 
>> ---
>>  drivers/gpu/drm/tegra/drm.c  | 21 +
>>  include/uapi/drm/tegra_drm.h |  9 +
>>  2 files changed, 30 insertions(+)
>
> To be honest, I'd rather just fix the existing IOCTL to do the right
> thing on 64-bit. All IOCTLs are still protected by the DRM_TEGRA_STAGING
> Kconfig symbol which depends on STAGING. We originally did that
> precisely so we'd have some leeway in fixing things up. And we've done
> precisely that in the past.
>
> The only user of this IOCTL is libdrm and I don't think that has any
> users aside from a few projects that are still under heavy development
> (like grate or the xf86-video-opentegra driver).
>
> Cc'ing Erik, who's probably the only one that's ever worked with this,
> besides me.

I also saw the patch, and had the same reaction. I'm fine with
changing the ABI, it was done already anyway
(cbfbbabb89b37f6bad05f478d906a385149f288d, "drm/tegra: Remove
gratuitous pad field"). And as you say, this is only in staging so
nobody is really relying on it, except grate and libdrm (in which this
is clearly marked as experimental). I'm fine with just changing it,
and updating grate and libdrm accordingly.

[PATCH v2 libdrm 4/7] tegra: Add channel, job, pushbuf and fence APIs

[Erik Faye-Lund]

On Fri, May 2, 2014 at 5:16 PM, Thierry Reding  
wrote:
> On Fri, May 02, 2014 at 04:53:55PM +0200, Erik Faye-Lund wrote:
>> On Fri, May 2, 2014 at 4:06 PM, Thierry Reding  
>> wrote:
>> > On Thu, Apr 10, 2014 at 07:13:22PM +0200, Erik Faye-Lund wrote:
>> >> > diff --git a/tegra/pushbuf.c b/tegra/pushbuf.c
>> >> > new file mode 100644
>> >> > index ..178d5cd57541
>> >> > --- /dev/null
>> >> > +++ b/tegra/pushbuf.c
>> >> > @@ -0,0 +1,205 @@
>> >> 
>> >> > +drm_public
>> >> > +int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp,
>> >> > + struct drm_tegra_job *job)
>> >> > +{
>> >> > +   struct drm_tegra_pushbuf_private *pushbuf;
>> >> > +   void *ptr;
>> >> > +   int err;
>> >> > +
>> >> > +   pushbuf = calloc(1, sizeof(*pushbuf));
>> >> > +   if (!pushbuf)
>> >> > +   return -ENOMEM;
>> >> > +
>> >> > +   DRMINITLISTHEAD(>list);
>> >> > +   DRMINITLISTHEAD(>bos);
>> >> > +   pushbuf->job = job;
>> >> > +
>> >> > +   *pushbufp = >base;
>> >> > +
>> >> > +   DRMLISTADD(>list, >pushbufs);
>> >>
>> >> Hmm. So the job keeps a list of pushbufs. What purpose does this
>> >> serve? Shouldn't the job only need the cmdbuf-list (which it already
>> >> has) and the BOs (so they can be dereferenced after being submitted)?
>> >
>> > This is currently used to keep track of existing pushbuffers and make
>> > sure that they are freed (when the job is freed).
>> >
>>
>> OK, it seems to me that the need for keeping the pushbuffers around is
>> simply not there. Only the BOs needs to be kept around, no?
>
> Right. But unless we come up with an alternative API I can't just drop
> this. Otherwise we'll be leaking pushbuffers all over the place if
> callers don't clean them up themselves.
>

OK. But just to get some clarity, if the same level of abstraction
calls drm_tegra_pushbuf_new() and drm_tegra_pushbuf_free() when it's
done, we'll currently get use-after-free and double-frees, right?

As-is, the client would have to call drm_tegra_pushbuf_new(), transfer
the ownership of that object to the job object by calling
drm_tegra_pushbuf_prepare(), and clean it up by calling
drm_tegra_job_submit()? Or am I missing something?

>> > If reusing the pushbuf objects makes sense, then perhaps a different API
>> > would be more useful. Rather than allocate in the context of a job, they
>> > could be allocated in a channel-wide context and added to/associated
>> > with a job only as needed.
>>
>> Yes, I think this might make sense. I'll have to ponder a bit more
>> about this, though.
>
> What I've been thinking is something rougly like this:
>
> int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbuf, struct
>   drm_tegra_channel *channel);
> int drm_tegra_pushbuf_free(struct drm_tegra_pushbuf *pushbuf);
> int drm_tegra_pushbuf_reset(struct drm_tegra_pushbuf *pushbuf);
>
> int drm_tegra_job_queue(struct drm_tegra_job *job,
> struct drm_tegra_pushbuf *pushbuf);
>
> Then we could either reset the pushbuf in drm_tegra_job_queue() (in
> which case drm_tegra_pushbuf_reset() could be private) or leave it up to
> the caller to manually reset when they need to reuse the pushbuffer.
>

OK, this looks better to me. I'll still have to wrap my head around
these things a bit better to tell for sure, though ;)

>> >> Either way, I think this should either call drm_tegra_pushbuf_prepare
>> >> to make sure two words are available, or be renamed to reflect that it
>> >> causes a push (or two, as in the current form).
>> >
>> > I've added a call to drm_tegra_pushbuf_prepare() here and in
>> > drm_tegra_pushbuf_relocate() which also pushes one word to the pushbuf.
>> >
>>
>> Actually, I think drm_tegra_pushbuf_relocate should just be renamed to
>> drm_tegra_pushbuf_push_reloc(), as that conveys quite clearly that it
>> is just a push-helper and need to be counted when preparing. That way
>> it doesn't need to prepare itself.
>
> I don't think we necessarily need to rename the function to make it
> obvious. This is completely new API anyway, so we can add comments to
> explain that drm_tegra_pushbuf_relocate() will push a placeholder word
> with a dummy address

[PATCH v2 libdrm 5/7] tegra: Add helper library for tests

[Erik Faye-Lund]

On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding  
wrote:
> +int drm_open(const char *path)
> +{
> +   int fd, err;
> +
> +   fd = open(path, O_RDWR);
> +   if (fd < 0)
> +   return -errno;
> +
> +   err = drmSetMaster(fd);
> +   if (err < 0) {
> +   close(fd);
> +   return -errno;
> +   }

Could we make this opt-in? Otherwise "make check" needs to be run as
root to pass, which is a bit iffy IMO as it also invokes the compiler
and all tools as root. I'd prefer not having to give my dev-tools (and
potentially buggy in-tree scripts) that much privilege...

[PATCH v2 libdrm 5/7] tegra: Add helper library for tests

[Erik Faye-Lund]

On Fri, May 2, 2014 at 4:42 PM, Thierry Reding  
wrote:
> On Thu, Apr 10, 2014 at 07:33:33PM +0200, Erik Faye-Lund wrote:
>> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding  
>> wrote:
>> > From: Thierry Reding 
>> >
>> > This library provides helpers for common functionality needed by test
>> > programs.
>> >
>> > Signed-off-by: Thierry Reding 
>> > ---
>> > Changes in v2:
>> > - fix a couple of memory leaks and get rid of some unneeded code
>> >
>> >  tests/tegra/Makefile.am  |  10 +-
>> >  tests/tegra/drm-test-tegra.c | 137 
>> >  tests/tegra/drm-test-tegra.h |  55 ++
>> >  tests/tegra/drm-test.c   | 248 
>> > +++
>> >  tests/tegra/drm-test.h   |  72 +
>> >  5 files changed, 521 insertions(+), 1 deletion(-)
>> >  create mode 100644 tests/tegra/drm-test-tegra.c
>> >  create mode 100644 tests/tegra/drm-test-tegra.h
>> >  create mode 100644 tests/tegra/drm-test.c
>> >  create mode 100644 tests/tegra/drm-test.h
>>
>> This isn't really important at this point, but it looks to me like
>> tests/tegra/drm-test.[ch] isn't really tegra-specific. If so, perhaps
>> some other tests can benefit from it? Doing so is of course something
>> whoever writes those tests should do, though. Leaving them in the
>> tegra-subdir is probably fine.
>
> Daniel Vetter and I have been "discussing" this for a while now. There
> are a bunch of tests in the intel-gpu-tools repository that aren't Intel
> specific either and the idea is to eventually collect all those test
> cases in a common location so that they can be reused by other drivers
> too, but so far nobody's had time to do that yet.

Right. Let's just leave it for later, then.

>> > +int drm_tegra_gr2d_fill(struct drm_tegra_gr2d *gr2d, struct 
>> > drm_framebuffer *fb,
>> > +   unsigned int x, unsigned int y, unsigned int width,
>> > +   unsigned int height, uint32_t color)
>> > +{
>> > +   struct drm_tegra_bo *fbo = fb->data;
>> > +   struct drm_tegra_pushbuf *pushbuf;
>> > +   struct drm_tegra_fence *fence;
>> > +   struct drm_tegra_job *job;
>> > +   int err;
>> > +
>> > +   err = drm_tegra_job_new(, gr2d->channel);
>> > +   if (err < 0)
>> > +   return err;
>> > +
>> > +   err = drm_tegra_pushbuf_new(, job);
>> > +   if (err < 0)
>> > +   return err;
>> > +
>>
>> I think this helper would be generally more useful if it skipped the
>> above two, and required the call-sites to do them instead.
>>
>> > +   err = drm_tegra_pushbuf_prepare(pushbuf, 32);
>> > +   if (err < 0)
>> > +   return err;
>> > +
>> > +   *pushbuf->ptr++ = HOST1X_OPCODE_SETCL(0, HOST1X_CLASS_GR2D, 0);
>> > +
>> > +   *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x9, 0x9);
>> > +   *pushbuf->ptr++ = 0x003a;
>> > +   *pushbuf->ptr++ = 0x;
>> > +
>> > +   *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x1e, 0x7);
>> > +   *pushbuf->ptr++ = 0x;
>> > +   *pushbuf->ptr++ = (2 << 16) | (1 << 6) | (1 << 2);
>> > +   *pushbuf->ptr++ = 0x00cc;
>> > +
>> > +   *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x2b, 0x9);
>> > +
>> > +   /* relocate destination buffer */
>> > +   err = drm_tegra_pushbuf_relocate(pushbuf, fbo, 0, 0);
>> > +   if (err < 0) {
>> > +   fprintf(stderr, "failed to relocate buffer object: %d\n", 
>> > err);
>> > +   return err;
>> > +   }
>> > +
>> > +   *pushbuf->ptr++ = fb->pitch;
>> > +
>> > +   *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x35, 1);
>> > +   *pushbuf->ptr++ = color;
>> > +
>> > +   *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x46, 1);
>> > +   *pushbuf->ptr++ = 0x;
>> > +
>> > +   *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x38, 0x5);
>> > +   *pushbuf->ptr++ = height << 16 | width;
>> > +   *pushbuf->ptr++ = y << 16 | x;
>> > +
>>
>> ...and stop here.
>>
>> That way we can use it for tests that perform multiple fills in one submit 
>> etc.
>

[PATCH v2 libdrm 6/7] tegra: Add gr2d-fill test

[Erik Faye-Lund]

On Fri, May 2, 2014 at 4:25 PM, Thierry Reding  
wrote:
> On Thu, Apr 10, 2014 at 07:28:16PM +0200, Erik Faye-Lund wrote:
>> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding  
>> wrote:
>> > diff --git a/tests/tegra/gr2d-fill.c b/tests/tegra/gr2d-fill.c
>> > new file mode 100644
>> > index ..b6ba35a9d668
>> > --- /dev/null
>> > +++ b/tests/tegra/gr2d-fill.c
>> > @@ -0,0 +1,146 @@
>> > +/*
>> > + * Copyright ? 2014 NVIDIA Corporation
>> > + *
>> > + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
>> > + */
>> > +
>> > +#ifdef HAVE_CONFIG_H
>> > +#  include "config.h"
>> > +#endif
>> > +
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +#include 
>> > +
>> > +#include 
>> > +
>> > +#include "xf86drm.h"
>> > +#include "xf86drmMode.h"
>> > +#include "drm_fourcc.h"
>> > +
>> > +#include "drm-test-tegra.h"
>> > +#include "tegra.h"
>> > +
>> > +int main(int argc, char *argv[])
>> > +{
>> > +   uint32_t format = DRM_FORMAT_XRGB;
>> > +   struct drm_tegra_gr2d *gr2d;
>> > +   struct drm_framebuffer *fb;
>> > +   struct drm_screen *screen;
>> > +   unsigned int pitch, size;
>> > +   struct drm_tegra_bo *bo;
>> > +   struct drm_tegra *drm;
>> > +   uint32_t handle;
>> > +   int fd, err;
>> > +   void *ptr;
>> > +
>> > +   fd = drm_open(argv[1]);
>> > +   if (fd < 0) {
>> > +   fprintf(stderr, "failed to open DRM device %s: %s\n", 
>> > argv[1],
>> > +   strerror(errno));
>> > +   return 1;
>> > +   }
>>
>> I'm not quite sure I understand this part. Why would argv[1] be
>> anything else than NULL? Is this useful for manual debugging, perhaps?
>
> Yes. On newer Tegra generations the nouveau driver can create its own
> device files in /dev/dri and depending on device probe order, the card0
> device can be the wrong one. For such cases these test programs can be
> passed the path to the correct device via the command-line.
>
> Probably a better approach would be to search for a compatible device
> via udev matching or iterating over /dev/dri/card* and querying the
> driver name (which is what drm_tegra_new() already does). But that seems
> overkill currently. Perhaps when this turns into a more fully-fledged
> test suite that could be implemented more cleverly.
>
> For now I've addressed this by making the tests fallback to a default
> device (/dev/dri/card0) if no argument has been specified.
>

Thanks for the explanation, makes sense!

>> > +   err = drm_tegra_gr2d_fill(gr2d, fb, fb->width / 4, fb->height / 4,
>> > + fb->width / 2, fb->height / 2, 
>> > 0x);
>> > +   if (err < 0) {
>> > +   fprintf(stderr, "failed to fill rectangle: %s\n",
>> > +   strerror(-err));
>> > +   return 1;
>> > +   }
>> > +
>> > +   sleep(1);
>> > +
>>
>> Why do we need to sleep here?
>
> This is a visual test, so that one second gives me some time to see if
> the result looks as expected.
>
> I have tentative plans to implement a set of more automated tests, where
> an image is rendered and the checksum computed over the content can be
> compared with a known good reference checksum, but for now this is
> better than nothing.

Aha, makes sense. Thanks, looks good!

[PATCH v2 libdrm 1/7] configure: Support symbol visibility when available

[Erik Faye-Lund]

On Fri, May 2, 2014 at 4:12 PM, Thierry Reding  
wrote:
> On Thu, Apr 10, 2014 at 07:15:14PM +0200, Erik Faye-Lund wrote:
>> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding  
>> wrote:
>> > diff --git a/libdrm.h b/libdrm.h
>> > new file mode 100644
>> > index ..23926e6f6741
>> > --- /dev/null
>> > +++ b/libdrm.h
>> > @@ -0,0 +1,34 @@
>> > +/*
>> > + * Copyright ? 2014 NVIDIA Corporation
>> > + *
>> > + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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 LIBDRM_LIBDRM_H
>> > +#define LIBDRM_LIBDRM_H
>>
>> LIBDRM_LIBDRM_H sounds a bit clunky to me. Why LIBDRM twice? The other
>> headers don't seem to prefix LIBDRM_ to their header-guards. In fact,
>> many of them don't even have header-guards.
>
> This was with the intention of marking it as an internal header file. So
> the LIBDRM_ prefix could be used consistently for all files that are not
> installed. xf86atomic.h uses that prefix as well.

If you look at the history of xf86atomic.h, it seems this strange
header-guard is the result of a slightly careless replace. It used to
be called intel_atomics.h, and have INTEL_ATOMICS_H as the
header-guard. So I wouldn't lake that set too much of a precedence.

>> Also, does these macro really warrant making a top-level, generically
>> named header?
>
> There isn't really another header file where this would fit. Others are
> either installed (and therefore shouldn't be exposing these macros) or
> have a very specific purpose (xf86atomic.h).

I guess this is a matter of taste, and it would be great with some
input from the other libdrm-people on this. I don't care too much
either way...

[PATCH v2 libdrm 4/7] tegra: Add channel, job, pushbuf and fence APIs

[Erik Faye-Lund]

On Fri, May 2, 2014 at 4:06 PM, Thierry Reding  
wrote:
> On Thu, Apr 10, 2014 at 07:13:22PM +0200, Erik Faye-Lund wrote:
>>
>> But this raises the question, how is job->cmdbufs (and job->relocs)
>> supposed to get free'd?
>>
>> I'm a bit curious as to what the expected life-time of these objects
>> are. Do I need to create a new job-object for each submit, or can I
>> reuse a job? I think the latter would be preferable... So perhaps we
>> should have a drm_tegra_job_reset that allows us to throw away the
>> accumulated cmdbufs and relocs, and start building a new job?
>
> They are currently freed by drm_tegra_job_free(), so yes, the current
> intended usage is to create a new job for each submit. Do you mind if we
> keep your proposal for a drm_tegra_job_reset() in mind for a follow-up
> patch. It's an optimization that we can easily add later on and I'd like
> to avoid too much premature optimization at this point.

Sure, I don't mind.

>> > diff --git a/tegra/private.h b/tegra/private.h
>> > index 9b6bc9395d23..fc74fb56b58d 100644
>> > --- a/tegra/private.h
>> > +++ b/tegra/private.h
>> > @@ -26,13 +26,31 @@
>> >  #define __DRM_TEGRA_PRIVATE_H__ 1
>> >
>> >  #include 
>> > +#include 
>> >  #include 
>> >
>> >  #include 
>> > +#include 
>> >  #include 
>> >
>> > +#include "tegra_drm.h"
>> >  #include "tegra.h"
>> >
>> > +#define container_of(ptr, type, member) ({ \
>> > +   const typeof(((type *)0)->member) *__mptr = (ptr);  \
>> > +   (type *)((char *)__mptr - offsetof(type, member));  \
>> > +   })
>> > +
>> 
>> > +
>> > +struct drm_tegra_pushbuf_private {
>> > +   struct drm_tegra_pushbuf base;
>> > +   struct drm_tegra_job *job;
>> > +   drmMMListHead list;
>> > +   drmMMListHead bos;
>> > +
>> > +   struct drm_tegra_bo *bo;
>> > +   uint32_t *start;
>> > +   uint32_t *end;
>> > +};
>> > +
>> > +static inline struct drm_tegra_pushbuf_private *
>> > +pushbuf_priv(struct drm_tegra_pushbuf *pb)
>> > +{
>> > +   return container_of(pb, struct drm_tegra_pushbuf_private, base);
>> > +}
>> > +
>>
>> This seems to be the only use-case for container_of, and it just
>> happens to return the exact same pointer as was passed in... so do we
>> really need that helper?
>
> It has the benefit of keeping this code working even when somebody
> suddenly adds to the beginning of drm_tegra_pushbuf_private. I'd rather
> not resolve to force casting just to be on the safe side.

Is that a really a legitimate worry? There's a very clear relationship
between the two structures. Both nouveau and freedreno does the same
thing (for nouveau_bo_priv, nouveau_device_priv, msm_device, msm_pipe
and msm_bo structures), and it seems to work fine for them. Perhaps a
comment along the lines of "/* this needs to be the first member */"
is enough to discourage people from messing with it?

>> > diff --git a/tegra/pushbuf.c b/tegra/pushbuf.c
>> > new file mode 100644
>> > index ..178d5cd57541
>> > --- /dev/null
>> > +++ b/tegra/pushbuf.c
>> > @@ -0,0 +1,205 @@
>> 
>> > +drm_public
>> > +int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp,
>> > + struct drm_tegra_job *job)
>> > +{
>> > +   struct drm_tegra_pushbuf_private *pushbuf;
>> > +   void *ptr;
>> > +   int err;
>> > +
>> > +   pushbuf = calloc(1, sizeof(*pushbuf));
>> > +   if (!pushbuf)
>> > +   return -ENOMEM;
>> > +
>> > +   DRMINITLISTHEAD(>list);
>> > +   DRMINITLISTHEAD(>bos);
>> > +   pushbuf->job = job;
>> > +
>> > +   *pushbufp = >base;
>> > +
>> > +   DRMLISTADD(>list, >pushbufs);
>>
>> Hmm. So the job keeps a list of pushbufs. What purpose does this
>> serve? Shouldn't the job only need the cmdbuf-list (which it already
>> has) and the BOs (so they can be dereferenced after being submitted)?
>
> This is currently used to keep track of existing pushbuffers and make
> sure that they are freed (when the job is freed).
>

OK, it seems to me that the need for keeping the pushbuffers around is
simply not there. Only the BOs needs to be kept around, no?

>> AFAICT, we could make drm_te

[PATCH v2 libdrm 3/7] tegra: Add simple test for drm_tegra_open()

[Erik Faye-Lund]

On Thu, Apr 10, 2014 at 7:20 PM, Erik Faye-Lund  wrote:
> On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding  
> wrote:
>> From: Thierry Reding 
>>
>> This test opens a device, dumps the version information and checks that
>> a Tegra DRM context can be opened on it.
>>
>> Signed-off-by: Thierry Reding 
>
> Looks good!

Actually, "make check" doesn't work here:

kusma at localhost:~/src/libdrm$ make -C tests/tegra/ check
make: Entering directory `/home/kusma/src/libdrm/tests/tegra'
make  openclose gr2d-fill
make[1]: Entering directory `/home/kusma/src/libdrm/tests/tegra'
make[1]: `openclose' is up to date.
make[1]: `gr2d-fill' is up to date.
make[1]: Leaving directory `/home/kusma/src/libdrm/tests/tegra'
make  check-TESTS
make[1]: Entering directory `/home/kusma/src/libdrm/tests/tegra'
FAIL: openclose
failed to open DRM device (null): Bad address
FAIL: gr2d-fill
===
2 of 2 tests failed
Please report to https://bugs.freedesktop.org/enter_bug.cgi?product=DRI
===
make[1]: *** [check-TESTS] Error 1
make[1]: Leaving directory `/home/kusma/src/libdrm/tests/tegra'
make: *** [check-am] Error 2
make: Leaving directory `/home/kusma/src/libdrm/tests/tegra'

[PATCH v2 libdrm 5/7] tegra: Add helper library for tests

[Erik Faye-Lund]

On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding  
wrote:
> From: Thierry Reding 
>
> This library provides helpers for common functionality needed by test
> programs.
>
> Signed-off-by: Thierry Reding 
> ---
> Changes in v2:
> - fix a couple of memory leaks and get rid of some unneeded code
>
>  tests/tegra/Makefile.am  |  10 +-
>  tests/tegra/drm-test-tegra.c | 137 
>  tests/tegra/drm-test-tegra.h |  55 ++
>  tests/tegra/drm-test.c   | 248 
> +++
>  tests/tegra/drm-test.h   |  72 +
>  5 files changed, 521 insertions(+), 1 deletion(-)
>  create mode 100644 tests/tegra/drm-test-tegra.c
>  create mode 100644 tests/tegra/drm-test-tegra.h
>  create mode 100644 tests/tegra/drm-test.c
>  create mode 100644 tests/tegra/drm-test.h
>
> diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
> index 8b481bde4f11..e468029d152e 100644
> --- a/tests/tegra/Makefile.am
> +++ b/tests/tegra/Makefile.am
> @@ -5,9 +5,17 @@ AM_CPPFLAGS = \
>
>  AM_CFLAGS = -Wall -Werror
>
> +noinst_LTLIBRARIES = libdrm-test.la
> +libdrm_test_la_SOURCES = \
> +   drm-test.c \
> +   drm-test.h \
> +   drm-test-tegra.c \
> +   drm-test-tegra.h
> +
>  LDADD = \
> ../../tegra/libdrm_tegra.la \
> -   ../../libdrm.la
> +   ../../libdrm.la \
> +   libdrm-test.la
>

Hmm, I need the following on top to please the linker:

diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
index 286af4b..88230c0 100644
--- a/tests/tegra/Makefile.am
+++ b/tests/tegra/Makefile.am
@@ -14,8 +14,8 @@ libdrm_test_la_SOURCES = \

 LDADD = \
../../tegra/libdrm_tegra.la \
-   ../../libdrm.la \
-   libdrm-test.la
+   libdrm-test.la \
+   ../../libdrm.la

 TESTS = \
openclose \

[PATCH v2 libdrm 5/7] tegra: Add helper library for tests

[Erik Faye-Lund]

On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding  
wrote:
> From: Thierry Reding 
>
> This library provides helpers for common functionality needed by test
> programs.
>
> Signed-off-by: Thierry Reding 
> ---
> Changes in v2:
> - fix a couple of memory leaks and get rid of some unneeded code
>
>  tests/tegra/Makefile.am  |  10 +-
>  tests/tegra/drm-test-tegra.c | 137 
>  tests/tegra/drm-test-tegra.h |  55 ++
>  tests/tegra/drm-test.c   | 248 
> +++
>  tests/tegra/drm-test.h   |  72 +
>  5 files changed, 521 insertions(+), 1 deletion(-)
>  create mode 100644 tests/tegra/drm-test-tegra.c
>  create mode 100644 tests/tegra/drm-test-tegra.h
>  create mode 100644 tests/tegra/drm-test.c
>  create mode 100644 tests/tegra/drm-test.h

This isn't really important at this point, but it looks to me like
tests/tegra/drm-test.[ch] isn't really tegra-specific. If so, perhaps
some other tests can benefit from it? Doing so is of course something
whoever writes those tests should do, though. Leaving them in the
tegra-subdir is probably fine.

> +int drm_tegra_gr2d_fill(struct drm_tegra_gr2d *gr2d, struct drm_framebuffer 
> *fb,
> +   unsigned int x, unsigned int y, unsigned int width,
> +   unsigned int height, uint32_t color)
> +{
> +   struct drm_tegra_bo *fbo = fb->data;
> +   struct drm_tegra_pushbuf *pushbuf;
> +   struct drm_tegra_fence *fence;
> +   struct drm_tegra_job *job;
> +   int err;
> +
> +   err = drm_tegra_job_new(, gr2d->channel);
> +   if (err < 0)
> +   return err;
> +
> +   err = drm_tegra_pushbuf_new(, job);
> +   if (err < 0)
> +   return err;
> +

I think this helper would be generally more useful if it skipped the
above two, and required the call-sites to do them instead.

> +   err = drm_tegra_pushbuf_prepare(pushbuf, 32);
> +   if (err < 0)
> +   return err;
> +
> +   *pushbuf->ptr++ = HOST1X_OPCODE_SETCL(0, HOST1X_CLASS_GR2D, 0);
> +
> +   *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x9, 0x9);
> +   *pushbuf->ptr++ = 0x003a;
> +   *pushbuf->ptr++ = 0x;
> +
> +   *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x1e, 0x7);
> +   *pushbuf->ptr++ = 0x;
> +   *pushbuf->ptr++ = (2 << 16) | (1 << 6) | (1 << 2);
> +   *pushbuf->ptr++ = 0x00cc;
> +
> +   *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x2b, 0x9);
> +
> +   /* relocate destination buffer */
> +   err = drm_tegra_pushbuf_relocate(pushbuf, fbo, 0, 0);
> +   if (err < 0) {
> +   fprintf(stderr, "failed to relocate buffer object: %d\n", 
> err);
> +   return err;
> +   }
> +
> +   *pushbuf->ptr++ = fb->pitch;
> +
> +   *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x35, 1);
> +   *pushbuf->ptr++ = color;
> +
> +   *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x46, 1);
> +   *pushbuf->ptr++ = 0x;
> +
> +   *pushbuf->ptr++ = HOST1X_OPCODE_MASK(0x38, 0x5);
> +   *pushbuf->ptr++ = height << 16 | width;
> +   *pushbuf->ptr++ = y << 16 | x;
> +

...and stop here.

That way we can use it for tests that perform multiple fills in one submit etc.

> +#define HOST1X_OPCODE_SETCL(offset, classid, mask) \
> +((0x0 << 28) | (((offset) & 0xfff) << 16) | (((classid) & 0x3ff) << 6) | 
> ((mask) & 0x3f))
> +#define HOST1X_OPCODE_INCR(offset, count) \
> +((0x1 << 28) | (((offset) & 0xfff) << 16) | ((count) & 0x))
> +#define HOST1X_OPCODE_NONINCR(offset, count) \
> +((0x2 << 28) | (((offset) & 0xfff) << 16) | ((count) & 0x))
> +#define HOST1X_OPCODE_MASK(offset, mask) \
> +((0x3 << 28) | (((offset) & 0xfff) << 16) | ((mask) & 0x))
> +#define HOST1X_OPCODE_IMM(offset, data) \
> +((0x4 << 28) | (((offset) & 0xfff) << 16) | ((data) & 0x))
> +#define HOST1X_OPCODE_EXTEND(subop, value) \
> +((0xe << 28) | (((subop) & 0xf) << 24) | ((value) & 0xff))
> +
> +#define HOST1X_CLASS_GR2D 0x51

Hmm, shouldn't these be available from somewhere else already? No
point in repeating the same macros over and over again, no?

> diff --git a/tests/tegra/drm-test.c b/tests/tegra/drm-test.c
> new file mode 100644
> index ..1f91d17f61bb
> --- /dev/null
> +++ b/tests/tegra/drm-test.c
> @@ -0,0 +1,248 @@
> +/*
> + * Copyright ? 2014 NVIDIA Corporation
> + *
> + * 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 shall be included in
> + * all copies or 

[PATCH v2 libdrm 6/7] tegra: Add gr2d-fill test

[Erik Faye-Lund]

On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding  
wrote:
> diff --git a/tests/tegra/gr2d-fill.c b/tests/tegra/gr2d-fill.c
> new file mode 100644
> index ..b6ba35a9d668
> --- /dev/null
> +++ b/tests/tegra/gr2d-fill.c
> @@ -0,0 +1,146 @@
> +/*
> + * Copyright ? 2014 NVIDIA Corporation
> + *
> + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#  include "config.h"
> +#endif
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "xf86drm.h"
> +#include "xf86drmMode.h"
> +#include "drm_fourcc.h"
> +
> +#include "drm-test-tegra.h"
> +#include "tegra.h"
> +
> +int main(int argc, char *argv[])
> +{
> +   uint32_t format = DRM_FORMAT_XRGB;
> +   struct drm_tegra_gr2d *gr2d;
> +   struct drm_framebuffer *fb;
> +   struct drm_screen *screen;
> +   unsigned int pitch, size;
> +   struct drm_tegra_bo *bo;
> +   struct drm_tegra *drm;
> +   uint32_t handle;
> +   int fd, err;
> +   void *ptr;
> +
> +   fd = drm_open(argv[1]);
> +   if (fd < 0) {
> +   fprintf(stderr, "failed to open DRM device %s: %s\n", argv[1],
> +   strerror(errno));
> +   return 1;
> +   }

I'm not quite sure I understand this part. Why would argv[1] be
anything else than NULL? Is this useful for manual debugging, perhaps?

> +   err = drm_tegra_gr2d_fill(gr2d, fb, fb->width / 4, fb->height / 4,
> + fb->width / 2, fb->height / 2, 0x);
> +   if (err < 0) {
> +   fprintf(stderr, "failed to fill rectangle: %s\n",
> +   strerror(-err));
> +   return 1;
> +   }
> +
> +   sleep(1);
> +

Why do we need to sleep here?

[PATCH v2 libdrm 3/7] tegra: Add simple test for drm_tegra_open()

[Erik Faye-Lund]

On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding  
wrote:
> From: Thierry Reding 
>
> This test opens a device, dumps the version information and checks that
> a Tegra DRM context can be opened on it.
>
> Signed-off-by: Thierry Reding 

Looks good!

[PATCH v2 libdrm 2/7] libdrm: Add NVIDIA Tegra support

[Erik Faye-Lund]

On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding  
wrote:
> From: Thierry Reding 
>
> Add the libdrm_tegra helper library to encapsulate Tegra-specific
> interfaces to the DRM.
>
> Furthermore, Tegra is added to the list of supported chips in the
> modetest and vbltest programs.
>
> Signed-off-by: Thierry Reding 
> Signed-off-by: Erik Faye-Lund 
> Signed-off-by: Thierry Reding 

Looks good to me!

[PATCH v2 libdrm 1/7] configure: Support symbol visibility when available

[Erik Faye-Lund]

On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding  
wrote:
> diff --git a/libdrm.h b/libdrm.h
> new file mode 100644
> index ..23926e6f6741
> --- /dev/null
> +++ b/libdrm.h
> @@ -0,0 +1,34 @@
> +/*
> + * Copyright ? 2014 NVIDIA Corporation
> + *
> + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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 LIBDRM_LIBDRM_H
> +#define LIBDRM_LIBDRM_H

LIBDRM_LIBDRM_H sounds a bit clunky to me. Why LIBDRM twice? The other
headers don't seem to prefix LIBDRM_ to their header-guards. In fact,
many of them don't even have header-guards.

Also, does these macro really warrant making a top-level, generically
named header?

[PATCH v2 libdrm 4/7] tegra: Add channel, job, pushbuf and fence APIs

[Erik Faye-Lund]

On Wed, Apr 9, 2014 at 1:40 PM, Thierry Reding  
wrote:
> diff --git a/tegra/fence.c b/tegra/fence.c
> new file mode 100644
> index ..f58725ca8472
> --- /dev/null
> +++ b/tegra/fence.c
> +drm_public
> +int drm_tegra_fence_wait(struct drm_tegra_fence *fence)
> +{
> +   return drm_tegra_fence_wait_timeout(fence, -1);
> +}

Perhaps a convenience-wrapper like this should be inline in the header
to reduce function-call overhead?

> +drm_public
> +int drm_tegra_job_submit(struct drm_tegra_job *job,
> +struct drm_tegra_fence **fencep)
> +{
> +   struct drm_tegra *drm = job->channel->drm;
> +   struct drm_tegra_pushbuf_private *pushbuf;
> +   struct drm_tegra_fence *fence = NULL;
> +   struct drm_tegra_cmdbuf *cmdbufs;
> +   struct drm_tegra_syncpt *syncpts;
> +   struct drm_tegra_submit args;
> +   unsigned int i;
> +   int err;
> +
> +   /*
> +* Make sure the current command stream buffer is queued for
> +* submission.
> +*/
> +   err = drm_tegra_pushbuf_queue(job->pushbuf);
> +   if (err < 0)
> +   return err;
> +
> +   job->pushbuf = NULL;
> +
> +   if (fencep) {
> +   fence = calloc(1, sizeof(*fence));
> +   if (!fence)
> +   return -ENOMEM;
> +   }
> +
> +   syncpts = calloc(1, sizeof(*syncpts));
> +   if (!syncpts) {
> +   free(cmdbufs);

cmdbufs never gets assigned to, yet it gets free'd here (and a few
more places further down). I guess this is left-overs from the
previous round that should just die?

But this raises the question, how is job->cmdbufs (and job->relocs)
supposed to get free'd?

I'm a bit curious as to what the expected life-time of these objects
are. Do I need to create a new job-object for each submit, or can I
reuse a job? I think the latter would be preferable... So perhaps we
should have a drm_tegra_job_reset that allows us to throw away the
accumulated cmdbufs and relocs, and start building a new job?

> diff --git a/tegra/private.h b/tegra/private.h
> index 9b6bc9395d23..fc74fb56b58d 100644
> --- a/tegra/private.h
> +++ b/tegra/private.h
> @@ -26,13 +26,31 @@
>  #define __DRM_TEGRA_PRIVATE_H__ 1
>
>  #include 
> +#include 
>  #include 
>
>  #include 
> +#include 
>  #include 
>
> +#include "tegra_drm.h"
>  #include "tegra.h"
>
> +#define container_of(ptr, type, member) ({ \
> +   const typeof(((type *)0)->member) *__mptr = (ptr);  \
> +   (type *)((char *)__mptr - offsetof(type, member));  \
> +   })
> +

> +
> +struct drm_tegra_pushbuf_private {
> +   struct drm_tegra_pushbuf base;
> +   struct drm_tegra_job *job;
> +   drmMMListHead list;
> +   drmMMListHead bos;
> +
> +   struct drm_tegra_bo *bo;
> +   uint32_t *start;
> +   uint32_t *end;
> +};
> +
> +static inline struct drm_tegra_pushbuf_private *
> +pushbuf_priv(struct drm_tegra_pushbuf *pb)
> +{
> +   return container_of(pb, struct drm_tegra_pushbuf_private, base);
> +}
> +

This seems to be the only use-case for container_of, and it just
happens to return the exact same pointer as was passed in... so do we
really need that helper?

> diff --git a/tegra/pushbuf.c b/tegra/pushbuf.c
> new file mode 100644
> index ..178d5cd57541
> --- /dev/null
> +++ b/tegra/pushbuf.c
> @@ -0,0 +1,205 @@

> +drm_public
> +int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp,
> + struct drm_tegra_job *job)
> +{
> +   struct drm_tegra_pushbuf_private *pushbuf;
> +   void *ptr;
> +   int err;
> +
> +   pushbuf = calloc(1, sizeof(*pushbuf));
> +   if (!pushbuf)
> +   return -ENOMEM;
> +
> +   DRMINITLISTHEAD(>list);
> +   DRMINITLISTHEAD(>bos);
> +   pushbuf->job = job;
> +
> +   *pushbufp = >base;
> +
> +   DRMLISTADD(>list, >pushbufs);

Hmm. So the job keeps a list of pushbufs. What purpose does this
serve? Shouldn't the job only need the cmdbuf-list (which it already
has) and the BOs (so they can be dereferenced after being submitted)?

AFAICT, we could make drm_tegra_pushbuf_queue append the BO to a list
in the job instead. That way we don't need to keep all the
pushbuf-objects around, and we might even be able to reuse the same
object rather than keep reallocating them. No?

> +drm_public
> +int drm_tegra_pushbuf_sync(struct drm_tegra_pushbuf *pushbuf,
> +  enum drm_tegra_syncpt_cond cond)
> +{
> +   struct drm_tegra_pushbuf_private *priv = pushbuf_priv(pushbuf);
> +
> +   if (cond >= DRM_TEGRA_SYNCPT_COND_MAX)
> +   return -EINVAL;
> +
> +   *pushbuf->ptr++ = HOST1X_OPCODE_NONINCR(0x0, 0x1);
> +   *pushbuf->ptr++ = cond << 8 | priv->job->syncpt;

Perhaps "HOST1X_OPCODE_IMM(0x0, cond << 8 | priv->job->syncpt)", which
saves a word?

Either way, I think this should either call drm_tegra_pushbuf_prepare
to make 

[RFC libdrm 0/6] Add NVIDIA Tegra support

[Erik Faye-Lund]

On Thu, Mar 20, 2014 at 4:23 PM, Erik Faye-Lund  wrote:
> On Wed, Feb 19, 2014 at 5:04 PM, Thierry Reding
>  wrote:
>> From: Thierry Reding 
>>
>> Hi,
>>
>> This series adds libdrm-tegra with a very lightweight API on top of the
>> kernel interfaces. Most of the functions provided here have been in use
>> in various driver efforts in different incarnations. This is an attempt
>> to consolidate, so I'm looking for review comments especially by Erik
>> to ensure it can be used by grate.
>
> Is there anything I can do to move this series along?

I'll ask again... Is there something I can do to help move this along?

[RFC libdrm 0/6] Add NVIDIA Tegra support

[Erik Faye-Lund]

On Wed, Feb 19, 2014 at 5:04 PM, Thierry Reding
 wrote:
> From: Thierry Reding 
>
> Hi,
>
> This series adds libdrm-tegra with a very lightweight API on top of the
> kernel interfaces. Most of the functions provided here have been in use
> in various driver efforts in different incarnations. This is an attempt
> to consolidate, so I'm looking for review comments especially by Erik
> to ensure it can be used by grate.

Is there anything I can do to move this series along?

[RFC libdrm 3/6] tegra: Add simple test for drm_tegra_open()

[Erik Faye-Lund]

On Wed, Feb 19, 2014 at 5:04 PM, Thierry Reding
 wrote:
> From: Thierry Reding 
>
> This test opens a device, dumps the version information and checks that
> a Tegra DRM context can be opened on it.
>
> Signed-off-by: Thierry Reding 
> ---
>  configure.ac|  1 +
>  tests/Makefile.am   |  4 
>  tests/tegra/Makefile.am | 20 
>  tests/tegra/openclose.c | 63 
> +
>  4 files changed, 88 insertions(+)
>  create mode 100644 tests/tegra/Makefile.am
>  create mode 100644 tests/tegra/openclose.c
>
> diff --git a/configure.ac b/configure.ac
> index 752a70592933..c4ae14e8d2e1 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -421,6 +421,7 @@ AC_CONFIG_FILES([
> tests/radeon/Makefile
> tests/vbltest/Makefile
> tests/exynos/Makefile
> +   tests/tegra/Makefile
> include/Makefile
> include/drm/Makefile
> man/Makefile
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index cd1149130214..0a3d21f2d99f 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -24,6 +24,10 @@ if HAVE_EXYNOS
>  SUBDIRS += exynos
>  endif
>
> +if HAVE_TEGRA
> +SUBDIRS += tegra
> +endif
> +
>  if HAVE_LIBUDEV
>
>  check_LTLIBRARIES = libdrmtest.la
> diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
> new file mode 100644
> index ..7039f09d38aa
> --- /dev/null
> +++ b/tests/tegra/Makefile.am
> @@ -0,0 +1,20 @@
> +AM_CPPFLAGS = \
> +   -I$(top_srcdir)/include/drm \
> +   -I$(top_srcdir)/tegra
> +
> +AM_CFLAGS = -Wall -Werror
> +
> +LDADD = \
> +   ../../tegra/libdrm_tegra.la
> +
> +TESTS = \
> +   openclose \
> +
> +if HAVE_INSTALL_TESTS
> +testdir = $(libexecdir)/libdrm/tests/tegra
> +test_PROGRAMS = \
> +   $(TESTS)
> +else
> +noinst_PROGRAMS = $(TESTS)
> +check_PROGRAMS = $(TESTS)
> +endif

You should probably add openclose to .gitignore also. The same goes
for gr2d-fill in the other commit.

[RFC libdrm 1/6] configure: Support symbol visibility when available

[Erik Faye-Lund]

On Wed, Feb 19, 2014 at 5:04 PM, Thierry Reding
 wrote:
> From: Thierry Reding 
>
> Checks whether or not the compiler supports the -fvisibility option. If
> so it sets the VISIBILITY_CFLAGS variable which can be added to the per
> directory AM_CFLAGS where appropriate.
>
> Libraries can use the HAVE_VISIBILITY preprocessor definition to check
> for availability and use something like this:
>
> #if defined(HAVE_VISIBILITY)
> #  define drm_private __attribute__((visibility("hidden")))
> #  define drm_public __attribute__((visibility("default")))
> #else
> #  define drm_private
> #  define drm_public
> #endif
>
> By default all symbols will be hidden via the VISIBILITY_CFLAGS. Only
> symbols explicitly marked drm_public will be exported.

As I said in the other patch, I think it makes more sense to define
these globally. The other drivers still need to opt-in on the feature,
but it's less work, less duplication of logic and less
points-of-failure ;)

[RFC libdrm 6/6] tegra: Add gr2d-fill test

[Erik Faye-Lund]

On Wed, Feb 19, 2014 at 5:04 PM, Thierry Reding
 wrote:
> From: Thierry Reding 
>
> This test uses the IOCTLs for job submission and fences to fill a sub-
> region of the screen to a specific color using gr2d.
>
> Signed-off-by: Thierry Reding 
> ---
>  tests/tegra/Makefile.am |   1 +
>  tests/tegra/gr2d-fill.c | 145 
> 
>  2 files changed, 146 insertions(+)
>  create mode 100644 tests/tegra/gr2d-fill.c
>
> diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
> index 90b11b278a42..9e9e75e91ad4 100644
> --- a/tests/tegra/Makefile.am
> +++ b/tests/tegra/Makefile.am
> @@ -17,6 +17,7 @@ LDADD = \
>
>  TESTS = \
> openclose \
> +   gr2d-fill
>
>  if HAVE_INSTALL_TESTS
>  testdir = $(libexecdir)/libdrm/tests/tegra
> diff --git a/tests/tegra/gr2d-fill.c b/tests/tegra/gr2d-fill.c
> new file mode 100644
> index ..7ffe6f0b0848
> --- /dev/null
> +++ b/tests/tegra/gr2d-fill.c
> @@ -0,0 +1,145 @@
> +/*
> + * Copyright ? 2014 NVIDIA Corporation
> + *
> + * 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 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 COPYRIGHT HOLDER(S) OR AUTHOR(S) 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.
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#  include "config.h"
> +#endif
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "xf86drm.h"
> +#include "xf86drmMode.h"
> +#include "drm_fourcc.h"
> +
> +#include "drm-test-tegra.h"
> +#include "tegra.h"
> +
> +int main(int argc, char *argv[])
> +{
> +   uint32_t format = DRM_FORMAT_XRGB;
> +   struct drm_tegra_gr2d *gr2d;
> +   struct drm_framebuffer *fb;
> +   struct drm_screen *screen;
> +   unsigned int pitch, size;
> +   struct drm_tegra_bo *bo;
> +   struct drm_tegra *drm;
> +   uint32_t handle;
> +   int fd, err;
> +   void *ptr;
> +
> +   fd = drm_open(argv[1]);
> +   if (fd < 0) {
> +   fprintf(stderr, "failed to open DRM device %s: %s\n", argv[1],
> +   strerror(errno));
> +   return 1;
> +   }
> +
> +   err = drm_screen_open(, fd);
> +   if (err < 0) {
> +   fprintf(stderr, "failed to open screen: %s\n", 
> strerror(-err));
> +   return 1;
> +   }
> +
> +   err = drm_tegra_new(, fd);
> +   if (err < 0) {
> +   fprintf(stderr, "failed to create Tegra DRM context: %s\n",
> +   strerror(-err));
> +   return 1;
> +   }
> +
> +   err = drm_tegra_gr2d_open(, drm);
> +   if (err < 0) {
> +   fprintf(stderr, "failed to open gr2d channel: %s\n",
> +   strerror(-err));
> +   return 1;
> +   }
> +
> +   pitch = screen->width * screen->bpp / 8;
> +   size = pitch * screen->height;
> +
> +   err = drm_tegra_bo_new(, drm, 0, size);
> +   if (err < 0) {
> +   fprintf(stderr, "failed to create buffer object: %s\n",
> +   strerror(-err));
> +   return 1;
> +   }
> +
> +   err = drm_tegra_bo_get_handle(bo, );
> +   if (err < 0) {
> +   fprintf(stderr, "failed to get handle to buffer object: %s\n",
> +   strerror(-err));
> +   return 1;
> +   }
> +
> +   err = drm_tegra_bo_map(bo, );
> +   if (err < 0) {
> +   fprintf(stderr, "failed to map buffer object: %s\n",
> +   strerror(-err));
> +   return 1;
> +   }
> +
> +   memset(ptr, 0xff, size);
> +
> +   err = drm_framebuffer_new(, screen, handle, screen->width,
> + screen->height, pitch, format, bo);
> +   if (err < 0) {
> +   fprintf(stderr, "failed to create framebuffer: %s\n",
> +   strerror(-err));
> +   return 1;
> +   }
> +
> +   err = drm_screen_set_framebuffer(screen, fb);
> +   if (err < 0) {
> 

[RFC libdrm 5/6] tegra: Add helper library for tests

[Erik Faye-Lund]

On Wed, Feb 19, 2014 at 5:04 PM, Thierry Reding
 wrote:
> diff --git a/tests/tegra/drm-test-tegra.h b/tests/tegra/drm-test-tegra.h
> new file mode 100644
> index ..d1cb6b1ee440
> --- /dev/null
> +++ b/tests/tegra/drm-test-tegra.h
> +int drm_open(const char *path)
> +{
> +   int fd, err;
> +
> +   fd = open(path, O_RDWR);
> +   if (fd < 0)
> +   return -errno;
> +
> +   err = drmSetMaster(fd);

Hmmpf, do we really need modesetting for all tests? Can't tests opt-in
on that instead?

[RFC libdrm 3/6] tegra: Add simple test for drm_tegra_open()

[Erik Faye-Lund]

On Wed, Feb 19, 2014 at 9:19 PM, Erik Faye-Lund  wrote:
> On Wed, Feb 19, 2014 at 5:04 PM, Thierry Reding
>  wrote:
>> diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
>> new file mode 100644
>> index ..7039f09d38aa
>> --- /dev/null
>> +++ b/tests/tegra/Makefile.am
>> @@ -0,0 +1,20 @@
>> +AM_CPPFLAGS = \
>> +   -I$(top_srcdir)/include/drm \
>> +   -I$(top_srcdir)/tegra
>> +
>> +AM_CFLAGS = -Wall -Werror
>> +
>> +LDADD = \
>> +   ../../tegra/libdrm_tegra.la
>
> You also need to add ../../libdrm.la here.

But even so, "make check" fails without any explanation.

However, gr2d-fill also fails. But with an error: "failed to open DRM
device (null): Bad address". Judging from the source-code, it seems
they expect arguments that "make check" doesn't provide.

[RFC libdrm 3/6] tegra: Add simple test for drm_tegra_open()

[Erik Faye-Lund]

On Wed, Feb 19, 2014 at 5:04 PM, Thierry Reding
 wrote:
> diff --git a/tests/tegra/Makefile.am b/tests/tegra/Makefile.am
> new file mode 100644
> index ..7039f09d38aa
> --- /dev/null
> +++ b/tests/tegra/Makefile.am
> @@ -0,0 +1,20 @@
> +AM_CPPFLAGS = \
> +   -I$(top_srcdir)/include/drm \
> +   -I$(top_srcdir)/tegra
> +
> +AM_CFLAGS = -Wall -Werror
> +
> +LDADD = \
> +   ../../tegra/libdrm_tegra.la

You also need to add ../../libdrm.la here.

[RFC libdrm 2/6] libdrm: Add NVIDIA Tegra support

[Erik Faye-Lund]

On Wed, Feb 19, 2014 at 5:04 PM, Thierry Reding
 wrote:
> diff --git a/tegra/Makefile.am b/tegra/Makefile.am
> new file mode 100644
> index ..1b83145b120d
> --- /dev/null
> +++ b/tegra/Makefile.am
> @@ -0,0 +1,20 @@
> +AM_CPPFLAGS = \
> +   -I$(top_srcdir) \
> +   -I$(top_srcdir)/include/drm
> +
> +AM_CFLAGS = \
> +   $(VISIBILITY_CFLAGS)
> +
> +libdrm_tegra_ladir = $(libdir)
> +libdrm_tegra_la_LTLIBRARIES = libdrm_tegra.la
> +libdrm_tegra_la_LDFLAGS = -version-number 0:0:0 -no-undefined
> +libdrm_tegra_la_LIBADD = ../libdrm.la @PTHREADSTUBS_LIBS@
> +
> +libdrm_tegra_la_SOURCES = \
> +   tegra.c
> +
> +libdrm_tegraincludedir = ${includedir}/libdrm
> +libdrm_tegrainclude_HEADERS = tegra.h
> +
> +pkgconfigdir = @pkgconfigdir@
> +pkgconfig_DATA = libdrm_tegra.pc

You should probably also add libdrm_tegra.pc to .gitignore also.

[RFC libdrm 2/6] libdrm: Add NVIDIA Tegra support

[Erik Faye-Lund]

On Wed, Feb 19, 2014 at 5:04 PM, Thierry Reding
 wrote:
> +#ifndef __DRM_TEGRA_PRIVATE_H__
> +#define __DRM_TEGRA_PRIVATE_H__ 1
> +
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "tegra.h"
> +
> +#if defined(HAVE_VISIBILITY)
> +#  define drm_private __attribute__((visibility("hidden")))
> +#  define drm_public __attribute__((visibility("default")))
> +#else
> +#  define drm_private
> +#  define drm_public
> +#endif
> +

Perhaps you could put this where it's visible to other drivers as
well, like xf86drm.h?

[RFC libdrm 4/6] tegra: Add channel, job, pushbuf and fence APIs

[Erik Faye-Lund]

On Wed, Feb 19, 2014 at 5:04 PM, Thierry Reding
 wrote:
> From: Thierry Reding 
>
> These functions can be used to open channels to engines, manage job
> submissions, create push buffers to store command streams in and wait
> until jobs have been completed.
>
> Signed-off-by: Thierry Reding 

Thanks a lot for doing this! I'm going right for the juicy patch ;)

> +drm_public
> +int drm_tegra_fence_wait_timeout(struct drm_tegra_fence *fence,
> +unsigned long timeout)
> +{
> +   struct drm_tegra_syncpt_wait args;
> +   int err;
> +
> +   memset(, 0, sizeof(args));

Nit: how about

struct drm_tegra_syncpt_wait args = { 0 };

instead?

> +   args.id = fence->syncpt;
> +   args.thresh = fence->value;
> +   args.timeout = timeout;
> +
> +   while (true) {
> +   err = ioctl(fence->drm->fd, DRM_IOCTL_TEGRA_SYNCPT_WAIT, 
> );
> +   if (err < 0) {
> +   if (errno == EINTR)
> +   continue;
> +
> +   drmMsg("DRM_IOCTL_TEGRA_SYNCPT_WAIT: %d\n", -errno);

What's the reason for printing the errno negated? And could we do
'...%s\n" strerror(errno));' instead?

> +int drm_tegra_job_add_reloc(struct drm_tegra_job *job,
> +   const struct drm_tegra_reloc *reloc)
> +{
> +   struct drm_tegra_reloc *relocs;
> +   size_t size;
> +
> +   size = (job->num_relocs + 1) * sizeof(*reloc);
> +
> +   relocs = realloc(job->relocs, size);

Nit: there's no point in not assigning those while declaring them, no?

size_t size = (job->num_relocs + 1) * sizeof(*reloc);
struct drm_tegra_reloc *relocs; = realloc(job->relocs, size);

> +drm_public
> +int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp,
> + struct drm_tegra_job *job,
> + struct drm_tegra_bo *bo,
> + unsigned long offset)
> +{
> +   struct drm_tegra_pushbuf_private *pushbuf;
> +   void *ptr;
> +   int err;
> +
> +   pushbuf = calloc(1, sizeof(*pushbuf));
> +   if (!pushbuf)
> +   return -ENOMEM;
> +
> +   pushbuf->bo = drm_tegra_bo_get(bo);
> +   DRMINITLISTHEAD(>list);
> +   pushbuf->job = job;
> +
> +   err = drm_tegra_bo_map(bo, );
> +   if (err < 0) {
> +   drm_tegra_bo_put(bo);
> +   free(pushbuf);
> +   return err;
> +   }
> +
> +   pushbuf->start = pushbuf->base.ptr = ptr + offset;
> +   pushbuf->offset = offset;
> +
> +   DRMLISTADD(>list, >pushbufs);
> +   job->num_pushbufs++;
> +
> +   *pushbufp = >base;
> +
> +   return 0;
> +}

It feels quite wasteful to me to have to allocate a new pushbuf in
order to be able to use a new BO. I'd much rather see the pushbuf
being a persisting object that's the interface to the command-stream
(that produces jobs).

I was thinking something like:

int drm_tegra_pushbuf_new(struct drm_tegra_pushbuf **pushbufp, struct
drm_tegra_job *job)
int drm_tegra_pushbuf_room(struct drm_tegra_pushbuf *pushbuf, int num_words);

Where room guarantees that there's space for those words in the
pushbuf. A simple implementation could just allocate a bo of that
size, but a slightly more sophisticated one can allocate larger ones
and reuse them. Even more sophisticated ones could keep old cmdbufs
around and reuse them once the hardware is done reading them, do
exponential grow-factors etc.

I've implemented the "slightly more sophisticated" approach here:

https://github.com/grate-driver/libdrm/commit/f90ea2f57ca4d8c81768402900c663ce526bac11

In my implementation, I've changed the job-structure to build the list
of cmdbufs directly rather than keeping a list of the pushbufs. Sure,
that means another allocation every time we need a new cmdbuf, but
hopefully we should be able to produce much less of them this way.

> +int drm_tegra_pushbuf_relocate(struct drm_tegra_pushbuf *pushbuf,
> +  struct drm_tegra_bo *target,
> +  unsigned long offset,
> +  unsigned long shift)
> +{
> +   struct drm_tegra_pushbuf_private *priv = pushbuf_priv(pushbuf);
> +   struct drm_tegra_reloc reloc;
> +   int err;
> +
> +   memset(, 0, sizeof(reloc));
> +   reloc.cmdbuf.handle = priv->bo->handle;
> +   reloc.cmdbuf.offset = drm_tegra_pushbuf_get_offset(pushbuf);
> +   reloc.target.handle = target->handle;
> +   reloc.target.offset = offset;
> +   reloc.shift = shift;
> +
> +   err = drm_tegra_job_add_reloc(priv->job, );
> +   if (err < 0)
> +   return err;
> +
> +   return 0;
> +}

Whenever we insert a reloc, we also insert a DEADBEEF in the command
stream. Why not formalize this into this function?

[PATCH 1/1] drm/tegra: Add guard to avoid double disable/enable of RGB outputs

[Erik Faye-Lund]

On Tue, Feb 11, 2014 at 6:12 PM, Dmitry Osipenko  wrote:
> Add guard to check whether RGB output is already enabled in the way it's
> done for HDMI output. Fixes possible hang on trying to disable output twice
> (first time during driver probe and second on fb registering).
>
> Signed-off-by: Dmitry Osipenko 
> ---
>  drivers/gpu/drm/tegra/rgb.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
> index 338f7f6..0266fb4 100644
> --- a/drivers/gpu/drm/tegra/rgb.c
> +++ b/drivers/gpu/drm/tegra/rgb.c
> @@ -15,6 +15,7 @@
>  struct tegra_rgb {
> struct tegra_output output;
> struct tegra_dc *dc;
> +   bool enabled;
>
> struct clk *clk_parent;
> struct clk *clk;
> @@ -89,6 +90,9 @@ static int tegra_output_rgb_enable(struct tegra_output 
> *output)
> struct tegra_rgb *rgb = to_rgb(output);
> unsigned long value;
>
> +   if (rgb->enabled)
> +   return 0;
> +
> tegra_dc_write_regs(rgb->dc, rgb_enable, ARRAY_SIZE(rgb_enable));
>
> value = DE_SELECT_ACTIVE | DE_CONTROL_NORMAL;
> @@ -122,6 +126,8 @@ static int tegra_output_rgb_enable(struct tegra_output 
> *output)
> tegra_dc_writel(rgb->dc, GENERAL_ACT_REQ << 8, DC_CMD_STATE_CONTROL);
> tegra_dc_writel(rgb->dc, GENERAL_ACT_REQ, DC_CMD_STATE_CONTROL);
>
> +   rgb->enabled = true;
> +
> return 0;
>  }
>
> @@ -130,6 +136,9 @@ static int tegra_output_rgb_disable(struct tegra_output 
> *output)
> struct tegra_rgb *rgb = to_rgb(output);
> unsigned long value;
>
> +   if (!rgb->enabled)
> +   return 0;
> +
> value = tegra_dc_readl(rgb->dc, DC_CMD_DISPLAY_POWER_CONTROL);
> value &= ~(PW0_ENABLE | PW1_ENABLE | PW2_ENABLE | PW3_ENABLE |
>PW4_ENABLE | PM0_ENABLE | PM1_ENABLE);
> @@ -144,6 +153,8 @@ static int tegra_output_rgb_disable(struct tegra_output 
> *output)
>
> tegra_dc_write_regs(rgb->dc, rgb_disable, ARRAY_SIZE(rgb_disable));
>
> +   rgb->enabled = false;
> +
> return 0;
>  }
>

Wouldn't it make more sense to make "enabled" and int that counts how
many times tegra_output_rgb_enable has been called? That way you can
have tegra_output_rgb_disable only really disable the display once the
same amount of disables have been performed...

[PATCH] gpu: host1x: do not check previously handled gathers

[Erik Faye-Lund]

Ping?

On Tue, Jan 7, 2014 at 9:03 PM, Erik Faye-Lund  wrote:
> When patching gathers, we don't need to check against
> gathers with lower indices than the current one, as
> they are guaranteed to already have been handled.
>
> Signed-off-by: Erik Faye-Lund 
> ---
>
> Here's a trivial optimization I have been running with for a while.
>
>  drivers/gpu/host1x/job.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
> index de5ec33..e965805 100644
> --- a/drivers/gpu/host1x/job.c
> +++ b/drivers/gpu/host1x/job.c
> @@ -534,7 +534,7 @@ int host1x_job_pin(struct host1x_job *job, struct device 
> *dev)
>
> g->base = job->gather_addr_phys[i];
>
> -   for (j = 0; j < job->num_gathers; j++)
> +   for (j = i + 1; j < job->num_gathers; j++)
> if (job->gathers[j].bo == g->bo)
> job->gathers[j].handled = true;
>
> --
> 1.8.1.2
>

[PATCH] gpu: host1x: do not check previously handled gathers

[Erik Faye-Lund]

When patching gathers, we don't need to check against
gathers with lower indices than the current one, as
they are guaranteed to already have been handled.

Signed-off-by: Erik Faye-Lund 
---

Here's a trivial optimization I have been running with for a while.

 drivers/gpu/host1x/job.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index de5ec33..e965805 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -534,7 +534,7 @@ int host1x_job_pin(struct host1x_job *job, struct device 
*dev)

g->base = job->gather_addr_phys[i];

-   for (j = 0; j < job->num_gathers; j++)
+   for (j = i + 1; j < job->num_gathers; j++)
if (job->gathers[j].bo == g->bo)
job->gathers[j].handled = true;

-- 
1.8.1.2

[PATCH 09/13] drm/msm: split out msm_kms.h

[Erik Faye-Lund]

On Sun, Dec 8, 2013 at 12:35 AM, Rob Clark  wrote:
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> new file mode 100644
> index 000..e42973c
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright (C) 2013 Red Hat
> + * Author: Rob Clark 
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published 
> by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along 
> with
> + * this program.  If not, see .
> + */
> +
> +#ifndef __MSM_KMS_H__
> +#define __MSM_KMS_H__
> +
> +#include 
> +#include 
> +
> +#include "msm_drv.h"
> +
> +/* As there are different display controller blocks depending on the
> + * snapdragon version, the kms support is split out and the appropriate
> + * implementation is loaded at runtime.  The kms module is responsible
> + * for constructing the appropriate planes/crtcs/encoders/connectors.
> + */
> +struct msm_kms_funcs {
> +   /* hw initialization: */
> +   int (*hw_init)(struct msm_kms *kms);
> +   /* irq handling: */
> +   void (*irq_preinstall)(struct msm_kms *kms);
> +   int (*irq_postinstall)(struct msm_kms *kms);
> +   void (*irq_uninstall)(struct msm_kms *kms);
> +   irqreturn_t (*irq)(struct msm_kms *kms);
> +   int (*enable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
> +   void (*disable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
> +   /* misc: */
> +   const struct msm_format *(*get_format)(struct msm_kms *kms, uint32_t 
> format);
> +   long (*round_pixclk)(struct msm_kms *kms, unsigned long rate,
> +   struct drm_encoder *encoder);
> +   /* cleanup: */
> +   void (*preclose)(struct msm_kms *kms, struct drm_file *file);
> +   void (*destroy)(struct msm_kms *kms);
> +};
> +
> +struct msm_kms {
> +   const struct msm_kms_funcs *funcs;
> +};
> +
> +struct msm_kms *mdp4_kms_init(struct drm_device *dev);
> +struct msm_kms *mdp5_kms_init(struct drm_device *dev);

Shouldn't this be introduced in a follow-up patch that also adds the definition?

Re: [PATCH v2 08/27] drm/tegra: gr2d: Miscellaneous cleanups

[Erik Faye-Lund]

On Tue, Oct 8, 2013 at 7:48 AM, Terje Bergström tbergst...@nvidia.com wrote:
 On 07.10.2013 16:02, Erik Faye-Lund wrote:
 So the question is really how the hardware treats writes to
 non-existent registers. My guess would be that they are simply not
 recorded, and if that's the case it doesn't matter what we do. And
 doing an unconditional AND is faster than doing a bit-test followed by
 a conditional branch.

 Hardware ignores writes to non-existent registers. Sometimes
 non-existent registers are taken into use in future versions, though.

Right. That might be a motivation to treat non-existent registers as
errors. But then I start to wonder about holes in the pointer
address space should be treated differently. For instance, from the
TRM it looks like registers 0x3 - 0x8, 0xe, 0x15 and 0x42 are
undefined for T20. If writes to registers beyond 0x4c fails, shouldn't
these also fail? Or are somehow guaranteed that these holes will never
be plugged? Are they simply missing from the TRM?

I dunno...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 08/27] drm/tegra: gr2d: Miscellaneous cleanups

[Erik Faye-Lund]

On Mon, Oct 7, 2013 at 10:34 AM, Thierry Reding
thierry.red...@gmail.com wrote:
 Rework the address table code for the host1x firewall. The previous
 implementation allocated a bitfield but didn't check for a valid pointer
 so it could potentially crash.

I don't think it could crash. The bitmaps was allocated as a 256-bit
field, and the register offset gets AND'ed with 0xFF before being
looked up. What am I missing?

 Furthermore setting up a bitfield makes
 the code more complex than it needs to be.

Doesn't this perform worse than the current implementation? Going from
1 to 13 checks per write sounds less than ideal to me...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 08/27] drm/tegra: gr2d: Miscellaneous cleanups

[Erik Faye-Lund]

On Mon, Oct 7, 2013 at 2:14 PM, Thierry Reding thierry.red...@gmail.com wrote:
 On Mon, Oct 07, 2013 at 01:34:44PM +0200, Erik Faye-Lund wrote:
 On Mon, Oct 7, 2013 at 10:34 AM, Thierry Reding
 thierry.red...@gmail.com wrote:
  Rework the address table code for the host1x firewall. The previous
  implementation allocated a bitfield but didn't check for a valid pointer
  so it could potentially crash.

 I don't think it could crash. The bitmaps was allocated as a 256-bit
 field, and the register offset gets AND'ed with 0xFF before being
 looked up. What am I missing?

 The pointer returned from the allocation is never checked, so it could
 theoretically be NULL and be used regardless.


Right. Thanks for clarifying.

 Also I'm not sure that AND'ing with 0xff is the right thing to do here.

Well, maybe not. I'm not entirely convinced it's *not*, though.

  Furthermore setting up a bitfield makes
  the code more complex than it needs to be.

 Doesn't this perform worse than the current implementation? Going from
 1 to 13 checks per write sounds less than ideal to me...

 I'm not so sure. Caching should alleviate the issue with the increased
 amount of data. Then there's the fact that previously we needed to
 divide and compute the remainder of the division for the BIT_MASK and
 BIT_WORD operations.

That's an AND and a shift, not division and modulo, both single-cycle
instructions on ARM. I'm pretty sure that'd be a win.

 One other added benefit of this approach is that the address registers
 are stored in a const array and therefore could reside in a read-only
 memory region. With the previous approach, once somebody had access to
 the bitmap, it could easily be overwritten with zeros and effectively
 make the firewall useless. I'm not sure how likely that would be, but it
 could be done.

Perhaps the bitmap could be generated compile-time and stuck in
read-only memory?

 I guess one could actually go and run a benchmark against both versions
 and balance the performance impact against the possible security
 implications. But given that we don't really have any benchmarks that's
 pretty hard to do.

Well, perhaps we could have both? :)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 08/27] drm/tegra: gr2d: Miscellaneous cleanups

[Erik Faye-Lund]

On Mon, Oct 7, 2013 at 2:53 PM, Erik Faye-Lund kusmab...@gmail.com wrote:
 On Mon, Oct 7, 2013 at 2:14 PM, Thierry Reding thierry.red...@gmail.com 
 wrote:
 On Mon, Oct 07, 2013 at 01:34:44PM +0200, Erik Faye-Lund wrote:
 On Mon, Oct 7, 2013 at 10:34 AM, Thierry Reding
 thierry.red...@gmail.com wrote:
  Rework the address table code for the host1x firewall. The previous
  implementation allocated a bitfield but didn't check for a valid pointer
  so it could potentially crash.

 I don't think it could crash. The bitmaps was allocated as a 256-bit
 field, and the register offset gets AND'ed with 0xFF before being
 looked up. What am I missing?

 The pointer returned from the allocation is never checked, so it could
 theoretically be NULL and be used regardless.

 Also I'm not sure that AND'ing with 0xff is the right thing to do here.

 Well, maybe not. I'm not entirely convinced it's *not*, though.


I'm sorry, I intended to fill out a bit more here before I hit send,
and totally forgot.

Point is, it seems only 0x00..0x4c is used (and then repeated at
multiples of 0x4000 for each context, but the offsets don't have
enough bits to reach that far), from looking at the TRM.

So the question is really how the hardware treats writes to
non-existent registers. My guess would be that they are simply not
recorded, and if that's the case it doesn't matter what we do. And
doing an unconditional AND is faster than doing a bit-test followed by
a conditional branch.

Of course, if the hardware does *not* ignore writes to non-existent
register, then we might be in trouble. But for that, we'd probably
want a full map of valid registers, not just saying that they aren't
pointers, no?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 08/27] drm/tegra: gr2d: Miscellaneous cleanups

[Erik Faye-Lund]

On Mon, Oct 7, 2013 at 2:52 PM, Terje Bergström tbergst...@nvidia.com wrote:
 On 07.10.2013 15:14, Thierry Reding wrote:
 * PGP Signed by an unknown key

 On Mon, Oct 07, 2013 at 01:34:44PM +0200, Erik Faye-Lund wrote:
 On Mon, Oct 7, 2013 at 10:34 AM, Thierry Reding
 thierry.red...@gmail.com wrote:
 Rework the address table code for the host1x firewall. The previous
 implementation allocated a bitfield but didn't check for a valid pointer
 so it could potentially crash.

 I don't think it could crash. The bitmaps was allocated as a 256-bit
 field, and the register offset gets AND'ed with 0xFF before being
 looked up. What am I missing?

 The pointer returned from the allocation is never checked, so it could
 theoretically be NULL and be used regardless.

 Also I'm not sure that AND'ing with 0xff is the right thing to do here.

 Oops, there's a check for NULL missing. If that allocation fails, probe
 should fail, so we just need to propagate the error condition. Otherwise
 we might just leave the firewall off, and let everything go in unchecked.

 AND 0xff is necessary, because the same registers are mirrored in
 multiple contexts. AND removes the offset coming from context, and
 leaves just the plain register offset.


The offsets in the commands don't have enough bits to reach over to
the next context. The contexts are repeated at multiples of 0x4000,
and 0xFFF is the largest encodable offset. So I don't really thing the
AND is needed for *that* purpose.

 If somebody gets access to the bitmap, he has access to kernel data
 structures. GR2D firewall is the least of our worries in this case.

Indeed, that's only a problem if someone is already on the other side
of the air-tight hatch.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] gpu: host1x: check relocs after all gathers are consumed

[Erik Faye-Lund]

The num_relocs count are passed to the kernel per job, not per gather.

For multi-gather jobs, we would previously fail if there were relocs in
other gathers aside from the first one.

Fix this by simply moving the check until all gathers have been
consumed.

Signed-off-by: Erik Faye-Lund kusmab...@gmail.com
---
 drivers/gpu/host1x/job.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index c4e1050..c9ddff8 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -436,10 +436,6 @@ static int validate(struct host1x_firewall *fw, struct 
host1x_job_gather *g)
}
}
 
-   /* No relocs should remain at this point */
-   if (fw-num_relocs)
-   err = -EINVAL;
-
 out:
return err;
 }
@@ -493,6 +489,10 @@ static inline int copy_gathers(struct host1x_job *job, 
struct device *dev)
offset += g-words * sizeof(u32);
}
 
+   /* No relocs should remain at this point */
+   if (fw.num_relocs)
+   return -EINVAL;
+
return 0;
 }
 
-- 
1.7.9.5

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel