Re: [PATCH] drm/komeda: Fix handling of atomic commits in the atomic_commit_tail hook
Looks good to me - as per log.
On 7/22/22 13:21, Liviu Dudau wrote:
Komeda driver relies on the generic DRM atomic helper functions to handle
commits. It only implements an atomic_commit_tail hook for the
mode_config_helper_funcs and even that one is pretty close to the generic
implementation with the exception of additional dma_fence signalling.
What the generic helper framework doesn't do is waiting for the actual
hardware to signal that the commit parameters have been written into the
appropriate registers. As we signal CRTC events only on the irq handlers,
we need to flush the configuration and wait for the hardware to respond.
Add the Komeda specific implementation for atomic_commit_hw_done() that
flushes and waits for flip done before calling
drm_atomic_helper_commit_hw_done().
The fix was prompted by a patch from Carsten Haitzler where he was trying to
solve the same issue but in a different way that I think can lead to wrong
event signaling to userspace.
Reported-by: Carsten Haitzler
Tested-by: Carsten Haitzler
Signed-off-by: Liviu Dudau
---
.../gpu/drm/arm/display/komeda/komeda_crtc.c | 4 ++--
.../gpu/drm/arm/display/komeda/komeda_kms.c | 21 ++-
.../gpu/drm/arm/display/komeda/komeda_kms.h | 2 ++
3 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 59172acb9738..292f533d8cf0 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -235,7 +235,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
crtc->state->event = NULL;
drm_crtc_send_vblank_event(crtc, event);
} else {
- DRM_WARN("CRTC[%d]: FLIP happen but no pending
commit.\n",
+ DRM_WARN("CRTC[%d]: FLIP happened but no pending
commit.\n",
drm_crtc_index(>base));
}
spin_unlock_irqrestore(>dev->event_lock, flags);
@@ -286,7 +286,7 @@ komeda_crtc_atomic_enable(struct drm_crtc *crtc,
komeda_crtc_do_flush(crtc, old);
}
-static void
+void
komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
struct completion *input_flip_done)
{
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index 93b7f09b96ca..327051bba5b6 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -69,6 +69,25 @@ static const struct drm_driver komeda_kms_driver = {
.minor = 1,
};
+static void komeda_kms_atomic_commit_hw_done(struct drm_atomic_state *state)
+{
+ struct drm_device *dev = state->dev;
+ struct komeda_kms_dev *kms = to_kdev(dev);
+ int i;
+
+ for (i = 0; i < kms->n_crtcs; i++) {
+ struct komeda_crtc *kcrtc = >crtcs[i];
+
+ if (kcrtc->base.state->active) {
+ struct completion *flip_done = NULL;
+ if (kcrtc->base.state->event)
+ flip_done =
kcrtc->base.state->event->base.completion;
+ komeda_crtc_flush_and_wait_for_flip_done(kcrtc,
flip_done);
+ }
+ }
+ drm_atomic_helper_commit_hw_done(state);
+}
+
static void komeda_kms_commit_tail(struct drm_atomic_state *old_state)
{
struct drm_device *dev = old_state->dev;
@@ -81,7 +100,7 @@ static void komeda_kms_commit_tail(struct drm_atomic_state
*old_state)
drm_atomic_helper_commit_modeset_enables(dev, old_state);
- drm_atomic_helper_commit_hw_done(old_state);
+ komeda_kms_atomic_commit_hw_done(old_state);
drm_atomic_helper_wait_for_flip_done(dev, old_state);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index 7889e380ab23..7339339ef6b8 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -183,6 +183,8 @@ void komeda_kms_cleanup_private_objs(struct komeda_kms_dev
*kms);
void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
struct komeda_events *evts);
+void komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
+ struct completion
*input_flip_done);
struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev);
void komeda_kms_detach(struct komeda_kms_dev *kms);
Re: [PATCH 2/3] drm/komeda - At init write GCU control block to handle already on DPU
On 7/8/22 17:07, Liviu Dudau wrote: On Mon, Jun 06, 2022 at 12:47:13PM +0100, carsten.haitz...@foss.arm.com wrote: From: Carsten Haitzler If something has already set up the DPU before the komeda driver comes up, it will fail to init because it was just writing to the SRST bit in the GCU control register and ignoring others. This resulted in TBU bringup stalling and init failing. By writing completely we also set the mode back to 0 (inactive) too and thus TBU bringup works. This is a rather large hammer, tbh. I would like to see if there is a better way of handling the handover from EFIFB that this patch is trying to fix, but I lack an usable plaform for that. It will generate a flicker at module load time, but if users of Morello are happy with that, then Just FYI - it'll flicker anyway as the PHY is external and gets re-initted etc. anyway... This also happens to handle the situation where something goes wrong and you have an already initted komeda sue to a previous module load (and it's still alive and working due to an unclean shutdown). It'll allow you to load the module again :) So it's multi-useful. Acked-by: Liviu Dudau Best regards, Liviu Signed-off-by: Carsten Haitzler --- drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c index 00fa56c29b3e..39618c1a4c81 100644 --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c @@ -309,8 +309,7 @@ static int d71_reset(struct d71_dev *d71) u32 __iomem *gcu = d71->gcu_addr; int ret; - malidp_write32_mask(gcu, BLK_CONTROL, - GCU_CONTROL_SRST, GCU_CONTROL_SRST); + malidp_write32(gcu, BLK_CONTROL, GCU_CONTROL_SRST); ret = dp_wait_cond(!(malidp_read32(gcu, BLK_CONTROL) & GCU_CONTROL_SRST), 100, 1000, 1); -- 2.32.0
Re: [PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up
On 7/11/22 11:13, Liviu Dudau wrote:
On Fri, Jul 08, 2022 at 07:03:37PM +0100, Carsten Haitzler wrote:
On 7/8/22 17:02, Liviu Dudau wrote:
On Mon, Jun 06, 2022 at 12:47:14PM +0100, carsten.haitz...@foss.arm.com wrote:
From: Carsten Haitzler
Hi Carsten,
Sometimes there is an extra dcm crtc state in the pipeline whose
penting vblank event has not been handled yet. We would previously
throw this out and the vblank event never triggers leading to hard
lockups higher up the stack where an expected vlank event never comes
back (screen freezes).
This fixes that by tracking a pending crtc state that needs handling
and handle it producing a vlank event next vblank if it had not
laready been handled before. This fixes the hangs and ensures our
display keeps updating seamlessly and is certainly a much better state
to be in than after some time ending up with a mysteriously frozen
screen and a lot of kernle messages complaining about this too.
Sorry it took me so long to review and reply to this patch, but I had this
nagging
No worries. :)
feeling that the patch is not actually correct so I've tried to track the actual
issue. It turns out that the problem is easy to reproduce in a different setup
with
Mali D71 and it comes from the fact that Komeda doesn't properly wait for the
hardware to signal acceptance of config valid on setting new commits. I have
created
a new patch that I would be happy if you can test to try to fix the actual
issue.
This works (tested).
Thank you very much for testing this! Can I add your Tested-by?
Indeed. Go for it.
One errant "flip without commit":
[9.402559] fbcon: Taking over console
[9.525373] [drm:komeda_print_events [komeda]] *ERROR* err detect: gcu:
MERR, pipes[0]: FLIP, pipes[1]: None
[9.525455] [drm] CRTC[0]: FLIP happened but no pending commit.
[9.542215] Console: switching to colour frame buffer device 240x67
Is this with your 2/3 patch applied and coming out from the firmware having
already
initialised the hardware? If so, the handover probably doesn't quiescence the
interrupts correctly so an interrupt is pending when the kernel driver is
initialised. That's another area worth looking at for the handover purposes.
Yeah. the firmware is not doing a very clean handover - it's leaving the
hardware in whatever state it has. it probably could shut down
interrupts on handover, but not something to worry about in the kernel
driver at this point.
But nothing worrying. It does work, though doesn't compile due to:
drivers/gpu/drm/arm/display/komeda/komeda_kms.c: In function
‘komeda_kms_atomic_commit_hw_done’:
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: error: ‘for’ loop
initial declarations are only allowed in C99 or C11 mode
77 | for (int i = 0; i < kms->n_crtcs; i++) {
| ^~~
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: note: use option
‘-std=c9
’, ‘-std=gnu99’, ‘-std=c11’ or ‘-std=gnu11’ to compile your code
but that was a trivial fixup.
Interesting that I'm not seeing that, probably due to using GCC12? Anyway, I'll
fix
that and send a proper patch.
I tend to use newer gcc's indeed, but I am using gcc11 in this case.
Your commit handler does sit and wait. I guess I avoided that and had it
still deferred and handled next time the vblank interrupt fires. Yours is a
bit shorter and less complex so it wins. :)
Yes, that's the essence of my issue with your patch. Delaying the handling of
the
event until the next vblank means older software that doesn't use the
timestamping
from the vblank event will get wrong framerates (playing video might be
affected).
But this is a rare situation. I certainly was very happy going from "my
entire display locks up and only a reboot really fixes it" to "look ma -
it works and I didn't even see a stuttered frame!" :) But your fix is
better in that regard.
Waiting here when we're also calling drm_atomic_helper_wait_for_flip_done()
after
drm_atomic_helper_commit_hw_done() feels wrong, but then the later is checking
if we
have consumed the event so we have to. Maybe the introduction of the
drm_atomic_helper_fake_vblank() is needed in komeda as well like the generic
commit_tail helper function does? I need to investigate more the next time I
get some
spare cycles on komeda.
I will send a new email with the updated patch and your Tested-by if that's OK.
All happy and fine with that.
Best regards,
Liviu
--8<---
From 76f9e5fed216a815e9eb56152f85454521079f10 Mon Sep 17 00:00:00 2001
From: Liviu Dudau
Date: Fri, 8 Jul 2022 16:39:21 +0100
Subject: [PATCH] drm/komeda: Fix handling of atomic commits in the
atomic_commit_tail hook
Komeda driver relies on the generic DRM atomic helper functions to handle
commits. It only implements an atomic_commit_tail hook for the
mode_config_helper_funcs and even
Re: [PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up
On 7/14/22 13:20, Robin Murphy wrote:
On 2022-07-11 11:13, Liviu Dudau wrote:
[...]
But nothing worrying. It does work, though doesn't compile due to:
drivers/gpu/drm/arm/display/komeda/komeda_kms.c: In function
‘komeda_kms_atomic_commit_hw_done’:
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: error: ‘for’ loop
initial declarations are only allowed in C99 or C11 mode
77 | for (int i = 0; i < kms->n_crtcs; i++) {
| ^~~
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: note: use option
‘-std=c9
’, ‘-std=gnu99’, ‘-std=c11’ or ‘-std=gnu11’ to compile your code
but that was a trivial fixup.
Interesting that I'm not seeing that, probably due to using GCC12?
Anyway, I'll fix
that and send a proper patch.
FWIW we do use -std=gnu11 since 5.18 (see e8c07082a810), but I'm not
entirely sure what the status quo is for using the new features in fixes
which might also warrant backporting to stable. I believe Carsten's
stuck on an older kernel thanks to constraints of the rest of that
project ;)
Not that old - my last sync was like end of April, but i was basing my
commits off a stable kernel release tree (5.17.4), I have multiple
kernels for different purposes and for this stuck to something released
vaguely recently (i synced my tree to latest release before sending off
the patch set). I'm not sure on the kernel policy for the above for (int
i = 0;...) etc. usage. I tend to still be more conservative and keep my
vars at top of the block anyway out of decades of habit.
Re: [PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up
On 7/8/22 17:02, Liviu Dudau wrote:
On Mon, Jun 06, 2022 at 12:47:14PM +0100, carsten.haitz...@foss.arm.com wrote:
From: Carsten Haitzler
Hi Carsten,
Sometimes there is an extra dcm crtc state in the pipeline whose
penting vblank event has not been handled yet. We would previously
throw this out and the vblank event never triggers leading to hard
lockups higher up the stack where an expected vlank event never comes
back (screen freezes).
This fixes that by tracking a pending crtc state that needs handling
and handle it producing a vlank event next vblank if it had not
laready been handled before. This fixes the hangs and ensures our
display keeps updating seamlessly and is certainly a much better state
to be in than after some time ending up with a mysteriously frozen
screen and a lot of kernle messages complaining about this too.
Sorry it took me so long to review and reply to this patch, but I had this
nagging
No worries. :)
feeling that the patch is not actually correct so I've tried to track the actual
issue. It turns out that the problem is easy to reproduce in a different setup
with
Mali D71 and it comes from the fact that Komeda doesn't properly wait for the
hardware to signal acceptance of config valid on setting new commits. I have
created
a new patch that I would be happy if you can test to try to fix the actual
issue.
This works (tested). One errant "flip without commit":
[9.402559] fbcon: Taking over console
[9.525373] [drm:komeda_print_events [komeda]] *ERROR* err detect:
gcu: MERR, pipes[0]: FLIP, pipes[1]: None
[9.525455] [drm] CRTC[0]: FLIP happened but no pending commit.
[9.542215] Console: switching to colour frame buffer device 240x67
But nothing worrying. It does work, though doesn't compile due to:
drivers/gpu/drm/arm/display/komeda/komeda_kms.c: In function
‘komeda_kms_atomic_commit_hw_done’:
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: error: ‘for’ loop
initial declarations are only allowed in C99 or C11 mode
77 | for (int i = 0; i < kms->n_crtcs; i++) {
| ^~~
drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: note: use option
‘-std=c9
’, ‘-std=gnu99’, ‘-std=c11’ or ‘-std=gnu11’ to compile your code
but that was a trivial fixup.
Your commit handler does sit and wait. I guess I avoided that and had it
still deferred and handled next time the vblank interrupt fires. Yours
is a bit shorter and less complex so it wins. :)
--8<---
From 76f9e5fed216a815e9eb56152f85454521079f10 Mon Sep 17 00:00:00 2001
From: Liviu Dudau
Date: Fri, 8 Jul 2022 16:39:21 +0100
Subject: [PATCH] drm/komeda: Fix handling of atomic commits in the
atomic_commit_tail hook
Komeda driver relies on the generic DRM atomic helper functions to handle
commits. It only implements an atomic_commit_tail hook for the
mode_config_helper_funcs and even that one is pretty close to the generic
implementation with the exception of additional dma_fence signalling.
What the generic helper framework doesn't do is waiting for the actual
hardware to signal that the commit parameters have been written into the
appropriate registers. As we signal CRTC events only on the irq handlers,
we need to flush the configuration and wait for the hardware to respond.
Add the Komeda specific implementation for atomic_commit_hw_done() that
flushes and waits for flip done before calling
drm_atomic_helper_commit_hw_done().
The fix was prompted by a patch from Carsten Haitzler where he was trying to
solve the same issue but in a different way that I think can lead to wrong
event signaling to userspace.
Reported-by: Carsten Haitzler
Signed-off-by: Liviu Dudau
---
.../gpu/drm/arm/display/komeda/komeda_crtc.c | 4 ++--
.../gpu/drm/arm/display/komeda/komeda_kms.c | 20 ++-
.../gpu/drm/arm/display/komeda/komeda_kms.h | 2 ++
3 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 59172acb973803d..292f533d8cf0de6 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -235,7 +235,7 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
crtc->state->event = NULL;
drm_crtc_send_vblank_event(crtc, event);
} else {
- DRM_WARN("CRTC[%d]: FLIP happen but no pending
commit.\n",
+ DRM_WARN("CRTC[%d]: FLIP happened but no pending
commit.\n",
drm_crtc_index(>base));
}
spin_unlock_irqrestore(>dev->event_lock, flags);
@@ -286,7 +286,7 @@ komeda_crtc_atomic_enable(struct drm_crtc *crtc,
komeda_crtc_do_flush(crtc, old);
}
-static void
+void
komed
Re: [PATCH 2/3] drm/komeda - At init write GCU control block to handle already on DPU
On 7/8/22 17:07, Liviu Dudau wrote: On Mon, Jun 06, 2022 at 12:47:13PM +0100, carsten.haitz...@foss.arm.com wrote: From: Carsten Haitzler If something has already set up the DPU before the komeda driver comes up, it will fail to init because it was just writing to the SRST bit in the GCU control register and ignoring others. This resulted in TBU bringup stalling and init failing. By writing completely we also set the mode back to 0 (inactive) too and thus TBU bringup works. This is a rather large hammer, tbh. I would like to see if there is a better way of handling the handover from EFIFB that this patch is trying to fix, but I lack an usable plaform for that. It will generate a flicker at module load time, but if users of Morello are happy with that, then We're pretty happy with that setup right now. Certainly better than Komeda failing to init. Acked-by: Liviu Dudau Best regards, Liviu Signed-off-by: Carsten Haitzler --- drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c index 00fa56c29b3e..39618c1a4c81 100644 --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c @@ -309,8 +309,7 @@ static int d71_reset(struct d71_dev *d71) u32 __iomem *gcu = d71->gcu_addr; int ret; - malidp_write32_mask(gcu, BLK_CONTROL, - GCU_CONTROL_SRST, GCU_CONTROL_SRST); + malidp_write32(gcu, BLK_CONTROL, GCU_CONTROL_SRST); ret = dp_wait_cond(!(malidp_read32(gcu, BLK_CONTROL) & GCU_CONTROL_SRST), 100, 1000, 1); -- 2.32.0
[PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up
From: Carsten Haitzler
Sometimes there is an extra dcm crtc state in the pipeline whose
penting vblank event has not been handled yet. We would previously
throw this out and the vblank event never triggers leading to hard
lockups higher up the stack where an expected vlank event never comes
back (screen freezes).
This fixes that by tracking a pending crtc state that needs handling
and handle it producing a vlank event next vblank if it had not
laready been handled before. This fixes the hangs and ensures our
display keeps updating seamlessly and is certainly a much better state
to be in than after some time ending up with a mysteriously frozen
screen and a lot of kernle messages complaining about this too.
Signed-off-by: Carsten Haitzler
---
.../gpu/drm/arm/display/komeda/komeda_crtc.c | 10 ++
.../gpu/drm/arm/display/komeda/komeda_kms.c | 19 ++-
.../gpu/drm/arm/display/komeda/komeda_kms.h | 3 +++
3 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 59172acb9738..b7f0a5f97222 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -227,6 +227,16 @@ void komeda_crtc_handle_event(struct komeda_crtc *kcrtc,
complete_all(kcrtc->disable_done);
kcrtc->disable_done = NULL;
} else if (crtc->state->event) {
+ if (kcrtc->state_needs_handling) {
+ event = kcrtc->state_needs_handling->event;
+ if (event) {
+ kcrtc->state_needs_handling->event =
NULL;
+ kcrtc->state_needs_handling = NULL;
+ drm_crtc_send_vblank_event(crtc, event);
+ } else {
+ kcrtc->state_needs_handling = NULL;
+ }
+ }
event = crtc->state->event;
/*
* Consume event before notifying drm core that flip
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index 93b7f09b96ca..bbc051a1896a 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -226,10 +226,27 @@ static int komeda_kms_check(struct drm_device *dev,
return 0;
}
+static int komeda_kms_commit(struct drm_device *drm,
+ struct drm_atomic_state *state,
+ bool nonblock)
+{
+ int i;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+ struct komeda_crtc *kcrtc;
+
+ for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
+ new_crtc_state, i) {
+ kcrtc = to_kcrtc(crtc);
+ kcrtc->state_needs_handling = crtc->state;
+ }
+ return drm_atomic_helper_commit(drm, state, nonblock);
+}
+
static const struct drm_mode_config_funcs komeda_mode_config_funcs = {
.fb_create = komeda_fb_create,
.atomic_check = komeda_kms_check,
- .atomic_commit = drm_atomic_helper_commit,
+ .atomic_commit = komeda_kms_commit,
};
static void komeda_kms_mode_config_init(struct komeda_kms_dev *kms,
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index 456f3c435719..8ff3ad04dfe4 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -84,6 +84,9 @@ struct komeda_crtc {
/** @disable_done: this flip_done is for tracing the disable */
struct completion *disable_done;
+
+ /** @state_needs_handling: Has not had it's vblank event handled yet */
+ struct drm_crtc_state *state_needs_handling;
};
/**
--
2.32.0
[PATCH 2/3] drm/komeda - At init write GCU control block to handle already on DPU
From: Carsten Haitzler If something has already set up the DPU before the komeda driver comes up, it will fail to init because it was just writing to the SRST bit in the GCU control register and ignoring others. This resulted in TBU bringup stalling and init failing. By writing completely we also set the mode back to 0 (inactive) too and thus TBU bringup works. Signed-off-by: Carsten Haitzler --- drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c index 00fa56c29b3e..39618c1a4c81 100644 --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_dev.c @@ -309,8 +309,7 @@ static int d71_reset(struct d71_dev *d71) u32 __iomem *gcu = d71->gcu_addr; int ret; - malidp_write32_mask(gcu, BLK_CONTROL, - GCU_CONTROL_SRST, GCU_CONTROL_SRST); + malidp_write32(gcu, BLK_CONTROL, GCU_CONTROL_SRST); ret = dp_wait_cond(!(malidp_read32(gcu, BLK_CONTROL) & GCU_CONTROL_SRST), 100, 1000, 1); -- 2.32.0
[PATCH 1/3] drm/komeda - Add legacy FB support so VT's work as expected
From: Carsten Haitzler The komeda driver doesn't come up with a visible text (FB) mode VT by default as it was missing legacy FB support. It's useful to have a working text VT on a system for debug and general usability, so enable it. You can always toggle CONFIG_FRAMEBUFFER_CONSOLE. Signed-off-by: Carsten Haitzler --- drivers/gpu/drm/arm/display/komeda/komeda_drv.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c index e7933930a657..c0c7933a9631 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include "komeda_dev.h" #include "komeda_kms.h" @@ -71,6 +72,7 @@ static int komeda_bind(struct device *dev) } dev_set_drvdata(dev, mdrv); + drm_fbdev_generic_setup(>kms->base, 32); return 0; -- 2.32.0
Re: [PATCH v2 11/11] dt-bindings: display: convert Arm Komeda to DT schema
That seems sensible to me. It matches the kind of DT content I know
works. It's certainly more detailed now.
On 5/6/22 15:05, Andre Przywara wrote:
The Arm Komeda (aka Mali-D71) is a display controller that scans out a
framebuffer and hands a signal to a digital encoder to generate a DVI
or HDMI signal. It supports up to two pipelines, each frame can be
composed of up to four layers.
Convert the existing DT binding to DT schema.
Signed-off-by: Andre Przywara
---
.../bindings/display/arm,komeda.txt | 78 ---
.../bindings/display/arm,komeda.yaml | 130 ++
2 files changed, 130 insertions(+), 78 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/display/arm,komeda.txt
create mode 100644 Documentation/devicetree/bindings/display/arm,komeda.yaml
diff --git a/Documentation/devicetree/bindings/display/arm,komeda.txt
b/Documentation/devicetree/bindings/display/arm,komeda.txt
deleted file mode 100644
index 8513695ee47fe..0
--- a/Documentation/devicetree/bindings/display/arm,komeda.txt
+++ /dev/null
@@ -1,78 +0,0 @@
-Device Tree bindings for Arm Komeda display driver
-
-Required properties:
-- compatible: Should be "arm,mali-d71"
-- reg: Physical base address and length of the registers in the system
-- interrupts: the interrupt line number of the device in the system
-- clocks: A list of phandle + clock-specifier pairs, one for each entry
-in 'clock-names'
-- clock-names: A list of clock names. It should contain:
- - "aclk": for the main processor clock
-- #address-cells: Must be 1
-- #size-cells: Must be 0
-- iommus: configure the stream id to IOMMU, Must be configured if want to
-enable iommu in display. for how to configure this node please reference
-devicetree/bindings/iommu/arm,smmu-v3.txt,
-devicetree/bindings/iommu/iommu.txt
-
-Required properties for sub-node: pipeline@nq
-Each device contains one or two pipeline sub-nodes (at least one), each
-pipeline node should provide properties:
-- reg: Zero-indexed identifier for the pipeline
-- clocks: A list of phandle + clock-specifier pairs, one for each entry
-in 'clock-names'
-- clock-names: should contain:
- - "pxclk": pixel clock
-
-- port: each pipeline connect to an encoder input port. The connection is
-modeled using the OF graph bindings specified in
-Documentation/devicetree/bindings/graph.txt
-
-Optional properties:
- - memory-region: phandle to a node describing memory (see
-Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt)
-to be used for the framebuffer; if not present, the framebuffer may
-be located anywhere in memory.
-
-Example:
-/ {
- ...
-
- dp0: display@c0 {
- #address-cells = <1>;
- #size-cells = <0>;
- compatible = "arm,mali-d71";
- reg = <0xc0 0x2>;
- interrupts = <0 168 4>;
- clocks = <_aclk>;
- clock-names = "aclk";
- iommus = < 0>, < 1>, < 2>, < 3>,
- < 4>, < 5>, < 6>, < 7>,
- < 8>, < 9>;
-
- dp0_pipe0: pipeline@0 {
- clocks = <>;
- clock-names = "pxclk";
- reg = <0>;
-
- port {
- dp0_pipe0_out: endpoint {
- remote-endpoint = <_dvi0_in>;
- };
- };
- };
-
- dp0_pipe1: pipeline@1 {
- clocks = <>;
- clock-names = "pxclk";
- reg = <1>;
-
- port {
- dp0_pipe1_out: endpoint {
- remote-endpoint = <_dvi1_in>;
- };
- };
- };
- };
- ...
-};
diff --git a/Documentation/devicetree/bindings/display/arm,komeda.yaml
b/Documentation/devicetree/bindings/display/arm,komeda.yaml
new file mode 100644
index 0..9f4aade97f10a
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/arm,komeda.yaml
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/arm,komeda.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Arm Komeda display processor
+
+maintainers:
+ - Liviu Dudau
+ - Andre Przywara
+
+description:
+ The Arm Mali D71 display processor supports up to two displays with up
+ to a 4K resolution each. Each pipeline can be composed of up to four
+ layers. It is typically connected to a digital display connector like HDMI.
+
+properties:
+ compatible:
+oneOf:
+ - items:
+ - const: arm,mali-d32
+ - const: arm,mali-d71
+ - const: arm,mali-d71
+
+ reg:
+maxItems: 1
+
+ interrupts:
+
Re: [RFC] drm/kms: control display brightness through drm_connector properties
On Mon, 11 Apr 2022 12:27:37 +0200 Hans de Goede said:
> Hi,
>
> On 4/7/22 20:58, Carsten Haitzler wrote:
> > On Thu, 7 Apr 2022 17:38:59 +0200 Hans de Goede said:
> >
> > Below you covered our usual /sys/class/backlight device friends... what
> > about DDC monitor controls? These function similarly but just remotely
> > control a screen via I2C and also suffer from the same problems of "need
> > root" and "have to do some fun in mapping them to a given screen".
>
> Right, supporting this definitely is part of the plan, this is why my original
> email had the following footnote:
Yay!
> 1) The need to be able to map the backlight device to a specific display
> has become clear once more with the recent proposal to add DDCDI support:
> https://lore.kernel.org/lkml/20220403230850.2986-1-yusisameri...@gmail.com/
Oh gee - I missed that. my bad!
> :)
>
> > Otherwise I welcome this de-uglification of the backlight device and
> > putting it together with the drm device that controls that monitor.
>
> Thx.
Having to deal with the backlight device madness is a big pain (have already
done it - DDC included) and properly exposing these things attached to the
proper KMS device is absolutely the right thing. Admittedly this punts the job
of matching a backlight device to the right video output in KMS to the kernel
so at least it gets solved in one place rather than it being re-invented again
and again per wm/desktop/compositor.
> > Just to make life more fun ... DDC does much more than backlight controls.
> > It can control essentially anything that is in the OSD for your monitor
> > (contrast, brightness, backlight, sharpness, color temperatures, input to
> > display (DP vs HDMI vs DVI etc.), an for extra fun points can even tel you
> > the current rotation state of your monitor. All of these do make sense to
> > live as drm connector properties too. Perhaps not a first iteration but
> > something to consider in this design.
>
> One thing which I do want to take into account is to make sure that userspace
> can still do DDC/CI for all the other features. I know there is demand for
> adding brightness control over DDC/CI. I'm not aware of any concrete use-cases
> for the other DDC/CI settings. Also DDC/CI can include some pretty crazy
> stuff like setting up picture in picture using 2 different inputs of the
> monitor, which is very vendor specific. So all in all I think that we should
> just punt most of the DDC/CI stuff to userspace.
Having spent some time with DDC you're right - it can have some interesting
properties, but a wide number seem to be highly common between monitors and
make total sense to regularly use if available. Backlight/brightness is just
the immediate focus here.
> With that said I agree that it would be good to think about possibly other
> some of the other settings in case some use-case does show up for those.
>
> The biggest problem with doing this is coming up with some prefix to
> namespace things. I've gone with bl_brightness to avoid a conflict
> with the existing TV specific properties which have plain "brightness"
> put if we want to e.g. also add DDC/CI contrast as a property later
> then it might be good to come up with another more generic prefix
> which can be shared between laptop-panel-brightness, DDC/CI-brightness
> and DDC/CI-contrast ... ?
>
> So any suggestions for a better prefix?
Well here is my take. Have DDC properties separate from a build-in backlight
device. Userspace code will have to essentially do something like:
if (built_in_backlight_exists(output)) // built in backlight device
set_backlight_brightness(output, val);
else if (ddc_prop_exists(output, 0x10)) // 0x10 is ddc brightness/backlight
set_ddc_int_val(output, 0x10, val);
else // fallback for ye olde setuid tooling
{ ...
}
DDC properties are quite simple in essence so just exposing the set so you can
read/write them (and check if they exist at all) would do the right thing - tie
DDC to the output visa KMS, (you still could use I2C directly if you like and
go behind KMS's back) but it'd then punt the policy decision of which
properties are common/sane to userspace without adding a possibly "endless" set
of "let's now adopt/abstract this DDC property" discussions. Wayland
compositors can adopts the properties they see as most useful for their uses.
Xorg could expose them via XRR output properties. So my take at least is to give
DDC it's own property namespace/set that allows an arbitrary set of numbered
properties and leave it pretty raw.
> Regards,
>
> Hans
>
>
>
>
>
> >
> >> As discussed already several times in the past:
> >> https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/
&
Re: [RFC] drm/kms: control display brightness through drm_connector properties
0) duty-cycle below which the driver will > never go. > > bl_brightness_control_method: ro, enum, possible values: > none: The GPU driver expects brightness control to be provided by another > driver and that driver has not loaded yet. > unknown: The underlying control mechanism is unknown. > pwm: The brightness property directly controls the duty-cycle of a PWM > output. > firmware: The brightness is controlled through firmware calls. > DDC/CI: The brightness is controlled through the DDC/CI protocol. > gmux: The brightness is controlled by the GMUX. > Note this enum may be extended in the future, so other values may > be read, these should be treated as "unknown". > > When brightness control becomes available after being reported > as not available before (bl_brightness_control_method=="none") > a uevent with CONNECTOR= and > PROPERTY= will be generated > at this point all the properties must be re-read. > > When/once brightness control is available then all the read-only > properties are fixed and will never change. > > Besides the "none" value for no driver having loaded yet, > the different bl_brightness_control_method values are intended for > (userspace) heuristics for such things as the brightness setting > linearly controlling electrical power or setting perceived brightness. > > Regards, > > Hans > > > 1) The need to be able to map the backlight device to a specific display > has become clear once more with the recent proposal to add DDCDI support: > https://lore.kernel.org/lkml/20220403230850.2986-1-yusisameri...@gmail.com/ > > 2) > https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/ > Note this proposal included a method for userspace to be able to tell the > kernel if the native/acpi_video/vendor backlight device should be used, but > this has been solved in the kernel for years now: > https://www.spinics.net/lists/linux-acpi/msg50526.html An initial > implementation of this proposal is available here: > https://cgit.freedesktop.org/~mperes/linux/log/?h=backlight > -- - Codito, ergo sum - "I code, therefore I am" -- Carsten Haitzler - ras...@rasterman.com
Re: [PATCH 2/3] drm/arm/komeda : change driver to use drm_writeback_connector.base pointer
This makes sense given the other patches in your series, but it seems as
yet no one has anything to say about this. I don't have anything
specific to comment on other than it seems to make the correct changes
to komeda given the rest.
Reviewed-by: Carsten Haitzler
On 1/11/22 10:18, Kandpal, Suraj wrote:
Making changes to komeda driver because we had to change
drm_writeback_connector.base into a pointer the reason for which is
expained in the Patch (drm: add writeback pointers to drm_connector).
Signed-off-by: Kandpal, Suraj
---
drivers/gpu/drm/arm/display/komeda/komeda_crtc.c | 2 +-
drivers/gpu/drm/arm/display/komeda/komeda_kms.h | 3 ++-
drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c | 9 +
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
index 59172acb9738..eb37f41c1790 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
@@ -265,7 +265,7 @@ komeda_crtc_do_flush(struct drm_crtc *crtc,
if (slave && has_bit(slave->id, kcrtc_st->affected_pipes))
komeda_pipeline_update(slave, old->state);
- conn_st = wb_conn ? wb_conn->base.base.state : NULL;
+ conn_st = wb_conn ? wb_conn->base.base->state : NULL;
if (conn_st && conn_st->writeback_job)
drm_writeback_queue_job(_conn->base, conn_st);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index 456f3c435719..8d83883a1d99 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -53,6 +53,7 @@ struct komeda_plane_state {
* struct komeda_wb_connector
*/
struct komeda_wb_connector {
+ struct drm_connector conn;
/** @base: _writeback_connector */
struct drm_writeback_connector base;
@@ -136,7 +137,7 @@ struct komeda_kms_dev {
static inline bool is_writeback_only(struct drm_crtc_state *st)
{
struct komeda_wb_connector *wb_conn = to_kcrtc(st->crtc)->wb_conn;
- struct drm_connector *conn = wb_conn ? _conn->base.base : NULL;
+ struct drm_connector *conn = wb_conn ? wb_conn->base.base : NULL;
return conn && (st->connector_mask == BIT(drm_connector_index(conn)));
}
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
index e465cc4879c9..0caaf483276d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_wb_connector.c
@@ -51,7 +51,7 @@ komeda_wb_encoder_atomic_check(struct drm_encoder *encoder,
return -EINVAL;
}
- wb_layer = to_kconn(to_wb_conn(conn_st->connector))->wb_layer;
+ wb_layer =
to_kconn(drm_connector_to_writeback(conn_st->connector))->wb_layer;
/*
* No need for a full modested when the only connector changed is the
@@ -123,7 +123,7 @@ komeda_wb_connector_fill_modes(struct drm_connector
*connector,
static void komeda_wb_connector_destroy(struct drm_connector *connector)
{
drm_connector_cleanup(connector);
- kfree(to_kconn(to_wb_conn(connector)));
+ kfree(to_kconn(drm_connector_to_writeback(connector)));
}
static const struct drm_connector_funcs komeda_wb_connector_funcs = {
@@ -155,6 +155,7 @@ static int komeda_wb_connector_add(struct komeda_kms_dev
*kms,
kwb_conn->wb_layer = kcrtc->master->wb_layer;
wb_conn = _conn->base;
+ wb_conn->base = _conn->conn;
wb_conn->encoder.possible_crtcs = BIT(drm_crtc_index(>base));
formats = komeda_get_layer_fourcc_list(>fmt_tbl,
@@ -171,9 +172,9 @@ static int komeda_wb_connector_add(struct komeda_kms_dev
*kms,
return err;
}
- drm_connector_helper_add(_conn->base, _wb_conn_helper_funcs);
+ drm_connector_helper_add(wb_conn->base, _wb_conn_helper_funcs);
- info = _conn->base.base.display_info;
+ info = _conn->base.base->display_info;
info->bpc = __fls(kcrtc->master->improc->supported_color_depths);
info->color_formats = kcrtc->master->improc->supported_color_formats;
Re: [PATCH 02/60] drm/arm/hdlcd: Add support for the nomodeset kernel parameter
Seems fine. Reviewed-by: Carsten Haitzler On 12/15/21 00:59, Javier Martinez Canillas wrote: According to disable Documentation/admin-guide/kernel-parameters.txt, this parameter can be used to disable kernel modesetting. DRM drivers will not perform display-mode changes or accelerated rendering and only the systewm system framebuffer will be available if it was set-up. But only a few DRM drivers currently check for nomodeset, make this driver to also support the command line parameter. Signed-off-by: Javier Martinez Canillas --- drivers/gpu/drm/arm/hdlcd_drv.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c index 479c2422a2e0..0939a64a9bd2 100644 --- a/drivers/gpu/drm/arm/hdlcd_drv.c +++ b/drivers/gpu/drm/arm/hdlcd_drv.c @@ -382,6 +382,9 @@ static int hdlcd_probe(struct platform_device *pdev) struct device_node *port; struct component_match *match = NULL; + if (drm_firmware_drivers_only()) + return -ENODEV; + /* there is only one output port inside each device, find it */ port = of_graph_get_remote_node(pdev->dev.of_node, 0, 0); if (!port)
Re: [PATCH 01/60] drm/komeda: Add support for the nomodeset kernel parameter
Seems fine. Reviewed-by: Carsten Haitzler On 12/15/21 00:59, Javier Martinez Canillas wrote: According to disable Documentation/admin-guide/kernel-parameters.txt, this parameter can be used to disable kernel modesetting. DRM drivers will not perform display-mode changes or accelerated rendering and only the systewm system framebuffer will be available if it was set-up. But only a few DRM drivers currently check for nomodeset, make this driver to also support the command line parameter. Signed-off-by: Javier Martinez Canillas --- drivers/gpu/drm/arm/display/komeda/komeda_drv.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c index e7933930a657..4f6d5c2103ec 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include "komeda_dev.h" #include "komeda_kms.h" @@ -117,6 +118,9 @@ static int komeda_platform_probe(struct platform_device *pdev) struct component_match *match = NULL; struct device_node *child; + if (drm_firmware_drivers_only()) + return -EINVAL; + if (!dev->of_node) return -ENODEV;
Re: [PATCH 03/60] drm/malidp: Add support for the nomodeset kernel parameter
Seems fine to me. Reviewed-by: Carsten Haitzler On 12/15/21 00:59, Javier Martinez Canillas wrote: According to disable Documentation/admin-guide/kernel-parameters.txt, this parameter can be used to disable kernel modesetting. DRM drivers will not perform display-mode changes or accelerated rendering and only the systewm system framebuffer will be available if it was set-up. But only a few DRM drivers currently check for nomodeset, make this driver to also support the command line parameter. Signed-off-by: Javier Martinez Canillas --- drivers/gpu/drm/arm/malidp_drv.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index 78d15b04b105..5da4168eb76d 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -938,6 +938,9 @@ static int malidp_platform_probe(struct platform_device *pdev) struct device_node *port; struct component_match *match = NULL; + if (drm_firmware_drivers_only()) + return -ENODEV; + if (!pdev->dev.of_node) return -ENODEV;
Re: [PATCH] drm/arm: arm hdlcd select DRM_GEM_CMA_HELPER
I sent an updated patch with the Fixes:
On 1/24/22 16:13, Steven Price wrote:
On 24/01/2022 15:13, carsten.haitz...@foss.arm.com wrote:
From: Carsten Haitzler
Without DRM_GEM_CMA_HELPER HDLCD won't build. This needs to be there too.
Signed-off-by: Carsten Haitzler
To add the context, DRM_HDLCD used to select DRM_KMS_CMA_HELPER but that
was removed in commit 09717af7d13d ("drm: Remove
CONFIG_DRM_KMS_CMA_HELPER option"). DRM_KMS_CMA_HELPER would select
DRM_GEM_CMA_HELPER but that transitive dependency was lost and
apparently the fixup was missed in that commit.
So we need a:
Fixes: 09717af7d13d ("drm: Remove CONFIG_DRM_KMS_CMA_HELPER option")
and with that...
Reviewed-by: Steven Price
Steve
---
drivers/gpu/drm/arm/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
index 58a242871b28..6e3f1d600541 100644
--- a/drivers/gpu/drm/arm/Kconfig
+++ b/drivers/gpu/drm/arm/Kconfig
@@ -6,6 +6,7 @@ config DRM_HDLCD
depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST)
depends on COMMON_CLK
select DRM_KMS_HELPER
+ select DRM_GEM_CMA_HELPER
help
Choose this option if you have an ARM High Definition Colour LCD
controller.
[PATCH] drm/arm: arm hdlcd select DRM_GEM_CMA_HELPER
From: Carsten Haitzler
Without DRM_GEM_CMA_HELPER HDLCD won't build. This needs to be there too.
Fixes: 09717af7d13d ("drm: Remove CONFIG_DRM_KMS_CMA_HELPER option")
Signed-off-by: Carsten Haitzler
---
drivers/gpu/drm/arm/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig
index 58a242871b28..6e3f1d600541 100644
--- a/drivers/gpu/drm/arm/Kconfig
+++ b/drivers/gpu/drm/arm/Kconfig
@@ -6,6 +6,7 @@ config DRM_HDLCD
depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST)
depends on COMMON_CLK
select DRM_KMS_HELPER
+ select DRM_GEM_CMA_HELPER
help
Choose this option if you have an ARM High Definition Colour LCD
controller.
--
2.30.1
Re: [PATCH] drm/arm: Fix hdlcd selects to add DRM_GEM_CMA_HELPER for build
Sorry - I meant disregard THIS one - following patch is right. On 1/24/22 14:58, carsten.haitz...@foss.arm.com wrote: From: Carsten Haitzler Without DRM_GEM_CMA_HELPER HDLCD won't build. This needs to be there too. Signed-off-by: Carsten Haitzler --- drivers/gpu/drm/arm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig index 58a242871b28..9eaceb981d92 100644 --- a/drivers/gpu/drm/arm/Kconfig +++ b/drivers/gpu/drm/arm/Kconfig @@ -6,6 +6,7 @@ config DRM_HDLCD depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST) depends on COMMON_CLK select DRM_KMS_HELPER +select DRM_GEM_CMA_HELPER help Choose this option if you have an ARM High Definition Colour LCD controller.
Re: [PATCH] drm/arm: arm hdlcd select DRM_GEM_CMA_HELPER
Sorry - noise. Ignore this one. Following one is fixed. On 1/24/22 15:13, carsten.haitz...@foss.arm.com wrote: From: Carsten Haitzler Without DRM_GEM_CMA_HELPER HDLCD won't build. This needs to be there too. Signed-off-by: Carsten Haitzler --- drivers/gpu/drm/arm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig index 58a242871b28..6e3f1d600541 100644 --- a/drivers/gpu/drm/arm/Kconfig +++ b/drivers/gpu/drm/arm/Kconfig @@ -6,6 +6,7 @@ config DRM_HDLCD depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST) depends on COMMON_CLK select DRM_KMS_HELPER + select DRM_GEM_CMA_HELPER help Choose this option if you have an ARM High Definition Colour LCD controller.
[PATCH] drm/arm: arm hdlcd select DRM_GEM_CMA_HELPER
From: Carsten Haitzler Without DRM_GEM_CMA_HELPER HDLCD won't build. This needs to be there too. Signed-off-by: Carsten Haitzler --- drivers/gpu/drm/arm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig index 58a242871b28..6e3f1d600541 100644 --- a/drivers/gpu/drm/arm/Kconfig +++ b/drivers/gpu/drm/arm/Kconfig @@ -6,6 +6,7 @@ config DRM_HDLCD depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST) depends on COMMON_CLK select DRM_KMS_HELPER + select DRM_GEM_CMA_HELPER help Choose this option if you have an ARM High Definition Colour LCD controller. -- 2.30.1
[PATCH] drm/arm: Fix hdlcd selects to add DRM_GEM_CMA_HELPER for build
From: Carsten Haitzler Without DRM_GEM_CMA_HELPER HDLCD won't build. This needs to be there too. Signed-off-by: Carsten Haitzler --- drivers/gpu/drm/arm/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/arm/Kconfig b/drivers/gpu/drm/arm/Kconfig index 58a242871b28..9eaceb981d92 100644 --- a/drivers/gpu/drm/arm/Kconfig +++ b/drivers/gpu/drm/arm/Kconfig @@ -6,6 +6,7 @@ config DRM_HDLCD depends on DRM && OF && (ARM || ARM64 || COMPILE_TEST) depends on COMMON_CLK select DRM_KMS_HELPER +select DRM_GEM_CMA_HELPER help Choose this option if you have an ARM High Definition Colour LCD controller. -- 2.30.1
Re: [PATCH] drm/plane: Move range check for format_count earlier
On 12/3/21 13:08, Liviu Dudau wrote:
On Fri, Dec 03, 2021 at 10:28:15AM +, Steven Price wrote:
While the check for format_count > 64 in __drm_universal_plane_init()
shouldn't be hit (it's a WARN_ON), in its current position it will then
leak the plane->format_types array and fail to call
drm_mode_object_unregister() leaking the modeset identifier. Move it to
the start of the function to avoid allocating those resources in the
first place.
Signed-off-by: Steven Price
Well spotted!
Reviewed-by: Liviu Dudau
I'm going to wait to see if anyone else has any comments before I'll merge this
into
drm-misc-fixes (or should it be drm-misc-next-fixes?)
This definitely looks to fix an ugly I spotted too while looking at your
prior patch thinking it might be wrong so a +1 from me.
Best regards,
Liviu
---
drivers/gpu/drm/drm_plane.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 82afb854141b..fd0bf90fb4c2 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -249,6 +249,13 @@ static int __drm_universal_plane_init(struct drm_device
*dev,
if (WARN_ON(config->num_total_plane >= 32))
return -EINVAL;
+ /*
+* First driver to need more than 64 formats needs to fix this. Each
+* format is encoded as a bit and the current code only supports a u64.
+*/
+ if (WARN_ON(format_count > 64))
+ return -EINVAL;
+
WARN_ON(drm_drv_uses_atomic_modeset(dev) &&
(!funcs->atomic_destroy_state ||
!funcs->atomic_duplicate_state));
@@ -270,13 +277,6 @@ static int __drm_universal_plane_init(struct drm_device
*dev,
return -ENOMEM;
}
- /*
-* First driver to need more than 64 formats needs to fix this. Each
-* format is encoded as a bit and the current code only supports a u64.
-*/
- if (WARN_ON(format_count > 64))
- return -EINVAL;
-
if (format_modifiers) {
const uint64_t *temp_modifiers = format_modifiers;
--
2.25.1
Re: [PATCH] drm/komeda: return early if drm_universal_plane_init() fails.
On 12/3/21 14:15, Carsten Haitzler wrote:
On 12/3/21 10:09, Liviu Dudau wrote:
If drm_universal_plane_init() fails early we jump to the common
cleanup code
that calls komeda_plane_destroy() which in turn could access the
uninitalised
drm_plane and crash. Return early if an error is detected without
going through
the common code.
Reported-by: Steven Price
Signed-off-by: Liviu Dudau
---
drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
index aa193c58f4bf6d9..517b94c3bcaf966 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
@@ -279,8 +279,10 @@ static int komeda_plane_add(struct komeda_kms_dev
*kms,
komeda_put_fourcc_list(formats);
- if (err)
- goto cleanup;
+ if (err) {
+ kfree(kplane);
+ return err;
+ }
drm_plane_helper_add(plane, _plane_helper_funcs);
Ummm... can I disagree here? this goto cleanup I think is just fine
because plane has been set before drm_universal_plane_init() is called
which is before the if (err) here. komeda_plane_destroy() in cleanup:
does all the right things, so this patch isn't needed. I think it's less
clean as it adds a new "handle error" path special-case instance where a
special case is not needed. The fix to Zhou's original patch was needed
for exactly the reason Liviu said - but not this one... ?
Let me take that back - it seems an init fail shouldn't call cleanup but
the init fail doesn't quite cleanup properly. Steven found this and
already sent a patch.
Re: [PATCH] drm/komeda: return early if drm_universal_plane_init() fails.
On 12/3/21 10:09, Liviu Dudau wrote:
If drm_universal_plane_init() fails early we jump to the common cleanup code
that calls komeda_plane_destroy() which in turn could access the uninitalised
drm_plane and crash. Return early if an error is detected without going through
the common code.
Reported-by: Steven Price
Signed-off-by: Liviu Dudau
---
drivers/gpu/drm/arm/display/komeda/komeda_plane.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
index aa193c58f4bf6d9..517b94c3bcaf966 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_plane.c
@@ -279,8 +279,10 @@ static int komeda_plane_add(struct komeda_kms_dev *kms,
komeda_put_fourcc_list(formats);
- if (err)
- goto cleanup;
+ if (err) {
+ kfree(kplane);
+ return err;
+ }
drm_plane_helper_add(plane, _plane_helper_funcs);
Ummm... can I disagree here? this goto cleanup I think is just fine
because plane has been set before drm_universal_plane_init() is called
which is before the if (err) here. komeda_plane_destroy() in cleanup:
does all the right things, so this patch isn't needed. I think it's less
clean as it adds a new "handle error" path special-case instance where a
special case is not needed. The fix to Zhou's original patch was needed
for exactly the reason Liviu said - but not this one... ?
Re: Call for an EDID parsing library
On Wed, 7 Apr 2021 11:44:04 +0300 Pekka Paalanen said: > Hi all, > > with display servers proliferating thanks to Wayland, and the Linux > kernel exposing only a very limited set of information based on EDID > (rightfully so!), the need to interpret EDID blobs is spreading even > more. I would like to start the discussion about starting a project to > develop a shared library for parsing EDID blobs. This is not the first > time either, other people have suggested it years and years ago already, > but apparently it didn't quite materialise as far as I know. > > Right now, it seems that more or less every display server and other > KMS application is hand-rolling its own EDID parsing code, even for the > most trivial information (monitor make, model, and serial number). With > HDR and color management support coming to Wayland, the need to parse > more things out of EDID will only increase. These things are not > exposed by the kernel, and most of these things have no use for the > kernel either. > > My personal motivation for this is that I don't want to be developing > or reviewing yet another partial EDID parser implementation in Weston. > > I recall ponderings about sharing the same EDID parsing code between > the kernel and userspace, but I got the feeling that it would be a > hindrance in process more than a benefit from sharing code. It would > need to live in the kernel tree, to be managed with the kernel > development process, use the kernel "standard libraries", and adhere to > kernel programming style - all which are good and fine, but maybe also > more restricting than useful in this case. Therefore I would suggest a > userspace-only library. > > Everyone hand-rolling their own parsing code has the obvious > disadvantages. In the opposite, I would expect a new shared EDID > parsing library and project to: > - be hosted under gitlab.freedesktop.org > - be MIT licensed > - offer at least a C ABI > - employ mandatory Gitlab CI to ensure with sample EDID blobs that it > cannot regress > > Prior art can be found in various places. I believe Xorg xserver has > its battle-tested EDID parsing code. Ajax once played with the idea in > https://cgit.freedesktop.org/~ajax/libminitru/ . Then we have > https://git.linuxtv.org/edid-decode.git too which has code and test > data but not a C ABI (right?). > > It does not necessarily need to be a new project. Would edid-decode > project be open to adding a C library ABI? > > edid-decode is already MIT licensed and seems to have a lot of code, > too, but that's all I know for now. > > Would there be anyone interested to take lead or work on a project like > this? > > Personally I don't think I'd be working on it, but I would be really > happy to use it in Weston. > > Should it be a new project, or grow inside edid-decode or something > else? > > I believe MIT license is important to have wide adoption of it. C ABI > similarly. Also that it would be a "small" library without heavy > dependencies. I'd say it needs nothing more than libc - I can't see the justification for more than that. If this is the case along with the above you have given, then I see no reason for it to not be used by everyone other than the usual user complaint of "too many dependencies (of a compositor)". :) I'd definitely consider using it. > What do you think? Could anyone spare their time for this? > > Who would be interested in using it if this library appeared? > > > Thanks, > pq -- - Codito, ergo sum - "I code, therefore I am" -- Carsten Haitzler - ras...@rasterman.com ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/komeda: Convert sysfs sprintf/snprintf family to sysfs_emit
On 3/30/21 2:25 AM, Tian Tao wrote:
Fix the following coccicheck warning:
drivers/gpu/drm/arm/display/komeda/komeda_dev.c:97:8-16: WARNING:
use scnprintf or sprintf
drivers/gpu/drm/arm/display/komeda/komeda_dev.c:88:8-16: WARNING:
use scnprintf or sprintf
drivers/gpu/drm/arm/display/komeda/komeda_dev.c:65:8-16: WARNING:
use scnprintf or sprintf
Signed-off-by: Tian Tao
---
drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
index ca891ae..cc7664c 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
@@ -62,7 +62,7 @@ core_id_show(struct device *dev, struct device_attribute
*attr, char *buf)
{
struct komeda_dev *mdev = dev_to_mdev(dev);
- return snprintf(buf, PAGE_SIZE, "0x%08x\n", mdev->chip.core_id);
+ return sysfs_emit(buf, "0x%08x\n", mdev->chip.core_id);
}
static DEVICE_ATTR_RO(core_id);
@@ -85,7 +85,7 @@ config_id_show(struct device *dev, struct device_attribute *attr, char *buf)
if (pipe->layers[i]->layer_type == KOMEDA_FMT_RICH_LAYER)
config_id.n_richs++;
}
- return snprintf(buf, PAGE_SIZE, "0x%08x\n", config_id.value);
+ return sysfs_emit(buf, "0x%08x\n", config_id.value);
}
static DEVICE_ATTR_RO(config_id);
@@ -94,7 +94,7 @@ aclk_hz_show(struct device *dev, struct device_attribute *attr, char *buf)
{
struct komeda_dev *mdev = dev_to_mdev(dev);
- return snprintf(buf, PAGE_SIZE, "%lu\n", clk_get_rate(mdev->aclk));
+ return sysfs_emit(buf, "%lu\n", clk_get_rate(mdev->aclk));
}
static DEVICE_ATTR_RO(aclk_hz);
Looks OK to me.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/komeda: Fix off-by-1 when with readback conn due to rounding
On 3/12/21 10:55 AM, Brian Starkey wrote: (Adding back James again - did you use get_maintainer.pl?) On Thu, Mar 11, 2021 at 12:08:46PM +, carsten.haitz...@foss.arm.com wrote: From: Carsten Haitzler When setting up a readback connector that writes data back to memory rather than to an actual output device (HDMI etc.), rounding was set to round. As the DPU uses a higher internal number of bits when generating a color value, this round-down back to 8bit ended up with everything being off-by one. e.g. #fefefe became #ff. This sets Perhaps overly pedantic, but now we've tracked down what was actually happening I think we can be more precise here. Not _everything_ is off-by-one, it's just rounding in the standard sense - if the most Well a very large number of pixels were off-by-1 ... I guess it's an exaggeration but a "vast number of pixels were off by 1". I guess I was just using common terms like "everything is expensive here" doesn't actually mean absolutely everything but a very vast number of things. You know what I mean. :) The comment as a whole describing rounding policies should provide more details. I just write the log as a "when spelunking through history, this log will give me some broader insight into what this change is without being war and peace and If I want to see more and this commit is interesting to my spelunking efforts, I'll git log -U to read that". significant bit-to-be-discarded is set, the value is rounded up to minimise the absolute error introduced by bit-depth reduction. rounding to "round-down" so things end up correct by turning on the LW_TRC round down flag. Can we call it "truncate" rather than round down? I think it makes "TRC" a bit more understandable. That's the official name from the docs though (TRC)... makes it easier to match to them... So I think you can argue this both ways. The comment where it's used though does make it clear... Signed-off-by: Carsten Haitzler --- drivers/gpu/drm/arm/display/komeda/d71/d71_component.c | 7 ++- drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c index 8a02ade369db..e97acc5519d1 100644 --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c @@ -468,7 +468,12 @@ static void d71_wb_layer_update(struct komeda_component *c, struct komeda_layer_state *st = to_layer_st(state); struct drm_connector_state *conn_st = state->wb_conn->state; struct komeda_fb *kfb = to_kfb(conn_st->writeback_job->fb); - u32 ctrl = L_EN | LW_OFM, mask = L_EN | LW_OFM | LW_TBU_EN; + /* LW_TRC sets rounding to truncate not round which is needed for +* the output of writeback to match the input in the most common +* use cases like RGB888 -> RGB888, so set this bit by default +*/ Hm, not sure why this file uses "net/" style comments, but as you said, this is in-keeping with the rest of the file, so meh :-) Yup. Just stick to "follow the style there" unless there is seemingly a good reason that what is there is horribly "broken" and needs fixing up. + u32 ctrl = LW_TRC | L_EN | LW_OFM; + u32 mask = LW_TRC | L_EN | LW_OFM | LW_TBU_EN; If you were aiming for matching register order, this should be: L_EN | LW_TRC | LW_OFM | LW_TBU_EN I think it'd be nice to have the exact behaviour in the commit message, but either way this seems OK as a pragmatic fix so: git log -U ? :) Reviewed-by: Brian Starkey Thanks, -Brian u32 __iomem *reg = c->reg; d71_layer_update_fb(c, kfb, st->addr); diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h index e80172a0b320..a8036689d721 100644 --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h @@ -321,6 +321,7 @@ #define LAYER_WR_FORMAT 0x0D8 /* Layer_WR control bits */ +#define LW_TRC BIT(1) #define LW_OFMBIT(4) #define LW_LALPHA(x) (((x) & 0xFF) << 8) #define LW_A_WCACHE(x)(((x) & 0xF) << 28) -- 2.30.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/komeda: Fix off-by-1 when with readback conn due to rounding
From: Carsten Haitzler When setting up a readback connector that writes data back to memory rather than to an actual output device (HDMI etc.), rounding was set to round. As the DPU uses a higher internal number of bits when generating a color value, this round-down back to 8bit ended up with everything being off-by one. e.g. #fefefe became #ff. This sets rounding to "round-down" so things end up correct by turning on the LW_TRC round down flag. Signed-off-by: Carsten Haitzler --- drivers/gpu/drm/arm/display/komeda/d71/d71_component.c | 7 ++- drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c index 8a02ade369db..e97acc5519d1 100644 --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c @@ -468,7 +468,12 @@ static void d71_wb_layer_update(struct komeda_component *c, struct komeda_layer_state *st = to_layer_st(state); struct drm_connector_state *conn_st = state->wb_conn->state; struct komeda_fb *kfb = to_kfb(conn_st->writeback_job->fb); - u32 ctrl = L_EN | LW_OFM, mask = L_EN | LW_OFM | LW_TBU_EN; + /* LW_TRC sets rounding to truncate not round which is needed for +* the output of writeback to match the input in the most common +* use cases like RGB888 -> RGB888, so set this bit by default +*/ + u32 ctrl = LW_TRC | L_EN | LW_OFM; + u32 mask = LW_TRC | L_EN | LW_OFM | LW_TBU_EN; u32 __iomem *reg = c->reg; d71_layer_update_fb(c, kfb, st->addr); diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h index e80172a0b320..a8036689d721 100644 --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h @@ -321,6 +321,7 @@ #define LAYER_WR_FORMAT0x0D8 /* Layer_WR control bits */ +#define LW_TRC BIT(1) #define LW_OFM BIT(4) #define LW_LALPHA(x) (((x) & 0xFF) << 8) #define LW_A_WCACHE(x) (((x) & 0xF) << 28) -- 2.30.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/komeda: Fix off-by-1 when with readback conn due to rounding
On 3/9/21 11:36 AM, Brian Starkey wrote: Hi Carsten, (+James for komeda) Thanks for typing this up. On Fri, Mar 05, 2021 at 04:38:53PM +, carsten.haitz...@foss.arm.com wrote: From: Carsten Haitzler When setting up a readback conenctor that writes data back to memory s/readback conenctor/writeback connector/ (similar in the subject) rather than to an actual output device (HDMI etc.), rounding was ses s/ses/set/ I swear I re-read the log text... I must be auto-correcting in my head as I read. :) to round-down. As the DPU uses a higher internal number of bits when "round-down" isn't really accurate - the rounding mode "rounds" based on the most-significant discarded bit - so can round-up too. Come to think of it, I can't explain 0xff becoming 0xfe, but still, truncation is likely fine. Actually it was the other way - I mixed up the src/dest, but TRC does fix it which is the important bit. generating a color value, this round-down back to 8bit ended up with everything being off-by one. e.g. #ff became #fefefe. This sets rounding to "round" so things end up correct by turning on the round flag (LW_TRC). LW_TRC is the truncation flag. 0: Round, 1: Truncate Signed-off-by: Carsten Haitzler --- drivers/gpu/drm/arm/display/komeda/d71/d71_component.c | 6 +- drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c index 8a02ade369db..d551e79fa0f1 100644 --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c @@ -468,7 +468,11 @@ static void d71_wb_layer_update(struct komeda_component *c, struct komeda_layer_state *st = to_layer_st(state); struct drm_connector_state *conn_st = state->wb_conn->state; struct komeda_fb *kfb = to_kfb(conn_st->writeback_job->fb); - u32 ctrl = L_EN | LW_OFM, mask = L_EN | LW_OFM | LW_TBU_EN; + /* LW_TRC sets rounding to round not truncate which is needed for + * the output of writeback to match the input in the most common + * use cases like RGB888 -> RGB888, so set this bit by default */ /* * Comment style should be like this */ Same as above though - your description is inverted. By setting the LW_TRC bit, you're forcing the hardware to truncate instead of round. Yeah - inverted. But the source does have mixed comment styles with some /* * */ and some /* */ and some /* */ with the last 2 most common. + u32 ctrl = L_EN | LW_OFM | LW_TRC; + u32 mask = L_EN | LW_OFM | LW_TBU_EN | LW_TRC; Really nitpicking, but I think it'd be good to keep these in the same order as the bits in the register: L_EN | LW_TRC | LW_OFM | LW_TBU_EN I can do that. I'll send another with the above. Cheers, -Brian u32 __iomem *reg = c->reg; d71_layer_update_fb(c, kfb, st->addr); diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h index e80172a0b320..a8036689d721 100644 --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h @@ -321,6 +321,7 @@ #define LAYER_WR_FORMAT 0x0D8 /* Layer_WR control bits */ +#define LW_TRC BIT(1) #define LW_OFMBIT(4) #define LW_LALPHA(x) (((x) & 0xFF) << 8) #define LW_A_WCACHE(x)(((x) & 0xF) << 28) -- 2.30.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/komeda: Fix off-by-1 when with readback conn due to rounding
From: Carsten Haitzler When setting up a readback conenctor that writes data back to memory rather than to an actual output device (HDMI etc.), rounding was ses to round-down. As the DPU uses a higher internal number of bits when generating a color value, this round-down back to 8bit ended up with everything being off-by one. e.g. #ff became #fefefe. This sets rounding to "round" so things end up correct by turning on the round flag (LW_TRC). Signed-off-by: Carsten Haitzler --- drivers/gpu/drm/arm/display/komeda/d71/d71_component.c | 6 +- drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h | 1 + 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c index 8a02ade369db..d551e79fa0f1 100644 --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_component.c @@ -468,7 +468,11 @@ static void d71_wb_layer_update(struct komeda_component *c, struct komeda_layer_state *st = to_layer_st(state); struct drm_connector_state *conn_st = state->wb_conn->state; struct komeda_fb *kfb = to_kfb(conn_st->writeback_job->fb); - u32 ctrl = L_EN | LW_OFM, mask = L_EN | LW_OFM | LW_TBU_EN; + /* LW_TRC sets rounding to round not truncate which is needed for + * the output of writeback to match the input in the most common + * use cases like RGB888 -> RGB888, so set this bit by default */ + u32 ctrl = L_EN | LW_OFM | LW_TRC; + u32 mask = L_EN | LW_OFM | LW_TBU_EN | LW_TRC; u32 __iomem *reg = c->reg; d71_layer_update_fb(c, kfb, st->addr); diff --git a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h index e80172a0b320..a8036689d721 100644 --- a/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h +++ b/drivers/gpu/drm/arm/display/komeda/d71/d71_regs.h @@ -321,6 +321,7 @@ #define LAYER_WR_FORMAT0x0D8 /* Layer_WR control bits */ +#define LW_TRC BIT(1) #define LW_OFM BIT(4) #define LW_LALPHA(x) (((x) & 0xFF) << 8) #define LW_A_WCACHE(x) (((x) & 0xF) << 28) -- 2.30.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/komeda: Fix bit check to import to value of proper type
From: Carsten Haitzler
Another issue found by KASAN. The bit finding is buried inside the
dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.
Signed-off-by: Carsten Haitzler
---
.../drm/arm/display/include/malidp_utils.h| 3 ---
.../drm/arm/display/komeda/komeda_pipeline.c | 16 +++-
.../display/komeda/komeda_pipeline_state.c| 19 +++
3 files changed, 22 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/include/malidp_utils.h
b/drivers/gpu/drm/arm/display/include/malidp_utils.h
index 3bc383d5bf73..49a1d7f3539c 100644
--- a/drivers/gpu/drm/arm/display/include/malidp_utils.h
+++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h
@@ -13,9 +13,6 @@
#define has_bit(nr, mask) (BIT(nr) & (mask))
#define has_bits(bits, mask) (((bits) & (mask)) == (bits))
-#define dp_for_each_set_bit(bit, mask) \
- for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
-
#define dp_wait_cond(__cond, __tries, __min_range, __max_range)\
({ \
int num_tries = __tries;\
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
index 719a79728e24..06c595378dda 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
@@ -46,8 +46,9 @@ void komeda_pipeline_destroy(struct komeda_dev *mdev,
{
struct komeda_component *c;
int i;
+ unsigned long avail_comps = pipe->avail_comps;
- dp_for_each_set_bit(i, pipe->avail_comps) {
+ for_each_set_bit(i, _comps, 32) {
c = komeda_pipeline_get_component(pipe, i);
komeda_component_destroy(mdev, c);
}
@@ -247,6 +248,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline
*pipe)
{
struct komeda_component *c;
int id;
+ unsigned long avail_comps = pipe->avail_comps;
DRM_INFO("Pipeline-%d: n_layers: %d, n_scalers: %d, output: %s.\n",
pipe->id, pipe->n_layers, pipe->n_scalers,
@@ -258,7 +260,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline
*pipe)
pipe->of_output_links[1] ?
pipe->of_output_links[1]->full_name : "none");
- dp_for_each_set_bit(id, pipe->avail_comps) {
+ for_each_set_bit(id, _comps, 32) {
c = komeda_pipeline_get_component(pipe, id);
komeda_component_dump(c);
@@ -270,8 +272,9 @@ static void komeda_component_verify_inputs(struct
komeda_component *c)
struct komeda_pipeline *pipe = c->pipeline;
struct komeda_component *input;
int id;
+ unsigned long supported_inputs = c->supported_inputs;
- dp_for_each_set_bit(id, c->supported_inputs) {
+ for_each_set_bit(id, _inputs, 32) {
input = komeda_pipeline_get_component(pipe, id);
if (!input) {
c->supported_inputs &= ~(BIT(id));
@@ -302,8 +305,9 @@ static void komeda_pipeline_assemble(struct komeda_pipeline
*pipe)
struct komeda_component *c;
struct komeda_layer *layer;
int i, id;
+ unsigned long avail_comps = pipe->avail_comps;
- dp_for_each_set_bit(id, pipe->avail_comps) {
+ for_each_set_bit(id, _comps, 32) {
c = komeda_pipeline_get_component(pipe, id);
komeda_component_verify_inputs(c);
}
@@ -355,13 +359,15 @@ void komeda_pipeline_dump_register(struct komeda_pipeline
*pipe,
{
struct komeda_component *c;
u32 id;
+ unsigned long avail_comps;
seq_printf(sf, "\n Pipeline-%d ==\n", pipe->id);
if (pipe->funcs && pipe->funcs->dump_register)
pipe->funcs->dump_register(pipe, sf);
- dp_for_each_set_bit(id, pipe->avail_comps) {
+ avail_comps = pipe->avail_comps;
+ for_each_set_bit(id, _comps, 32) {
c = komeda_pipeline_get_component(pipe, id);
seq_printf(sf, "\n--%s--\n", c->name);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index e8b1e15312d8..176cdf411f9f 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -1232,14 +1232,15 @@ komeda_pipeline_unbound_components(struct
komeda_pipeline *pipe,
struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
struct komeda_component_state *c_st;
Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type
On 1/20/21 3:44 PM, Steven Price wrote:
Sent a new patch to list with updates as discussed.
On 18/01/2021 14:20, carsten.haitz...@foss.arm.com wrote:
From: Carsten Haitzler
Another issue found by KASAN. The bit finding is bueried inside the
NIT: s/bueried/buried/
dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.
This seems like an underlying bug/lack of clear documentation for the
underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the
right thing:
#define dp_for_each_set_bit(bit, mask) \
for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask)
* 8)
i.e. passing the actual size of type. However because of the case to
unsigned long (and subsequent accesses using that type) the compiler is
free to make accesses beyond the size of the variable (and indeed this
is completely broken on big-endian). The implementation actually
requires that the bitmap is really an array of unsigned long - no other
type will work.
So I think the fix should also include fixing the dp_for_each_set_bit()
macro - the cast is bogus as it stands.
Doing that I also get warnings on komeda_pipeline::avail_comps and
komeda_pipeline::supported_inputs - although I note there are other
bitmasks mentioned.
The other option is to fix dp_for_each_set_bit() itself using a little
hack:
- for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask)
* 8)
+ for_each_set_bit((bit), (&((unsigned long){mask})), sizeof(mask)
* 8)
With that I don't think you need any other change as the mask is actually
in a new unsigned long on the stack.
Steve
Signed-off-by: Carsten Haitzler
---
.../drm/arm/display/komeda/komeda_pipeline_state.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git
a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index e8b1e15312d8..f7dddb9f015d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct
komeda_pipeline *pipe,
struct komeda_pipeline_state *old =
priv_to_pipe_st(pipe->obj.state);
struct komeda_component_state *c_st;
struct komeda_component *c;
- u32 disabling_comps, id;
+ u32 id;
+ unsigned long disabling_comps;
WARN_ON(!old);
@@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct
komeda_pipeline *pipe,
st = komeda_pipeline_get_new_state(pipe, drm_st);
else
st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st,
NULL);
-
NIT: Random white space change
if (WARN_ON(IS_ERR_OR_NULL(st)))
return -EINVAL;
@@ -1287,7 +1287,8 @@ bool komeda_pipeline_disable(struct
komeda_pipeline *pipe,
struct komeda_pipeline_state *old;
struct komeda_component *c;
struct komeda_component_state *c_st;
- u32 id, disabling_comps = 0;
+ u32 id;
+ unsigned long disabling_comps;
old = komeda_pipeline_get_old_state(pipe, old_state);
@@ -1297,7 +1298,7 @@ bool komeda_pipeline_disable(struct
komeda_pipeline *pipe,
disabling_comps = old->active_comps &
pipe->standalone_disabled_comps;
- DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps:
0x%x.\n",
+ DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps:
0x%lx.\n",
pipe->id, old->active_comps, disabling_comps);
dp_for_each_set_bit(id, disabling_comps) {
@@ -1331,13 +1332,14 @@ void komeda_pipeline_update(struct
komeda_pipeline *pipe,
struct komeda_pipeline_state *new =
priv_to_pipe_st(pipe->obj.state);
struct komeda_pipeline_state *old;
struct komeda_component *c;
- u32 id, changed_comps = 0;
+ u32 id;
+ unsigned long changed_comps;
old = komeda_pipeline_get_old_state(pipe, old_state);
changed_comps = new->active_comps | old->active_comps;
- DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
+ DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
pipe->id, new->active_comps, changed_comps);
dp_for_each_set_bit(id, changed_comps) {
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/komeda: Fix bit check to import to value of proper type
From: Carsten Haitzler
Another issue found by KASAN. The bit finding is buried inside the
dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.
Signed-off-by: Carsten Haitzler
---
.../gpu/drm/arm/display/include/malidp_utils.h | 10 --
.../gpu/drm/arm/display/komeda/komeda_pipeline.c | 16 +++-
.../arm/display/komeda/komeda_pipeline_state.c | 13 -
3 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/include/malidp_utils.h
b/drivers/gpu/drm/arm/display/include/malidp_utils.h
index 3bc383d5bf73..8d289cd0b5b8 100644
--- a/drivers/gpu/drm/arm/display/include/malidp_utils.h
+++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h
@@ -12,9 +12,15 @@
#define has_bit(nr, mask) (BIT(nr) & (mask))
#define has_bits(bits, mask) (((bits) & (mask)) == (bits))
-
+/*
+#define dp_for_each_set_bit(bit, mask) \
+ for_each_set_bit((bit), (&((unsigned long)(mask))), sizeof(mask) * 8)
+#define dp_for_each_set_bit(bit, mask) \
+ unsigned long __local_mask = mask; \
+ for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
+*/
#define dp_for_each_set_bit(bit, mask) \
- for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
+ for_each_set_bit((bit), &(mask), sizeof(mask) * 8)
#define dp_wait_cond(__cond, __tries, __min_range, __max_range)\
({ \
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
index 719a79728e24..a85c8a806334 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
@@ -46,8 +46,9 @@ void komeda_pipeline_destroy(struct komeda_dev *mdev,
{
struct komeda_component *c;
int i;
+ unsigned long avail_comps = pipe->avail_comps;
- dp_for_each_set_bit(i, pipe->avail_comps) {
+ dp_for_each_set_bit(i, avail_comps) {
c = komeda_pipeline_get_component(pipe, i);
komeda_component_destroy(mdev, c);
}
@@ -247,6 +248,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline
*pipe)
{
struct komeda_component *c;
int id;
+ unsigned long avail_comps = pipe->avail_comps;
DRM_INFO("Pipeline-%d: n_layers: %d, n_scalers: %d, output: %s.\n",
pipe->id, pipe->n_layers, pipe->n_scalers,
@@ -258,7 +260,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline
*pipe)
pipe->of_output_links[1] ?
pipe->of_output_links[1]->full_name : "none");
- dp_for_each_set_bit(id, pipe->avail_comps) {
+ dp_for_each_set_bit(id, avail_comps) {
c = komeda_pipeline_get_component(pipe, id);
komeda_component_dump(c);
@@ -270,8 +272,9 @@ static void komeda_component_verify_inputs(struct
komeda_component *c)
struct komeda_pipeline *pipe = c->pipeline;
struct komeda_component *input;
int id;
+ unsigned long supported_inputs = c->supported_inputs;
- dp_for_each_set_bit(id, c->supported_inputs) {
+ dp_for_each_set_bit(id, supported_inputs) {
input = komeda_pipeline_get_component(pipe, id);
if (!input) {
c->supported_inputs &= ~(BIT(id));
@@ -302,8 +305,9 @@ static void komeda_pipeline_assemble(struct komeda_pipeline
*pipe)
struct komeda_component *c;
struct komeda_layer *layer;
int i, id;
+ unsigned long avail_comps = pipe->avail_comps;
- dp_for_each_set_bit(id, pipe->avail_comps) {
+ dp_for_each_set_bit(id, avail_comps) {
c = komeda_pipeline_get_component(pipe, id);
komeda_component_verify_inputs(c);
}
@@ -355,13 +359,15 @@ void komeda_pipeline_dump_register(struct komeda_pipeline
*pipe,
{
struct komeda_component *c;
u32 id;
+ unsigned long avail_comps;
seq_printf(sf, "\n Pipeline-%d ==\n", pipe->id);
if (pipe->funcs && pipe->funcs->dump_register)
pipe->funcs->dump_register(pipe, sf);
- dp_for_each_set_bit(id, pipe->avail_comps) {
+ avail_comps = pipe->avail_comps;
+ dp_for_each_set_bit(id, avail_comps) {
c = komeda_pipeline_get_component(pipe, id);
seq_printf(sf, "\n--%s--\n", c->name);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index e8b1e15312d8..7640dae7f4bf 1006
Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type
On 1/21/21 4:40 PM, Steven Price wrote:
On 21/01/2021 12:22, Carsten Haitzler wrote:
On 1/20/21 3:44 PM, Steven Price wrote:
On 18/01/2021 14:20, carsten.haitz...@foss.arm.com wrote:
From: Carsten Haitzler
Another issue found by KASAN. The bit finding is bueried inside the
NIT: s/bueried/buried/
Yup. typo not spotted by me. Thanks. Also - i spotted an accidental
whitespace change along the way so can fix both in a new patch.
dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.
This seems like an underlying bug/lack of clear documentation for the
underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the
right thing:
Correct. This was a general problem I spotted - the bit funcs were
written to want a unsigned long but were being used on u32's by just
casting the ptr to the type and this did indeed have technical issues.
#define dp_for_each_set_bit(bit, mask) \
for_each_set_bit((bit), ((unsigned long *)&(mask)),
sizeof(mask) * 8)
i.e. passing the actual size of type. However because of the case to
unsigned long (and subsequent accesses using that type) the compiler is
free to make accesses beyond the size of the variable (and indeed this
is completely broken on big-endian). The implementation actually
requires that the bitmap is really an array of unsigned long - no other
type will work.
Precisely. So a bit loose. The bit funcs are used widely enough, so
just fixing this code here to pass in the expected type is probably
the least disruptive fix.
So I think the fix should also include fixing the dp_for_each_set_bit()
macro - the cast is bogus as it stands.
Yup. Removing the cast does indeed find more badness that needs
fixing. I'll do an updated patch with this.
Doing that I also get warnings on komeda_pipeline::avail_comps and
komeda_pipeline::supported_inputs - although I note there are other
bitmasks mentioned.
The other option is to fix dp_for_each_set_bit() itself using a
little hack:
- for_each_set_bit((bit), ((unsigned long *)&(mask)),
sizeof(mask) * 8)
+ for_each_set_bit((bit), (&((unsigned long){mask})),
sizeof(mask) * 8)
With that I don't think you need any other change as the mask is
actually
in a new unsigned long on the stack.
That would be wonderful if it worked :). Unfortunately your above
proposal leads to:
./drivers/gpu/drm/arm/display/komeda/../include/malidp_utils.h:17:27:
error: lvalue required as unary ‘&’ operand
17 | for_each_set_bit((bit), (&((unsigned long)(mask))),
sizeof(mask) * 8)
| ^
Looks like you didn't notice the subtle change above. My change uses
braces ('{}') around 'mask' - I believe it's a GCC extension ("compound
literals") and it creates an lvalue so you can then take the address of
it...
Oh... ewww. I did indeed skip the {}'s and just looked at them as ()'s
:) I'm not so hot on using such extensions if it can be avoided. :) I
just removed the cast and fixed up all the usages. Patch will come with
this.
I'm not sure if it's a good approach to the problem or not. The
alternative is to fix up various places to use unsigned longs so you can
use the unwrapped for_each_set_bit().
Steve
./include/linux/bitops.h:34:30: note: in definition of macro
‘for_each_set_bit’
34 | (bit) = find_next_bit((addr), (size), (bit) + 1))
| ^~~~
drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c:1243:2:
note: in expansion of macro ‘dp_for_each_set_bit’
1243 | dp_for_each_set_bit(id, disabling_comps) {
| ^~~
Basically can't take address of an "unnamed local var". :| That is with:
#define dp_for_each_set_bit(bit, mask) \
for_each_set_bit((bit), (&((unsigned long)(mask))),
sizeof(mask) * 8)
I could try have the dp_for_each macro create new local vars on its
own a bit like:
#define dp_for_each_set_bit(bit, mask) \
unsigned long __local_mask = mask; \
for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
But we all know where this leads... (multiple bad places with adding
warnings and potential and pitfalls that then lead with ever more
invasive changes to address like if code in future might do if (x)
dp_for_each...). I'd prefer to be able to write code more loosely
(pass in any time and it just does the right thing), but trying to
balance this with least disruption and ugliness.
Steve
Signed-off-by: Carsten Haitzler
---
.../drm/arm/display/komeda/komeda_pipeline_state.c | 14
--
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git
a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index e8b1e15312d
Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type
On 1/20/21 3:44 PM, Steven Price wrote:
On 18/01/2021 14:20, carsten.haitz...@foss.arm.com wrote:
From: Carsten Haitzler
Another issue found by KASAN. The bit finding is bueried inside the
NIT: s/bueried/buried/
Yup. typo not spotted by me. Thanks. Also - i spotted an accidental
whitespace change along the way so can fix both in a new patch.
dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.
This seems like an underlying bug/lack of clear documentation for the
underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the
right thing:
Correct. This was a general problem I spotted - the bit funcs were
written to want a unsigned long but were being used on u32's by just
casting the ptr to the type and this did indeed have technical issues.
#define dp_for_each_set_bit(bit, mask) \
for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask)
* 8)
i.e. passing the actual size of type. However because of the case to
unsigned long (and subsequent accesses using that type) the compiler is
free to make accesses beyond the size of the variable (and indeed this
is completely broken on big-endian). The implementation actually
requires that the bitmap is really an array of unsigned long - no other
type will work.
Precisely. So a bit loose. The bit funcs are used widely enough, so just
fixing this code here to pass in the expected type is probably the least
disruptive fix.
So I think the fix should also include fixing the dp_for_each_set_bit()
macro - the cast is bogus as it stands.
Yup. Removing the cast does indeed find more badness that needs fixing.
I'll do an updated patch with this.
Doing that I also get warnings on komeda_pipeline::avail_comps and
komeda_pipeline::supported_inputs - although I note there are other
bitmasks mentioned.
The other option is to fix dp_for_each_set_bit() itself using a little
hack:
- for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask)
* 8)
+ for_each_set_bit((bit), (&((unsigned long){mask})), sizeof(mask)
* 8)
With that I don't think you need any other change as the mask is actually
in a new unsigned long on the stack.
That would be wonderful if it worked :). Unfortunately your above
proposal leads to:
./drivers/gpu/drm/arm/display/komeda/../include/malidp_utils.h:17:27:
error: lvalue required as unary ‘&’ operand
17 | for_each_set_bit((bit), (&((unsigned long)(mask))),
sizeof(mask) * 8)
| ^
./include/linux/bitops.h:34:30: note: in definition of macro
‘for_each_set_bit’
34 | (bit) = find_next_bit((addr), (size), (bit) + 1))
| ^~~~
drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c:1243:2: note:
in expansion of macro ‘dp_for_each_set_bit’
1243 | dp_for_each_set_bit(id, disabling_comps) {
| ^~~
Basically can't take address of an "unnamed local var". :| That is with:
#define dp_for_each_set_bit(bit, mask) \
for_each_set_bit((bit), (&((unsigned long)(mask))),
sizeof(mask) * 8)
I could try have the dp_for_each macro create new local vars on its own
a bit like:
#define dp_for_each_set_bit(bit, mask) \
unsigned long __local_mask = mask; \
for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
But we all know where this leads... (multiple bad places with adding
warnings and potential and pitfalls that then lead with ever more
invasive changes to address like if code in future might do if (x)
dp_for_each...). I'd prefer to be able to write code more loosely (pass
in any time and it just does the right thing), but trying to balance
this with least disruption and ugliness.
Steve
Signed-off-by: Carsten Haitzler
---
.../drm/arm/display/komeda/komeda_pipeline_state.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git
a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index e8b1e15312d8..f7dddb9f015d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct
komeda_pipeline *pipe,
struct komeda_pipeline_state *old =
priv_to_pipe_st(pipe->obj.state);
struct komeda_component_state *c_st;
struct komeda_component *c;
- u32 disabling_comps, id;
+ u32 id;
+ unsigned long disabling_comps;
WARN_ON(!old);
@@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct
komeda_pipeline *pipe,
st = komeda_pipeline_get_new_state(pipe, drm_st);
else
st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st,
N
[PATCH] drm/komeda: Fix bit check to import to value of proper type
From: Carsten Haitzler
Another issue found by KASAN. The bit finding is bueried inside the
dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.
Signed-off-by: Carsten Haitzler
---
.../drm/arm/display/komeda/komeda_pipeline_state.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index e8b1e15312d8..f7dddb9f015d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct komeda_pipeline
*pipe,
struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
struct komeda_component_state *c_st;
struct komeda_component *c;
- u32 disabling_comps, id;
+ u32 id;
+ unsigned long disabling_comps;
WARN_ON(!old);
@@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct
komeda_pipeline *pipe,
st = komeda_pipeline_get_new_state(pipe, drm_st);
else
st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, NULL);
-
if (WARN_ON(IS_ERR_OR_NULL(st)))
return -EINVAL;
@@ -1287,7 +1287,8 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
struct komeda_pipeline_state *old;
struct komeda_component *c;
struct komeda_component_state *c_st;
- u32 id, disabling_comps = 0;
+ u32 id;
+ unsigned long disabling_comps;
old = komeda_pipeline_get_old_state(pipe, old_state);
@@ -1297,7 +1298,7 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
disabling_comps = old->active_comps &
pipe->standalone_disabled_comps;
- DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%x.\n",
+ DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps:
0x%lx.\n",
pipe->id, old->active_comps, disabling_comps);
dp_for_each_set_bit(id, disabling_comps) {
@@ -1331,13 +1332,14 @@ void komeda_pipeline_update(struct komeda_pipeline
*pipe,
struct komeda_pipeline_state *new = priv_to_pipe_st(pipe->obj.state);
struct komeda_pipeline_state *old;
struct komeda_component *c;
- u32 id, changed_comps = 0;
+ u32 id;
+ unsigned long changed_comps;
old = komeda_pipeline_get_old_state(pipe, old_state);
changed_comps = new->active_comps | old->active_comps;
- DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
+ DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
pipe->id, new->active_comps, changed_comps);
dp_for_each_set_bit(id, changed_comps) {
--
2.29.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/komeda: Fix bit check to import to value of proper type
From: Carsten Haitzler
KASAN found this problem. find_first_bit() expects to look at a
pointer pointing to a long, but we look at a u32 - this is going to be
an issue with endianess but, KSAN already flags this as out-of-bounds
stack reads. This fixes it by just importing inot a local long.
Signed-off-by: Carsten Haitzler
---
drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
index 452e505a1fd3..719a79728e24 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
@@ -137,9 +137,10 @@ komeda_pipeline_get_first_component(struct komeda_pipeline
*pipe,
u32 comp_mask)
{
struct komeda_component *c = NULL;
+ unsigned long comp_mask_local = (unsigned long)comp_mask;
int id;
- id = find_first_bit((unsigned long *)_mask, 32);
+ id = find_first_bit(_mask_local, 32);
if (id < 32)
c = komeda_pipeline_get_component(pipe, id);
--
2.29.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/komeda: Handle NULL pointer access code path in error case
From: Carsten Haitzler komeda_component_get_old_state() technically can return a NULL pointer. komeda_compiz_set_input() even warns when this happens, but then proceeeds to use that NULL pointer tocompare memory content there agains the new sate to see if it changed. In this case, it's better to assume that the input changed as there is no old state to compare against and thus assume the changes happen anyway. Signed-off-by: Carsten Haitzler --- drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c index 8f32ae7c25d0..e8b1e15312d8 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c @@ -707,7 +707,8 @@ komeda_compiz_set_input(struct komeda_compiz *compiz, WARN_ON(!old_st); /* compare with old to check if this input has been changed */ - if (memcmp(&(to_compiz_st(old_st)->cins[idx]), cin, sizeof(*cin))) + if (!old_st || + memcmp(&(to_compiz_st(old_st)->cins[idx]), cin, sizeof(*cin))) c_st->changed_active_inputs |= BIT(idx); komeda_component_add_input(c_st, >input, idx); -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/komeda: Remove useless variable assignment
From: Carsten Haitzler
ret is not actually read after this (only written in one case then
returned), so this assign line is useless. This removes that assignment.
Signed-off-by: Carsten Haitzler
---
drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
index 1d767473ba8a..eea76f51f662 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.c
@@ -163,7 +163,6 @@ static int komeda_parse_dt(struct device *dev, struct
komeda_dev *mdev)
ret = of_reserved_mem_device_init(dev);
if (ret && ret != -ENODEV)
return ret;
- ret = 0;
for_each_available_child_of_node(np, child) {
if (of_node_name_eq(child, "pipeline")) {
--
2.29.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
