Re: [bug-report] task info hung problem in fb_deferred_io_work()
On Fri, 19 Apr 2024 09:39:09 +0200, Harshit Mogalapalli wrote: > > Hi Takashi, > > On 19/04/24 12:14, Takashi Iwai wrote: > > On Thu, 18 Apr 2024 21:29:57 +0200, > > Helge Deller wrote: > >> > >> On 4/18/24 16:26, Takashi Iwai wrote: > >>> On Thu, 18 Apr 2024 16:06:52 +0200, > >>> Nam Cao wrote: > >>>> > >>>> On 2024-04-18 Harshit Mogalapalli wrote: > >>>>> While fuzzing 5.15.y kernel with Syzkaller, we noticed a INFO: task hung > >>>>> bug in fb_deferred_io_work() > >>>> > >>>> Which framebuffer device are you using exactly? It is possible that > >>>> the problem is with the device driver, not core framebuffer. > >>> > >>> Note that it was already known that using flush_delayed_work() caused > >>> a problem. See the thread of the fix patch: > >>> https://lore.kernel.org/all/20230129082856.22113-1-ti...@suse.de/ > >> > >> Harshit reported the hung tasks with kernel v5.15-stable, and can even > >> reproduce > >> that issue with kernel v6.9-rc4 although it has all of your patches from > >> that referenced mail thread applied. > >> So, what does your statement that "it was already known that it causes > >> problems" exactly mean? > >> Can it be fixed? Is someone looking into fixing it? > > > > My original fix was intentionally with cancel_delayed_work_sync() > > because flush_delayed_work() didn't work. We knew that it'd miss some > > last-minute queued change, but it's better than crash, so it was > > applied in that way. > > > > Thanks for sharing these details. > > > Then later on, the commit 33cd6ea9c067 changed cancel_*() to > > flush_delayed_work() blindly, and the known problem resurfaced again. > > > > I have reverted that commit, but still could see some other task hung > message as shared here on other reply: > > https://lore.kernel.org/all/d2485cb9-277d-4b8e-9794-02f1efaba...@oracle.com/ Yes, then it could be a different cause, I suppose. The crash with flush_delayed_work() was a real crash, no hanging task, IIRC. Can you reproduce the issue with the latest Linus upstream, too? thanks, Takashi
Re: [bug-report] task info hung problem in fb_deferred_io_work()
On Thu, 18 Apr 2024 21:29:57 +0200, Helge Deller wrote: > > On 4/18/24 16:26, Takashi Iwai wrote: > > On Thu, 18 Apr 2024 16:06:52 +0200, > > Nam Cao wrote: > >> > >> On 2024-04-18 Harshit Mogalapalli wrote: > >>> While fuzzing 5.15.y kernel with Syzkaller, we noticed a INFO: task hung > >>> bug in fb_deferred_io_work() > >> > >> Which framebuffer device are you using exactly? It is possible that > >> the problem is with the device driver, not core framebuffer. > > > > Note that it was already known that using flush_delayed_work() caused > > a problem. See the thread of the fix patch: > >https://lore.kernel.org/all/20230129082856.22113-1-ti...@suse.de/ > > Harshit reported the hung tasks with kernel v5.15-stable, and can even > reproduce > that issue with kernel v6.9-rc4 although it has all of your patches from > that referenced mail thread applied. > So, what does your statement that "it was already known that it causes > problems" exactly mean? > Can it be fixed? Is someone looking into fixing it? My original fix was intentionally with cancel_delayed_work_sync() because flush_delayed_work() didn't work. We knew that it'd miss some last-minute queued change, but it's better than crash, so it was applied in that way. Then later on, the commit 33cd6ea9c067 changed cancel_*() to flush_delayed_work() blindly, and the known problem resurfaced again. Takashi
Re: [bug-report] task info hung problem in fb_deferred_io_work()
On Thu, 18 Apr 2024 16:06:52 +0200, Nam Cao wrote: > > On 2024-04-18 Harshit Mogalapalli wrote: > > While fuzzing 5.15.y kernel with Syzkaller, we noticed a INFO: task hung > > bug in fb_deferred_io_work() > > Which framebuffer device are you using exactly? It is possible that > the problem is with the device driver, not core framebuffer. Note that it was already known that using flush_delayed_work() caused a problem. See the thread of the fix patch: https://lore.kernel.org/all/20230129082856.22113-1-ti...@suse.de/ BTW, the problem is seen with bochs drm. Takashi
Re: [PATCH 2/5] ALSA: hda/intel: Use pci_get_base_class() to reduce duplicated code
On Fri, 25 Aug 2023 08:27:11 +0200,
Sui Jingfeng wrote:
>
> From: Sui Jingfeng
>
> Should be no functional change
>
> Cc: Jaroslav Kysela
> Cc: Takashi Iwai
> Cc: Fred Oh
> Cc: Pierre-Louis Bossart
> Cc: Kai Vehmanen
> Cc: Bjorn Helgaas
> Signed-off-by: Sui Jingfeng
> ---
> sound/pci/hda/hda_intel.c | 16 +---
> 1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index a21b61ad08d1..811a149584f2 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -1429,17 +1429,11 @@ static bool atpx_present(void)
> acpi_handle dhandle, atpx_handle;
> acpi_status status;
>
> - while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) !=
> NULL) {
> - dhandle = ACPI_HANDLE(>dev);
> - if (dhandle) {
> - status = acpi_get_handle(dhandle, "ATPX", _handle);
> - if (ACPI_SUCCESS(status)) {
> - pci_dev_put(pdev);
> - return true;
> - }
> - }
> - }
> - while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_OTHER << 8, pdev)) !=
> NULL) {
> + while ((pdev = pci_get_base_class(PCI_BASE_CLASS_DISPLAY, pdev))) {
> + if ((pdev->class != PCI_CLASS_DISPLAY_VGA << 8) &&
> + (pdev->class != PCI_CLASS_DISPLAY_OTHER << 8))
> + continue;
> +
> dhandle = ACPI_HANDLE(>dev);
> if (dhandle) {
> status = acpi_get_handle(dhandle, "ATPX", _handle);
Reviewed-by: Takashi Iwai
thanks,
Takashi
Re: [PATCH] drm/nouveau/disp: fix use-after-free in error handling of nouveau_connector_create
On Wed, 16 Aug 2023 12:14:24 +0200, Borislav Petkov wrote: > > On Wed, Aug 16, 2023 at 12:11:57PM +0200, Borislav Petkov wrote: > > Does that help? > > Btw, note that this is *plain* -rc5, without your patch. The UAF looks very same as I had and that's the bug Karol's patch should address. So more interesting would be the result with the patch :) Takashi
Re: 2b5d1c29f6c4 ("drm/nouveau/disp: PIOR DP uses GPIO for HPD, not PMGR AUX interrupts")
On Mon, 14 Aug 2023 17:06:02 +0200, Takashi Iwai wrote: > > On Mon, 14 Aug 2023 16:51:08 +0200, > Karol Herbst wrote: > > > > I've sent a patch out to address this memory corruption > > https://patchwork.freedesktop.org/patch/552642/ > > > > It might or might not fix regressions from the original I2C fix, so > > please test and report if there are remaining issues. > > Thanks! I'll build a test kernel and ask the reporter for testing > with it. Let's cross fingers :) The feedback is positive, so far. It seems fixing the regression reported for 6.4.8 kernel. thanks, Takashi
Re: 2b5d1c29f6c4 ("drm/nouveau/disp: PIOR DP uses GPIO for HPD, not PMGR AUX interrupts")
On Mon, 14 Aug 2023 16:51:08 +0200, Karol Herbst wrote: > > I've sent a patch out to address this memory corruption > https://patchwork.freedesktop.org/patch/552642/ > > It might or might not fix regressions from the original I2C fix, so > please test and report if there are remaining issues. Thanks! I'll build a test kernel and ask the reporter for testing with it. Let's cross fingers :) Takashi
Re: 2b5d1c29f6c4 ("drm/nouveau/disp: PIOR DP uses GPIO for HPD, not PMGR AUX interrupts")
On Mon, 14 Aug 2023 15:19:11 +0200, Karol Herbst wrote: > > On Mon, Aug 14, 2023 at 2:56 PM Karol Herbst wrote: > > > > On Mon, Aug 14, 2023 at 2:48 PM Takashi Iwai wrote: > > > > > > On Mon, 14 Aug 2023 14:38:18 +0200, > > > Karol Herbst wrote: > > > > > > > > On Wed, Aug 9, 2023 at 6:16 PM Takashi Iwai wrote: > > > > > > > > > > On Wed, 09 Aug 2023 16:46:38 +0200, > > > > > Takashi Iwai wrote: > > > > > > > > > > > > On Wed, 09 Aug 2023 15:13:23 +0200, > > > > > > Takashi Iwai wrote: > > > > > > > > > > > > > > On Wed, 09 Aug 2023 14:19:23 +0200, > > > > > > > Karol Herbst wrote: > > > > > > > > > > > > > > > > On Wed, Aug 9, 2023 at 1:46 PM Takashi Iwai > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Wed, 09 Aug 2023 13:42:09 +0200, > > > > > > > > > Karol Herbst wrote: > > > > > > > > > > > > > > > > > > > > On Wed, Aug 9, 2023 at 11:22 AM Takashi Iwai > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > On Tue, 08 Aug 2023 12:39:32 +0200, > > > > > > > > > > > Karol Herbst wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Aug 7, 2023 at 5:05 PM Borislav Petkov > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Aug 07, 2023 at 01:49:42PM +0200, Karol > > > > > > > > > > > > > Herbst wrote: > > > > > > > > > > > > > > in what way does it stop? Just not progressing? > > > > > > > > > > > > > > That would be kinda > > > > > > > > > > > > > > concerning. Mind tracing with what arguments > > > > > > > > > > > > > > `nvkm_uevent_add` is > > > > > > > > > > > > > > called with and without that patch? > > > > > > > > > > > > > > > > > > > > > > > > > > Well, me dumping those args I guess made the box not > > > > > > > > > > > > > freeze before > > > > > > > > > > > > > catching a #PF over serial. Does that help? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [3.410135] Unpacking initramfs... > > > > > > > > > > > > > [3.416319] software IO TLB: mapped [mem > > > > > > > > > > > > > 0xa877d000-0xac77d000] (64MB) > > > > > > > > > > > > > [3.418227] Initialise system trusted keyrings > > > > > > > > > > > > > [3.432273] workingset: timestamp_bits=56 > > > > > > > > > > > > > max_order=22 bucket_order=0 > > > > > > > > > > > > > [3.439006] ntfs: driver 2.1.32 [Flags: R/W]. > > > > > > > > > > > > > [3.443368] fuse: init (API version 7.38) > > > > > > > > > > > > > [3.447601] 9p: Installing v9fs 9p2000 file system > > > > > > > > > > > > > support > > > > > > > > > > > > > [3.453223] Key type asymmetric registered > > > > > > > > > > > > > [3.457332] Asymmetric key parser 'x509' registered > > > > > > > > > > > > > [3.462236] Block layer SCSI generic (bsg) driver > > > > > > > > > > > > > version 0.4 loaded (major 250) > > > > > > > > > > > > > [3.475865] efifb: probing for efifb > > > > > > > > > > > > > [3.479458] efifb: framebuffer at 0xf900, > > > > > > > > > > > > > using 1920k, total 1920k > > > > > > > > >
Re: 2b5d1c29f6c4 ("drm/nouveau/disp: PIOR DP uses GPIO for HPD, not PMGR AUX interrupts")
On Mon, 14 Aug 2023 14:38:18 +0200, Karol Herbst wrote: > > On Wed, Aug 9, 2023 at 6:16 PM Takashi Iwai wrote: > > > > On Wed, 09 Aug 2023 16:46:38 +0200, > > Takashi Iwai wrote: > > > > > > On Wed, 09 Aug 2023 15:13:23 +0200, > > > Takashi Iwai wrote: > > > > > > > > On Wed, 09 Aug 2023 14:19:23 +0200, > > > > Karol Herbst wrote: > > > > > > > > > > On Wed, Aug 9, 2023 at 1:46 PM Takashi Iwai wrote: > > > > > > > > > > > > On Wed, 09 Aug 2023 13:42:09 +0200, > > > > > > Karol Herbst wrote: > > > > > > > > > > > > > > On Wed, Aug 9, 2023 at 11:22 AM Takashi Iwai > > > > > > > wrote: > > > > > > > > > > > > > > > > On Tue, 08 Aug 2023 12:39:32 +0200, > > > > > > > > Karol Herbst wrote: > > > > > > > > > > > > > > > > > > On Mon, Aug 7, 2023 at 5:05 PM Borislav Petkov > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Mon, Aug 07, 2023 at 01:49:42PM +0200, Karol Herbst > > > > > > > > > > wrote: > > > > > > > > > > > in what way does it stop? Just not progressing? That > > > > > > > > > > > would be kinda > > > > > > > > > > > concerning. Mind tracing with what arguments > > > > > > > > > > > `nvkm_uevent_add` is > > > > > > > > > > > called with and without that patch? > > > > > > > > > > > > > > > > > > > > Well, me dumping those args I guess made the box not freeze > > > > > > > > > > before > > > > > > > > > > catching a #PF over serial. Does that help? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [3.410135] Unpacking initramfs... > > > > > > > > > > [3.416319] software IO TLB: mapped [mem > > > > > > > > > > 0xa877d000-0xac77d000] (64MB) > > > > > > > > > > [3.418227] Initialise system trusted keyrings > > > > > > > > > > [3.432273] workingset: timestamp_bits=56 max_order=22 > > > > > > > > > > bucket_order=0 > > > > > > > > > > [3.439006] ntfs: driver 2.1.32 [Flags: R/W]. > > > > > > > > > > [3.443368] fuse: init (API version 7.38) > > > > > > > > > > [3.447601] 9p: Installing v9fs 9p2000 file system > > > > > > > > > > support > > > > > > > > > > [3.453223] Key type asymmetric registered > > > > > > > > > > [3.457332] Asymmetric key parser 'x509' registered > > > > > > > > > > [3.462236] Block layer SCSI generic (bsg) driver > > > > > > > > > > version 0.4 loaded (major 250) > > > > > > > > > > [3.475865] efifb: probing for efifb > > > > > > > > > > [3.479458] efifb: framebuffer at 0xf900, using > > > > > > > > > > 1920k, total 1920k > > > > > > > > > > [3.485969] efifb: mode is 800x600x32, linelength=3200, > > > > > > > > > > pages=1 > > > > > > > > > > [3.491872] efifb: scrolling: redraw > > > > > > > > > > [3.495438] efifb: Truecolor: size=8:8:8:8, > > > > > > > > > > shift=24:16:8:0 > > > > > > > > > > [3.502349] Console: switching to colour frame buffer > > > > > > > > > > device 100x37 > > > > > > > > > > [3.509564] fb0: EFI VGA frame buffer device > > > > > > > > > > [3.514013] ACPI: \_PR_.CP00: Found 4 idle states > > > > > > > > > > [3.518850] ACPI: \_PR_.CP01: Found 4 idle states > > > > > > > > > > [3.523687] ACPI: \_PR_.CP02: Found 4 idle states > > > > > > > > > > [3.528515] ACPI: \_PR_.CP03: Found 4 idle states > > > > > > > > > > [
Re: 2b5d1c29f6c4 ("drm/nouveau/disp: PIOR DP uses GPIO for HPD, not PMGR AUX interrupts")
On Wed, 09 Aug 2023 16:46:38 +0200, Takashi Iwai wrote: > > On Wed, 09 Aug 2023 15:13:23 +0200, > Takashi Iwai wrote: > > > > On Wed, 09 Aug 2023 14:19:23 +0200, > > Karol Herbst wrote: > > > > > > On Wed, Aug 9, 2023 at 1:46 PM Takashi Iwai wrote: > > > > > > > > On Wed, 09 Aug 2023 13:42:09 +0200, > > > > Karol Herbst wrote: > > > > > > > > > > On Wed, Aug 9, 2023 at 11:22 AM Takashi Iwai wrote: > > > > > > > > > > > > On Tue, 08 Aug 2023 12:39:32 +0200, > > > > > > Karol Herbst wrote: > > > > > > > > > > > > > > On Mon, Aug 7, 2023 at 5:05 PM Borislav Petkov > > > > > > > wrote: > > > > > > > > > > > > > > > > On Mon, Aug 07, 2023 at 01:49:42PM +0200, Karol Herbst wrote: > > > > > > > > > in what way does it stop? Just not progressing? That would be > > > > > > > > > kinda > > > > > > > > > concerning. Mind tracing with what arguments > > > > > > > > > `nvkm_uevent_add` is > > > > > > > > > called with and without that patch? > > > > > > > > > > > > > > > > Well, me dumping those args I guess made the box not freeze > > > > > > > > before > > > > > > > > catching a #PF over serial. Does that help? > > > > > > > > > > > > > > > > > > > > > > > > [3.410135] Unpacking initramfs... > > > > > > > > [3.416319] software IO TLB: mapped [mem > > > > > > > > 0xa877d000-0xac77d000] (64MB) > > > > > > > > [3.418227] Initialise system trusted keyrings > > > > > > > > [3.432273] workingset: timestamp_bits=56 max_order=22 > > > > > > > > bucket_order=0 > > > > > > > > [3.439006] ntfs: driver 2.1.32 [Flags: R/W]. > > > > > > > > [3.443368] fuse: init (API version 7.38) > > > > > > > > [3.447601] 9p: Installing v9fs 9p2000 file system support > > > > > > > > [3.453223] Key type asymmetric registered > > > > > > > > [3.457332] Asymmetric key parser 'x509' registered > > > > > > > > [3.462236] Block layer SCSI generic (bsg) driver version > > > > > > > > 0.4 loaded (major 250) > > > > > > > > [3.475865] efifb: probing for efifb > > > > > > > > [3.479458] efifb: framebuffer at 0xf900, using 1920k, > > > > > > > > total 1920k > > > > > > > > [3.485969] efifb: mode is 800x600x32, linelength=3200, > > > > > > > > pages=1 > > > > > > > > [3.491872] efifb: scrolling: redraw > > > > > > > > [3.495438] efifb: Truecolor: size=8:8:8:8, shift=24:16:8:0 > > > > > > > > [3.502349] Console: switching to colour frame buffer device > > > > > > > > 100x37 > > > > > > > > [3.509564] fb0: EFI VGA frame buffer device > > > > > > > > [3.514013] ACPI: \_PR_.CP00: Found 4 idle states > > > > > > > > [3.518850] ACPI: \_PR_.CP01: Found 4 idle states > > > > > > > > [3.523687] ACPI: \_PR_.CP02: Found 4 idle states > > > > > > > > [3.528515] ACPI: \_PR_.CP03: Found 4 idle states > > > > > > > > [3.533346] ACPI: \_PR_.CP04: Found 4 idle states > > > > > > > > [3.538173] ACPI: \_PR_.CP05: Found 4 idle states > > > > > > > > [3.543003] ACPI: \_PR_.CP06: Found 4 idle states > > > > > > > > [3.544219] Freeing initrd memory: 8196K > > > > > > > > [3.547844] ACPI: \_PR_.CP07: Found 4 idle states > > > > > > > > [3.609542] Serial: 8250/16550 driver, 4 ports, IRQ sharing > > > > > > > > enabled > > > > > > > > [3.616224] 00:05: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = > > > > > > > > 115200) is a 16550A > > > > > > > > [3.625552] serial :00:16.3: enabling device ( -> > > > > > > > > 0003) > > > > > > > > [3
Re: 2b5d1c29f6c4 ("drm/nouveau/disp: PIOR DP uses GPIO for HPD, not PMGR AUX interrupts")
On Wed, 09 Aug 2023 15:13:23 +0200, Takashi Iwai wrote: > > On Wed, 09 Aug 2023 14:19:23 +0200, > Karol Herbst wrote: > > > > On Wed, Aug 9, 2023 at 1:46 PM Takashi Iwai wrote: > > > > > > On Wed, 09 Aug 2023 13:42:09 +0200, > > > Karol Herbst wrote: > > > > > > > > On Wed, Aug 9, 2023 at 11:22 AM Takashi Iwai wrote: > > > > > > > > > > On Tue, 08 Aug 2023 12:39:32 +0200, > > > > > Karol Herbst wrote: > > > > > > > > > > > > On Mon, Aug 7, 2023 at 5:05 PM Borislav Petkov > > > > > > wrote: > > > > > > > > > > > > > > On Mon, Aug 07, 2023 at 01:49:42PM +0200, Karol Herbst wrote: > > > > > > > > in what way does it stop? Just not progressing? That would be > > > > > > > > kinda > > > > > > > > concerning. Mind tracing with what arguments `nvkm_uevent_add` > > > > > > > > is > > > > > > > > called with and without that patch? > > > > > > > > > > > > > > Well, me dumping those args I guess made the box not freeze before > > > > > > > catching a #PF over serial. Does that help? > > > > > > > > > > > > > > > > > > > > > [3.410135] Unpacking initramfs... > > > > > > > [3.416319] software IO TLB: mapped [mem > > > > > > > 0xa877d000-0xac77d000] (64MB) > > > > > > > [3.418227] Initialise system trusted keyrings > > > > > > > [3.432273] workingset: timestamp_bits=56 max_order=22 > > > > > > > bucket_order=0 > > > > > > > [3.439006] ntfs: driver 2.1.32 [Flags: R/W]. > > > > > > > [3.443368] fuse: init (API version 7.38) > > > > > > > [3.447601] 9p: Installing v9fs 9p2000 file system support > > > > > > > [3.453223] Key type asymmetric registered > > > > > > > [3.457332] Asymmetric key parser 'x509' registered > > > > > > > [3.462236] Block layer SCSI generic (bsg) driver version 0.4 > > > > > > > loaded (major 250) > > > > > > > [3.475865] efifb: probing for efifb > > > > > > > [3.479458] efifb: framebuffer at 0xf900, using 1920k, > > > > > > > total 1920k > > > > > > > [3.485969] efifb: mode is 800x600x32, linelength=3200, pages=1 > > > > > > > [3.491872] efifb: scrolling: redraw > > > > > > > [3.495438] efifb: Truecolor: size=8:8:8:8, shift=24:16:8:0 > > > > > > > [3.502349] Console: switching to colour frame buffer device > > > > > > > 100x37 > > > > > > > [3.509564] fb0: EFI VGA frame buffer device > > > > > > > [3.514013] ACPI: \_PR_.CP00: Found 4 idle states > > > > > > > [3.518850] ACPI: \_PR_.CP01: Found 4 idle states > > > > > > > [3.523687] ACPI: \_PR_.CP02: Found 4 idle states > > > > > > > [3.528515] ACPI: \_PR_.CP03: Found 4 idle states > > > > > > > [3.533346] ACPI: \_PR_.CP04: Found 4 idle states > > > > > > > [3.538173] ACPI: \_PR_.CP05: Found 4 idle states > > > > > > > [3.543003] ACPI: \_PR_.CP06: Found 4 idle states > > > > > > > [3.544219] Freeing initrd memory: 8196K > > > > > > > [3.547844] ACPI: \_PR_.CP07: Found 4 idle states > > > > > > > [3.609542] Serial: 8250/16550 driver, 4 ports, IRQ sharing > > > > > > > enabled > > > > > > > [3.616224] 00:05: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = > > > > > > > 115200) is a 16550A > > > > > > > [3.625552] serial :00:16.3: enabling device ( -> 0003) > > > > > > > [3.633034] :00:16.3: ttyS1 at I/O 0xf0a0 (irq = 17, > > > > > > > base_baud = 115200) is a 16550A > > > > > > > [3.642451] Linux agpgart interface v0.103 > > > > > > > [3.647141] ACPI: bus type drm_connector registered > > > > > > > [3.653261] Console: switching to colour dummy device 80x25 > > > > > > > [3.659092] nouveau :03:00.0: vgaarb: deactivate vga > > > > > > > co
Re: 2b5d1c29f6c4 ("drm/nouveau/disp: PIOR DP uses GPIO for HPD, not PMGR AUX interrupts")
On Wed, 09 Aug 2023 14:19:23 +0200, Karol Herbst wrote: > > On Wed, Aug 9, 2023 at 1:46 PM Takashi Iwai wrote: > > > > On Wed, 09 Aug 2023 13:42:09 +0200, > > Karol Herbst wrote: > > > > > > On Wed, Aug 9, 2023 at 11:22 AM Takashi Iwai wrote: > > > > > > > > On Tue, 08 Aug 2023 12:39:32 +0200, > > > > Karol Herbst wrote: > > > > > > > > > > On Mon, Aug 7, 2023 at 5:05 PM Borislav Petkov wrote: > > > > > > > > > > > > On Mon, Aug 07, 2023 at 01:49:42PM +0200, Karol Herbst wrote: > > > > > > > in what way does it stop? Just not progressing? That would be > > > > > > > kinda > > > > > > > concerning. Mind tracing with what arguments `nvkm_uevent_add` is > > > > > > > called with and without that patch? > > > > > > > > > > > > Well, me dumping those args I guess made the box not freeze before > > > > > > catching a #PF over serial. Does that help? > > > > > > > > > > > > > > > > > > [3.410135] Unpacking initramfs... > > > > > > [3.416319] software IO TLB: mapped [mem > > > > > > 0xa877d000-0xac77d000] (64MB) > > > > > > [3.418227] Initialise system trusted keyrings > > > > > > [3.432273] workingset: timestamp_bits=56 max_order=22 > > > > > > bucket_order=0 > > > > > > [3.439006] ntfs: driver 2.1.32 [Flags: R/W]. > > > > > > [3.443368] fuse: init (API version 7.38) > > > > > > [3.447601] 9p: Installing v9fs 9p2000 file system support > > > > > > [3.453223] Key type asymmetric registered > > > > > > [3.457332] Asymmetric key parser 'x509' registered > > > > > > [3.462236] Block layer SCSI generic (bsg) driver version 0.4 > > > > > > loaded (major 250) > > > > > > [3.475865] efifb: probing for efifb > > > > > > [3.479458] efifb: framebuffer at 0xf900, using 1920k, total > > > > > > 1920k > > > > > > [3.485969] efifb: mode is 800x600x32, linelength=3200, pages=1 > > > > > > [3.491872] efifb: scrolling: redraw > > > > > > [3.495438] efifb: Truecolor: size=8:8:8:8, shift=24:16:8:0 > > > > > > [3.502349] Console: switching to colour frame buffer device > > > > > > 100x37 > > > > > > [3.509564] fb0: EFI VGA frame buffer device > > > > > > [3.514013] ACPI: \_PR_.CP00: Found 4 idle states > > > > > > [3.518850] ACPI: \_PR_.CP01: Found 4 idle states > > > > > > [3.523687] ACPI: \_PR_.CP02: Found 4 idle states > > > > > > [3.528515] ACPI: \_PR_.CP03: Found 4 idle states > > > > > > [3.533346] ACPI: \_PR_.CP04: Found 4 idle states > > > > > > [3.538173] ACPI: \_PR_.CP05: Found 4 idle states > > > > > > [3.543003] ACPI: \_PR_.CP06: Found 4 idle states > > > > > > [3.544219] Freeing initrd memory: 8196K > > > > > > [3.547844] ACPI: \_PR_.CP07: Found 4 idle states > > > > > > [3.609542] Serial: 8250/16550 driver, 4 ports, IRQ sharing > > > > > > enabled > > > > > > [3.616224] 00:05: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = > > > > > > 115200) is a 16550A > > > > > > [3.625552] serial :00:16.3: enabling device ( -> 0003) > > > > > > [3.633034] :00:16.3: ttyS1 at I/O 0xf0a0 (irq = 17, > > > > > > base_baud = 115200) is a 16550A > > > > > > [3.642451] Linux agpgart interface v0.103 > > > > > > [3.647141] ACPI: bus type drm_connector registered > > > > > > [3.653261] Console: switching to colour dummy device 80x25 > > > > > > [3.659092] nouveau :03:00.0: vgaarb: deactivate vga console > > > > > > [3.665174] nouveau :03:00.0: NVIDIA GT218 (0a8c00b1) > > > > > > [3.784585] nouveau :03:00.0: bios: version 70.18.83.00.08 > > > > > > [3.792244] nouveau :03:00.0: fb: 512 MiB DDR3 > > > > > > [3.948786] nouveau :03:00.0: DRM: VRAM: 512 MiB > > > > > > [3.953755] nouveau :03:00.0: DRM: GART: 1048576 MiB > > > > > > [3.959073] nouveau :0
Re: 2b5d1c29f6c4 ("drm/nouveau/disp: PIOR DP uses GPIO for HPD, not PMGR AUX interrupts")
On Wed, 09 Aug 2023 13:42:09 +0200, Karol Herbst wrote: > > On Wed, Aug 9, 2023 at 11:22 AM Takashi Iwai wrote: > > > > On Tue, 08 Aug 2023 12:39:32 +0200, > > Karol Herbst wrote: > > > > > > On Mon, Aug 7, 2023 at 5:05 PM Borislav Petkov wrote: > > > > > > > > On Mon, Aug 07, 2023 at 01:49:42PM +0200, Karol Herbst wrote: > > > > > in what way does it stop? Just not progressing? That would be kinda > > > > > concerning. Mind tracing with what arguments `nvkm_uevent_add` is > > > > > called with and without that patch? > > > > > > > > Well, me dumping those args I guess made the box not freeze before > > > > catching a #PF over serial. Does that help? > > > > > > > > > > > > [3.410135] Unpacking initramfs... > > > > [3.416319] software IO TLB: mapped [mem > > > > 0xa877d000-0xac77d000] (64MB) > > > > [3.418227] Initialise system trusted keyrings > > > > [3.432273] workingset: timestamp_bits=56 max_order=22 bucket_order=0 > > > > [3.439006] ntfs: driver 2.1.32 [Flags: R/W]. > > > > [3.443368] fuse: init (API version 7.38) > > > > [3.447601] 9p: Installing v9fs 9p2000 file system support > > > > [3.453223] Key type asymmetric registered > > > > [3.457332] Asymmetric key parser 'x509' registered > > > > [3.462236] Block layer SCSI generic (bsg) driver version 0.4 loaded > > > > (major 250) > > > > [3.475865] efifb: probing for efifb > > > > [3.479458] efifb: framebuffer at 0xf900, using 1920k, total > > > > 1920k > > > > [3.485969] efifb: mode is 800x600x32, linelength=3200, pages=1 > > > > [3.491872] efifb: scrolling: redraw > > > > [3.495438] efifb: Truecolor: size=8:8:8:8, shift=24:16:8:0 > > > > [3.502349] Console: switching to colour frame buffer device 100x37 > > > > [3.509564] fb0: EFI VGA frame buffer device > > > > [3.514013] ACPI: \_PR_.CP00: Found 4 idle states > > > > [3.518850] ACPI: \_PR_.CP01: Found 4 idle states > > > > [3.523687] ACPI: \_PR_.CP02: Found 4 idle states > > > > [3.528515] ACPI: \_PR_.CP03: Found 4 idle states > > > > [3.533346] ACPI: \_PR_.CP04: Found 4 idle states > > > > [3.538173] ACPI: \_PR_.CP05: Found 4 idle states > > > > [3.543003] ACPI: \_PR_.CP06: Found 4 idle states > > > > [3.544219] Freeing initrd memory: 8196K > > > > [3.547844] ACPI: \_PR_.CP07: Found 4 idle states > > > > [3.609542] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled > > > > [3.616224] 00:05: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) > > > > is a 16550A > > > > [3.625552] serial :00:16.3: enabling device ( -> 0003) > > > > [3.633034] :00:16.3: ttyS1 at I/O 0xf0a0 (irq = 17, base_baud = > > > > 115200) is a 16550A > > > > [3.642451] Linux agpgart interface v0.103 > > > > [3.647141] ACPI: bus type drm_connector registered > > > > [3.653261] Console: switching to colour dummy device 80x25 > > > > [3.659092] nouveau :03:00.0: vgaarb: deactivate vga console > > > > [3.665174] nouveau :03:00.0: NVIDIA GT218 (0a8c00b1) > > > > [3.784585] nouveau :03:00.0: bios: version 70.18.83.00.08 > > > > [3.792244] nouveau :03:00.0: fb: 512 MiB DDR3 > > > > [3.948786] nouveau :03:00.0: DRM: VRAM: 512 MiB > > > > [3.953755] nouveau :03:00.0: DRM: GART: 1048576 MiB > > > > [3.959073] nouveau :03:00.0: DRM: TMDS table version 2.0 > > > > [3.964808] nouveau :03:00.0: DRM: DCB version 4.0 > > > > [3.969938] nouveau :03:00.0: DRM: DCB outp 00: 02000360 > > > > [3.976367] nouveau :03:00.0: DRM: DCB outp 01: 02000362 00020010 > > > > [3.982792] nouveau :03:00.0: DRM: DCB outp 02: 028003a6 0f220010 > > > > [3.989223] nouveau :03:00.0: DRM: DCB outp 03: 01011380 > > > > [3.995647] nouveau :03:00.0: DRM: DCB outp 04: 08011382 00020010 > > > > [4.002076] nouveau :03:00.0: DRM: DCB outp 05: 088113c6 0f220010 > > > > [4.008511] nouveau :03:00.0: DRM: DCB conn 00: 00101064 > > > > [4.014151] nouveau :03:00.0: DRM: DCB conn 01: 00202165 > > > > [4.021710] nv
Re: 2b5d1c29f6c4 ("drm/nouveau/disp: PIOR DP uses GPIO for HPD, not PMGR AUX interrupts")
On Tue, 08 Aug 2023 12:39:32 +0200, Karol Herbst wrote: > > On Mon, Aug 7, 2023 at 5:05 PM Borislav Petkov wrote: > > > > On Mon, Aug 07, 2023 at 01:49:42PM +0200, Karol Herbst wrote: > > > in what way does it stop? Just not progressing? That would be kinda > > > concerning. Mind tracing with what arguments `nvkm_uevent_add` is > > > called with and without that patch? > > > > Well, me dumping those args I guess made the box not freeze before > > catching a #PF over serial. Does that help? > > > > > > [3.410135] Unpacking initramfs... > > [3.416319] software IO TLB: mapped [mem > > 0xa877d000-0xac77d000] (64MB) > > [3.418227] Initialise system trusted keyrings > > [3.432273] workingset: timestamp_bits=56 max_order=22 bucket_order=0 > > [3.439006] ntfs: driver 2.1.32 [Flags: R/W]. > > [3.443368] fuse: init (API version 7.38) > > [3.447601] 9p: Installing v9fs 9p2000 file system support > > [3.453223] Key type asymmetric registered > > [3.457332] Asymmetric key parser 'x509' registered > > [3.462236] Block layer SCSI generic (bsg) driver version 0.4 loaded > > (major 250) > > [3.475865] efifb: probing for efifb > > [3.479458] efifb: framebuffer at 0xf900, using 1920k, total 1920k > > [3.485969] efifb: mode is 800x600x32, linelength=3200, pages=1 > > [3.491872] efifb: scrolling: redraw > > [3.495438] efifb: Truecolor: size=8:8:8:8, shift=24:16:8:0 > > [3.502349] Console: switching to colour frame buffer device 100x37 > > [3.509564] fb0: EFI VGA frame buffer device > > [3.514013] ACPI: \_PR_.CP00: Found 4 idle states > > [3.518850] ACPI: \_PR_.CP01: Found 4 idle states > > [3.523687] ACPI: \_PR_.CP02: Found 4 idle states > > [3.528515] ACPI: \_PR_.CP03: Found 4 idle states > > [3.533346] ACPI: \_PR_.CP04: Found 4 idle states > > [3.538173] ACPI: \_PR_.CP05: Found 4 idle states > > [3.543003] ACPI: \_PR_.CP06: Found 4 idle states > > [3.544219] Freeing initrd memory: 8196K > > [3.547844] ACPI: \_PR_.CP07: Found 4 idle states > > [3.609542] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled > > [3.616224] 00:05: ttyS0 at I/O 0x3f8 (irq = 4, base_baud = 115200) is a > > 16550A > > [3.625552] serial :00:16.3: enabling device ( -> 0003) > > [3.633034] :00:16.3: ttyS1 at I/O 0xf0a0 (irq = 17, base_baud = > > 115200) is a 16550A > > [3.642451] Linux agpgart interface v0.103 > > [3.647141] ACPI: bus type drm_connector registered > > [3.653261] Console: switching to colour dummy device 80x25 > > [3.659092] nouveau :03:00.0: vgaarb: deactivate vga console > > [3.665174] nouveau :03:00.0: NVIDIA GT218 (0a8c00b1) > > [3.784585] nouveau :03:00.0: bios: version 70.18.83.00.08 > > [3.792244] nouveau :03:00.0: fb: 512 MiB DDR3 > > [3.948786] nouveau :03:00.0: DRM: VRAM: 512 MiB > > [3.953755] nouveau :03:00.0: DRM: GART: 1048576 MiB > > [3.959073] nouveau :03:00.0: DRM: TMDS table version 2.0 > > [3.964808] nouveau :03:00.0: DRM: DCB version 4.0 > > [3.969938] nouveau :03:00.0: DRM: DCB outp 00: 02000360 > > [3.976367] nouveau :03:00.0: DRM: DCB outp 01: 02000362 00020010 > > [3.982792] nouveau :03:00.0: DRM: DCB outp 02: 028003a6 0f220010 > > [3.989223] nouveau :03:00.0: DRM: DCB outp 03: 01011380 > > [3.995647] nouveau :03:00.0: DRM: DCB outp 04: 08011382 00020010 > > [4.002076] nouveau :03:00.0: DRM: DCB outp 05: 088113c6 0f220010 > > [4.008511] nouveau :03:00.0: DRM: DCB conn 00: 00101064 > > [4.014151] nouveau :03:00.0: DRM: DCB conn 01: 00202165 > > [4.021710] nvkm_uevent_add: uevent: 0x888100242100, event: > > 0x8881022de1a0, id: 0x0, bits: 0x1, func: 0x > > [4.033680] nvkm_uevent_add: uevent: 0x888100242300, event: > > 0x8881022de1a0, id: 0x0, bits: 0x1, func: 0x > > [4.045429] nouveau :03:00.0: DRM: MM: using COPY for buffer copies > > [4.052059] stackdepot: allocating hash table of 1048576 entries via > > kvcalloc > > [4.067191] nvkm_uevent_add: uevent: 0x888100242800, event: > > 0x888104b3e260, id: 0x0, bits: 0x1, func: 0x > > [4.078936] nvkm_uevent_add: uevent: 0x888100242900, event: > > 0x888104b3e260, id: 0x1, bits: 0x1, func: 0x > > [4.090514] nvkm_uevent_add: uevent: 0x888100242a00, event: > > 0x888102091f28, id: 0x1, bits: 0x3, func: 0x8177b700 > > [4.102118] tsc: Refined TSC clocksource calibration: 3591.345 MHz > > [4.108342] clocksource: tsc: mask: 0x max_cycles: > > 0x33c4635c383, max_idle_ns: 440795314831 ns > > [4.108401] nvkm_uevent_add: uevent: 0x8881020b6000, event: > > 0x888102091f28, id: 0xf, bits: 0x3, func: 0x8177b700 > > [4.129864] clocksource: Switched to clocksource tsc > > [
Re: Missing AMDGPU drm-fixes-6.4 merges
On Wed, 05 Jul 2023 14:17:20 +0200, Alex Deucher wrote: > > On Wed, Jul 5, 2023 at 2:26 AM Takashi Iwai wrote: > > > > Hi Dave, Alex, > > > > it seems that the last PR for AMDGPU 6.4 fixes wasn't taken by Linus > > due to the missing signed tag: > > > > https://lore.kernel.org/lkml/CAHk-=wiOCgiwzVPOwORHPML9eBphnbtM2DhRcv+v=-tnrrg...@mail.gmail.com/ > > > > And more importantly, this series isn't seen on linux-next, either; so > > the whole fixes are missing in the upstream. Could you check it? > > I included all of the necessary changes in last week's 6.5 fixes PR. > Once that lands, I'll make sure the necessary changes make it to > stable. Great, thanks for the update! Another bug report (for Leap 15.5) showed that the series fixed some nasty suspend/resume issue, so let's hope that all those get merged soon :) Takashi > > Alex > > > > > FWIW, I stumbled on this due to a recent regression report on openSUSE > > Tumbleweed: > > https://bugzilla.suse.com/show_bug.cgi?id=1212848 > > > > > > thanks, > > > > Takashi >
Missing AMDGPU drm-fixes-6.4 merges
Hi Dave, Alex, it seems that the last PR for AMDGPU 6.4 fixes wasn't taken by Linus due to the missing signed tag: https://lore.kernel.org/lkml/CAHk-=wiOCgiwzVPOwORHPML9eBphnbtM2DhRcv+v=-tnrrg...@mail.gmail.com/ And more importantly, this series isn't seen on linux-next, either; so the whole fixes are missing in the upstream. Could you check it? FWIW, I stumbled on this due to a recent regression report on openSUSE Tumbleweed: https://bugzilla.suse.com/show_bug.cgi?id=1212848 thanks, Takashi
Re: [PATCH] drm/udl: delete dead code
On Tue, 02 May 2023 14:59:56 +0200,
Dan Carpenter wrote:
>
> The "unode" pointer cannot be NULL here and checking for it causes
> Smatch warnings:
>
>drivers/gpu/drm/udl/udl_main.c:259 udl_get_urb_locked()
>warn: can 'unode' even be NULL?
>
> Fortunately, it's just harmless dead code which can be removed. It's
> left over from commit c5c354a3a472 ("drm/udl: Fix inconsistent urbs.count
> value during udl_free_urb_list()").
>
> Reported-by: kernel test robot
> Signed-off-by: Dan Carpenter
Reviewed-by: Takashi Iwai
thanks,
Takashi
Re: [REGRESSION] Missing nouveau backlight control on 6.2.x kernel
On Sat, 18 Mar 2023 17:49:28 +0100, Hans de Goede wrote: > > Hi Takashi, > > On 3/17/23 11:04, Takashi Iwai wrote: > > Hi, > > > > we've received a regression report on openSUSE Bugzilla about the > > missing backlight control of nouveau device: > > https://bugzilla.opensuse.org/show_bug.cgi?id=1209296 > > > > On 6.1, with acpi_video=native option, the system provided > > /sys/class/backlight/nv_backlight entry. > > > > On 6.2, with acpi_video=native option, there is no entry in > > /sys/class/backlight. And without acpi_video option, it falls back to > > /sys/class/backlight/acpi_video0, which doesn't work as expected. > > > > Hans, could you check whether the recent change in video_detect.c (or > > anything else) may influence on this? My gut feeling is that the > > culprit could be rather some change in nouveau (so dri-devel is > > Cc'ed), but I'm not entirely sure... > > As I already told the reporter, who first contacted me by private email about > this, all the drivers/acpi/video_detect.c changes which may influence this > are also present in 6.1.y (IIRC >= 6.1.5) . So since this is being reported > as only happening with 6.2.y I don't think the recent backlight detect rework > is involved. > > And even if the recent backlight detect rework were involved, specifying > acpi_backlight=native on the kernel commandline (as the reporter was doing > before) should still work since it overrides all other backlight detection. > > So yes this seems to be a nouveau bug and should probably be reported > following the instructions from: https://nouveau.freedesktop.org/Bugs.html > (which I havealso told the reporter already). Thanks for verification! Takashi
[REGRESSION] Missing nouveau backlight control on 6.2.x kernel
Hi, we've received a regression report on openSUSE Bugzilla about the missing backlight control of nouveau device: https://bugzilla.opensuse.org/show_bug.cgi?id=1209296 On 6.1, with acpi_video=native option, the system provided /sys/class/backlight/nv_backlight entry. On 6.2, with acpi_video=native option, there is no entry in /sys/class/backlight. And without acpi_video option, it falls back to /sys/class/backlight/acpi_video0, which doesn't work as expected. Hans, could you check whether the recent change in video_detect.c (or anything else) may influence on this? My gut feeling is that the culprit could be rather some change in nouveau (so dri-devel is Cc'ed), but I'm not entirely sure... Thanks! Takashi
[PATCH v2] fbdev: Fix incorrect page mapping clearance at fb_deferred_io_release()
The recent fix for the deferred I/O by the commit
3efc61d95259 ("fbdev: Fix invalid page access after closing deferred I/O
devices")
caused a regression when the same fb device is opened/closed while
it's being used. It resulted in a frozen screen even if something
is redrawn there after the close. The breakage is because the patch
was made under a wrong assumption of a single open; in the current
code, fb_deferred_io_release() cleans up the page mapping of the
pageref list and it calls cancel_delayed_work_sync() unconditionally,
where both are no correct behavior for multiple opens.
This patch adds a refcount for the opens of the device, and applies
the cleanup only when all files get closed.
As both fb_deferred_io_open() and _close() are called always in the
fb_info lock (mutex), it's safe to use the normal int for the
refcounting.
Also, a useless BUG_ON() is dropped.
Fixes: 3efc61d95259 ("fbdev: Fix invalid page access after closing deferred I/O
devices")
Cc:
Signed-off-by: Takashi Iwai
---
v1->v2:
* Rename to fb_deferred_io_lastclose()
* Rename the new field from opens to open_count
* Removed unused variable
* More comments about fb_info locking
drivers/video/fbdev/core/fb_defio.c | 17 +
include/linux/fb.h | 1 +
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c
b/drivers/video/fbdev/core/fb_defio.c
index aa5f059d0222..274f5d0fa247 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -305,17 +305,18 @@ void fb_deferred_io_open(struct fb_info *info,
struct inode *inode,
struct file *file)
{
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+
file->f_mapping->a_ops = _deferred_io_aops;
+ fbdefio->open_count++;
}
EXPORT_SYMBOL_GPL(fb_deferred_io_open);
-void fb_deferred_io_release(struct fb_info *info)
+static void fb_deferred_io_lastclose(struct fb_info *info)
{
- struct fb_deferred_io *fbdefio = info->fbdefio;
struct page *page;
int i;
- BUG_ON(!fbdefio);
cancel_delayed_work_sync(>deferred_work);
/* clear out the mapping that we setup */
@@ -324,13 +325,21 @@ void fb_deferred_io_release(struct fb_info *info)
page->mapping = NULL;
}
}
+
+void fb_deferred_io_release(struct fb_info *info)
+{
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+
+ if (!--fbdefio->open_count)
+ fb_deferred_io_lastclose(info);
+}
EXPORT_SYMBOL_GPL(fb_deferred_io_release);
void fb_deferred_io_cleanup(struct fb_info *info)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
- fb_deferred_io_release(info);
+ fb_deferred_io_lastclose(info);
kvfree(info->pagerefs);
mutex_destroy(>lock);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index d8d20514ea05..02d09cb57f6c 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -212,6 +212,7 @@ struct fb_deferred_io {
/* delay between mkwrite and deferred handler */
unsigned long delay;
bool sort_pagereflist; /* sort pagelist by offset */
+ int open_count; /* number of opened files; protected by fb_info lock */
struct mutex lock; /* mutex that protects the pageref list */
struct list_head pagereflist; /* list of pagerefs for touched pages */
/* callback */
--
2.35.3
Re: [PATCH] fbdev: Fix incorrect page mapping clearance at fb_deferred_io_release()
On Wed, 08 Mar 2023 10:08:24 +0100,
Patrik Jakobsson wrote:
>
> On Wed, Mar 8, 2023 at 7:36 AM Takashi Iwai wrote:
> >
> > The recent fix for the deferred I/O by the commit
> > 3efc61d95259 ("fbdev: Fix invalid page access after closing deferred I/O
> > devices")
> > caused a regression when the same fb device is opened/closed while
> > it's being used. It resulted in a frozen screen even if something
> > is redrawn there after the close. The breakage is because the patch
> > was made under a wrong assumption of a single open; in the current
> > code, fb_deferred_io_release() cleans up the page mapping of the
> > pageref list and it calls cancel_delayed_work_sync() unconditionally,
> > where both are no correct behavior for multiple opens.
> >
> > This patch adds a refcount for the opens of the device, and applies
> > the cleanup only when all files get closed.
> >
> > Fixes: 3efc61d95259 ("fbdev: Fix invalid page access after closing deferred
> > I/O devices")
> > Cc:
> > Signed-off-by: Takashi Iwai
> > ---
> > drivers/video/fbdev/core/fb_defio.c | 16 +---
> > include/linux/fb.h | 1 +
> > 2 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/core/fb_defio.c
> > b/drivers/video/fbdev/core/fb_defio.c
> > index aa5f059d0222..9dcec9e020b6 100644
> > --- a/drivers/video/fbdev/core/fb_defio.c
> > +++ b/drivers/video/fbdev/core/fb_defio.c
> > @@ -305,17 +305,19 @@ void fb_deferred_io_open(struct fb_info *info,
> > struct inode *inode,
> > struct file *file)
> > {
> > + struct fb_deferred_io *fbdefio = info->fbdefio;
> > +
> > file->f_mapping->a_ops = _deferred_io_aops;
> > + fbdefio->opens++;
> > }
> > EXPORT_SYMBOL_GPL(fb_deferred_io_open);
> >
> > -void fb_deferred_io_release(struct fb_info *info)
> > +static void fb_deferred_io_release_internal(struct fb_info *info)
>
> Maybe a better name would be fb_deferred_io_lastclose() to be more in
> line with DRM?
Sounds good.
> > {
> > struct fb_deferred_io *fbdefio = info->fbdefio;
> > struct page *page;
> > int i;
> >
> > - BUG_ON(!fbdefio);
>
> Should the BUG_ON be put back into fb_deferred_io_release()?
It can be, but honestly speaking, such a BUG_ON() is utterly useless.
It should be WARN_ON() and return, if the sanity check is inevitably
needed.
> > cancel_delayed_work_sync(>deferred_work);
> >
> > /* clear out the mapping that we setup */
> > @@ -324,13 +326,21 @@ void fb_deferred_io_release(struct fb_info *info)
> > page->mapping = NULL;
> > }
> > }
> > +
> > +void fb_deferred_io_release(struct fb_info *info)
> > +{
> > + struct fb_deferred_io *fbdefio = info->fbdefio;
> > +
> > + if (!--fbdefio->opens)
> > + fb_deferred_io_release_internal(info);
>
> I think this can race so we need locking.
This one is fine, as it's always called inside the fb lock in the
caller side. Maybe worth to comment in the code.
> > +}
> > EXPORT_SYMBOL_GPL(fb_deferred_io_release);
> >
> > void fb_deferred_io_cleanup(struct fb_info *info)
> > {
> > struct fb_deferred_io *fbdefio = info->fbdefio;
> >
> > - fb_deferred_io_release(info);
> > + fb_deferred_io_release_internal(info);
> >
> > kvfree(info->pagerefs);
> > mutex_destroy(>lock);
> > diff --git a/include/linux/fb.h b/include/linux/fb.h
> > index d8d20514ea05..29674a29d1c4 100644
> > --- a/include/linux/fb.h
> > +++ b/include/linux/fb.h
> > @@ -212,6 +212,7 @@ struct fb_deferred_io {
> > /* delay between mkwrite and deferred handler */
> > unsigned long delay;
> > bool sort_pagereflist; /* sort pagelist by offset */
> > + int opens; /* number of opened files */
>
> I would prefer the name num_opens (or open_count as in DRM) instead of
> opens since it can be interpreted as a verb.
I don't mind either way. I'd choose the latter.
> Also, don't we need it to be atomic_t?
It's always in the fb lock, so that should be fine with the standard
int.
thanks,
Takashi
[PATCH] fbdev: Fix incorrect page mapping clearance at fb_deferred_io_release()
The recent fix for the deferred I/O by the commit
3efc61d95259 ("fbdev: Fix invalid page access after closing deferred I/O
devices")
caused a regression when the same fb device is opened/closed while
it's being used. It resulted in a frozen screen even if something
is redrawn there after the close. The breakage is because the patch
was made under a wrong assumption of a single open; in the current
code, fb_deferred_io_release() cleans up the page mapping of the
pageref list and it calls cancel_delayed_work_sync() unconditionally,
where both are no correct behavior for multiple opens.
This patch adds a refcount for the opens of the device, and applies
the cleanup only when all files get closed.
Fixes: 3efc61d95259 ("fbdev: Fix invalid page access after closing deferred I/O
devices")
Cc:
Signed-off-by: Takashi Iwai
---
drivers/video/fbdev/core/fb_defio.c | 16 +---
include/linux/fb.h | 1 +
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c
b/drivers/video/fbdev/core/fb_defio.c
index aa5f059d0222..9dcec9e020b6 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -305,17 +305,19 @@ void fb_deferred_io_open(struct fb_info *info,
struct inode *inode,
struct file *file)
{
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+
file->f_mapping->a_ops = _deferred_io_aops;
+ fbdefio->opens++;
}
EXPORT_SYMBOL_GPL(fb_deferred_io_open);
-void fb_deferred_io_release(struct fb_info *info)
+static void fb_deferred_io_release_internal(struct fb_info *info)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
struct page *page;
int i;
- BUG_ON(!fbdefio);
cancel_delayed_work_sync(>deferred_work);
/* clear out the mapping that we setup */
@@ -324,13 +326,21 @@ void fb_deferred_io_release(struct fb_info *info)
page->mapping = NULL;
}
}
+
+void fb_deferred_io_release(struct fb_info *info)
+{
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+
+ if (!--fbdefio->opens)
+ fb_deferred_io_release_internal(info);
+}
EXPORT_SYMBOL_GPL(fb_deferred_io_release);
void fb_deferred_io_cleanup(struct fb_info *info)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
- fb_deferred_io_release(info);
+ fb_deferred_io_release_internal(info);
kvfree(info->pagerefs);
mutex_destroy(>lock);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index d8d20514ea05..29674a29d1c4 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -212,6 +212,7 @@ struct fb_deferred_io {
/* delay between mkwrite and deferred handler */
unsigned long delay;
bool sort_pagereflist; /* sort pagelist by offset */
+ int opens; /* number of opened files */
struct mutex lock; /* mutex that protects the pageref list */
struct list_head pagereflist; /* list of pagerefs for touched pages */
/* callback */
--
2.35.3
Re: [PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
On Mon, 30 Jan 2023 09:52:43 +0100,
Takashi Iwai wrote:
>
> > There's a call to cancel_delayed_work_sync() in the new helper
> > fb_deferred_io_release(). Is this the right function? Maybe
> > flush_delayed_work() is a better choice.
>
> I thought of that, but took a shorter path.
> OK, let's check whether this keeps working with that change.
Interestingly, this still makes the bug happening at the very same
place. (The tested patch is below.)
So, more looking and testing, more confusing the problem becomes :-<
Takashi
-- 8< --
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -313,20 +313,31 @@ void fb_deferred_io_open(struct fb_info *info,
}
EXPORT_SYMBOL_GPL(fb_deferred_io_open);
-void fb_deferred_io_cleanup(struct fb_info *info)
+/* clear out the mapping that we setup */
+static void fb_deferred_io_clear_mapping(struct fb_info *info)
{
- struct fb_deferred_io *fbdefio = info->fbdefio;
struct page *page;
int i;
- BUG_ON(!fbdefio);
- cancel_delayed_work_sync(>deferred_work);
-
- /* clear out the mapping that we setup */
for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
page = fb_deferred_io_page(info, i);
page->mapping = NULL;
}
+}
+
+void fb_deferred_io_release(struct fb_info *info)
+{
+ flush_delayed_work(>deferred_work);
+ fb_deferred_io_clear_mapping(info);
+}
+
+void fb_deferred_io_cleanup(struct fb_info *info)
+{
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+
+ BUG_ON(!fbdefio);
+ cancel_delayed_work_sync(>deferred_work);
+ fb_deferred_io_clear_mapping(info);
kvfree(info->pagerefs);
mutex_destroy(>lock);
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1454,6 +1454,10 @@ __releases(>lock)
struct fb_info * const info = file->private_data;
lock_fb_info(info);
+#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
+ if (info->fbdefio)
+ fb_deferred_io_release(info);
+#endif
if (info->fbops->fb_release)
info->fbops->fb_release(info,1);
module_put(info->fbops->owner);
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -662,6 +662,7 @@ extern int fb_deferred_io_init(struct fb_info *info);
extern void fb_deferred_io_open(struct fb_info *info,
struct inode *inode,
struct file *file);
+extern void fb_deferred_io_release(struct fb_info *info);
extern void fb_deferred_io_cleanup(struct fb_info *info);
extern int fb_deferred_io_fsync(struct file *file, loff_t start,
loff_t end, int datasync);
--
2.35.3
>
> > > Reviewed-by: Patrik Jakobsson
> > > Cc:
> > > Signed-off-by: Takashi Iwai
> >
> > This could use a Fixes tag. It's not exactly clear to me when this
> > problem got originally introduced, but the recent refactoring seems a
> > candidate.
> >
> > Fixes: 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref struct")
>
> Hrm, this might be. Maybe Patrik can test with the revert of this?
>
> > Cc: Thomas Zimmermann
> > Cc: Javier Martinez Canillas
> > Cc: Maarten Lankhorst
> > Cc: Maxime Ripard
> > Cc: Zack Rusin
> > Cc: VMware Graphics Reviewers
> > Cc: Jaya Kumar
> > Cc: Daniel Vetter
> > Cc: "K. Y. Srinivasan"
> > Cc: Haiyang Zhang
> > Cc: Wei Liu
> > Cc: Dexuan Cui
> > Cc: Steve Glendinning
> > Cc: Bernie Thompson
> > Cc: Helge Deller
> > Cc: Andy Shevchenko
> > Cc: Greg Kroah-Hartman
> > Cc: Stephen Kitt
> > Cc: Peter Suti
> > Cc: Sam Ravnborg
> > Cc: Geert Uytterhoeven
> > Cc: ye xingchen
> > Cc: Petr Mladek
> > Cc: John Ogness
> > Cc: Tom Rix
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-fb...@vger.kernel.org
> > Cc: linux-hyp...@vger.kernel.org
> > Cc: # v5.19+
>
> Nah, please don't. Too many Cc's, literally a spam.
>
> > > ---
> > > v1->v2: Fix build error without CONFIG_FB_DEFERRED_IO
> > >
> > > drivers/video/fbdev/core/fb_defio.c | 10 +-
> > > drivers/video/fbdev/core/fbmem.c| 4
> > > include/linux/fb.h | 1 +
> > > 3 files changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/video/fbdev/core/fb_defio.c
> > > b/drivers/video/fbdev/core/fb_defio.c
> > > index c730253ab85c..583cbcf09446 100644
> > > --- a/drivers/video/fbdev/core/fb_defio.c
> > > +++ b/drivers/video/fbdev/core/fb_defio.c
Re: [PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
On Mon, 30 Jan 2023 10:28:16 +0100, Thomas Zimmermann wrote: > > Hi > > Am 30.01.23 um 09:52 schrieb Takashi Iwai: > > On Mon, 30 Jan 2023 09:28:36 +0100, > > Thomas Zimmermann wrote: > >> > >> Hi > >> > >> Am 29.01.23 um 09:28 schrieb Takashi Iwai: > >>> When a fbdev with deferred I/O is once opened and closed, the dirty > >>> pages still remain queued in the pageref list, and eventually later > >>> those may be processed in the delayed work. This may lead to a > >>> corruption of pages, hitting an Oops. > >> > >> Do you have more information on this problem? > > > > The details are in SUSE bugzilla, but that's an internal bug entry > > (and you know the number :) It happens at the following at least: > > > > - A VM is started with VGA console, no fb, on the installer > > - VM is switched to bochs drm > > - Start fbiterm on VT1, switching to the graphics mode on VT > > - Exit fbiterm, going back to the text mode on VT; > >at this moment, it gets Oops like: > > > > [ 42.338319][ T122] BUG: unable to handle page fault for address: > > e570c130 > > [ 42.340063][ T122] #PF: supervisor read access in kernel mode > > [ 42.340519][ T122] #PF: error_code(0x) - not-present page > > [ 42.340979][ T122] PGD 34c38067 P4D 34c38067 PUD 34c37067 PMD 0 > > [ 42.341456][ T122] Oops: [#1] PREEMPT SMP NOPTI > > [ 42.341853][ T122] CPU: 1 PID: 122 Comm: kworker/1:2 Not tainted > > 5.14.21-150500.5.g2ad24ee-default #1 SLE15-SP5 (unreleased) > > b7a28d028376a517e888a7ff28c5e5dede93267c > > [ 42.343000][ T122] Hardware name: QEMU Standard PC (i440FX + PIIX, > > 1996), > > BIOS rel-1.16.0-0-gd239552-rebuilt.opensuse.org 04/01/2014 > > [ 42.343929][ T122] Workqueue: events fb_deferred_io_work > > [ 42.344355][ T122] RIP: 0010:page_mapped+0x5e/0x90 > > [ 42.344743][ T122] Code: a8 01 75 d7 8b 47 30 f7 d0 c1 e8 1f c3 cc cc > > cc cc > > 48 89 df e8 33 9c 05 00 89 c1 31 c0 85 c9 74 13 eb d3 48 c1 e2 06 48 01 da > > <8b> > > 42 30 85 c0 79 c0 83 c1 01 48 8b 33 48 63 d1 b8 01 00 00 00 f7 > > [ 42.346285][ T122] RSP: 0018:b68640207e08 EFLAGS: 00010286 > > [ 42.346749][ T122] RAX: b3aea8f0 RBX: e570c0f0 RCX: > > 4000 > > [ 42.347355][ T122] RDX: e570c100 RSI: 000fc0010009 RDI: > > e570c0f0 > > [ 42.347960][ T122] RBP: c0503050 R08: R09: > > 0001 > > [ 42.348568][ T122] R10: R11: b68640207c88 R12: > > c0503020 > > [ 42.349180][ T122] R13: 921281dcdc00 R14: 9212bcf08000 R15: > > e570c0f0 > > [ 42.349789][ T122] FS: () GS:9212b3b0() > > knlGS: > > [ 42.350471][ T122] CS: 0010 DS: ES: CR0: 80050033 > > [ 42.350975][ T122] CR2: e570c130 CR3: 1b81 CR4: > > 06e0 > > [ 42.351588][ T122] Call Trace: > > [ 42.351845][ T122] > > [ 42.352069][ T122] page_mkclean+0x6e/0xc0 > > [ 42.352400][ T122] ? page_referenced_one+0x190/0x190 > > [ 42.353714][ T122] ? pmdp_collapse_flush+0x60/0x60 > > [ 42.354106][ T122] fb_deferred_io_work+0x13d/0x190 > > [ 42.354496][ T122] process_one_work+0x267/0x440 > > [ 42.354866][ T122] ? process_one_work+0x440/0x440 > > [ 42.355247][ T122] worker_thread+0x2d/0x3d0 > > [ 42.355590][ T122] ? process_one_work+0x440/0x440 > > [ 42.355972][ T122] kthread+0x156/0x180 > > [ 42.356281][ T122] ? set_kthread_struct+0x50/0x50 > > [ 42.356662][ T122] ret_from_fork+0x22/0x30 > > [ 42.357006][ T122] > > > > The page info shows that it's a compound page but it's somehow > > broken. On VM, it's triggered reliably with the scenario above, > > always at the same position. > > > > FWIW, the Oops is hit even if there is no rewrite on the screen. > > That is, another procedure is: > > - Start VM, run fbiterm on VT1 > > - Switch to VT2, text mode > > - On VT2, kill fbiterm; the crash still happens even if no screen > >change is performed > > > >> The mmap'ed buffer of the fbdev device comes from a vmalloc call. That > >> memory's location never changes; even across pairs of open/close on > >> the device file. I'm surprised that a page entry becomes invalid. > >> > >> In drm_fbdev_cleanup(), we first remove the fbdefio at [1] and then > >> vfree() the sh
Re: [PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
On Mon, 30 Jan 2023 09:28:36 +0100, Thomas Zimmermann wrote: > > Hi > > Am 29.01.23 um 09:28 schrieb Takashi Iwai: > > When a fbdev with deferred I/O is once opened and closed, the dirty > > pages still remain queued in the pageref list, and eventually later > > those may be processed in the delayed work. This may lead to a > > corruption of pages, hitting an Oops. > > Do you have more information on this problem? The details are in SUSE bugzilla, but that's an internal bug entry (and you know the number :) It happens at the following at least: - A VM is started with VGA console, no fb, on the installer - VM is switched to bochs drm - Start fbiterm on VT1, switching to the graphics mode on VT - Exit fbiterm, going back to the text mode on VT; at this moment, it gets Oops like: [ 42.338319][ T122] BUG: unable to handle page fault for address: e570c130 [ 42.340063][ T122] #PF: supervisor read access in kernel mode [ 42.340519][ T122] #PF: error_code(0x) - not-present page [ 42.340979][ T122] PGD 34c38067 P4D 34c38067 PUD 34c37067 PMD 0 [ 42.341456][ T122] Oops: [#1] PREEMPT SMP NOPTI [ 42.341853][ T122] CPU: 1 PID: 122 Comm: kworker/1:2 Not tainted 5.14.21-150500.5.g2ad24ee-default #1 SLE15-SP5 (unreleased) b7a28d028376a517e888a7ff28c5e5dede93267c [ 42.343000][ T122] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552-rebuilt.opensuse.org 04/01/2014 [ 42.343929][ T122] Workqueue: events fb_deferred_io_work [ 42.344355][ T122] RIP: 0010:page_mapped+0x5e/0x90 [ 42.344743][ T122] Code: a8 01 75 d7 8b 47 30 f7 d0 c1 e8 1f c3 cc cc cc cc 48 89 df e8 33 9c 05 00 89 c1 31 c0 85 c9 74 13 eb d3 48 c1 e2 06 48 01 da <8b> 42 30 85 c0 79 c0 83 c1 01 48 8b 33 48 63 d1 b8 01 00 00 00 f7 [ 42.346285][ T122] RSP: 0018:b68640207e08 EFLAGS: 00010286 [ 42.346749][ T122] RAX: b3aea8f0 RBX: e570c0f0 RCX: 4000 [ 42.347355][ T122] RDX: e570c100 RSI: 000fc0010009 RDI: e570c0f0 [ 42.347960][ T122] RBP: c0503050 R08: R09: 0001 [ 42.348568][ T122] R10: R11: b68640207c88 R12: c0503020 [ 42.349180][ T122] R13: 921281dcdc00 R14: 9212bcf08000 R15: e570c0f0 [ 42.349789][ T122] FS: () GS:9212b3b0() knlGS: [ 42.350471][ T122] CS: 0010 DS: ES: CR0: 80050033 [ 42.350975][ T122] CR2: e570c130 CR3: 1b81 CR4: 06e0 [ 42.351588][ T122] Call Trace: [ 42.351845][ T122] [ 42.352069][ T122] page_mkclean+0x6e/0xc0 [ 42.352400][ T122] ? page_referenced_one+0x190/0x190 [ 42.353714][ T122] ? pmdp_collapse_flush+0x60/0x60 [ 42.354106][ T122] fb_deferred_io_work+0x13d/0x190 [ 42.354496][ T122] process_one_work+0x267/0x440 [ 42.354866][ T122] ? process_one_work+0x440/0x440 [ 42.355247][ T122] worker_thread+0x2d/0x3d0 [ 42.355590][ T122] ? process_one_work+0x440/0x440 [ 42.355972][ T122] kthread+0x156/0x180 [ 42.356281][ T122] ? set_kthread_struct+0x50/0x50 [ 42.356662][ T122] ret_from_fork+0x22/0x30 [ 42.357006][ T122] The page info shows that it's a compound page but it's somehow broken. On VM, it's triggered reliably with the scenario above, always at the same position. FWIW, the Oops is hit even if there is no rewrite on the screen. That is, another procedure is: - Start VM, run fbiterm on VT1 - Switch to VT2, text mode - On VT2, kill fbiterm; the crash still happens even if no screen change is performed > The mmap'ed buffer of the fbdev device comes from a vmalloc call. That > memory's location never changes; even across pairs of open/close on > the device file. I'm surprised that a page entry becomes invalid. > > In drm_fbdev_cleanup(), we first remove the fbdefio at [1] and then > vfree() the shadow buffer. So the memory should still be around until > fbdevio is gone. Yes, that's the puzzling part, too. Also, another thing is that the bug couldn't be triggered easily when the fb is started in a different way. e.g. when you run fbiterm & exit on the VM that had efifb, it didn't hit. So, overall, it might be that I'm scratching a wrong surface. But at least it "fixes" the problem above apparently, and the deferred io base code itself has certainly the potential problem in general as my patch suggests. > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_fb_helper.c#L2146 > > > > > This patch makes sure to cancel the delayed work and clean up the > > pageref list at closing the device for addressing the bug. A part of > > the cleanup code is factored out as a new helper function that is > > called from the common fb_release(). > > The delayed work is required to copy the framebuffer to the de
[PATCH v2] fbdev: Fix invalid page access after closing deferred I/O devices
When a fbdev with deferred I/O is once opened and closed, the dirty
pages still remain queued in the pageref list, and eventually later
those may be processed in the delayed work. This may lead to a
corruption of pages, hitting an Oops.
This patch makes sure to cancel the delayed work and clean up the
pageref list at closing the device for addressing the bug. A part of
the cleanup code is factored out as a new helper function that is
called from the common fb_release().
Reviewed-by: Patrik Jakobsson
Cc:
Signed-off-by: Takashi Iwai
---
v1->v2: Fix build error without CONFIG_FB_DEFERRED_IO
drivers/video/fbdev/core/fb_defio.c | 10 +-
drivers/video/fbdev/core/fbmem.c| 4
include/linux/fb.h | 1 +
3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c
b/drivers/video/fbdev/core/fb_defio.c
index c730253ab85c..583cbcf09446 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -313,7 +313,7 @@ void fb_deferred_io_open(struct fb_info *info,
}
EXPORT_SYMBOL_GPL(fb_deferred_io_open);
-void fb_deferred_io_cleanup(struct fb_info *info)
+void fb_deferred_io_release(struct fb_info *info)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
struct page *page;
@@ -327,6 +327,14 @@ void fb_deferred_io_cleanup(struct fb_info *info)
page = fb_deferred_io_page(info, i);
page->mapping = NULL;
}
+}
+EXPORT_SYMBOL_GPL(fb_deferred_io_release);
+
+void fb_deferred_io_cleanup(struct fb_info *info)
+{
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+
+ fb_deferred_io_release(info);
kvfree(info->pagerefs);
mutex_destroy(>lock);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 3a6c8458eb8d..ab3545a00abc 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1454,6 +1454,10 @@ __releases(>lock)
struct fb_info * const info = file->private_data;
lock_fb_info(info);
+#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
+ if (info->fbdefio)
+ fb_deferred_io_release(info);
+#endif
if (info->fbops->fb_release)
info->fbops->fb_release(info,1);
module_put(info->fbops->owner);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 96b96323e9cb..73eb1f85ea8e 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -662,6 +662,7 @@ extern int fb_deferred_io_init(struct fb_info *info);
extern void fb_deferred_io_open(struct fb_info *info,
struct inode *inode,
struct file *file);
+extern void fb_deferred_io_release(struct fb_info *info);
extern void fb_deferred_io_cleanup(struct fb_info *info);
extern int fb_deferred_io_fsync(struct file *file, loff_t start,
loff_t end, int datasync);
--
2.35.3
Re: [PATCH] drm/nouveau/mmu: fix Use after Free bug in nvkm_vmm_node_split
On Sat, 28 Jan 2023 03:17:15 +0100,
Danilo Krummrich wrote:
>
> On Fri, Jan 27, 2023 at 01:10:46PM +0100, Takashi Iwai wrote:
> > On Tue, 03 Jan 2023 15:07:55 +0100,
> > Takashi Iwai wrote:
> > >
> > > On Fri, 30 Dec 2022 08:27:58 +0100,
> > > Zheng Wang wrote:
> > > >
> > > > Here is a function call chain.
> > > > nvkm_vmm_pfn_map->nvkm_vmm_pfn_split_merge->nvkm_vmm_node_split
> > > > If nvkm_vma_tail return NULL in nvkm_vmm_node_split, it will
> > > > finally invoke nvkm_vmm_node_merge->nvkm_vmm_node_delete, which
> > > > will free the vma. However, nvkm_vmm_pfn_map didn't notice that.
> > > > It goes into next label and UAF happens.
> > > >
> > > > Fix it by returning the return-value of nvkm_vmm_node_merge
> > > > instead of NULL.
> > > >
> > > > Signed-off-by: Zheng Wang
> > >
> > > FWIW, CVE-2023-0030 has been assigned to this bug.
> > > It's a question whether it really deserves as a security issue, but a
> > > bug is a bug...
> > >
> > > Ben, could you review this please?
> >
> > A gentle ping as reminder. The bug is still present.
>
> This was also reported in [1]. I had a closer look and FWICT this code is fine
> and there isn't a bug.
>
> Zheng Wang, the reporter of the BZ, also confirmed this to be a false positive
> from CodeQL.
>
> Anyway, here's the explaination I also posted in the BZ:
>
> "In nvkm_vmm_node_merge() nvkm_vmm_node_delete() is only called when prev is
> set. However, prev is NULL unless we enter the "if (vma->addr != addr)" path
> in
> nvkm_vmm_node_split(). In such a case the vma pointer, which is also passed to
> nvkm_vmm_node_merge(), is set to a freshly allocated struct nvkm_vma with
> nvkm_vma_tail() right before prev is set to the old vma pointer.
>
> Hence, the only thing happening there when nvkm_vma_tail() fails in the
> "if (vma->size != size)" path is that either nvkm_vmm_node_merge() does
> nothing
> in case prev wasn't set or it merges and frees the new vma created in the
> "if (vma->addr != addr)" path. Or in other words the proper cleanup for the
> error condition is done.
>
> I can't see any case where the original vma pointer given by
> nvkm_vmm_pfn_map()
> is actually freed."
>
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=2157041
Thanks for the information! Then we should try to dispute the CVE.
I'll ask our security team.
Takashi
>
> - Danilo
>
> >
> >
> > thanks,
> >
> > Takashi
> >
> > >
> > >
> > > thanks,
> > >
> > > Takashi
> > >
> > > > ---
> > > > drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
> > > > b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
> > > > index ae793f400ba1..84d6fc87b2e8 100644
> > > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
> > > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
> > > > @@ -937,8 +937,8 @@ nvkm_vmm_node_split(struct nvkm_vmm *vmm,
> > > > if (vma->size != size) {
> > > > struct nvkm_vma *tmp;
> > > > if (!(tmp = nvkm_vma_tail(vma, vma->size - size))) {
> > > > - nvkm_vmm_node_merge(vmm, prev, vma, NULL,
> > > > vma->size);
> > > > - return NULL;
> > > > + tmp = nvkm_vmm_node_merge(vmm, prev, vma, NULL,
> > > > vma->size);
> > > > + return tmp;
> > > > }
> > > > tmp->part = true;
> > > > nvkm_vmm_node_insert(vmm, tmp);
> > > > --
> > > > 2.25.1
> > > >
> >
>
[PATCH] fbdev: Fix invalid page access after closing deferred I/O devices
When a fbdev with deferred I/O is once opened and closed, the dirty
pages still remain queued in the pageref list, and eventually later
those may be processed in the delayed work. This may lead to a
corruption of pages, hitting an Oops.
This patch makes sure to cancel the delayed work and clean up the
pageref list at closing the device for addressing the bug. A part of
the cleanup code is factored out as a new helper function that is
called from the common fb_release().
Cc:
Signed-off-by: Takashi Iwai
---
drivers/video/fbdev/core/fb_defio.c | 10 +-
drivers/video/fbdev/core/fbmem.c| 2 ++
include/linux/fb.h | 1 +
3 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c
b/drivers/video/fbdev/core/fb_defio.c
index c730253ab85c..583cbcf09446 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -313,7 +313,7 @@ void fb_deferred_io_open(struct fb_info *info,
}
EXPORT_SYMBOL_GPL(fb_deferred_io_open);
-void fb_deferred_io_cleanup(struct fb_info *info)
+void fb_deferred_io_release(struct fb_info *info)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
struct page *page;
@@ -327,6 +327,14 @@ void fb_deferred_io_cleanup(struct fb_info *info)
page = fb_deferred_io_page(info, i);
page->mapping = NULL;
}
+}
+EXPORT_SYMBOL_GPL(fb_deferred_io_release);
+
+void fb_deferred_io_cleanup(struct fb_info *info)
+{
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+
+ fb_deferred_io_release(info);
kvfree(info->pagerefs);
mutex_destroy(>lock);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 3a6c8458eb8d..78c4cb5ee7c9 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1454,6 +1454,8 @@ __releases(>lock)
struct fb_info * const info = file->private_data;
lock_fb_info(info);
+ if (info->fbdefio)
+ fb_deferred_io_release(info);
if (info->fbops->fb_release)
info->fbops->fb_release(info,1);
module_put(info->fbops->owner);
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 96b96323e9cb..73eb1f85ea8e 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -662,6 +662,7 @@ extern int fb_deferred_io_init(struct fb_info *info);
extern void fb_deferred_io_open(struct fb_info *info,
struct inode *inode,
struct file *file);
+extern void fb_deferred_io_release(struct fb_info *info);
extern void fb_deferred_io_cleanup(struct fb_info *info);
extern int fb_deferred_io_fsync(struct file *file, loff_t start,
loff_t end, int datasync);
--
2.35.3
Re: [PATCH] drm/nouveau/mmu: fix Use after Free bug in nvkm_vmm_node_split
On Tue, 03 Jan 2023 15:07:55 +0100,
Takashi Iwai wrote:
>
> On Fri, 30 Dec 2022 08:27:58 +0100,
> Zheng Wang wrote:
> >
> > Here is a function call chain.
> > nvkm_vmm_pfn_map->nvkm_vmm_pfn_split_merge->nvkm_vmm_node_split
> > If nvkm_vma_tail return NULL in nvkm_vmm_node_split, it will
> > finally invoke nvkm_vmm_node_merge->nvkm_vmm_node_delete, which
> > will free the vma. However, nvkm_vmm_pfn_map didn't notice that.
> > It goes into next label and UAF happens.
> >
> > Fix it by returning the return-value of nvkm_vmm_node_merge
> > instead of NULL.
> >
> > Signed-off-by: Zheng Wang
>
> FWIW, CVE-2023-0030 has been assigned to this bug.
> It's a question whether it really deserves as a security issue, but a
> bug is a bug...
>
> Ben, could you review this please?
A gentle ping as reminder. The bug is still present.
thanks,
Takashi
>
>
> thanks,
>
> Takashi
>
> > ---
> > drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
> > b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
> > index ae793f400ba1..84d6fc87b2e8 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
> > @@ -937,8 +937,8 @@ nvkm_vmm_node_split(struct nvkm_vmm *vmm,
> > if (vma->size != size) {
> > struct nvkm_vma *tmp;
> > if (!(tmp = nvkm_vma_tail(vma, vma->size - size))) {
> > - nvkm_vmm_node_merge(vmm, prev, vma, NULL, vma->size);
> > - return NULL;
> > + tmp = nvkm_vmm_node_merge(vmm, prev, vma, NULL,
> > vma->size);
> > + return tmp;
> > }
> > tmp->part = true;
> > nvkm_vmm_node_insert(vmm, tmp);
> > --
> > 2.25.1
> >
Re: [PATCH 04/22] sound: remove sound/sh
On Fri, 13 Jan 2023 07:23:21 +0100,
Christoph Hellwig wrote:
>
> Now that arch/sh is removed these drivers are dead code.
>
> Signed-off-by: Christoph Hellwig
Supposed you take in your tree:
Acked-by: Takashi Iwai
thanks,
Takashi
> ---
> sound/Kconfig | 2 -
> sound/Makefile | 2 +-
> sound/sh/Kconfig| 32 --
> sound/sh/Makefile | 11 -
> sound/sh/aica.c | 628
> sound/sh/aica.h | 68 -
> sound/sh/sh_dac_audio.c | 412 --
> 7 files changed, 1 insertion(+), 1154 deletions(-)
> delete mode 100644 sound/sh/Kconfig
> delete mode 100644 sound/sh/Makefile
> delete mode 100644 sound/sh/aica.c
> delete mode 100644 sound/sh/aica.h
> delete mode 100644 sound/sh/sh_dac_audio.c
>
> diff --git a/sound/Kconfig b/sound/Kconfig
> index e56d96d2b11cae..14361bb428baa1 100644
> --- a/sound/Kconfig
> +++ b/sound/Kconfig
> @@ -75,8 +75,6 @@ source "sound/spi/Kconfig"
>
> source "sound/mips/Kconfig"
>
> -source "sound/sh/Kconfig"
> -
> # the following will depend on the order of config.
> # here assuming USB is defined before ALSA
> source "sound/usb/Kconfig"
> diff --git a/sound/Makefile b/sound/Makefile
> index 04ef04b1168f39..bb4b8806321c67 100644
> --- a/sound/Makefile
> +++ b/sound/Makefile
> @@ -4,7 +4,7 @@
>
> obj-$(CONFIG_SOUND) += soundcore.o
> obj-$(CONFIG_DMASOUND) += oss/dmasound/
> -obj-$(CONFIG_SND) += core/ i2c/ drivers/ isa/ pci/ ppc/ arm/ sh/ synth/ usb/
> \
> +obj-$(CONFIG_SND) += core/ i2c/ drivers/ isa/ pci/ ppc/ arm/ synth/ usb/ \
> firewire/ sparc/ spi/ parisc/ pcmcia/ mips/ soc/ atmel/ hda/ x86/ xen/ \
> virtio/
> obj-$(CONFIG_SND_AOA) += aoa/
> diff --git a/sound/sh/Kconfig b/sound/sh/Kconfig
> deleted file mode 100644
> index b75fbb3236a7b9..00
> --- a/sound/sh/Kconfig
> +++ /dev/null
> @@ -1,32 +0,0 @@
> -# SPDX-License-Identifier: GPL-2.0-only
> -# ALSA SH drivers
> -
> -menuconfig SND_SUPERH
> - bool "SUPERH sound devices"
> - depends on SUPERH
> - default y
> - help
> - Support for sound devices specific to SUPERH architectures.
> - Drivers that are implemented on ASoC can be found in
> - "ALSA for SoC audio support" section.
> -
> -if SND_SUPERH
> -
> -config SND_AICA
> - tristate "Dreamcast Yamaha AICA sound"
> - depends on SH_DREAMCAST
> - select SND_PCM
> - select G2_DMA
> - help
> - ALSA Sound driver for the SEGA Dreamcast console.
> -
> -config SND_SH_DAC_AUDIO
> - tristate "SuperH DAC audio support"
> - depends on SND
> - depends on CPU_SH3 && HIGH_RES_TIMERS
> - select SND_PCM
> - help
> - Say Y here to include support for the on-chip DAC.
> -
> -endif# SND_SUPERH
> -
> diff --git a/sound/sh/Makefile b/sound/sh/Makefile
> deleted file mode 100644
> index c0bbc500c17c73..00
> --- a/sound/sh/Makefile
> +++ /dev/null
> @@ -1,11 +0,0 @@
> -# SPDX-License-Identifier: GPL-2.0-only
> -#
> -# Makefile for ALSA
> -#
> -
> -snd-aica-objs := aica.o
> -snd-sh_dac_audio-objs := sh_dac_audio.o
> -
> -# Toplevel Module Dependency
> -obj-$(CONFIG_SND_AICA) += snd-aica.o
> -obj-$(CONFIG_SND_SH_DAC_AUDIO) += snd-sh_dac_audio.o
> diff --git a/sound/sh/aica.c b/sound/sh/aica.c
> deleted file mode 100644
> index 6e9d6bd67369af..00
> --- a/sound/sh/aica.c
> +++ /dev/null
> @@ -1,628 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> -*
> -* Copyright Adrian McMenamin 2005, 2006, 2007
> -*
> -* Requires firmware (BSD licenced) available from:
> -*
> http://linuxdc.cvs.sourceforge.net/linuxdc/linux-sh-dc/sound/oss/aica/firmware/
> -* or the maintainer
> -*/
> -
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include
> -#include "aica.h"
> -
> -MODULE_AUTHOR("Adrian McMenamin ");
> -MODULE_DESCRIPTION("Dreamcast AICA sound (pcm) driver");
> -MODULE_LICENSE("GPL");
> -MODULE_FIRMWARE("aica_firmware.bin");
> -
> -/* module parameters */
> -#define CARD_NAME "AICA"
> -static int index = -1;
> -static char *id;
> -static bool enable = 1;
> -module_param(index, int, 0444);
> -MODULE_PARM_DESC(index, "Index value for " CARD_NAM
Re: [PATCH] drm/nouveau/mmu: fix Use after Free bug in nvkm_vmm_node_split
On Fri, 30 Dec 2022 08:27:58 +0100,
Zheng Wang wrote:
>
> Here is a function call chain.
> nvkm_vmm_pfn_map->nvkm_vmm_pfn_split_merge->nvkm_vmm_node_split
> If nvkm_vma_tail return NULL in nvkm_vmm_node_split, it will
> finally invoke nvkm_vmm_node_merge->nvkm_vmm_node_delete, which
> will free the vma. However, nvkm_vmm_pfn_map didn't notice that.
> It goes into next label and UAF happens.
>
> Fix it by returning the return-value of nvkm_vmm_node_merge
> instead of NULL.
>
> Signed-off-by: Zheng Wang
FWIW, CVE-2023-0030 has been assigned to this bug.
It's a question whether it really deserves as a security issue, but a
bug is a bug...
Ben, could you review this please?
thanks,
Takashi
> ---
> drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
> index ae793f400ba1..84d6fc87b2e8 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c
> @@ -937,8 +937,8 @@ nvkm_vmm_node_split(struct nvkm_vmm *vmm,
> if (vma->size != size) {
> struct nvkm_vma *tmp;
> if (!(tmp = nvkm_vma_tail(vma, vma->size - size))) {
> - nvkm_vmm_node_merge(vmm, prev, vma, NULL, vma->size);
> - return NULL;
> + tmp = nvkm_vmm_node_merge(vmm, prev, vma, NULL,
> vma->size);
> + return tmp;
> }
> tmp->part = true;
> nvkm_vmm_node_insert(vmm, tmp);
> --
> 2.25.1
>
[PATCH] drm: radeon: Fix audio get_eld callback
Check the availability of the audio capability and mode config before
going to the loop for avoiding the access to an unusable state. Also,
change the loop iterations over encoder instead of connector in order
to align with radeon_audio_enable().
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2236
Signed-off-by: Takashi Iwai
---
Note: this is the additional fix on top of the previously submitted
audio component support for radeon.
drivers/gpu/drm/radeon/radeon_audio.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_audio.c
b/drivers/gpu/drm/radeon/radeon_audio.c
index 71b67d4efe08..d6ccaf24ee0c 100644
--- a/drivers/gpu/drm/radeon/radeon_audio.c
+++ b/drivers/gpu/drm/radeon/radeon_audio.c
@@ -747,6 +747,7 @@ static int radeon_audio_component_get_eld(struct device
*kdev, int port,
unsigned char *buf, int max_bytes)
{
struct drm_device *dev = dev_get_drvdata(kdev);
+ struct radeon_device *rdev = dev->dev_private;
struct drm_encoder *encoder;
struct radeon_encoder *radeon_encoder;
struct radeon_encoder_atom_dig *dig;
@@ -754,19 +755,19 @@ static int radeon_audio_component_get_eld(struct device
*kdev, int port,
int ret = 0;
*enabled = false;
- list_for_each_entry(connector, >mode_config.connector_list, head) {
- const struct drm_connector_helper_funcs *connector_funcs =
- connector->helper_private;
- encoder = connector_funcs->best_encoder(connector);
+ if (!rdev->audio.enabled || !rdev->mode_info.mode_config_initialized)
+ return 0;
- if (!encoder)
- continue;
+ list_for_each_entry(encoder, >ddev->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)
+ continue;
*enabled = true;
ret = drm_eld_size(connector->eld);
memcpy(buf, connector->eld, min(max_bytes, ret));
--
2.35.3
[PATCH] drm/radeon: Use a local mutex for bind/unbind protection
We used drm_modeset_lock_all() for protecting the audio component
bind/unbind operations against the notification, but it seems leading
to kernel WARNING and Oops. Moreover, the use of
drm_modeset_lock_all() is rather an overkill only for this
protection.
This patch introduces a new mutex that protects just the bind/unbind
and the notify calls and replaces the drm_modeset_lock with it.
Fixes: 34d84636e5e0 ("drm/radeon: Add HD-audio component notifier support (v4)")
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1569
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/radeon/radeon.h| 1 +
drivers/gpu/drm/radeon/radeon_audio.c | 21 -
drivers/gpu/drm/radeon/radeon_device.c | 1 +
3 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index d82424525f5a..2e7161acd443 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -1799,6 +1799,7 @@ struct r600_audio {
struct radeon_audio_basic_funcs *funcs;
struct drm_audio_component *component;
bool component_registered;
+ struct mutex component_mutex;
};
/*
diff --git a/drivers/gpu/drm/radeon/radeon_audio.c
b/drivers/gpu/drm/radeon/radeon_audio.c
index 4ceb90556127..71b67d4efe08 100644
--- a/drivers/gpu/drm/radeon/radeon_audio.c
+++ b/drivers/gpu/drm/radeon/radeon_audio.c
@@ -181,8 +181,7 @@ 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);
+static void radeon_audio_component_notify(struct radeon_device *rdev, int
port);
static void radeon_audio_enable(struct radeon_device *rdev,
struct r600_audio_pin *pin, u8 enable_mask)
@@ -212,7 +211,7 @@ 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);
+ radeon_audio_component_notify(rdev, pin->id);
}
static void radeon_audio_interface_init(struct radeon_device *rdev)
@@ -731,12 +730,16 @@ unsigned int radeon_audio_decode_dfs_div(unsigned int div)
/*
* Audio component support
*/
-static void radeon_audio_component_notify(struct drm_audio_component *acomp,
- int port)
+static void radeon_audio_component_notify(struct radeon_device *rdev, int port)
{
+ struct drm_audio_component *acomp;
+
+ mutex_lock(>audio.component_mutex);
+ acomp = rdev->audio.component;
if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr,
port, -1);
+ mutex_unlock(>audio.component_mutex);
}
static int radeon_audio_component_get_eld(struct device *kdev, int port,
@@ -787,11 +790,11 @@ static int radeon_audio_component_bind(struct device
*kdev,
if (WARN_ON(!device_link_add(hda_kdev, kdev, DL_FLAG_STATELESS)))
return -ENOMEM;
- drm_modeset_lock_all(dev);
+ mutex_lock(>audio.component_mutex);
acomp->ops = _audio_component_ops;
acomp->dev = kdev;
rdev->audio.component = acomp;
- drm_modeset_unlock_all(dev);
+ mutex_unlock(>audio.component_mutex);
return 0;
}
@@ -805,11 +808,11 @@ static void radeon_audio_component_unbind(struct device
*kdev,
device_link_remove(hda_kdev, kdev);
- drm_modeset_lock_all(dev);
+ mutex_lock(>audio.component_mutex);
rdev->audio.component = NULL;
acomp->ops = NULL;
acomp->dev = NULL;
- drm_modeset_unlock_all(dev);
+ mutex_unlock(>audio.component_mutex);
}
static const struct component_ops radeon_audio_component_bind_ops = {
diff --git a/drivers/gpu/drm/radeon/radeon_device.c
b/drivers/gpu/drm/radeon/radeon_device.c
index 17bfbbe906c8..2e13ce2b65d3 100644
--- a/drivers/gpu/drm/radeon/radeon_device.c
+++ b/drivers/gpu/drm/radeon/radeon_device.c
@@ -1312,6 +1312,7 @@ int radeon_device_init(struct radeon_device *rdev,
mutex_init(>pm.mutex);
mutex_init(>gpu_clock_mutex);
mutex_init(>srbm_mutex);
+ mutex_init(>audio.component_mutex);
init_rwsem(>pm.mclk_lock);
init_rwsem(>exclusive_lock);
init_waitqueue_head(>irq.vblank_queue);
--
2.35.3
[PATCH] drm/radeon: Add HD-audio component notifier support (v4)
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.
Also, as a gratis bonus, this patch "fixes" the regression by the
recent change in HD-audio to be more strict for the HDMI/DP
connection, too. Since the HD-audio HDMI/DP codec requires both the
connection bit and the valid ELD to be provided, it started failing on
some RADEON gfx boards where the ELD update performed instably. As
this change switches the communication to a direct way between the
audio and the graphics drivers, now the system receives the proper
ELD, and the HDMI/DP hotplug starts working again.
[ v2: fix the logic in radeon_audio_component_get_eld to walk the
connector list since that is where the EDID lives and we can
derive the encoder from the connector because the encoder has
not been assigned at this point (i.e., during monitor probe).
v3: the component binding is moved outside radeon_audio_init() and
_fini(), as those are called from suspend/resume, too.
Drop modeset lock calls that caused Oops.
Moved Kconfig change so that it can be applied on older kernels.
v4: revive drm_modeset_lock*() again, add the missing
device_link_remove() call at unbinding ]
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1569
Signed-off-by: Takashi Iwai
---
drivers/gpu/drm/Kconfig| 1 +
drivers/gpu/drm/radeon/radeon.h| 7 ++
drivers/gpu/drm/radeon/radeon_audio.c | 113 +
drivers/gpu/drm/radeon/radeon_device.c | 3 +
4 files changed, 124 insertions(+)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 34f5a092c99e..fa986075e8fb 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -243,6 +243,7 @@ config DRM_RADEON
select DRM_KMS_HELPER
select DRM_TTM
select DRM_TTM_HELPER
+ select SND_HDA_COMPONENT if SND_HDA_CORE
select POWER_SUPPLY
select HWMON
select BACKLIGHT_CLASS_DEVICE
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 166c18d62f6d..d82424525f5a 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -79,6 +79,7 @@
#include
#include
+#include
#include "radeon_family.h"
#include "radeon_mode.h"
@@ -1796,6 +1797,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;
};
/*
@@ -2994,6 +2997,10 @@ void radeon_irq_kms_set_irq_n_enabled(struct
radeon_device *rdev,
bool enable, const char *name,
unsigned n);
+/* Audio component binding */
+void radeon_audio_component_init(struct radeon_device *rdev);
+void radeon_audio_component_fini(struct radeon_device *rdev);
+
#include "radeon_object.h"
#endif
diff --git a/drivers/gpu/drm/radeon/radeon_audio.c
b/drivers/gpu/drm/radeon/radeon_audio.c
index 7c5e80d03fc9..4ceb90556127 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 "dce6_afmt.h"
@@ -180,6 +181,9 @@ 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);
+
static void radeon_audio_enable(struct radeon_device *rdev,
struct r600_audio_pin *pin, u8 enable_mask)
{
@@ -207,6 +211,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)
@@ -721,3 +727,110 @@ unsigned int radeon_audio_decode_dfs_div(unsigned int div)
else
return 0;
}
+
+/*
+ * Audio component support
+ */
+static void radeon_audio_component_notify(struct drm_aud
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 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(, 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, , (char *)vaddr,
> >, 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 = _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,
return ret;
log
[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(, x, y, width, height);
- if (ret)
- return ret;
- else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
- return -EINVAL;
+ drm_rect_init(, 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 *) [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 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(>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(>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 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 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(, 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, , (char *)vaddr,
, 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 = _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, _plane_state->data[0], 0, 0, fb->width,
fb->height);
+ drm_rect_init(, 0, 0, fb->width, fb->height);
+ udl_handle_damage(fb, _plane_state->data[0], );
/* 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, ))
- udl_handle_damage(fb, _plane_state->data[0], rect.x1,
rect.y1,
- rect.x2 - rect.x1, rect.y2 - rect.y1);
+ udl_handle_damage(fb, _plane_state->data[0], );
}
static const struct drm_simple_display_pipe_funcs
udl_simple_display_pipe_funcs = {
--
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(>urbs.lock);
+ urb = udl_get_urb_locked(udl, MAX_SCHEDULE_TIMEOUT);
+ udl->urbs.count--;
+ spin_unlock_irq(>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(>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(>urbs.lock);
/* Wait for an in-flight buffer to complete and get re-queued */
- spin_lock_irq(>urbs.lock);
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
+!udl->urbs.count ||
!list_empty(>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(>urbs.list, struct urb_node, entry);
list_del_init(>entry);
udl->urbs.available--;
-unlock:
- spin_unlock_irq(>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(>urbs.lock);
+ urb = udl_get_urb_locked(udl, GET_URB_TIMEOU
[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(>urbs.lock);
-
-retry:
- udl->urbs.size = size;
INIT_LIST_HEAD(>urbs.list);
-
init_waitqueue_head(>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 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 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(>display_pipe.plane); drm_mode_config_reset(dev); -- 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 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, _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 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(>urbs.lock, flags);
- list_move(>entry, >urbs.list);
+ list_add_tail(>entry, >urbs.list);
udl->urbs.available++;
spin_unlock_irqrestore(>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(>urbs.list);
- INIT_LIST_HEAD(>urbs.in_flight);
init_waitqueue_head(>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(>urbs.list, struct urb_node, entry);
- list_move(>entry, >urbs.in_flight);
+ list_del_init(>entry);
udl->urbs.available--;
unlock:
@@ -281,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
spin_lock_irq(>urbs.lock);
/* 2 seconds as a sane timeout */
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
-list_empty(>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(>urbs.lock);
- while (!list_empty(>urbs.in_flight)) {
- unode = list_first_entry(>urbs.in_flight,
-struct urb_node, entry);
- spin_unlock_irq(>urbs.lock);
- usb_kill_urb(unode->urb);
- spin_lock_irq(>urbs.lock);
- }
- spin_unlock_irq(>urbs.lock);
-}
-
int udl_init(struct udl_device *udl)
{
struct drm_device *dev = >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 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 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
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(, x, y, width, height);
> > - if (ret)
> > - return ret;
> > - else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
> > - return -EINVAL;
> > + drm_rect_init(, 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
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, _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 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
[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 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(>urbs.lock, flags);
- wake_up(>urbs.sleep);
+ wake_up_all(>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(>urbs.lock);
+ urb = udl_get_urb_locked(udl, MAX_SCHEDULE_TIMEOUT);
+ udl->urbs.count--;
+ spin_unlock_irq(>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(>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(>urbs.lock);
/* Wait for an in-flight buffer to complete and get re-queued */
- spin_lock_irq(>urbs.lock);
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
+!udl->urbs.count ||
!list_empty(>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(>urbs.list, struct urb_node, entry);
list_del_init(>entry);
udl->urbs.available--;
-unlock:
- spin_unlock_irq(>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
[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(, x, y, width, height);
- if (ret)
- return ret;
- else if ((clip.x2 > fb->width) || (clip.y2 > fb->height))
- return -EINVAL;
+ drm_rect_init(, 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 *) [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 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(>urbs.lock);
-
-retry:
- udl->urbs.size = size;
INIT_LIST_HEAD(>urbs.list);
-
init_waitqueue_head(>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 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 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(>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(>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 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(>display_pipe.plane); drm_mode_config_reset(dev); -- 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(>urbs.lock, flags);
- list_move(>entry, >urbs.list);
+ list_add_tail(>entry, >urbs.list);
udl->urbs.available++;
spin_unlock_irqrestore(>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(>urbs.list);
- INIT_LIST_HEAD(>urbs.in_flight);
init_waitqueue_head(>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(>urbs.list, struct urb_node, entry);
- list_move(>entry, >urbs.in_flight);
+ list_del_init(>entry);
udl->urbs.available--;
unlock:
@@ -281,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
spin_lock_irq(>urbs.lock);
/* 2 seconds as a sane timeout */
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
-list_empty(>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(>urbs.lock);
- while (!list_empty(>urbs.in_flight)) {
- unode = list_first_entry(>urbs.in_flight,
-struct urb_node, entry);
- spin_unlock_irq(>urbs.lock);
- usb_kill_urb(unode->urb);
- spin_lock_irq(>urbs.lock);
- }
- spin_unlock_irq(>urbs.lock);
-}
-
int udl_init(struct udl_device *udl)
{
struct drm_device *dev = >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 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 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 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, _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
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
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(>urbs.lock);
> > + urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
> > + udl->urbs.count--;
> > + spin_unlock_irq(>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(>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 *unode;
> > +
> > + assert_spin_locke
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(, 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
[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(>urbs.lock, flags);
- list_move(>entry, >urbs.list);
+ list_add_tail(>entry, >urbs.list);
udl->urbs.available++;
spin_unlock_irqrestore(>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(>urbs.list);
- INIT_LIST_HEAD(>urbs.in_flight);
init_waitqueue_head(>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(>urbs.list, struct urb_node, entry);
- list_move(>entry, >urbs.in_flight);
+ list_del_init(>entry);
udl->urbs.available--;
unlock:
@@ -281,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
spin_lock_irq(>urbs.lock);
/* 2 seconds as a sane timeout */
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
-list_empty(>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(>urbs.lock);
- while (!list_empty(>urbs.in_flight)) {
- unode = list_first_entry(>urbs.in_flight,
-struct urb_node, entry);
- spin_unlock_irq(>urbs.lock);
- usb_kill_urb(unode->urb);
- spin_lock_irq(>urbs.lock);
- }
- spin_unlock_irq(>urbs.lock);
-}
-
int udl_init(struct udl_device *udl)
{
struct drm_device *dev = >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: [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 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(>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(>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
[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 = >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 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 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(, 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 *) [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 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 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 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(>urbs.lock);
+ urb = __udl_get_urb(udl, MAX_SCHEDULE_TIMEOUT);
+ udl->urbs.count--;
+ spin_unlock_irq(>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(>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(>urbs.lock);
if (!udl->urbs.count)
return NULL;
/* Wait for an in-flight buffer to complete and get re-queued */
- spin_lock_irq(>urbs.lock);
if (!wait_event_lock_irq_timeout(udl->urbs.sleep,
!list_empty(>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(>urbs.list, struct urb_node, entry);
list_del_init(>entry);
udl->urbs.available--;
-unlock:
- spin_unlock_irq(>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(>urbs.lock);
+ urb = __udl_get_urb(udl, GET_URB_TIMEOUT);
+ spin_unlock_irq(>urbs.lock);
+ return urb;
+}
+
int udl_submit_urb(struct drm_device *dev, struct urb *urb, size_t len)
{
struct udl_device *udl = to_udl(dev);
--
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(>urbs.lock);
-
-retry:
- udl->urbs.size = size;
INIT_LIST_HEAD(>urbs.list);
-
init_waitqueue_head(>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 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, _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 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(>display_pipe.plane); drm_mode_config_reset(dev); -- 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 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
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(>urbs.lock, flags);
> >>>>>>>> -list_add_tail(>entry, >urbs.list);
> >>>>>>>> +list_move(>entry, >urbs.list);
> >>>>>>>> udl->urbs.available++;
> >>>>>>>> spin_unlock_irqrestore(>urbs.lock, flags);
> >>>>>>>> @@ -180,6 +180,7 @@ static int udl_alloc_urb_list(struct
> >>>>>>>> drm_device *dev, int count, size_t size)
> >>>>>>>> retry:
> >>>>>>>
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(>urbs.lock, flags);
> > >>>>> - list_add_tail(>entry, >urbs.list);
> > >>>>> + list_move(>entry, >urbs.list);
> > >>>>> udl->urbs.available++;
> > >>>>> spin_unlock_irqrestore(>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(>urbs.list);
> > >>>>> + INIT_LIST_HEAD(>urbs.in_flight);
> > >>>>> init_waitqueue_head(>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(>urbs.list, struct
> > >>>>> urb_node,
> > >>>>> entry);
> > >>>>> -
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(>urbs.lock, flags);
> >>>>> - list_add_tail(>entry, >urbs.list);
> >>>>> + list_move(>entry, >urbs.list);
> >>>>> udl->urbs.available++;
> >>>>> spin_unlock_irqrestore(>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(>urbs.list);
> >>>>> + INIT_LIST_HEAD(>urbs.in_flight);
> >>>>> init_waitqueue_head(>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(>urbs.list, struct urb_node,
> >>>>> entry);
> >>>>> - list_del_init(>entry);
> >>>>> + list_move(>entry, >urbs.in_flight);
> >>>>> udl->urbs.available--;
> >>>>> unlock:
> >>>>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>>>> spin_lock_irq(>urbs.lock);
> >>>>> /* 2 seconds as a sane timeout */
> >>>>> if (!wait_ev
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(>urbs.lock, flags);
> >>> - list_add_tail(>entry, >urbs.list);
> >>> + list_move(>entry, >urbs.list);
> >>> udl->urbs.available++;
> >>> spin_unlock_irqrestore(>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(>urbs.list);
> >>> + INIT_LIST_HEAD(>urbs.in_flight);
> >>> init_waitqueue_head(>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(>urbs.list, struct urb_node,
> >>> entry);
> >>> - list_del_init(>entry);
> >>> + list_move(>entry, >urbs.in_flight);
> >>> udl->urbs.available--;
> >>> unlock:
> >>> @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> >>> spin_lock_irq(>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(>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);
> >&
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(>urbs.lock, flags);
> > - list_add_tail(>entry, >urbs.list);
> > + list_move(>entry, >urbs.list);
> > udl->urbs.available++;
> > spin_unlock_irqrestore(>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(>urbs.list);
> > + INIT_LIST_HEAD(>urbs.in_flight);
> > init_waitqueue_head(>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(>urbs.list, struct urb_node,
> > entry);
> > - list_del_init(>entry);
> > + list_move(>entry, >urbs.in_flight);
> > udl->urbs.available--;
> > unlock:
> > @@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
> > spin_lock_irq(>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(>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(>urbs.lock);
> > + while (!list_empty(>urbs.in_flight)) {
> > + unode = list_first_entry(>urbs.in_flight,
> > +struct urb_node, entry);
> > + spin_unlock_irq(>urbs.lock);
> > + usb_kill_urb(unode->urb);
> > + spin_lock_irq(>urbs.lock);
> > + }
> > + spin_unlock_irq(>urbs.lock);
> > +}
> > +
> > int udl_init(struct udl_device *udl)
> > {
> > struct drm_device *dev = >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
[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
[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(>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(>urbs.lock);
+ return ret;
+}
+
int udl_init(struct udl_device *udl)
{
struct drm_device *dev = >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 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(>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(>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(>release_urb_work, 0);
- else
-#endif
- up(>urbs.limit_sem);
+ wake_up(>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(>urbs.limit_sem);
-
- spin_lock_irq(>urbs.lock);
-
- node = udl->urbs.list.next; /* have reserved one with sem */
- list_del_init(node);
-
- spin_unlock_irq(>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(>urbs.list);
- sema_init(>urbs.limit_sem, 0);
+ init_waitqueue_head(>urbs.sleep);
udl->urbs.count = 0;
udl->urbs.available = 0;
@@ -220,9 +191,6 @@ static int udl_alloc_urb_list(struct drm_device *dev, int
count, size_t size)
break;
[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 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(>urbs.lock, flags);
- list_add_tail(>entry, >urbs.list);
+ list_move(>entry, >urbs.list);
udl->urbs.available++;
spin_unlock_irqrestore(>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(>urbs.list);
+ INIT_LIST_HEAD(>urbs.in_flight);
init_waitqueue_head(>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(>urbs.list, struct urb_node, entry);
- list_del_init(>entry);
+ list_move(>entry, >urbs.in_flight);
udl->urbs.available--;
unlock:
@@ -279,7 +280,7 @@ int udl_sync_pending_urbs(struct drm_device *dev)
spin_lock_irq(>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(>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(>urbs.lock);
+ while (!list_empty(>urbs.in_flight)) {
+ unode = list_first_entry(>urbs.in_flight,
+struct urb_node, entry);
+ spin_unlock_irq(>urbs.lock);
+ usb_kill_urb(unode->urb);
+ spin_lock_irq(>urbs.lock);
+ }
+ spin_unlock_irq(>urbs.lock);
+}
+
int udl_init(struct udl_device *udl)
{
struct drm_device *dev = >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
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(>mode_config.connector_ida,
connector->index);
+ kfree(connector->firmware_edid);
kfree(connector->display_info.bus_formats);
drm_mode_object_unregister(dev, >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;
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: [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 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
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 v2 05/12] ASoC: hdmi-codec: Add a prepare hook
On Mon, 31 May 2021 11:42:13 +0200, Maxime Ripard wrote: > > Hi Mark, Takashi, > > On Wed, May 26, 2021 at 11:39:21AM +0100, Mark Brown wrote: > > On Tue, May 25, 2021 at 03:23:47PM +0200, Maxime Ripard wrote: > > > The IEC958 status bit is usually set by the userspace after hw_params > > > has been called, so in order to use whatever is set by the userspace, we > > > need to implement the prepare hook. Let's add it to the hdmi_codec_ops, > > > and mandate that either prepare or hw_params is implemented. > > > > Acked-by: Mark Brown > > It looks like you're both happy with the ALSA/ASoC side, how do you want > to get this merged? > > There's a build dependency between the DRM bits and the new hook > introduced in hdmi-codec, would you be ok with merging it through the > drm tree? Speaking of ALSA core changes, I'm fine with that. thanks, Takashi
Re: [PATCH v2 02/12] ALSA: iec958: Split status creation and fill
On Tue, 25 May 2021 15:23:44 +0200,
Maxime Ripard wrote:
>
> In some situations, like a codec probe, we need to provide an IEC status
> default but don't have access to the sampling rate and width yet since
> no stream has been configured yet.
>
> Each and every driver has its own default, whereas the core iec958 code
> also has some buried in the snd_pcm_create_iec958_consumer functions.
>
> Let's split these functions in two to provide a default that doesn't
> rely on the sampling rate and width, and another function to fill them
> when available.
>
> Signed-off-by: Maxime Ripard
Reviewed-by: Takashi Iwai
thanks,
Takashi
> ---
> include/sound/pcm_iec958.h | 8 ++
> sound/core/pcm_iec958.c| 176 -
> 2 files changed, 141 insertions(+), 43 deletions(-)
>
> diff --git a/include/sound/pcm_iec958.h b/include/sound/pcm_iec958.h
> index 0939aa45e2fe..64e84441cde1 100644
> --- a/include/sound/pcm_iec958.h
> +++ b/include/sound/pcm_iec958.h
> @@ -4,6 +4,14 @@
>
> #include
>
> +int snd_pcm_create_iec958_consumer_default(u8 *cs, size_t len);
> +
> +int snd_pcm_fill_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> + size_t len);
> +
> +int snd_pcm_fill_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
> +u8 *cs, size_t len);
> +
> int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
> size_t len);
>
> diff --git a/sound/core/pcm_iec958.c b/sound/core/pcm_iec958.c
> index f9a211cc1f2c..7a1b816f67cc 100644
> --- a/sound/core/pcm_iec958.c
> +++ b/sound/core/pcm_iec958.c
> @@ -9,41 +9,85 @@
> #include
> #include
>
> -static int create_iec958_consumer(uint rate, uint sample_width,
> - u8 *cs, size_t len)
> +/**
> + * snd_pcm_create_iec958_consumer_default - create default consumer format
> IEC958 channel status
> + * @cs: channel status buffer, at least four bytes
> + * @len: length of channel status buffer
> + *
> + * Create the consumer format channel status data in @cs of maximum size
> + * @len. When relevant, the configuration-dependant bits will be set as
> + * unspecified.
> + *
> + * Drivers should then call einter snd_pcm_fill_iec958_consumer() or
> + * snd_pcm_fill_iec958_consumer_hw_params() to replace these unspecified
> + * bits by their actual values.
> + *
> + * Drivers may wish to tweak the contents of the buffer after creation.
> + *
> + * Returns: length of buffer, or negative error code if something failed.
> + */
> +int snd_pcm_create_iec958_consumer_default(u8 *cs, size_t len)
> {
> - unsigned int fs, ws;
> -
> if (len < 4)
> return -EINVAL;
>
> - switch (rate) {
> - case 32000:
> - fs = IEC958_AES3_CON_FS_32000;
> - break;
> - case 44100:
> - fs = IEC958_AES3_CON_FS_44100;
> - break;
> - case 48000:
> - fs = IEC958_AES3_CON_FS_48000;
> - break;
> - case 88200:
> - fs = IEC958_AES3_CON_FS_88200;
> - break;
> - case 96000:
> - fs = IEC958_AES3_CON_FS_96000;
> - break;
> - case 176400:
> - fs = IEC958_AES3_CON_FS_176400;
> - break;
> - case 192000:
> - fs = IEC958_AES3_CON_FS_192000;
> - break;
> - default:
> + memset(cs, 0, len);
> +
> + cs[0] = IEC958_AES0_CON_NOT_COPYRIGHT | IEC958_AES0_CON_EMPHASIS_NONE;
> + cs[1] = IEC958_AES1_CON_GENERAL;
> + cs[2] = IEC958_AES2_CON_SOURCE_UNSPEC | IEC958_AES2_CON_CHANNEL_UNSPEC;
> + cs[3] = IEC958_AES3_CON_CLOCK_1000PPM | IEC958_AES3_CON_FS_NOTID;
> +
> + if (len > 4)
> + cs[4] = IEC958_AES4_CON_WORDLEN_NOTID;
> +
> + return len;
> +}
> +EXPORT_SYMBOL_GPL(snd_pcm_create_iec958_consumer_default);
> +
> +static int fill_iec958_consumer(uint rate, uint sample_width,
> + u8 *cs, size_t len)
> +{
> + if (len < 4)
> return -EINVAL;
> +
> + if ((cs[3] & IEC958_AES3_CON_FS) == IEC958_AES3_CON_FS_NOTID) {
> + unsigned int fs;
> +
> + switch (rate) {
> + case 32000:
> + fs = IEC958_AES3_CON_FS_32000;
> + break;
> + case 44100:
> + fs = IEC958_AES3_CON_FS_44100;
> + break;
> + case 48000:
> + fs = IEC958_AES3_CON_FS
Re: [PATCH v2 01/12] ALSA: doc: Clarify IEC958 controls iface
On Tue, 25 May 2021 15:23:43 +0200, Maxime Ripard wrote: > > The doc currently mentions that the IEC958 Playback Default should be > exposed on the PCM iface, and the Playback Mask on the mixer iface. > > It's a bit confusing to advise to have two related controls on two > separate ifaces, and it looks like the drivers that currently expose > those controls use any combination of the mixer and PCM ifaces. > > Let's try to clarify the situation a bit, and encourage to at least have > the controls on the same iface. > > Signed-off-by: Maxime Ripard Reviewed-by: Takashi Iwai thanks, Takashi > --- > .../sound/kernel-api/writing-an-alsa-driver.rst | 13 +++-- > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst > b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst > index e6365836fa8b..01d59b8aea92 100644 > --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst > +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst > @@ -3508,14 +3508,15 @@ field must be set, though). > > “IEC958 Playback Con Mask” is used to return the bit-mask for the IEC958 > status bits of consumer mode. Similarly, “IEC958 Playback Pro Mask” > -returns the bitmask for professional mode. They are read-only controls, > -and are defined as MIXER controls (iface = > -``SNDRV_CTL_ELEM_IFACE_MIXER``). > +returns the bitmask for professional mode. They are read-only controls. > > Meanwhile, “IEC958 Playback Default” control is defined for getting and > -setting the current default IEC958 bits. Note that this one is usually > -defined as a PCM control (iface = ``SNDRV_CTL_ELEM_IFACE_PCM``), > -although in some places it's defined as a MIXER control. > +setting the current default IEC958 bits. > + > +Due to historical reasons, both variants of the Playback Mask and the > +Playback Default controls can be implemented on either a > +``SNDRV_CTL_ELEM_IFACE_PCM`` or a ``SNDRV_CTL_ELEM_IFACE_MIXER`` iface. > +Drivers should expose the mask and default on the same iface though. > > In addition, you can define the control switches to enable/disable or to > set the raw bit mode. The implementation will depend on the chip, but > -- > 2.31.1 >
Re: [PATCH 00/11] drm/vc4: hdmi: Enable Channel Mapping, IEC958, HBR Passthrough using hdmi-codec
On Tue, 25 May 2021 11:23:53 +0200, Maxime Ripard wrote: > > Hi Takashi, > > On Tue, May 25, 2021 at 10:35:14AM +0200, Takashi Iwai wrote: > > On Mon, 24 May 2021 15:39:04 +0200, > > Maxime Ripard wrote: > > > > > > Hi, > > > > > > On Fri, May 07, 2021 at 04:03:23PM +0200, Maxime Ripard wrote: > > > > Hi, > > > > > > > > hdmi-codec allows to have a lot of HDMI-audio related infrastructure in > > > > place, > > > > it's missing a few controls to be able to provide HBR passthrough. This > > > > series > > > > adds more infrastructure for the drivers, and leverages it in the vc4 > > > > HDMI > > > > controller driver. > > > > > > > > One thing that felt a bit weird is that even though > > > > https://www.kernel.org/doc/html/latest/sound/kernel-api/writing-an-alsa-driver.html#iec958-s-pdif > > > > mentions that the iec958 mask control should be a mixer control and the > > > > default control should be a PCM one, it feels a bit weird to have two > > > > different > > > > control type for two controls so similar, and other drivers are pretty > > > > inconsistent with this. Should we update the documentation? > > > > > > Any comments on this series? > > > > A patch for updating the documentation is welcome. > > Currently, as de facto standard, we allow both MIXER and PCM ifaces > > for all IEC958-related controls, and it's unlikely that we would > > change that in future. > > Ok, I'll write a patch for the documentation make it clearer then :) > > Do we want to make sure that all the iec958 controls are on the same > iface, or is it also left to the driver (or should we just leave the > existing drivers as is but encourage a consistent use in the future)? I'd leave the existing drivers as-is. Changing the iface is basically an incompatible change, and although most of applications and alsa-lib should look at both ifaces, there can be any surprise by that change. thanks, Takashi
Re: [PATCH 00/11] drm/vc4: hdmi: Enable Channel Mapping, IEC958, HBR Passthrough using hdmi-codec
On Mon, 24 May 2021 15:39:04 +0200, Maxime Ripard wrote: > > Hi, > > On Fri, May 07, 2021 at 04:03:23PM +0200, Maxime Ripard wrote: > > Hi, > > > > hdmi-codec allows to have a lot of HDMI-audio related infrastructure in > > place, > > it's missing a few controls to be able to provide HBR passthrough. This > > series > > adds more infrastructure for the drivers, and leverages it in the vc4 HDMI > > controller driver. > > > > One thing that felt a bit weird is that even though > > https://www.kernel.org/doc/html/latest/sound/kernel-api/writing-an-alsa-driver.html#iec958-s-pdif > > mentions that the iec958 mask control should be a mixer control and the > > default control should be a PCM one, it feels a bit weird to have two > > different > > control type for two controls so similar, and other drivers are pretty > > inconsistent with this. Should we update the documentation? > > Any comments on this series? A patch for updating the documentation is welcome. Currently, as de facto standard, we allow both MIXER and PCM ifaces for all IEC958-related controls, and it's unlikely that we would change that in future. thanks, Takashi
