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

[Eric Anholt]

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

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


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

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

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

Re: [PATCH v3 1/5] drm/scheduler: rework job destruction

[Eric Anholt]

Andrey Grodzovsky  writes:

> From: Christian König 
>
> We now destroy finished jobs from the worker thread to make sure that
> we never destroy a job currently in timeout processing.
> By this we avoid holding lock around ring mirror list in drm_sched_stop
> which should solve a deadlock reported by a user.
>
> v2: Remove unused variable.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109692
>
> Signed-off-by: Christian König 
> Signed-off-by: Andrey Grodzovsky 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  17 ++--
>  drivers/gpu/drm/etnaviv/etnaviv_dump.c |   4 -
>  drivers/gpu/drm/etnaviv/etnaviv_sched.c|   9 +-
>  drivers/gpu/drm/scheduler/sched_main.c | 138 
> +
>  drivers/gpu/drm/v3d/v3d_sched.c|   9 +-

Missing corresponding panfrost and lima updates.  You should probably
pull in drm-misc for hacking on the scheduler.

> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
> index ce7c737b..8efb091 100644
> --- a/drivers/gpu/drm/v3d/v3d_sched.c
> +++ b/drivers/gpu/drm/v3d/v3d_sched.c
> @@ -232,11 +232,18 @@ v3d_gpu_reset_for_timeout(struct v3d_dev *v3d, struct 
> drm_sched_job *sched_job)
>  
>   /* block scheduler */
>   for (q = 0; q < V3D_MAX_QUEUES; q++)
> - drm_sched_stop(>queue[q].sched);
> + drm_sched_stop(>queue[q].sched, sched_job);
>  
>   if(sched_job)
>   drm_sched_increase_karma(sched_job);
>  
> + /*
> +  * Guilty job did complete and hence needs to be manually removed
> +  * See drm_sched_stop doc.
> +  */
> + if (list_empty(_job->node))
> + sched_job->sched->ops->free_job(sched_job);

If the if (sched_job) is necessary up above, then this should clearly be
under it.

But, can we please have a core scheduler thing we call here instead of
drivers all replicating it?

> +
>   /* get the GPU back into the init state */
>   v3d_reset(v3d);
>  


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

Re: [PATCH] drm/v3d: Fix calling drm_sched_resubmit_jobs for same sched.

[Eric Anholt]

"Grodzovsky, Andrey"  writes:

> On 3/13/19 12:13 PM, Eric Anholt wrote:
>> "Grodzovsky, Andrey"  writes:
>>
>>> They are not the same, but the guilty job belongs to only one {entity,
>>> scheduler} pair and so we mark as guilty only for that particular
>>> entity in the context of that scheduler only once.
>> I get it now, sorry.  I'll merge this through drm-misc-next.
>
> np, i actually pushed it into our internal branch already so you can do 
> that or wait for our next pull request.

I also fixed the whitespace in the moved code and added the missing
Fixes: line, so I'd like to get it merged through the proper tree for
maintaining v3d.


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

Re: [PATCH] drm/v3d: Fix calling drm_sched_resubmit_jobs for same sched.

[Eric Anholt]

"Grodzovsky, Andrey"  writes:

> They are not the same, but the guilty job belongs to only one {entity,
> scheduler} pair and so we mark as guilty only for that particular
> entity in the context of that scheduler only once.

I get it now, sorry.  I'll merge this through drm-misc-next.


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

Re: [PATCH] drm/v3d: Fix calling drm_sched_resubmit_jobs for same sched.

[Eric Anholt]

Andrey Grodzovsky  writes:

> Also stop calling drm_sched_increase_karma multiple times.

Each v3d->queue[q].sched was initialized with a separate
drm_sched_init().  I wouldn't have thought they were all the "same
sched".


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

Re: [PATCH] drm/sched: Always trace the dependencies we wait on, to fix a race.

[Eric Anholt]

"Koenig, Christian"  writes:

> Am 07.12.18 um 20:16 schrieb Eric Anholt:
>> The entity->dependency can go away completely once we've called
>> drm_sched_entity_add_dependency_cb() (if the cb is called before we
>> get around to tracing).  The tracepoint is more useful if we trace
>> every dependency instead of just ones that get callbacks installed,
>> anyway, so just do that.
>>
>> Fixes any easy-to-produce OOPS when tracing the scheduler on V3D with
>> "perf record -a -e gpu_scheduler:.\* glxgears" and DEBUG_SLAB enabled.
>>
>> Signed-off-by: Eric Anholt 
>
> Reviewed-by: Christian König 
>
> Going to pick that up for upstream and will add with a CC: stable.

Looks like this got misplaced.


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

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

[Eric Anholt]

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

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


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

Re: [PATCH] drm: return false in drm_arch_can_wc_memory() for ARM

[Eric Anholt]

Alex Deucher  writes:

> On Fri, Dec 21, 2018 at 9:16 AM Liviu Dudau  wrote:
>>
>> On Thu, Dec 20, 2018 at 04:36:19PM +0100, Daniel Vetter wrote:
>> > On Thu, Dec 20, 2018 at 09:56:57AM -0500, Alex Deucher wrote:
>> > > I'm not familiar enough with ARM to know if write combining
>> > > is actually an architectural limitation or if it's an issue
>> > > with the PCIe IPs used on various platforms, but so far
>> > > everyone that has tried to run radeon hardware on
>> > > ARM has had to disable it.  So let's just make it official.
>> >
>> > wc on arm is Really Complicated (tm) afaiui. There's issues with aliasing
>> > mappings and stuff, so you need to allocate your wc memory from special
>> > pools. So probably best to just disable it until we figure this out.
>>
>> I believe both of you are conflating different issues under the wrong
>> name. Write combining happens all the time with Arm, the ARMv8
>> architecture is a weakly-ordered model of memory so hardware is allowed
>> to re-order or combine memory access as they seem fit.
>>
>> A while ago I did run an AMD GPU card on my Juno dev board and it worked
>> (for a very limited definition of worked, I've only validated the fact
>> that I could get an fbcon and could run un-accelerated X11). So I would
>> be interested if Alex could share some of the scenarios where people are
>> seeing failures.
>
> Here's an example:
> https://bugs.freedesktop.org/show_bug.cgi?id=108625
> But there are probably 5 or 6 other cases where people have emailed me
> or our team directly with issues on ARM resolved by disabling WC.
> Generally the driver seems to load ok, but then hangs as soon as you
> try and use acceleration from userspace or we end up with page
> flipping timeouts.  Not really sure what the issue is.  Michel
> suggested maybe ARM has a cacheable kernel mapping of all "normal"
> system memory, and having
> both that mapping and another non-cacheable mapping of the same page
> can result in bad behaviour.
>
>>
>> As for aliasing, yeah, having multiple aliases to the same piece of
>> memory is a bad thing. The problem arises when devices on the PCI bus
>> have memory allocated as device memory (which on Arm is non-cacheable
>> and non-reorderable), but the PCI bus effectively acts as a write-combiner
>> which changes the order of transactions. Therefore, for devices that
>> have local memory associated with them (i.e. more than just register
>> accesses) one should allocate memory in the first place that is
>> Device-GRE (gathering, reordering and early-access). Otherwise, problems
>> will surface that are not visible on x86 as that is a strongly ordered
>> architecture.
>
> PCI framebuffer BARs are mapped on the CPU with WC.  We also use
> uncached WC mappings for system memory in cases where it's not likely
> we will be doing any CPU reads.  When accessing system memory, the GPU
> can either do a CPU cache snooped transaction or a non-snooped
> transaction.  The non-snooped transaction has lower latency and better
> throughput since it doesn't have to snoop the CPU cache.
>
>>
>> >
>> > > Signed-off-by: Alex Deucher 
>> >
>> > Reviewed-by: Daniel Vetter 
>>
>> Given that this API is only used by AMD I'm OK for now with the change,
>> but I think in general it is misleading and we should work towards
>> fixing radeon and amd drivers.
>
> Alternatively, we could just disable WC in the amdgpu driver on ARM.
> I'm not sure to what extent other drivers are using WC in general or
> have been tested on ARM.

FWIW, I use WC mappings of BOs on V3D (shmem) and VC4 (cma).  V3D is
totally stable.  VC4 I've heard reports of stability issues long-term
but I don't think it's related.  I don't do any cached mappings of my
BOs, though.


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

[PATCH] drm/sched: Always trace the dependencies we wait on, to fix a race.

[Eric Anholt]

The entity->dependency can go away completely once we've called
drm_sched_entity_add_dependency_cb() (if the cb is called before we
get around to tracing).  The tracepoint is more useful if we trace
every dependency instead of just ones that get callbacks installed,
anyway, so just do that.

Fixes any easy-to-produce OOPS when tracing the scheduler on V3D with
"perf record -a -e gpu_scheduler:.\* glxgears" and DEBUG_SLAB enabled.

Signed-off-by: Eric Anholt 
---
 drivers/gpu/drm/scheduler/sched_entity.c | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
b/drivers/gpu/drm/scheduler/sched_entity.c
index 4463d3826ecb..e2942c9a11a7 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -440,13 +440,10 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct 
drm_sched_entity *entity)
 
while ((entity->dependency =
sched->ops->dependency(sched_job, entity))) {
+   trace_drm_sched_job_wait_dep(sched_job, entity->dependency);
 
-   if (drm_sched_entity_add_dependency_cb(entity)) {
-
-   trace_drm_sched_job_wait_dep(sched_job,
-entity->dependency);
+   if (drm_sched_entity_add_dependency_cb(entity))
return NULL;
-   }
}
 
/* skip jobs from entity that marked guilty */
-- 
2.20.0.rc1

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

[PATCH] drm/sched: Fix a use-after-free when tracing the scheduler.

[Eric Anholt]

With DEBUG_SLAB (poisoning on free) enabled, I could quickly produce
an oops when tracing V3D.

Signed-off-by: Eric Anholt 
---

I think this patch is correct (though maybe a bigger refactor could
avoid the extra get/put?), but I've still got this with "vblank_mode=0
perf record -a -e v3d:.\* -e gpu_scheduler:.\* glxgears".  Any ideas?

[  139.842191] Unable to handle kernel NULL pointer dereference at virtual 
address 0020
[  139.850413] pgd = eab7bb57
[  139.854424] [0020] *pgd=8040004003, *pmd=
[  139.860523] Internal error: Oops: 206 [#1] SMP ARM
[  139.865340] Modules linked in:
[  139.868404] CPU: 0 PID: 1161 Comm: v3d_render Not tainted 4.20.0-rc4+ #552
[  139.875287] Hardware name: Broadcom STB (Flattened Device Tree)
[  139.881228] PC is at perf_trace_drm_sched_job_wait_dep+0xa8/0xf4
[  139.887243] LR is at 0xe9790274
[  139.890388] pc : []lr : []psr: a0050013
[  139.896662] sp : ed21dec0  ip : ed21dec0  fp : ed21df04
[  139.901893] r10: ed267478  r9 :   r8 : ff7bde04
[  139.907123] r7 :   r6 : 0063  r5 :   r4 : c1208448
[  139.913659] r3 : c1265690  r2 : ff7bf660  r1 : 0034  r0 : ff7bf660
[  139.920196] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
[  139.927339] Control: 30c5383d  Table: 68fa3b40  DAC: fffd
[  139.933095] Process v3d_render (pid: 1161, stack limit = 0xb3c84b1b)
[  139.939457] Stack: (0xed21dec0 to 0xed21e000)
[  139.943821] dec0: 20050013  eb9700cc  ec0e8e80  
eb9700cc e9790274
[  139.952009] dee0:  e2f59345 eb970078 eba8f680 c12ae00c c1208478 
 e8c2b048
[  139.960197] df00: eb9700cc c06e92e4 c06e8f04  80050013 ed267478 
eb970078 
[  139.968385] df20: ed267578 c0e45ae0 e9093080 c06e831c ed267630 c06e8120 
c06e77d4 c1208448
[  139.976573] df40: ee2e8acc 0001  ee2e8640 c0272ab4 ed21df54 
ed21df54 e2f59345
[  139.984762] df60: ed21c000 ed1b4800 ed2d7840  ed21c000 ed267478 
c06e8084 ee935cb0
[  139.992950] df80: ed1b4838 c0249b44 ed21c000 ed2d7840 c02499e4  
 
[  140.001138] dfa0:    c02010ac   
 
[  140.009326] dfc0:       
 
[  140.017514] dfe0:     0013  
 
[  140.025707] [] (perf_trace_drm_sched_job_wait_dep) from 
[] (drm_sched_entity_pop_job+0x394/0x438)
[  140.036332] [] (drm_sched_entity_pop_job) from [] 
(drm_sched_main+0x9c/0x298)
[  140.045221] [] (drm_sched_main) from [] 
(kthread+0x160/0x168)
[  140.052716] [] (kthread) from [] 
(ret_from_fork+0x14/0x28)

 drivers/gpu/drm/scheduler/sched_entity.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
b/drivers/gpu/drm/scheduler/sched_entity.c
index 4463d3826ecb..0d4fc86089cb 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -440,13 +440,15 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct 
drm_sched_entity *entity)
 
while ((entity->dependency =
sched->ops->dependency(sched_job, entity))) {
-
+   dma_fence_get(entity->dependency);
if (drm_sched_entity_add_dependency_cb(entity)) {
 
trace_drm_sched_job_wait_dep(sched_job,
 entity->dependency);
+   dma_fence_put(entity->dependency);
return NULL;
}
+   dma_fence_put(entity->dependency);
}
 
/* skip jobs from entity that marked guilty */
-- 
2.20.0.rc1

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

Re: [PATCH 1/3] dma-buf: add dma_fence_get_stub

[Eric Anholt]

Christian König  writes:

> Am 03.12.18 um 17:08 schrieb Eric Anholt:
>> Christian König  writes:
>>
>>> Extract of useful code from the timeline work. This provides a function
>>> to return a stub or dummy fence which is always signaled.
>>>
>>> Signed-off-by: Christian König 
>>> ---
>>>   drivers/dma-buf/dma-fence.c | 36 +++-
>>>   include/linux/dma-fence.h   |  1 +
>>>   2 files changed, 36 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>>> index 1551ca7df394..136ec04d683f 100644
>>> --- a/drivers/dma-buf/dma-fence.c
>>> +++ b/drivers/dma-buf/dma-fence.c
>>>   /*
>>>* fence context counter: each execution context should have its own
>>>* fence context, this allows checking if fences belong to the same
>>>* context or not. One device can have multiple separate contexts,
>>>* and they're used if some engine can run independently of another.
>>>*/
>>> -static atomic64_t dma_fence_context_counter = ATOMIC64_INIT(0);
>>> +static atomic64_t dma_fence_context_counter = ATOMIC64_INIT(1);
>> What's this change for?  I don't see a context allocation in patch #2
>> where the stub fence is being moved from, so this seems out of place.
>
> The stub fence is using context 0 and seqno 0, but since it is always 
> signaled this actually shouldn't matter.
>
> So this is just to be on the extra clean side I made sure that nobody 
> else is using context 0.
>
> Alternatively we could also just allocate one when we initialize it for 
> the first time.

I like the allocate at init idea.


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

Re: [PATCH 1/3] dma-buf: add dma_fence_get_stub

[Eric Anholt]

Christian König  writes:

> Extract of useful code from the timeline work. This provides a function
> to return a stub or dummy fence which is always signaled.
>
> Signed-off-by: Christian König 
> ---
>  drivers/dma-buf/dma-fence.c | 36 +++-
>  include/linux/dma-fence.h   |  1 +
>  2 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 1551ca7df394..136ec04d683f 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c

>  /*
>   * fence context counter: each execution context should have its own
>   * fence context, this allows checking if fences belong to the same
>   * context or not. One device can have multiple separate contexts,
>   * and they're used if some engine can run independently of another.
>   */
> -static atomic64_t dma_fence_context_counter = ATOMIC64_INIT(0);
> +static atomic64_t dma_fence_context_counter = ATOMIC64_INIT(1);

What's this change for?  I don't see a context allocation in patch #2
where the stub fence is being moved from, so this seems out of place.


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

Re: [PATCH v3 1/3] drm/connector: Add generic underscan properties

[Eric Anholt]

Boris Brezillon  writes:

> On Mon, 3 Dec 2018 16:40:11 +0200
> Ville Syrjälä  wrote:
>
>> On Thu, Nov 22, 2018 at 12:23:29PM +0100, Boris Brezillon wrote:
>> > @@ -924,6 +978,29 @@ struct drm_connector {
>> > */
>> >struct drm_property_blob *path_blob_ptr;
>> >  
>> > +  /**
>> > +   * @underscan_mode_property: Optional connector underscan mode. Used by
>> > +   * the driver to scale the output image and compensate an overscan done
>> > +   * on the display side.
>> > +   */
>> > +  struct drm_property *underscan_mode_property;
>> > +
>> > +  /**
>> > +   * @underscan_hborder_property: Optional connector underscan horizontal
>> > +   * border (expressed in pixels). Used by the driver to adjust the
>> > +   * output image position and compensate an overscan done on the display
>> > +   * side.
>> > +   */
>> > +  struct drm_property *underscan_hborder_property;
>> > +
>> > +  /**
>> > +   * @underscan_hborder_property: Optional connector underscan vertical
>> > +   * border (expressed in pixels). Used by the driver to adjust the
>> > +   * output image position and compensate an overscan done on the display
>> > +   * side.
>> > +   */
>> > +  struct drm_property *underscan_vborder_property;  
>> 
>> I'm wondering why we're adding these new props when we already have the
>> (slightly more flexible) margin properties for TV out. We could just
>> reuse those AFAICS.
>
> I'm not against the idea, but I can't use
> drm_mode_create_tv_properties() directly, as most props created by this
> function are not applicable to an HDMI displays. Should I move the
> margins props out of the tv_connector_state and provide new helpers to
> create those props?

TV margin props look good to me, FWIW.


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

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

[Eric Anholt]

Ville Syrjala  writes:

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

For vc4,
Acked-by: Eric Anholt 

Looks like a nice cleanup!


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

Re: [PATCH v3 1/3] drm/connector: Add generic underscan properties

[Eric Anholt]

Brian Starkey  writes:

> Hi Boris,
>
> Just because I happened to read the docs in here, one typo below:
>
> On Thu, Nov 22, 2018 at 12:23:29PM +0100, Boris Brezillon wrote:
>>diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
>>index c555e17ab8d7..170317248da6 100644
>>--- a/drivers/gpu/drm/drm_connector.c
>>+++ b/drivers/gpu/drm/drm_connector.c
>>@@ -971,6 +971,38 @@ DRM_ENUM_NAME_FN(drm_get_content_protection_name, 
>>drm_cp_enum_list)
>>  *   can also expose this property to external outputs, in which case they
>>  *   must support "None", which should be the default (since external screens
>>  *   have a built-in scaler).
>>+ *
>>+ * Connectors for non-analog outputs may also have standardized underscan
>>+ * properties (drivers can set this up by calling
>>+ * drm_connector_attach_content_protection_property() on initialization):
>
> Should be drm_connector_attach_underscan_properties()

Other than this typo, this series is:

Reviewed-by: Eric Anholt 


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

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

[Eric Anholt]

Christian König  writes:

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

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


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

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

[Eric Anholt]

Kenny Ho  writes:

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

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


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

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

[Eric Anholt]

Kenny Ho  writes:

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

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

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


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

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

[Eric Anholt]

Sean Paul  writes:

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

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


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

Re: [PATCH 2/2] drm/scheduler: remove sched field from the entity

[Eric Anholt]

Nayan Deshmukh  writes:

> The scheduler of the entity is decided by the run queue on which
> it is queued. This patch avoids us the effort required to maintain
> a sync between rq and sched field when we start shifting entites
> among different rqs.

Reviewed-by: Eric Anholt 


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

Re: [PATCH 1/2] drm/scheduler: modify API to avoid redundancy

[Eric Anholt]

Nayan Deshmukh  writes:

> entity has a scheduler field and we don't need the sched argument
> in any of the functions where entity is provided.
>
> Signed-off-by: Nayan Deshmukh 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c|  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c   | 13 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c   |  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c   |  3 +--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c   |  3 +--
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c   |  2 +-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c|  4 ++--
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c |  3 +--
>  drivers/gpu/drm/etnaviv/etnaviv_sched.c   |  4 ++--
>  drivers/gpu/drm/scheduler/gpu_scheduler.c | 20 +++-
>  drivers/gpu/drm/v3d/v3d_drv.c |  4 +---
>  drivers/gpu/drm/v3d/v3d_gem.c |  2 --
>  include/drm/gpu_scheduler.h   | 10 +++---
>  13 files changed, 30 insertions(+), 42 deletions(-)
>

> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
> b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> index dac71e3b4514..a3b55c542025 100644
> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> @@ -273,11 +273,12 @@ static void drm_sched_entity_kill_jobs_cb(struct 
> dma_fence *f,
>   *
>   * Returns the remaining time in jiffies left from the input timeout
>   */
> -long drm_sched_entity_flush(struct drm_gpu_scheduler *sched,
> -struct drm_sched_entity *entity, long timeout)
> +long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout)
>  {
> + struct drm_gpu_scheduler *sched;
>   long ret = timeout;
>  
> + sched = entity->sched;
>   if (!drm_sched_entity_is_initialized(sched, entity))
>   return ret;
>   /**
> @@ -312,10 +313,11 @@ EXPORT_SYMBOL(drm_sched_entity_flush);
>   * entity and signals all jobs with an error code if the process was killed.
>   *
>   */
> -void drm_sched_entity_fini(struct drm_gpu_scheduler *sched,
> -struct drm_sched_entity *entity)
> +void drm_sched_entity_fini(struct drm_sched_entity *entity)
>  {
> + struct drm_gpu_scheduler *sched;
>  
> + sched = entity->sched;

Maybe fold the initialization into the declaration above, like you did
elsewhere?

Regardless, this is a wonderful cleanup of the API.

Reviewed-by: Eric Anholt 


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

Re: [PATCH 3/3] drm/scheduler: modify args of drm_sched_entity_init

[Eric Anholt]

Nayan Deshmukh  writes:
> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c 
> b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> index 3dc1a4f07e3f..b2dbd1c1ba69 100644
> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
> @@ -162,26 +162,32 @@ drm_sched_rq_select_entity(struct drm_sched_rq *rq)
>   * drm_sched_entity_init - Init a context entity used by scheduler when
>   * submit to HW ring.
>   *
> - * @sched: scheduler instance
>   * @entity: scheduler entity to init
> - * @rq: the run queue this entity belongs
> + * @rq_list: the list of run queue on which jobs from this
> + *   entity can be submitted
> + * @num_rq_list: number of run queue in rq_list
>   * @guilty: atomic_t set to 1 when a job on this queue
>   *  is found to be guilty causing a timeout
>   *
> + * Note: the rq_list should have atleast one element to schedule
> + *   the entity
> + *
>   * Returns 0 on success or a negative error code on failure.
>  */
> -int drm_sched_entity_init(struct drm_gpu_scheduler *sched,
> -   struct drm_sched_entity *entity,
> -   struct drm_sched_rq *rq,
> +int drm_sched_entity_init(struct drm_sched_entity *entity,
> +   struct drm_sched_rq **rq_list,
> +   unsigned int num_rq_list,
> atomic_t *guilty)
>  {
> - if (!(sched && entity && rq))
> + if (!(entity && rq_list && num_rq_list > 0 && rq_list[0]))
>   return -EINVAL;
>  
>   memset(entity, 0, sizeof(struct drm_sched_entity));
>   INIT_LIST_HEAD(>list);
> - entity->rq = rq;
> - entity->sched = sched;
> + entity->rq_list = NULL;
> + entity->rq = rq_list[0];
> + entity->sched = rq_list[0]->sched;
> + entity->num_rq_list = num_rq_list;

The API change makes sense as prep work, but I don't really like adding
the field to the struct (and changing the struct's docs for the existing
rq field) if it's going to always be NULL until a future change.

Similarly, I'd rather see patch 2 as part of a series that uses the
value.

That said, while I don't currently have a usecase for load-balancing
between entities, I may in the future, so thanks for working on this!


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

Re: [PATCH 1/3] drm/scheduler: add a pointer to scheduler in the rq

[Eric Anholt]

Nayan Deshmukh  writes:

> Signed-off-by: Nayan Deshmukh 
> ---

Acked-by: Eric Anholt 


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

Re: [PATCH 3/3] drm/v3d: Add a note about locking of v3d_fence_create().

[Eric Anholt]

Lucas Stach  writes:

> Am Dienstag, den 05.06.2018, 12:03 -0700 schrieb Eric Anholt:
>> This isn't the first time I've had to argue to myself why the '++' was
>> safe.
>
> And now you need to do the same thing with me...
>
>> Signed-off-by: Eric Anholt 
>> ---
>>  drivers/gpu/drm/v3d/v3d_fence.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/v3d/v3d_fence.c 
>> b/drivers/gpu/drm/v3d/v3d_fence.c
>> index bfe31a89668b..6265e9ab4a13 100644
>> --- a/drivers/gpu/drm/v3d/v3d_fence.c
>> +++ b/drivers/gpu/drm/v3d/v3d_fence.c
>> @@ -3,6 +3,9 @@
>>  
>>  #include "v3d_drv.h"
>>  
>> +/* Note that V3D fences are created during v3d_job_run(), so we're
>> + * already implictly locked.
>> + */
> I don't see where you would be locked in the job_run path. I think what
> you mean is that this path needs no locks, as it is driven by a single
> scheduler thread, right?

Yeah, it's only called from run_job, and run_job can't reenter.


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

[PATCH 1/3 v2] drm/v3d: Take a lock across GPU scheduler job creation and queuing.

[Eric Anholt]

Between creation and queueing of a job, you need to prevent any other
job from being created and queued.  Otherwise the scheduler's fences
may be signaled out of seqno order.

v2: move mutex unlock to the error label.

Signed-off-by: Eric Anholt 
Fixes: 57692c94dcbe ("drm/v3d: Introduce a new DRM driver for Broadcom V3D 
V3.x+")
---
 drivers/gpu/drm/v3d/v3d_drv.h | 5 +
 drivers/gpu/drm/v3d/v3d_gem.c | 4 
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index a043ac3aae98..26005abd9c5d 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -85,6 +85,11 @@ struct v3d_dev {
 */
struct mutex reset_lock;
 
+   /* Lock taken when creating and pushing the GPU scheduler
+* jobs, to keep the sched-fence seqnos in order.
+*/
+   struct mutex sched_lock;
+
struct {
u32 num_allocated;
u32 pages_allocated;
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index b513f9189caf..269fe16379c0 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -550,6 +550,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail;
 
+   mutex_lock(>sched_lock);
if (exec->bin.start != exec->bin.end) {
ret = drm_sched_job_init(>bin.base,
 >queue[V3D_BIN].sched,
@@ -576,6 +577,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
kref_get(>refcount); /* put by scheduler job completion */
drm_sched_entity_push_job(>render.base,
  _priv->sched_entity[V3D_RENDER]);
+   mutex_unlock(>sched_lock);
 
v3d_attach_object_fences(exec);
 
@@ -594,6 +596,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
return 0;
 
 fail_unreserve:
+   mutex_unlock(>sched_lock);
v3d_unlock_bo_reservations(dev, exec, _ctx);
 fail:
v3d_exec_put(exec);
@@ -615,6 +618,7 @@ v3d_gem_init(struct drm_device *dev)
spin_lock_init(>job_lock);
mutex_init(>bo_lock);
mutex_init(>reset_lock);
+   mutex_init(>sched_lock);
 
/* Note: We don't allocate address 0.  Various bits of HW
 * treat 0 as special, such as the occlusion query counters
-- 
2.17.0

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

[PATCH 3/3] drm/v3d: Add a note about locking of v3d_fence_create().

[Eric Anholt]

This isn't the first time I've had to argue to myself why the '++' was
safe.

Signed-off-by: Eric Anholt 
---
 drivers/gpu/drm/v3d/v3d_fence.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/v3d/v3d_fence.c b/drivers/gpu/drm/v3d/v3d_fence.c
index bfe31a89668b..6265e9ab4a13 100644
--- a/drivers/gpu/drm/v3d/v3d_fence.c
+++ b/drivers/gpu/drm/v3d/v3d_fence.c
@@ -3,6 +3,9 @@
 
 #include "v3d_drv.h"
 
+/* Note that V3D fences are created during v3d_job_run(), so we're
+ * already implictly locked.
+ */
 struct dma_fence *v3d_fence_create(struct v3d_dev *v3d, enum v3d_queue queue)
 {
struct v3d_fence *fence;
-- 
2.17.0

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

[PATCH 2/3] drm/v3d: Remove the bad signaled() implementation.

[Eric Anholt]

Since our seqno value comes from a counter associated with the GPU
ring, not the entity (aka client), they'll be completed out of order.
There's actually no need for this code at all, since we don't have
enable_signaling() and thus DMA_FENCE_SIGNALED_BIT will be set before
we could be called.

Signed-off-by: Eric Anholt 
---
 drivers/gpu/drm/v3d/v3d_drv.h   |  1 -
 drivers/gpu/drm/v3d/v3d_fence.c | 13 -
 drivers/gpu/drm/v3d/v3d_gem.c   |  7 ++-
 drivers/gpu/drm/v3d/v3d_irq.c   |  3 ---
 4 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index 26005abd9c5d..f32ac8c98f37 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -25,7 +25,6 @@ struct v3d_queue_state {
 
u64 fence_context;
u64 emit_seqno;
-   u64 finished_seqno;
 };
 
 struct v3d_dev {
diff --git a/drivers/gpu/drm/v3d/v3d_fence.c b/drivers/gpu/drm/v3d/v3d_fence.c
index 087d49c8cb12..bfe31a89668b 100644
--- a/drivers/gpu/drm/v3d/v3d_fence.c
+++ b/drivers/gpu/drm/v3d/v3d_fence.c
@@ -40,19 +40,14 @@ static bool v3d_fence_enable_signaling(struct dma_fence 
*fence)
return true;
 }
 
-static bool v3d_fence_signaled(struct dma_fence *fence)
-{
-   struct v3d_fence *f = to_v3d_fence(fence);
-   struct v3d_dev *v3d = to_v3d_dev(f->dev);
-
-   return v3d->queue[f->queue].finished_seqno >= f->seqno;
-}
-
 const struct dma_fence_ops v3d_fence_ops = {
.get_driver_name = v3d_fence_get_driver_name,
.get_timeline_name = v3d_fence_get_timeline_name,
.enable_signaling = v3d_fence_enable_signaling,
-   .signaled = v3d_fence_signaled,
+   /* Each of our fences gets signaled as complete by the IRQ
+* handler, so we rely on the core's tracking of signaling.
+*/
+   .signaled = NULL,
.wait = dma_fence_default_wait,
.release = dma_fence_free,
 };
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index 9ea83bdb9a30..d06d6697e089 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -657,17 +657,14 @@ void
 v3d_gem_destroy(struct drm_device *dev)
 {
struct v3d_dev *v3d = to_v3d_dev(dev);
-   enum v3d_queue q;
 
v3d_sched_fini(v3d);
 
/* Waiting for exec to finish would need to be done before
 * unregistering V3D.
 */
-   for (q = 0; q < V3D_MAX_QUEUES; q++) {
-   WARN_ON(v3d->queue[q].emit_seqno !=
-   v3d->queue[q].finished_seqno);
-   }
+   WARN_ON(v3d->bin_job);
+   WARN_ON(v3d->render_job);
 
drm_mm_takedown(>mm);
 
diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
index 77e1fa046c10..e07514eb11b5 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -87,15 +87,12 @@ v3d_irq(int irq, void *arg)
}
 
if (intsts & V3D_INT_FLDONE) {
-   v3d->queue[V3D_BIN].finished_seqno++;
dma_fence_signal(v3d->bin_job->bin.done_fence);
status = IRQ_HANDLED;
}
 
if (intsts & V3D_INT_FRDONE) {
-   v3d->queue[V3D_RENDER].finished_seqno++;
dma_fence_signal(v3d->render_job->render.done_fence);
-
status = IRQ_HANDLED;
}
 
-- 
2.17.0

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

[PATCH 1/3] drm/v3d: Take a lock across GPU scheduler job creation and queuing.

[Eric Anholt]

Between creation and queueing of a job, you need to prevent any other
job from being created and queued.  Otherwise the scheduler's fences
may be signaled out of seqno order.

Signed-off-by: Eric Anholt 
Fixes: 57692c94dcbe ("drm/v3d: Introduce a new DRM driver for Broadcom V3D 
V3.x+")
---

ccing amd-gfx due to interaction of this series with the scheduler.

 drivers/gpu/drm/v3d/v3d_drv.h |  5 +
 drivers/gpu/drm/v3d/v3d_gem.c | 11 +--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
index a043ac3aae98..26005abd9c5d 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.h
+++ b/drivers/gpu/drm/v3d/v3d_drv.h
@@ -85,6 +85,11 @@ struct v3d_dev {
 */
struct mutex reset_lock;
 
+   /* Lock taken when creating and pushing the GPU scheduler
+* jobs, to keep the sched-fence seqnos in order.
+*/
+   struct mutex sched_lock;
+
struct {
u32 num_allocated;
u32 pages_allocated;
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
index b513f9189caf..9ea83bdb9a30 100644
--- a/drivers/gpu/drm/v3d/v3d_gem.c
+++ b/drivers/gpu/drm/v3d/v3d_gem.c
@@ -550,13 +550,16 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail;
 
+   mutex_lock(>sched_lock);
if (exec->bin.start != exec->bin.end) {
ret = drm_sched_job_init(>bin.base,
 >queue[V3D_BIN].sched,
 _priv->sched_entity[V3D_BIN],
 v3d_priv);
-   if (ret)
+   if (ret) {
+   mutex_unlock(>sched_lock);
goto fail_unreserve;
+   }
 
exec->bin_done_fence =
dma_fence_get(>bin.base.s_fence->finished);
@@ -570,12 +573,15 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
 >queue[V3D_RENDER].sched,
 _priv->sched_entity[V3D_RENDER],
 v3d_priv);
-   if (ret)
+   if (ret) {
+   mutex_unlock(>sched_lock);
goto fail_unreserve;
+   }
 
kref_get(>refcount); /* put by scheduler job completion */
drm_sched_entity_push_job(>render.base,
  _priv->sched_entity[V3D_RENDER]);
+   mutex_unlock(>sched_lock);
 
v3d_attach_object_fences(exec);
 
@@ -615,6 +621,7 @@ v3d_gem_init(struct drm_device *dev)
spin_lock_init(>job_lock);
mutex_init(>bo_lock);
mutex_init(>reset_lock);
+   mutex_init(>sched_lock);
 
/* Note: We don't allocate address 0.  Various bits of HW
 * treat 0 as special, such as the occlusion query counters
-- 
2.17.0

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

Re: [PATCH v2 2/4] drm/vc4: Take underscan setup into account when updating planes

[Eric Anholt]

Ville Syrjälä  writes:

> On Fri, May 11, 2018 at 09:47:49PM +0200, Boris Brezillon wrote:
>> On Fri, 11 May 2018 20:29:48 +0300
>> Ville Syrjälä  wrote:
>> 
>> > On Fri, May 11, 2018 at 07:12:21PM +0200, Boris Brezillon wrote:
>> > > On Fri, 11 May 2018 19:54:02 +0300
>> > > Ville Syrjälä  wrote:
>> > >   
>> > > > On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote:  
>> > > > > On Fri, 11 May 2018 18:34:50 +0300
>> > > > > Ville Syrjälä  wrote:
>> > > > > 
>> > > > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote:   
>> > > > > >  
>> > > > > > > Applying an underscan setup is just a matter of scaling all 
>> > > > > > > planes
>> > > > > > > appropriately and adjusting the CRTC X/Y offset to account for 
>> > > > > > > the
>> > > > > > > horizontal and vertical border.
>> > > > > > > 
>> > > > > > > Create an vc4_plane_underscan_adj() function doing that and call 
>> > > > > > > it from
>> > > > > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to 
>> > > > > > > attach
>> > > > > > > underscan properties to the HDMI connector.
>> > > > > > > 
>> > > > > > > Signed-off-by: Boris Brezillon 
>> > > > > > > ---
>> > > > > > > Changes in v2:
>> > > > > > > - Take changes on hborder/vborder meaning into account
>> > > > > > > ---
>> > > > > > >  drivers/gpu/drm/vc4/vc4_plane.c | 49 
>> > > > > > > -
>> > > > > > >  1 file changed, 48 insertions(+), 1 deletion(-)
>> > > > > > > 
>> > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c 
>> > > > > > > b/drivers/gpu/drm/vc4/vc4_plane.c
>> > > > > > > index 71d44c357d35..61ed60841cd6 100644
>> > > > > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c
>> > > > > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
>> > > > > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct 
>> > > > > > > drm_plane_state *state, int plane)
>> > > > > > >  }
>> > > > > > >  }
>> > > > > > >  
>> > > > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state 
>> > > > > > > *pstate)
>> > > > > > > +{
>> > > > > > > +struct vc4_plane_state *vc4_pstate = 
>> > > > > > > to_vc4_plane_state(pstate);
>> > > > > > > +struct drm_connector_state *conn_state = NULL;
>> > > > > > > +struct drm_connector *conn;
>> > > > > > > +struct drm_crtc_state *crtc_state;
>> > > > > > > +int i;
>> > > > > > > +
>> > > > > > > +for_each_new_connector_in_state(pstate->state, conn, 
>> > > > > > > conn_state, i) {
>> > > > > > > +if (conn_state->crtc == pstate->crtc)
>> > > > > > > +break;
>> > > > > > > +}
>> > > > > > > +
>> > > > > > > +if (i == pstate->state->num_connector)
>> > > > > > > +return 0;
>> > > > > > > +
>> > > > > > > +if (conn_state->underscan.mode != DRM_UNDERSCAN_ON)
>> > > > > > > +return 0;
>> > > > > > > +
>> > > > > > > +crtc_state = 
>> > > > > > > drm_atomic_get_new_crtc_state(pstate->state,
>> > > > > > > +   
>> > > > > > > pstate->crtc);
>> > > > > > > +
>> > > > > > > +if (conn_state->underscan.hborder >= 
>> > > > > > > crtc_state->mode.hdisplay ||
>> > > > > > > +conn_state->underscan.vborder >= 
>> > > > > > > crtc_state->mode.vdisplay)
>> > > > > > > +return -EINVAL;  
>> > > > > > 
>> > > > > > border * 2 ?
>> > > > > 
>> > > > > Oops, indeed. I'll fix that.
>> > > > > 
>> > > > > > 
>> > > > > > > +
>> > > > > > > +vc4_pstate->crtc_x += conn_state->underscan.hborder;
>> > > > > > > +vc4_pstate->crtc_y += conn_state->underscan.vborder;
>> > > > > > > +vc4_pstate->crtc_w = (vc4_pstate->crtc_w *
>> > > > > > > +  (crtc_state->mode.hdisplay -
>> > > > > > > +   (conn_state->underscan.hborder * 
>> > > > > > > 2))) /
>> > > > > > > + crtc_state->mode.hdisplay;
>> > > > > > > +vc4_pstate->crtc_h = (vc4_pstate->crtc_h *
>> > > > > > > +  (crtc_state->mode.vdisplay -
>> > > > > > > +   (conn_state->underscan.vborder * 
>> > > > > > > 2))) /
>> > > > > > > + crtc_state->mode.vdisplay;  
>> > > > > > 
>> > > > > > So you're now scaling all planes? The code seems to reject scaling 
>> > > > > > for
>> > > > > > the cursor plane, how are you dealing with that? Or just no cursor
>> > > > > > allowed when underscanning?
>> > > > > 
>> > > > > No, I just didn't test with a cursor plane. We should probably avoid
>> > > > > scaling the cursor plane and just adjust its position. Eric any 
>> > > > > opinion
>> > > > > on that?
>> > > > 
>> > > > I don't think you can just not 

Re: [PATCH v2 2/4] drm/vc4: Take underscan setup into account when updating planes

[Eric Anholt]

Ville Syrjälä  writes:

> On Fri, May 11, 2018 at 07:12:21PM +0200, Boris Brezillon wrote:
>> On Fri, 11 May 2018 19:54:02 +0300
>> Ville Syrjälä  wrote:
>> 
>> > On Fri, May 11, 2018 at 05:52:56PM +0200, Boris Brezillon wrote:
>> > > On Fri, 11 May 2018 18:34:50 +0300
>> > > Ville Syrjälä  wrote:
>> > >   
>> > > > On Fri, May 11, 2018 at 04:59:17PM +0200, Boris Brezillon wrote:  
>> > > > > Applying an underscan setup is just a matter of scaling all planes
>> > > > > appropriately and adjusting the CRTC X/Y offset to account for the
>> > > > > horizontal and vertical border.
>> > > > > 
>> > > > > Create an vc4_plane_underscan_adj() function doing that and call it 
>> > > > > from
>> > > > > vc4_plane_setup_clipping_and_scaling() so that we are ready to attach
>> > > > > underscan properties to the HDMI connector.
>> > > > > 
>> > > > > Signed-off-by: Boris Brezillon 
>> > > > > ---
>> > > > > Changes in v2:
>> > > > > - Take changes on hborder/vborder meaning into account
>> > > > > ---
>> > > > >  drivers/gpu/drm/vc4/vc4_plane.c | 49 
>> > > > > -
>> > > > >  1 file changed, 48 insertions(+), 1 deletion(-)
>> > > > > 
>> > > > > diff --git a/drivers/gpu/drm/vc4/vc4_plane.c 
>> > > > > b/drivers/gpu/drm/vc4/vc4_plane.c
>> > > > > index 71d44c357d35..61ed60841cd6 100644
>> > > > > --- a/drivers/gpu/drm/vc4/vc4_plane.c
>> > > > > +++ b/drivers/gpu/drm/vc4/vc4_plane.c
>> > > > > @@ -258,6 +258,49 @@ static u32 vc4_get_scl_field(struct 
>> > > > > drm_plane_state *state, int plane)
>> > > > >  }
>> > > > >  }
>> > > > >  
>> > > > > +static int vc4_plane_underscan_adj(struct drm_plane_state *pstate)
>> > > > > +{
>> > > > > +struct vc4_plane_state *vc4_pstate = to_vc4_plane_state(pstate);
>> > > > > +struct drm_connector_state *conn_state = NULL;
>> > > > > +struct drm_connector *conn;
>> > > > > +struct drm_crtc_state *crtc_state;
>> > > > > +int i;
>> > > > > +
>> > > > > +for_each_new_connector_in_state(pstate->state, conn, 
>> > > > > conn_state, i) {
>> > > > > +if (conn_state->crtc == pstate->crtc)
>> > > > > +break;
>> > > > > +}
>> > > > > +
>> > > > > +if (i == pstate->state->num_connector)
>> > > > > +return 0;
>> > > > > +
>> > > > > +if (conn_state->underscan.mode != DRM_UNDERSCAN_ON)
>> > > > > +return 0;
>> > > > > +
>> > > > > +crtc_state = drm_atomic_get_new_crtc_state(pstate->state,
>> > > > > +   pstate->crtc);
>> > > > > +
>> > > > > +if (conn_state->underscan.hborder >= crtc_state->mode.hdisplay 
>> > > > > ||
>> > > > > +conn_state->underscan.vborder >= crtc_state->mode.vdisplay)
>> > > > > +return -EINVAL;
>> > > > 
>> > > > border * 2 ?  
>> > > 
>> > > Oops, indeed. I'll fix that.
>> > >   
>> > > >   
>> > > > > +
>> > > > > +vc4_pstate->crtc_x += conn_state->underscan.hborder;
>> > > > > +vc4_pstate->crtc_y += conn_state->underscan.vborder;
>> > > > > +vc4_pstate->crtc_w = (vc4_pstate->crtc_w *
>> > > > > +  (crtc_state->mode.hdisplay -
>> > > > > +   (conn_state->underscan.hborder * 2))) /
>> > > > > + crtc_state->mode.hdisplay;
>> > > > > +vc4_pstate->crtc_h = (vc4_pstate->crtc_h *
>> > > > > +  (crtc_state->mode.vdisplay -
>> > > > > +   (conn_state->underscan.vborder * 2))) /
>> > > > > + crtc_state->mode.vdisplay;
>> > > > 
>> > > > So you're now scaling all planes? The code seems to reject scaling for
>> > > > the cursor plane, how are you dealing with that? Or just no cursor
>> > > > allowed when underscanning?  
>> > > 
>> > > No, I just didn't test with a cursor plane. We should probably avoid
>> > > scaling the cursor plane and just adjust its position. Eric any opinion
>> > > on that?  
>> > 
>> > I don't think you can just not scale it. The user asked for the cursor
>> > to be at a specific place with a specific size. Can't just ignore
>> > that and do something else. Also eg. i915 would definitely scale the
>> > cursor since we'd just scale the entire crtc instead of scaling
>> > individual planes. Different drivers doing different things wouldn't
>> > be good.
>> 
>> Except in our case the scaling takes place before the composition, so
>> we don't have a choice.
>
> The choice is to either do what userspace asked, or return an error.
>
>> 
>> > 
>> > >   
>> > > > 
>> > > > I'm also wondering if there's any way we can reconcile these border
>> > > > props with the scaling mode prop, should we ever wish to expose
>> > > > these props on connectors that already have the scaling mode prop.  
>> > > 
>> > > Nouveau seems to expose both, and I don't see why we couldn't.  
>> > 
>> > 

Re: [RFC] Per file OOM badness

[Eric Anholt]

Michel Dänzer  writes:

> On 2018-01-19 11:02 AM, Michel Dänzer wrote:
>> On 2018-01-19 10:58 AM, Christian König wrote:
>>> Am 19.01.2018 um 10:32 schrieb Michel Dänzer:
 On 2018-01-19 09:39 AM, Christian König wrote:
> Am 19.01.2018 um 09:20 schrieb Michal Hocko:
>> OK, in that case I would propose a different approach. We already
>> have rss_stat. So why do not we simply add a new counter there
>> MM_KERNELPAGES and consider those in oom_badness? The rule would be
>> that such a memory is bound to the process life time. I guess we will
>> find more users for this later.
> I already tried that and the problem with that approach is that some
> buffers are not created by the application which actually uses them.
>
> For example X/Wayland is creating and handing out render buffers to
> application which want to use OpenGL.
>
> So the result is when you always account the application who created the
> buffer the OOM killer will certainly reap X/Wayland first. And that is
> exactly what we want to avoid here.
 FWIW, what you describe is true with DRI2, but not with DRI3 or Wayland
 anymore. With DRI3 and Wayland, buffers are allocated by the clients and
 then shared with the X / Wayland server.
>>>
>>> Good point, when I initially looked at that problem DRI3 wasn't widely
>>> used yet.
>>>
 Also, in all cases, the amount of memory allocated for buffers shared
 between DRI/Wayland clients and the server should be relatively small
 compared to the amount of memory allocated for buffers used only locally
 in the client, particularly for clients which create significant memory
 pressure.
>>>
>>> That is unfortunately only partially true. When you have a single
>>> runaway application which tries to allocate everything it would indeed
>>> work as you described.
>>>
>>> But when I tested this a few years ago with X based desktop the
>>> applications which actually used most of the memory where Firefox and
>>> Thunderbird. Unfortunately they never got accounted for that.
>>>
>>> Now, on my current Wayland based desktop it actually doesn't look much
>>> better. Taking a look at radeon_gem_info/amdgpu_gem_info the majority of
>>> all memory was allocated either by gnome-shell or Xwayland.
>> 
>> My guess would be this is due to pixmaps, which allow X clients to cause
>> the X server to allocate essentially unlimited amounts of memory. It's a
>> separate issue, which would require a different solution than what we're
>> discussing in this thread. Maybe something that would allow the X server
>> to tell the kernel that some of the memory it allocates is for the
>> client process.
>
> Of course, such a mechanism could probably be abused to incorrectly
> blame other processes for one's own memory consumption...
>
>
> I'm not sure if the pixmap issue can be solved for the OOM killer. It's
> an X design issue which is fixed with Wayland. So it's probably better
> to ignore it for this discussion.
>
> Also, I really think the issue with DRM buffers being shared between
> processes isn't significant for the OOM killer compared to DRM buffers
> only used in the same process that allocates them. So I suggest focusing
> on the latter.

Agreed.  The 95% case is non-shared buffers, so just don't account for
them and we'll have a solution good enough that we probably never need
to handle the shared case.  On the DRM side, removing buffers from the
accounting once they get shared would be easy.


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

Re: [RFC] Per file OOM badness

[Eric Anholt]

Michal Hocko  writes:

> On Thu 18-01-18 18:00:06, Michal Hocko wrote:
>> On Thu 18-01-18 11:47:48, Andrey Grodzovsky wrote:
>> > Hi, this series is a revised version of an RFC sent by Christian König
>> > a few years ago. The original RFC can be found at 
>> > https://lists.freedesktop.org/archives/dri-devel/2015-September/089778.html
>> > 
>> > This is the same idea and I've just adressed his concern from the original 
>> > RFC 
>> > and switched to a callback into file_ops instead of a new member in struct 
>> > file.
>> 
>> Please add the full description to the cover letter and do not make
>> people hunt links.
>> 
>> Here is the origin cover letter text
>> : I'm currently working on the issue that when device drivers allocate 
>> memory on
>> : behalf of an application the OOM killer usually doesn't knew about that 
>> unless
>> : the application also get this memory mapped into their address space.
>> : 
>> : This is especially annoying for graphics drivers where a lot of the VRAM
>> : usually isn't CPU accessible and so doesn't make sense to map into the
>> : address space of the process using it.
>> : 
>> : The problem now is that when an application starts to use a lot of VRAM 
>> those
>> : buffers objects sooner or later get swapped out to system memory, but when 
>> we
>> : now run into an out of memory situation the OOM killer obviously doesn't 
>> knew
>> : anything about that memory and so usually kills the wrong process.
>
> OK, but how do you attribute that memory to a particular OOM killable
> entity? And how do you actually enforce that those resources get freed
> on the oom killer action?
>
>> : The following set of patches tries to address this problem by introducing 
>> a per
>> : file OOM badness score, which device drivers can use to give the OOM 
>> killer a
>> : hint how many resources are bound to a file descriptor so that it can make
>> : better decisions which process to kill.
>
> But files are not killable, they can be shared... In other words this
> doesn't help the oom killer to make an educated guess at all.

Maybe some more context would help the discussion?

The struct file in patch 3 is the DRM fd.  That's effectively "my
process's interface to talking to the GPU" not "a single GPU resource".
Once that file is closed, all of the process's private, idle GPU buffers
will be immediately freed (this will be most of their allocations), and
some will be freed once the GPU completes some work (this will be most
of the rest of their allocations).

Some GEM BOs won't be freed just by closing the fd, if they've been
shared between processes.  Those are usually about 8-24MB total in a
process, rather than the GBs that modern apps use (or that our testcases
like to allocate and thus trigger oomkilling of the test harness instead
of the offending testcase...)

Even if we just had the private+idle buffers being accounted in OOM
badness, that would be a huge step forward in system reliability.

>> : So question at every one: What do you think about this approach?
>
> I thing is just just wrong semantically. Non-reclaimable memory is a
> pain, especially when there is way too much of it. If you can free that
> memory somehow then you can hook into slab shrinker API and react on the
> memory pressure. If you can account such a memory to a particular
> process and make sure that the consumption is bound by the process life
> time then we can think of an accounting that oom_badness can consider
> when selecting a victim.

For graphics, we can't free most of our memory without also effectively
killing the process.  i915 and vc4 have "purgeable" interfaces for
userspace (on i915 this is exposed all the way to GL applications and is
hooked into shrinker, and on vc4 this is so far just used for
userspace-internal buffer caches to be purged when a CMA allocation
fails).  However, those purgeable pools are expected to be a tiny
fraction of the GPU allocations by the process.


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

Re: [PATCH 0/6 v3] Add ASoC support for AMD Stoney APUs

[Eric Anholt]

Mark Brown  writes:

> [ Unknown signature status ]
> On Wed, Aug 30, 2017 at 09:33:35AM -0400, Alex Deucher wrote:
>
>> Any comments?  Can this patch set go in?  This is the second time I've
>> resent it since the addressing the initial feedback.  Does anyone have
>> a preference on which tree?
>
> You need to get someone from the DRM side to pay attention to the second
> patch and you need to stop resending the first patch since as has been
> pointed out a few times now you need to stop sending the first patch
> which is already in Linus' tree.

Alex *is* from the DRM side and has reviewed that patch.


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

Re: [PATCH 1/4] drm/vc4: Allow vblank_disable_immediate on non-fw-kms.

[Eric Anholt]

Mario Kleiner <mario.kleiner...@gmail.com> writes:

> With instantaneous high precision vblank timestamping
> that updates at leading edge of vblank, the emulated
> "hw vblank counter" from vblank timestamping which
> increments at leading edge of vblank, and reliable
> page flip execution and completion at leading edge
> of vblank, we should meet the requirements for fast
> vblank irq disable/enable.
>
> Testing against rpi-4.12-rc5 Linux kernel with timing
> measurement equipment indicates this works fine,
> so allow immediate vblank disable for power saving.
>
> For debugging in case of unexpected trouble, booting
> with kernel cmdline option drm.vblankoffdelay=0
> would keep vblank irqs on to approximate old behavior.
>
> Signed-off-by: Mario Kleiner <mario.kleiner...@gmail.com>
> Cc: Eric Anholt <e...@anholt.net>

If you can spin this against drm-misc-next instead of the downstream
tree, I can get it applied.


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

Re: [PATCH] drm: add check for plane functions

[Eric Anholt]

Ville Syrjälä  writes:

> On Fri, Mar 17, 2017 at 05:57:52PM +0100, Daniel Vetter wrote:
>> On Fri, Mar 17, 2017 at 01:08:43PM +0200, Ville Syrjälä wrote:
>> > On Fri, Mar 17, 2017 at 03:46:34PM +0530, Shirish S wrote:
>> > > On Fri, Mar 17, 2017 at 3:26 PM, Ville Syrjälä
>> > >  wrote:
>> > > > On Fri, Mar 17, 2017 at 01:25:08PM +0530, Shirish S wrote:
>> > > >> update_plane() and disable_plane() functions
>> > > >> assoiciated with setting plane are called
>> > > >> without any check, causing kernel panic.
>> > > >
>> > > > Why are you registering a plane without the funcs?
>> > > >
>> > > Basically, enabling planes and making them fully functional is
>> > > generally a 2 -step process,
>> > > so i suggest for new drivers wanting to implement/re-design  planes,
>> > > would like to tap
>> > > the flow at enabling(listing caps) and later at ensuring it works.
>> > 
>> > I don't think there's much point in exposing something that
>> > doesn't work. And even if you do, you could always just use
>> > stub functions.
>> 
>> Yes, just wire up stub functions if you want to enable planes with
>> multi-step patch series.
>> 
>> > > I noticed that there is a underlying assumption only for
>> > > plane->(funcs) are implemented, whereas for
>> > > other function for crtc/connector/encoder function calls there is a
>> > > sanity check(or WARN_ON) through out the framework.
>> > > 
>> > > I believe this check wont cause any performance/functional impact.
>> > > Please let me know if am missing anything.
>> > > And further more help developers to focus on enabling planes via
>> > > various tests without causing reboots/system hangs.
>> > 
>> > I don't particularly like adding more unconditional runtime checks
>> > that just to protect developers from themselves. If you really
>> > think there's value in these, then at least add the checks into
>> > the plane init codepath so that it's a one time cost.
>> > 
>> > The same approach could be used for all the other non-optional
>> > hooks. Otherwise the same WARN_ON()s would have to be sprinkled
>> > all over the place, and there's always the risk of missing a few
>> > codepaths that call a specific hook.
>> 
>> I think for these here there's negative value - it allows developers to
>> create completely broken planes. Stub functions really seem like a much
>> better idea.
>
> I was thinking 
>
> drm_whatever_init()
> {
>   if (WARN_ON(!funcs->mandatory_thing))
>   return -EINVAL;
> }
>
> rather than putting the WARN_ON()s around each call of
> funcs->mandatory_thing().
>
> That will fail gracefully (which I guess is what people are after here),
> and gives the developer a clear message what's missing.

Having this in our init functions for funcs and helpers would have saved
me tons of time in vc4 and clcd.


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