date:20220615

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.

2022-06-15 Thread Christian König

Am 06.06.22 um 13:00 schrieb Bas Nieuwenhuizen:

On Mon, Jun 6, 2022 at 12:35 PM Christian König
wrote:

[SNIP]
That part won't work at all and would cause additional synchronization
problems.

First of all for implicit synced CS we should use READ, not BOOKKEEP.
Because BOOKKEEP would incorrectly be ignored by OpenGL importers. I've
fixed that this causes memory corruption, but it is still nice to avoid.

Yes, what I'm saying is that on implicit sync CS submission should add
READ fences to the dma resv and on explicit sync CS submission should
add BOOKKEEP fences.

No, exactly that is wrong.

Implicit CS submissions should add WRITE fences.

Explicit CS submissions should add READ fences.

Only VM updates should add BOOKKEEP fences.

BOOKKEEP can only be used by VM updates themselves. So that they don't
interfere with CS.

That is the point why we would go BOOKKEEP for explicit sync CS
submissions, no? Explicit submission shouldn't interfere with any
other CS submissions. That includes being totally ignored by GL
importers (if we want to have synchronization there between an
explicit submission and GL, userspace is expected to use Jason's
dmabuf fence import/export IOCTLs)

No, that would break existing DMA-buf rules.

Explicit CS submissions are still a dependency for implicit submissions.

Then the second problem is that the VM IOCTL has absolutely no idea what
the CS IOCTL would be doing. That's why we have added the EXPLICIT sync
flag on the BO.
It doesn't need to? We just use a different sync_mode for BOOKKEEP
fences vs others:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fpatch%2F487887%2F%3Fseries%3D104578%26rev%3D2&data=05%7C01%7Cchristian.koenig%40amd.com%7C81db0fea1c854076fc4408da47abafaa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637901099957139870%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=F72Boaesx83MD2pjGuucA1buawi205XLSsQHg5EH39A%3D&reserved=0

No, exactly that's completely broken.

Regards,
Christian.

(the nice thing about doing it this way is that it is independent of
the IOCTL, i.e. also works for the delayed mapping changes we trigger
on CS submit)

Regards,
Christian.

That should be doable, but then you don't need any of the other changes.

Regards,
Christian.

#1 Is rather easy to fix, you just need to copy all dma_fences from the
page table dma_resv object over to the BOs dma_resv object in the gem
close handler. E.g. exactly what you suggested with the dma_resv_copy
function.

#2 is a nightmare.

We can't move the TLB flush at the end of the unmap operation because on
async TLB flushes are either a bit complicated (double flushes etc..) or
don't even work at all because of hw bugs. So to have a reliable TLB
flush we must make sure that nothing else is ongoing and that means
CS->VM->CS barrier.

We try very hard to circumvent that already on maps by (for example)
using a completely new VMID for CS after the VM map operation.

But for the unmap operation we would need some kind special dma_fence
implementation which would not only wait for all existing dma_fence but
also for the one added until the unmap operation is completed. Cause
otherwise our operation we do at #1 would simply not catch all
dma_fences which have access to the memory.

That's certainly doable, but I think just using the drm_exec stuff I
already came up with is easier.

When we can grab locks for all the BOs involved amdgpu_vm_clear_freed()
goes away and we can keep track of the unmap operations in the bo_va
structure.

With that done you can make the explicit sync you noted in the bo_va
structure and implicit sync when the bo_va structure goes away.

Then the only reason I can see why we would need a CS->VM dependency is
implicit synchronization, and that's what we are trying to avoid here in
the first place.

Regards,
Christian.

To get rid of this barrier you must first fix the part where CS
submissions wait for the VM operation to complete, e.g. the necessity of
the barrier.

I'm working on this for a couple of years now and I'm really running out
of idea how to explain this restriction.

Regards,
Christian.

Re: [RFC PATCH 3/5] drm/amdgpu: Allow explicit sync for VM ops.

2022-06-15 Thread Christian König

Hi Bas,

sorry I totally missed your reply. Just tried to answer your original
questions.

Regards,
Christian.

Am 15.06.22 um 02:40 schrieb Bas Nieuwenhuizen:

Hi Christian,

Friendly ping on the comments here. Are you okay with me re-spinning
the series with a fixed patch 1 or do we need further discussion on
the approach here?

Thanks,
Bas

On Mon, Jun 6, 2022 at 1:00 PM Bas Nieuwenhuizen
wrote:

On Mon, Jun 6, 2022 at 12:35 PM Christian König
wrote:

Am 06.06.22 um 12:30 schrieb Bas Nieuwenhuizen:

On Mon, Jun 6, 2022 at 12:15 PM Christian König
wrote:

Am 03.06.22 um 21:11 schrieb Bas Nieuwenhuizen:

On Fri, Jun 3, 2022 at 8:41 PM Christian König wrote:

Am 03.06.22 um 19:50 schrieb Bas Nieuwenhuizen:

[SNIP]

Yeah, but that's exactly the bubble we try to avoid. Isn't it?

For this series, not really. To clarify there are two sides for
getting GPU bubbles and no overlap:

(1) VM operations implicitly wait for earlier CS submissions
(2) CS submissions implicitly wait for earlier VM operations

Together, these combine to ensure that you get a (potentially small)
bubble any time VM work happens.

Your series (and further ideas) tackles (2), and is a worthwhile thing
to do. However, while writing the userspace for this I noticed this
isn't enough to get rid of all our GPU bubbles. In particular when
doing a non-sparse map of a new BO, that tends to need to be waited on
for the next CS anyway for API semantics. Due to VM operations
happening on a single timeline that means this high priority map can
end up being blocked by earlier sparse maps and hence the bubble in
that case still exists.

So in this series I try to tackle (1) instead. Since GPU work
typically lags behind CPU submissions and VM operations aren't that
slow, we can typically execute VM operations early enough that any
implicit syncs from (2) are less/no issue.

Ok, once more since you don't seem to understand what I want to say: It
isn't possible to fix #1 before you have fixed #2.

The VM unmap operation here is a barrier which divides the CS operations
in a before and after. This is intentional design.

Why is that barrier needed? The two barriers I got and understood and
I think we can deal with:

1) the VM unmap is a barrier between prior CS and later memory free.
2) The TLB flush need to happen between a VM unmap and later CS.

But why do we need the VM unmap to be a strict barrier between prior
CS and later CS?

Exactly because of the two reasons you mentioned.

This is the part I'm not seeing. I get that removing #2 is a
nightmare, which is why I did something that doesn't violate that
constraint.

Like if an explicit CS that was running before the VM operation runs
till after the VM operation (and hence possibly till after the TLB
flush, or otherwise have the TLB flush not apply due to lack of async
TLB flush support), that is not an issue. It might see the state from
before the unmap, or after the unmap, or some intermediate state and
all of those would be okay.

We still get the constraint that the TLB flush happens between the VM
unmap and later CS and hence the unmap is certainly visible to them.

So you basically just want to set the sync mode in
amdgpu_vm_update_range() to AMDGPU_SYNC_EXPLICIT even when it is an unmap?

Yes, with the caveat that I want to do that only for
DMA_RESV_USAGE_BOOKKEEP or higher (i.e. if we submit a CS with
implicit sync we get the old implicit behavior, if we submit a CS with
explicit sync we get the new explicit behavior). The rest of the
series is basically just for enabling explicit sync submissions.

That part won't work at all and would cause additional synchronization
problems.

Yes, what I'm saying is that on implicit sync CS submission should add
READ fences to the dma resv and on explicit sync CS submission should
add BOOKKEEP fences.

BOOKKEEP can only be used by VM updates themselves. So that they don't
interfere with CS.

Then the second problem is that the VM IOCTL has absolutely no idea what
the CS IOCTL would be doing. That's why we have added the EXPLICIT sync
flag on the BO.

It doesn't need to? We just use a different sync_mode for BOOKKEEP
fences vs others:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fpatch%2F487887%2F%3Fseries%3D104578%26rev%3D2&data=05%7C01%7Cchristian.koenig%40amd.com%7C0c76d5c34db846f2fff208da4e67ad7b%7C3dd8961fe4884e608e11a82d994e183d%7

Re: [Intel-gfx] [PATCH 3/3] drm/doc/rfc: VM_BIND uapi definition

2022-06-15 Thread Tvrtko Ursulin




On 14/06/2022 17:42, Niranjana Vishwanathapura wrote:

On Tue, Jun 14, 2022 at 05:07:37PM +0100, Tvrtko Ursulin wrote:


On 14/06/2022 17:02, Tvrtko Ursulin wrote:


On 14/06/2022 16:43, Niranjana Vishwanathapura wrote:

On Tue, Jun 14, 2022 at 08:16:41AM +0100, Tvrtko Ursulin wrote:


On 14/06/2022 00:39, Matthew Brost wrote:

On Mon, Jun 13, 2022 at 07:09:06PM +0100, Tvrtko Ursulin wrote:


On 13/06/2022 18:49, Niranjana Vishwanathapura wrote:

On Mon, Jun 13, 2022 at 05:22:02PM +0100, Tvrtko Ursulin wrote:


On 13/06/2022 16:05, Niranjana Vishwanathapura wrote:

On Mon, Jun 13, 2022 at 09:24:18AM +0100, Tvrtko Ursulin wrote:


On 10/06/2022 17:14, Niranjana Vishwanathapura wrote:
On Fri, Jun 10, 2022 at 05:48:39PM +0300, Lionel Landwerlin 
wrote:

On 10/06/2022 13:37, Tvrtko Ursulin wrote:


On 10/06/2022 08:07, Niranjana Vishwanathapura wrote:

VM_BIND and related uapi definitions

Signed-off-by: Niranjana Vishwanathapura

---
  Documentation/gpu/rfc/i915_vm_bind.h | 490
+++
  1 file changed, 490 insertions(+)
  create mode 100644 Documentation/gpu/rfc/i915_vm_bind.h

diff --git
a/Documentation/gpu/rfc/i915_vm_bind.h
b/Documentation/gpu/rfc/i915_vm_bind.h
new file mode 100644
index ..9fc854969cfb
--- /dev/null
+++ b/Documentation/gpu/rfc/i915_vm_bind.h
@@ -0,0 +1,490 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+/**
+ * DOC: I915_PARAM_HAS_VM_BIND
+ *
+ * VM_BIND feature availability.
+ * See typedef drm_i915_getparam_t param.
+ * bit[0]: If set, VM_BIND is supported, otherwise not.
+ * bits[8-15]: VM_BIND implementation version.
+ * version 0 will not have VM_BIND/UNBIND
timeline fence array support.
+ */
+#define I915_PARAM_HAS_VM_BIND    57
+
+/**
+ * DOC: I915_VM_CREATE_FLAGS_USE_VM_BIND
+ *
+ * Flag to opt-in for VM_BIND mode of binding during VM 
creation.

+ * See struct drm_i915_gem_vm_control flags.
+ *
+ * The older execbuf2 ioctl will not
support VM_BIND mode of operation.
+ * For VM_BIND mode, we have new execbuf3
ioctl which will not accept any
+ * execlist (See struct
drm_i915_gem_execbuffer3 for more details).
+ *
+ */
+#define I915_VM_CREATE_FLAGS_USE_VM_BIND    (1 << 0)
+
+/**
+ * DOC: I915_CONTEXT_CREATE_FLAGS_LONG_RUNNING
+ *
+ * Flag to declare context as long running.
+ * See struct drm_i915_gem_context_create_ext flags.
+ *
+ * Usage of dma-fence expects that they
complete in reasonable amount of time.
+ * Compute on the other hand can be long
running. Hence it is not appropriate
+ * for compute contexts to export request
completion dma-fence to user.
+ * The dma-fence usage will be limited to
in-kernel consumption only.
+ * Compute contexts need to use user/memory fence.
+ *
+ * So, long running contexts do not support output 
fences. Hence,

+ * I915_EXEC_FENCE_SIGNAL (See
&drm_i915_gem_exec_fence.flags) is expected
+ * to be not used. DRM_I915_GEM_WAIT ioctl
call is also not supported for
+ * objects mapped to long running contexts.
+ */
+#define I915_CONTEXT_CREATE_FLAGS_LONG_RUNNING   (1u << 2)
+
+/* VM_BIND related ioctls */
+#define DRM_I915_GEM_VM_BIND    0x3d
+#define DRM_I915_GEM_VM_UNBIND    0x3e
+#define DRM_I915_GEM_EXECBUFFER3    0x3f
+#define DRM_I915_GEM_WAIT_USER_FENCE    0x40
+
+#define DRM_IOCTL_I915_GEM_VM_BIND
DRM_IOWR(DRM_COMMAND_BASE +
DRM_I915_GEM_VM_BIND, struct
drm_i915_gem_vm_bind)
+#define DRM_IOCTL_I915_GEM_VM_UNBIND
DRM_IOWR(DRM_COMMAND_BASE +
DRM_I915_GEM_VM_UNBIND, struct
drm_i915_gem_vm_bind)
+#define DRM_IOCTL_I915_GEM_EXECBUFFER3
DRM_IOWR(DRM_COMMAND_BASE +
DRM_I915_GEM_EXECBUFFER3, struct
drm_i915_gem_execbuffer3)
+#define DRM_IOCTL_I915_GEM_WAIT_USER_FENCE
DRM_IOWR(DRM_COMMAND_BASE +
DRM_I915_GEM_WAIT_USER_FENCE, struct
drm_i915_gem_wait_user_fence)
+
+/**
+ * struct drm_i915_gem_vm_bind - VA to object mapping to 
bind.

+ *
+ * This structure is passed to VM_BIND
ioctl and specifies the mapping of GPU
+ * virtual address (VA) range to the
section of an object that should be bound
+ * in the device page table of the specified address 
space (VM).

+ * The VA range specified must be unique
(ie., not currently bound) and can
+ * be mapped to whole object or a section
of the object (partial binding).
+ * Multiple VA mappings can be created to
the same section of the object
+ * (aliasing).
+ *
+ * The @queue_idx specifies the queue to
use for binding. Same queue can be
+ * used for both VM_BIND and VM_UNBIND
calls. All submitted bind and unbind
+ * operations in a queue are performed in the order of 
submission.

+ *
+ * The @start, @offset and @length should
be 4K page aligned. However the DG2
+ * and XEHPSDV has 64K page size for device
local-memory and has compact page
+ * table. On those platforms, for binding
device local-memory objects, the
+ * @start should be 2M aligned, @offset and
@length should be 64K aligned.
+ * Also, on those platforms, it is not
allowed to bind an device local-memory
+ * object and a system memory object in a
single 2M sect

Re: [PATCH 02/64] drm/crtc: Introduce drmm_crtc_init_with_planes

2022-06-15 Thread Thomas Zimmermann


Hi

Am 14.06.22 um 14:09 schrieb Maxime Ripard:

On Tue, Jun 14, 2022 at 01:47:28PM +0200, Thomas Zimmermann wrote:

Am 14.06.22 um 11:04 schrieb Maxime Ripard:

On Tue, Jun 14, 2022 at 10:29:20AM +0200, Thomas Zimmermann wrote:

Am 14.06.22 um 09:37 schrieb Maxime Ripard:

Hi Thomas,

On Mon, Jun 13, 2022 at 01:23:54PM +0200, Thomas Zimmermann wrote:

Am 10.06.22 um 11:28 schrieb Maxime Ripard:

The DRM-managed function to register a CRTC is
drmm_crtc_alloc_with_planes(), which will allocate the underlying
structure and initialisation the CRTC.

However, we might want to separate the structure creation and the CRTC
initialisation, for example if the structure is shared across multiple
DRM entities, for example an encoder and a connector.

Let's create an helper to only initialise a CRTC that would be passed as
an argument.


Before I review all of thes patches, one question. it's yet not clear to me
why drm_crtc_init_with_planes() wouldn't work. (I know we discussed this on
IRC.)

If you're calling drmm_mode_config_init(), it will clean up all the CRTC,
encoder connector, etc. data structures for you. For CRTC instances in
kmalloced memory, wouldn't it be simpler to put the corresponding kfree into
vc4_crtc_destroy()?


My intent was to remove as much of the lifetime handling and
memory-management from drivers as possible.

My feeling is that while the solution you suggest would definitely work,
it relies on drivers authors to know about this, and make the effort to
convert the drivers themselves. And then we would have to review that,
which we will probably miss (collectively), because it's a bit obscure.

While with either the drmm_alloc_* functions, or the new functions I
introduce there, we can just deprecate the old ones, create a TODO entry
and a coccinelle script and trust that it works properly.


Thanks for explaining the motivation.

I would not want to deprecate any of the existing functions, as they work
for many drivers. The drmm_ functions add additional overhead that not
everyone is willing to pay.


I'm a bit confused. drm_crtc_init_with_planes() at the moment has:

* Note: consider using drmm_crtc_alloc_with_planes() instead of
* drm_crtc_init_with_planes() to let the DRM managed resource infrastructure
* take care of cleanup and deallocation.

Just like drm_encoder_init(), drm_simple_encoder_init() and
drm_universal_plane_init(), so all the functions we have a drmm_* helper
for.

And we have a TODO-list item that heavily hints at using them:
https://dri.freedesktop.org/docs/drm/gpu/todo.html#object-lifetime-fixes

So it looks like we're already well on the deprecation path?


AFAIU that TODO item is about replacing devm_kzalloc() with drmm_kzalloc().
It's not about the plain init functions, such as drm_crtc_init_with_planes()
or drm_universal_plane_init(). Many simple drivers allocate their
modesetting pipeline's components first and then build the pipeline with the
drm_ functions. I don't think we can take that away from them.


Sure, that's exactly what those first patches are about? It allows to
use a DRM managed initialization without disrupting the drivers
structure too much?


The concern I have is that we're adding lots of helpers for all kind of
scenarios and end up with a lot of duplication (and fragmentation among
drivers).


I can see two: whether you want to allocate / init, or just init?
We're not going to have more than that.


For example, drmm_crtc_alloc_with_planes() really isn't much used
by anything. [1]


Not that I disagree here, but it might be that it isn't the most helpful
helper?

In vc4 case, we just can't use it easily.

Our CRTC driver is shared between the "regular" CRTCs in the display
path, and another instance dedicated to the writeback connector.

The shared stuff is initialized through vc4_crtc_init():
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vc4/vc4_crtc.c#L1120

It initializes the structure, set up the planes, etc. Basically
everything that our CRTC controller will be doing, and would be shared
by both cases.

However, since the writeback and regular CRTC structures are different,
we can't really make that function allocate the backing structure
either.


It appears to me that it's a problem with how vc4 organizes its 
pipeline. That's why I suggested to move some of vc4's init code around 
to make such allocations more flexible.




We could do some compiler magic to pass the total size and the offset to
that function, just like what drmm_crtc_alloc_with_planes is doing, but
then we would have the same issue with the writeback stuff that needs to
initialize the encoder and connector.


In vc4_crtc.c it should be possible to use 
drmm_crtc_alloc_with_planes(). In vc4_txp.c, the code apparently 
initializes struct vc4_txp, so it would be better to use a managed 
cleanup of struct vc4_txp.



See, helpers should be useful to many drivers. If we add them, we also 
add a resources and maintenance overhead to our libraries. And right

Re: [PATCH] drm/arm/hdlcd: Take over EFI framebuffer properly

2022-06-15 Thread Thomas Zimmermann


Hi

Am 14.06.22 um 23:06 schrieb Robin Murphy:

On 2022-06-14 14:48, Thomas Zimmermann wrote:

Hi

Am 14.06.22 um 15:04 schrieb Robin Murphy:

The Arm Juno board EDK2 port has provided an EFI GOP display via HDLCD0
for some time now, which works nicely as an early framebuffer. However,
once the HDLCD driver probes and takes over the hardware, it should
take over the logical framebuffer as well, otherwise the now-defunct GOP
device hangs about and virtual console output inevitably disappears into
the wrong place most of the time.

Signed-off-by: Robin Murphy 
---
  drivers/gpu/drm/arm/hdlcd_drv.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c 
b/drivers/gpu/drm/arm/hdlcd_drv.c

index af59077a5481..a5d04884658b 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -331,6 +331,8 @@ static int hdlcd_drm_bind(struct device *dev)
  goto err_vblank;
  }
+    drm_fb_helper_remove_conflicting_framebuffers(NULL, "hdlcd", 
false);

+


In addition to what Javier said, it appears to be too late to call 
this function. If anything her etouches hardware, you might 
accidentally interfere with the EFI-related driver. Rather call it at 
the top of ldlcd_drm_bind().


OK, thanks for the info. I mostly just copied the pattern from the 
simplest-looking other users (sun4i, tegra, vc4) who all seemed to call 
it fairly late, and indeed naively it seemed logical not to do it *too* 
early when there's more chance we might fail to bind and leave the user 
with no framebuffer at all. In particular, waiting until we've bound the 
HDMI encoder seems like a good idea in the case of the Juno board (which 
is the only real HDLCD user), as the I2C bus often gets stuck if the 
System Control Processor is having a bad day. I also don't believe 
there's anything here that would affect efifb more than the fact that 
once the DRM CRTC is alive we simply stop scanning out from the region 
of memory that efifb is managing, but if it's considered good practice 
to do this early then I can certainly make that change too.
We've been struggling with this a bit. If it works reliably, you're 
welcome to leave it where it is.


Historically, most drivers call this function very early. But for error 
recovery it would be better to do it as late as possible.  Ideally, 
drivers would first initialize their DRM software state, then kickout 
the generic driver, and finally take over hardware. But that would 
require a careful review of each driver. :/


Best regards
Thomas



Cheers,
Robin.


--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH] drm/arm/hdlcd: Take over EFI framebuffer properly

2022-06-15 Thread Javier Martinez Canillas

On 6/15/22 09:39, Thomas Zimmermann wrote:
> Hi
> 
> Am 14.06.22 um 23:06 schrieb Robin Murphy:
>> On 2022-06-14 14:48, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 14.06.22 um 15:04 schrieb Robin Murphy:
 The Arm Juno board EDK2 port has provided an EFI GOP display via HDLCD0
 for some time now, which works nicely as an early framebuffer. However,
 once the HDLCD driver probes and takes over the hardware, it should
 take over the logical framebuffer as well, otherwise the now-defunct GOP
 device hangs about and virtual console output inevitably disappears into
 the wrong place most of the time.

 Signed-off-by: Robin Murphy 
 ---
   drivers/gpu/drm/arm/hdlcd_drv.c | 2 ++
   1 file changed, 2 insertions(+)

 diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c 
 b/drivers/gpu/drm/arm/hdlcd_drv.c
 index af59077a5481..a5d04884658b 100644
 --- a/drivers/gpu/drm/arm/hdlcd_drv.c
 +++ b/drivers/gpu/drm/arm/hdlcd_drv.c
 @@ -331,6 +331,8 @@ static int hdlcd_drm_bind(struct device *dev)
   goto err_vblank;
   }
 +    drm_fb_helper_remove_conflicting_framebuffers(NULL, "hdlcd", 
 false);
 +
>>>
>>> In addition to what Javier said, it appears to be too late to call 
>>> this function. If anything her etouches hardware, you might 
>>> accidentally interfere with the EFI-related driver. Rather call it at 
>>> the top of ldlcd_drm_bind().
>>
>> OK, thanks for the info. I mostly just copied the pattern from the 
>> simplest-looking other users (sun4i, tegra, vc4) who all seemed to call 
>> it fairly late, and indeed naively it seemed logical not to do it *too* 
>> early when there's more chance we might fail to bind and leave the user 
>> with no framebuffer at all. In particular, waiting until we've bound the 
>> HDMI encoder seems like a good idea in the case of the Juno board (which 
>> is the only real HDLCD user), as the I2C bus often gets stuck if the 
>> System Control Processor is having a bad day. I also don't believe 
>> there's anything here that would affect efifb more than the fact that 
>> once the DRM CRTC is alive we simply stop scanning out from the region 
>> of memory that efifb is managing, but if it's considered good practice 
>> to do this early then I can certainly make that change too.
> We've been struggling with this a bit. If it works reliably, you're 
> welcome to leave it where it is.
> 
> Historically, most drivers call this function very early. But for error 
> recovery it would be better to do it as late as possible.  Ideally, 
> drivers would first initialize their DRM software state, then kickout 
> the generic driver, and finally take over hardware. But that would 
> require a careful review of each driver. :/
>

We got bug reports on Fedora about regressions caused by the fact that some
programs made the (wrong) assumption that /dev/dri/card0 would be the "main"
display and just hard-coded that path.

But removing the conflicting framebuffers after calling devm_drm_dev_alloc()
breaks this assumption, since the registered device will be /dev/dri/card1.

All this is to say that doing it too late, even if nothing is touching the HW
yet, could still have unexpected consequences across your graphics stack.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH] drm/arm/hdlcd: Take over EFI framebuffer properly

2022-06-15 Thread Thomas Zimmermann




Am 15.06.22 um 09:50 schrieb Javier Martinez Canillas:
[...]

Historically, most drivers call this function very early. But for error
recovery it would be better to do it as late as possible.  Ideally,
drivers would first initialize their DRM software state, then kickout
the generic driver, and finally take over hardware. But that would
require a careful review of each driver. :/



We got bug reports on Fedora about regressions caused by the fact that some
programs made the (wrong) assumption that /dev/dri/card0 would be the "main"
display and just hard-coded that path.


Shh! Don't tell anyone.



But removing the conflicting framebuffers after calling devm_drm_dev_alloc()
breaks this assumption, since the registered device will be /dev/dri/card1.

All this is to say that doing it too late, even if nothing is touching the HW
yet, could still have unexpected consequences across your graphics stack.



--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev


OpenPGP_signature
Description: OpenPGP digital signature

Re: [PATCH] drm/arm/hdlcd: Take over EFI framebuffer properly

2022-06-15 Thread Javier Martinez Canillas

On 6/15/22 09:53, Thomas Zimmermann wrote:
> 
> 
> Am 15.06.22 um 09:50 schrieb Javier Martinez Canillas:
> [...]
>>> Historically, most drivers call this function very early. But for error
>>> recovery it would be better to do it as late as possible.  Ideally,
>>> drivers would first initialize their DRM software state, then kickout
>>> the generic driver, and finally take over hardware. But that would
>>> require a careful review of each driver. :/
>>>
>>
>> We got bug reports on Fedora about regressions caused by the fact that some
>> programs made the (wrong) assumption that /dev/dri/card0 would be the "main"
>> display and just hard-coded that path.
> 
> Shh! Don't tell anyone.
>

:)

What I tried to say is that deciding where to kick out the firmware-provided
framebuffer isn't trivial and would just land the patch as is. At some point
we should probably agree on the best place and audit all the drivers to make
sure that are doing it properly. 

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 02/64] drm/crtc: Introduce drmm_crtc_init_with_planes

2022-06-15 Thread Maxime Ripard

On Wed, Jun 15, 2022 at 09:22:55AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 14.06.22 um 14:09 schrieb Maxime Ripard:
> > On Tue, Jun 14, 2022 at 01:47:28PM +0200, Thomas Zimmermann wrote:
> > > Am 14.06.22 um 11:04 schrieb Maxime Ripard:
> > > > On Tue, Jun 14, 2022 at 10:29:20AM +0200, Thomas Zimmermann wrote:
> > > > > Am 14.06.22 um 09:37 schrieb Maxime Ripard:
> > > > > > Hi Thomas,
> > > > > > 
> > > > > > On Mon, Jun 13, 2022 at 01:23:54PM +0200, Thomas Zimmermann wrote:
> > > > > > > Am 10.06.22 um 11:28 schrieb Maxime Ripard:
> > > > > > > > The DRM-managed function to register a CRTC is
> > > > > > > > drmm_crtc_alloc_with_planes(), which will allocate the 
> > > > > > > > underlying
> > > > > > > > structure and initialisation the CRTC.
> > > > > > > > 
> > > > > > > > However, we might want to separate the structure creation and 
> > > > > > > > the CRTC
> > > > > > > > initialisation, for example if the structure is shared across 
> > > > > > > > multiple
> > > > > > > > DRM entities, for example an encoder and a connector.
> > > > > > > > 
> > > > > > > > Let's create an helper to only initialise a CRTC that would be 
> > > > > > > > passed as
> > > > > > > > an argument.
> > > > > > > 
> > > > > > > Before I review all of thes patches, one question. it's yet not 
> > > > > > > clear to me
> > > > > > > why drm_crtc_init_with_planes() wouldn't work. (I know we 
> > > > > > > discussed this on
> > > > > > > IRC.)
> > > > > > > 
> > > > > > > If you're calling drmm_mode_config_init(), it will clean up all 
> > > > > > > the CRTC,
> > > > > > > encoder connector, etc. data structures for you. For CRTC 
> > > > > > > instances in
> > > > > > > kmalloced memory, wouldn't it be simpler to put the corresponding 
> > > > > > > kfree into
> > > > > > > vc4_crtc_destroy()?
> > > > > > 
> > > > > > My intent was to remove as much of the lifetime handling and
> > > > > > memory-management from drivers as possible.
> > > > > > 
> > > > > > My feeling is that while the solution you suggest would definitely 
> > > > > > work,
> > > > > > it relies on drivers authors to know about this, and make the 
> > > > > > effort to
> > > > > > convert the drivers themselves. And then we would have to review 
> > > > > > that,
> > > > > > which we will probably miss (collectively), because it's a bit 
> > > > > > obscure.
> > > > > > 
> > > > > > While with either the drmm_alloc_* functions, or the new functions I
> > > > > > introduce there, we can just deprecate the old ones, create a TODO 
> > > > > > entry
> > > > > > and a coccinelle script and trust that it works properly.
> > > > > 
> > > > > Thanks for explaining the motivation.
> > > > > 
> > > > > I would not want to deprecate any of the existing functions, as they 
> > > > > work
> > > > > for many drivers. The drmm_ functions add additional overhead that not
> > > > > everyone is willing to pay.
> > > > 
> > > > I'm a bit confused. drm_crtc_init_with_planes() at the moment has:
> > > > 
> > > > * Note: consider using drmm_crtc_alloc_with_planes() instead of
> > > > * drm_crtc_init_with_planes() to let the DRM managed resource 
> > > > infrastructure
> > > > * take care of cleanup and deallocation.
> > > > 
> > > > Just like drm_encoder_init(), drm_simple_encoder_init() and
> > > > drm_universal_plane_init(), so all the functions we have a drmm_* helper
> > > > for.
> > > > 
> > > > And we have a TODO-list item that heavily hints at using them:
> > > > https://dri.freedesktop.org/docs/drm/gpu/todo.html#object-lifetime-fixes
> > > > 
> > > > So it looks like we're already well on the deprecation path?
> > > 
> > > AFAIU that TODO item is about replacing devm_kzalloc() with 
> > > drmm_kzalloc().
> > > It's not about the plain init functions, such as 
> > > drm_crtc_init_with_planes()
> > > or drm_universal_plane_init(). Many simple drivers allocate their
> > > modesetting pipeline's components first and then build the pipeline with 
> > > the
> > > drm_ functions. I don't think we can take that away from them.
> > 
> > Sure, that's exactly what those first patches are about? It allows to
> > use a DRM managed initialization without disrupting the drivers
> > structure too much?
> > 
> > > The concern I have is that we're adding lots of helpers for all kind of
> > > scenarios and end up with a lot of duplication (and fragmentation among
> > > drivers).
> > 
> > I can see two: whether you want to allocate / init, or just init?
> > We're not going to have more than that.
> > 
> > > For example, drmm_crtc_alloc_with_planes() really isn't much used
> > > by anything. [1]
> > 
> > Not that I disagree here, but it might be that it isn't the most helpful
> > helper?
> > 
> > In vc4 case, we just can't use it easily.
> > 
> > Our CRTC driver is shared between the "regular" CRTCs in the display
> > path, and another instance dedicated to the writeback connector.
> > 
> > The shared stuff is initialized through vc4_crtc_init():
> > https://elixir.bootlin.com/linux/la

Re: [PATCH v2 6/7] drm/bridge: anx7625: Register Type-C mode switches

2022-06-15 Thread AngeloGioacchino Del Regno


Il 14/06/22 18:57, Prashant Malani ha scritto:

On Tue, Jun 14, 2022 at 1:18 AM AngeloGioacchino Del Regno
 wrote:


Il 09/06/22 20:09, Prashant Malani ha scritto:

When the DT node has "switches" available, register a Type-C mode-switch
for each listed "switch". This allows the driver to receive state
information about what operating mode a Type-C port and its connected
peripherals are in, as well as status information (like VDOs) related to
that state.

The callback function is currently a stub, but subsequent patches will
implement the required functionality.

Signed-off-by: Prashant Malani 
---

Changes since v2:
- No changes.

   drivers/gpu/drm/bridge/analogix/anx7625.c | 73 +++
   drivers/gpu/drm/bridge/analogix/anx7625.h |  6 ++
   2 files changed, 79 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c 
b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 07ed44c6b839..d41a21103bd3 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -15,6 +15,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 

   #include 
@@ -2581,9 +2582,59 @@ static void anx7625_runtime_disable(void *data)
   pm_runtime_disable(data);
   }

+static int anx7625_typec_mux_set(struct typec_mux_dev *mux,
+  struct typec_mux_state *state)
+{
+ return 0;
+}
+
+static int anx7625_register_mode_switch(struct device *dev, struct device_node 
*node,
+ struct anx7625_data *ctx)
+{
+ struct anx7625_port_data *port_data;
+ struct typec_mux_desc mux_desc = {};
+ char name[32];
+ u32 port_num;
+ int ret;
+
+ ret = of_property_read_u32(node, "reg", &port_num);
+ if (ret)
+ return ret;
+
+ if (port_num >= ctx->num_typec_switches) {
+ dev_err(dev, "Invalid port number specified: %d\n", port_num);
+ return -EINVAL;
+ }
+
+ port_data = &ctx->typec_ports[port_num];
+ port_data->ctx = ctx;
+ mux_desc.fwnode = &node->fwnode;
+ mux_desc.drvdata = port_data;
+ snprintf(name, sizeof(name), "%s-%u", node->name, port_num);
+ mux_desc.name = name;
+ mux_desc.set = anx7625_typec_mux_set;
+
+ port_data->typec_mux = typec_mux_register(dev, &mux_desc);
+ if (IS_ERR(port_data->typec_mux)) {
+ ret = PTR_ERR(port_data->typec_mux);
+ dev_err(dev, "Mode switch register for port %d failed: %d", 
port_num, ret);
+ }


Please return 0 at the end of this function.

 if (IS_ERR()) {
 ..code..
 return ret;
 }

 return 0;
}


May I ask why? We're not missing any return paths. I would rather we
keep it as is (which has the valid return value).



I know that you're not missing any return paths.

That's only because the proposed one is a common pattern in the kernel
and it's only for consistency.

Regards,
Angelo

1 2 >

1 - 100 of 184 matches

Mail list logo