* [igt-dev] [PATCH i-g-t 3/3] igt/kms_draw_crc: Test for a working GPU first
From: Chris Wilson @ 2020-02-20 9:41 UTC (permalink / raw)
To: intel-gfx; +Cc: igt-dev
In-Reply-To: <20200220094134.1560435-1-chris@chris-wilson.co.uk>
The draw-method-blt subtests require a working GPU, so create a subtest
group for the draw-methods, and skip the BLT group using
igt_require_gem() in its fixture.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
tests/kms_draw_crc.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/tests/kms_draw_crc.c b/tests/kms_draw_crc.c
index fa7433ab2..917a55432 100644
--- a/tests/kms_draw_crc.c
+++ b/tests/kms_draw_crc.c
@@ -331,14 +331,24 @@ igt_main
for (format_idx = 0; format_idx < N_FORMATS; format_idx++) {
for (method = 0; method < IGT_DRAW_METHOD_COUNT; method++) {
- for (tiling_idx = 0; tiling_idx < N_TILING_METHODS; tiling_idx++) {
- igt_subtest_f("draw-method-%s-%s-%s",
- format_str(format_idx),
- igt_draw_get_method_name(method),
- tiling_str(tiling_idx))
- draw_method_subtest(method, format_idx,
- tilings[tiling_idx]);
- } } }
+ igt_subtest_group {
+ igt_fixture {
+ if (method == IGT_DRAW_BLT)
+ igt_require_gem(drm_fd);
+ }
+
+ for (tiling_idx = 0;
+ tiling_idx < N_TILING_METHODS;
+ tiling_idx++) {
+ igt_subtest_f("draw-method-%s-%s-%s",
+ format_str(format_idx),
+ igt_draw_get_method_name(method),
+ tiling_str(tiling_idx))
+ draw_method_subtest(method,
+ format_idx,
+ tilings[tiling_idx]);
+ }
+ }}}
igt_subtest("fill-fb")
fill_fb_subtest();
--
2.25.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related
* [igt-dev] [PATCH i-g-t 2/3] igt/kms_flip_tiling: Check GEM availability before use
From: Chris Wilson @ 2020-02-20 9:41 UTC (permalink / raw)
To: intel-gfx; +Cc: igt-dev
In-Reply-To: <20200220094134.1560435-1-chris@chris-wilson.co.uk>
We use the GPU to convert between tiling formats, indirectly via the
call to igt_create_pattern_fb. So before we try and execute commands on
the GPU, we should check that the GPU is available.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
tests/kms_flip_tiling.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/tests/kms_flip_tiling.c b/tests/kms_flip_tiling.c
index 17cf816de..1465e73a1 100644
--- a/tests/kms_flip_tiling.c
+++ b/tests/kms_flip_tiling.c
@@ -154,6 +154,7 @@ igt_main
igt_fixture {
data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
data.gen = intel_gen(intel_get_drm_devid(data.drm_fd));
+ igt_require_gem(data.drm_fd);
data.testformat = DRM_FORMAT_XRGB8888;
--
2.25.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related
* [igt-dev] [PATCH i-g-t 1/3] igt/kms_frontbuffer_tracking: Skip over IGT_DRAW_BLT when there's no BLT
From: Chris Wilson @ 2020-02-20 9:41 UTC (permalink / raw)
To: intel-gfx; +Cc: igt-dev
If the blitter is not available, we cannot use it as a source for dirty
rectangles. We shall have to rely on the other engines to create GPU
dirty instead.
v2: Try using lots of subgroup+fixtures
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
tests/kms_frontbuffer_tracking.c | 57 ++++++++++++++++++++++++++++++--
1 file changed, 55 insertions(+), 2 deletions(-)
diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index c4b4af43a..9e00fa2e1 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -3043,6 +3043,8 @@ static void basic_subtest(const struct test_mode *t)
fb1 = params->primary.fb;
for (r = 0, method = 0; method < IGT_DRAW_METHOD_COUNT; method++) {
+ if (method == IGT_DRAW_BLT && !gem_has_blitter(drm.fd))
+ continue;
if (method == IGT_DRAW_MMAP_GTT &&
!gem_has_mappable_ggtt(drm.fd))
continue;
@@ -3275,10 +3277,11 @@ static const char *flip_str(enum flip_type flip)
continue; \
if (!opt.show_hidden && t.fbs == FBS_SHARED && \
(t.plane == PLANE_CUR || t.plane == PLANE_SPR)) \
- continue;
+ continue; \
+ igt_subtest_group {
-#define TEST_MODE_ITER_END } } } } } }
+#define TEST_MODE_ITER_END } } } } } } }
struct option long_options[] = {
{ "no-status-check", 0, 0, 's'},
@@ -3324,6 +3327,10 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
}
TEST_MODE_ITER_BEGIN(t)
+ igt_fixture {
+ if (t.method == IGT_DRAW_BLT)
+ gem_require_blitter(drm.fd);
+ }
igt_subtest_f("%s-%s-%s-%s-%s-draw-%s",
feature_str(t.feature),
pipes_str(t.pipes),
@@ -3340,6 +3347,11 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
(!opt.show_hidden && t.method != IGT_DRAW_BLT))
continue;
+ igt_fixture {
+ if (t.method == IGT_DRAW_BLT)
+ gem_require_blitter(drm.fd);
+ }
+
for (t.flip = 0; t.flip < FLIP_COUNT; t.flip++)
igt_subtest_f("%s-%s-%s-%s-%sflip-%s",
feature_str(t.feature),
@@ -3358,6 +3370,11 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
(t.feature & FEATURE_FBC) == 0)
continue;
+ igt_fixture {
+ if (t.method == IGT_DRAW_BLT)
+ gem_require_blitter(drm.fd);
+ }
+
igt_subtest_f("%s-%s-%s-fliptrack",
feature_str(t.feature),
pipes_str(t.pipes),
@@ -3371,6 +3388,11 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
t.plane == PLANE_PRI)
continue;
+ igt_fixture {
+ if (t.method == IGT_DRAW_BLT)
+ gem_require_blitter(drm.fd);
+ }
+
igt_subtest_f("%s-%s-%s-%s-%s-move",
feature_str(t.feature),
pipes_str(t.pipes),
@@ -3394,6 +3416,11 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
t.plane != PLANE_SPR)
continue;
+ igt_fixture {
+ if (t.method == IGT_DRAW_BLT)
+ gem_require_blitter(drm.fd);
+ }
+
igt_subtest_f("%s-%s-%s-%s-%s-fullscreen",
feature_str(t.feature),
pipes_str(t.pipes),
@@ -3410,6 +3437,11 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
(!opt.show_hidden && t.fbs != FBS_INDIVIDUAL))
continue;
+ igt_fixture {
+ if (t.method == IGT_DRAW_BLT)
+ gem_require_blitter(drm.fd);
+ }
+
igt_subtest_f("%s-%s-%s-%s-multidraw",
feature_str(t.feature),
pipes_str(t.pipes),
@@ -3426,6 +3458,11 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
t.method != IGT_DRAW_MMAP_GTT)
continue;
+ igt_fixture {
+ if (t.method == IGT_DRAW_BLT)
+ gem_require_blitter(drm.fd);
+ }
+
igt_subtest_f("%s-farfromfence", feature_str(t.feature))
farfromfence_subtest(&t);
TEST_MODE_ITER_END
@@ -3437,6 +3474,11 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
t.fbs != FBS_INDIVIDUAL)
continue;
+ igt_fixture {
+ if (t.method == IGT_DRAW_BLT)
+ gem_require_blitter(drm.fd);
+ }
+
for (t.format = 0; t.format < FORMAT_COUNT; t.format++) {
/* Skip what we already tested. */
if (t.format == FORMAT_DEFAULT)
@@ -3456,6 +3498,12 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
t.plane != PLANE_PRI ||
t.method != IGT_DRAW_BLT)
continue;
+
+ igt_fixture {
+ if (t.method == IGT_DRAW_BLT)
+ gem_require_blitter(drm.fd);
+ }
+
igt_subtest_f("%s-%s-scaledprimary",
feature_str(t.feature),
fbs_str(t.fbs))
@@ -3470,6 +3518,11 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
t.method != IGT_DRAW_BLT)
continue;
+ igt_fixture {
+ if (t.method == IGT_DRAW_BLT)
+ gem_require_blitter(drm.fd);
+ }
+
igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature))
modesetfrombusy_subtest(&t);
--
2.25.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related
* Re: vc4 on rpi3 A+
From: Sergey Suloev @ 2020-02-20 9:39 UTC (permalink / raw)
To: Nicolas Saenz Julienne, linux-rpi-kernel
Cc: Stefan Wahren, Noralf Trønnes, bcm-kernel-feedback-list,
linux-arm-kernel
In-Reply-To: <4850fa1006d5a329c2e6d4e61ec6fb9420125414.camel@suse.de>
Ni, Nicolas,
On 2/20/20 12:25 PM, Nicolas Saenz Julienne wrote:
> Hi Sergey,
>
> On Thu, 2020-02-20 at 11:21 +0300, Sergey Suloev wrote:
>> Hello, guys,
>>
>> could anyone clarify the status of vc4 drm support on RPI 3A+ ?
> I don't have one so I can't really tell for that specific board, but I'm going
> to try to reproduce it on a rpi3b.
>
>> I tried to build kernel 5.5 and 5.6-rc2 in 32bit and aarch64
>> configurations with VC4 turned ON but both unsuccessful - vc4 drm driver
>> is listed in memory but not working and not producing any typical DRM
>> log output.
> AFAIK there is a known issue in 5.6-rc2, which has already been addressed[1].
> Note that the driver fails on probe so there is some amount of DRM output.
>
> I tried to reproduce your issue with v5.5, but vc4 seems to probe alright
> (rpi3b+aarch64+defconfig):
>
> [ 15.443047] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <-> 3f902000.hdmi mapping ok
> [ 15.452864] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name!
> [ 15.459836] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
> [ 15.467062] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
> [ 15.478722] vc4-drm soc:gpu: bound 3f004000.txp (ops vc4_txp_ops [vc4])
> [ 15.485749] vc4-drm soc:gpu: bound 3f400000.hvs (ops vc4_hvs_ops [vc4])
> [ 15.499009] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops [vc4])
> [ 15.526217] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops [vc4])
> [ 15.542257] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops [vc4])
> [ 15.560113] vc4-drm soc:gpu: bound 3fc00000.v3d (ops vc4_v3d_ops [vc4])
> [ 15.574684] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> [ 15.587375] [drm] Driver supports precise vblank timestamp query.
> [ 15.606831] [drm] Initialized vc4 0.0.0 20140616 for soc:gpu on minor 0
> [ 15.617505] Console: switching to colour frame buffer device 90x30
> [ 15.627858] vc4-drm soc:gpu: fb0: vc4drmfb frame buffer device
>
> Could it be that you forgot to update the device tree or kernel modules?
>
> Regards,
> Nicolas
>
> [1] https://lkml.kernel.org/lkml/20200219102526.692126-1-jbrunet@baylibre.com/T/
>
thanks for reply.
It works fine with Rpi3B, I agree with you. The issue happens with RPi
3A+ only.
Thank you
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [Intel-gfx] [PATCH 5/5] drm/amdgpu: implement amdgpu_gem_prime_move_notify v2
From: Thomas Hellström (VMware) @ 2020-02-20 9:39 UTC (permalink / raw)
To: Daniel Vetter
Cc: moderated list:DMA BUFFER SHARING FRAMEWORK, Christian König,
intel-gfx, dri-devel, open list:DMA BUFFER SHARING FRAMEWORK
In-Reply-To: <79a0d79f-91bd-2481-740c-20e6c819c7c9@shipmail.org>
On 2/19/20 7:42 AM, Thomas Hellström (VMware) wrote:
> On 2/18/20 10:01 PM, Daniel Vetter wrote:
>> On Tue, Feb 18, 2020 at 9:17 PM Thomas Hellström (VMware)
>> <thomas_os@shipmail.org> wrote:
>>> On 2/17/20 6:55 PM, Daniel Vetter wrote:
>>>> On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote:
>>>>> Implement the importer side of unpinned DMA-buf handling.
>>>>>
>>>>> v2: update page tables immediately
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66
>>>>> ++++++++++++++++++++-
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 ++
>>>>> 2 files changed, 71 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>>> index 770baba621b3..48de7624d49c 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>>> @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct drm_device
>>>>> *dev, struct dma_buf *dma_buf)
>>>>> return ERR_PTR(ret);
>>>>> }
>>>>>
>>>>> +/**
>>>>> + * amdgpu_dma_buf_move_notify - &attach.move_notify implementation
>>>>> + *
>>>>> + * @attach: the DMA-buf attachment
>>>>> + *
>>>>> + * Invalidate the DMA-buf attachment, making sure that the we
>>>>> re-create the
>>>>> + * mapping before the next use.
>>>>> + */
>>>>> +static void
>>>>> +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach)
>>>>> +{
>>>>> + struct drm_gem_object *obj = attach->importer_priv;
>>>>> + struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv);
>>>>> + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>>>>> + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>>>>> + struct ttm_operation_ctx ctx = { false, false };
>>>>> + struct ttm_placement placement = {};
>>>>> + struct amdgpu_vm_bo_base *bo_base;
>>>>> + int r;
>>>>> +
>>>>> + if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM)
>>>>> + return;
>>>>> +
>>>>> + r = ttm_bo_validate(&bo->tbo, &placement, &ctx);
>>>>> + if (r) {
>>>>> + DRM_ERROR("Failed to invalidate DMA-buf import
>>>>> (%d))\n", r);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>>>>> + struct amdgpu_vm *vm = bo_base->vm;
>>>>> + struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
>>>>> +
>>>>> + if (ticket) {
>>>> Yeah so this is kinda why I've been a total pain about the exact
>>>> semantics
>>>> of the move_notify hook. I think we should flat-out require that
>>>> importers
>>>> _always_ have a ticket attach when they call this, and that they
>>>> can cope
>>>> with additional locks being taken (i.e. full EDEADLCK) handling.
>>>>
>>>> Simplest way to force that contract is to add a dummy 2nd ww_mutex
>>>> lock to
>>>> the dma_resv object, which we then can take #ifdef
>>>> CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket).
>>>>
>>>> Now the real disaster is how we handle deadlocks. Two issues:
>>>>
>>>> - Ideally we'd keep any lock we've taken locked until the end, it
>>>> helps
>>>> needless backoffs. I've played around a bit with that but not
>>>> even poc
>>>> level, just an idea:
>>>>
>>>> https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d27be37331255ab7582
>>>>
>>>>
>>>> Idea is essentially to track a list of objects we had to lock
>>>> as part of
>>>> the ttm_bo_validate of the main object.
>>>>
>>>> - Second one is if we get a EDEADLCK on one of these sublocks (like
>>>> the
>>>> one here). We need to pass that up the entire callchain,
>>>> including a
>>>> temporary reference (we have to drop locks to do the
>>>> ww_mutex_lock_slow
>>>> call), and need a custom callback to drop that temporary reference
>>>> (since that's all driver specific, might even be internal
>>>> ww_mutex and
>>>> not anything remotely looking like a normal dma_buf). This
>>>> probably
>>>> needs the exec util helpers from ttm, but at the dma_resv
>>>> level, so that
>>>> we can do something like this:
>>>>
>>>> struct dma_resv_ticket {
>>>> struct ww_acquire_ctx base;
>>>>
>>>> /* can be set by anyone (including other drivers) that got
>>>> hold of
>>>> * this ticket and had to acquire some new lock. This lock
>>>> might
>>>> * protect anything, including driver-internal stuff, and isn't
>>>> * required to be a dma_buf or even just a dma_resv. */
>>>> struct ww_mutex *contended_lock;
>>>>
>>>> /* callback which the driver (which might be a dma-buf exporter
>>>> * and not matching the driver that started this locking
>>>> ticket)
>>>> * sets together with @contended_lock, for the main driver
>>>> to drop
>>>> * when it calls dma_resv_unlock on the contended_lock. */
>>>> void (drop_ref*)(struct ww_mutex *contended_lock);
>>>> };
>>>>
>>>> This is all supremely nasty (also ttm_bo_validate would need to be
>>>> improved to handle these sublocks and random new objects that could
>>>> force
>>>> a ww_mutex_lock_slow).
>>>>
>>> Just a short comment on this:
>>>
>>> Neither the currently used wait-die or the wound-wait algorithm
>>> *strictly* requires a slow lock on the contended lock. For wait-die
>>> it's
>>> just very convenient since it makes us sleep instead of spinning with
>>> -EDEADLK on the contended lock. For wound-wait IIRC one could just
>>> immediately restart the whole locking transaction after an -EDEADLK,
>>> and
>>> the transaction would automatically end up waiting on the contended
>>> lock, provided the mutex lock stealing is not allowed. There is however
>>> a possibility that the transaction will be wounded again on another
>>> lock, taken before the contended lock, but I think there are ways to
>>> improve the wound-wait algorithm to reduce that probability.
>>>
>>> So in short, choosing the wound-wait algorithm instead of wait-die and
>>> perhaps modifying the ww mutex code somewhat would probably help
>>> passing
>>> an -EDEADLK up the call chain without requiring passing the contended
>>> lock, as long as each locker releases its own locks when receiving an
>>> -EDEADLK.
>> Hm this is kinda tempting, since rolling out the full backoff tricker
>> across driver boundaries is going to be real painful.
>>
>> What I'm kinda worried about is the debug/validation checks we're
>> losing with this. The required backoff has this nice property that
>> ww_mutex debug code can check that we've fully unwound everything when
>> we should, that we've blocked on the right lock, and that we're
>> restarting everything without keeling over. Without that I think we
>> could end up with situations where a driver in the middle feels like
>> handling the EDEADLCK, which might go well most of the times (the
>> deadlock will probably be mostly within a given driver, not across).
>> Right up to the point where someone creates a deadlock across drivers,
>> and the lack of full rollback will be felt.
>>
>> So not sure whether we can still keep all these debug/validation
>> checks, or whether this is a step too far towards clever tricks.
>
> I think we could definitely find a way to keep debugging to make sure
> everything is unwound before attempting to restart the locking
> transaction. But the debug check that we're restarting on the
> contended lock only really makes sense for wait-die, (and we could
> easily keep it for wait-die). The lock returning -EDEADLK for
> wound-wait may actually not be the contending lock but an arbitrary
> lock that the wounded transaction attempts to take after it is wounded.
>
> So in the end IMO this is a tradeoff between added (possibly severe)
> locking complexity into dma-buf and not being able to switch back to
> wait-die efficiently if we need / want to do that.
>
> /Thomas
And as a consequence an interface *could* be:
*) We introduce functions
void ww_acquire_relax(struct ww_acquire_ctx *ctx);
int ww_acquire_relax_interruptible(struct ww_acquire_ctx *ctx);
that can be used instead of ww_mutex_lock_slow() in the absence of a
contending lock to avoid spinning on -EDEADLK. While trying to take the
contending lock is probably the best choice there are various second
best approaches that can be explored, for example waiting on the
contending acquire to finish or in the wound-wait case, perhaps do
nothing. These functions will also help us keep the debugging.
*) A function returning -EDEADLK to a caller *must* have already
released its own locks.
*) move_notify() explicitly takes a struct ww_acquire_ctx * to make sure
there is no ambiguity. (I think it would be valuable if we could do the
same for ttm_bo_validate()).
/Thomas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH 5/5] drm/amdgpu: implement amdgpu_gem_prime_move_notify v2
From: Thomas Hellström (VMware) @ 2020-02-20 9:39 UTC (permalink / raw)
To: Daniel Vetter
Cc: Christian König, moderated list:DMA BUFFER SHARING FRAMEWORK,
intel-gfx, dri-devel, open list:DMA BUFFER SHARING FRAMEWORK
In-Reply-To: <79a0d79f-91bd-2481-740c-20e6c819c7c9@shipmail.org>
On 2/19/20 7:42 AM, Thomas Hellström (VMware) wrote:
> On 2/18/20 10:01 PM, Daniel Vetter wrote:
>> On Tue, Feb 18, 2020 at 9:17 PM Thomas Hellström (VMware)
>> <thomas_os@shipmail.org> wrote:
>>> On 2/17/20 6:55 PM, Daniel Vetter wrote:
>>>> On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote:
>>>>> Implement the importer side of unpinned DMA-buf handling.
>>>>>
>>>>> v2: update page tables immediately
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66
>>>>> ++++++++++++++++++++-
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 ++
>>>>> 2 files changed, 71 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>>> index 770baba621b3..48de7624d49c 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>>> @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct drm_device
>>>>> *dev, struct dma_buf *dma_buf)
>>>>> return ERR_PTR(ret);
>>>>> }
>>>>>
>>>>> +/**
>>>>> + * amdgpu_dma_buf_move_notify - &attach.move_notify implementation
>>>>> + *
>>>>> + * @attach: the DMA-buf attachment
>>>>> + *
>>>>> + * Invalidate the DMA-buf attachment, making sure that the we
>>>>> re-create the
>>>>> + * mapping before the next use.
>>>>> + */
>>>>> +static void
>>>>> +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach)
>>>>> +{
>>>>> + struct drm_gem_object *obj = attach->importer_priv;
>>>>> + struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv);
>>>>> + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>>>>> + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>>>>> + struct ttm_operation_ctx ctx = { false, false };
>>>>> + struct ttm_placement placement = {};
>>>>> + struct amdgpu_vm_bo_base *bo_base;
>>>>> + int r;
>>>>> +
>>>>> + if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM)
>>>>> + return;
>>>>> +
>>>>> + r = ttm_bo_validate(&bo->tbo, &placement, &ctx);
>>>>> + if (r) {
>>>>> + DRM_ERROR("Failed to invalidate DMA-buf import
>>>>> (%d))\n", r);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>>>>> + struct amdgpu_vm *vm = bo_base->vm;
>>>>> + struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
>>>>> +
>>>>> + if (ticket) {
>>>> Yeah so this is kinda why I've been a total pain about the exact
>>>> semantics
>>>> of the move_notify hook. I think we should flat-out require that
>>>> importers
>>>> _always_ have a ticket attach when they call this, and that they
>>>> can cope
>>>> with additional locks being taken (i.e. full EDEADLCK) handling.
>>>>
>>>> Simplest way to force that contract is to add a dummy 2nd ww_mutex
>>>> lock to
>>>> the dma_resv object, which we then can take #ifdef
>>>> CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket).
>>>>
>>>> Now the real disaster is how we handle deadlocks. Two issues:
>>>>
>>>> - Ideally we'd keep any lock we've taken locked until the end, it
>>>> helps
>>>> needless backoffs. I've played around a bit with that but not
>>>> even poc
>>>> level, just an idea:
>>>>
>>>> https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d27be37331255ab7582
>>>>
>>>>
>>>> Idea is essentially to track a list of objects we had to lock
>>>> as part of
>>>> the ttm_bo_validate of the main object.
>>>>
>>>> - Second one is if we get a EDEADLCK on one of these sublocks (like
>>>> the
>>>> one here). We need to pass that up the entire callchain,
>>>> including a
>>>> temporary reference (we have to drop locks to do the
>>>> ww_mutex_lock_slow
>>>> call), and need a custom callback to drop that temporary reference
>>>> (since that's all driver specific, might even be internal
>>>> ww_mutex and
>>>> not anything remotely looking like a normal dma_buf). This
>>>> probably
>>>> needs the exec util helpers from ttm, but at the dma_resv
>>>> level, so that
>>>> we can do something like this:
>>>>
>>>> struct dma_resv_ticket {
>>>> struct ww_acquire_ctx base;
>>>>
>>>> /* can be set by anyone (including other drivers) that got
>>>> hold of
>>>> * this ticket and had to acquire some new lock. This lock
>>>> might
>>>> * protect anything, including driver-internal stuff, and isn't
>>>> * required to be a dma_buf or even just a dma_resv. */
>>>> struct ww_mutex *contended_lock;
>>>>
>>>> /* callback which the driver (which might be a dma-buf exporter
>>>> * and not matching the driver that started this locking
>>>> ticket)
>>>> * sets together with @contended_lock, for the main driver
>>>> to drop
>>>> * when it calls dma_resv_unlock on the contended_lock. */
>>>> void (drop_ref*)(struct ww_mutex *contended_lock);
>>>> };
>>>>
>>>> This is all supremely nasty (also ttm_bo_validate would need to be
>>>> improved to handle these sublocks and random new objects that could
>>>> force
>>>> a ww_mutex_lock_slow).
>>>>
>>> Just a short comment on this:
>>>
>>> Neither the currently used wait-die or the wound-wait algorithm
>>> *strictly* requires a slow lock on the contended lock. For wait-die
>>> it's
>>> just very convenient since it makes us sleep instead of spinning with
>>> -EDEADLK on the contended lock. For wound-wait IIRC one could just
>>> immediately restart the whole locking transaction after an -EDEADLK,
>>> and
>>> the transaction would automatically end up waiting on the contended
>>> lock, provided the mutex lock stealing is not allowed. There is however
>>> a possibility that the transaction will be wounded again on another
>>> lock, taken before the contended lock, but I think there are ways to
>>> improve the wound-wait algorithm to reduce that probability.
>>>
>>> So in short, choosing the wound-wait algorithm instead of wait-die and
>>> perhaps modifying the ww mutex code somewhat would probably help
>>> passing
>>> an -EDEADLK up the call chain without requiring passing the contended
>>> lock, as long as each locker releases its own locks when receiving an
>>> -EDEADLK.
>> Hm this is kinda tempting, since rolling out the full backoff tricker
>> across driver boundaries is going to be real painful.
>>
>> What I'm kinda worried about is the debug/validation checks we're
>> losing with this. The required backoff has this nice property that
>> ww_mutex debug code can check that we've fully unwound everything when
>> we should, that we've blocked on the right lock, and that we're
>> restarting everything without keeling over. Without that I think we
>> could end up with situations where a driver in the middle feels like
>> handling the EDEADLCK, which might go well most of the times (the
>> deadlock will probably be mostly within a given driver, not across).
>> Right up to the point where someone creates a deadlock across drivers,
>> and the lack of full rollback will be felt.
>>
>> So not sure whether we can still keep all these debug/validation
>> checks, or whether this is a step too far towards clever tricks.
>
> I think we could definitely find a way to keep debugging to make sure
> everything is unwound before attempting to restart the locking
> transaction. But the debug check that we're restarting on the
> contended lock only really makes sense for wait-die, (and we could
> easily keep it for wait-die). The lock returning -EDEADLK for
> wound-wait may actually not be the contending lock but an arbitrary
> lock that the wounded transaction attempts to take after it is wounded.
>
> So in the end IMO this is a tradeoff between added (possibly severe)
> locking complexity into dma-buf and not being able to switch back to
> wait-die efficiently if we need / want to do that.
>
> /Thomas
And as a consequence an interface *could* be:
*) We introduce functions
void ww_acquire_relax(struct ww_acquire_ctx *ctx);
int ww_acquire_relax_interruptible(struct ww_acquire_ctx *ctx);
that can be used instead of ww_mutex_lock_slow() in the absence of a
contending lock to avoid spinning on -EDEADLK. While trying to take the
contending lock is probably the best choice there are various second
best approaches that can be explored, for example waiting on the
contending acquire to finish or in the wound-wait case, perhaps do
nothing. These functions will also help us keep the debugging.
*) A function returning -EDEADLK to a caller *must* have already
released its own locks.
*) move_notify() explicitly takes a struct ww_acquire_ctx * to make sure
there is no ambiguity. (I think it would be valuable if we could do the
same for ttm_bo_validate()).
/Thomas
^ permalink raw reply
* Re: [PATCH 5/5] drm/amdgpu: implement amdgpu_gem_prime_move_notify v2
From: Thomas Hellström (VMware) @ 2020-02-20 9:39 UTC (permalink / raw)
To: Daniel Vetter
Cc: moderated list:DMA BUFFER SHARING FRAMEWORK, Christian König,
intel-gfx, dri-devel, open list:DMA BUFFER SHARING FRAMEWORK
In-Reply-To: <79a0d79f-91bd-2481-740c-20e6c819c7c9@shipmail.org>
On 2/19/20 7:42 AM, Thomas Hellström (VMware) wrote:
> On 2/18/20 10:01 PM, Daniel Vetter wrote:
>> On Tue, Feb 18, 2020 at 9:17 PM Thomas Hellström (VMware)
>> <thomas_os@shipmail.org> wrote:
>>> On 2/17/20 6:55 PM, Daniel Vetter wrote:
>>>> On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote:
>>>>> Implement the importer side of unpinned DMA-buf handling.
>>>>>
>>>>> v2: update page tables immediately
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66
>>>>> ++++++++++++++++++++-
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 ++
>>>>> 2 files changed, 71 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>>> index 770baba621b3..48de7624d49c 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
>>>>> @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct drm_device
>>>>> *dev, struct dma_buf *dma_buf)
>>>>> return ERR_PTR(ret);
>>>>> }
>>>>>
>>>>> +/**
>>>>> + * amdgpu_dma_buf_move_notify - &attach.move_notify implementation
>>>>> + *
>>>>> + * @attach: the DMA-buf attachment
>>>>> + *
>>>>> + * Invalidate the DMA-buf attachment, making sure that the we
>>>>> re-create the
>>>>> + * mapping before the next use.
>>>>> + */
>>>>> +static void
>>>>> +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach)
>>>>> +{
>>>>> + struct drm_gem_object *obj = attach->importer_priv;
>>>>> + struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv);
>>>>> + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>>>>> + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>>>>> + struct ttm_operation_ctx ctx = { false, false };
>>>>> + struct ttm_placement placement = {};
>>>>> + struct amdgpu_vm_bo_base *bo_base;
>>>>> + int r;
>>>>> +
>>>>> + if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM)
>>>>> + return;
>>>>> +
>>>>> + r = ttm_bo_validate(&bo->tbo, &placement, &ctx);
>>>>> + if (r) {
>>>>> + DRM_ERROR("Failed to invalidate DMA-buf import
>>>>> (%d))\n", r);
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>>>>> + struct amdgpu_vm *vm = bo_base->vm;
>>>>> + struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
>>>>> +
>>>>> + if (ticket) {
>>>> Yeah so this is kinda why I've been a total pain about the exact
>>>> semantics
>>>> of the move_notify hook. I think we should flat-out require that
>>>> importers
>>>> _always_ have a ticket attach when they call this, and that they
>>>> can cope
>>>> with additional locks being taken (i.e. full EDEADLCK) handling.
>>>>
>>>> Simplest way to force that contract is to add a dummy 2nd ww_mutex
>>>> lock to
>>>> the dma_resv object, which we then can take #ifdef
>>>> CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket).
>>>>
>>>> Now the real disaster is how we handle deadlocks. Two issues:
>>>>
>>>> - Ideally we'd keep any lock we've taken locked until the end, it
>>>> helps
>>>> needless backoffs. I've played around a bit with that but not
>>>> even poc
>>>> level, just an idea:
>>>>
>>>> https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d27be37331255ab7582
>>>>
>>>>
>>>> Idea is essentially to track a list of objects we had to lock
>>>> as part of
>>>> the ttm_bo_validate of the main object.
>>>>
>>>> - Second one is if we get a EDEADLCK on one of these sublocks (like
>>>> the
>>>> one here). We need to pass that up the entire callchain,
>>>> including a
>>>> temporary reference (we have to drop locks to do the
>>>> ww_mutex_lock_slow
>>>> call), and need a custom callback to drop that temporary reference
>>>> (since that's all driver specific, might even be internal
>>>> ww_mutex and
>>>> not anything remotely looking like a normal dma_buf). This
>>>> probably
>>>> needs the exec util helpers from ttm, but at the dma_resv
>>>> level, so that
>>>> we can do something like this:
>>>>
>>>> struct dma_resv_ticket {
>>>> struct ww_acquire_ctx base;
>>>>
>>>> /* can be set by anyone (including other drivers) that got
>>>> hold of
>>>> * this ticket and had to acquire some new lock. This lock
>>>> might
>>>> * protect anything, including driver-internal stuff, and isn't
>>>> * required to be a dma_buf or even just a dma_resv. */
>>>> struct ww_mutex *contended_lock;
>>>>
>>>> /* callback which the driver (which might be a dma-buf exporter
>>>> * and not matching the driver that started this locking
>>>> ticket)
>>>> * sets together with @contended_lock, for the main driver
>>>> to drop
>>>> * when it calls dma_resv_unlock on the contended_lock. */
>>>> void (drop_ref*)(struct ww_mutex *contended_lock);
>>>> };
>>>>
>>>> This is all supremely nasty (also ttm_bo_validate would need to be
>>>> improved to handle these sublocks and random new objects that could
>>>> force
>>>> a ww_mutex_lock_slow).
>>>>
>>> Just a short comment on this:
>>>
>>> Neither the currently used wait-die or the wound-wait algorithm
>>> *strictly* requires a slow lock on the contended lock. For wait-die
>>> it's
>>> just very convenient since it makes us sleep instead of spinning with
>>> -EDEADLK on the contended lock. For wound-wait IIRC one could just
>>> immediately restart the whole locking transaction after an -EDEADLK,
>>> and
>>> the transaction would automatically end up waiting on the contended
>>> lock, provided the mutex lock stealing is not allowed. There is however
>>> a possibility that the transaction will be wounded again on another
>>> lock, taken before the contended lock, but I think there are ways to
>>> improve the wound-wait algorithm to reduce that probability.
>>>
>>> So in short, choosing the wound-wait algorithm instead of wait-die and
>>> perhaps modifying the ww mutex code somewhat would probably help
>>> passing
>>> an -EDEADLK up the call chain without requiring passing the contended
>>> lock, as long as each locker releases its own locks when receiving an
>>> -EDEADLK.
>> Hm this is kinda tempting, since rolling out the full backoff tricker
>> across driver boundaries is going to be real painful.
>>
>> What I'm kinda worried about is the debug/validation checks we're
>> losing with this. The required backoff has this nice property that
>> ww_mutex debug code can check that we've fully unwound everything when
>> we should, that we've blocked on the right lock, and that we're
>> restarting everything without keeling over. Without that I think we
>> could end up with situations where a driver in the middle feels like
>> handling the EDEADLCK, which might go well most of the times (the
>> deadlock will probably be mostly within a given driver, not across).
>> Right up to the point where someone creates a deadlock across drivers,
>> and the lack of full rollback will be felt.
>>
>> So not sure whether we can still keep all these debug/validation
>> checks, or whether this is a step too far towards clever tricks.
>
> I think we could definitely find a way to keep debugging to make sure
> everything is unwound before attempting to restart the locking
> transaction. But the debug check that we're restarting on the
> contended lock only really makes sense for wait-die, (and we could
> easily keep it for wait-die). The lock returning -EDEADLK for
> wound-wait may actually not be the contending lock but an arbitrary
> lock that the wounded transaction attempts to take after it is wounded.
>
> So in the end IMO this is a tradeoff between added (possibly severe)
> locking complexity into dma-buf and not being able to switch back to
> wait-die efficiently if we need / want to do that.
>
> /Thomas
And as a consequence an interface *could* be:
*) We introduce functions
void ww_acquire_relax(struct ww_acquire_ctx *ctx);
int ww_acquire_relax_interruptible(struct ww_acquire_ctx *ctx);
that can be used instead of ww_mutex_lock_slow() in the absence of a
contending lock to avoid spinning on -EDEADLK. While trying to take the
contending lock is probably the best choice there are various second
best approaches that can be explored, for example waiting on the
contending acquire to finish or in the wound-wait case, perhaps do
nothing. These functions will also help us keep the debugging.
*) A function returning -EDEADLK to a caller *must* have already
released its own locks.
*) move_notify() explicitly takes a struct ww_acquire_ctx * to make sure
there is no ambiguity. (I think it would be valuable if we could do the
same for ttm_bo_validate()).
/Thomas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH] sched/fair: add !se->on_rq check before dequeue entity
From: Vincent Guittot @ 2020-02-20 9:38 UTC (permalink / raw)
To: qiwuchen55
Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Dietmar Eggemann,
Steven Rostedt, Ben Segall, Mel Gorman, linux-kernel, chenqiwu
In-Reply-To: <1582183784-13502-1-git-send-email-qiwuchen55@gmail.com>
On Thu, 20 Feb 2020 at 08:29, <qiwuchen55@gmail.com> wrote:
>
> From: chenqiwu <chenqiwu@xiaomi.com>
>
> We igonre checking for !se->on_rq condition before dequeue one
> entity from cfs rq. It must be required in case the entity has
> been dequeued.
Do you have a use case that triggers this situation ?
This is the only way to reach this situation seems to be dequeuing a
task on a throttled cfs_rq
>
> Signed-off-by: chenqiwu <chenqiwu@xiaomi.com>
> ---
> kernel/sched/fair.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 3c8a379..945dcaf 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5341,6 +5341,8 @@ static void dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
> bool was_sched_idle = sched_idle_rq(rq);
>
> for_each_sched_entity(se) {
> + if (!se->on_rq)
> + break;
> cfs_rq = cfs_rq_of(se);
> dequeue_entity(cfs_rq, se, flags);
>
> --
> 1.9.1
>
^ permalink raw reply
* [dpdk-dev] [PATCH 2/2] examples/ipsec-gw: fix gcc 10 maybe-uninitialized warning
From: Kevin Traynor @ 2020-02-20 9:37 UTC (permalink / raw)
To: dev; +Cc: Kevin Traynor, stable, konstantin.ananyev, Radu Nicolau,
Akhil Goyal
In-Reply-To: <20200220093744.13925-1-ktraynor@redhat.com>
gcc 10.0.1 reports:
../examples/ipsec-secgw/ipsec_process.c: In function ‘ipsec_process’:
../examples/ipsec-secgw/ipsec_process.c:132:34:
error: ‘grp.m’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
132 | grp[n].cnt = pkts + i - grp[n].m;
| ~~~~~~^~
Fix by initializing the array.
Fixes: 3e5f4625dc17 ("examples/ipsec-secgw: make data-path to use IPsec library")
Cc: stable@dpdk.org
Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
note, commit log violates line length but I didn't want to split warning msg.
Cc: konstantin.ananyev@intel.com
Cc: Radu Nicolau <radu.nicolau@intel.com>
Cc: Akhil Goyal <akhil.goyal@nxp.com>
---
examples/ipsec-secgw/ipsec_process.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/examples/ipsec-secgw/ipsec_process.c b/examples/ipsec-secgw/ipsec_process.c
index bb2f2b82d..0032c5c08 100644
--- a/examples/ipsec-secgw/ipsec_process.c
+++ b/examples/ipsec-secgw/ipsec_process.c
@@ -287,5 +287,5 @@ ipsec_process(struct ipsec_ctx *ctx, struct ipsec_traffic *trf)
struct rte_ipsec_group *pg;
struct rte_ipsec_session *ips;
- struct rte_ipsec_group grp[RTE_DIM(trf->ipsec.pkts)];
+ struct rte_ipsec_group grp[RTE_DIM(trf->ipsec.pkts)] = {};
n = sa_group(trf->ipsec.saptr, trf->ipsec.pkts, grp, trf->ipsec.num);
--
2.21.1
^ permalink raw reply related
* Re: [PATCH v4 2/3] mfd: stm32: Add defines to be used for clkevent purpose
From: Daniel Lezcano @ 2020-02-20 9:38 UTC (permalink / raw)
To: Benjamin Gaignard, lee.jones, robh+dt, mark.rutland,
mcoquelin.stm32, alexandre.torgue, tglx, fabrice.gasnier
Cc: devicetree, linux-stm32, linux-arm-kernel, linux-kernel
In-Reply-To: <20200217134546.14562-3-benjamin.gaignard@st.com>
Hi Lee,
On 17/02/2020 14:45, Benjamin Gaignard wrote:
> Add defines to be able to enable/clear irq and configure one shot mode.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
Are you fine if I pick this patch with the series?
> ---
> version 4:
> - move defines in mfd/stm32-lptimer.h
>
> include/linux/mfd/stm32-lptimer.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/mfd/stm32-lptimer.h b/include/linux/mfd/stm32-lptimer.h
> index 605f62264825..90b20550c1c8 100644
> --- a/include/linux/mfd/stm32-lptimer.h
> +++ b/include/linux/mfd/stm32-lptimer.h
> @@ -27,10 +27,15 @@
> #define STM32_LPTIM_CMPOK BIT(3)
>
> /* STM32_LPTIM_ICR - bit fields */
> +#define STM32_LPTIM_ARRMCF BIT(1)
> #define STM32_LPTIM_CMPOKCF_ARROKCF GENMASK(4, 3)
>
> +/* STM32_LPTIM_IER - bit flieds */
> +#define STM32_LPTIM_ARRMIE BIT(1)
> +
> /* STM32_LPTIM_CR - bit fields */
> #define STM32_LPTIM_CNTSTRT BIT(2)
> +#define STM32_LPTIM_SNGSTRT BIT(1)
> #define STM32_LPTIM_ENABLE BIT(0)
>
> /* STM32_LPTIM_CFGR - bit fields */
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 2/3] mfd: stm32: Add defines to be used for clkevent purpose
From: Daniel Lezcano @ 2020-02-20 9:38 UTC (permalink / raw)
To: Benjamin Gaignard, lee.jones, robh+dt, mark.rutland,
mcoquelin.stm32, alexandre.torgue, tglx, fabrice.gasnier
Cc: devicetree, linux-stm32, linux-arm-kernel, linux-kernel
In-Reply-To: <20200217134546.14562-3-benjamin.gaignard@st.com>
Hi Lee,
On 17/02/2020 14:45, Benjamin Gaignard wrote:
> Add defines to be able to enable/clear irq and configure one shot mode.
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
Are you fine if I pick this patch with the series?
> ---
> version 4:
> - move defines in mfd/stm32-lptimer.h
>
> include/linux/mfd/stm32-lptimer.h | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/include/linux/mfd/stm32-lptimer.h b/include/linux/mfd/stm32-lptimer.h
> index 605f62264825..90b20550c1c8 100644
> --- a/include/linux/mfd/stm32-lptimer.h
> +++ b/include/linux/mfd/stm32-lptimer.h
> @@ -27,10 +27,15 @@
> #define STM32_LPTIM_CMPOK BIT(3)
>
> /* STM32_LPTIM_ICR - bit fields */
> +#define STM32_LPTIM_ARRMCF BIT(1)
> #define STM32_LPTIM_CMPOKCF_ARROKCF GENMASK(4, 3)
>
> +/* STM32_LPTIM_IER - bit flieds */
> +#define STM32_LPTIM_ARRMIE BIT(1)
> +
> /* STM32_LPTIM_CR - bit fields */
> #define STM32_LPTIM_CNTSTRT BIT(2)
> +#define STM32_LPTIM_SNGSTRT BIT(1)
> #define STM32_LPTIM_ENABLE BIT(0)
>
> /* STM32_LPTIM_CFGR - bit fields */
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply
* [dpdk-dev] [PATCH 1/2] net/avp: fix gcc 10 maybe-uninitialized warning
From: Kevin Traynor @ 2020-02-20 9:37 UTC (permalink / raw)
To: dev; +Cc: Kevin Traynor, stable, allain.legacy, Steven Webster, Matt Peters
gcc 10.0.1 reports:
../drivers/net/avp/avp_ethdev.c: In function ‘avp_xmit_scattered_pkts’:
../drivers/net/avp/avp_ethdev.c:1791:24:
warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
1791 | tx_bufs[i] = avp_bufs[count];
| ~~~~~~~~^~~~~~~
../drivers/net/avp/avp_ethdev.c:1791:24:
warning: ‘avp_bufs[count]’ may be used uninitialized in this function [-Wmaybe-uninitialized]
Fix by intializing the array.
Fixes: 295abce2d25b ("net/avp: add packet transmit functions")
Cc: stable@dpdk.org
Signed-off-by: Kevin Traynor <ktraynor@redhat.com>
---
note, commit log violates line length but I didn't want to split warning msg.
Cc: allain.legacy@windriver.com
Cc: Steven Webster <steven.webster@windriver.com>
Cc: Matt Peters <matt.peters@windriver.com>
---
drivers/net/avp/avp_ethdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/avp/avp_ethdev.c b/drivers/net/avp/avp_ethdev.c
index cd747b6be..1abe96ce5 100644
--- a/drivers/net/avp/avp_ethdev.c
+++ b/drivers/net/avp/avp_ethdev.c
@@ -1695,5 +1695,5 @@ avp_xmit_scattered_pkts(void *tx_queue,
{
struct rte_avp_desc *avp_bufs[(AVP_MAX_TX_BURST *
- RTE_AVP_MAX_MBUF_SEGMENTS)];
+ RTE_AVP_MAX_MBUF_SEGMENTS)] = {};
struct avp_queue *txq = (struct avp_queue *)tx_queue;
struct rte_avp_desc *tx_bufs[AVP_MAX_TX_BURST];
--
2.21.1
^ permalink raw reply related
* Re: switching ARC to 64-bit time_t (Re: [RFC v6 07/23] RISC-V: Use 64-bit time_t and off_t for RV32 and RV64)
From: Lukasz Majewski @ 2020-02-20 9:37 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Florian Weimer, Helmut Grohne, GNU C Library, Vineet Gupta,
Palmer Dabbelt, Zong Li, debian-arm, Alistair Francis,
Adhemerval Zanella, Maciej W. Rozycki, Alistair Francis, arcml,
Joseph Myers
In-Reply-To: <CAK8P3a1b73K+RjfHONWLy_dFUucXxwd+0jTnHmkf6YqwRjit4w@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 3197 bytes --]
Hi Arnd,
> On Thu, Feb 20, 2020 at 12:11 AM Lukasz Majewski <lukma@denx.de>
> wrote:
> > > On 2/14/20 2:39 PM, Alistair Francis wrote:
> > > > On Tue, Feb 11, 2020 at 5:30 PM Joseph Myers
> > > An the reason this all works on RISCV is that your kernel doesn't
> > > define __ARCH_WANT_STAT64 -> lacks __NR_statat64 and instead uses
> > > the statx call which does itemized copy and would work fine when
> > > copying from 32-bits time (in kernel) to 64-bits container in
> > > glibc. Is this is right understanding or am I missing something
> > > here.
> > >
> > > How do I build a latest RISCV 32-bit kernel + userland - do you
> > > have a buildroot branch somewhere that I can build / test with
> > > qemu ?
> >
> > Maybe a bit off topic - there is such QEMU and Yocto/OE based test
> > sandbox for ARM32:
> >
> > https://github.com/lmajewski/meta-y2038
> >
> > (the README provides steps for setup).
>
> (continuing off-topic, with debian-arm and Helmut on Cc)
>
> Would it be possible to take a snapshot of your glibc tree
The description of the status of Y2038 supporting glibc on ARM 32 can
be found here [1].
The most recent patches for Y2038 supporting glibc can be always found
in the 'y2038_edge' branch [2].
I also do have a 'warrior' based glibc branch [3], which is a bunch of
hacks to have glibc 2.29 Y2038 supporting. However, my policy now is
"upstream first" - so I would recommend adding any further glibc work
on top of [2].
> and
> start testing this out with debian-rebootstrap [1]?
I've been using OE/Yocto for testing as it allows building glibc
sources for x86_64, x86, x86-x32, arm32 (probably also for ppc32 and
mips - but not tested).
I'm able to use runqemu to test the built glibc with kernel 4.19, 5.1
(for armv7).
This qemu run system can be used to run-test glibc tests on ARM32 with
test-wrapper='/opt/Y2038/glibc/src/scripts/cross-test-ssh.sh
Last but not least - OE/Yocto is used to provide BSP for embedded
systems, so I'm aligned with customers' needs.
However, I did not yet tried debian-rebootstrap. I will look if this
can be reused as well.
>
> Are there any glibc issues that prevent it from working correctly,
I think that the glibc wrappers for most important syscalls are now
converted.
What is missing:
- NTPL (threads)
- stat
- In-glibc test coverage when -D_TIME_BITS=64 is used. I do have
some basic tests [4], but this may be not enough.
> aside from the exact ABI not being final yet?
>
> Arnd
>
> [1] https://wiki.debian.org/HelmutGrohne/rebootstrap
Links:
[1] -
https://github.com/lmajewski/y2038_glibc/commit/4f72f695d1ac428fe945cd7d5e95770180d4a7c1
[2] - https://github.com/lmajewski/y2038_glibc/commits/y2038_edge
[3] -
https://github.com/lmajewski/y2038_glibc/commits/Y2038-2.29-glibc-warrior-01-08-2019
[4] - https://github.com/lmajewski/y2038-tests
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 170 bytes --]
_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
^ permalink raw reply
* Re: Race condition in overlayed qcow2?
From: Vladimir Sementsov-Ogievskiy @ 2020-02-20 9:36 UTC (permalink / raw)
To: dovgaluk; +Cc: kwolf, qemu-devel, mreitz
In-Reply-To: <af246719-910b-1394-2f18-b88e3daa9c81@virtuozzo.com>
20.02.2020 12:05, Vladimir Sementsov-Ogievskiy wrote:
> 20.02.2020 11:31, dovgaluk wrote:
>> Vladimir Sementsov-Ogievskiy писал 2020-02-19 19:07:
>>> 19.02.2020 17:32, dovgaluk wrote:
>>>> I encountered a problem with record/replay of QEMU execution and figured out the following, when
>>>> QEMU is started with one virtual disk connected to the qcow2 image with applied 'snapshot' option.
>>>>
>>>> The patch d710cf575ad5fb3ab329204620de45bfe50caa53 "block/qcow2: introduce parallel subrequest handling in read and write"
>>>> introduces some kind of race condition, which causes difference in the data read from the disk.
>>>>
>>>> I detected this by adding the following code, which logs IO operation checksum. And this checksum may be different in different runs of the same recorded execution.
>>>>
>>>> logging in blk_aio_complete function:
>>>> qemu_log("%"PRId64": blk_aio_complete\n", replay_get_current_icount());
>>>> QEMUIOVector *qiov = acb->rwco.iobuf;
>>>> if (qiov && qiov->iov) {
>>>> size_t i, j;
>>>> uint64_t sum = 0;
>>>> int count = 0;
>>>> for (i = 0 ; i < qiov->niov ; ++i) {
>>>> for (j = 0 ; j < qiov->iov[i].iov_len ; ++j) {
>>>> sum += ((uint8_t*)qiov->iov[i].iov_base)[j];
>>>> ++count;
>>>> }
>>>> }
>>>> qemu_log("--- iobuf offset %"PRIx64" len %x sum: %"PRIx64"\n", acb->rwco.offset, count, sum);
>>>> }
>>>>
>>>> I tried to get rid of aio task by patching qcow2_co_preadv_part:
>>>> ret = qcow2_co_preadv_task(bs, ret, cluster_offset, offset, cur_bytes, qiov, qiov_offset);
>>>>
>>>> That change fixed a bug, but I have no idea what to debug next to figure out the exact reason of the failure.
>>>>
>>>> Do you have any ideas or hints?
>>>>
>>>
>>> Hi!
>>>
>>> Hmm, do mean that read from the disk may return wrong data? It would
>>> be very bad of course :(
>>> Could you provide a reproducer, so that I can look at it and debug?
>>
>> It is just a winxp-32 image. I record the execution and replay it with the following command lines:
>>
>> qemu-system-i386 -icount shift=7,rr=record,rrfile=replay.bin -m 512M -drive file=xp.qcow2,if=none,id=device-34-file,snapshot -drive driver=blkreplay,if=none,image=device-34-file,id=device-34-driver -device ide-hd,drive=device-34-driver,bus=ide.0,id=device-34 -net none
>>
>> qemu-system-i386 -icount shift=7,rr=replay,rrfile=replay.bin -m 512M -drive file=xp.qcow2,if=none,id=device-34-file,snapshot -drive driver=blkreplay,if=none,image=device-34-file,id=device-34-driver -device ide-hd,drive=device-34-driver,bus=ide.0,id=device-34 -net none
>>
>> Replay stalls at some moment due to the non-determinism of the execution (probably caused by the wrong data read).
>
> Hmm.. I tried it (with x86_64 qemu and centos image). I waited for some time for a first command, than Ctrl+C it. After it replay.bin was 4M. Than started the second command. It works, not failing, not finishing. Is it bad? What is expected behavior and what is wrong?
>
>>
>>> What is exactly the case? May be you have other parallel aio
>>> operations to the same region?
>>
>> As far as I understand, all aio operations, initiated by IDE controller, are performed one-by-one.
>> I don't see anything else in the logs.
>>
>>> Ideas to experiment:
>>>
>>> 1. change QCOW2_MAX_WORKERS to 1 or to 2, will it help?
>>
>> 1 or 2 are ok, and 4 or 8 lead to the failures.
>>
>>> 2. understand what is the case in code: is it read from one or several
>>> clusters, is it aligned,
>>> what is the type of clusters, is encryption in use, compression?
>>
>> There is no encryption and I thinks compression is not enabled too.
>> Clusters are read from the temporary overlay:
>>
>> blk_aio_prwv
>> blk_aio_read_entry
>> bdrv_co_preadv_part complete offset: 26300000 qiov_offset: 1c200 len: 1e00
>> bdrv_co_preadv_part complete offset: 24723e00 qiov_offset: 0 len: 1c200
>> bdrv_co_preadv_part complete offset: c0393e00 qiov_offset: 0 len: 1e000
>> bdrv_co_preadv_part complete offset: c0393e00 qiov_offset: 0 len: 1e000
>> bdrv_co_preadv_part complete offset: c0393e00 qiov_offset: 0 len: 1e000
>>
>>
>>> 3. understand what kind of data corruption. What we read instead of
>>> correct data? Just garbage, or may be zeroes, or what..
>>
>> Most bytes are the same, but some are different:
>>
>> < 00 00 00 00 00 00 00 00 00 00 00 00 00 00 02 00
>> < 46 49 4c 45 30 00 03 00 18 d1 33 02 00 00 00 00
>> < 01 00 01 00 38 00 01 00 68 01 00 00 00 04 00 00
>> < 00 00 00 00 00 00 00 00 04 00 00 00 9d 0e 00 00
>> < 02 00 00 00 00 00 00 00 10 00 00 00 60 00 00 00
>> ---
>>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 04 00
>>> 46 49 4c 45 30 00 03 00 86 78 35 03 00 00 00 00
>>> 01 00 01 00 38 00 01 00 60 01 00 00 00 04 00 00
>>> 00 00 00 00 00 00 00 00 04 00 00 00 a1 0e 00 00
>>> 04 00 00 00 00 00 00 00 10 00 00 00 60 00 00 00
>>
>> That is strange. I could think, that it was caused by the bugs in
>> deterministic CPU execution, but the first difference in logs
>> occur in READ operation (I dump read/write buffers in blk_aio_complete).
>>
>
> Aha, yes, looks strange.
>
> Then next steps:
>
> 1. Does problem hit into the same offset every time?
> 2. Do we write to this region before this strange read?
>
> 2.1. If yes, we need to check that we read what we write.. You say you dump buffers
> in blk_aio_complete... I think it would be more reliable to dump at start of
> bdrv_co_pwritev and at end of bdrv_co_preadv. Also, guest may modify its buffers
> during operation which would be strange but possible.
>
> 2.2 If not, hmm...
>
>
Another idea to check: use blkverify
blkverify is filter driver, which on write writes to two separate files and on read
check that it reads the same from these two files (and print error and exits if differs)
So, I imagine creating two overlays by hand, like
qemu-img create -f qcow2 -b /path/to/xp.qcow2 a.qcow2
qemu-img create -f qcow2 -b /path/to/xp.qcow2 b.qcow2
and then stating vm with something like this:
-drive driver=blkverify,raw.filename=/work/a.qcow2,test.file.filename=/work/b.qcow2
--
Best regards,
Vladimir
^ permalink raw reply
* Re: [PATCH v2] mm/hugetlb: Fix file_region entry allocations
From: Miguel Ojeda @ 2020-02-20 9:37 UTC (permalink / raw)
To: Mina Almasry
Cc: Andrew Morton, Linux-MM, linux-kernel, Qian Cai, Mike Kravetz
In-Reply-To: <CAHS8izOBDxoDqyOQ2RkndRJ09yf2qZfoUvKdVqqfMXuKsYRjVg@mail.gmail.com>
On Thu, Feb 20, 2020 at 12:28 AM Mina Almasry <almasrymina@google.com> wrote:
>
> I figured it was related to that, but it's still annoying as
> clang-format works perfectly for me aside from handling the
> list_for_each_* macros. I tried digging into clang-format docs to find
> the setting to add to the .clang-format but I couldn't find one. It
> would be nice if someone fixed that.
Yep, there are a handful of things clang-format does not do as we
would like (yet). Nevertheless, it is always good to hear people is
taking advantage of the config! :)
Cheers,
Miguel
^ permalink raw reply
* Re: [Intel-gfx] [CI v3 1/3] drm/i915: Introduce encoder->compute_config_late()
From: Shankar, Uma @ 2020-02-20 9:36 UTC (permalink / raw)
To: Navare, Manasi D, intel-gfx@lists.freedesktop.org
In-Reply-To: <20200214114126.13192-1-manasi.d.navare@intel.com>
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Manasi
> Navare
> Sent: Friday, February 14, 2020 5:11 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [CI v3 1/3] drm/i915: Introduce encoder->compute_config_late()
>
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Add an optional secondary encoder state compute hook. This gets called after the
> normak .compute_config() has been called for all the encoders in the state. Thus in
> the new hook we can rely on all derived state populated by .compute_config() to be
> already set up. Should be useful for MST and port sync master/slave transcoder
> selection.
Pushed the series to dinq. Thanks for the patches and review.
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 39 +++++++++++++++++++
> .../drm/i915/display/intel_display_types.h | 3 ++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index e09d3c93c52b..ce72551ba16a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13549,6 +13549,35 @@ intel_modeset_pipe_config(struct intel_crtc_state
> *pipe_config)
> return 0;
> }
>
> +static int
> +intel_modeset_pipe_config_late(struct intel_crtc_state *crtc_state) {
> + struct intel_atomic_state *state =
> + to_intel_atomic_state(crtc_state->uapi.state);
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> + struct drm_connector_state *conn_state;
> + struct drm_connector *connector;
> + int i;
> +
> + for_each_new_connector_in_state(&state->base, connector,
> + conn_state, i) {
> + struct intel_encoder *encoder =
> + to_intel_encoder(conn_state->best_encoder);
> + int ret;
> +
> + if (conn_state->crtc != &crtc->base ||
> + !encoder->compute_config_late)
> + continue;
> +
> + ret = encoder->compute_config_late(encoder, crtc_state,
> + conn_state);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> bool intel_fuzzy_clock_check(int clock1, int clock2) {
> int diff;
> @@ -14954,6 +14983,16 @@ static int intel_atomic_check(struct drm_device *dev,
> ret = intel_modeset_pipe_config(new_crtc_state);
> if (ret)
> goto fail;
> + }
> +
> + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> + new_crtc_state, i) {
> + if (!needs_modeset(new_crtc_state))
> + continue;
> +
> + ret = intel_modeset_pipe_config_late(new_crtc_state);
> + if (ret)
> + goto fail;
>
> intel_crtc_check_fastset(old_crtc_state, new_crtc_state);
> }
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 283c622f8ba1..0d8a64305464 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -141,6 +141,9 @@ struct intel_encoder {
> int (*compute_config)(struct intel_encoder *,
> struct intel_crtc_state *,
> struct drm_connector_state *);
> + int (*compute_config_late)(struct intel_encoder *,
> + struct intel_crtc_state *,
> + struct drm_connector_state *);
> void (*update_prepare)(struct intel_atomic_state *,
> struct intel_encoder *,
> struct intel_crtc *);
> --
> 2.19.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH 3/6] dt-bindings: Add Guangdong Neweast Optoelectronics CO. LTD vendor prefix
From: Sam Ravnborg @ 2020-02-20 9:35 UTC (permalink / raw)
To: Vasily Khoruzhick
Cc: Mark Rutland, Neil Armstrong, David Airlie, Linus Walleij,
dri-devel, Andrzej Hajda, Thierry Reding, Laurent Pinchart,
Stephen Rothwell, Samuel Holland, Heiko Stuebner, Chen-Yu Tsai,
Icenowy Zheng, Stephan Gerhold, Jonas Karlman, Torsten Duwe,
Rob Herring, Maxime Ripard, linux-arm-kernel, Jernej Skrabec,
linux-kernel, Mark Brown, Daniel Vetter
In-Reply-To: <20200220083508.792071-4-anarsoul@gmail.com>
Hi Vasily
On Thu, Feb 20, 2020 at 12:35:05AM -0800, Vasily Khoruzhick wrote:
> Add vendor prefix for Guangdong Neweast Optoelectronics CO. LTD
>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 6456a6dfd83d..a390a793422b 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -665,6 +665,8 @@ patternProperties:
> description: Nexbox
> "^nextthing,.*":
> description: Next Thing Co.
> + "^neweast,.*":
> + description: Guangdong Neweast Optoelectronics CO., LT
Alphabetical order.
"new" comes before "nex".
Sam
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 3/6] dt-bindings: Add Guangdong Neweast Optoelectronics CO. LTD vendor prefix
From: Sam Ravnborg @ 2020-02-20 9:35 UTC (permalink / raw)
To: Vasily Khoruzhick
Cc: Mark Rutland, Neil Armstrong, David Airlie, dri-devel,
Andrzej Hajda, Thierry Reding, Laurent Pinchart, Stephen Rothwell,
Samuel Holland, Heiko Stuebner, Chen-Yu Tsai, Icenowy Zheng,
Stephan Gerhold, Jonas Karlman, Torsten Duwe, Rob Herring,
Maxime Ripard, linux-arm-kernel, Jernej Skrabec, linux-kernel,
Mark Brown
In-Reply-To: <20200220083508.792071-4-anarsoul@gmail.com>
Hi Vasily
On Thu, Feb 20, 2020 at 12:35:05AM -0800, Vasily Khoruzhick wrote:
> Add vendor prefix for Guangdong Neweast Optoelectronics CO. LTD
>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 6456a6dfd83d..a390a793422b 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -665,6 +665,8 @@ patternProperties:
> description: Nexbox
> "^nextthing,.*":
> description: Next Thing Co.
> + "^neweast,.*":
> + description: Guangdong Neweast Optoelectronics CO., LT
Alphabetical order.
"new" comes before "nex".
Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH 3/6] dt-bindings: Add Guangdong Neweast Optoelectronics CO. LTD vendor prefix
From: Sam Ravnborg @ 2020-02-20 9:35 UTC (permalink / raw)
To: Vasily Khoruzhick
Cc: Thierry Reding, David Airlie, Daniel Vetter, Rob Herring,
Mark Rutland, Maxime Ripard, Chen-Yu Tsai, Andrzej Hajda,
Neil Armstrong, Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
Icenowy Zheng, Torsten Duwe, Heiko Stuebner, Linus Walleij,
Stephan Gerhold, Mark Brown, Stephen Rothwell, Samuel Holland,
dri-devel, linux-kernel, linux-arm-kernel
In-Reply-To: <20200220083508.792071-4-anarsoul@gmail.com>
Hi Vasily
On Thu, Feb 20, 2020 at 12:35:05AM -0800, Vasily Khoruzhick wrote:
> Add vendor prefix for Guangdong Neweast Optoelectronics CO. LTD
>
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 6456a6dfd83d..a390a793422b 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -665,6 +665,8 @@ patternProperties:
> description: Nexbox
> "^nextthing,.*":
> description: Next Thing Co.
> + "^neweast,.*":
> + description: Guangdong Neweast Optoelectronics CO., LT
Alphabetical order.
"new" comes before "nex".
Sam
^ permalink raw reply
* Re: [PATCH v5 1/3] remoteproc: add support for co-processor loaded and booted before kernel
From: Arnaud POULIQUEN @ 2020-02-20 9:35 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Bjorn Andersson, Rob Herring, Mark Rutland, linux-remoteproc,
devicetree, Ohad Ben-Cohen, Loic PALLARDY, Suman Anna,
Fabien DESSENNE, Linux Kernel Mailing List, linux-stm32
In-Reply-To: <CANLsYkzQz5yyu+KViEL8GwWtp7cfBotS8Fuvs1MJzvYq4LxOig@mail.gmail.com>
On 2/19/20 9:56 PM, Mathieu Poirier wrote:
> Hey Arnaud,
>
> On Tue, 18 Feb 2020 at 10:31, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>>
>> Hi Mathieu, Bjorn,
>>
>> On 2/17/20 7:40 PM, Mathieu Poirier wrote:
>>> On Fri, 14 Feb 2020 at 09:33, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>>>>
>>>> Hi Mathieu,
>>>>
>>>> On 2/13/20 9:08 PM, Mathieu Poirier wrote:
>>>>> Good day,
>>>>>
>>>>> On Tue, Feb 11, 2020 at 06:42:03PM +0100, Arnaud Pouliquen wrote:
>>>>>> From: Loic Pallardy <loic.pallardy@st.com>
>>>>>>
>>>>>> Remote processor could boot independently or be loaded/started before
>>>>>> Linux kernel by bootloader or any firmware.
>>>>>> This patch introduces a new property in rproc core, named skip_fw_load,
>>>>>> to be able to allocate resources and sub-devices like vdev and to
>>>>>> synchronize with current state without loading firmware from file system.
>>>>>> It is platform driver responsibility to implement the right firmware
>>>>>> load ops according to HW specificities.
>>>>>>
>>>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>>>>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>>>>>> ---
>>>>>> drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++------
>>>>>> include/linux/remoteproc.h | 2 +
>>>>>> 2 files changed, 55 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>>>>> index 097f33e4f1f3..876b5420a32b 100644
>>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>>> @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> -/*
>>>>>> - * take a firmware and boot a remote processor with it.
>>>>>> +/**
>>>>>> + * rproc_fw_boot() - boot specified remote processor according to specified
>>>>>> + * firmware
>>>>>> + * @rproc: handle of a remote processor
>>>>>> + * @fw: pointer on firmware to handle
>>>>>> + *
>>>>>> + * Handle resources defined in resource table, load firmware and
>>>>>> + * start remote processor.
>>>>>> + *
>>>>>> + * If firmware pointer fw is NULL, firmware is not handled by remoteproc
>>>>>> + * core, but under the responsibility of platform driver.
>>>>>> + *
>>>>>> + * Returns 0 on success, and an appropriate error value otherwise.
>>>>>> */
>>>>>> static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>>>>> {
>>>>>> @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>>>>> if (ret)
>>>>>> return ret;
>>>>>>
>>>>>> - dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>>>>>> + if (fw)
>>>>>> + dev_info(dev, "Booting fw image %s, size %zd\n", name,
>>>>>> + fw->size);
>>>>>> + else
>>>>>> + dev_info(dev, "Synchronizing with preloaded co-processor\n");
>>>>>>
>>>>>> /*
>>>>>> * if enabling an IOMMU isn't relevant for this rproc, this is
>>>>>> @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work)
>>>>>> * rproc_boot() - boot a remote processor
>>>>>> * @rproc: handle of a remote processor
>>>>>> *
>>>>>> - * Boot a remote processor (i.e. load its firmware, power it on, ...).
>>>>>> + * Boot a remote processor (i.e. load its firmware, power it on, ...) from
>>>>>> + * different contexts:
>>>>>> + * - power off
>>>>>> + * - preloaded firmware
>>>>>> + * - started before kernel execution
>>>>>> + * The different operations are selected thanks to properties defined by
>>>>>> + * platform driver.
>>>>>> *
>>>>>> - * If the remote processor is already powered on, this function immediately
>>>>>> - * returns (successfully).
>>>>>> + * If the remote processor is already powered on at rproc level, this function
>>>>>> + * immediately returns (successfully).
>>>>>> *
>>>>>> * Returns 0 on success, and an appropriate error value otherwise.
>>>>>> */
>>>>>> int rproc_boot(struct rproc *rproc)
>>>>>> {
>>>>>> - const struct firmware *firmware_p;
>>>>>> + const struct firmware *firmware_p = NULL;
>>>>>> struct device *dev;
>>>>>> int ret;
>>>>>>
>>>>>> @@ -1758,11 +1779,20 @@ int rproc_boot(struct rproc *rproc)
>>>>>>
>>>>>> dev_info(dev, "powering up %s\n", rproc->name);
>>>>>>
>>>>>> - /* load firmware */
>>>>>> - ret = request_firmware(&firmware_p, rproc->firmware, dev);
>>>>>> - if (ret < 0) {
>>>>>> - dev_err(dev, "request_firmware failed: %d\n", ret);
>>>>>> - goto downref_rproc;
>>>>>> + if (!rproc->skip_fw_load) {
>>>>>> + /* load firmware */
>>>>>> + ret = request_firmware(&firmware_p, rproc->firmware, dev);
>>>>>> + if (ret < 0) {
>>>>>> + dev_err(dev, "request_firmware failed: %d\n", ret);
>>>>>> + goto downref_rproc;
>>>>>> + }
>>>>>> + } else {
>>>>>> + /*
>>>>>> + * Set firmware name pointer to null as remoteproc core is not
>>>>>> + * in charge of firmware loading
>>>>>> + */
>>>>>> + kfree(rproc->firmware);
>>>>>> + rproc->firmware = NULL;
>>>>>
>>>>> If the MCU with pre-loaded FW crashes request_firmware() in
>>>>> rproc_trigger_recovery() will return an error and rproc_start()
>>>>> never called.
>>>>
>>>> Right, something is missing in the recovery function to prevent request_firmware call if skip_fw_load is set
>>>>
>>>> We also identify an issue if recovery fails:
>>>> In case of recovery issue the rproc state is RPROC_CRASHED, so that it is no more possible to load a new firmware from
>>>> user space.
>>>> This issue is not linked to this patchset. We have patches on our shelves for this.
>>>>
>>>>>> }
>>>>>>
>>>>>> ret = rproc_fw_boot(rproc, firmware_p);
>>>>>> @@ -1916,8 +1946,17 @@ int rproc_add(struct rproc *rproc)
>>>>>> /* create debugfs entries */
>>>>>> rproc_create_debug_dir(rproc);
>>>>>>
>>>>>> - /* if rproc is marked always-on, request it to boot */
>>>>>> - if (rproc->auto_boot) {
>>>>>> + if (rproc->skip_fw_load) {
>>>>>> + /*
>>>>>> + * If rproc is marked already booted, no need to wait
>>>>>> + * for firmware.
>>>>>> + * Just handle associated resources and start sub devices
>>>>>> + */
>>>>>> + ret = rproc_boot(rproc);
>>>>>> + if (ret < 0)
>>>>>> + return ret;
>>>>>> + } else if (rproc->auto_boot) {
>>>>>> + /* if rproc is marked always-on, request it to boot */
>>>>>
>>>>> I spent way too much time staring at this modification... I can't decide if a
>>>>> system where the FW has been pre-loaded should be considered "auto_boot".
>>>>> Indeed the result is the same, i.e the MCU is started at boot time without user
>>>>> intervention.
>>>>
>>>> The main difference is that the firmware is loaded by the Linux remote proc in case of auto-boot.
>>>> In auto-boot mode the remoteproc loads a firmware, on probe, with a specified name without any request from user space.
>>>> One constraint of this mode is that the file system has to be accessible before the rproc probe.
>>>
>>> Indeed, but in both cases the MCU is booted automatically. In one
>>> case the FW is loaded by the framework and in the other it is not. As
>>> such both scenarios are "auto_boot", they simply have different
>>> flavours.
>> Regarding your concerns i would like to propose an alternative that will answer to following use cases:
>>
>> In term of use cases we can start the remote proc firmware in following modes:
>> - auto boot with FW loading, resource table parsing and FW start/stop
>> - auto boot without FW loading, with FW resource table parsing and FW start/stop
>> - auto boot with FW attachment and resource table parsing
>> - boot on userspace request with FW loading, resource table parsing and FW start/stop
>> - boot on userspace request without FW loading, with FW resource table parsing and FW start/stop
>> - boot on userspace request with FW attachment and resource table parsing
>>
>> I considered the recovery covered by these use cases...
>>
>> I tried to concatenate all use case to determine the behavior of the core and platform driver:
>> - "auto-boot" used to decide if boot is from driver or user space request (independently from fw loading and live cycle management)
>> - "skip_fw_load" allows to determine if a firmware has to be loaded or not.
>> - remote Firmware live cycle (start,stop,...) are managed by the platform driver, it would have to determine the manage the remote proc depending on the mode detected.
>>
>> If i apply this for stm32mp1 driver:
>> normal boot( FW started on user space request):
>> - auto-boot = 0
>> - skip_fw_load = 0
>> FW loaded and started by the bootloader
>> - auto-boot = 1
>> - skip_firmware = 1;
>>
>> => on a stop: the "auto-boot" and "skip_firmware flag will be reset by the stm32rproc driver, to allow user space to load a new firmware or reste the system.
>> this is considered as a ack by Bjorn today, if you have an alternative please share.
>
> I wonder if we can achieve the same results without needing
> rproc::skip_fw_load... For cases where the FW would have been loaded
> and the MCU started by another entity we could simply set rproc->state
> = RPROC_RUNNING in the platform driver. That way when the MCU is
> stopped or crashes, there is no flag to reset, rproc->state is simply
> set correctly by the current code.
>
> I would also set auto_boot =1 in order to start the AP synchronisation
> as quickly as possible and add a check in rproc_trigger_auto_boot() to
> see if rproc->state == RPROC_RUNNING. If so simply call rproc_boot()
> where platform specific rproc_ops would be tailored to handle a
> running processor.
Your proposal is interesting, what concerns me is that seems to work only
for a first start. And calling rproc_boot, while state is RPROC_RUNNING seems
pretty strange for me.
Also, as Peng mentions in https://patchwork.kernel.org/patch/11390485/,
the need also exists to skip the load of the firmware on recovery.
How to manage ROM/XIP Firmwares, no handling of the FW code only management
of the live cycle (using sysfs, crash management ....)?
>
> In my opinion the above would represent the state of the MCU rather
> than the state of the FW used by the MCU. It would also provide an
> opening for supporting systems where the MCU is not the life cycle
> manager.
Not sure to catch your point here. By "above" you mention your proposal or mine?
In my opinion, rproc->state already represents the MCU state
what seems missing is the FW state
Could you clarify what you mean by "systems where the MCU is not the life cycle
manager" MCU = rproc framework?
Regards
Arnaud
>
> Let me know what you think...
>
>>
>> I need to rework the patchset in consequence but i would appreciate your feedback on this proposal before, to be sure that i well interpreted your concerns...
>>
>> Regards,
>> Arnaud
>>
>>>
>>>> This is not necessary the case, even if EPROBE_DEFER is used. In this case the driver has to be build as kernel module.
>>>>
>>>> Thanks,
>>>> Arnaud
>>>>>
>>>>> I'd welcome other people's opinion on this.
>>>>>
>>>>>> ret = rproc_trigger_auto_boot(rproc);
>>>>>> if (ret < 0)
>>>>>> return ret;
>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>>> index 16ad66683ad0..4fd5bedab4fa 100644
>>>>>> --- a/include/linux/remoteproc.h
>>>>>> +++ b/include/linux/remoteproc.h
>>>>>> @@ -479,6 +479,7 @@ struct rproc_dump_segment {
>>>>>> * @table_sz: size of @cached_table
>>>>>> * @has_iommu: flag to indicate if remote processor is behind an MMU
>>>>>> * @auto_boot: flag to indicate if remote processor should be auto-started
>>>>>> + * @skip_fw_load: remote processor has been preloaded before start sequence
>>>>>> * @dump_segments: list of segments in the firmware
>>>>>> * @nb_vdev: number of vdev currently handled by rproc
>>>>>> */
>>>>>> @@ -512,6 +513,7 @@ struct rproc {
>>>>>> size_t table_sz;
>>>>>> bool has_iommu;
>>>>>> bool auto_boot;
>>>>>> + bool skip_fw_load;
>>>>>> struct list_head dump_segments;
>>>>>> int nb_vdev;
>>>>>> };
>>>>>> --
>>>>>> 2.17.1
>>>>>>
^ permalink raw reply
* Re: [PATCH v5 1/3] remoteproc: add support for co-processor loaded and booted before kernel
From: Arnaud POULIQUEN @ 2020-02-20 9:35 UTC (permalink / raw)
To: Mathieu Poirier
Cc: Bjorn Andersson, Rob Herring, Mark Rutland, linux-remoteproc,
devicetree, Ohad Ben-Cohen, Loic PALLARDY, Suman Anna,
Fabien DESSENNE, Linux Kernel Mailing List, linux-stm32
In-Reply-To: <CANLsYkzQz5yyu+KViEL8GwWtp7cfBotS8Fuvs1MJzvYq4LxOig@mail.gmail.com>
On 2/19/20 9:56 PM, Mathieu Poirier wrote:
> Hey Arnaud,
>
> On Tue, 18 Feb 2020 at 10:31, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>>
>> Hi Mathieu, Bjorn,
>>
>> On 2/17/20 7:40 PM, Mathieu Poirier wrote:
>>> On Fri, 14 Feb 2020 at 09:33, Arnaud POULIQUEN <arnaud.pouliquen@st.com> wrote:
>>>>
>>>> Hi Mathieu,
>>>>
>>>> On 2/13/20 9:08 PM, Mathieu Poirier wrote:
>>>>> Good day,
>>>>>
>>>>> On Tue, Feb 11, 2020 at 06:42:03PM +0100, Arnaud Pouliquen wrote:
>>>>>> From: Loic Pallardy <loic.pallardy@st.com>
>>>>>>
>>>>>> Remote processor could boot independently or be loaded/started before
>>>>>> Linux kernel by bootloader or any firmware.
>>>>>> This patch introduces a new property in rproc core, named skip_fw_load,
>>>>>> to be able to allocate resources and sub-devices like vdev and to
>>>>>> synchronize with current state without loading firmware from file system.
>>>>>> It is platform driver responsibility to implement the right firmware
>>>>>> load ops according to HW specificities.
>>>>>>
>>>>>> Signed-off-by: Loic Pallardy <loic.pallardy@st.com>
>>>>>> Acked-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>>>>>> ---
>>>>>> drivers/remoteproc/remoteproc_core.c | 67 ++++++++++++++++++++++------
>>>>>> include/linux/remoteproc.h | 2 +
>>>>>> 2 files changed, 55 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
>>>>>> index 097f33e4f1f3..876b5420a32b 100644
>>>>>> --- a/drivers/remoteproc/remoteproc_core.c
>>>>>> +++ b/drivers/remoteproc/remoteproc_core.c
>>>>>> @@ -1358,8 +1358,19 @@ static int rproc_start(struct rproc *rproc, const struct firmware *fw)
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> -/*
>>>>>> - * take a firmware and boot a remote processor with it.
>>>>>> +/**
>>>>>> + * rproc_fw_boot() - boot specified remote processor according to specified
>>>>>> + * firmware
>>>>>> + * @rproc: handle of a remote processor
>>>>>> + * @fw: pointer on firmware to handle
>>>>>> + *
>>>>>> + * Handle resources defined in resource table, load firmware and
>>>>>> + * start remote processor.
>>>>>> + *
>>>>>> + * If firmware pointer fw is NULL, firmware is not handled by remoteproc
>>>>>> + * core, but under the responsibility of platform driver.
>>>>>> + *
>>>>>> + * Returns 0 on success, and an appropriate error value otherwise.
>>>>>> */
>>>>>> static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>>>>> {
>>>>>> @@ -1371,7 +1382,11 @@ static int rproc_fw_boot(struct rproc *rproc, const struct firmware *fw)
>>>>>> if (ret)
>>>>>> return ret;
>>>>>>
>>>>>> - dev_info(dev, "Booting fw image %s, size %zd\n", name, fw->size);
>>>>>> + if (fw)
>>>>>> + dev_info(dev, "Booting fw image %s, size %zd\n", name,
>>>>>> + fw->size);
>>>>>> + else
>>>>>> + dev_info(dev, "Synchronizing with preloaded co-processor\n");
>>>>>>
>>>>>> /*
>>>>>> * if enabling an IOMMU isn't relevant for this rproc, this is
>>>>>> @@ -1718,16 +1733,22 @@ static void rproc_crash_handler_work(struct work_struct *work)
>>>>>> * rproc_boot() - boot a remote processor
>>>>>> * @rproc: handle of a remote processor
>>>>>> *
>>>>>> - * Boot a remote processor (i.e. load its firmware, power it on, ...).
>>>>>> + * Boot a remote processor (i.e. load its firmware, power it on, ...) from
>>>>>> + * different contexts:
>>>>>> + * - power off
>>>>>> + * - preloaded firmware
>>>>>> + * - started before kernel execution
>>>>>> + * The different operations are selected thanks to properties defined by
>>>>>> + * platform driver.
>>>>>> *
>>>>>> - * If the remote processor is already powered on, this function immediately
>>>>>> - * returns (successfully).
>>>>>> + * If the remote processor is already powered on at rproc level, this function
>>>>>> + * immediately returns (successfully).
>>>>>> *
>>>>>> * Returns 0 on success, and an appropriate error value otherwise.
>>>>>> */
>>>>>> int rproc_boot(struct rproc *rproc)
>>>>>> {
>>>>>> - const struct firmware *firmware_p;
>>>>>> + const struct firmware *firmware_p = NULL;
>>>>>> struct device *dev;
>>>>>> int ret;
>>>>>>
>>>>>> @@ -1758,11 +1779,20 @@ int rproc_boot(struct rproc *rproc)
>>>>>>
>>>>>> dev_info(dev, "powering up %s\n", rproc->name);
>>>>>>
>>>>>> - /* load firmware */
>>>>>> - ret = request_firmware(&firmware_p, rproc->firmware, dev);
>>>>>> - if (ret < 0) {
>>>>>> - dev_err(dev, "request_firmware failed: %d\n", ret);
>>>>>> - goto downref_rproc;
>>>>>> + if (!rproc->skip_fw_load) {
>>>>>> + /* load firmware */
>>>>>> + ret = request_firmware(&firmware_p, rproc->firmware, dev);
>>>>>> + if (ret < 0) {
>>>>>> + dev_err(dev, "request_firmware failed: %d\n", ret);
>>>>>> + goto downref_rproc;
>>>>>> + }
>>>>>> + } else {
>>>>>> + /*
>>>>>> + * Set firmware name pointer to null as remoteproc core is not
>>>>>> + * in charge of firmware loading
>>>>>> + */
>>>>>> + kfree(rproc->firmware);
>>>>>> + rproc->firmware = NULL;
>>>>>
>>>>> If the MCU with pre-loaded FW crashes request_firmware() in
>>>>> rproc_trigger_recovery() will return an error and rproc_start()
>>>>> never called.
>>>>
>>>> Right, something is missing in the recovery function to prevent request_firmware call if skip_fw_load is set
>>>>
>>>> We also identify an issue if recovery fails:
>>>> In case of recovery issue the rproc state is RPROC_CRASHED, so that it is no more possible to load a new firmware from
>>>> user space.
>>>> This issue is not linked to this patchset. We have patches on our shelves for this.
>>>>
>>>>>> }
>>>>>>
>>>>>> ret = rproc_fw_boot(rproc, firmware_p);
>>>>>> @@ -1916,8 +1946,17 @@ int rproc_add(struct rproc *rproc)
>>>>>> /* create debugfs entries */
>>>>>> rproc_create_debug_dir(rproc);
>>>>>>
>>>>>> - /* if rproc is marked always-on, request it to boot */
>>>>>> - if (rproc->auto_boot) {
>>>>>> + if (rproc->skip_fw_load) {
>>>>>> + /*
>>>>>> + * If rproc is marked already booted, no need to wait
>>>>>> + * for firmware.
>>>>>> + * Just handle associated resources and start sub devices
>>>>>> + */
>>>>>> + ret = rproc_boot(rproc);
>>>>>> + if (ret < 0)
>>>>>> + return ret;
>>>>>> + } else if (rproc->auto_boot) {
>>>>>> + /* if rproc is marked always-on, request it to boot */
>>>>>
>>>>> I spent way too much time staring at this modification... I can't decide if a
>>>>> system where the FW has been pre-loaded should be considered "auto_boot".
>>>>> Indeed the result is the same, i.e the MCU is started at boot time without user
>>>>> intervention.
>>>>
>>>> The main difference is that the firmware is loaded by the Linux remote proc in case of auto-boot.
>>>> In auto-boot mode the remoteproc loads a firmware, on probe, with a specified name without any request from user space.
>>>> One constraint of this mode is that the file system has to be accessible before the rproc probe.
>>>
>>> Indeed, but in both cases the MCU is booted automatically. In one
>>> case the FW is loaded by the framework and in the other it is not. As
>>> such both scenarios are "auto_boot", they simply have different
>>> flavours.
>> Regarding your concerns i would like to propose an alternative that will answer to following use cases:
>>
>> In term of use cases we can start the remote proc firmware in following modes:
>> - auto boot with FW loading, resource table parsing and FW start/stop
>> - auto boot without FW loading, with FW resource table parsing and FW start/stop
>> - auto boot with FW attachment and resource table parsing
>> - boot on userspace request with FW loading, resource table parsing and FW start/stop
>> - boot on userspace request without FW loading, with FW resource table parsing and FW start/stop
>> - boot on userspace request with FW attachment and resource table parsing
>>
>> I considered the recovery covered by these use cases...
>>
>> I tried to concatenate all use case to determine the behavior of the core and platform driver:
>> - "auto-boot" used to decide if boot is from driver or user space request (independently from fw loading and live cycle management)
>> - "skip_fw_load" allows to determine if a firmware has to be loaded or not.
>> - remote Firmware live cycle (start,stop,...) are managed by the platform driver, it would have to determine the manage the remote proc depending on the mode detected.
>>
>> If i apply this for stm32mp1 driver:
>> normal boot( FW started on user space request):
>> - auto-boot = 0
>> - skip_fw_load = 0
>> FW loaded and started by the bootloader
>> - auto-boot = 1
>> - skip_firmware = 1;
>>
>> => on a stop: the "auto-boot" and "skip_firmware flag will be reset by the stm32rproc driver, to allow user space to load a new firmware or reste the system.
>> this is considered as a ack by Bjorn today, if you have an alternative please share.
>
> I wonder if we can achieve the same results without needing
> rproc::skip_fw_load... For cases where the FW would have been loaded
> and the MCU started by another entity we could simply set rproc->state
> = RPROC_RUNNING in the platform driver. That way when the MCU is
> stopped or crashes, there is no flag to reset, rproc->state is simply
> set correctly by the current code.
>
> I would also set auto_boot =1 in order to start the AP synchronisation
> as quickly as possible and add a check in rproc_trigger_auto_boot() to
> see if rproc->state == RPROC_RUNNING. If so simply call rproc_boot()
> where platform specific rproc_ops would be tailored to handle a
> running processor.
Your proposal is interesting, what concerns me is that seems to work only
for a first start. And calling rproc_boot, while state is RPROC_RUNNING seems
pretty strange for me.
Also, as Peng mentions in https://patchwork.kernel.org/patch/11390485/,
the need also exists to skip the load of the firmware on recovery.
How to manage ROM/XIP Firmwares, no handling of the FW code only management
of the live cycle (using sysfs, crash management ....)?
>
> In my opinion the above would represent the state of the MCU rather
> than the state of the FW used by the MCU. It would also provide an
> opening for supporting systems where the MCU is not the life cycle
> manager.
Not sure to catch your point here. By "above" you mention your proposal or mine?
In my opinion, rproc->state already represents the MCU state
what seems missing is the FW state
Could you clarify what you mean by "systems where the MCU is not the life cycle
manager" MCU = rproc framework?
Regards
Arnaud
>
> Let me know what you think...
>
>>
>> I need to rework the patchset in consequence but i would appreciate your feedback on this proposal before, to be sure that i well interpreted your concerns...
>>
>> Regards,
>> Arnaud
>>
>>>
>>>> This is not necessary the case, even if EPROBE_DEFER is used. In this case the driver has to be build as kernel module.
>>>>
>>>> Thanks,
>>>> Arnaud
>>>>>
>>>>> I'd welcome other people's opinion on this.
>>>>>
>>>>>> ret = rproc_trigger_auto_boot(rproc);
>>>>>> if (ret < 0)
>>>>>> return ret;
>>>>>> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
>>>>>> index 16ad66683ad0..4fd5bedab4fa 100644
>>>>>> --- a/include/linux/remoteproc.h
>>>>>> +++ b/include/linux/remoteproc.h
>>>>>> @@ -479,6 +479,7 @@ struct rproc_dump_segment {
>>>>>> * @table_sz: size of @cached_table
>>>>>> * @has_iommu: flag to indicate if remote processor is behind an MMU
>>>>>> * @auto_boot: flag to indicate if remote processor should be auto-started
>>>>>> + * @skip_fw_load: remote processor has been preloaded before start sequence
>>>>>> * @dump_segments: list of segments in the firmware
>>>>>> * @nb_vdev: number of vdev currently handled by rproc
>>>>>> */
>>>>>> @@ -512,6 +513,7 @@ struct rproc {
>>>>>> size_t table_sz;
>>>>>> bool has_iommu;
>>>>>> bool auto_boot;
>>>>>> + bool skip_fw_load;
>>>>>> struct list_head dump_segments;
>>>>>> int nb_vdev;
>>>>>> };
>>>>>> --
>>>>>> 2.17.1
>>>>>>
^ permalink raw reply
* [PATCH] videobuf2-core.h: improve buf_struct_size documentation
From: Hans Verkuil @ 2020-02-20 9:34 UTC (permalink / raw)
To: Linux Media Mailing List
Explicitly document that the driver-specific buffer structure
must start with the subsystem-specific struct (vb2_v4l2_buffer
in the case of V4L2).
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index a2b2208b02da..f11b96514cf7 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -509,8 +509,11 @@ struct vb2_buf_ops {
* by the vb2 core.
* @buf_struct_size: size of the driver-specific buffer structure;
* "0" indicates the driver doesn't want to use a custom buffer
- * structure type. for example, ``sizeof(struct vb2_v4l2_buffer)``
- * will be used for v4l2.
+ * structure type. In that case a subsystem-specific struct
+ * will be used (in the case of V4L2 that is
+ * ``sizeof(struct vb2_v4l2_buffer)``). The first field of the
+ * driver-specific buffer structure must be the subsystem-specific
+ * struct (vb2_v4l2_buffer in the case of V4L2).
* @timestamp_flags: Timestamp flags; ``V4L2_BUF_FLAG_TIMESTAMP_*`` and
* ``V4L2_BUF_FLAG_TSTAMP_SRC_*``
* @gfp_flags: additional gfp flags used when allocating the buffers.
^ permalink raw reply related
* [PATCH] reset: add stub for device_reset when CONFIG_RESET_CONTROLLER=n
From: Ahmad Fatoum @ 2020-02-20 9:33 UTC (permalink / raw)
To: barebox
Users of an optional device_reset that have CONFIG_RESET_CONTROLLER=n
will trigger compilation errors. Add a static inline stub to fix it.
[ Note: device_reset is unused and unusable because we have no reset
controller implementing .reset. It's available in Linux though and
apparently useful for shared resets, so it's left in ]
Signed-off-by: Ahmad Fatoum <ahmad@a3f.at>
---
include/linux/reset.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/include/linux/reset.h b/include/linux/reset.h
index a22bcf9a5d64..4a92a177bc75 100644
--- a/include/linux/reset.h
+++ b/include/linux/reset.h
@@ -55,6 +55,12 @@ static inline int device_reset_us(struct device_d *dev, int us)
return 0;
}
+static inline int device_reset(struct device_d *dev)
+{
+ WARN_ON(1);
+ return 0;
+}
+
#endif /* CONFIG_RESET_CONTROLLER */
#endif
--
2.20.1
_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
^ permalink raw reply related
* Re: [PATCH] ASoC: soc-pcm: Revert "call snd_soc_component_open/close() once"
From: Kai Vehmanen @ 2020-02-20 9:33 UTC (permalink / raw)
To: Kuninori Morimoto
Cc: alsa-devel, Kai Vehmanen, ranjani.sridharan, pierre-louis.bossart,
broonie, digetx
In-Reply-To: <87blput7hh.wl-kuninori.morimoto.gx@renesas.com>
Hi,
On Thu, 20 Feb 2020, Kuninori Morimoto wrote:
> > ASoC component open/close and snd_soc_component_module_get/put are
> > called independently for each component-substream pair, so the logic
> > in the reverted patch was not sufficient and led to PCM playback and
> > module unload errors.
>
> But, unfortunately I don't want spaghetti error handling code again.
> I think we can solve it if we can *count* open / module ?
> Can you please test this patch ?
I tested and this version works as well, so:
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
... but, but, I have some doubts about th "opened" tracking as a solution.
A single counter will track the overall number of component-substream
combinations, but if we have bugs in calling code and e.g. same
component-substream is passed multiple times to open or close, the
the single counter will go out of sync.
So if the primary problem is the messy rollback in case one open fails,
what if we do the rollback directly in soc_pcm_components_open() and do
not add any additional tracking..?
Let me send a proposal patch for that.
Br, Kai
^ permalink raw reply
* Re: [dpdk-dev] [RFC 00/12] introduce s390x architecture
From: Pradeep Satyanarayana @ 2020-02-20 0:20 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, David Christensen, Vivian Kong, David Wilder
In-Reply-To: <17413425.geO5KgaWL5@xps>
Thomas Monjalon <thomas@monjalon.net> wrote on 02/18/2020 01:03:27 PM:
> From: Thomas Monjalon <thomas@monjalon.net>
> To: Vivian Kong <vivkong@ca.ibm.com>
> Cc: dev@dpdk.org, Pradeep Satyanarayana <pradeep@us.ibm.com>, David
> Christensen <drc@linux.vnet.ibm.com>, David Wilder <wilder@us.ibm.com>
> Date: 02/18/2020 01:03 PM
> Subject: [EXTERNAL] Re: [dpdk-dev] [RFC 00/12] introduce s390x
architecture
>
> Hi,
>
> 09/04/2019 21:06, Vivian Kong:
> > To build and run DPDK on Linux on IBM Z (s390x), big endian support
> > is added and s390x vector intrinics are used in the implementation
> > of DPDK libraries.
>
> What is the status of this work?
> Is it abandoned?
Thomas, Our team is not associated with this effort. Let me reach out to
Vivian and
request her to respond.
Thanks
Pradeep
pradeep@us.ibm.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.