[PATCH] drm: Fix fbcon blank on QEMU graphics drivers
Currently the DRM fbcon helper for console blank,
drm_fb_helper_blank(), simply calls drm_fb_helper_dpms() and always
returns zero, supposing the driver dealing with DPMS or atomic
crtc->active flip to handle blanking the screen. It works on most of
devices, but broken on most of KVM/QEMU graphics: bochs, qxl and
cirrus drivers just ignore crtc->active state change as blanking (or
cirrus ignoring DPMS). In practice, when you run like
% setterm --blank force
on a VT console, the screen freezes without actually blanking.
A simple fix for this problem would be not to rely on DPMS but let
fbcon performs the generic blank code. This can be achieved just by
returning an error from drm_fb_helper_blank().
In this patch, we add a flag, no_dpms_blank, to drm_fb_helper for
indicating that the driver doesn't handle blank via DPMS or
crtc->active flip. When this flag is set, drm_fb_helper_blank()
simply returns an error, so that fbcon falls back to its generic blank
handler. The flag is set to both bochs and qxl drivers in this patch,
while cirrus is left untouched as it's declared as to-be-deprecated.
Link: https://lore.kernel.org/dri-devel/20170726205636.19144-1-ti...@suse.de/
BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1095700
Signed-off-by: Takashi Iwai
---
Here I whip a dead horse again, revisiting the long-standing issue
stated in the previous patch set in 2017:
https://lore.kernel.org/dri-devel/20170726205636.19144-1-ti...@suse.de/
I thought to refresh the previous patch set at first, but it seems
invalid for the atomic modeset case. And for the atomic, it's even
more difficult to propagate the return from the bottom to up.
So I ended up with this approach as it's much simpler.
But if there is any better way (even simpler or more robust), I'd
happily rewrite, too.
---
drivers/gpu/drm/bochs/bochs_drv.c | 3 +++
drivers/gpu/drm/drm_fb_helper.c | 5 +
drivers/gpu/drm/qxl/qxl_drv.c | 3 +++
include/drm/drm_fb_helper.h | 8
4 files changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/bochs/bochs_drv.c
b/drivers/gpu/drm/bochs/bochs_drv.c
index b469624fe40d..816899a266ff 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -132,6 +132,9 @@ static int bochs_pci_probe(struct pci_dev *pdev,
goto err_unload;
drm_fbdev_generic_setup(dev, 32);
+ if (dev->fb_helper)
+ dev->fb_helper->no_dpms_blank = true;
+
return ret;
err_unload:
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index f6baa2046124..b892f02ff2f1 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -332,9 +332,14 @@ static void drm_fb_helper_dpms(struct fb_info *info, int
dpms_mode)
*/
int drm_fb_helper_blank(int blank, struct fb_info *info)
{
+ struct drm_fb_helper *fb_helper = info->par;
+
if (oops_in_progress)
return -EBUSY;
+ if (fb_helper->no_dpms_blank)
+ return -EINVAL;
+
switch (blank) {
/* Display: On; HSync: On, VSync: On */
case FB_BLANK_UNBLANK:
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index 1864467f1063..58ecfaeed7c1 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -120,6 +120,9 @@ qxl_pci_probe(struct pci_dev *pdev, const struct
pci_device_id *ent)
goto modeset_cleanup;
drm_fbdev_generic_setup(&qdev->ddev, 32);
+ if (qdev->fb_helper)
+ qdev->fb_helper->no_dpms_blank = true;
+
return 0;
modeset_cleanup:
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 3b273f9ca39a..151be4219c32 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -176,6 +176,14 @@ struct drm_fb_helper {
*/
bool deferred_setup;
+ /**
+* @no_dpms_blank:
+*
+* A flag indicating that the driver doesn't support blanking.
+* Then fbcon core code falls back to its generic handler.
+*/
+ bool no_dpms_blank;
+
/**
* @preferred_bpp:
*
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Fix fbcon blank on QEMU graphics drivers
On Fri, 16 Apr 2021 20:30:05 +0200, Daniel Vetter wrote: > > On Fri, Apr 16, 2021 at 2:53 PM Takashi Iwai wrote: > > > > Currently the DRM fbcon helper for console blank, > > drm_fb_helper_blank(), simply calls drm_fb_helper_dpms() and always > > returns zero, supposing the driver dealing with DPMS or atomic > > crtc->active flip to handle blanking the screen. It works on most of > > devices, but broken on most of KVM/QEMU graphics: bochs, qxl and > > cirrus drivers just ignore crtc->active state change as blanking (or > > cirrus ignoring DPMS). In practice, when you run like > > % setterm --blank force > > on a VT console, the screen freezes without actually blanking. > > > > A simple fix for this problem would be not to rely on DPMS but let > > fbcon performs the generic blank code. This can be achieved just by > > returning an error from drm_fb_helper_blank(). > > > > In this patch, we add a flag, no_dpms_blank, to drm_fb_helper for > > indicating that the driver doesn't handle blank via DPMS or > > crtc->active flip. When this flag is set, drm_fb_helper_blank() > > simply returns an error, so that fbcon falls back to its generic blank > > handler. The flag is set to both bochs and qxl drivers in this patch, > > while cirrus is left untouched as it's declared as to-be-deprecated. > > > > Link: > > https://lore.kernel.org/dri-devel/20170726205636.19144-1-ti...@suse.de/ > > BugLink: https://bugzilla.suse.com/show_bug.cgi?id=1095700 > > Signed-off-by: Takashi Iwai > > Uh we're supposed to fix these drivers to actually blank (scan out > black, worst case), not paper over it in higher levels. Atomic kms is > meant to be a somewhat useful abstraction. So now fbcon blanks, but > your desktop still just freezes. > > > --- > > > > Here I whip a dead horse again, revisiting the long-standing issue > > stated in the previous patch set in 2017: > > https://lore.kernel.org/dri-devel/20170726205636.19144-1-ti...@suse.de/ > > > > I thought to refresh the previous patch set at first, but it seems > > invalid for the atomic modeset case. And for the atomic, it's even > > more difficult to propagate the return from the bottom to up. > > So I ended up with this approach as it's much simpler. > > Yeah that's because atomic assume you can at least blank your screen to black. OK, then there is no other way than fixing those drivers to deal with the blanking... I cooked up an experimental patch for bochs, and will submit later. thanks, Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/bochs: Add screen blanking support
On bochs DRM driver, the execution of "setterm --blank force" results
in a frozen screen instead of a blank screen. It's due to the lack of
the screen blanking support in its code.
Actually, the QEMU bochs vga side can switch to the blanking mode when
the bit 0x20 is cleared on VGA_ATT_IW register (0x3c0), which updates
ar_index in QEMU side. So, essentially, we'd just need to clear the
bit at pipe disable callback; that's what this patch does essentially.
However, a tricky part is that the QEMU vga code does treat VGA_ATT_IW
register always as "flip-flop"; the first write is for index and the
second write is for the data like palette. Meanwhile, in the current
bochs DRM driver, the flip-flop wasn't considered, and it calls only
the register update once with the value 0x20.
So, in this patch, we fix the behavior by simply writing the
VGA_ATT_IW register value twice at each place as well.
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/bochs/bochs_hw.c | 13 -
drivers/gpu/drm/bochs/bochs_kms.c | 8
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
index 2d7380a9890e..9a6f90216d6c 100644
--- a/drivers/gpu/drm/bochs/bochs_hw.c
+++ b/drivers/gpu/drm/bochs/bochs_hw.c
@@ -213,6 +213,14 @@ void bochs_hw_setmode(struct bochs_device *bochs,
if (!drm_dev_enter(bochs->dev, &idx))
return;
+ if (!mode) {
+ DRM_DEBUG_DRIVER("crtc disabled\n");
+ /* set to blank mode; send twice for ar_flip_flop */
+ bochs_vga_writeb(bochs, 0x3c0, 0);
+ bochs_vga_writeb(bochs, 0x3c0, 0);
+ goto exit;
+ }
+
bochs->xres = mode->hdisplay;
bochs->yres = mode->vdisplay;
bochs->bpp = 32;
@@ -223,7 +231,9 @@ void bochs_hw_setmode(struct bochs_device *bochs,
bochs->xres, bochs->yres, bochs->bpp,
bochs->yres_virtual);
- bochs_vga_writeb(bochs, 0x3c0, 0x20); /* unblank */
+ /* unblank; send twice for ar_flip_flop */
+ bochs_vga_writeb(bochs, 0x3c0, 0x20);
+ bochs_vga_writeb(bochs, 0x3c0, 0x20);
bochs_dispi_write(bochs, VBE_DISPI_INDEX_ENABLE, 0);
bochs_dispi_write(bochs, VBE_DISPI_INDEX_BPP, bochs->bpp);
@@ -239,6 +249,7 @@ void bochs_hw_setmode(struct bochs_device *bochs,
bochs_dispi_write(bochs, VBE_DISPI_INDEX_ENABLE,
VBE_DISPI_ENABLED | VBE_DISPI_LFB_ENABLED);
+ exit:
drm_dev_exit(idx);
}
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c
b/drivers/gpu/drm/bochs/bochs_kms.c
index 853081d186d5..b0d77d6d3ae4 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -57,6 +57,13 @@ static void bochs_pipe_enable(struct drm_simple_display_pipe
*pipe,
bochs_plane_update(bochs, plane_state);
}
+static void bochs_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+ struct bochs_device *bochs = pipe->crtc.dev->dev_private;
+
+ bochs_hw_setmode(bochs, NULL);
+}
+
static void bochs_pipe_update(struct drm_simple_display_pipe *pipe,
struct drm_plane_state *old_state)
{
@@ -67,6 +74,7 @@ static void bochs_pipe_update(struct drm_simple_display_pipe
*pipe,
static const struct drm_simple_display_pipe_funcs bochs_pipe_funcs = {
.enable = bochs_pipe_enable,
+ .disable= bochs_pipe_disable,
.update = bochs_pipe_update,
.prepare_fb = drm_gem_vram_simple_display_pipe_prepare_fb,
.cleanup_fb = drm_gem_vram_simple_display_pipe_cleanup_fb,
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bochs: Add screen blanking support
On Tue, 20 Apr 2021 19:47:31 +0200, Thomas Zimmermann wrote: > > Hi > > Am 20.04.21 um 18:56 schrieb Takashi Iwai: > > On bochs DRM driver, the execution of "setterm --blank force" results > > in a frozen screen instead of a blank screen. It's due to the lack of > > the screen blanking support in its code. > > > > Actually, the QEMU bochs vga side can switch to the blanking mode when > > the bit 0x20 is cleared on VGA_ATT_IW register (0x3c0), which updates > > ar_index in QEMU side. So, essentially, we'd just need to clear the > > bit at pipe disable callback; that's what this patch does essentially. > > > > However, a tricky part is that the QEMU vga code does treat VGA_ATT_IW > > register always as "flip-flop"; the first write is for index and the > > second write is for the data like palette. Meanwhile, in the current > > bochs DRM driver, the flip-flop wasn't considered, and it calls only > > the register update once with the value 0x20. > > > > Unless bochs does things very different, the index should first be > reset by reading 0x3da. Then write the index, then the data. > > https://web.stanford.edu/class/cs140/projects/pintos/specs/freevga/vga/vgareg.htm#attribute Thanks for the pointer! It seems that QEMU stdvga actually implements the discard of flip flop bit by reading 0x3da. Meanwhile, the write of the data isn't needed in this case because we do care only about the enablement bit 0x20 of ar_index. I'll resubmit v2 patch. Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bochs: Add screen blanking support
On Wed, 21 Apr 2021 09:19:42 +0200, Gerd Hoffmann wrote: > > > > However, a tricky part is that the QEMU vga code does treat VGA_ATT_IW > > > register always as "flip-flop"; the first write is for index and the > > > second write is for the data like palette. Meanwhile, in the current > > > bochs DRM driver, the flip-flop wasn't considered, and it calls only > > > the register update once with the value 0x20. > > > > > > > Unless bochs does things very different, the index should first be reset by > > reading 0x3da. Then write the index, then the data. > > > > https://web.stanford.edu/class/cs140/projects/pintos/specs/freevga/vga/vgareg.htm#attribute > > bochs should follow standard vga logic here. > Also a bochs_set_blank(true/false) helper function probably makes sense. OK, I factored it out to bochs_hw_blank() in v2 patch. thanks, Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/bochs: Add screen blanking support
On bochs DRM driver, the execution of "setterm --blank force" results
in a frozen screen instead of a blank screen. It's due to the lack of
the screen blanking support in its code.
Actually, the QEMU bochs vga side can switch to the blanking mode when
the bit 0x20 is cleared on VGA_ATT_IW register (0x3c0), which updates
ar_index in QEMU side. So, essentially, we'd just need to clear the
bit at pipe disable callback; that's what this patch does essentially.
However, a tricky part is that the access via VGA_ATT_IW is done in
"flip-flop"; the first write is for index and the second write is for
the data like palette. Meanwhile, in the current bochs DRM driver,
the flip-flop wasn't considered, and it calls only the register update
once with the value 0x20.
The spec and the actual VGA implementation in QEMU suggests that the
flip flop flag is discarded by reading the CRTC index register
(VGA_IS1_RC, 0x3da). So, in this patch, we add the helper to read a
byte and the call to clear the flip flop flag before changing the
blank / unblank setup via VGA_ATT_IW register.
v1->v2:
* discard ar_flip_flop by reading 0x3da, add bochs_vga_readb()
* include video/vga.h for VGA register definitions
* move the blank/unblank code to bochs_hw_blank()
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/bochs/bochs.h | 1 +
drivers/gpu/drm/bochs/bochs_hw.c | 25 -
drivers/gpu/drm/bochs/bochs_kms.c | 8
3 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index e5bd1d517a18..e9645c612aff 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -78,6 +78,7 @@ struct bochs_device {
int bochs_hw_init(struct drm_device *dev);
void bochs_hw_fini(struct drm_device *dev);
+void bochs_hw_blank(struct bochs_device *bochs, bool blank);
void bochs_hw_setmode(struct bochs_device *bochs,
struct drm_display_mode *mode);
void bochs_hw_setformat(struct bochs_device *bochs,
diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
index 2d7380a9890e..7d3426d8cc69 100644
--- a/drivers/gpu/drm/bochs/bochs_hw.c
+++ b/drivers/gpu/drm/bochs/bochs_hw.c
@@ -7,6 +7,7 @@
#include
#include
+#include
#include "bochs.h"
/* -- */
@@ -24,6 +25,19 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16
ioport, u8 val)
}
}
+static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport)
+{
+ if (WARN_ON(ioport < 0x3c0 || ioport > 0x3df))
+ return 0xff;
+
+ if (bochs->mmio) {
+ int offset = ioport - 0x3c0 + 0x400;
+ return readb(bochs->mmio + offset);
+ } else {
+ return inb(ioport);
+ }
+}
+
static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg)
{
u16 ret = 0;
@@ -205,6 +219,15 @@ void bochs_hw_fini(struct drm_device *dev)
kfree(bochs->edid);
}
+void bochs_hw_blank(struct bochs_device *bochs, bool blank)
+{
+ DRM_DEBUG_DRIVER("hw_blank %d\n", blank);
+ /* discard ar_flip_flop */
+ (void)bochs_vga_readb(bochs, VGA_IS1_RC);
+ /* blank or unblank; we need only update index and set 0x20 */
+ bochs_vga_writeb(bochs, VGA_ATT_W, blank ? 0 : 0x20);
+}
+
void bochs_hw_setmode(struct bochs_device *bochs,
struct drm_display_mode *mode)
{
@@ -223,7 +246,7 @@ void bochs_hw_setmode(struct bochs_device *bochs,
bochs->xres, bochs->yres, bochs->bpp,
bochs->yres_virtual);
- bochs_vga_writeb(bochs, 0x3c0, 0x20); /* unblank */
+ bochs_hw_blank(bochs, false);
bochs_dispi_write(bochs, VBE_DISPI_INDEX_ENABLE, 0);
bochs_dispi_write(bochs, VBE_DISPI_INDEX_BPP, bochs->bpp);
diff --git a/drivers/gpu/drm/bochs/bochs_kms.c
b/drivers/gpu/drm/bochs/bochs_kms.c
index 853081d186d5..99410e77d51a 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -57,6 +57,13 @@ static void bochs_pipe_enable(struct drm_simple_display_pipe
*pipe,
bochs_plane_update(bochs, plane_state);
}
+static void bochs_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+ struct bochs_device *bochs = pipe->crtc.dev->dev_private;
+
+ bochs_hw_blank(bochs, true);
+}
+
static void bochs_pipe_update(struct drm_simple_display_pipe *pipe,
struct drm_plane_state *old_state)
{
@@ -67,6 +74,7 @@ static void bochs_pipe_update(struct drm_simple_display_pipe
*pipe,
static const struct drm_simple_display_pipe_funcs bochs_pipe_funcs = {
.enable = bochs_pipe_enable,
+ .disable= bochs_pipe_disable,
.update = bochs_pipe_update,
.prepare_fb = drm_gem_vra
[PATCH] drm/ast: Fix missing conversions to managed API
The commit 7cbb93d89838 ("drm/ast: Use managed pci functions")
converted a few PCI accessors to the managed API and dropped the
manual pci_iounmap() calls, but it seems to have forgotten converting
pci_iomap() to the managed one. It resulted in the leftover resources
after the driver unbind. Let's fix them.
Fixes: 7cbb93d89838 ("drm/ast: Use managed pci functions")
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/ast/ast_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 0ac3c2039c4b..c29cc7f19863 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -413,7 +413,7 @@ struct ast_private *ast_device_create(const struct
drm_driver *drv,
pci_set_drvdata(pdev, dev);
- ast->regs = pci_iomap(pdev, 1, 0);
+ ast->regs = pcim_iomap(pdev, 1, 0);
if (!ast->regs)
return ERR_PTR(-EIO);
@@ -429,7 +429,7 @@ struct ast_private *ast_device_create(const struct
drm_driver *drv,
/* "map" IO regs if the above hasn't done so already */
if (!ast->ioregs) {
- ast->ioregs = pci_iomap(pdev, 2, 0);
+ ast->ioregs = pcim_iomap(pdev, 2, 0);
if (!ast->ioregs)
return ERR_PTR(-EIO);
}
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/bochs: Add screen blanking support
On Mon, 26 Apr 2021 20:44:55 +0200,
Thomas Zimmermann wrote:
>
> Hi
>
> Am 21.04.21 um 10:08 schrieb Takashi Iwai:
> > On bochs DRM driver, the execution of "setterm --blank force" results
> > in a frozen screen instead of a blank screen. It's due to the lack of
> > the screen blanking support in its code.
> >
> > Actually, the QEMU bochs vga side can switch to the blanking mode when
> > the bit 0x20 is cleared on VGA_ATT_IW register (0x3c0), which updates
> > ar_index in QEMU side. So, essentially, we'd just need to clear the
> > bit at pipe disable callback; that's what this patch does essentially.
> >
> > However, a tricky part is that the access via VGA_ATT_IW is done in
> > "flip-flop"; the first write is for index and the second write is for
> > the data like palette. Meanwhile, in the current bochs DRM driver,
> > the flip-flop wasn't considered, and it calls only the register update
> > once with the value 0x20.
>
> I read up on the details of what the attribute registers do and what
> you're modifying is the PAS field in the attribute index register. It
> controls write access to the attribute fields.
>
>
> https://web.stanford.edu/class/cs140/projects/pintos/specs/freevga/vga/attrreg.htm#3C0
>
> It's located in the index register and cleared while
> attributes/palettes are updated. I guess that in this mode the stdvga
> disables the palette entirely (hence the screen turns dark).
>
> While it works, it feels wrong to do this.
>
> I to do blanking/unblanking with the SR field in SEQ0
>
>
> https://web.stanford.edu/class/cs140/projects/pintos/specs/freevga/vga/seqreg.htm#00
>
> That's what drivers usually do AFAICT. I think the 'unblank' comment
> next to the existing code might be misleading.
Yeah, when you look at the existing vga16fb.c in kernel, we can find a
relatively complex blanking procedure. OTOH, what I changed is rather
based on the the actual behavior of bochs is more or less simplified.
QEMU hw/display/vga.c contains a code like:
static void vga_update_display(void *opaque)
{
VGACommonState *s = opaque;
DisplaySurface *surface = qemu_console_surface(s->con);
int full_update, graphic_mode;
qemu_flush_coalesced_mmio_buffer();
if (surface_bits_per_pixel(surface) == 0) {
/* nothing to do */
} else {
full_update = 0;
if (!(s->ar_index & 0x20)) {
graphic_mode = GMODE_BLANK;
} else {
graphic_mode = s->gr[VGA_GFX_MISC] & VGA_GR06_GRAPHICS_MODE;
}
if (graphic_mode != s->graphic_mode) {
s->graphic_mode = graphic_mode;
s->cursor_blink_time = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
full_update = 1;
}
switch(graphic_mode) {
case GMODE_TEXT:
vga_draw_text(s, full_update);
break;
case GMODE_GRAPH:
vga_draw_graphic(s, full_update);
break;
case GMODE_BLANK:
default:
vga_draw_blank(s, full_update);
break;
}
}
}
So, it simply checks the ar_index () at each update and switches
from/to the blank mode depending on the bit 0x20.
I'm fine to change in any better way, of course, so feel free to
modify the patch.
thanks,
Takashi
>
> Best regards
> Thomas
>
> >
> > The spec and the actual VGA implementation in QEMU suggests that the
> > flip flop flag is discarded by reading the CRTC index register
> > (VGA_IS1_RC, 0x3da). So, in this patch, we add the helper to read a
> > byte and the call to clear the flip flop flag before changing the
> > blank / unblank setup via VGA_ATT_IW register.
> >
> > v1->v2:
> > * discard ar_flip_flop by reading 0x3da, add bochs_vga_readb()
> > * include video/vga.h for VGA register definitions
> > * move the blank/unblank code to bochs_hw_blank()
> >
> > Signed-off-by: Takashi Iwai
> > ---
> > drivers/gpu/drm/bochs/bochs.h | 1 +
> > drivers/gpu/drm/bochs/bochs_hw.c | 25 -
> > drivers/gpu/drm/bochs/bochs_kms.c | 8
> > 3 files changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
> > index e5bd1d517a18..e9645c612aff 100644
> > --- a/drivers/gpu/drm/bochs/bochs.h
> > +++ b/drivers/gpu/drm/bochs/bochs.h
> > @@ -78,6 +78,7 @@ struct bochs_device {
> > int bochs_hw_init(struct drm_device *dev);
> > void bochs_hw_fini(struct drm_device *dev);
> > +void bochs_hw_blank(struct bochs_device *bochs, bool blank);
> > void bochs_hw_setmode(struct bochs_d
Re: [PATCH] drm/vc4: Unify PCM card's driver_name
On Fri, 15 Jan 2021 20:12:09 +0100,
Nicolas Saenz Julienne wrote:
>
> User-space ALSA matches a card's driver name against an internal list of
> aliases in order to select the correct configuration for the system.
> When the driver name isn't defined, the match is performed against the
> card's name.
>
> With the introduction of RPi4 we now have two HDMI ports with two
> distinct audio cards. This is reflected in their names, making them
> different from previous RPi versions. With this, ALSA ultimately misses
> the board's configuration on RPi4.
>
> In order to avoid this, set "card->driver_name" to "vc4-hdmi"
> unanimously.
>
> Signed-off-by: Nicolas Saenz Julienne
> Fixes: f437bc1ec731 ("drm/vc4: drv: Support BCM2711")
Looks good to me.
Reviewed-by: Takashi Iwai
thanks,
Takashi
> ---
> drivers/gpu/drm/vc4/vc4_hdmi.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 97f368bc1c67..4bdc8e71b5e5 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -1404,6 +1404,7 @@ static int vc4_hdmi_audio_init(struct vc4_hdmi
> *vc4_hdmi)
> card->dai_link = dai_link;
> card->num_links = 1;
> card->name = vc4_hdmi->variant->card_name;
> + card->driver_name = "vc4-hdmi";
> card->dev = dev;
> card->owner = THIS_MODULE;
>
> --
> 2.29.2
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/5] amba: Make the remove callback return void
On Tue, 26 Jan 2021 17:58:34 +0100, Uwe Kleine-König wrote: > > All amba drivers return 0 in their remove callback. Together with the > driver core ignoring the return value anyhow, it doesn't make sense to > return a value here. > > Change the remove prototype to return void, which makes it explicit that > returning an error value doesn't work as expected. This simplifies changing > the core remove callback to return void, too. > > Reviewed-by: Ulf Hansson > Reviewed-by: Arnd Bergmann > Acked-by: Alexandre Belloni > Acked-by: Dmitry Torokhov > Acked-by: Krzysztof Kozlowski # for drivers/memory > Acked-by: Mark Brown > Acked-by: Dmitry Torokhov > Acked-by: Linus Walleij > Signed-off-by: Uwe Kleine-König > --- > drivers/amba/bus.c | 5 ++--- > drivers/char/hw_random/nomadik-rng.c | 3 +-- > drivers/dma/pl330.c| 3 +-- > drivers/gpu/drm/pl111/pl111_drv.c | 4 +--- > drivers/hwtracing/coresight/coresight-catu.c | 3 +-- > drivers/hwtracing/coresight/coresight-cpu-debug.c | 4 +--- > drivers/hwtracing/coresight/coresight-cti-core.c | 4 +--- > drivers/hwtracing/coresight/coresight-etb10.c | 4 +--- > drivers/hwtracing/coresight/coresight-etm3x-core.c | 4 +--- > drivers/hwtracing/coresight/coresight-etm4x-core.c | 4 +--- > drivers/hwtracing/coresight/coresight-funnel.c | 4 ++-- > drivers/hwtracing/coresight/coresight-replicator.c | 4 ++-- > drivers/hwtracing/coresight/coresight-stm.c| 4 +--- > drivers/hwtracing/coresight/coresight-tmc-core.c | 4 +--- > drivers/hwtracing/coresight/coresight-tpiu.c | 4 +--- > drivers/i2c/busses/i2c-nomadik.c | 4 +--- > drivers/input/serio/ambakmi.c | 3 +-- > drivers/memory/pl172.c | 4 +--- > drivers/memory/pl353-smc.c | 4 +--- > drivers/mmc/host/mmci.c| 4 +--- > drivers/rtc/rtc-pl030.c| 4 +--- > drivers/rtc/rtc-pl031.c| 4 +--- > drivers/spi/spi-pl022.c| 5 ++--- > drivers/tty/serial/amba-pl010.c| 4 +--- > drivers/tty/serial/amba-pl011.c| 3 +-- > drivers/vfio/platform/vfio_amba.c | 3 +-- > drivers/video/fbdev/amba-clcd.c| 4 +--- > drivers/watchdog/sp805_wdt.c | 4 +--- > include/linux/amba/bus.h | 2 +- > sound/arm/aaci.c | 4 +--- For the sound/*: Acked-by: Takashi Iwai thanks, Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/amd/display: Fix the brightness read via aux
The current code tries to read the brightness value via
dc_link_get_backlight_level() no matter whether it's controlled via
aux or not, and this results in a bogus value returned.
Fix it to read the current value via
dc_link_get_backlight_level_nits() for the aux.
BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1180749
BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1438
Signed-off-by: Takashi Iwai
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 -
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index c6da89df055d..fb62886ae013 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3140,6 +3140,16 @@ static int set_backlight_via_aux(struct dc_link *link,
uint32_t brightness)
return rc ? 0 : 1;
}
+static int get_backlight_via_aux(struct dc_link *link)
+{
+ uint32_t avg, peak;
+
+ if (!link ||
+ !dc_link_get_backlight_level_nits(link, &avg, &peak))
+ return DC_ERROR_UNEXPECTED;
+ return avg;
+}
+
static int get_brightness_range(const struct amdgpu_dm_backlight_caps *caps,
unsigned *min, unsigned *max)
{
@@ -3212,8 +3222,13 @@ static int amdgpu_dm_backlight_update_status(struct
backlight_device *bd)
static int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd)
{
struct amdgpu_display_manager *dm = bl_get_data(bd);
- int ret = dc_link_get_backlight_level(dm->backlight_link);
+ struct dc_link *link = (struct dc_link *)dm->backlight_link;
+ int ret;
+ if (dm->backlight_caps.aux_support)
+ ret = get_backlight_via_aux(link);
+ else
+ ret = dc_link_get_backlight_level(link);
if (ret == DC_ERROR_UNEXPECTED)
return bd->props.brightness;
return convert_brightness_to_user(&dm->backlight_caps, ret);
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/amd/display: Add aux_backlight module option
There seem devices that don't work with the aux channel backlight control. For allowing such users to test with the other backlight control method, provide a new module option, aux_backlight, to specify enabling or disabling the aux backport support explicitly. As default, the aux support is detected by the hardware capability. BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1180749 BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1438 Signed-off-by: Takashi Iwai --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++ 3 files changed, 8 insertions(+) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 5993dd0fdd8e..4793cd5e69f9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -179,6 +179,7 @@ extern uint amdgpu_smu_memory_pool_size; extern uint amdgpu_dc_feature_mask; extern uint amdgpu_dc_debug_mask; extern uint amdgpu_dm_abm_level; +extern int amdgpu_aux_backlight; extern struct amdgpu_mgpu_info mgpu_info; extern int amdgpu_ras_enable; extern uint amdgpu_ras_mask; diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 7169fb5e3d9c..5b66822da954 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -777,6 +777,10 @@ uint amdgpu_dm_abm_level; MODULE_PARM_DESC(abmlevel, "ABM level (0 = off (default), 1-4 = backlight reduction level) "); module_param_named(abmlevel, amdgpu_dm_abm_level, uint, 0444); +int amdgpu_aux_backlight = -1; +MODULE_PARM_DESC(aux_backlight, "Aux backlight control (0 = off, 1 = on, default auto)"); +module_param_named(aux_backlight, amdgpu_aux_backlight, bint, 0444); + /** * DOC: tmz (int) * Trusted Memory Zone (TMZ) is a method to protect data being written diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c index fb62886ae013..6ad384ef61b8 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -2209,6 +2209,9 @@ static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector) caps->ext_caps->bits.hdr_aux_backlight_control == 1) caps->aux_support = true; + if (amdgpu_aux_backlight >= 0) + caps->aux_support = amdgpu_aux_backlight; + /* From the specification (CTA-861-G), for calculating the maximum * luminance we need to use: * Luminance = 50*2**(CV/32) -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/2] drm/amd/display: some backlight fixes
Hi, here are two patches handling the issues in the backlight control. The first one is to fix the sysfs brightness read for devices with the backlight controlled by aux channel. The second one is to add a new module option, aux_backlight, to forcibly enable/disable the aux channel backlight control. It's no direct solution for the bug we've hit, but it gives at least a workaround. BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1180749 BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1438 thanks, Takashi === Takashi Iwai (2): drm/amd/display: Fix the brightness read via aux drm/amd/display: Add aux_backlight module option drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 ++- 3 files changed, 24 insertions(+), 1 deletion(-) -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/amd/display: Add aux_backlight module option
On Fri, 05 Feb 2021 17:34:36 +0100, Alex Deucher wrote: > > On Wed, Feb 3, 2021 at 7:42 AM Takashi Iwai wrote: > > > > There seem devices that don't work with the aux channel backlight > > control. For allowing such users to test with the other backlight > > control method, provide a new module option, aux_backlight, to specify > > enabling or disabling the aux backport support explicitly. As > > default, the aux support is detected by the hardware capability. > > > > BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1180749 > > BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1438 > > Signed-off-by: Takashi Iwai > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 + > > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 > > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++ > > 3 files changed, 8 insertions(+) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > index 5993dd0fdd8e..4793cd5e69f9 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > @@ -179,6 +179,7 @@ extern uint amdgpu_smu_memory_pool_size; > > extern uint amdgpu_dc_feature_mask; > > extern uint amdgpu_dc_debug_mask; > > extern uint amdgpu_dm_abm_level; > > +extern int amdgpu_aux_backlight; > > extern struct amdgpu_mgpu_info mgpu_info; > > extern int amdgpu_ras_enable; > > extern uint amdgpu_ras_mask; > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > > index 7169fb5e3d9c..5b66822da954 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > > @@ -777,6 +777,10 @@ uint amdgpu_dm_abm_level; > > MODULE_PARM_DESC(abmlevel, "ABM level (0 = off (default), 1-4 = backlight > > reduction level) "); > > module_param_named(abmlevel, amdgpu_dm_abm_level, uint, 0444); > > > > +int amdgpu_aux_backlight = -1; > > +MODULE_PARM_DESC(aux_backlight, "Aux backlight control (0 = off, 1 = on, > > default auto)"); > > +module_param_named(aux_backlight, amdgpu_aux_backlight, bint, 0444); > > I'd suggest making this something more generic like "backlight" and > make -1 auto, 0 pwm, 1 aux. That way we can handle potential future > types more cleanly. OK, will respin later. thanks, Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/amd/display: Fix the brightness read via aux
On Fri, 05 Feb 2021 17:36:44 +0100, Alex Deucher wrote: > > On Wed, Feb 3, 2021 at 7:42 AM Takashi Iwai wrote: > > > > The current code tries to read the brightness value via > > dc_link_get_backlight_level() no matter whether it's controlled via > > aux or not, and this results in a bogus value returned. > > Fix it to read the current value via > > dc_link_get_backlight_level_nits() for the aux. > > > > BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1180749 > > BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1438 > > Signed-off-by: Takashi Iwai > > This looks fine to me. FWIW, I have a similar patch set here: > https://cgit.freedesktop.org/~agd5f/linux/log/?h=backlight_wip I'm fine to scratch mine as long as the issue gets fixed :) FWIW, the biggest problem so far was the aux channel backlight didn't work as expected, the actual backlight isn't changed by the backlight sysfs write. (And the sysfs read gives a bogus value, but it's not the cause of the non-working backlight control.) Does the aux channel backlight really work with the current code? Or is this rather a device-specific issue (e.g. broken BIOS) and we might need to come up with a deny list or such? thanks, Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] drm: Use USB controller's DMA mask when importing dmabufs
On Tue, 23 Feb 2021 11:58:42 +0100,
Thomas Zimmermann wrote:
>
> USB devices cannot perform DMA and hence have no dma_mask set in their
> device structure. Importing dmabuf into a USB-based driver fails, which
> break joining and mirroring of display in X11.
>
> For USB devices, pick the associated USB controller as attachment device,
> so that it can perform DMA. If the DMa controller does not support DMA
> transfers, we're aout of luck and cannot import.
>
> Drivers should use DRM_GEM_SHMEM_DROVER_OPS_USB to initialize their
> instance of struct drm_driver.
>
> Tested by joining/mirroring displays of udl and radeon un der Gnome/X11.
>
> v3:
> * drop gem_create_object
> * use DMA mask of USB controller, if any (Daniel, Christian, Noralf)
> v2:
> * move fix to importer side (Christian, Daniel)
> * update SHMEM and CMA helpers for new PRIME callbacks
>
> Signed-off-by: Thomas Zimmermann
> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> Cc: Christoph Hellwig
> Cc: Greg Kroah-Hartman
> Cc: Johan Hovold
> Cc: Alan Stern
> Cc: Andy Shevchenko
> Cc: Sebastian Andrzej Siewior
> Cc: Mathias Nyman
> Cc: Oliver Neukum
> Cc: Thomas Gleixner
> Cc: # v5.10+
> ---
> drivers/gpu/drm/drm_prime.c| 36 ++
> drivers/gpu/drm/tiny/gm12u320.c| 2 +-
> drivers/gpu/drm/udl/udl_drv.c | 2 +-
> include/drm/drm_gem_shmem_helper.h | 13 +++
> include/drm/drm_prime.h| 5 +
> 5 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 2a54f86856af..9015850f2160 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -1055,3 +1056,38 @@ void drm_prime_gem_destroy(struct drm_gem_object *obj,
> struct sg_table *sg)
> dma_buf_put(dma_buf);
> }
> EXPORT_SYMBOL(drm_prime_gem_destroy);
> +
> +/**
> + * drm_gem_prime_import_usb - helper library implementation of the import
> callback for USB devices
> + * @dev: drm_device to import into
> + * @dma_buf: dma-buf object to import
> + *
> + * This is an implementation of drm_gem_prime_import() for USB-based devices.
> + * USB devices cannot perform DMA directly. This function selects the USB
> host
> + * controller as DMA device instead. Drivers can use this as their
> + * &drm_driver.gem_prime_import implementation.
> + *
> + * See also drm_gem_prime_import().
> + */
> +#ifdef CONFIG_USB
> +struct drm_gem_object *drm_gem_prime_import_usb(struct drm_device *dev,
> + struct dma_buf *dma_buf)
> +{
> + struct usb_device *udev;
> + struct device *usbhost;
> +
> + if (dev->dev->bus != &usb_bus_type)
> + return ERR_PTR(-ENODEV);
> +
> + udev = interface_to_usbdev(to_usb_interface(dev->dev));
> + if (!udev->bus)
> + return ERR_PTR(-ENODEV);
> +
> + usbhost = udev->bus->controller;
Aside from the discussion whether this "workaround" is needed, the use
of udev->bus->controller here looks a bit suspicious. As the old USB
code (before the commit 6eb0233ec2d0) indicated, it was rather
usb->bus->sysdev that was used for the DMA mask, and it's also the one
most of USB core code refers to. A similar question came up while
fixing the same kind of bug in the media subsystem, and we concluded
that bus->sysdev is a better choice.
Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] drm: Use USB controller's DMA mask when importing dmabufs
On Tue, 23 Feb 2021 17:00:54 +0100, Alan Stern wrote: > > On Tue, Feb 23, 2021 at 03:06:07PM +0100, Thomas Zimmermann wrote: > > Hi > > > > Am 23.02.21 um 14:44 schrieb Takashi Iwai: > > > > Aside from the discussion whether this "workaround" is needed, the use > > > of udev->bus->controller here looks a bit suspicious. As the old USB > > > code (before the commit 6eb0233ec2d0) indicated, it was rather > > > usb->bus->sysdev that was used for the DMA mask, and it's also the one > > > most of USB core code refers to. A similar question came up while > > > fixing the same kind of bug in the media subsystem, and we concluded > > > that bus->sysdev is a better choice. > > > > Good to hear that we're not the only ones affected by this. Wrt the original > > code, using sysdev makes even more sense. > > Hmmm, I had forgotten about this. So DMA masks aren't inherited after > all, thanks to commit 6eb0233ec2d0. That leas me to wonder how well > usb-storage is really working these days... > > The impression I get is that Greg would like the USB core to export a > function which takes struct usb_interface * as argument and returns the > appropriate DMA mask value. Then instead of messing around with USB > internals, drm_gem_prime_import_usb could just call this new function. > > Adding such a utility function would be a sufficiently small change that > it could go into the -stable kernels with no trouble. Note that drm isn't the only subsystem that needs the proper DMA mask. The media and sound subsystems require it, at least. Those had already used sysdev for the DMA mask and the actual memory allocation. Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4] drm: Use USB controller's DMA mask when importing dmabufs
On Thu, 25 Feb 2021 08:57:14 +0100,
Thomas Zimmermann wrote:
>
> Hi
>
> Am 24.02.21 um 16:21 schrieb Alan Stern:
> > On Wed, Feb 24, 2021 at 10:23:04AM +0100, Thomas Zimmermann wrote:
> >> USB devices cannot perform DMA and hence have no dma_mask set in their
> >> device structure. Therefore importing dmabuf into a USB-based driver
> >> fails, which breaks joining and mirroring of display in X11.
> >>
> >> For USB devices, pick the associated USB controller as attachment device.
> >> This allows the DRM import helpers to perform the DMA setup. If the DMA
> >> controller does not support DMA transfers, we're out of luck and cannot
> >> import. Our current USB-based DRM drivers don't use DMA, so the actual
> >> DMA device is not important.
> >>
> >> Drivers should use DRM_GEM_SHMEM_DROVER_OPS_USB to initialize their
> >> instance of struct drm_driver.
> >>
> >> Tested by joining/mirroring displays of udl and radeon un der Gnome/X11.
> >>
> >> v4:
> >>* implement workaround with USB helper functions (Greg)
> >>* use struct usb_device->bus->sysdev as DMA device (Takashi)
> >> v3:
> >>* drop gem_create_object
> >>* use DMA mask of USB controller, if any (Daniel, Christian, Noralf)
> >> v2:
> >>* move fix to importer side (Christian, Daniel)
> >>* update SHMEM and CMA helpers for new PRIME callbacks
> >>
> >> Signed-off-by: Thomas Zimmermann
> >> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices")
> >> Cc: Christoph Hellwig
> >> Cc: Greg Kroah-Hartman
> >> Cc: # v5.10+
> >> ---
> >
> >> +struct drm_gem_object *drm_gem_prime_import_usb(struct drm_device *dev,
> >> + struct dma_buf *dma_buf)
> >> +{
> >> + struct usb_device *udev;
> >> + struct device *dmadev;
> >> + struct drm_gem_object *obj;
> >> +
> >> + if (!dev_is_usb(dev->dev))
> >> + return ERR_PTR(-ENODEV);
> >> + udev = interface_to_usbdev(to_usb_interface(dev->dev));
> >> +
> >> + dmadev = usb_get_dma_device(udev);
> >
> > You can do it this way if you want, but I think usb_get_dma_device would
> > be easier to use if its argument was a pointer to struct usb_interface
> > or (even better) a pointer to a usb_interface's embedded struct device.
> > Then you wouldn't need to compute udev, and the same would be true for
> > other callers.
>
> It seemed natural to me to use usb_device, because it contains the bus
> pointer. But maybe a little wrapper for usb_interface in the header
> file makes things easier to read. I'll wait a bit for other reviews to
> come in.
I agree with Thomas, as most of users referring to the sysdev do
access in a pattern like udev->bus->sysdev, AFAIK.
thanks,
Takashi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[5.12 regression] ttm->pages NULL dereference with radeon driver
Hi,
we've received a regression report showing NULL dereference Oops with
radeon driver on 5.12 kernel:
https://bugzilla.opensuse.org/show_bug.cgi?id=1185516
It turned out that the recent TTM cleanup / refactoring via commit
0575ff3d33cd ("drm/radeon: stop using pages with
drm_prime_sg_to_page_addr_arrays v2") is the culprit. On 5.12 kernel,
ttm->pages is no longer allocated / set up, while the radeon driver
still has a few places assuming the valid ttm->pages, and for the
reporter (running the modesettig driver), radeon_gart_bind() hits the
problem.
A hackish patch below was confirmed to work, at least, but obviously
we need a proper fix.
Could you take a look at it?
thanks,
Takashi
--- a/drivers/gpu/drm/radeon/radeon_gart.c
+++ b/drivers/gpu/drm/radeon/radeon_gart.c
@@ -253,7 +253,7 @@ void radeon_gart_unbind(struct radeon_de
t = offset / RADEON_GPU_PAGE_SIZE;
p = t / (PAGE_SIZE / RADEON_GPU_PAGE_SIZE);
for (i = 0; i < pages; i++, p++) {
- if (rdev->gart.pages[p]) {
+ if (1 /*rdev->gart.pages[p]*/) {
rdev->gart.pages[p] = NULL;
for (j = 0; j < (PAGE_SIZE / RADEON_GPU_PAGE_SIZE);
j++, t++) {
rdev->gart.pages_entry[t] =
rdev->dummy_page.entry;
@@ -301,7 +301,7 @@ int radeon_gart_bind(struct radeon_devic
p = t / (PAGE_SIZE / RADEON_GPU_PAGE_SIZE);
for (i = 0; i < pages; i++, p++) {
- rdev->gart.pages[p] = pagelist[i];
+ /* rdev->gart.pages[p] = pagelist[i]; */
page_base = dma_addr[i];
for (j = 0; j < (PAGE_SIZE / RADEON_GPU_PAGE_SIZE); j++, t++) {
page_entry = radeon_gart_get_page_entry(page_base,
flags);
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -360,6 +360,8 @@ static int radeon_ttm_tt_pin_userptr(str
if (current->mm != gtt->usermm)
return -EPERM;
+ if (!ttm->pages)
+ return -EPERM;
if (gtt->userflags & RADEON_GEM_USERPTR_ANONONLY) {
/* check that we only pin down anonymous memory
Re: [PATCH 1/2] ALSA: ppc: drop if block with always false condition
On Thu, 26 Nov 2020 17:59:49 +0100, Uwe Kleine-König wrote: > > The remove callback is only called for devices that were probed > successfully before. As the matching probe function cannot complete > without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to > check this here. > > Signed-off-by: Uwe Kleine-König Applied this one now. Thanks. Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] powerpc/ps3: make system bus's remove and shutdown callbacks return void
On Thu, 26 Nov 2020 17:59:50 +0100,
Uwe Kleine-König wrote:
>
> The driver core ignores the return value of struct device_driver::remove
> because there is only little that can be done. For the shutdown callback
> it's ps3_system_bus_shutdown() which ignores the return value.
>
> To simplify the quest to make struct device_driver::remove return void,
> let struct ps3_system_bus_driver::remove return void, too. All users
> already unconditionally return 0, this commit makes it obvious that
> returning an error code is a bad idea and ensures future users behave
> accordingly.
>
> Signed-off-by: Uwe Kleine-König
For the sound bit:
Acked-by: Takashi Iwai
thanks,
Takashi
> ---
> arch/powerpc/include/asm/ps3.h | 4 ++--
> arch/powerpc/platforms/ps3/system-bus.c | 5 ++---
> drivers/block/ps3disk.c | 3 +--
> drivers/block/ps3vram.c | 3 +--
> drivers/char/ps3flash.c | 3 +--
> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 3 +--
> drivers/ps3/ps3-lpm.c| 3 +--
> drivers/ps3/ps3-vuart.c | 10 --
> drivers/scsi/ps3rom.c| 3 +--
> drivers/usb/host/ehci-ps3.c | 4 +---
> drivers/usb/host/ohci-ps3.c | 4 +---
> drivers/video/fbdev/ps3fb.c | 4 +---
> sound/ppc/snd_ps3.c | 3 +--
> 13 files changed, 18 insertions(+), 34 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ps3.h b/arch/powerpc/include/asm/ps3.h
> index cb89e4bf55ce..e646c7f218bc 100644
> --- a/arch/powerpc/include/asm/ps3.h
> +++ b/arch/powerpc/include/asm/ps3.h
> @@ -378,8 +378,8 @@ struct ps3_system_bus_driver {
> enum ps3_match_sub_id match_sub_id;
> struct device_driver core;
> int (*probe)(struct ps3_system_bus_device *);
> - int (*remove)(struct ps3_system_bus_device *);
> - int (*shutdown)(struct ps3_system_bus_device *);
> + void (*remove)(struct ps3_system_bus_device *);
> + void (*shutdown)(struct ps3_system_bus_device *);
> /* int (*suspend)(struct ps3_system_bus_device *, pm_message_t); */
> /* int (*resume)(struct ps3_system_bus_device *); */
> };
> diff --git a/arch/powerpc/platforms/ps3/system-bus.c
> b/arch/powerpc/platforms/ps3/system-bus.c
> index c62aaa29a9d5..b431f41c6cb5 100644
> --- a/arch/powerpc/platforms/ps3/system-bus.c
> +++ b/arch/powerpc/platforms/ps3/system-bus.c
> @@ -382,7 +382,6 @@ static int ps3_system_bus_probe(struct device *_dev)
>
> static int ps3_system_bus_remove(struct device *_dev)
> {
> - int result = 0;
> struct ps3_system_bus_device *dev = ps3_dev_to_system_bus_dev(_dev);
> struct ps3_system_bus_driver *drv;
>
> @@ -393,13 +392,13 @@ static int ps3_system_bus_remove(struct device *_dev)
> BUG_ON(!drv);
>
> if (drv->remove)
> - result = drv->remove(dev);
> + drv->remove(dev);
> else
> dev_dbg(&dev->core, "%s:%d %s: no remove method\n",
> __func__, __LINE__, drv->core.name);
>
> pr_debug(" <- %s:%d: %s\n", __func__, __LINE__, dev_name(&dev->core));
> - return result;
> + return 0;
> }
>
> static void ps3_system_bus_shutdown(struct device *_dev)
> diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
> index 7b55811c2a81..ba3ece56cbb3 100644
> --- a/drivers/block/ps3disk.c
> +++ b/drivers/block/ps3disk.c
> @@ -507,7 +507,7 @@ static int ps3disk_probe(struct ps3_system_bus_device
> *_dev)
> return error;
> }
>
> -static int ps3disk_remove(struct ps3_system_bus_device *_dev)
> +static void ps3disk_remove(struct ps3_system_bus_device *_dev)
> {
> struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
> struct ps3disk_private *priv = ps3_system_bus_get_drvdata(&dev->sbd);
> @@ -526,7 +526,6 @@ static int ps3disk_remove(struct ps3_system_bus_device
> *_dev)
> kfree(dev->bounce_buf);
> kfree(priv);
> ps3_system_bus_set_drvdata(_dev, NULL);
> - return 0;
> }
>
> static struct ps3_system_bus_driver ps3disk = {
> diff --git a/drivers/block/ps3vram.c b/drivers/block/ps3vram.c
> index 1088798c8dd0..b71d28372ef3 100644
> --- a/drivers/block/ps3vram.c
> +++ b/drivers/block/ps3vram.c
> @@ -797,7 +797,7 @@ static int ps3vram_probe(struct ps3_system_bus_device
> *dev)
> return error;
> }
>
> -static int ps3vram_remove(struct ps3_system_bus_device *dev)
> +static void ps3vram_remove(struct ps3_system_bus_device *dev)
> {
> struc
Re: [PATCH 2/2] powerpc/ps3: make system bus's remove and shutdown callbacks return void
On Wed, 02 Dec 2020 13:14:06 +0100, Michael Ellerman wrote: > > Uwe Kleine-König writes: > > Hello Michael, > > > > On Sat, Nov 28, 2020 at 09:48:30AM +0100, Takashi Iwai wrote: > >> On Thu, 26 Nov 2020 17:59:50 +0100, > >> Uwe Kleine-König wrote: > >> > > >> > The driver core ignores the return value of struct device_driver::remove > >> > because there is only little that can be done. For the shutdown callback > >> > it's ps3_system_bus_shutdown() which ignores the return value. > >> > > >> > To simplify the quest to make struct device_driver::remove return void, > >> > let struct ps3_system_bus_driver::remove return void, too. All users > >> > already unconditionally return 0, this commit makes it obvious that > >> > returning an error code is a bad idea and ensures future users behave > >> > accordingly. > >> > > >> > Signed-off-by: Uwe Kleine-König > >> > >> For the sound bit: > >> Acked-by: Takashi Iwai > > > > assuming that you are the one who will apply this patch: Note that it > > depends on patch 1 that Takashi already applied to his tree. So you > > either have to wait untils patch 1 appears in some tree that you merge > > before applying, or you have to take patch 1, too. (With Takashi > > optinally dropping it then.) > > Thanks. I've picked up both patches. > > If Takashi doesn't want to rebase his tree to drop patch 1 that's OK, it > will just arrive in mainline via two paths, but git should handle it. Yeah, I'd like to avoid rebasing, so let's get it merge from both trees. git can handle such a case gracefully. thanks, Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 2/2] hda/i915: split the wait for the component binding
On Thu, 24 Feb 2022 17:34:54 +0100, Kai Vehmanen wrote: > > Hi, > > On Thu, 24 Feb 2022, Ramalingam C wrote: > > > Split the wait for component binding from i915 in multiples of > > sysctl_hung_task_timeout_secs. This helps to avoid the possible kworker > > thread hung detection given below. > > while I understand the problem, I'm not sure whether a simpler option > should be chosen. Maybe just split the wait_for_completion_timeout() > into small 5sec iterations, without consulting value of hung_task_timeout. > This would seem unligned with more mainstream use of > wait_for_completion_timeout() in kernel and still do the job. > > I'll loop in Takashi here as well. Basicly the 60 sec timeout in > hda/hdac_i915.c is getting caught by hung_task_detection logic in builds > where the hung_task_timeout is below 60secs. > > I have a patch that tries to avoid hitting the timeout in some of the more > common cases: > "ALSA: hda/i915 - skip acomp init if no matching display" > https://lists.freedesktop.org/archives/intel-gfx-trybot/2022-February/128278.html > ... but we'll still be stuck with some configurations where the timeout > will be hit. And above needs careful testing. > > One logic comment below as well, but I'll quote the whole patch to give > context to Takashi. I agree with Kai, we can just make the wait_for_completion_timeout() split in a loop independently from hung_task_timeout. It's simpler, less error-prone. thanks, Takashi
Re: [PATCH v2] component: do not leave master devres group open after bind
On Wed, 22 Sep 2021 10:54:32 +0200, Kai Vehmanen wrote: (snip) > --- a/drivers/base/component.c > +++ b/drivers/base/component.c > @@ -246,7 +246,7 @@ static int try_to_bring_up_master(struct master *master, > return 0; > } > > - if (!devres_open_group(master->parent, NULL, GFP_KERNEL)) > + if (!devres_open_group(master->parent, master, GFP_KERNEL)) > return -ENOMEM; > > /* Found all components */ > @@ -258,6 +258,7 @@ static int try_to_bring_up_master(struct master *master, > return ret; > } > > + devres_close_group(master->parent, NULL); Just wondering whether we should pass master here instead of NULL, too? thanks, Takashi
Re: [PATCH v6 29/35] sound: hdac: Migrate to aggregate driver
On Thu, 27 Jan 2022 21:01:35 +0100,
Stephen Boyd wrote:
>
> Use an aggregate driver instead of component ops so that we can get
> proper driver probe ordering of the aggregate device with respect to all
> the component devices that make up the aggregate device.
>
> Cc: Jaroslav Kysela
> Cc: Takashi Iwai
> Cc: Kai Vehmanen
> Cc: Daniel Vetter
> Cc: "Rafael J. Wysocki"
> Cc: Rob Clark
> Cc: Russell King
> Cc: Saravana Kannan
> Signed-off-by: Stephen Boyd
The patch looks good, but just a minor concern:
> +static struct aggregate_driver hdac_aggregate_driver = {
> + .probe = hdac_component_master_bind,
> + .remove = hdac_component_master_unbind,
> + .driver = {
> + .name = "hdac_agg",
Shouldn't we define some standard name scheme?
This one has "hdac_agg", while the patch for HD-audio Realtek stuff
has "realtek_aggregate".
(And maybe the latter one should be something like
"hda_realtek_agg" or such, as Realtek covers pretty different devices
and there might be conflict in future.)
With those considered: please take my ack
Acked-by: Takashi Iwai
thanks,
Takashi
Why drm-mipi-dsi is built-in only?
On Tue, 02 Aug 2016 13:47:10 +0200, Thierry Reding wrote: > > On Tue, Aug 02, 2016 at 12:47:06PM +0200, Andrzej Hajda wrote: > > On 08/02/2016 10:36 AM, Thierry Reding wrote: > > > On Mon, Aug 01, 2016 at 08:04:55PM +0200, Andrzej Hajda wrote: > > >> On 08/01/2016 03:59 PM, Jani Nikula wrote: > > >>> Cc Andrzej, Thierry > > >>> > > >>> On Fri, 22 Jul 2016, Daniel Vetter wrote: > > >>>> On Fri, Jul 22, 2016 at 04:30:24PM +0200, Takashi Iwai wrote: > > >>>>> Hi, > > >>>>> > > >>>>> is there any reason drm-mipi-dsi can't be a module? It's fixed as a > > >>>>> built-in since its Kconfig is bool. > > >>>> Probably none except embedded folks eshew modules ;-) Submit patch, > > >>>> I'll > > >>>> apply. > > >>> Possibly this? > > >>> > > >>> postcore_initcall(mipi_dsi_bus_init); > > >> If I remember correctly, the only reason for this is to have mipi_dsi bus > > >> registered before mipi_dsi drivers, which usually are registered > > >> at module initcall. But maybe bus registration can be performed at > > >> first mipi_dsi driver registration. This way we could modularize it. > > > I think it should work fine if this was built as a module. The purpose > > > for having this as postcore_initcall() is simply so that the bus is > > > fully initialized before any driver gets registered with it. If this > > > code is built as a module, symbol dependencies will make sure that the > > > drm_mipi_dsi.ko module will be loaded before any users. > > > > If you change initcall of mipi_dsi to module and then you compile > > it as built-in, only link order will guard correct initialization sequence. > > As for now panels are linked after mipi-dsi, so it should be OK, > > even if little bit hacky. > > I wasn't suggesting that we turn the postcore_initcall() into > module_init(). postcore_initcall() works just fine for modules (it's > automatically replaced by a module_init() if the code is built as a > module) and it will still do the right thing with regard to ordering > when built-in. Right, I already tested the code, just building as a module without any other changes. And it worked. About the rest, what Thierry suggested: > Two things are missing, though: 1) the drm_mipi_dsi.ko module would need > to be reference counted so that the symbols stay around as long as there > are any drivers (this might be covered by symbol dependencies already) Yeah, the symbol dependency should cover it enough. > and 2) there would need to be an exit function for the module to cleanup > the bus. So only this one is missing. I'm going to cook this as well and submit patches later. thanks, Takashi
[PATCH 1/2] drm/mipi-dsi: Unregister bus at exit
This is a preliminary patch for building drm-mipi-dsi as a module.
Add the module exit callback to unregister the bus properly.
Suggested-by: Thierry Reding
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/drm_mipi_dsi.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index f5d80839a90c..e2f3441663be 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -1069,6 +1069,12 @@ static int __init mipi_dsi_bus_init(void)
}
postcore_initcall(mipi_dsi_bus_init);
+static void __exit mipi_dsi_bus_exit(void)
+{
+ bus_unregister(&mipi_dsi_bus_type);
+}
+module_exit(mipi_dsi_bus_exit);
+
MODULE_AUTHOR("Andrzej Hajda ");
MODULE_DESCRIPTION("MIPI DSI Bus");
MODULE_LICENSE("GPL and additional rights");
--
2.9.2
[PATCH 2/2] drm/mipi-dsi: Allow to build drm-mipi-dsi as a module
The drm-mipi-dsi driver has been only built-in although this isn't strictly required to be so. Since it's referred by lots of DRM drivers nowadays, most of distro kernels include the driver as built-in as a result, even though many systems don't need it at all. This patch fixes Kconfig to allow drm-mipi-dsi driver built as a module, so that we can save footprint on systems without such DRM drivers. The probe order is managed by the module dependency, and postcore_initcall() works just fine as a module init call. Signed-off-by: Takashi Iwai --- drivers/gpu/drm/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index fc357319de35..67668a04baf6 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -22,7 +22,7 @@ menuconfig DRM (/dev/agpgart) support if it is available for your platform. config DRM_MIPI_DSI - bool + tristate depends on DRM config DRM_DP_AUX_CHARDEV -- 2.9.2
Re: [PATCH v10 02/40] i915/snd_hdac: I915 subcomponent for the snd_hdac
On Mon, 04 Feb 2019 16:00:18 +0100, Daniel Vetter wrote: > > On Thu, Jan 31, 2019 at 12:29:18PM +0530, Ramalingam C wrote: > > From: Daniel Vetter > > > > Since we need multiple components for I915 for different purposes > > (Audio & Mei_hdcp), we adopt the subcomponents methodology introduced > > by the previous patch (mentioned below). > > > > Author: Daniel Vetter > > Date: Mon Jan 28 17:08:20 2019 +0530 > > > > components: multiple components for a device > > > > Signed-off-by: Daniel Vetter > > Signed-off-by: Ramalingam C > > cc: Greg Kroah-Hartman > > cc: Russell King > > cc: Rafael J. Wysocki > > cc: Jaroslav Kysela > > cc: Takashi Iwai > > cc: Rodrigo Vivi > > cc: Jani Nikula > > Takashi, can you pls take a look and ack for merging through drm-intel? We > can also do a topic branch in case this conflicts with something on the > audio side. I'm fine with the change as long as others agree with this API extension. Reviewed-by: Takashi Iwai thanks, Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PULL] topic/component-typed
On Mon, 11 Feb 2019 19:25:12 +0100, Sam Ravnborg wrote: > > Hi Daniel. > > On Mon, Feb 11, 2019 at 06:15:20PM +0100, Daniel Vetter wrote: > > Hi all, > > > > Here's the typed component topic branch. > > > > drm-intel maintainers: Please pull, I need this for the mei hdcp work from > > Ram. > > > > drm-misc maintainers: Please pull, there's a drm doc patch follow-up > > that I want to stuff into drm-misc-next. > > > > Greg: The drm side missed our feature cutoff, so will only land in 5.2. > > With all the dependencies why not bend the rule a little and get this in now? > It is not like this is a huge patchset. Yeah, that's likely the most straightforward way. BTW, I tried to pull onto sound git tree for-next branch, and it worked without conflicts. So I don't think it needs to be pulled onto mine. thanks, Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/7] drm/edid: Allow to ignore the audio EDID data
On Tue, 05 Mar 2019 20:36:38 +0100, Ville Syrjälä wrote: > > On Tue, Mar 05, 2019 at 02:21:04PM -0500, Alex Deucher wrote: > > On Tue, Mar 5, 2019 at 2:15 PM Ville Syrjälä > > wrote: > > > > > > On Tue, Mar 05, 2019 at 05:24:13PM +0200, Ville Syrjälä wrote: > > > > On Tue, Mar 05, 2019 at 10:12:40AM +0100, Maxime Ripard wrote: > > > > > On Mon, Mar 04, 2019 at 03:05:31PM -0500, Alex Deucher wrote: > > > > > > On Mon, Mar 4, 2019 at 2:53 PM Eric Anholt wrote: > > > > > > > > > > > > > > Maxime Ripard writes: > > > > > > > > > > > > > > > In some cases, in order to accomodate with displays with poor > > > > > > > > EDIDs, we > > > > > > > > need to ignore that the monitor alledgedly supports audio > > > > > > > > output and > > > > > > > > disable the audio output. > > > > > > > > > > > > > > > > Signed-off-by: Maxime Ripard > > > > > > > > --- > > > > > > > > drivers/gpu/drm/drm_edid.c | 8 > > > > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c > > > > > > > > b/drivers/gpu/drm/drm_edid.c > > > > > > > > index 990b1909f9d7..c0258b011bb2 100644 > > > > > > > > --- a/drivers/gpu/drm/drm_edid.c > > > > > > > > +++ b/drivers/gpu/drm/drm_edid.c > > > > > > > > @@ -4190,6 +4190,11 @@ bool drm_detect_hdmi_monitor(struct edid > > > > > > > > *edid) > > > > > > > > } > > > > > > > > EXPORT_SYMBOL(drm_detect_hdmi_monitor); > > > > > > > > > > > > > > > > +static bool ignore_edid_audio = false; > > > > > > > > +module_param(ignore_edid_audio, bool, 0644); > > > > > > > > +MODULE_PARM_DESC(ignore_edid_audio, > > > > > > > > + "Ignore the EDID and always consider that a > > > > > > > > monitor doesn't have audio capabilities"); > > > > > > > > + > > > > > > > > /** > > > > > > > > * drm_detect_monitor_audio - check monitor audio capability > > > > > > > > * @edid: EDID block to scan > > > > > > > > @@ -4209,6 +4214,9 @@ bool drm_detect_monitor_audio(struct edid > > > > > > > > *edid) > > > > > > > > bool has_audio = false; > > > > > > > > int start_offset, end_offset; > > > > > > > > > > > > > > > > + if (ignore_edid_audio) > > > > > > > > + goto end; > > > > > > > > + > > > > > > > > edid_ext = drm_find_cea_extension(edid); > > > > > > > > if (!edid_ext) > > > > > > > > goto end; > > > > > > > > > > > > > > It looks like the motivation for the original flag on Raspberry > > > > > > > Pi was > > > > > > > "I've got a non-audio monitor, but the system comes up trying to > > > > > > > play > > > > > > > audio to HDMI instead of the analog jack". Do we have some way > > > > > > > for DRM > > > > > > > to communicate to ALSA that this is not the right place to try to > > > > > > > play > > > > > > > audio by default? > > > > > > > > > > > > Apparently not. We have users using debug knobs in our drivers to > > > > > > disable display audio because ALSA defaults to that rather than > > > > > > other > > > > > > audio. > > > > > > > > > > I guess one way to do this would be to register the card only when an > > > > > audio-capable monitor is connected instead of doing this at probe > > > > > time. I'm not sure how convenient it is for userspace though. > > > > > > > > We already provide the ELD to alsa. I'm pretty sure pulseaudio uses > > > > that stuff somehow to figure out whether to play audio over HDMI. > > > > But since I don't use pulseaudio myself I can't be 100% sure. > > > > > > > > Cc:ing Takashi/alsa folks for confirmation. > > > > > > I forgot that the .pin_eld_notify() stuff is i915 specific. But > > > I see some kind of hdmi_codec_ops thing used by some other drivers. > > > I guess that is supposed to achieve the same thing more or less? > > > I'm not immediately seeing any kind of drm->alsa notification > > > hook in there though. > > > > On AMD hw, the GPU has backdoor access to some of the audio state, so > > when stuff happens on the GPU side, it's reflected on the audio side > > automatically. > > Right. i915 has a similar thing (my theory is that it's basically > an industry wide hardware workaround for inflexible Windows driver > architecture). But that was problematic for some power management > related reasons (IIRC) so we added a software mechanism for it. > Though I believe we still write the ELD into the hardware buffer > as well. I'm late to the game as I've been off in the last week, and here is just the confirmation. The direct communication from drm to ALSA via component has been implemented currently only for i915. I had some patches to enable the feature for radeon and amdgpu, but the enablement on amdgpu DC is still missing, and the work is pending for now. For other drivers, we'd need more or less similar mechanism. We might want to choose a better infrastructure than the component binding, but it's a thing to be discussed. thanks, Takashi ___ dri-devel mailing list dri-devel@l
Re: [alsa-devel] Linking ALSA playback devices and DRM connectors
On Tue, 04 Jun 2019 18:25:53 +0200, Ville Syrjälä wrote: > > On Tue, Jun 04, 2019 at 05:24:35PM +0200, Daniel Vetter wrote: > > On Tue, Jun 4, 2019 at 5:15 PM Christian König > > wrote: > > > > > > Am 04.06.19 um 17:05 schrieb Ser, Simon: > > > > Hi, > > > > > > > > I'm trying to link ALSA playback devices and DRM connectors. In other > > > > words, I'd like to be able to know which ALSA device I should open to > > > > play audio on a given connector. > > > > > > > > Unfortunately, I haven't found a way to extract this information. I > > > > know /proc/asound/cardN/eld* expose the EDID-like data. However by > > > > looking at the ALSA API (also: aplay -l and -L) I can't find a way to > > > > figure out which PCM device maps to the ELD. > > > > > > > > Am I missing something? > > > > > > Is that actually fixed on all hardware? Or do we maybe have some > > > hardware with only one audio codec and multiple connectors? > > Certain old i915 hardware is like that. You can drive > multiple HDMI connectors at once but only one of them > can get the audio. If you try to output audio to multiple > ports at once you get no audio whatsoever. ATM we don't > really handle that case properly. > > > > > > > > > > > > If not, what would be the best way to expose this? > > > > > > > > - A symlink to the ALSA audio PCM device in > > > >/sys/class/drm/cardN-CONNECTOR? > > > > - A symlink to the DRM connector in /sys/class/sound/pcm*? > > > > > > If it's fixed than those two options sound sane to me. > > > > > > > - A DRM connector property? > > > > > > If it's configurable than that sounds like a good option to me. > > > > > > Anyway added our DC team, stuff like that is their construction site. > > > > > > Regards, > > > Christian. > > > > > > > - Somehow expose the connector name via the ALSA API? > > > > - Expose the connector EDID via ALSA? > > > > - Other ideas? > > > > I think on our MST hw you get a combination of CRTCs x CONNECTORs on > > the alsa side. I.e. for every pair of connector and crtc you get a > > separate alsa pin. This is because with mst, you could have up to > > num_crtcs streams on a single connector. Not sure how to model that. > > IIRC the current i915 vs. alsa model is that for SST/HDMI you have > a 1:1 relationship between the port and the pcm device, but with MST > you nave a 1:1 relationship between the pipe and the pcm device. > > I think the only way to have any kind of static connector<->pcm > relationship with MST would involve dynamically adding/removing > pcm devices when the correcponding drm connector is added/removed. > If we don't want to/can't add such pcm devices then we'd need to > dynamically change the symlinks/whatever whenever an MST stream > is started/stopped. And probably we should do the same for SST/HDMI > as well, if for no other reason than to make sure userspace is > prepared for it even if they didn't test with MST. The idea with the dynamic PCM device creation pops up occasionally, but it never flies. The biggest reason so far is the support of PulseAudio and others; although PA can deal with the hotplug *card* device, the devices below the card level aren't managed dynamically, hence the dynamic creation of PCM device doesn't work for them. From API POV, the cleanest way would be to provide some information over ALSA control element for each PCM stream that indicates the corresponding DRM connector. But other methods shouldn't be too hard to implement -- once when we have a certain way to obtain the DRM connector / PCM relationship :) thanks, Takashi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/radeon: Add HD-audio component notifier support
This patch adds the support for the notification of HD-audio hotplug
via the already existing drm_audio_component framework to radeon
driver. This allows us more reliable hotplug notification and ELD
transfer without accessing HD-audio bus; it's more efficient, and more
importantly, it works without waking up the runtime PM.
The implementation is rather simplistic: radeon driver provides the
get_eld ops for HD-audio, and it notifies the audio hotplug via
pin_eld_notify callback upon each radeon_audio_enable() call.
The pin->id is referred as the port number passed to the notifier
callback, and the corresponding connector is looked through the
encoder list in the get_eld callback in turn.
The bind and unbind callbacks handle the device-link so that it
assures the PM call order.
Acked-by: Jim Qu
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/Kconfig | 1 +
drivers/gpu/drm/radeon/radeon.h | 3 ++
drivers/gpu/drm/radeon/radeon_audio.c | 92 +++
3 files changed, 96 insertions(+)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 1d80222587ad..8b44cc458830 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -209,6 +209,7 @@ config DRM_RADEON
select HWMON
select BACKLIGHT_CLASS_DEVICE
select INTERVAL_TREE
+ select SND_HDA_COMPONENT if SND_HDA_CORE
help
Choose this option if you have an ATI Radeon graphics card. There
are both PCI and AGP versions. You don't need to choose this to
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 32808e50be12..2973284b7961 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -75,6 +75,7 @@
#include
#include
+#include
#include "radeon_family.h"
#include "radeon_mode.h"
@@ -1761,6 +1762,8 @@ struct r600_audio {
struct radeon_audio_funcs *hdmi_funcs;
struct radeon_audio_funcs *dp_funcs;
struct radeon_audio_basic_funcs *funcs;
+ struct drm_audio_component *component;
+ bool component_registered;
};
/*
diff --git a/drivers/gpu/drm/radeon/radeon_audio.c
b/drivers/gpu/drm/radeon/radeon_audio.c
index b9aea5776d3d..976502e31c43 100644
--- a/drivers/gpu/drm/radeon/radeon_audio.c
+++ b/drivers/gpu/drm/radeon/radeon_audio.c
@@ -23,6 +23,7 @@
*/
#include
+#include
#include
#include "radeon.h"
@@ -242,6 +243,14 @@ static struct radeon_audio_funcs dce6_dp_funcs = {
.dpms = evergreen_dp_enable,
};
+static void radeon_audio_component_notify(struct drm_audio_component *acomp,
+ int port)
+{
+ if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
+ acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr,
+port, -1);
+}
+
static void radeon_audio_enable(struct radeon_device *rdev,
struct r600_audio_pin *pin, u8 enable_mask)
{
@@ -269,6 +278,8 @@ static void radeon_audio_enable(struct radeon_device *rdev,
if (rdev->audio.funcs->enable)
rdev->audio.funcs->enable(rdev, pin, enable_mask);
+
+ radeon_audio_component_notify(rdev->audio.component, pin->id);
}
static void radeon_audio_interface_init(struct radeon_device *rdev)
@@ -292,6 +303,79 @@ static void radeon_audio_interface_init(struct
radeon_device *rdev)
}
}
+static int radeon_audio_component_get_eld(struct device *kdev, int port,
+ int pipe, bool *enabled,
+ unsigned char *buf, int max_bytes)
+{
+ struct drm_device *dev = dev_get_drvdata(kdev);
+ struct drm_encoder *encoder;
+ struct radeon_encoder *radeon_encoder;
+ struct radeon_encoder_atom_dig *dig;
+ struct drm_connector *connector;
+ int ret = 0;
+
+ *enabled = false;
+ list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) {
+ if (!radeon_encoder_is_digital(encoder))
+ continue;
+ radeon_encoder = to_radeon_encoder(encoder);
+ dig = radeon_encoder->enc_priv;
+ if (!dig->pin || dig->pin->id != port)
+ continue;
+ connector = radeon_get_connector_for_encoder(encoder);
+ if (connector) {
+ *enabled = true;
+ ret = drm_eld_size(connector->eld);
+ memcpy(buf, connector->eld, min(max_bytes, ret));
+ break;
+ }
+ }
+
+ return ret;
+}
+
+static const struct drm_audio_component_ops radeon_audio_component_ops = {
+ .get_eld = radeon_audio_component_get_eld,
+};
+
+static int radeon_audio_compon
[PATCH 2/2] drm/nouveau: Add HD-audio component notifier support
This patch adds the support for the notification of HD-audio hotplug
via the already existing drm_audio_component framework. This allows
us more reliable hotplug notification and ELD transfer without
accessing HD-audio bus; it's more efficient, and more importantly, it
works without waking up the runtime PM.
The implementation is rather simplistic: nouveau driver provides the
get_eld ops for HD-audio, and it notifies the audio hotplug via
pin_eld_notify callback upon each nv50_audio_enable() and _disable()
call. As the HD-audio pin assignment seems corresponding to the CRTC,
the crtc->index number is passed directly as the zero-based port
number.
The bind and unbind callbacks handle the device-link so that it
assures the PM call order.
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/nouveau/Kconfig | 1 +
drivers/gpu/drm/nouveau/dispnv50/disp.c | 111
drivers/gpu/drm/nouveau/nouveau_drv.h | 7 ++
3 files changed, 119 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index 96b9814e6d06..33ccf11bd70d 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -16,6 +16,7 @@ config DRM_NOUVEAU
select INPUT if ACPI && X86
select THERMAL if ACPI && X86
select ACPI_VIDEO if ACPI && X86
+ select SND_HDA_COMPONENT if SND_HDA_CORE
help
Choose this option for open-source NVIDIA support.
diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c
b/drivers/gpu/drm/nouveau/dispnv50/disp.c
index 8497768f1b41..919f3d3db161 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
@@ -29,6 +29,7 @@
#include
#include
+#include
#include
#include
@@ -466,12 +467,113 @@ nv50_dac_create(struct drm_connector *connector, struct
dcb_output *dcbe)
return 0;
}
+/*
+ * audio component binding for ELD notification
+ */
+static void
+nv50_audio_component_eld_notify(struct drm_audio_component *acomp, int port)
+{
+ if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
+ acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr,
+port, -1);
+}
+
+static int
+nv50_audio_component_get_eld(struct device *kdev, int port, int pipe,
+bool *enabled, unsigned char *buf, int max_bytes)
+{
+ struct drm_device *drm_dev = dev_get_drvdata(kdev);
+ struct nouveau_drm *drm = nouveau_drm(drm_dev);
+ struct drm_encoder *encoder;
+ struct nouveau_encoder *nv_encoder;
+ struct nouveau_connector *nv_connector;
+ struct nouveau_crtc *nv_crtc;
+ int ret = 0;
+
+ *enabled = false;
+ drm_for_each_encoder(encoder, drm->dev) {
+ nv_encoder = nouveau_encoder(encoder);
+ nv_connector = nouveau_encoder_connector_get(nv_encoder);
+ nv_crtc = nouveau_crtc(encoder->crtc);
+ if (!nv_connector || !nv_crtc || nv_crtc->index != port)
+ continue;
+ *enabled = drm_detect_monitor_audio(nv_connector->edid);
+ if (*enabled) {
+ ret = drm_eld_size(nv_connector->base.eld);
+ memcpy(buf, nv_connector->base.eld,
+ min(max_bytes, ret));
+ }
+ break;
+ }
+ return ret;
+}
+
+static const struct drm_audio_component_ops nv50_audio_component_ops = {
+ .get_eld = nv50_audio_component_get_eld,
+};
+
+static int
+nv50_audio_component_bind(struct device *kdev, struct device *hda_kdev,
+ void *data)
+{
+ struct drm_device *drm_dev = dev_get_drvdata(kdev);
+ struct nouveau_drm *drm = nouveau_drm(drm_dev);
+ struct drm_audio_component *acomp = data;
+
+ if (WARN_ON(!device_link_add(hda_kdev, kdev, DL_FLAG_STATELESS)))
+ return -ENOMEM;
+
+ drm_modeset_lock_all(drm_dev);
+ acomp->ops = &nv50_audio_component_ops;
+ acomp->dev = kdev;
+ drm->audio.component = acomp;
+ drm_modeset_unlock_all(drm_dev);
+ return 0;
+}
+
+static void
+nv50_audio_component_unbind(struct device *kdev, struct device *hda_kdev,
+ void *data)
+{
+ struct drm_device *drm_dev = dev_get_drvdata(kdev);
+ struct nouveau_drm *drm = nouveau_drm(drm_dev);
+ struct drm_audio_component *acomp = data;
+
+ drm_modeset_lock_all(drm_dev);
+ drm->audio.component = NULL;
+ acomp->ops = NULL;
+ acomp->dev = NULL;
+ drm_modeset_unlock_all(drm_dev);
+}
+
+static const struct component_ops nv50_audio_component_bind_ops = {
+ .bind = nv50_audio_component_bind,
+ .unbind = nv50_audio_component_unbind,
+};
+
+static void
+nv50_audio_compon
[PATCH 0/2] Audio component support for radeon and nouveau
Hi, this is a patchset to add the support for HDMI/DP audio notification via drm_audio_component framework on radeon and nouveau drivers. The notification mechanism works for long time on i915, and now AMDGPU patch is floating around. This fills the rest gap, the support for remaining radeon and nouveau drivers. The implementation is rather simple, and it just adds the hook for notification and ELD retrieval. The patch for radeon is basically a respin of my previous RFC patch. The counterpart change in HD-audio side has been already merged in sound.git tree for 5.4 kernel. The persistent branch, topic/hda-acomp-base, is provided there, so that you can pull into yours freely: git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git topic/hda-acomp-base This patchset itself is almost harmless without the HD-audio side changes (IOW, it doesn't help anything else, either :) thanks, Takashi === Takashi Iwai (2): drm/radeon: Add HD-audio component notifier support drm/nouveau: Add HD-audio component notifier support drivers/gpu/drm/Kconfig | 1 + drivers/gpu/drm/nouveau/Kconfig | 1 + drivers/gpu/drm/nouveau/dispnv50/disp.c | 111 drivers/gpu/drm/nouveau/nouveau_drv.h | 7 ++ drivers/gpu/drm/radeon/radeon.h | 3 + drivers/gpu/drm/radeon/radeon_audio.c | 92 ++ 6 files changed, 215 insertions(+) -- 2.16.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support
On Mon, 22 Jul 2019 16:44:06 +0200,
Koenig, Christian wrote:
>
> Am 22.07.19 um 16:38 schrieb Takashi Iwai:
> > This patch adds the support for the notification of HD-audio hotplug
> > via the already existing drm_audio_component framework to radeon
> > driver. This allows us more reliable hotplug notification and ELD
> > transfer without accessing HD-audio bus; it's more efficient, and more
> > importantly, it works without waking up the runtime PM.
> >
> > The implementation is rather simplistic: radeon driver provides the
> > get_eld ops for HD-audio, and it notifies the audio hotplug via
> > pin_eld_notify callback upon each radeon_audio_enable() call.
> > The pin->id is referred as the port number passed to the notifier
> > callback, and the corresponding connector is looked through the
> > encoder list in the get_eld callback in turn.
>
> On most older Radeon parts you only got one audio codec which can be
> routed to multiple connectors.
>
> As far as I can see this is not correctly supported with this framework
> here since we only grab the first ELD. Is that correct?
Hrm, how does it work currently (without the patch) in the hardware
level? I mean, the unsolicited event is still triggered via HD-audio
bus as well as the partial ELD pass-through via HD-audio. Isn't the
reported pin ID corresponding to that connector?
> Additional to that I'm not sure if that is the right design for AMD
> hardware in general since we don't really support ELD in the first place.
ELD is a kind of mandatory and we've assembled the ELD from the
partial information taken via HD-audio bus like speaker allocation and
other bits, so far. I thought the very same information is found in
connector->edid that is always parsed at EDID parsing time.
Let me know if I'm obviously overlooking something.
Thanks!
Takashi
>
> Regards,
> Christian.
>
> >
> > The bind and unbind callbacks handle the device-link so that it
> > assures the PM call order.
> >
> > Acked-by: Jim Qu
> > Signed-off-by: Takashi Iwai
> > ---
> > drivers/gpu/drm/Kconfig | 1 +
> > drivers/gpu/drm/radeon/radeon.h | 3 ++
> > drivers/gpu/drm/radeon/radeon_audio.c | 92
> > +++
> > 3 files changed, 96 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 1d80222587ad..8b44cc458830 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -209,6 +209,7 @@ config DRM_RADEON
> > select HWMON
> > select BACKLIGHT_CLASS_DEVICE
> > select INTERVAL_TREE
> > + select SND_HDA_COMPONENT if SND_HDA_CORE
> > help
> > Choose this option if you have an ATI Radeon graphics card. There
> > are both PCI and AGP versions. You don't need to choose this to
> > diff --git a/drivers/gpu/drm/radeon/radeon.h
> > b/drivers/gpu/drm/radeon/radeon.h
> > index 32808e50be12..2973284b7961 100644
> > --- a/drivers/gpu/drm/radeon/radeon.h
> > +++ b/drivers/gpu/drm/radeon/radeon.h
> > @@ -75,6 +75,7 @@
> > #include
> >
> > #include
> > +#include
> >
> > #include "radeon_family.h"
> > #include "radeon_mode.h"
> > @@ -1761,6 +1762,8 @@ struct r600_audio {
> > struct radeon_audio_funcs *hdmi_funcs;
> > struct radeon_audio_funcs *dp_funcs;
> > struct radeon_audio_basic_funcs *funcs;
> > + struct drm_audio_component *component;
> > + bool component_registered;
> > };
> >
> > /*
> > diff --git a/drivers/gpu/drm/radeon/radeon_audio.c
> > b/drivers/gpu/drm/radeon/radeon_audio.c
> > index b9aea5776d3d..976502e31c43 100644
> > --- a/drivers/gpu/drm/radeon/radeon_audio.c
> > +++ b/drivers/gpu/drm/radeon/radeon_audio.c
> > @@ -23,6 +23,7 @@
> >*/
> >
> > #include
> > +#include
> >
> > #include
> > #include "radeon.h"
> > @@ -242,6 +243,14 @@ static struct radeon_audio_funcs dce6_dp_funcs = {
> > .dpms = evergreen_dp_enable,
> > };
> >
> > +static void radeon_audio_component_notify(struct drm_audio_component
> > *acomp,
> > + int port)
> > +{
> > + if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
> > + acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr,
> > +port, -1);
> > +}
> >
Re: [PATCH 2/2] drm/nouveau: Add HD-audio component notifier support
On Mon, 22 Jul 2019 17:00:15 +0200,
Ilia Mirkin wrote:
>
> On Mon, Jul 22, 2019 at 10:38 AM Takashi Iwai wrote:
> > +static void
> > +nv50_audio_component_init(struct nouveau_drm *drm)
> > +{
> > + if (!component_add(drm->dev->dev, &nv50_audio_component_bind_ops))
> > + drm->audio.component_registered = true;
> > +}
>
> Pardon my ignorance on this topic ... but are there any ill effects
> from always doing this? For example, the board in question might not
> have ELD support at all (the original G80, covered by dispnv50), or it
> might not have any HDMI/DP ports, or it may have them but just nothing
> is plugged in.
In general, this change shouldn't do anything worse than what we have
currently.
If the board has no ELD support, the HDMI audio won't work at all, and
HD-audio side would know that it's an invalid ELD.
If the nvidia chips has no audio support and it doesn't have the
corresponding HD-audio audio driver exposed as a PCI device, this
component registration is also harmless, just a placeholder until it's
bound.
> Could you confirm that this is all OK?
At least I tested other way round -- the nvidia component implemented
but without HD-audio master component.
Thanks!
Takashi
>
> Thanks,
>
> -ilia
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support
On Mon, 22 Jul 2019 17:07:09 +0200, Koenig, Christian wrote: > > Am 22.07.19 um 16:53 schrieb Takashi Iwai: > > On Mon, 22 Jul 2019 16:44:06 +0200, > > Koenig, Christian wrote: > >> Am 22.07.19 um 16:38 schrieb Takashi Iwai: > >>> This patch adds the support for the notification of HD-audio hotplug > >>> via the already existing drm_audio_component framework to radeon > >>> driver. This allows us more reliable hotplug notification and ELD > >>> transfer without accessing HD-audio bus; it's more efficient, and more > >>> importantly, it works without waking up the runtime PM. > >>> > >>> The implementation is rather simplistic: radeon driver provides the > >>> get_eld ops for HD-audio, and it notifies the audio hotplug via > >>> pin_eld_notify callback upon each radeon_audio_enable() call. > >>> The pin->id is referred as the port number passed to the notifier > >>> callback, and the corresponding connector is looked through the > >>> encoder list in the get_eld callback in turn. > >> On most older Radeon parts you only got one audio codec which can be > >> routed to multiple connectors. > >> > >> As far as I can see this is not correctly supported with this framework > >> here since we only grab the first ELD. Is that correct? > > Hrm, how does it work currently (without the patch) in the hardware > > level? I mean, the unsolicited event is still triggered via HD-audio > > bus as well as the partial ELD pass-through via HD-audio. Isn't the > > reported pin ID corresponding to that connector? > > The hardware we are talking about here doesn't have ELD support at all. > > So no ELD pass-through or unsolicited event when something is connected. > > You basically have to setup the capabilities in the applications manually. Well, if ELD isn't passed, the HDMI audio doesn't work at all -- even for the driver as of today. So we're talking about a stuff that is already broken now, if any. > >> Additional to that I'm not sure if that is the right design for AMD > >> hardware in general since we don't really support ELD in the first place. > > ELD is a kind of mandatory and we've assembled the ELD from the > > partial information taken via HD-audio bus like speaker allocation and > > other bits, so far. I thought the very same information is found in > > connector->edid that is always parsed at EDID parsing time. > > > > Let me know if I'm obviously overlooking something. > > The hardware we are talking about existed way before the ELD standard. > So nothing from the ELD standard is supported here. > > I mean it would be great to get this cleaned up, but you need to take > care all those nasty corner cases not supported by ELD. That's not clear to me: the ELD information bits are filled in drm_edid_to_eld() generically for the given EDID. Do you mean that this isn't applied to the old radeon chip? IOW, doesn't such a chip read EDID at all...? I know that the old radeon chip doesn't pass the ELD information via HD-audio bus unlike others. For such chips, we've assembled the ELD from the partial information, as I already mentioned. > Like one audio codec routed to multiple physical connectors. In theory > you even have the ability to route the left and right channel to > different connectors. That's a question how currently it's exposed in the hardware level at all to HD-audio side. Again, my patchset tries to simplify this communication -- between gfx and HD-audio codec chip. If the translation of connector / pin can't be done in the way I implemented, I'd like to hear how the hardware actually decides the HD-audio pin index from the given configuration. > I think nouveau has an even nastier case where you have an audio codec > which can be attached both to a normal audio plug as well as to a > display device to route stuff over HDMI. The nouveau looks even more straightforward :) > You won't find any of that on modern hardware, so I'm not sure if it's > worth putting to much effort into it :) Heh, I'm fine to drop the radeon part, as it's becoming dead nowadays, if you think it being too risky. For AMDGPU, you guys seem working on it, and I'd just wait for the final patches. The nouveau is still in active deployment and development, so I'd like to have it done there, though. thanks, Takashi > > Regards, > Christian. > > > > > > > Thanks! > > > > Takashi > > > > > >> Regards, > >> Christian. > >> > >&
Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support
On Mon, 22 Jul 2019 17:35:09 +0200, Alex Deucher wrote: > > On Mon, Jul 22, 2019 at 11:21 AM Takashi Iwai wrote: > > > > On Mon, 22 Jul 2019 17:07:09 +0200, > > Koenig, Christian wrote: > > > > > > Am 22.07.19 um 16:53 schrieb Takashi Iwai: > > > > On Mon, 22 Jul 2019 16:44:06 +0200, > > > > Koenig, Christian wrote: > > > >> Am 22.07.19 um 16:38 schrieb Takashi Iwai: > > > >>> This patch adds the support for the notification of HD-audio hotplug > > > >>> via the already existing drm_audio_component framework to radeon > > > >>> driver. This allows us more reliable hotplug notification and ELD > > > >>> transfer without accessing HD-audio bus; it's more efficient, and more > > > >>> importantly, it works without waking up the runtime PM. > > > >>> > > > >>> The implementation is rather simplistic: radeon driver provides the > > > >>> get_eld ops for HD-audio, and it notifies the audio hotplug via > > > >>> pin_eld_notify callback upon each radeon_audio_enable() call. > > > >>> The pin->id is referred as the port number passed to the notifier > > > >>> callback, and the corresponding connector is looked through the > > > >>> encoder list in the get_eld callback in turn. > > > >> On most older Radeon parts you only got one audio codec which can be > > > >> routed to multiple connectors. > > > >> > > > >> As far as I can see this is not correctly supported with this framework > > > >> here since we only grab the first ELD. Is that correct? > > > > Hrm, how does it work currently (without the patch) in the hardware > > > > level? I mean, the unsolicited event is still triggered via HD-audio > > > > bus as well as the partial ELD pass-through via HD-audio. Isn't the > > > > reported pin ID corresponding to that connector? > > > > > > The hardware we are talking about here doesn't have ELD support at all. > > > > > > So no ELD pass-through or unsolicited event when something is connected. > > > > > > You basically have to setup the capabilities in the applications manually. > > > > Well, if ELD isn't passed, the HDMI audio doesn't work at all -- even > > for the driver as of today. So we're talking about a stuff that is > > already broken now, if any. > > > > > >> Additional to that I'm not sure if that is the right design for AMD > > > >> hardware in general since we don't really support ELD in the first > > > >> place. > > > > ELD is a kind of mandatory and we've assembled the ELD from the > > > > partial information taken via HD-audio bus like speaker allocation and > > > > other bits, so far. I thought the very same information is found in > > > > connector->edid that is always parsed at EDID parsing time. > > > > > > > > Let me know if I'm obviously overlooking something. > > > > > > The hardware we are talking about existed way before the ELD standard. > > > So nothing from the ELD standard is supported here. > > > > > > I mean it would be great to get this cleaned up, but you need to take > > > care all those nasty corner cases not supported by ELD. > > > > That's not clear to me: the ELD information bits are filled in > > drm_edid_to_eld() generically for the given EDID. Do you mean that > > this isn't applied to the old radeon chip? IOW, doesn't such a chip > > read EDID at all...? > > No AMD audio hw uses ELD. The original hw design for display auto > pre-dated ELD. the display driver updates the the hw state and that > state is updated in the audio hw as well. So there is an internal hw > interface between the two drivers. The display driver updates via GPU > MMIO, and the audio driver can interact with that state via AMD > specific VERBs. See this document: > http://developer.amd.com/wordpress/media/2013/10/AMD_HDA_verbs_v2.pdf Yes, that's what I mentioned as "assembling ELD in HD-audio side". The HD-audio driver has an AMD-specific code for building up the ELD from these partial bits. But my question is whether these AMD chips do read EDID and process via the standard EDID helper (drm_add_edid_modes(), etc). If yes, the ELD is filled up there, and we can use the same information from there as done in my patch. > On older AMD chips (everything prior to SI chips IIRC), had a sin
Re: [PATCH 1/2] drm/radeon: Add HD-audio component notifier support
On Mon, 22 Jul 2019 17:38:36 +0200, Koenig, Christian wrote: > > Am 22.07.19 um 17:21 schrieb Takashi Iwai: > > On Mon, 22 Jul 2019 17:07:09 +0200, > > Koenig, Christian wrote: > >> Am 22.07.19 um 16:53 schrieb Takashi Iwai: > >>> On Mon, 22 Jul 2019 16:44:06 +0200, > >>> Koenig, Christian wrote: > >>>> Am 22.07.19 um 16:38 schrieb Takashi Iwai: > >>>> [SNIP] > >> The hardware we are talking about here doesn't have ELD support at all. > >> > >> So no ELD pass-through or unsolicited event when something is connected. > >> > >> You basically have to setup the capabilities in the applications manually. > > Well, if ELD isn't passed, the HDMI audio doesn't work at all -- even > > for the driver as of today. So we're talking about a stuff that is > > already broken now, if any. > > At least the last time I looked that worked perfectly fine. > > ELD is only optional since it was introduced way later than the first > HDMI audio capable devices. > > If ELD became mandatory at some point then that could have potentially > broken the hardware support and that is a really no-go. Hrm, are you sure? The ELD bits are assembled in HD-audio side for AMD chips, and ELD has bee always a mandatory requirement. I don't mean about the direct ELD passing but the ELD information exposed from the audio driver. > >>>> Additional to that I'm not sure if that is the right design for AMD > >>>> hardware in general since we don't really support ELD in the first place. > >>> ELD is a kind of mandatory and we've assembled the ELD from the > >>> partial information taken via HD-audio bus like speaker allocation and > >>> other bits, so far. I thought the very same information is found in > >>> connector->edid that is always parsed at EDID parsing time. > >>> > >>> Let me know if I'm obviously overlooking something. > >> The hardware we are talking about existed way before the ELD standard. > >> So nothing from the ELD standard is supported here. > >> > >> I mean it would be great to get this cleaned up, but you need to take > >> care all those nasty corner cases not supported by ELD. > > That's not clear to me: the ELD information bits are filled in > > drm_edid_to_eld() generically for the given EDID. Do you mean that > > this isn't applied to the old radeon chip? IOW, doesn't such a chip > > read EDID at all...? > > EDID is read to get the video information for the given connector, but > as far as I know radeon never called drm_edid_to_eld() and doesn't > provide the ELD information in any way to the audio codec. > > > I know that the old radeon chip doesn't pass the ELD information via > > HD-audio bus unlike others. For such chips, we've assembled the ELD > > from the partial information, as I already mentioned. > > What partial information do you mean here? The speaker allocation, audio descriptor, audio video delay, sink info index, etc, that are obtained via the vendor-specific HD-audio codec verbs. > It's about a decade ago that I wrote that code, but IIRC radeon never > passed anything to the audio codec. As far as I read correctly, the whole radeon_audio_write_xxx() calls update the corresponding registers, and these are taken from connector's EDID bits. HD-audio driver reads these values via the shadowed HD-audio verbs over HD-audio bus in turn, and builds up the ELD by itself. With the eld_notification callback, we can skip this decompose / compose step. > >> Like one audio codec routed to multiple physical connectors. In theory > >> you even have the ability to route the left and right channel to > >> different connectors. > > That's a question how currently it's exposed in the hardware level at > > all to HD-audio side. Again, my patchset tries to simplify this > > communication -- between gfx and HD-audio codec chip. If the > > translation of connector / pin can't be done in the way I implemented, > > I'd like to hear how the hardware actually decides the HD-audio pin > > index from the given configuration. > > Well that's why I tried to explain. The hardware doesn't care about the > HD-audio pin in any way as far as I know. So essentially it implies that such chips do have only a single HD-audio pin. In that case, it's easy to treat in HD-audio side -- just ignore the pin index. > >> You won't find any of that on modern hardware, so I'm not sure if it's > >> worth putting to much effo
[PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect
At both suspend and disconnect, we should rather cancel the pending
URBs immediately. For the suspend case, the display will be turned
off, so it makes no sense to process the rendering. And for the
disconnect case, the device may be no longer accessible, hence we
shouldn't do any submission.
Tested-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.h | 2 ++
drivers/gpu/drm/udl/udl_main.c| 25 ++---
drivers/gpu/drm/udl/udl_modeset.c | 2 ++
3 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index f01e50c5b7b7..28aaf75d71cf 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -39,6 +39,7 @@ struct urb_node {
struct urb_list {
struct list_head list;
+ struct list_head in_flight;
spinlock_t lock;
wait_queue_head_t sleep;
int available;
@@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
int udl_sync_pending_urbs(struct drm_device *dev);
+void udl_kill_pending_urbs(struct drm_device *dev);
void udl_urb_completion(struct urb *urb);
int udl_init(struct udl_device *udl);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 93615648414b..47204b7eb10e 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
spin_lock_irqsave(&udl->urbs.lock, flags);
- list_add_tail(&unode->entry, &udl->urbs.list);
+ list_move(&unode->entry, &udl->urbs.list);
udl->urbs.available++;
spin_unlock_irqrestore(&udl->urbs.lock, flags);
@@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int
count, size_t size)
retry:
udl->urbs.size = size;
INIT_LIST_HEAD(&udl->urbs.list);
+ INIT_LIST_HEAD(&udl->urbs.in_flight);
init_waitqueue_head(&udl->urbs.sleep);
udl->urbs.count = 0;
@@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev,
long timeout)
}
unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
- list_del_init(&unode->entry);
+ list_move(&unode->entry, &udl->urbs.in_flight);
udl->urbs.available--;
unlock:
@@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
spin_lock_irq(&udl->urbs.lock);
/* 2 seconds as a sane timeout */
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
-udl->urbs.available == udl->urbs.count,
+list_empty(&udl->urbs.in_flight),
udl->urbs.lock,
msecs_to_jiffies(2000)))
ret = -ETIMEDOUT;
@@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
return ret;
}
+/* kill pending URBs */
+void udl_kill_pending_urbs(struct drm_device *dev)
+{
+ struct udl_device *udl = to_udl(dev);
+ struct urb_node *unode;
+
+ spin_lock_irq(&udl->urbs.lock);
+ while (!list_empty(&udl->urbs.in_flight)) {
+ unode = list_first_entry(&udl->urbs.in_flight,
+struct urb_node, entry);
+ spin_unlock_irq(&udl->urbs.lock);
+ usb_kill_urb(unode->urb);
+ spin_lock_irq(&udl->urbs.lock);
+ }
+ spin_unlock_irq(&udl->urbs.lock);
+}
+
int udl_init(struct udl_device *udl)
{
struct drm_device *dev = &udl->drm;
@@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
{
struct udl_device *udl = to_udl(dev);
+ udl_kill_pending_urbs(dev);
udl_free_urb_list(dev);
put_device(udl->dmadev);
udl->dmadev = NULL;
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index 50025606b6ad..169110d8fc2e 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -397,6 +397,8 @@ udl_simple_display_pipe_disable(struct
drm_simple_display_pipe *pipe)
struct urb *urb;
char *buf;
+ udl_kill_pending_urbs(dev);
+
urb = udl_get_urb(dev);
if (!urb)
return;
--
2.35.3
[PATCH 0/4] drm/udl: Fix stray URBs and cleanup
Hi, this is a series of fixes for UDL driver to address the leftover URBs at suspend/resume. It begins with the simplification of FIFO control code with the standard wait queue, followed by the handling of pending URBs, and replace BUG_ON() with WARN_ON() as a cleanup. Takashi === Takashi Iwai (4): drm/udl: Replace semaphore with a simple wait queue drm/udl: Sync pending URBs at suspend / disconnect drm/udl: Kill pending URBs at suspend and disconnect drm/udl: Replace BUG_ON() with WARN_ON() drivers/gpu/drm/udl/udl_drv.h | 14 +++- drivers/gpu/drm/udl/udl_main.c | 125 ++--- drivers/gpu/drm/udl/udl_modeset.c | 4 + drivers/gpu/drm/udl/udl_transfer.c | 3 +- 4 files changed, 79 insertions(+), 67 deletions(-) -- 2.35.3
[PATCH 1/4] drm/udl: Replace semaphore with a simple wait queue
UDL driver uses a semaphore for controlling the emptiness of FIFO in a
slightly funky way. This patch replaces it with a wait queue and
controls the emptiness with the standard wait_event*() macro instead,
which is a more straightforward implementation.
While we are at it, drop the dead code for delayed semaphore down,
too.
Tested-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.h | 11 +++--
drivers/gpu/drm/udl/udl_main.c | 84 ++
2 files changed, 31 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index cc16a13316e4..e008686ec738 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -34,14 +34,13 @@ struct udl_device;
struct urb_node {
struct list_head entry;
struct udl_device *dev;
- struct delayed_work release_urb_work;
struct urb *urb;
};
struct urb_list {
struct list_head list;
spinlock_t lock;
- struct semaphore limit_sem;
+ wait_queue_head_t sleep;
int available;
int count;
size_t size;
@@ -75,7 +74,13 @@ static inline struct usb_device *udl_to_usb_device(struct
udl_device *udl)
int udl_modeset_init(struct drm_device *dev);
struct drm_connector *udl_connector_init(struct drm_device *dev);
-struct urb *udl_get_urb(struct drm_device *dev);
+struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout);
+
+#define GET_URB_TIMEOUTHZ
+static inline struct urb *udl_get_urb(struct drm_device *dev)
+{
+ return udl_get_urb_timeout(dev, GET_URB_TIMEOUT);
+}
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
void udl_urb_completion(struct urb *urb);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 853f147036f6..67fd41e59b80 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -23,9 +23,6 @@
#define WRITES_IN_FLIGHT (4)
#define MAX_VENDOR_DESCRIPTOR_SIZE 256
-#define GET_URB_TIMEOUTHZ
-#define FREE_URB_TIMEOUT (HZ*2)
-
static int udl_parse_vendor_descriptor(struct udl_device *udl)
{
struct usb_device *udev = udl_to_usb_device(udl);
@@ -119,14 +116,6 @@ static int udl_select_std_channel(struct udl_device *udl)
return ret < 0 ? ret : 0;
}
-static void udl_release_urb_work(struct work_struct *work)
-{
- struct urb_node *unode = container_of(work, struct urb_node,
- release_urb_work.work);
-
- up(&unode->dev->urbs.limit_sem);
-}
-
void udl_urb_completion(struct urb *urb)
{
struct urb_node *unode = urb->context;
@@ -150,23 +139,13 @@ void udl_urb_completion(struct urb *urb)
udl->urbs.available++;
spin_unlock_irqrestore(&udl->urbs.lock, flags);
-#if 0
- /*
-* When using fb_defio, we deadlock if up() is called
-* while another is waiting. So queue to another process.
-*/
- if (fb_defio)
- schedule_delayed_work(&unode->release_urb_work, 0);
- else
-#endif
- up(&udl->urbs.limit_sem);
+ wake_up(&udl->urbs.sleep);
}
static void udl_free_urb_list(struct drm_device *dev)
{
struct udl_device *udl = to_udl(dev);
int count = udl->urbs.count;
- struct list_head *node;
struct urb_node *unode;
struct urb *urb;
@@ -174,23 +153,15 @@ static void udl_free_urb_list(struct drm_device *dev)
/* keep waiting and freeing, until we've got 'em all */
while (count--) {
- down(&udl->urbs.limit_sem);
-
- spin_lock_irq(&udl->urbs.lock);
-
- node = udl->urbs.list.next; /* have reserved one with sem */
- list_del_init(node);
-
- spin_unlock_irq(&udl->urbs.lock);
-
- unode = list_entry(node, struct urb_node, entry);
- urb = unode->urb;
-
+ urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
+ if (WARN_ON(!urb))
+ break;
+ unode = urb->context;
/* Free each separately allocated piece */
usb_free_coherent(urb->dev, udl->urbs.size,
urb->transfer_buffer, urb->transfer_dma);
usb_free_urb(urb);
- kfree(node);
+ kfree(unode);
}
udl->urbs.count = 0;
}
@@ -210,7 +181,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int
count, size_t size)
udl->urbs.size = size;
INIT_LIST_HEAD(&udl->urbs.list);
- sema_init(&udl->urbs.limit_sem, 0);
+ init_waitqueue_head(&udl->urbs.sleep);
udl->urbs.count = 0;
udl->urbs.available = 0;
@@ -220,9 +191,6 @@ static
[PATCH 2/4] drm/udl: Sync pending URBs at suspend / disconnect
We need to wait for finishing to process the all URBs after disabling
the pipe; otherwise pending URBs may stray at suspend/resume, leading
to a possible memory corruption in a worst case.
Tested-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.h | 1 +
drivers/gpu/drm/udl/udl_main.c| 17 +
drivers/gpu/drm/udl/udl_modeset.c | 2 ++
3 files changed, 20 insertions(+)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index e008686ec738..f01e50c5b7b7 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -83,6 +83,7 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
}
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
+int udl_sync_pending_urbs(struct drm_device *dev);
void udl_urb_completion(struct urb *urb);
int udl_init(struct udl_device *udl);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 67fd41e59b80..93615648414b 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -270,6 +270,23 @@ int udl_submit_urb(struct drm_device *dev, struct urb
*urb, size_t len)
return ret;
}
+/* wait until all pending URBs have been processed */
+int udl_sync_pending_urbs(struct drm_device *dev)
+{
+ struct udl_device *udl = to_udl(dev);
+ int ret = 0;
+
+ spin_lock_irq(&udl->urbs.lock);
+ /* 2 seconds as a sane timeout */
+ if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
+udl->urbs.available == udl->urbs.count,
+udl->urbs.lock,
+msecs_to_jiffies(2000)))
+ ret = -ETIMEDOUT;
+ spin_unlock_irq(&udl->urbs.lock);
+ return ret;
+}
+
int udl_init(struct udl_device *udl)
{
struct drm_device *dev = &udl->drm;
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index e67c40a48fb4..50025606b6ad 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -408,6 +408,8 @@ udl_simple_display_pipe_disable(struct
drm_simple_display_pipe *pipe)
buf = udl_dummy_render(buf);
udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer);
+
+ udl_sync_pending_urbs(dev);
}
static void
--
2.35.3
[PATCH 4/4] drm/udl: Replace BUG_ON() with WARN_ON()
BUG_ON() is a tasteless choice as a sanity check for a driver like UDL that isn't really a core code. Replace with WARN_ON() and proper error handling instead. Tested-by: Thomas Zimmermann Signed-off-by: Takashi Iwai --- drivers/gpu/drm/udl/udl_main.c | 3 ++- drivers/gpu/drm/udl/udl_transfer.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 47204b7eb10e..fdafbf8f3c3c 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -260,7 +260,8 @@ int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len) struct udl_device *udl = to_udl(dev); int ret; - BUG_ON(len > udl->urbs.size); + if (WARN_ON(len > udl->urbs.size)) + return -EINVAL; urb->transfer_buffer_length = len; /* set to actual payload len */ ret = usb_submit_urb(urb, GFP_ATOMIC); diff --git a/drivers/gpu/drm/udl/udl_transfer.c b/drivers/gpu/drm/udl/udl_transfer.c index 971927669d6b..176ef2a6a731 100644 --- a/drivers/gpu/drm/udl/udl_transfer.c +++ b/drivers/gpu/drm/udl/udl_transfer.c @@ -220,7 +220,8 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr, u8 *cmd = *urb_buf_ptr; u8 *cmd_end = (u8 *) urb->transfer_buffer + urb->transfer_buffer_length; - BUG_ON(!(log_bpp == 1 || log_bpp == 2)); + if (WARN_ON(!(log_bpp == 1 || log_bpp == 2))) + return -EINVAL; line_start = (u8 *) (front + byte_offset); next_pixel = line_start; -- 2.35.3
Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect
On Tue, 09 Aug 2022 09:13:16 +0200,
Thomas Zimmermann wrote:
>
> Hi
>
> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> > At both suspend and disconnect, we should rather cancel the pending
> > URBs immediately. For the suspend case, the display will be turned
> > off, so it makes no sense to process the rendering. And for the
> > disconnect case, the device may be no longer accessible, hence we
> > shouldn't do any submission.
> >
> > Tested-by: Thomas Zimmermann
> > Signed-off-by: Takashi Iwai
> > ---
> > drivers/gpu/drm/udl/udl_drv.h | 2 ++
> > drivers/gpu/drm/udl/udl_main.c| 25 ++---
> > drivers/gpu/drm/udl/udl_modeset.c | 2 ++
> > 3 files changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> > index f01e50c5b7b7..28aaf75d71cf 100644
> > --- a/drivers/gpu/drm/udl/udl_drv.h
> > +++ b/drivers/gpu/drm/udl/udl_drv.h
> > @@ -39,6 +39,7 @@ struct urb_node {
> > struct urb_list {
> > struct list_head list;
> > + struct list_head in_flight;
> > spinlock_t lock;
> > wait_queue_head_t sleep;
> > int available;
> > @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device
> > *dev)
> > int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> > size_t len);
> > int udl_sync_pending_urbs(struct drm_device *dev);
> > +void udl_kill_pending_urbs(struct drm_device *dev);
> > void udl_urb_completion(struct urb *urb);
> > int udl_init(struct udl_device *udl);
> > diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> > index 93615648414b..47204b7eb10e 100644
> > --- a/drivers/gpu/drm/udl/udl_main.c
> > +++ b/drivers/gpu/drm/udl/udl_main.c
> > @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> > urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
> > spin_lock_irqsave(&udl->urbs.lock, flags);
> > - list_add_tail(&unode->entry, &udl->urbs.list);
> > + list_move(&unode->entry, &udl->urbs.list);
> > udl->urbs.available++;
> > spin_unlock_irqrestore(&udl->urbs.lock, flags);
> > @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> > drm_device *dev, int count, size_t size)
> > retry:
> > udl->urbs.size = size;
> > INIT_LIST_HEAD(&udl->urbs.list);
> > + INIT_LIST_HEAD(&udl->urbs.in_flight);
> > init_waitqueue_head(&udl->urbs.sleep);
> > udl->urbs.count = 0;
> > @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev,
> > long timeout)
> > }
> > unode = list_first_entry(&udl->urbs.list, struct urb_node,
> > entry);
> > - list_del_init(&unode->entry);
> > + list_move(&unode->entry, &udl->urbs.in_flight);
> > udl->urbs.available--;
> > unlock:
> > @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> > spin_lock_irq(&udl->urbs.lock);
> > /* 2 seconds as a sane timeout */
> > if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> > -udl->urbs.available == udl->urbs.count,
> > +list_empty(&udl->urbs.in_flight),
> > udl->urbs.lock,
> > msecs_to_jiffies(2000)))
> > ret = -ETIMEDOUT;
> > @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> > return ret;
> > }
> > +/* kill pending URBs */
> > +void udl_kill_pending_urbs(struct drm_device *dev)
> > +{
> > + struct udl_device *udl = to_udl(dev);
> > + struct urb_node *unode;
> > +
> > + spin_lock_irq(&udl->urbs.lock);
> > + while (!list_empty(&udl->urbs.in_flight)) {
> > + unode = list_first_entry(&udl->urbs.in_flight,
> > +struct urb_node, entry);
> > + spin_unlock_irq(&udl->urbs.lock);
> > + usb_kill_urb(unode->urb);
> > + spin_lock_irq(&udl->urbs.lock);
> > + }
> > + spin_unlock_irq(&udl->urbs.lock);
> > +}
> > +
> > int udl_init(struct udl_device *udl)
> > {
> > struct drm_device *dev = &udl->drm;
> > @@ -335,6 +353,7 @@ int udl_drop_usb(struct drm_device *dev)
> > {
> > stru
Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect
On Tue, 09 Aug 2022 09:41:19 +0200,
Thomas Zimmermann wrote:
>
> Hi
>
> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
> > On Tue, 09 Aug 2022 09:13:16 +0200,
> > Thomas Zimmermann wrote:
> >>
> >> Hi
> >>
> >> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> >>> At both suspend and disconnect, we should rather cancel the pending
> >>> URBs immediately. For the suspend case, the display will be turned
> >>> off, so it makes no sense to process the rendering. And for the
> >>> disconnect case, the device may be no longer accessible, hence we
> >>> shouldn't do any submission.
> >>>
> >>> Tested-by: Thomas Zimmermann
> >>> Signed-off-by: Takashi Iwai
> >>> ---
> >>>drivers/gpu/drm/udl/udl_drv.h | 2 ++
> >>>drivers/gpu/drm/udl/udl_main.c| 25 ++---
> >>>drivers/gpu/drm/udl/udl_modeset.c | 2 ++
> >>>3 files changed, 26 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> >>> index f01e50c5b7b7..28aaf75d71cf 100644
> >>> --- a/drivers/gpu/drm/udl/udl_drv.h
> >>> +++ b/drivers/gpu/drm/udl/udl_drv.h
> >>> @@ -39,6 +39,7 @@ struct urb_node {
> >>> struct urb_list {
> >>> struct list_head list;
> >>> + struct list_head in_flight;
> >>> spinlock_t lock;
> >>> wait_queue_head_t sleep;
> >>> int available;
> >>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct drm_device
> >>> *dev)
> >>> int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> >>> size_t len);
> >>>int udl_sync_pending_urbs(struct drm_device *dev);
> >>> +void udl_kill_pending_urbs(struct drm_device *dev);
> >>>void udl_urb_completion(struct urb *urb);
> >>> int udl_init(struct udl_device *udl);
> >>> diff --git a/drivers/gpu/drm/udl/udl_main.c
> >>> b/drivers/gpu/drm/udl/udl_main.c
> >>> index 93615648414b..47204b7eb10e 100644
> >>> --- a/drivers/gpu/drm/udl/udl_main.c
> >>> +++ b/drivers/gpu/drm/udl/udl_main.c
> >>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> >>> urb->transfer_buffer_length = udl->urbs.size; /* reset to
> >>> actual */
> >>> spin_lock_irqsave(&udl->urbs.lock, flags);
> >>> - list_add_tail(&unode->entry, &udl->urbs.list);
> >>> + list_move(&unode->entry, &udl->urbs.list);
> >>> udl->urbs.available++;
> >>> spin_unlock_irqrestore(&udl->urbs.lock, flags);
> >>>@@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> >>> drm_device *dev, int count, size_t size)
> >>>retry:
> >>> udl->urbs.size = size;
> >>> INIT_LIST_HEAD(&udl->urbs.list);
> >>> + INIT_LIST_HEAD(&udl->urbs.in_flight);
> >>> init_waitqueue_head(&udl->urbs.sleep);
> >>> udl->urbs.count = 0;
> >>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device
> >>> *dev, long timeout)
> >>> }
> >>> unode = list_first_entry(&udl->urbs.list, struct urb_node,
> >>> entry);
> >>> - list_del_init(&unode->entry);
> >>> + list_move(&unode->entry, &udl->urbs.in_flight);
> >>> udl->urbs.available--;
> >>> unlock:
> >>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>> spin_lock_irq(&udl->urbs.lock);
> >>> /* 2 seconds as a sane timeout */
> >>> if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
> >>> - udl->urbs.available == udl->urbs.count,
> >>> + list_empty(&udl->urbs.in_flight),
> >>>udl->urbs.lock,
> >>>msecs_to_jiffies(2000)))
> >>> ret = -ETIMEDOUT;
> >>> @@ -287,6 +288,23 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>> return ret;
> >>>}
> >>>+/* kill pending URBs */
> >>&
Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect
On Tue, 09 Aug 2022 11:13:46 +0200,
Thomas Zimmermann wrote:
>
> Hi
>
> Am 09.08.22 um 11:03 schrieb Takashi Iwai:
> > On Tue, 09 Aug 2022 09:41:19 +0200,
> > Thomas Zimmermann wrote:
> >>
> >> Hi
> >>
> >> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
> >>> On Tue, 09 Aug 2022 09:13:16 +0200,
> >>> Thomas Zimmermann wrote:
> >>>>
> >>>> Hi
> >>>>
> >>>> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> >>>>> At both suspend and disconnect, we should rather cancel the pending
> >>>>> URBs immediately. For the suspend case, the display will be turned
> >>>>> off, so it makes no sense to process the rendering. And for the
> >>>>> disconnect case, the device may be no longer accessible, hence we
> >>>>> shouldn't do any submission.
> >>>>>
> >>>>> Tested-by: Thomas Zimmermann
> >>>>> Signed-off-by: Takashi Iwai
> >>>>> ---
> >>>>> drivers/gpu/drm/udl/udl_drv.h | 2 ++
> >>>>> drivers/gpu/drm/udl/udl_main.c| 25 ++---
> >>>>> drivers/gpu/drm/udl/udl_modeset.c | 2 ++
> >>>>> 3 files changed, 26 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h
> >>>>> b/drivers/gpu/drm/udl/udl_drv.h
> >>>>> index f01e50c5b7b7..28aaf75d71cf 100644
> >>>>> --- a/drivers/gpu/drm/udl/udl_drv.h
> >>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
> >>>>> @@ -39,6 +39,7 @@ struct urb_node {
> >>>>> struct urb_list {
> >>>>> struct list_head list;
> >>>>> + struct list_head in_flight;
> >>>>> spinlock_t lock;
> >>>>> wait_queue_head_t sleep;
> >>>>> int available;
> >>>>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct
> >>>>> drm_device *dev)
> >>>>> int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> >>>>> size_t len);
> >>>>> int udl_sync_pending_urbs(struct drm_device *dev);
> >>>>> +void udl_kill_pending_urbs(struct drm_device *dev);
> >>>>> void udl_urb_completion(struct urb *urb);
> >>>>> int udl_init(struct udl_device *udl);
> >>>>> diff --git a/drivers/gpu/drm/udl/udl_main.c
> >>>>> b/drivers/gpu/drm/udl/udl_main.c
> >>>>> index 93615648414b..47204b7eb10e 100644
> >>>>> --- a/drivers/gpu/drm/udl/udl_main.c
> >>>>> +++ b/drivers/gpu/drm/udl/udl_main.c
> >>>>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> >>>>> urb->transfer_buffer_length = udl->urbs.size; /* reset to
> >>>>> actual */
> >>>>> spin_lock_irqsave(&udl->urbs.lock, flags);
> >>>>> - list_add_tail(&unode->entry, &udl->urbs.list);
> >>>>> + list_move(&unode->entry, &udl->urbs.list);
> >>>>> udl->urbs.available++;
> >>>>> spin_unlock_irqrestore(&udl->urbs.lock, flags);
> >>>>> @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> >>>>> drm_device *dev, int count, size_t size)
> >>>>> retry:
> >>>>> udl->urbs.size = size;
> >>>>> INIT_LIST_HEAD(&udl->urbs.list);
> >>>>> + INIT_LIST_HEAD(&udl->urbs.in_flight);
> >>>>> init_waitqueue_head(&udl->urbs.sleep);
> >>>>> udl->urbs.count = 0;
> >>>>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device
> >>>>> *dev, long timeout)
> >>>>> }
> >>>>> unode = list_first_entry(&udl->urbs.list, struct urb_node,
> >>>>> entry);
> >>>>> - list_del_init(&unode->entry);
> >>>>> + list_move(&unode->entry, &udl->urbs.in_flight);
> >>>>> udl->urbs.available--;
> >>>>> unlock:
> >>>>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>>> spin_lock_irq
Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect
On Tue, 09 Aug 2022 11:19:30 +0200,
Takashi Iwai wrote:
>
> On Tue, 09 Aug 2022 11:13:46 +0200,
> Thomas Zimmermann wrote:
> >
> > Hi
> >
> > Am 09.08.22 um 11:03 schrieb Takashi Iwai:
> > > On Tue, 09 Aug 2022 09:41:19 +0200,
> > > Thomas Zimmermann wrote:
> > >>
> > >> Hi
> > >>
> > >> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
> > >>> On Tue, 09 Aug 2022 09:13:16 +0200,
> > >>> Thomas Zimmermann wrote:
> > >>>>
> > >>>> Hi
> > >>>>
> > >>>> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> > >>>>> At both suspend and disconnect, we should rather cancel the pending
> > >>>>> URBs immediately. For the suspend case, the display will be turned
> > >>>>> off, so it makes no sense to process the rendering. And for the
> > >>>>> disconnect case, the device may be no longer accessible, hence we
> > >>>>> shouldn't do any submission.
> > >>>>>
> > >>>>> Tested-by: Thomas Zimmermann
> > >>>>> Signed-off-by: Takashi Iwai
> > >>>>> ---
> > >>>>> drivers/gpu/drm/udl/udl_drv.h | 2 ++
> > >>>>> drivers/gpu/drm/udl/udl_main.c| 25 ++---
> > >>>>> drivers/gpu/drm/udl/udl_modeset.c | 2 ++
> > >>>>> 3 files changed, 26 insertions(+), 3 deletions(-)
> > >>>>>
> > >>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h
> > >>>>> b/drivers/gpu/drm/udl/udl_drv.h
> > >>>>> index f01e50c5b7b7..28aaf75d71cf 100644
> > >>>>> --- a/drivers/gpu/drm/udl/udl_drv.h
> > >>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
> > >>>>> @@ -39,6 +39,7 @@ struct urb_node {
> > >>>>> struct urb_list {
> > >>>>> struct list_head list;
> > >>>>> + struct list_head in_flight;
> > >>>>> spinlock_t lock;
> > >>>>> wait_queue_head_t sleep;
> > >>>>> int available;
> > >>>>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct
> > >>>>> drm_device *dev)
> > >>>>> int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> > >>>>> size_t len);
> > >>>>> int udl_sync_pending_urbs(struct drm_device *dev);
> > >>>>> +void udl_kill_pending_urbs(struct drm_device *dev);
> > >>>>> void udl_urb_completion(struct urb *urb);
> > >>>>> int udl_init(struct udl_device *udl);
> > >>>>> diff --git a/drivers/gpu/drm/udl/udl_main.c
> > >>>>> b/drivers/gpu/drm/udl/udl_main.c
> > >>>>> index 93615648414b..47204b7eb10e 100644
> > >>>>> --- a/drivers/gpu/drm/udl/udl_main.c
> > >>>>> +++ b/drivers/gpu/drm/udl/udl_main.c
> > >>>>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> > >>>>> urb->transfer_buffer_length = udl->urbs.size; /* reset to
> > >>>>> actual */
> > >>>>> spin_lock_irqsave(&udl->urbs.lock, flags);
> > >>>>> - list_add_tail(&unode->entry, &udl->urbs.list);
> > >>>>> + list_move(&unode->entry, &udl->urbs.list);
> > >>>>> udl->urbs.available++;
> > >>>>> spin_unlock_irqrestore(&udl->urbs.lock, flags);
> > >>>>> @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> > >>>>> drm_device *dev, int count, size_t size)
> > >>>>> retry:
> > >>>>> udl->urbs.size = size;
> > >>>>> INIT_LIST_HEAD(&udl->urbs.list);
> > >>>>> + INIT_LIST_HEAD(&udl->urbs.in_flight);
> > >>>>> init_waitqueue_head(&udl->urbs.sleep);
> > >>>>> udl->urbs.count = 0;
> > >>>>> @@ -246,7 +247,7 @@ struct urb *udl_get_urb_timeout(struct drm_device
> > >>>>> *dev, long timeout)
> > >>>>> }
> > >>>>> unode = list_first_entry(&udl->urbs.list, struct
>
Re: [PATCH 3/4] drm/udl: Kill pending URBs at suspend and disconnect
On Tue, 16 Aug 2022 16:01:34 +0200,
Thomas Zimmermann wrote:
>
> Hi Takashi
>
> Am 16.08.22 um 15:55 schrieb Takashi Iwai:
> > On Tue, 09 Aug 2022 11:19:30 +0200,
> > Takashi Iwai wrote:
> >>
> >> On Tue, 09 Aug 2022 11:13:46 +0200,
> >> Thomas Zimmermann wrote:
> >>>
> >>> Hi
> >>>
> >>> Am 09.08.22 um 11:03 schrieb Takashi Iwai:
> >>>> On Tue, 09 Aug 2022 09:41:19 +0200,
> >>>> Thomas Zimmermann wrote:
> >>>>>
> >>>>> Hi
> >>>>>
> >>>>> Am 09.08.22 um 09:15 schrieb Takashi Iwai:
> >>>>>> On Tue, 09 Aug 2022 09:13:16 +0200,
> >>>>>> Thomas Zimmermann wrote:
> >>>>>>>
> >>>>>>> Hi
> >>>>>>>
> >>>>>>> Am 04.08.22 um 09:58 schrieb Takashi Iwai:
> >>>>>>>> At both suspend and disconnect, we should rather cancel the pending
> >>>>>>>> URBs immediately. For the suspend case, the display will be turned
> >>>>>>>> off, so it makes no sense to process the rendering. And for the
> >>>>>>>> disconnect case, the device may be no longer accessible, hence we
> >>>>>>>> shouldn't do any submission.
> >>>>>>>>
> >>>>>>>> Tested-by: Thomas Zimmermann
> >>>>>>>> Signed-off-by: Takashi Iwai
> >>>>>>>> ---
> >>>>>>>> drivers/gpu/drm/udl/udl_drv.h | 2 ++
> >>>>>>>> drivers/gpu/drm/udl/udl_main.c| 25 ++---
> >>>>>>>> drivers/gpu/drm/udl/udl_modeset.c | 2 ++
> >>>>>>>> 3 files changed, 26 insertions(+), 3 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h
> >>>>>>>> b/drivers/gpu/drm/udl/udl_drv.h
> >>>>>>>> index f01e50c5b7b7..28aaf75d71cf 100644
> >>>>>>>> --- a/drivers/gpu/drm/udl/udl_drv.h
> >>>>>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
> >>>>>>>> @@ -39,6 +39,7 @@ struct urb_node {
> >>>>>>>>struct urb_list {
> >>>>>>>> struct list_head list;
> >>>>>>>> +struct list_head in_flight;
> >>>>>>>> spinlock_t lock;
> >>>>>>>> wait_queue_head_t sleep;
> >>>>>>>> int available;
> >>>>>>>> @@ -84,6 +85,7 @@ static inline struct urb *udl_get_urb(struct
> >>>>>>>> drm_device *dev)
> >>>>>>>>int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> >>>>>>>> size_t len);
> >>>>>>>> int udl_sync_pending_urbs(struct drm_device *dev);
> >>>>>>>> +void udl_kill_pending_urbs(struct drm_device *dev);
> >>>>>>>> void udl_urb_completion(struct urb *urb);
> >>>>>>>>int udl_init(struct udl_device *udl);
> >>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_main.c
> >>>>>>>> b/drivers/gpu/drm/udl/udl_main.c
> >>>>>>>> index 93615648414b..47204b7eb10e 100644
> >>>>>>>> --- a/drivers/gpu/drm/udl/udl_main.c
> >>>>>>>> +++ b/drivers/gpu/drm/udl/udl_main.c
> >>>>>>>> @@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
> >>>>>>>> urb->transfer_buffer_length = udl->urbs.size; /* reset
> >>>>>>>> to actual */
> >>>>>>>> spin_lock_irqsave(&udl->urbs.lock, flags);
> >>>>>>>> -list_add_tail(&unode->entry, &udl->urbs.list);
> >>>>>>>> +list_move(&unode->entry, &udl->urbs.list);
> >>>>>>>> udl->urbs.available++;
> >>>>>>>> spin_unlock_irqrestore(&udl->urbs.lock, flags);
> >>>>>>>> @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> >>>>>>>> drm_device *dev, int count, size_t size)
> >>>>>>&g
[PATCH 00/12] drm/udl: More fixes
Hi, this patch set contains more fixes for UDL driver, to be applied on top of my previous patch set [*]. It covers the PM problems, regressions in the previous patch set, fixes for the stalls on some systems, as well as more hardening. Takashi [*] https://lore.kernel.org/r/20220804075826.27036-1-ti...@suse.de === Takashi Iwai (8): Revert "drm/udl: Kill pending URBs at suspend and disconnect" drm/udl: Suppress error print for -EPROTO at URB completion drm/udl: Increase the default URB list size to 20 drm/udl: Drop unneeded alignment drm/udl: Fix potential URB leaks drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list() drm/udl: Don't re-initialize stuff at retrying the URB list allocation drm/udl: Sync pending URBs at the end of suspend Thomas Zimmermann (4): drm/udl: Restore display mode on resume drm/udl: Add reset_resume drm/udl: Enable damage clipping drm/udl: Add parameter to set number of URBs drivers/gpu/drm/udl/udl_drv.c | 19 +- drivers/gpu/drm/udl/udl_drv.h | 13 +--- drivers/gpu/drm/udl/udl_main.c | 97 +++--- drivers/gpu/drm/udl/udl_modeset.c | 42 - drivers/gpu/drm/udl/udl_transfer.c | 45 ++ 5 files changed, 86 insertions(+), 130 deletions(-) -- 2.35.3
[PATCH 02/12] drm/udl: Add reset_resume
From: Thomas Zimmermann
Implement the reset_resume callback of struct usb_driver. Set the
standard channel when called.
Signed-off-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.c | 11 +++
drivers/gpu/drm/udl/udl_drv.h | 1 +
drivers/gpu/drm/udl/udl_main.c | 2 +-
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 5703277c6f52..0ba88e5472a9 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -32,6 +32,16 @@ static int udl_usb_resume(struct usb_interface *interface)
return drm_mode_config_helper_resume(dev);
}
+static int udl_usb_reset_resume(struct usb_interface *interface)
+{
+ struct drm_device *dev = usb_get_intfdata(interface);
+ struct udl_device *udl = to_udl(dev);
+
+ udl_select_std_channel(udl);
+
+ return drm_mode_config_helper_resume(dev);
+}
+
/*
* FIXME: Dma-buf sharing requires DMA support by the importing device.
*This function is a workaround to make USB devices work as well.
@@ -140,6 +150,7 @@ static struct usb_driver udl_driver = {
.disconnect = udl_usb_disconnect,
.suspend = udl_usb_suspend,
.resume = udl_usb_resume,
+ .reset_resume = udl_usb_reset_resume,
.id_table = id_table,
};
module_usb_driver(udl_driver);
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 28aaf75d71cf..37c14b0ff1fc 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -95,6 +95,7 @@ int udl_render_hline(struct drm_device *dev, int log_bpp,
struct urb **urb_ptr,
u32 byte_offset, u32 device_byte_offset, u32 byte_width);
int udl_drop_usb(struct drm_device *dev);
+int udl_select_std_channel(struct udl_device *udl);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */
#define CMD_WRITE_RL8"\xAF\x61" /**< 8 bit run length command. */
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index fdafbf8f3c3c..7d1e6bbc165c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -92,7 +92,7 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
/*
* Need to ensure a channel is selected before submitting URBs
*/
-static int udl_select_std_channel(struct udl_device *udl)
+int udl_select_std_channel(struct udl_device *udl)
{
static const u8 set_def_chn[] = {0x57, 0xCD, 0xDC, 0xA7,
0x1C, 0x88, 0x5E, 0x15,
--
2.35.3
[PATCH 03/12] drm/udl: Enable damage clipping
From: Thomas Zimmermann Call drm_plane_enable_fb_damage_clips() and give userspace a chance of minimizing the updated display area. Signed-off-by: Thomas Zimmermann Signed-off-by: Takashi Iwai --- drivers/gpu/drm/udl/udl_modeset.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c index df987644fb5d..187aba2d7825 100644 --- a/drivers/gpu/drm/udl/udl_modeset.c +++ b/drivers/gpu/drm/udl/udl_modeset.c @@ -484,6 +484,7 @@ int udl_modeset_init(struct drm_device *dev) format_count, NULL, connector); if (ret) return ret; + drm_plane_enable_fb_damage_clips(&udl->display_pipe.plane); drm_mode_config_reset(dev); -- 2.35.3
[PATCH 01/12] drm/udl: Restore display mode on resume
From: Thomas Zimmermann Restore the display mode whne resuming from suspend. Currently, the display remains dark. On resume, the CRTC's mode does not change, but the 'active' flag changes to 'true'. Taking this into account when considering a mode switch restores the display mode. The bug is reproducable by using Gnome with udl and observing the adapter's suspend/resume behavior. Signed-off-by: Thomas Zimmermann Signed-off-by: Takashi Iwai --- drivers/gpu/drm/udl/udl_modeset.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c index 169110d8fc2e..df987644fb5d 100644 --- a/drivers/gpu/drm/udl/udl_modeset.c +++ b/drivers/gpu/drm/udl/udl_modeset.c @@ -8,6 +8,7 @@ * Copyright (C) 2009 Bernie Thompson */ +#include #include #include #include @@ -382,7 +383,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe, udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height); - if (!crtc_state->mode_changed) + if (!drm_atomic_crtc_needs_modeset(crtc_state)) return; /* enable display */ -- 2.35.3
[PATCH 11/12] drm/udl: Don't re-initialize stuff at retrying the URB list allocation
udl_alloc_urb_list() retires the allocation if there is no enough room
left, and it reinitializes the stuff unnecessarily such as the linked
list head and the waitqueue, which could be harmful. Those should be
outside the retry loop.
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_main.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 19dc8317e843..c1f4b6199949 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -187,15 +187,14 @@ static int udl_alloc_urb_list(struct drm_device *dev, int
count, size_t size)
struct usb_device *udev = udl_to_usb_device(udl);
spin_lock_init(&udl->urbs.lock);
-
-retry:
- udl->urbs.size = size;
INIT_LIST_HEAD(&udl->urbs.list);
-
init_waitqueue_head(&udl->urbs.sleep);
udl->urbs.count = 0;
udl->urbs.available = 0;
+retry:
+ udl->urbs.size = size;
+
while (udl->urbs.count * size < wanted_size) {
unode = kzalloc(sizeof(struct urb_node), GFP_KERNEL);
if (!unode)
--
2.35.3
[PATCH 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
In the current design, udl_get_urb() may be called asynchronously
during the driver freeing its URL list via udl_free_urb_list().
The problem is that the sync is determined by comparing the urbs.count
and urbs.available fields, while we clear urbs.count field only once
after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
the state becomes inconsistent.
For fixing this inconsistency and also for hardening the locking
scheme, this patch does a slight refactoring of the code around
udl_get_urb() and udl_free_urb_list(). Now urbs.count is updated in
the same spinlock at extracting a URB from the list in
udl_free_url_list().
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.h | 8 +---
drivers/gpu/drm/udl/udl_main.c | 37 --
2 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 5923d2e02bc8..d943684b5bbb 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -74,13 +74,7 @@ static inline struct usb_device *udl_to_usb_device(struct
udl_device *udl)
int udl_modeset_init(struct drm_device *dev);
struct drm_connector *udl_connector_init(struct drm_device *dev);
-struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout);
-
-#define GET_URB_TIMEOUTHZ
-static inline struct urb *udl_get_urb(struct drm_device *dev)
-{
- return udl_get_urb_timeout(dev, GET_URB_TIMEOUT);
-}
+struct urb *udl_get_urb(struct drm_device *dev);
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
int udl_sync_pending_urbs(struct drm_device *dev);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 8bbb4e2861fb..19dc8317e843 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -28,6 +28,8 @@
static uint udl_num_urbs = WRITES_IN_FLIGHT;
module_param_named(numurbs, udl_num_urbs, uint, 0600);
+static struct urb *__udl_get_urb(struct udl_device *udl, long timeout);
+
static int udl_parse_vendor_descriptor(struct udl_device *udl)
{
struct usb_device *udev = udl_to_usb_device(udl);
@@ -151,15 +153,17 @@ void udl_urb_completion(struct urb *urb)
static void udl_free_urb_list(struct drm_device *dev)
{
struct udl_device *udl = to_udl(dev);
- int count = udl->urbs.count;
struct urb_node *unode;
struct urb *urb;
DRM_DEBUG("Waiting for completes and freeing all render urbs\n");
/* keep waiting and freeing, until we've got 'em all */
- while (count--) {
- urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
+ while (udl->urbs.count) {
+ spin_lock_irq(&udl->urbs.lock);
+ urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
+ udl->urbs.count--;
+ spin_unlock_irq(&udl->urbs.lock);
if (WARN_ON(!urb))
break;
unode = urb->context;
@@ -169,7 +173,8 @@ static void udl_free_urb_list(struct drm_device *dev)
usb_free_urb(urb);
kfree(unode);
}
- udl->urbs.count = 0;
+
+ wake_up(&udl->urbs.sleep);
}
static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
@@ -233,33 +238,43 @@ static int udl_alloc_urb_list(struct drm_device *dev, int
count, size_t size)
return udl->urbs.count;
}
-struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
+static struct urb *__udl_get_urb(struct udl_device *udl, long timeout)
{
- struct udl_device *udl = to_udl(dev);
- struct urb_node *unode = NULL;
+ struct urb_node *unode;
+
+ assert_spin_locked(&udl->urbs.lock);
if (!udl->urbs.count)
return NULL;
/* Wait for an in-flight buffer to complete and get re-queued */
- spin_lock_irq(&udl->urbs.lock);
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
!list_empty(&udl->urbs.list),
udl->urbs.lock, timeout)) {
DRM_INFO("wait for urb interrupted: available: %d\n",
udl->urbs.available);
- goto unlock;
+ return NULL;
}
unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
list_del_init(&unode->entry);
udl->urbs.available--;
-unlock:
- spin_unlock_irq(&udl->urbs.lock);
return unode ? unode->urb : NULL;
}
+#define GET_URB_TIMEOUTHZ
+struct urb *udl_get_urb(struct drm_device *dev)
+{
+ struct udl_device *udl = to_udl(dev);
+ struct urb *urb;
+
+ spin_lock_irq(&udl->urbs.lock);
+ urb = __udl_get_urb(udl, GET_URB_TIMEOUT);
+ spin_unlock_irq(&udl->urbs.lock
[PATCH 05/12] drm/udl: Suppress error print for -EPROTO at URB completion
The driver may receive -EPROTO at the URB completion when the device
gets disconnected, and it's a normal situation. Suppress the error
print for that, too.
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index a9f6b710b254..6aed6e0f669c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -126,6 +126,7 @@ void udl_urb_completion(struct urb *urb)
if (urb->status) {
if (!(urb->status == -ENOENT ||
urb->status == -ECONNRESET ||
+ urb->status == -EPROTO ||
urb->status == -ESHUTDOWN)) {
DRM_ERROR("%s - nonzero write bulk status received:
%d\n",
__func__, urb->status);
--
2.35.3
[PATCH 06/12] drm/udl: Increase the default URB list size to 20
It seems that the current size (4) for the URB list is too small on some devices, and it resulted in the occasional stalls. Increase the default URB list size to 20 for working around it. Signed-off-by: Takashi Iwai --- drivers/gpu/drm/udl/udl_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 6aed6e0f669c..2b7eafd48ec2 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -20,7 +20,7 @@ #define NR_USB_REQUEST_CHANNEL 0x12 #define MAX_TRANSFER (PAGE_SIZE*16 - BULK_SIZE) -#define WRITES_IN_FLIGHT (4) +#define WRITES_IN_FLIGHT (20) #define MAX_VENDOR_DESCRIPTOR_SIZE 256 static int udl_parse_vendor_descriptor(struct udl_device *udl) -- 2.35.3
[PATCH 08/12] drm/udl: Drop unneeded alignment
The alignment of damaged area was needed for the original udlfb driver
that tried to trim the superfluous copies between front and backend
buffers and handle data in long int. It's not the case for udl DRM
driver, hence we can omit the whole unneeded alignment, as well as the
dead code.
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_modeset.c | 34 ++---
drivers/gpu/drm/udl/udl_transfer.c | 40 --
2 files changed, 8 insertions(+), 66 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index c34d564773a3..bca31c890108 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -243,28 +243,6 @@ static long udl_log_cpp(unsigned int cpp)
return __ffs(cpp);
}
-static int udl_aligned_damage_clip(struct drm_rect *clip, int x, int y,
- int width, int height)
-{
- int x1, x2;
-
- if (WARN_ON_ONCE(x < 0) ||
- WARN_ON_ONCE(y < 0) ||
- WARN_ON_ONCE(width < 0) ||
- WARN_ON_ONCE(height < 0))
- return -EINVAL;
-
- x1 = ALIGN_DOWN(x, sizeof(unsigned long));
- x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
-
- clip->x1 = x1;
- clip->y1 = y;
- clip->x2 = x2;
- clip->y2 = y + height;
-
- return 0;
-}
-
static int udl_handle_damage(struct drm_framebuffer *fb,
const struct iosys_map *map,
int x, int y, int width, int height)
@@ -277,15 +255,19 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
struct drm_rect clip;
int log_bpp;
+ if (width <= 0 || height <= 0)
+ return 0;
+
ret = udl_log_cpp(fb->format->cpp[0]);
if (ret < 0)
return ret;
log_bpp = ret;
- ret = udl_aligned_damage_clip(&clip, x, y, width, height);
- if (ret)
- return ret;
- else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
+ clip.x1 = x;
+ clip.y1 = y;
+ clip.x2 = x + width;
+ clip.y2 = y + height;
+ if (clip.x2 > fb->width || clip.y2 > fb->height)
return -EINVAL;
ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
diff --git a/drivers/gpu/drm/udl/udl_transfer.c
b/drivers/gpu/drm/udl/udl_transfer.c
index 176ef2a6a731..a431208dda85 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -25,46 +25,6 @@
#define MIN_RAW_PIX_BYTES 2
#define MIN_RAW_CMD_BYTES (RAW_HEADER_BYTES + MIN_RAW_PIX_BYTES)
-/*
- * Trims identical data from front and back of line
- * Sets new front buffer address and width
- * And returns byte count of identical pixels
- * Assumes CPU natural alignment (unsigned long)
- * for back and front buffer ptrs and width
- */
-#if 0
-static int udl_trim_hline(const u8 *bback, const u8 **bfront, int *width_bytes)
-{
- int j, k;
- const unsigned long *back = (const unsigned long *) bback;
- const unsigned long *front = (const unsigned long *) *bfront;
- const int width = *width_bytes / sizeof(unsigned long);
- int identical = width;
- int start = width;
- int end = width;
-
- for (j = 0; j < width; j++) {
- if (back[j] != front[j]) {
- start = j;
- break;
- }
- }
-
- for (k = width - 1; k > j; k--) {
- if (back[k] != front[k]) {
- end = k+1;
- break;
- }
- }
-
- identical = start + (width - end);
- *bfront = (u8 *) &front[start];
- *width_bytes = (end - start) * sizeof(unsigned long);
-
- return identical * sizeof(unsigned long);
-}
-#endif
-
static inline u16 pixel32_to_be16(const uint32_t pixel)
{
return (((pixel >> 3) & 0x001f) |
--
2.35.3
[PATCH 09/12] drm/udl: Fix potential URB leaks
A couple of error handlings forgot to process the URB completion.
Those are both with WARN_ON() so should be visible, but we must fix
them in anyway.
Fixes: 7350b2a3fbc6 ("drm/udl: Replace BUG_ON() with WARN_ON()")
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_main.c | 8 +---
drivers/gpu/drm/udl/udl_transfer.c | 5 -
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 3c97f647883f..8bbb4e2861fb 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -265,11 +265,13 @@ int udl_submit_urb(struct drm_device *dev, struct urb
*urb, size_t len)
struct udl_device *udl = to_udl(dev);
int ret;
- if (WARN_ON(len > udl->urbs.size))
- return -EINVAL;
-
+ if (WARN_ON(len > udl->urbs.size)) {
+ ret = -EINVAL;
+ goto error;
+ }
urb->transfer_buffer_length = len; /* set to actual payload len */
ret = usb_submit_urb(urb, GFP_ATOMIC);
+ error:
if (ret) {
udl_urb_completion(urb); /* because no one else will */
DRM_ERROR("usb_submit_urb error %x\n", ret);
diff --git a/drivers/gpu/drm/udl/udl_transfer.c
b/drivers/gpu/drm/udl/udl_transfer.c
index a431208dda85..b57844632dbd 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -180,8 +180,11 @@ int udl_render_hline(struct drm_device *dev, int log_bpp,
struct urb **urb_ptr,
u8 *cmd = *urb_buf_ptr;
u8 *cmd_end = (u8 *) urb->transfer_buffer + urb->transfer_buffer_length;
- if (WARN_ON(!(log_bpp == 1 || log_bpp == 2)))
+ if (WARN_ON(!(log_bpp == 1 || log_bpp == 2))) {
+ /* need to finish URB at error from this function */
+ udl_urb_completion(urb);
return -EINVAL;
+ }
line_start = (u8 *) (front + byte_offset);
next_pixel = line_start;
--
2.35.3
[PATCH 07/12] drm/udl: Add parameter to set number of URBs
From: Thomas Zimmermann
For further debugging and optimization purpose, allow users to adjust
the number of URBs via a new module parameter, numurbs.
Signed-off-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_main.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 2b7eafd48ec2..3c97f647883f 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -8,6 +8,8 @@
* Copyright (C) 2009 Bernie Thompson
*/
+#include
+
#include
#include
#include
@@ -23,6 +25,9 @@
#define WRITES_IN_FLIGHT (20)
#define MAX_VENDOR_DESCRIPTOR_SIZE 256
+static uint udl_num_urbs = WRITES_IN_FLIGHT;
+module_param_named(numurbs, udl_num_urbs, uint, 0600);
+
static int udl_parse_vendor_descriptor(struct udl_device *udl)
{
struct usb_device *udev = udl_to_usb_device(udl);
@@ -294,6 +299,8 @@ int udl_init(struct udl_device *udl)
struct drm_device *dev = &udl->drm;
int ret = -ENOMEM;
+ drm_info(dev, "pre-allocating %d URBs\n", udl_num_urbs);
+
DRM_DEBUG("\n");
udl->dmadev = usb_intf_get_dma_device(to_usb_interface(dev->dev));
@@ -311,7 +318,7 @@ int udl_init(struct udl_device *udl)
if (udl_select_std_channel(udl))
DRM_ERROR("Selecting channel failed\n");
- if (!udl_alloc_urb_list(dev, WRITES_IN_FLIGHT, MAX_TRANSFER)) {
+ if (!udl_alloc_urb_list(dev, udl_num_urbs, MAX_TRANSFER)) {
DRM_ERROR("udl_alloc_urb_list failed\n");
goto err;
}
--
2.35.3
[PATCH 12/12] drm/udl: Sync pending URBs at the end of suspend
It's better to perform the sync at the very last of the suspend
instead of the pipe-disable function, so that we can catch all pending
URBs (if any).
While we're at it, drop the error code from udl_sync_pending_urb()
since we basically ignore it; instead, give a clear error message
indicating a problem.
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.c | 8 +++-
drivers/gpu/drm/udl/udl_drv.h | 2 +-
drivers/gpu/drm/udl/udl_main.c| 6 ++
drivers/gpu/drm/udl/udl_modeset.c | 2 --
4 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 0ba88e5472a9..91effdcefb6d 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -21,8 +21,14 @@ static int udl_usb_suspend(struct usb_interface *interface,
pm_message_t message)
{
struct drm_device *dev = usb_get_intfdata(interface);
+ int ret;
- return drm_mode_config_helper_suspend(dev);
+ ret = drm_mode_config_helper_suspend(dev);
+ if (ret)
+ return ret;
+
+ udl_sync_pending_urbs(dev);
+ return 0;
}
static int udl_usb_resume(struct usb_interface *interface)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index d943684b5bbb..b4cc7cc568c7 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -77,7 +77,7 @@ struct drm_connector *udl_connector_init(struct drm_device
*dev);
struct urb *udl_get_urb(struct drm_device *dev);
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
-int udl_sync_pending_urbs(struct drm_device *dev);
+void udl_sync_pending_urbs(struct drm_device *dev);
void udl_urb_completion(struct urb *urb);
int udl_init(struct udl_device *udl);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index c1f4b6199949..df92f6518e1c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -294,10 +294,9 @@ int udl_submit_urb(struct drm_device *dev, struct urb
*urb, size_t len)
}
/* wait until all pending URBs have been processed */
-int udl_sync_pending_urbs(struct drm_device *dev)
+void udl_sync_pending_urbs(struct drm_device *dev)
{
struct udl_device *udl = to_udl(dev);
- int ret = 0;
spin_lock_irq(&udl->urbs.lock);
/* 2 seconds as a sane timeout */
@@ -305,9 +304,8 @@ int udl_sync_pending_urbs(struct drm_device *dev)
udl->urbs.available == udl->urbs.count,
udl->urbs.lock,
msecs_to_jiffies(2000)))
- ret = -ETIMEDOUT;
+ drm_err(dev, "Timeout for syncing pending URBs\n");
spin_unlock_irq(&udl->urbs.lock);
- return ret;
}
int udl_init(struct udl_device *udl)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index bca31c890108..9d72288d9967 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -391,8 +391,6 @@ udl_simple_display_pipe_disable(struct
drm_simple_display_pipe *pipe)
buf = udl_dummy_render(buf);
udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer);
-
- udl_sync_pending_urbs(dev);
}
static void
--
2.35.3
Re: [syzbot] KASAN: use-after-free Read in udl_get_urb_timeout
On Mon, 22 Aug 2022 11:09:31 +0200, syzbot wrote: > > Hello, > > syzbot found the following issue on: > > HEAD commit:5b6a4bf680d6 Add linux-next specific files for 20220818 > git tree: linux-next > console+strace: https://syzkaller.appspot.com/x/log.txt?x=12341a3d08 > kernel config: https://syzkaller.appspot.com/x/.config?x=ead6107a3bbe3c62 > dashboard link: https://syzkaller.appspot.com/bug?extid=f24934fe125a19d77eae > compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils > for Debian) 2.35.2 > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1273186708 > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=165b64f308 > > The issue was bisected to: > > commit e25d5954264d1871ab2792c7ca2298b811462500 > Author: Takashi Iwai > Date: Thu Aug 4 07:58:25 2022 + > > drm/udl: Kill pending URBs at suspend and disconnect FYI, the fix including the revert of this commit was already submitted, waiting for review & merge: https://lore.kernel.org/r/20220816153655.27526-1-ti...@suse.de thanks, Takashi
[PATCH 04/12] Revert "drm/udl: Kill pending URBs at suspend and disconnect"
This reverts the recent fix commit
e25d5954264d ("drm/udl: Kill pending URBs at suspend and disconnect")
as it turned out to lead to potential hangup at a disconnection, and
it doesn't help much for suspend/resume problem, either.
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.h | 2 --
drivers/gpu/drm/udl/udl_main.c| 25 +++--
drivers/gpu/drm/udl/udl_modeset.c | 2 --
3 files changed, 3 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 37c14b0ff1fc..5923d2e02bc8 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -39,7 +39,6 @@ struct urb_node {
struct urb_list {
struct list_head list;
- struct list_head in_flight;
spinlock_t lock;
wait_queue_head_t sleep;
int available;
@@ -85,7 +84,6 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
int udl_sync_pending_urbs(struct drm_device *dev);
-void udl_kill_pending_urbs(struct drm_device *dev);
void udl_urb_completion(struct urb *urb);
int udl_init(struct udl_device *udl);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 7d1e6bbc165c..a9f6b710b254 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
spin_lock_irqsave(&udl->urbs.lock, flags);
- list_move(&unode->entry, &udl->urbs.list);
+ list_add_tail(&unode->entry, &udl->urbs.list);
udl->urbs.available++;
spin_unlock_irqrestore(&udl->urbs.lock, flags);
@@ -180,7 +180,6 @@ static int udl_alloc_urb_list(struct drm_device *dev, int
count, size_t size)
retry:
udl->urbs.size = size;
INIT_LIST_HEAD(&udl->urbs.list);
- INIT_LIST_HEAD(&udl->urbs.in_flight);
init_waitqueue_head(&udl->urbs.sleep);
udl->urbs.count = 0;
@@ -247,7 +246,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev,
long timeout)
}
unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
- list_move(&unode->entry, &udl->urbs.in_flight);
+ list_del_init(&unode->entry);
udl->urbs.available--;
unlock:
@@ -281,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
spin_lock_irq(&udl->urbs.lock);
/* 2 seconds as a sane timeout */
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
-list_empty(&udl->urbs.in_flight),
+udl->urbs.available == udl->urbs.count,
udl->urbs.lock,
msecs_to_jiffies(2000)))
ret = -ETIMEDOUT;
@@ -289,23 +288,6 @@ int udl_sync_pending_urbs(struct drm_device *dev)
return ret;
}
-/* kill pending URBs */
-void udl_kill_pending_urbs(struct drm_device *dev)
-{
- struct udl_device *udl = to_udl(dev);
- struct urb_node *unode;
-
- spin_lock_irq(&udl->urbs.lock);
- while (!list_empty(&udl->urbs.in_flight)) {
- unode = list_first_entry(&udl->urbs.in_flight,
-struct urb_node, entry);
- spin_unlock_irq(&udl->urbs.lock);
- usb_kill_urb(unode->urb);
- spin_lock_irq(&udl->urbs.lock);
- }
- spin_unlock_irq(&udl->urbs.lock);
-}
-
int udl_init(struct udl_device *udl)
{
struct drm_device *dev = &udl->drm;
@@ -354,7 +336,6 @@ int udl_drop_usb(struct drm_device *dev)
{
struct udl_device *udl = to_udl(dev);
- udl_kill_pending_urbs(dev);
udl_free_urb_list(dev);
put_device(udl->dmadev);
udl->dmadev = NULL;
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index 187aba2d7825..c34d564773a3 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -398,8 +398,6 @@ udl_simple_display_pipe_disable(struct
drm_simple_display_pipe *pipe)
struct urb *urb;
char *buf;
- udl_kill_pending_urbs(dev);
-
urb = udl_get_urb(dev);
if (!urb)
return;
--
2.35.3
Re: [PATCH v5 2/2] ALSA: hda - identify when audio is provided by a video driver
On Sat, 30 Apr 2022 22:04:55 +0200, Mauro Carvalho Chehab wrote: > > On some devices, the hda driver needs to hook into a video driver, > in order to be able to properly access the audio hardware and/or > the power management function. > > That's the case of several snd_hda_intel devices that depends on > i915 driver. > > Ensure that a proper reference between the snd-hda driver needing > such binding is shown at /proc/modules, in order to allow userspace > to know about such binding. > > Signed-off-by: Mauro Carvalho Chehab Maybe I was too late to the game (just back from vacation), but FWIW: Reviewed-by: Takashi Iwai thanks, Takashi
Re: [PATCH 08/12] drm/udl: Drop unneeded alignment
On Mon, 05 Sep 2022 10:40:58 +0200,
Thomas Zimmermann wrote:
>
> Hi
>
> Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> > The alignment of damaged area was needed for the original udlfb driver
> > that tried to trim the superfluous copies between front and backend
> > buffers and handle data in long int. It's not the case for udl DRM
> > driver, hence we can omit the whole unneeded alignment, as well as the
> > dead code.
> >
> > Signed-off-by: Takashi Iwai
> > ---
> > drivers/gpu/drm/udl/udl_modeset.c | 34 ++---
> > drivers/gpu/drm/udl/udl_transfer.c | 40 --
> > 2 files changed, 8 insertions(+), 66 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/udl/udl_modeset.c
> > b/drivers/gpu/drm/udl/udl_modeset.c
> > index c34d564773a3..bca31c890108 100644
> > --- a/drivers/gpu/drm/udl/udl_modeset.c
> > +++ b/drivers/gpu/drm/udl/udl_modeset.c
> > @@ -243,28 +243,6 @@ static long udl_log_cpp(unsigned int cpp)
> > return __ffs(cpp);
> > }
> > -static int udl_aligned_damage_clip(struct drm_rect *clip, int x,
> > int y,
> > - int width, int height)
> > -{
> > - int x1, x2;
> > -
> > - if (WARN_ON_ONCE(x < 0) ||
> > - WARN_ON_ONCE(y < 0) ||
> > - WARN_ON_ONCE(width < 0) ||
> > - WARN_ON_ONCE(height < 0))
> > - return -EINVAL;
> > -
> > - x1 = ALIGN_DOWN(x, sizeof(unsigned long));
> > - x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
> > -
> > - clip->x1 = x1;
> > - clip->y1 = y;
> > - clip->x2 = x2;
> > - clip->y2 = y + height;
> > -
> > - return 0;
> > -}
> > -
> > static int udl_handle_damage(struct drm_framebuffer *fb,
> > const struct iosys_map *map,
> > int x, int y, int width, int height)
> > @@ -277,15 +255,19 @@ static int udl_handle_damage(struct drm_framebuffer
> > *fb,
> > struct drm_rect clip;
> > int log_bpp;
> > + if (width <= 0 || height <= 0)
> > + return 0;
> > +
>
> That shouldn't happen.
>
> > ret = udl_log_cpp(fb->format->cpp[0]);
> > if (ret < 0)
> > return ret;
> > log_bpp = ret;
> > - ret = udl_aligned_damage_clip(&clip, x, y, width, height);
> > - if (ret)
> > - return ret;
> > - else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
> > + clip.x1 = x;
> > + clip.y1 = y;
> > + clip.x2 = x + width;
> > + clip.y2 = y + height;
>
> drm_rect_init() please.
>
> > + if (clip.x2 > fb->width || clip.y2 > fb->height)
>
> That's another thing that should not happen. The damage clips in the
> plane state is what you what to copy. The DRM helpers ensure that
> these various plane, fb and clip coordinates add up.
OK, then we can drop those clip size checks completely.
Will do that in v2 patch.
thanks,
Takashi
Re: [PATCH 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
On Mon, 05 Sep 2022 10:32:55 +0200,
Thomas Zimmermann wrote:
>
> Hi
>
> Am 16.08.22 um 17:36 schrieb Takashi Iwai:
> > In the current design, udl_get_urb() may be called asynchronously
> > during the driver freeing its URL list via udl_free_urb_list().
> > The problem is that the sync is determined by comparing the urbs.count
> > and urbs.available fields, while we clear urbs.count field only once
> > after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
> > the state becomes inconsistent.
> >
> > For fixing this inconsistency and also for hardening the locking
> > scheme, this patch does a slight refactoring of the code around
> > udl_get_urb() and udl_free_urb_list(). Now urbs.count is updated in
> > the same spinlock at extracting a URB from the list in
> > udl_free_url_list().
> >
> > Signed-off-by: Takashi Iwai
> > ---
> > drivers/gpu/drm/udl/udl_drv.h | 8 +---
> > drivers/gpu/drm/udl/udl_main.c | 37 --
> > 2 files changed, 27 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> > index 5923d2e02bc8..d943684b5bbb 100644
> > --- a/drivers/gpu/drm/udl/udl_drv.h
> > +++ b/drivers/gpu/drm/udl/udl_drv.h
> > @@ -74,13 +74,7 @@ static inline struct usb_device
> > *udl_to_usb_device(struct udl_device *udl)
> > int udl_modeset_init(struct drm_device *dev);
> > struct drm_connector *udl_connector_init(struct drm_device *dev);
> > -struct urb *udl_get_urb_timeout(struct drm_device *dev, long
> > timeout);
> > -
> > -#define GET_URB_TIMEOUTHZ
> > -static inline struct urb *udl_get_urb(struct drm_device *dev)
> > -{
> > - return udl_get_urb_timeout(dev, GET_URB_TIMEOUT);
> > -}
> > +struct urb *udl_get_urb(struct drm_device *dev);
> > int udl_submit_urb(struct drm_device *dev, struct urb *urb,
> > size_t len);
> > int udl_sync_pending_urbs(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> > index 8bbb4e2861fb..19dc8317e843 100644
> > --- a/drivers/gpu/drm/udl/udl_main.c
> > +++ b/drivers/gpu/drm/udl/udl_main.c
> > @@ -28,6 +28,8 @@
> > static uint udl_num_urbs = WRITES_IN_FLIGHT;
> > module_param_named(numurbs, udl_num_urbs, uint, 0600);
> > +static struct urb *__udl_get_urb(struct udl_device *udl, long
> > timeout);
> > +
> > static int udl_parse_vendor_descriptor(struct udl_device *udl)
> > {
> > struct usb_device *udev = udl_to_usb_device(udl);
> > @@ -151,15 +153,17 @@ void udl_urb_completion(struct urb *urb)
> > static void udl_free_urb_list(struct drm_device *dev)
> > {
> > struct udl_device *udl = to_udl(dev);
> > - int count = udl->urbs.count;
> > struct urb_node *unode;
> > struct urb *urb;
> > DRM_DEBUG("Waiting for completes and freeing all render
> > urbs\n");
> > /* keep waiting and freeing, until we've got 'em all */
> > - while (count--) {
> > - urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
> > + while (udl->urbs.count) {
> > + spin_lock_irq(&udl->urbs.lock);
> > + urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
> > + udl->urbs.count--;
> > + spin_unlock_irq(&udl->urbs.lock);
> > if (WARN_ON(!urb))
> > break;
> > unode = urb->context;
> > @@ -169,7 +173,8 @@ static void udl_free_urb_list(struct drm_device *dev)
> > usb_free_urb(urb);
> > kfree(unode);
> > }
> > - udl->urbs.count = 0;
> > +
> > + wake_up(&udl->urbs.sleep);
>
> There's just one waiter, but it's the shutdown code. Maybe
> wake_up_all() would more clearly communicate the intention.
OK.
> > }
> > static int udl_alloc_urb_list(struct drm_device *dev, int count,
> > size_t size)
> > @@ -233,33 +238,43 @@ static int udl_alloc_urb_list(struct drm_device *dev,
> > int count, size_t size)
> > return udl->urbs.count;
> > }
> > -struct urb *udl_get_urb_timeout(struct drm_device *dev, long
> > timeout)
> > +static struct urb *__udl_get_urb(struct udl_device *udl, long timeout)
>
> I think in DRM, the correct name for this function would be
> udl_get_urb_locked().
OK.
> > {
> > - struct udl_device *udl = to_udl(dev);
> > - struct urb_node *unode = NULL;
> > + struct urb_node *uno
Re: [PATCH 12/12] drm/udl: Sync pending URBs at the end of suspend
On Mon, 05 Sep 2022 10:44:25 +0200, Thomas Zimmermann wrote: > > Hi > > Am 16.08.22 um 17:36 schrieb Takashi Iwai: > > It's better to perform the sync at the very last of the suspend > > instead of the pipe-disable function, so that we can catch all pending > > URBs (if any). > > > > While we're at it, drop the error code from udl_sync_pending_urb() > > since we basically ignore it; instead, give a clear error message > > indicating a problem. > > But if we fail, shouldn't we report that error to the caller of the > suspend function? It's an open question. We may fail the suspend, but OTOH, the sync error is likely nothing we can recover from at any time later, either; that is, even if we return an error and abort the suspend, it wouldn't help so much from the practical POV. So for now -- and just for this URL device handling -- I'm inclined to continue suspending. But if anyone has more strong argument against it, I'm all ears. thanks, Takashi
[PATCH v2 01/11] drm/udl: Restore display mode on resume
From: Thomas Zimmermann Restore the display mode whne resuming from suspend. Currently, the display remains dark. On resume, the CRTC's mode does not change, but the 'active' flag changes to 'true'. Taking this into account when considering a mode switch restores the display mode. The bug is reproducable by using Gnome with udl and observing the adapter's suspend/resume behavior. Signed-off-by: Thomas Zimmermann Signed-off-by: Takashi Iwai --- drivers/gpu/drm/udl/udl_modeset.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c index 169110d8fc2e..df987644fb5d 100644 --- a/drivers/gpu/drm/udl/udl_modeset.c +++ b/drivers/gpu/drm/udl/udl_modeset.c @@ -8,6 +8,7 @@ * Copyright (C) 2009 Bernie Thompson */ +#include #include #include #include @@ -382,7 +383,7 @@ udl_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe, udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width, fb->height); - if (!crtc_state->mode_changed) + if (!drm_atomic_crtc_needs_modeset(crtc_state)) return; /* enable display */ -- 2.35.3
[PATCH v2 02/11] drm/udl: Add reset_resume
From: Thomas Zimmermann
Implement the reset_resume callback of struct usb_driver. Set the
standard channel when called.
Signed-off-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.c | 11 +++
drivers/gpu/drm/udl/udl_drv.h | 1 +
drivers/gpu/drm/udl/udl_main.c | 2 +-
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 5703277c6f52..0ba88e5472a9 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -32,6 +32,16 @@ static int udl_usb_resume(struct usb_interface *interface)
return drm_mode_config_helper_resume(dev);
}
+static int udl_usb_reset_resume(struct usb_interface *interface)
+{
+ struct drm_device *dev = usb_get_intfdata(interface);
+ struct udl_device *udl = to_udl(dev);
+
+ udl_select_std_channel(udl);
+
+ return drm_mode_config_helper_resume(dev);
+}
+
/*
* FIXME: Dma-buf sharing requires DMA support by the importing device.
*This function is a workaround to make USB devices work as well.
@@ -140,6 +150,7 @@ static struct usb_driver udl_driver = {
.disconnect = udl_usb_disconnect,
.suspend = udl_usb_suspend,
.resume = udl_usb_resume,
+ .reset_resume = udl_usb_reset_resume,
.id_table = id_table,
};
module_usb_driver(udl_driver);
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 28aaf75d71cf..37c14b0ff1fc 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -95,6 +95,7 @@ int udl_render_hline(struct drm_device *dev, int log_bpp,
struct urb **urb_ptr,
u32 byte_offset, u32 device_byte_offset, u32 byte_width);
int udl_drop_usb(struct drm_device *dev);
+int udl_select_std_channel(struct udl_device *udl);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */
#define CMD_WRITE_RL8"\xAF\x61" /**< 8 bit run length command. */
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index fdafbf8f3c3c..7d1e6bbc165c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -92,7 +92,7 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
/*
* Need to ensure a channel is selected before submitting URBs
*/
-static int udl_select_std_channel(struct udl_device *udl)
+int udl_select_std_channel(struct udl_device *udl)
{
static const u8 set_def_chn[] = {0x57, 0xCD, 0xDC, 0xA7,
0x1C, 0x88, 0x5E, 0x15,
--
2.35.3
[PATCH v2 00/11] drm/udl: More fixes
Hi, this is a revised patch set for cleaning up and fixes for UDL driver. It covers the PM problems, regressions in the previous patch set, fixes for the stalls on some systems, as well as more hardening. Takashi === v1->v2: cleanups as suggested by Thomas - Drop numurbs parameter patch - Clean up / simplify clipping patch - Code cleanup and changes for urb management patch - Put Acks on some given patches === Takashi Iwai (8): Revert "drm/udl: Kill pending URBs at suspend and disconnect" drm/udl: Suppress error print for -EPROTO at URB completion drm/udl: Increase the default URB list size to 20 drm/udl: Drop unneeded alignment drm/udl: Fix potential URB leaks drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list() drm/udl: Don't re-initialize stuff at retrying the URB list allocation drm/udl: Sync pending URBs at the end of suspend Thomas Zimmermann (3): drm/udl: Restore display mode on resume drm/udl: Add reset_resume drm/udl: Enable damage clipping drivers/gpu/drm/udl/udl_drv.c | 19 +- drivers/gpu/drm/udl/udl_drv.h | 13 +--- drivers/gpu/drm/udl/udl_main.c | 95 +++--- drivers/gpu/drm/udl/udl_modeset.c | 36 ++- drivers/gpu/drm/udl/udl_transfer.c | 45 ++ 5 files changed, 75 insertions(+), 133 deletions(-) -- 2.35.3
[PATCH v2 04/11] Revert "drm/udl: Kill pending URBs at suspend and disconnect"
This reverts the recent fix commit
e25d5954264d ("drm/udl: Kill pending URBs at suspend and disconnect")
as it turned out to lead to potential hangup at a disconnection, and
it doesn't help much for suspend/resume problem, either.
Acked-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.h | 2 --
drivers/gpu/drm/udl/udl_main.c| 25 +++--
drivers/gpu/drm/udl/udl_modeset.c | 2 --
3 files changed, 3 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 37c14b0ff1fc..5923d2e02bc8 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -39,7 +39,6 @@ struct urb_node {
struct urb_list {
struct list_head list;
- struct list_head in_flight;
spinlock_t lock;
wait_queue_head_t sleep;
int available;
@@ -85,7 +84,6 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
int udl_sync_pending_urbs(struct drm_device *dev);
-void udl_kill_pending_urbs(struct drm_device *dev);
void udl_urb_completion(struct urb *urb);
int udl_init(struct udl_device *udl);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 7d1e6bbc165c..a9f6b710b254 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
spin_lock_irqsave(&udl->urbs.lock, flags);
- list_move(&unode->entry, &udl->urbs.list);
+ list_add_tail(&unode->entry, &udl->urbs.list);
udl->urbs.available++;
spin_unlock_irqrestore(&udl->urbs.lock, flags);
@@ -180,7 +180,6 @@ static int udl_alloc_urb_list(struct drm_device *dev, int
count, size_t size)
retry:
udl->urbs.size = size;
INIT_LIST_HEAD(&udl->urbs.list);
- INIT_LIST_HEAD(&udl->urbs.in_flight);
init_waitqueue_head(&udl->urbs.sleep);
udl->urbs.count = 0;
@@ -247,7 +246,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev,
long timeout)
}
unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
- list_move(&unode->entry, &udl->urbs.in_flight);
+ list_del_init(&unode->entry);
udl->urbs.available--;
unlock:
@@ -281,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
spin_lock_irq(&udl->urbs.lock);
/* 2 seconds as a sane timeout */
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
-list_empty(&udl->urbs.in_flight),
+udl->urbs.available == udl->urbs.count,
udl->urbs.lock,
msecs_to_jiffies(2000)))
ret = -ETIMEDOUT;
@@ -289,23 +288,6 @@ int udl_sync_pending_urbs(struct drm_device *dev)
return ret;
}
-/* kill pending URBs */
-void udl_kill_pending_urbs(struct drm_device *dev)
-{
- struct udl_device *udl = to_udl(dev);
- struct urb_node *unode;
-
- spin_lock_irq(&udl->urbs.lock);
- while (!list_empty(&udl->urbs.in_flight)) {
- unode = list_first_entry(&udl->urbs.in_flight,
-struct urb_node, entry);
- spin_unlock_irq(&udl->urbs.lock);
- usb_kill_urb(unode->urb);
- spin_lock_irq(&udl->urbs.lock);
- }
- spin_unlock_irq(&udl->urbs.lock);
-}
-
int udl_init(struct udl_device *udl)
{
struct drm_device *dev = &udl->drm;
@@ -354,7 +336,6 @@ int udl_drop_usb(struct drm_device *dev)
{
struct udl_device *udl = to_udl(dev);
- udl_kill_pending_urbs(dev);
udl_free_urb_list(dev);
put_device(udl->dmadev);
udl->dmadev = NULL;
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index 187aba2d7825..c34d564773a3 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -398,8 +398,6 @@ udl_simple_display_pipe_disable(struct
drm_simple_display_pipe *pipe)
struct urb *urb;
char *buf;
- udl_kill_pending_urbs(dev);
-
urb = udl_get_urb(dev);
if (!urb)
return;
--
2.35.3
[PATCH v2 03/11] drm/udl: Enable damage clipping
From: Thomas Zimmermann Call drm_plane_enable_fb_damage_clips() and give userspace a chance of minimizing the updated display area. Signed-off-by: Thomas Zimmermann Signed-off-by: Takashi Iwai --- drivers/gpu/drm/udl/udl_modeset.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c index df987644fb5d..187aba2d7825 100644 --- a/drivers/gpu/drm/udl/udl_modeset.c +++ b/drivers/gpu/drm/udl/udl_modeset.c @@ -484,6 +484,7 @@ int udl_modeset_init(struct drm_device *dev) format_count, NULL, connector); if (ret) return ret; + drm_plane_enable_fb_damage_clips(&udl->display_pipe.plane); drm_mode_config_reset(dev); -- 2.35.3
[PATCH v2 11/11] drm/udl: Sync pending URBs at the end of suspend
It's better to perform the sync at the very last of the suspend
instead of the pipe-disable function, so that we can catch all pending
URBs (if any).
While we're at it, drop the error code from udl_sync_pending_urb()
since we basically ignore it; instead, give a clear error message
indicating a problem.
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.c | 8 +++-
drivers/gpu/drm/udl/udl_drv.h | 2 +-
drivers/gpu/drm/udl/udl_main.c| 6 ++
drivers/gpu/drm/udl/udl_modeset.c | 2 --
4 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 0ba88e5472a9..91effdcefb6d 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -21,8 +21,14 @@ static int udl_usb_suspend(struct usb_interface *interface,
pm_message_t message)
{
struct drm_device *dev = usb_get_intfdata(interface);
+ int ret;
- return drm_mode_config_helper_suspend(dev);
+ ret = drm_mode_config_helper_suspend(dev);
+ if (ret)
+ return ret;
+
+ udl_sync_pending_urbs(dev);
+ return 0;
}
static int udl_usb_resume(struct usb_interface *interface)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index d943684b5bbb..b4cc7cc568c7 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -77,7 +77,7 @@ struct drm_connector *udl_connector_init(struct drm_device
*dev);
struct urb *udl_get_urb(struct drm_device *dev);
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
-int udl_sync_pending_urbs(struct drm_device *dev);
+void udl_sync_pending_urbs(struct drm_device *dev);
void udl_urb_completion(struct urb *urb);
int udl_init(struct udl_device *udl);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 808a5ab5e14e..442080fa1061 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -290,10 +290,9 @@ int udl_submit_urb(struct drm_device *dev, struct urb
*urb, size_t len)
}
/* wait until all pending URBs have been processed */
-int udl_sync_pending_urbs(struct drm_device *dev)
+void udl_sync_pending_urbs(struct drm_device *dev)
{
struct udl_device *udl = to_udl(dev);
- int ret = 0;
spin_lock_irq(&udl->urbs.lock);
/* 2 seconds as a sane timeout */
@@ -301,9 +300,8 @@ int udl_sync_pending_urbs(struct drm_device *dev)
udl->urbs.available == udl->urbs.count,
udl->urbs.lock,
msecs_to_jiffies(2000)))
- ret = -ETIMEDOUT;
+ drm_err(dev, "Timeout for syncing pending URBs\n");
spin_unlock_irq(&udl->urbs.lock);
- return ret;
}
int udl_init(struct udl_device *udl)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index 9896c16c74f5..c506fff8f0c4 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -383,8 +383,6 @@ udl_simple_display_pipe_disable(struct
drm_simple_display_pipe *pipe)
buf = udl_dummy_render(buf);
udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer);
-
- udl_sync_pending_urbs(dev);
}
static void
--
2.35.3
[PATCH v2 06/11] drm/udl: Increase the default URB list size to 20
It seems that the current size (4) for the URB list is too small on some devices, and it resulted in the occasional stalls. Increase the default URB list size to 20 for working around it. Acked-by: Thomas Zimmermann Signed-off-by: Takashi Iwai --- drivers/gpu/drm/udl/udl_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 6aed6e0f669c..2b7eafd48ec2 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -20,7 +20,7 @@ #define NR_USB_REQUEST_CHANNEL 0x12 #define MAX_TRANSFER (PAGE_SIZE*16 - BULK_SIZE) -#define WRITES_IN_FLIGHT (4) +#define WRITES_IN_FLIGHT (20) #define MAX_VENDOR_DESCRIPTOR_SIZE 256 static int udl_parse_vendor_descriptor(struct udl_device *udl) -- 2.35.3
[PATCH v2 10/11] drm/udl: Don't re-initialize stuff at retrying the URB list allocation
udl_alloc_urb_list() retires the allocation if there is no enough room
left, and it reinitializes the stuff unnecessarily such as the linked
list head and the waitqueue, which could be harmful. Those should be
outside the retry loop.
Acked-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_main.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index df7ebe1fdc90..808a5ab5e14e 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -182,15 +182,14 @@ static int udl_alloc_urb_list(struct drm_device *dev, int
count, size_t size)
struct usb_device *udev = udl_to_usb_device(udl);
spin_lock_init(&udl->urbs.lock);
-
-retry:
- udl->urbs.size = size;
INIT_LIST_HEAD(&udl->urbs.list);
-
init_waitqueue_head(&udl->urbs.sleep);
udl->urbs.count = 0;
udl->urbs.available = 0;
+retry:
+ udl->urbs.size = size;
+
while (udl->urbs.count * size < wanted_size) {
unode = kzalloc(sizeof(struct urb_node), GFP_KERNEL);
if (!unode)
--
2.35.3
[PATCH v2 07/11] drm/udl: Drop unneeded alignment
The alignment of damaged area was needed for the original udlfb driver
that tried to trim the superfluous copies between front and backend
buffers and handle data in long int. It's not the case for udl DRM
driver, hence we can omit the whole unneeded alignment, as well as the
dead code.
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_modeset.c | 28 +
drivers/gpu/drm/udl/udl_transfer.c | 40 --
2 files changed, 1 insertion(+), 67 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index c34d564773a3..9896c16c74f5 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -243,28 +243,6 @@ static long udl_log_cpp(unsigned int cpp)
return __ffs(cpp);
}
-static int udl_aligned_damage_clip(struct drm_rect *clip, int x, int y,
- int width, int height)
-{
- int x1, x2;
-
- if (WARN_ON_ONCE(x < 0) ||
- WARN_ON_ONCE(y < 0) ||
- WARN_ON_ONCE(width < 0) ||
- WARN_ON_ONCE(height < 0))
- return -EINVAL;
-
- x1 = ALIGN_DOWN(x, sizeof(unsigned long));
- x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
-
- clip->x1 = x1;
- clip->y1 = y;
- clip->x2 = x2;
- clip->y2 = y + height;
-
- return 0;
-}
-
static int udl_handle_damage(struct drm_framebuffer *fb,
const struct iosys_map *map,
int x, int y, int width, int height)
@@ -282,11 +260,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
return ret;
log_bpp = ret;
- ret = udl_aligned_damage_clip(&clip, x, y, width, height);
- if (ret)
- return ret;
- else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
- return -EINVAL;
+ drm_rect_init(&clip, x, y, width, height);
ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
if (ret)
diff --git a/drivers/gpu/drm/udl/udl_transfer.c
b/drivers/gpu/drm/udl/udl_transfer.c
index 176ef2a6a731..a431208dda85 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -25,46 +25,6 @@
#define MIN_RAW_PIX_BYTES 2
#define MIN_RAW_CMD_BYTES (RAW_HEADER_BYTES + MIN_RAW_PIX_BYTES)
-/*
- * Trims identical data from front and back of line
- * Sets new front buffer address and width
- * And returns byte count of identical pixels
- * Assumes CPU natural alignment (unsigned long)
- * for back and front buffer ptrs and width
- */
-#if 0
-static int udl_trim_hline(const u8 *bback, const u8 **bfront, int *width_bytes)
-{
- int j, k;
- const unsigned long *back = (const unsigned long *) bback;
- const unsigned long *front = (const unsigned long *) *bfront;
- const int width = *width_bytes / sizeof(unsigned long);
- int identical = width;
- int start = width;
- int end = width;
-
- for (j = 0; j < width; j++) {
- if (back[j] != front[j]) {
- start = j;
- break;
- }
- }
-
- for (k = width - 1; k > j; k--) {
- if (back[k] != front[k]) {
- end = k+1;
- break;
- }
- }
-
- identical = start + (width - end);
- *bfront = (u8 *) &front[start];
- *width_bytes = (end - start) * sizeof(unsigned long);
-
- return identical * sizeof(unsigned long);
-}
-#endif
-
static inline u16 pixel32_to_be16(const uint32_t pixel)
{
return (((pixel >> 3) & 0x001f) |
--
2.35.3
[PATCH v2 09/11] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
In the current design, udl_get_urb() may be called asynchronously
during the driver freeing its URL list via udl_free_urb_list().
The problem is that the sync is determined by comparing the urbs.count
and urbs.available fields, while we clear urbs.count field only once
after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
the state becomes inconsistent.
For fixing this inconsistency and also for hardening the locking
scheme, this patch does a slight refactoring of the code around
udl_get_urb() and udl_free_urb_list(). Now urbs.count is updated in
the same spinlock at extracting a URB from the list in
udl_free_url_list().
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.h | 8 +--
drivers/gpu/drm/udl/udl_main.c | 44 +++---
2 files changed, 31 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 5923d2e02bc8..d943684b5bbb 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -74,13 +74,7 @@ static inline struct usb_device *udl_to_usb_device(struct
udl_device *udl)
int udl_modeset_init(struct drm_device *dev);
struct drm_connector *udl_connector_init(struct drm_device *dev);
-struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout);
-
-#define GET_URB_TIMEOUTHZ
-static inline struct urb *udl_get_urb(struct drm_device *dev)
-{
- return udl_get_urb_timeout(dev, GET_URB_TIMEOUT);
-}
+struct urb *udl_get_urb(struct drm_device *dev);
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
int udl_sync_pending_urbs(struct drm_device *dev);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index de28eeff3155..df7ebe1fdc90 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -23,6 +23,8 @@
#define WRITES_IN_FLIGHT (20)
#define MAX_VENDOR_DESCRIPTOR_SIZE 256
+static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout);
+
static int udl_parse_vendor_descriptor(struct udl_device *udl)
{
struct usb_device *udev = udl_to_usb_device(udl);
@@ -140,21 +142,23 @@ void udl_urb_completion(struct urb *urb)
udl->urbs.available++;
spin_unlock_irqrestore(&udl->urbs.lock, flags);
- wake_up(&udl->urbs.sleep);
+ wake_up_all(&udl->urbs.sleep);
}
static void udl_free_urb_list(struct drm_device *dev)
{
struct udl_device *udl = to_udl(dev);
- int count = udl->urbs.count;
struct urb_node *unode;
struct urb *urb;
DRM_DEBUG("Waiting for completes and freeing all render urbs\n");
/* keep waiting and freeing, until we've got 'em all */
- while (count--) {
- urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
+ while (udl->urbs.count) {
+ spin_lock_irq(&udl->urbs.lock);
+ urb = udl_get_urb_locked(udl, MAX_SCHEDULE_TIMEOUT);
+ udl->urbs.count--;
+ spin_unlock_irq(&udl->urbs.lock);
if (WARN_ON(!urb))
break;
unode = urb->context;
@@ -164,7 +168,8 @@ static void udl_free_urb_list(struct drm_device *dev)
usb_free_urb(urb);
kfree(unode);
}
- udl->urbs.count = 0;
+
+ wake_up(&udl->urbs.sleep);
}
static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
@@ -228,33 +233,44 @@ static int udl_alloc_urb_list(struct drm_device *dev, int
count, size_t size)
return udl->urbs.count;
}
-struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
+static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout)
{
- struct udl_device *udl = to_udl(dev);
- struct urb_node *unode = NULL;
+ struct urb_node *unode;
- if (!udl->urbs.count)
- return NULL;
+ assert_spin_locked(&udl->urbs.lock);
/* Wait for an in-flight buffer to complete and get re-queued */
- spin_lock_irq(&udl->urbs.lock);
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
+!udl->urbs.count ||
!list_empty(&udl->urbs.list),
udl->urbs.lock, timeout)) {
DRM_INFO("wait for urb interrupted: available: %d\n",
udl->urbs.available);
- goto unlock;
+ return NULL;
}
+ if (!udl->urbs.count)
+ return NULL;
+
unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
list_del_init(&unode->entry);
udl->urbs.available--;
-unlock:
- spin_unlock_irq(&udl->urbs.lock);
return unode ? unode->urb : NULL;
}
[PATCH v2 05/11] drm/udl: Suppress error print for -EPROTO at URB completion
The driver may receive -EPROTO at the URB completion when the device
gets disconnected, and it's a normal situation. Suppress the error
print for that, too.
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index a9f6b710b254..6aed6e0f669c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -126,6 +126,7 @@ void udl_urb_completion(struct urb *urb)
if (urb->status) {
if (!(urb->status == -ENOENT ||
urb->status == -ECONNRESET ||
+ urb->status == -EPROTO ||
urb->status == -ESHUTDOWN)) {
DRM_ERROR("%s - nonzero write bulk status received:
%d\n",
__func__, urb->status);
--
2.35.3
[PATCH v2 08/11] drm/udl: Fix potential URB leaks
A couple of error handlings forgot to process the URB completion.
Those are both with WARN_ON() so should be visible, but we must fix
them in anyway.
Fixes: 7350b2a3fbc6 ("drm/udl: Replace BUG_ON() with WARN_ON()")
Acked-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_main.c | 8 +---
drivers/gpu/drm/udl/udl_transfer.c | 5 -
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 2b7eafd48ec2..de28eeff3155 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -260,11 +260,13 @@ int udl_submit_urb(struct drm_device *dev, struct urb
*urb, size_t len)
struct udl_device *udl = to_udl(dev);
int ret;
- if (WARN_ON(len > udl->urbs.size))
- return -EINVAL;
-
+ if (WARN_ON(len > udl->urbs.size)) {
+ ret = -EINVAL;
+ goto error;
+ }
urb->transfer_buffer_length = len; /* set to actual payload len */
ret = usb_submit_urb(urb, GFP_ATOMIC);
+ error:
if (ret) {
udl_urb_completion(urb); /* because no one else will */
DRM_ERROR("usb_submit_urb error %x\n", ret);
diff --git a/drivers/gpu/drm/udl/udl_transfer.c
b/drivers/gpu/drm/udl/udl_transfer.c
index a431208dda85..b57844632dbd 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -180,8 +180,11 @@ int udl_render_hline(struct drm_device *dev, int log_bpp,
struct urb **urb_ptr,
u8 *cmd = *urb_buf_ptr;
u8 *cmd_end = (u8 *) urb->transfer_buffer + urb->transfer_buffer_length;
- if (WARN_ON(!(log_bpp == 1 || log_bpp == 2)))
+ if (WARN_ON(!(log_bpp == 1 || log_bpp == 2))) {
+ /* need to finish URB at error from this function */
+ udl_urb_completion(urb);
return -EINVAL;
+ }
line_start = (u8 *) (front + byte_offset);
next_pixel = line_start;
--
2.35.3
Re: [PATCH 01/12] drm/udl: Restore display mode on resume
On Tue, 06 Sep 2022 22:06:55 +0200,
Daniel Vetter wrote:
>
> On Tue, Aug 16, 2022 at 05:36:44PM +0200, Takashi Iwai wrote:
> > From: Thomas Zimmermann
> >
> > Restore the display mode whne resuming from suspend. Currently, the
> > display remains dark.
> >
> > On resume, the CRTC's mode does not change, but the 'active' flag
> > changes to 'true'. Taking this into account when considering a mode
> > switch restores the display mode.
> >
> > The bug is reproducable by using Gnome with udl and observing the
> > adapter's suspend/resume behavior.
> >
> > Signed-off-by: Thomas Zimmermann
> > Signed-off-by: Takashi Iwai
>
> This patch isn't great and incomplete, see
>
> https://lore.kernel.org/dri-devel/YxegiQFAv+OWjjqE@phenom.ffwll.local/
>
> You need cc: stable and fixes: 997d33c35618 and actually just remove the
> entire check :-)
OK, then is something like below?
I already submitted v2 yesterday (as I overlooked your reply), so I'll
respin v3 with this (and your ack) if that's OK.
thanks,
Takashi
-- 8< --
From: Takashi Iwai
Subject: [PATCH] drm/udl: Restore display mode on resume
Restore the display mode whne resuming from suspend. Currently, the
display remains dark.
On resume, the CRTC's mode does not change, but the 'active' flag
changes to 'true'. Taking this into account when considering a mode
switch restores the display mode.
The bug is reproducable by using Gnome with udl and observing the
adapter's suspend/resume behavior.
Actually, the whole check added in udl_simple_display_pipe_enable()
about the crtc_state->mode_changed was bogus. We should drop the
whole check and always apply the mode change in this function.
[ tiwai -- Drop the mode_changed check entirely instead, per Daniel's
suggestion ]
Fixes: 997d33c35618 ("drm/udl: Inline DPMS code into CRTC enable and disable
functions")
Cc:
Signed-off-by: Thomas Zimmermann
Suggested-by: Daniel Vetter
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_modeset.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index 169110d8fc2e..34ce5b43c5db 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -382,9 +382,6 @@ udl_simple_display_pipe_enable(struct
drm_simple_display_pipe *pipe,
udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width,
fb->height);
- if (!crtc_state->mode_changed)
- return;
-
/* enable display */
udl_crtc_write_mode_to_hw(crtc);
}
--
2.35.3
Re: [PATCH v2 07/11] drm/udl: Drop unneeded alignment
On Wed, 07 Sep 2022 09:29:37 +0200,
Thomas Zimmermann wrote:
>
> Hi
>
> Am 06.09.22 um 09:39 schrieb Takashi Iwai:
> > The alignment of damaged area was needed for the original udlfb driver
> > that tried to trim the superfluous copies between front and backend
> > buffers and handle data in long int. It's not the case for udl DRM
> > driver, hence we can omit the whole unneeded alignment, as well as the
> > dead code.
> >
> > Signed-off-by: Takashi Iwai
>
> Acked-by: Thomas Zimmermann
>
> with an entirely optional comment below.
>
> > ---
> > drivers/gpu/drm/udl/udl_modeset.c | 28 +
> > drivers/gpu/drm/udl/udl_transfer.c | 40 --
> > 2 files changed, 1 insertion(+), 67 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/udl/udl_modeset.c
> > b/drivers/gpu/drm/udl/udl_modeset.c
> > index c34d564773a3..9896c16c74f5 100644
> > --- a/drivers/gpu/drm/udl/udl_modeset.c
> > +++ b/drivers/gpu/drm/udl/udl_modeset.c
> > @@ -243,28 +243,6 @@ static long udl_log_cpp(unsigned int cpp)
> > return __ffs(cpp);
> > }
> > -static int udl_aligned_damage_clip(struct drm_rect *clip, int x,
> > int y,
> > - int width, int height)
> > -{
> > - int x1, x2;
> > -
> > - if (WARN_ON_ONCE(x < 0) ||
> > - WARN_ON_ONCE(y < 0) ||
> > - WARN_ON_ONCE(width < 0) ||
> > - WARN_ON_ONCE(height < 0))
> > - return -EINVAL;
> > -
> > - x1 = ALIGN_DOWN(x, sizeof(unsigned long));
> > - x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
> > -
> > - clip->x1 = x1;
> > - clip->y1 = y;
> > - clip->x2 = x2;
> > - clip->y2 = y + height;
> > -
> > - return 0;
> > -}
> > -
> > static int udl_handle_damage(struct drm_framebuffer *fb,
> > const struct iosys_map *map,
> > int x, int y, int width, int height)
> > @@ -282,11 +260,7 @@ static int udl_handle_damage(struct drm_framebuffer
> > *fb,
> > return ret;
> > log_bpp = ret;
> > - ret = udl_aligned_damage_clip(&clip, x, y, width, height);
> > - if (ret)
> > - return ret;
> > - else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
> > - return -EINVAL;
> > + drm_rect_init(&clip, x, y, width, height);
>
> The clip rectangle could be passed directly by the caller, which would
> simplify the update function. But that's really just nitpicking.
OK, will add a patch to do this, too :)
thanks,
Takashi
[PATCH v3 00/12] drm/udl: More fixes
Hi, this is another respin of patch set for cleaning up and fixes for UDL driver [*]. It covers the PM problems, regressions in the previous patch set, fixes for the stalls on some systems, as well as more hardening. thanks, Takashi [*] v2: https://lore.kernel.org/r/20220906073951.2085-1-ti...@suse.de === v2->v3: - More fix on Restore-on-display-mode patch, suggested by Daniel - Yet more fix for ubs.count check patch, suggested by Thomas - Another patch for passing rectangle directly, suggested by Thomas - Put more Acks from Daniel and Thomas v1->v2: cleanups as suggested by Thomas - Drop numurbs parameter patch - Clean up / simplify clipping patch - Code cleanup and changes for urb management patch - Put Acks on some given patches === Takashi Iwai (10): drm/udl: Restore display mode on resume Revert "drm/udl: Kill pending URBs at suspend and disconnect" drm/udl: Suppress error print for -EPROTO at URB completion drm/udl: Increase the default URB list size to 20 drm/udl: Drop unneeded alignment drm/udl: Pass rectangle directly to udl_handle_damage() drm/udl: Fix potential URB leaks drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list() drm/udl: Don't re-initialize stuff at retrying the URB list allocation drm/udl: Sync pending URBs at the end of suspend Thomas Zimmermann (2): drm/udl: Add reset_resume drm/udl: Enable damage clipping drivers/gpu/drm/udl/udl_drv.c | 19 +- drivers/gpu/drm/udl/udl_drv.h | 13 + drivers/gpu/drm/udl/udl_main.c | 93 +++--- drivers/gpu/drm/udl/udl_modeset.c | 54 - drivers/gpu/drm/udl/udl_transfer.c | 45 ++- 5 files changed, 80 insertions(+), 144 deletions(-) -- 2.35.3
[PATCH v3 02/12] drm/udl: Add reset_resume
From: Thomas Zimmermann
Implement the reset_resume callback of struct usb_driver. Set the
standard channel when called.
Signed-off-by: Thomas Zimmermann
Reviewed-by: Daniel Vetter
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.c | 11 +++
drivers/gpu/drm/udl/udl_drv.h | 1 +
drivers/gpu/drm/udl/udl_main.c | 2 +-
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 5703277c6f52..0ba88e5472a9 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -32,6 +32,16 @@ static int udl_usb_resume(struct usb_interface *interface)
return drm_mode_config_helper_resume(dev);
}
+static int udl_usb_reset_resume(struct usb_interface *interface)
+{
+ struct drm_device *dev = usb_get_intfdata(interface);
+ struct udl_device *udl = to_udl(dev);
+
+ udl_select_std_channel(udl);
+
+ return drm_mode_config_helper_resume(dev);
+}
+
/*
* FIXME: Dma-buf sharing requires DMA support by the importing device.
*This function is a workaround to make USB devices work as well.
@@ -140,6 +150,7 @@ static struct usb_driver udl_driver = {
.disconnect = udl_usb_disconnect,
.suspend = udl_usb_suspend,
.resume = udl_usb_resume,
+ .reset_resume = udl_usb_reset_resume,
.id_table = id_table,
};
module_usb_driver(udl_driver);
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 28aaf75d71cf..37c14b0ff1fc 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -95,6 +95,7 @@ int udl_render_hline(struct drm_device *dev, int log_bpp,
struct urb **urb_ptr,
u32 byte_offset, u32 device_byte_offset, u32 byte_width);
int udl_drop_usb(struct drm_device *dev);
+int udl_select_std_channel(struct udl_device *udl);
#define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */
#define CMD_WRITE_RL8"\xAF\x61" /**< 8 bit run length command. */
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index fdafbf8f3c3c..7d1e6bbc165c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -92,7 +92,7 @@ static int udl_parse_vendor_descriptor(struct udl_device *udl)
/*
* Need to ensure a channel is selected before submitting URBs
*/
-static int udl_select_std_channel(struct udl_device *udl)
+int udl_select_std_channel(struct udl_device *udl)
{
static const u8 set_def_chn[] = {0x57, 0xCD, 0xDC, 0xA7,
0x1C, 0x88, 0x5E, 0x15,
--
2.35.3
[PATCH v3 04/12] Revert "drm/udl: Kill pending URBs at suspend and disconnect"
This reverts the recent fix commit
e25d5954264d ("drm/udl: Kill pending URBs at suspend and disconnect")
as it turned out to lead to potential hangup at a disconnection, and
it doesn't help much for suspend/resume problem, either.
Acked-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.h | 2 --
drivers/gpu/drm/udl/udl_main.c| 25 +++--
drivers/gpu/drm/udl/udl_modeset.c | 2 --
3 files changed, 3 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 37c14b0ff1fc..5923d2e02bc8 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -39,7 +39,6 @@ struct urb_node {
struct urb_list {
struct list_head list;
- struct list_head in_flight;
spinlock_t lock;
wait_queue_head_t sleep;
int available;
@@ -85,7 +84,6 @@ static inline struct urb *udl_get_urb(struct drm_device *dev)
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
int udl_sync_pending_urbs(struct drm_device *dev);
-void udl_kill_pending_urbs(struct drm_device *dev);
void udl_urb_completion(struct urb *urb);
int udl_init(struct udl_device *udl);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 7d1e6bbc165c..a9f6b710b254 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -135,7 +135,7 @@ void udl_urb_completion(struct urb *urb)
urb->transfer_buffer_length = udl->urbs.size; /* reset to actual */
spin_lock_irqsave(&udl->urbs.lock, flags);
- list_move(&unode->entry, &udl->urbs.list);
+ list_add_tail(&unode->entry, &udl->urbs.list);
udl->urbs.available++;
spin_unlock_irqrestore(&udl->urbs.lock, flags);
@@ -180,7 +180,6 @@ static int udl_alloc_urb_list(struct drm_device *dev, int
count, size_t size)
retry:
udl->urbs.size = size;
INIT_LIST_HEAD(&udl->urbs.list);
- INIT_LIST_HEAD(&udl->urbs.in_flight);
init_waitqueue_head(&udl->urbs.sleep);
udl->urbs.count = 0;
@@ -247,7 +246,7 @@ struct urb *udl_get_urb_timeout(struct drm_device *dev,
long timeout)
}
unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
- list_move(&unode->entry, &udl->urbs.in_flight);
+ list_del_init(&unode->entry);
udl->urbs.available--;
unlock:
@@ -281,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
spin_lock_irq(&udl->urbs.lock);
/* 2 seconds as a sane timeout */
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
-list_empty(&udl->urbs.in_flight),
+udl->urbs.available == udl->urbs.count,
udl->urbs.lock,
msecs_to_jiffies(2000)))
ret = -ETIMEDOUT;
@@ -289,23 +288,6 @@ int udl_sync_pending_urbs(struct drm_device *dev)
return ret;
}
-/* kill pending URBs */
-void udl_kill_pending_urbs(struct drm_device *dev)
-{
- struct udl_device *udl = to_udl(dev);
- struct urb_node *unode;
-
- spin_lock_irq(&udl->urbs.lock);
- while (!list_empty(&udl->urbs.in_flight)) {
- unode = list_first_entry(&udl->urbs.in_flight,
-struct urb_node, entry);
- spin_unlock_irq(&udl->urbs.lock);
- usb_kill_urb(unode->urb);
- spin_lock_irq(&udl->urbs.lock);
- }
- spin_unlock_irq(&udl->urbs.lock);
-}
-
int udl_init(struct udl_device *udl)
{
struct drm_device *dev = &udl->drm;
@@ -354,7 +336,6 @@ int udl_drop_usb(struct drm_device *dev)
{
struct udl_device *udl = to_udl(dev);
- udl_kill_pending_urbs(dev);
udl_free_urb_list(dev);
put_device(udl->dmadev);
udl->dmadev = NULL;
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index b2377b706482..cbdda2d8b882 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -394,8 +394,6 @@ udl_simple_display_pipe_disable(struct
drm_simple_display_pipe *pipe)
struct urb *urb;
char *buf;
- udl_kill_pending_urbs(dev);
-
urb = udl_get_urb(dev);
if (!urb)
return;
--
2.35.3
[PATCH v3 01/12] drm/udl: Restore display mode on resume
Restore the display mode whne resuming from suspend. Currently, the
display remains dark.
On resume, the CRTC's mode does not change, but the 'active' flag
changes to 'true'. Taking this into account when considering a mode
switch restores the display mode.
The bug is reproducable by using Gnome with udl and observing the
adapter's suspend/resume behavior.
Actually, the whole check added in udl_simple_display_pipe_enable()
about the crtc_state->mode_changed was bogus. We should drop the
whole check and always apply the mode change in this function.
[ tiwai -- Drop the mode_changed check entirely instead, per Daniel's
suggestion ]
Fixes: 997d33c35618 ("drm/udl: Inline DPMS code into CRTC enable and disable
functions")
Cc:
Signed-off-by: Thomas Zimmermann
Suggested-by: Daniel Vetter
Reviewed-by: Daniel Vetter
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_modeset.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index 169110d8fc2e..34ce5b43c5db 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -382,9 +382,6 @@ udl_simple_display_pipe_enable(struct
drm_simple_display_pipe *pipe,
udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width,
fb->height);
- if (!crtc_state->mode_changed)
- return;
-
/* enable display */
udl_crtc_write_mode_to_hw(crtc);
}
--
2.35.3
[PATCH v3 06/12] drm/udl: Increase the default URB list size to 20
It seems that the current size (4) for the URB list is too small on some devices, and it resulted in the occasional stalls. Increase the default URB list size to 20 for working around it. Acked-by: Thomas Zimmermann Signed-off-by: Takashi Iwai --- drivers/gpu/drm/udl/udl_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c index 6aed6e0f669c..2b7eafd48ec2 100644 --- a/drivers/gpu/drm/udl/udl_main.c +++ b/drivers/gpu/drm/udl/udl_main.c @@ -20,7 +20,7 @@ #define NR_USB_REQUEST_CHANNEL 0x12 #define MAX_TRANSFER (PAGE_SIZE*16 - BULK_SIZE) -#define WRITES_IN_FLIGHT (4) +#define WRITES_IN_FLIGHT (20) #define MAX_VENDOR_DESCRIPTOR_SIZE 256 static int udl_parse_vendor_descriptor(struct udl_device *udl) -- 2.35.3
[PATCH v3 03/12] drm/udl: Enable damage clipping
From: Thomas Zimmermann Call drm_plane_enable_fb_damage_clips() and give userspace a chance of minimizing the updated display area. Signed-off-by: Thomas Zimmermann Signed-off-by: Takashi Iwai --- drivers/gpu/drm/udl/udl_modeset.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c index 34ce5b43c5db..b2377b706482 100644 --- a/drivers/gpu/drm/udl/udl_modeset.c +++ b/drivers/gpu/drm/udl/udl_modeset.c @@ -480,6 +480,7 @@ int udl_modeset_init(struct drm_device *dev) format_count, NULL, connector); if (ret) return ret; + drm_plane_enable_fb_damage_clips(&udl->display_pipe.plane); drm_mode_config_reset(dev); -- 2.35.3
[PATCH v3 09/12] drm/udl: Fix potential URB leaks
A couple of error handlings forgot to process the URB completion.
Those are both with WARN_ON() so should be visible, but we must fix
them in anyway.
Fixes: 7350b2a3fbc6 ("drm/udl: Replace BUG_ON() with WARN_ON()")
Acked-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_main.c | 8 +---
drivers/gpu/drm/udl/udl_transfer.c | 5 -
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 2b7eafd48ec2..de28eeff3155 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -260,11 +260,13 @@ int udl_submit_urb(struct drm_device *dev, struct urb
*urb, size_t len)
struct udl_device *udl = to_udl(dev);
int ret;
- if (WARN_ON(len > udl->urbs.size))
- return -EINVAL;
-
+ if (WARN_ON(len > udl->urbs.size)) {
+ ret = -EINVAL;
+ goto error;
+ }
urb->transfer_buffer_length = len; /* set to actual payload len */
ret = usb_submit_urb(urb, GFP_ATOMIC);
+ error:
if (ret) {
udl_urb_completion(urb); /* because no one else will */
DRM_ERROR("usb_submit_urb error %x\n", ret);
diff --git a/drivers/gpu/drm/udl/udl_transfer.c
b/drivers/gpu/drm/udl/udl_transfer.c
index a431208dda85..b57844632dbd 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -180,8 +180,11 @@ int udl_render_hline(struct drm_device *dev, int log_bpp,
struct urb **urb_ptr,
u8 *cmd = *urb_buf_ptr;
u8 *cmd_end = (u8 *) urb->transfer_buffer + urb->transfer_buffer_length;
- if (WARN_ON(!(log_bpp == 1 || log_bpp == 2)))
+ if (WARN_ON(!(log_bpp == 1 || log_bpp == 2))) {
+ /* need to finish URB at error from this function */
+ udl_urb_completion(urb);
return -EINVAL;
+ }
line_start = (u8 *) (front + byte_offset);
next_pixel = line_start;
--
2.35.3
[PATCH v3 11/12] drm/udl: Don't re-initialize stuff at retrying the URB list allocation
udl_alloc_urb_list() retires the allocation if there is no enough room
left, and it reinitializes the stuff unnecessarily such as the linked
list head and the waitqueue, which could be harmful. Those should be
outside the retry loop.
Acked-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_main.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 16aa4a655e7f..829edb60a987 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -182,15 +182,14 @@ static int udl_alloc_urb_list(struct drm_device *dev, int
count, size_t size)
struct usb_device *udev = udl_to_usb_device(udl);
spin_lock_init(&udl->urbs.lock);
-
-retry:
- udl->urbs.size = size;
INIT_LIST_HEAD(&udl->urbs.list);
-
init_waitqueue_head(&udl->urbs.sleep);
udl->urbs.count = 0;
udl->urbs.available = 0;
+retry:
+ udl->urbs.size = size;
+
while (udl->urbs.count * size < wanted_size) {
unode = kzalloc(sizeof(struct urb_node), GFP_KERNEL);
if (!unode)
--
2.35.3
[PATCH v3 10/12] drm/udl: Fix inconsistent urbs.count value during udl_free_urb_list()
In the current design, udl_get_urb() may be called asynchronously
during the driver freeing its URL list via udl_free_urb_list().
The problem is that the sync is determined by comparing the urbs.count
and urbs.available fields, while we clear urbs.count field only once
after udl_free_urb_list() finishes, i.e. during udl_free_urb_list(),
the state becomes inconsistent.
For fixing this inconsistency and also for hardening the locking
scheme, this patch does a slight refactoring of the code around
udl_get_urb() and udl_free_urb_list(). Now urbs.count is updated in
the same spinlock at extracting a URB from the list in
udl_free_url_list().
Acked-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.h | 8 +--
drivers/gpu/drm/udl/udl_main.c | 42 +++---
2 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 5923d2e02bc8..d943684b5bbb 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -74,13 +74,7 @@ static inline struct usb_device *udl_to_usb_device(struct
udl_device *udl)
int udl_modeset_init(struct drm_device *dev);
struct drm_connector *udl_connector_init(struct drm_device *dev);
-struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout);
-
-#define GET_URB_TIMEOUTHZ
-static inline struct urb *udl_get_urb(struct drm_device *dev)
-{
- return udl_get_urb_timeout(dev, GET_URB_TIMEOUT);
-}
+struct urb *udl_get_urb(struct drm_device *dev);
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
int udl_sync_pending_urbs(struct drm_device *dev);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index de28eeff3155..16aa4a655e7f 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -23,6 +23,8 @@
#define WRITES_IN_FLIGHT (20)
#define MAX_VENDOR_DESCRIPTOR_SIZE 256
+static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout);
+
static int udl_parse_vendor_descriptor(struct udl_device *udl)
{
struct usb_device *udev = udl_to_usb_device(udl);
@@ -146,15 +148,17 @@ void udl_urb_completion(struct urb *urb)
static void udl_free_urb_list(struct drm_device *dev)
{
struct udl_device *udl = to_udl(dev);
- int count = udl->urbs.count;
struct urb_node *unode;
struct urb *urb;
DRM_DEBUG("Waiting for completes and freeing all render urbs\n");
/* keep waiting and freeing, until we've got 'em all */
- while (count--) {
- urb = udl_get_urb_timeout(dev, MAX_SCHEDULE_TIMEOUT);
+ while (udl->urbs.count) {
+ spin_lock_irq(&udl->urbs.lock);
+ urb = udl_get_urb_locked(udl, MAX_SCHEDULE_TIMEOUT);
+ udl->urbs.count--;
+ spin_unlock_irq(&udl->urbs.lock);
if (WARN_ON(!urb))
break;
unode = urb->context;
@@ -164,7 +168,8 @@ static void udl_free_urb_list(struct drm_device *dev)
usb_free_urb(urb);
kfree(unode);
}
- udl->urbs.count = 0;
+
+ wake_up_all(&udl->urbs.sleep);
}
static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
@@ -228,33 +233,44 @@ static int udl_alloc_urb_list(struct drm_device *dev, int
count, size_t size)
return udl->urbs.count;
}
-struct urb *udl_get_urb_timeout(struct drm_device *dev, long timeout)
+static struct urb *udl_get_urb_locked(struct udl_device *udl, long timeout)
{
- struct udl_device *udl = to_udl(dev);
- struct urb_node *unode = NULL;
+ struct urb_node *unode;
- if (!udl->urbs.count)
- return NULL;
+ assert_spin_locked(&udl->urbs.lock);
/* Wait for an in-flight buffer to complete and get re-queued */
- spin_lock_irq(&udl->urbs.lock);
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
+!udl->urbs.count ||
!list_empty(&udl->urbs.list),
udl->urbs.lock, timeout)) {
DRM_INFO("wait for urb interrupted: available: %d\n",
udl->urbs.available);
- goto unlock;
+ return NULL;
}
+ if (!udl->urbs.count)
+ return NULL;
+
unode = list_first_entry(&udl->urbs.list, struct urb_node, entry);
list_del_init(&unode->entry);
udl->urbs.available--;
-unlock:
- spin_unlock_irq(&udl->urbs.lock);
return unode ? unode->urb : NULL;
}
+#define GET_URB_TIMEOUTHZ
+struct urb *udl_get_urb(struct drm_device *dev)
+{
+ struct udl_device *udl = to_udl(dev);
+ struct urb *urb;
+
[PATCH v3 08/12] drm/udl: Pass rectangle directly to udl_handle_damage()
Just for some code simplification.
Suggested-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_modeset.c | 20 +---
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index c9b837ac26a7..0142fc6a478a 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -244,14 +244,13 @@ static long udl_log_cpp(unsigned int cpp)
static int udl_handle_damage(struct drm_framebuffer *fb,
const struct iosys_map *map,
-int x, int y, int width, int height)
+struct drm_rect *clip)
{
struct drm_device *dev = fb->dev;
void *vaddr = map->vaddr; /* TODO: Use mapping abstraction properly */
int i, ret;
char *cmd;
struct urb *urb;
- struct drm_rect clip;
int log_bpp;
ret = udl_log_cpp(fb->format->cpp[0]);
@@ -259,8 +258,6 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
return ret;
log_bpp = ret;
- drm_rect_init(&clip, x, y, width, height);
-
ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
if (ret)
return ret;
@@ -272,11 +269,11 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
}
cmd = urb->transfer_buffer;
- for (i = clip.y1; i < clip.y2; i++) {
+ for (i = clip->y1; i < clip->y2; i++) {
const int line_offset = fb->pitches[0] * i;
- const int byte_offset = line_offset + (clip.x1 << log_bpp);
- const int dev_byte_offset = (fb->width * i + clip.x1) <<
log_bpp;
- const int byte_width = (clip.x2 - clip.x1) << log_bpp;
+ const int byte_offset = line_offset + (clip->x1 << log_bpp);
+ const int dev_byte_offset = (fb->width * i + clip->x1) <<
log_bpp;
+ const int byte_width = (clip->x2 - clip->x1) << log_bpp;
ret = udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
&cmd, byte_offset, dev_byte_offset,
byte_width);
@@ -329,6 +326,7 @@ udl_simple_display_pipe_enable(struct
drm_simple_display_pipe *pipe,
struct udl_device *udl = to_udl(dev);
struct drm_display_mode *mode = &crtc_state->mode;
struct drm_shadow_plane_state *shadow_plane_state =
to_drm_shadow_plane_state(plane_state);
+ struct drm_rect clip;
char *buf;
char *wrptr;
int color_depth = UDL_COLOR_DEPTH_16BPP;
@@ -354,7 +352,8 @@ udl_simple_display_pipe_enable(struct
drm_simple_display_pipe *pipe,
udl->mode_buf_len = wrptr - buf;
- udl_handle_damage(fb, &shadow_plane_state->data[0], 0, 0, fb->width,
fb->height);
+ drm_rect_init(&clip, 0, 0, fb->width, fb->height);
+ udl_handle_damage(fb, &shadow_plane_state->data[0], &clip);
/* enable display */
udl_crtc_write_mode_to_hw(crtc);
@@ -396,8 +395,7 @@ udl_simple_display_pipe_update(struct
drm_simple_display_pipe *pipe,
return;
if (drm_atomic_helper_damage_merged(old_plane_state, state, &rect))
- udl_handle_damage(fb, &shadow_plane_state->data[0], rect.x1,
rect.y1,
- rect.x2 - rect.x1, rect.y2 - rect.y1);
+ udl_handle_damage(fb, &shadow_plane_state->data[0], &rect);
}
static const struct drm_simple_display_pipe_funcs
udl_simple_display_pipe_funcs = {
--
2.35.3
[PATCH v3 05/12] drm/udl: Suppress error print for -EPROTO at URB completion
The driver may receive -EPROTO at the URB completion when the device
gets disconnected, and it's a normal situation. Suppress the error
print for that, too.
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_main.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index a9f6b710b254..6aed6e0f669c 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -126,6 +126,7 @@ void udl_urb_completion(struct urb *urb)
if (urb->status) {
if (!(urb->status == -ENOENT ||
urb->status == -ECONNRESET ||
+ urb->status == -EPROTO ||
urb->status == -ESHUTDOWN)) {
DRM_ERROR("%s - nonzero write bulk status received:
%d\n",
__func__, urb->status);
--
2.35.3
[PATCH v3 12/12] drm/udl: Sync pending URBs at the end of suspend
It's better to perform the sync at the very last of the suspend
instead of the pipe-disable function, so that we can catch all pending
URBs (if any).
While we're at it, drop the error code from udl_sync_pending_urb()
since we basically ignore it; instead, give a clear error message
indicating a problem.
Acked-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_drv.c | 8 +++-
drivers/gpu/drm/udl/udl_drv.h | 2 +-
drivers/gpu/drm/udl/udl_main.c| 6 ++
drivers/gpu/drm/udl/udl_modeset.c | 2 --
4 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 0ba88e5472a9..91effdcefb6d 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -21,8 +21,14 @@ static int udl_usb_suspend(struct usb_interface *interface,
pm_message_t message)
{
struct drm_device *dev = usb_get_intfdata(interface);
+ int ret;
- return drm_mode_config_helper_suspend(dev);
+ ret = drm_mode_config_helper_suspend(dev);
+ if (ret)
+ return ret;
+
+ udl_sync_pending_urbs(dev);
+ return 0;
}
static int udl_usb_resume(struct usb_interface *interface)
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index d943684b5bbb..b4cc7cc568c7 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -77,7 +77,7 @@ struct drm_connector *udl_connector_init(struct drm_device
*dev);
struct urb *udl_get_urb(struct drm_device *dev);
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len);
-int udl_sync_pending_urbs(struct drm_device *dev);
+void udl_sync_pending_urbs(struct drm_device *dev);
void udl_urb_completion(struct urb *urb);
int udl_init(struct udl_device *udl);
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 829edb60a987..061cb88c08a2 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -290,10 +290,9 @@ int udl_submit_urb(struct drm_device *dev, struct urb
*urb, size_t len)
}
/* wait until all pending URBs have been processed */
-int udl_sync_pending_urbs(struct drm_device *dev)
+void udl_sync_pending_urbs(struct drm_device *dev)
{
struct udl_device *udl = to_udl(dev);
- int ret = 0;
spin_lock_irq(&udl->urbs.lock);
/* 2 seconds as a sane timeout */
@@ -301,9 +300,8 @@ int udl_sync_pending_urbs(struct drm_device *dev)
udl->urbs.available == udl->urbs.count,
udl->urbs.lock,
msecs_to_jiffies(2000)))
- ret = -ETIMEDOUT;
+ drm_err(dev, "Timeout for syncing pending URBs\n");
spin_unlock_irq(&udl->urbs.lock);
- return ret;
}
int udl_init(struct udl_device *udl)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index 0142fc6a478a..d4f409f6d533 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -378,8 +378,6 @@ udl_simple_display_pipe_disable(struct
drm_simple_display_pipe *pipe)
buf = udl_dummy_render(buf);
udl_submit_urb(dev, urb, buf - (char *)urb->transfer_buffer);
-
- udl_sync_pending_urbs(dev);
}
static void
--
2.35.3
[PATCH v3 07/12] drm/udl: Drop unneeded alignment
The alignment of damaged area was needed for the original udlfb driver
that tried to trim the superfluous copies between front and backend
buffers and handle data in long int. It's not the case for udl DRM
driver, hence we can omit the whole unneeded alignment, as well as the
dead code.
Acked-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_modeset.c | 28 +
drivers/gpu/drm/udl/udl_transfer.c | 40 --
2 files changed, 1 insertion(+), 67 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index cbdda2d8b882..c9b837ac26a7 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -242,28 +242,6 @@ static long udl_log_cpp(unsigned int cpp)
return __ffs(cpp);
}
-static int udl_aligned_damage_clip(struct drm_rect *clip, int x, int y,
- int width, int height)
-{
- int x1, x2;
-
- if (WARN_ON_ONCE(x < 0) ||
- WARN_ON_ONCE(y < 0) ||
- WARN_ON_ONCE(width < 0) ||
- WARN_ON_ONCE(height < 0))
- return -EINVAL;
-
- x1 = ALIGN_DOWN(x, sizeof(unsigned long));
- x2 = ALIGN(width + (x - x1), sizeof(unsigned long)) + x1;
-
- clip->x1 = x1;
- clip->y1 = y;
- clip->x2 = x2;
- clip->y2 = y + height;
-
- return 0;
-}
-
static int udl_handle_damage(struct drm_framebuffer *fb,
const struct iosys_map *map,
int x, int y, int width, int height)
@@ -281,11 +259,7 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
return ret;
log_bpp = ret;
- ret = udl_aligned_damage_clip(&clip, x, y, width, height);
- if (ret)
- return ret;
- else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
- return -EINVAL;
+ drm_rect_init(&clip, x, y, width, height);
ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
if (ret)
diff --git a/drivers/gpu/drm/udl/udl_transfer.c
b/drivers/gpu/drm/udl/udl_transfer.c
index 176ef2a6a731..a431208dda85 100644
--- a/drivers/gpu/drm/udl/udl_transfer.c
+++ b/drivers/gpu/drm/udl/udl_transfer.c
@@ -25,46 +25,6 @@
#define MIN_RAW_PIX_BYTES 2
#define MIN_RAW_CMD_BYTES (RAW_HEADER_BYTES + MIN_RAW_PIX_BYTES)
-/*
- * Trims identical data from front and back of line
- * Sets new front buffer address and width
- * And returns byte count of identical pixels
- * Assumes CPU natural alignment (unsigned long)
- * for back and front buffer ptrs and width
- */
-#if 0
-static int udl_trim_hline(const u8 *bback, const u8 **bfront, int *width_bytes)
-{
- int j, k;
- const unsigned long *back = (const unsigned long *) bback;
- const unsigned long *front = (const unsigned long *) *bfront;
- const int width = *width_bytes / sizeof(unsigned long);
- int identical = width;
- int start = width;
- int end = width;
-
- for (j = 0; j < width; j++) {
- if (back[j] != front[j]) {
- start = j;
- break;
- }
- }
-
- for (k = width - 1; k > j; k--) {
- if (back[k] != front[k]) {
- end = k+1;
- break;
- }
- }
-
- identical = start + (width - end);
- *bfront = (u8 *) &front[start];
- *width_bytes = (end - start) * sizeof(unsigned long);
-
- return identical * sizeof(unsigned long);
-}
-#endif
-
static inline u16 pixel32_to_be16(const uint32_t pixel)
{
return (((pixel >> 3) & 0x001f) |
--
2.35.3
Re: [PATCH v3 08/12] drm/udl: Pass rectangle directly to udl_handle_damage()
On Thu, 08 Sep 2022 14:47:52 +0200,
Thomas Zimmermann wrote:
>
> Hi
>
> Am 08.09.22 um 11:51 schrieb Takashi Iwai:
> > Just for some code simplification.
> >
> > Suggested-by: Thomas Zimmermann
> > Signed-off-by: Takashi Iwai
>
> With my comments fixed, you can add
>
> Acked-by: Thomas Zimmermann
>
> > ---
> > drivers/gpu/drm/udl/udl_modeset.c | 20 +---
> > 1 file changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/udl/udl_modeset.c
> > b/drivers/gpu/drm/udl/udl_modeset.c
> > index c9b837ac26a7..0142fc6a478a 100644
> > --- a/drivers/gpu/drm/udl/udl_modeset.c
> > +++ b/drivers/gpu/drm/udl/udl_modeset.c
> > @@ -244,14 +244,13 @@ static long udl_log_cpp(unsigned int cpp)
> > static int udl_handle_damage(struct drm_framebuffer *fb,
> > const struct iosys_map *map,
> > -int x, int y, int width, int height)
> > +struct drm_rect *clip)
>
> Should probably be declared const.
>
> > {
> > struct drm_device *dev = fb->dev;
> > void *vaddr = map->vaddr; /* TODO: Use mapping abstraction properly */
> > int i, ret;
> > char *cmd;
> > struct urb *urb;
> > - struct drm_rect clip;
> > int log_bpp;
> > ret = udl_log_cpp(fb->format->cpp[0]);
> > @@ -259,8 +258,6 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
> > return ret;
> > log_bpp = ret;
> > - drm_rect_init(&clip, x, y, width, height);
> > -
> > ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> > if (ret)
> > return ret;
> > @@ -272,11 +269,11 @@ static int udl_handle_damage(struct drm_framebuffer
> > *fb,
> > }
> > cmd = urb->transfer_buffer;
> > - for (i = clip.y1; i < clip.y2; i++) {
> > + for (i = clip->y1; i < clip->y2; i++) {
> > const int line_offset = fb->pitches[0] * i;
> > - const int byte_offset = line_offset + (clip.x1 << log_bpp);
> > - const int dev_byte_offset = (fb->width * i + clip.x1) <<
> > log_bpp;
> > - const int byte_width = (clip.x2 - clip.x1) << log_bpp;
> > + const int byte_offset = line_offset + (clip->x1 << log_bpp);
> > + const int dev_byte_offset = (fb->width * i + clip->x1) <<
> > log_bpp;
> > + const int byte_width = (clip->x2 - clip->x1) << log_bpp;
>
> Please use drm_rect_width(clip) instead. Somehow there's already too
> much code that open-codes this.
>
> > ret = udl_render_hline(dev, log_bpp, &urb, (char *)vaddr,
> >&cmd, byte_offset, dev_byte_offset,
> >byte_width);
> > @@ -329,6 +326,7 @@ udl_simple_display_pipe_enable(struct
> > drm_simple_display_pipe *pipe,
> > struct udl_device *udl = to_udl(dev);
> > struct drm_display_mode *mode = &crtc_state->mode;
> > struct drm_shadow_plane_state *shadow_plane_state =
> > to_drm_shadow_plane_state(plane_state);
> > + struct drm_rect clip;
>
> Better do a static init with DRM_RECT_INIT(0, 0, fb->width,
> fb->height) and remove the other init call below.
OK, below is the revised patch.
Do you want me a full respin for v4?
Takashi
-- 8< --
From: Takashi Iwai
Subject: [PATCH] drm/udl: Pass rectangle directly to udl_handle_damage()
Just for some code simplification.
Suggested-by: Thomas Zimmermann
Acked-by: Thomas Zimmermann
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/udl/udl_modeset.c | 19 ---
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_modeset.c
b/drivers/gpu/drm/udl/udl_modeset.c
index c9b837ac26a7..d5e20bf144bc 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -244,14 +244,13 @@ static long udl_log_cpp(unsigned int cpp)
static int udl_handle_damage(struct drm_framebuffer *fb,
const struct iosys_map *map,
-int x, int y, int width, int height)
+const struct drm_rect *clip)
{
struct drm_device *dev = fb->dev;
void *vaddr = map->vaddr; /* TODO: Use mapping abstraction properly */
int i, ret;
char *cmd;
struct urb *urb;
- struct drm_rect clip;
int log_bpp;
ret = udl_log_cpp(fb->format->cpp[0]);
@@ -259,8 +258,6 @@ static int udl_handle_damage(struct drm_framebuffer *fb,
Re: [PATCH v3 00/12] drm/udl: More fixes
On Mon, 12 Sep 2022 09:06:47 +0200, Thomas Zimmermann wrote: > > Hi, > > I've meanwhile merged the patchset, including the one updated patch > and the missing r-b. Great, thanks! Takashi
Re: [PATCH] drm: Fix EDID firmware load on resume
On Wed, 27 Jul 2022 09:41:52 +0200,
Matthieu CHARETTE wrote:
>
> Loading an EDID using drm.edid_firmware parameter makes resume to fail
> after firmware cache is being cleaned. This is because edid_load() use a
> temporary device to request the firmware. This cause the EDID firmware
> not to be cached from suspend. And, requesting the EDID firmware return
> an error during resume.
> So the request_firmware() call should use a permanent device for each
> connector. Also, we should cache the EDID even if no monitor is
> connected, in case it's plugged while suspended.
>
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2061
> Signed-off-by: Matthieu CHARETTE
Can we simply cache the already loaded EDID bytes instead?
Something like below (totally untested).
thanks,
Takashi
-- 8< --
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 1c48d162c77e..b9d2803b518b 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -286,6 +286,7 @@ int drm_connector_init(struct drm_device *dev,
connector->status = connector_status_unknown;
connector->display_info.panel_orientation =
DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+ connector->firmware_edid = NULL;
drm_connector_get_cmdline_mode(connector);
@@ -485,6 +486,7 @@ void drm_connector_cleanup(struct drm_connector *connector)
ida_simple_remove(&dev->mode_config.connector_ida,
connector->index);
+ kfree(connector->firmware_edid);
kfree(connector->display_info.bus_formats);
drm_mode_object_unregister(dev, &connector->base);
kfree(connector->name);
diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
index 37d8ba3ddb46..a38fe4e00e4a 100644
--- a/drivers/gpu/drm/drm_edid_load.c
+++ b/drivers/gpu/drm/drm_edid_load.c
@@ -253,6 +253,13 @@ static void *edid_load(struct drm_connector *connector,
const char *name,
edid = new_edid;
}
+ connector->firmware_edid = drm_edid_duplicate((struct edid *)edid);
+ if (!connector->firmware_edid) {
+ kfree(edid);
+ edid = ERR_PTR(-ENOMEM);
+ goto out;
+ }
+
DRM_INFO("Got %s EDID base block and %d extension%s from "
"\"%s\" for connector \"%s\"\n", (builtin >= 0) ? "built-in" :
"external", valid_extensions, valid_extensions == 1 ? "" : "s",
@@ -269,6 +276,12 @@ struct edid *drm_load_edid_firmware(struct drm_connector
*connector)
char *edidname, *last, *colon, *fwstr, *edidstr, *fallback = NULL;
struct edid *edid;
+ /* already loaded? */
+ if (connector->firmware_edid) {
+ edid = drm_edid_duplicate(connector->firmware_edid);
+ return edid ? edid : ERR_PTR(-ENOMEM);
+ }
+
if (edid_firmware[0] == '\0')
return ERR_PTR(-ENOENT);
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 3ac4bf87f257..b5d0c87327a3 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1528,6 +1528,8 @@ struct drm_connector {
enum drm_connector_force force;
/** @override_edid: has the EDID been overwritten through debugfs for
testing? */
bool override_edid;
+ /** @firmware_edid: the cached firmware EDID bytes */
+ struct edid *firmware_edid;
/** @epoch_counter: used to detect any other changes in connector,
besides status */
u64 epoch_counter;
Incorrect buffer handling in dw-hdmi bridge audio
Hi,
while recently working on the ALSA memory allocator API cleanup, I
noticed that dw-hdmi bridge driver seems doing weird about the buffer
management. It pre-allocates the usual device buffers fully at the
probe time, while each stream allocates the buffer via the vmalloc
helpers and replaces with it at each open.
I guess it's no expected behavior? It's basically a full waste of
resources, and the vmalloc buffer isn't guaranteed to work well for
mmap on every architecture.
Below is the patch to address it. Can anyone check whether this still
works?
Since I have a cleanup series and this is involved, I'd like to take
the fix through my tree once after it's confirmed (and get ACK if
possible).
Thanks!
Takashi
-- 8< --
From: Takashi Iwai
Subject: [PATCH] drm/bridge: dw-hdmi: Fix the incorrect buffer allocations
The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV,
while it re-allocates and releases vmalloc pages. This is not only a
lot of waste resources but also causes the mmap malfunction.
Change / drop the vmalloc-related code and use the standard buffer
allocation / release code instead.
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
index 2b7539701b42..8fe7a6e8ff94 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c
@@ -384,15 +384,14 @@ static int dw_hdmi_close(struct snd_pcm_substream
*substream)
static int dw_hdmi_hw_free(struct snd_pcm_substream *substream)
{
- return snd_pcm_lib_free_vmalloc_buffer(substream);
+ return snd_pcm_lib_free_pages(substream);
}
static int dw_hdmi_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
{
/* Allocate the PCM runtime buffer, which is exposed to userspace. */
- return snd_pcm_lib_alloc_vmalloc_buffer(substream,
- params_buffer_bytes(params));
+ return snd_pcm_lib_malloc_pages(substream, params_buffer_bytes(params));
}
static int dw_hdmi_prepare(struct snd_pcm_substream *substream)
@@ -511,7 +510,6 @@ static const struct snd_pcm_ops snd_dw_hdmi_ops = {
.prepare = dw_hdmi_prepare,
.trigger = dw_hdmi_trigger,
.pointer = dw_hdmi_pointer,
- .page = snd_pcm_lib_get_vmalloc_page,
};
static int snd_dw_hdmi_probe(struct platform_device *pdev)
--
2.16.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Incorrect buffer handling in dw-hdmi bridge audio
On Tue, 05 Nov 2019 17:02:15 +0100, Russell King - ARM Linux admin wrote: > > On Tue, Nov 05, 2019 at 09:07:43AM +0100, Neil Armstrong wrote: > > Hi, > > > > On 05/11/2019 08:55, Takashi Iwai wrote: > > > Hi, > > > > > > while recently working on the ALSA memory allocator API cleanup, I > > > noticed that dw-hdmi bridge driver seems doing weird about the buffer > > > management. It pre-allocates the usual device buffers fully at the > > > probe time, while each stream allocates the buffer via the vmalloc > > > helpers and replaces with it at each open. > > > > > > I guess it's no expected behavior? It's basically a full waste of > > > resources, and the vmalloc buffer isn't guaranteed to work well for > > > mmap on every architecture. > > > > > > Below is the patch to address it. Can anyone check whether this still > > > works? > > > > I don't have the setup to check, but this has been pushed by Russell I > > Added in CC. > > > > I also added the imx maintainer since it's (AFAIK) only used on iMX SoCs. > > > > Neil > > > > > > > > Since I have a cleanup series and this is involved, I'd like to take > > > the fix through my tree once after it's confirmed (and get ACK if > > > possible). > > > > > > > > > Thanks! > > > > > > Takashi > > > > > > -- 8< -- > > > From: Takashi Iwai > > > Subject: [PATCH] drm/bridge: dw-hdmi: Fix the incorrect buffer allocations > > > > > > The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV, > > > while it re-allocates and releases vmalloc pages. This is not only a > > > lot of waste resources but also causes the mmap malfunction. > > > > > > Change / drop the vmalloc-related code and use the standard buffer > > > allocation / release code instead. > > I think getting rid of the vmalloc code here is a mistake - I seem to > remember using the standard buffer allocation causes failures, due to > memory fragmentation. Since the hardware is limited to DMA from at > most one page, there is no reason for this driver to require contiguous > pages, hence why it's using - and should use - vmalloc pages. vmalloc > is way kinder to the MM subsystem than trying to request large order > contiguous pages. > > So, NAK on this patch. OK, then we should do other way round, rather drop the buffer preallocation instead. Currently vmalloc buffer is always allocated at each open and overrides the preallocated buffer, so the whole 64k and more are wasted for no use. (BTW, the current code has this snippet: /* Limit the buffer size to the size of the preallocated buffer */ ret = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_SIZE, 0, substream->dma_buffer.bytes); ... and this would have to limit the buffer size only to the preallocated size -- which essentially makes the argument above invalid. However, this check looks actually bogus, the constraint parameter should be SNDRV_PCM_HW_PARAM_BUFFER_BYTES, not _SIZE. It might be the reason it worked somehow...) So below is the revised patch. Could you guys check it again? There I copied the comment as is, although the 512k mentioned there looks inconsistent with the actual code. Should it be 1M? Thanks! Takashi -- 8< -- From: Takashi Iwai Subject: [PATCH] drm/bridge: dw-hdmi: A couple of fixes wrt buffer allocation The driver sets up the buffer preallocation with SNDRV_DMA_TYPE_DEV, while it always re-allocates and releases vmalloc pages, hence the preallocated pages are never used. Also, the current code contains the hw constraint to limit the buffer size to the preallocated size. It doesn't work as expected, however, because it's applying to an incorrect parameter (SNDRV_PCM_HW_PARAM_BUFFER_SIZE instead of _BUFFER_BYTES). This patch tries to address these issues: drop the unnecessary buffer preallocation and fix the hw constraint to the right parameter. Since the buffer preallocation is dropped, the max buffer size that was passed to the preallocation is passed directly now to the hw constraint call. Signed-off-by: Takashi Iwai --- drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 17 +++-- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c index 2b7539701b42..3efbbc59994b 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-h
