Re: [Nouveau] [RFC] treewide: cleanup unreachable breaks
On Sat, Oct 17, 2020 at 9:10 AM wrote:
>
> From: Tom Rix
>
> This is a upcoming change to clean up a new warning treewide.
> I am wondering if the change could be one mega patch (see below) or
> normal patch per file about 100 patches or somewhere half way by collecting
> early acks.
>
> clang has a number of useful, new warnings see
> https://clang.llvm.org/docs/DiagnosticsReference.html
>
> This change cleans up -Wunreachable-code-break
> https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break
> for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64.
>
> The method of fixing was to look for warnings where the preceding statement
> was a simple statement and by inspection made the subsequent break unneeded.
> In order of frequency these look like
>
> return and break
>
> switch (c->x86_vendor) {
> case X86_VENDOR_INTEL:
> intel_p5_mcheck_init(c);
> return 1;
> - break;
>
> goto and break
>
> default:
> operation = 0; /* make gcc happy */
> goto fail_response;
> - break;
>
> break and break
> case COLOR_SPACE_SRGB:
> /* by pass */
> REG_SET(OUTPUT_CSC_CONTROL, 0,
> OUTPUT_CSC_GRPH_MODE, 0);
> break;
> - break;
>
> The exception to the simple statement, is a switch case with a block
> and the end of block is a return
>
> struct obj_buffer *buff = r->ptr;
> return scnprintf(str, PRIV_STR_SIZE,
> "size=%u\naddr=0x%X\n", buff->size,
> buff->addr);
> }
> - break;
>
> Not considered obvious and excluded, breaks after
> multi level switches
> complicated if-else if-else blocks
> panic() or similar calls
>
> And there is an odd addition of a 'fallthrough' in drivers/tty/nozomi.c
[..]
> diff --git a/drivers/nvdimm/claim.c b/drivers/nvdimm/claim.c
> index 5a7c80053c62..2f250874b1a4 100644
> --- a/drivers/nvdimm/claim.c
> +++ b/drivers/nvdimm/claim.c
> @@ -200,11 +200,10 @@ ssize_t nd_namespace_store(struct device *dev,
> }
> break;
> default:
> len = -EBUSY;
> goto out_attach;
> - break;
> }
Acked-by: Dan Williams
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [Cocci] [RFC] treewide: cleanup unreachable breaks
On Sat, 2020-10-17 at 20:21 +0200, Julia Lawall wrote: > On Sat, 17 Oct 2020, Joe Perches wrote: > > On Sat, 2020-10-17 at 09:09 -0700, t...@redhat.com wrote: > > > From: Tom Rix > > > > > > This is a upcoming change to clean up a new warning treewide. > > > I am wondering if the change could be one mega patch (see below) or > > > normal patch per file about 100 patches or somewhere half way by > > > collecting > > > early acks. > > > > > > clang has a number of useful, new warnings see > > > https://clang.llvm.org/docs/DiagnosticsReference.html > > > > > > This change cleans up -Wunreachable-code-break > > > https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break > > > for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64. > > > > Early acks/individual patches by subsystem would be good. > > Better still would be an automated cocci script. > > Coccinelle is not especially good at this, because it is based on control > flow, and a return or goto diverts the control flow away from the break. > A hack to solve the problem is to put an if around the return or goto, but > that gives the break a meaningless file name and line number. I collected > the following list, but it only has 439 results, so fewer than clang. But > maybe there are some files that are not considered by clang in the x86 > allyesconfig configuration. > > Probably checkpatch is the best solution here, since it is not > configuration sensitive and doesn't care about control flow. Likely the clang compiler is the best option here. It might be useful to add -Wunreachable-code-break to W=1 or just always enable it if it isn't already enabled. diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index 95e4cdb94fe9..3819787579d5 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -32,6 +32,7 @@ KBUILD_CFLAGS += $(call cc-option, -Wunused-but-set-variable) KBUILD_CFLAGS += $(call cc-option, -Wunused-const-variable) KBUILD_CFLAGS += $(call cc-option, -Wpacked-not-aligned) KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation) +KBUILD_CFLAGS += $(call cc-option, -Wunreachable-code-break) # The following turn off the warnings enabled by -Wextra KBUILD_CFLAGS += -Wno-missing-field-initializers KBUILD_CFLAGS += -Wno-sign-compare (and thank you Tom for pushing this forward) checkpatch can't find instances like: case FOO: if (foo) return 1; else return 2; break; As it doesn't track flow and relies on the number of tabs to be the same for any goto/return and break; checkpatch will warn on: case FOO: ... goto bar; break; ___ Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [Cocci] [RFC] treewide: cleanup unreachable breaks
On Sat, 17 Oct 2020, Joe Perches wrote: > On Sat, 2020-10-17 at 09:09 -0700, t...@redhat.com wrote: > > From: Tom Rix > > > > This is a upcoming change to clean up a new warning treewide. > > I am wondering if the change could be one mega patch (see below) or > > normal patch per file about 100 patches or somewhere half way by collecting > > early acks. > > > > clang has a number of useful, new warnings see > > https://clang.llvm.org/docs/DiagnosticsReference.html > > > > This change cleans up -Wunreachable-code-break > > https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break > > for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64. > > Early acks/individual patches by subsystem would be good. > Better still would be an automated cocci script. Coccinelle is not especially good at this, because it is based on control flow, and a return or goto diverts the control flow away from the break. A hack to solve the problem is to put an if around the return or goto, but that gives the break a meaningless file name and line number. I collected the following list, but it only has 439 results, so fewer than clang. But maybe there are some files that are not considered by clang in the x86 allyesconfig configuration. Probably checkpatch is the best solution here, since it is not configuration sensitive and doesn't care about control flow. julia drivers/scsi/mvumi.c: function mvumi_cfg_hw_reg line 114 drivers/watchdog/geodewdt.c: function geodewdt_ioctl line 18 drivers/media/usb/b2c2/flexcop-usb.c: function flexcop_usb_init line 21 drivers/media/usb/b2c2/flexcop-usb.c: function flexcop_usb_memory_req line 20 drivers/tty/nozomi.c: function write_mem32 line 17 drivers/tty/nozomi.c: function write_mem32 line 25 drivers/tty/nozomi.c: function read_mem32 line 17 drivers/tty/nozomi.c: function read_mem32 line 21 sound/soc/codecs/wl1273.c: function wl1273_startup line 27 drivers/iio/adc/meson_saradc.c: function meson_sar_adc_iio_info_read_raw line 12 drivers/iio/adc/meson_saradc.c: function meson_sar_adc_iio_info_read_raw line 19 drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c: function map_transmitter_id_to_phy_instance line 6 drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c: function map_transmitter_id_to_phy_instance line 9 drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c: function map_transmitter_id_to_phy_instance line 12 drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c: function map_transmitter_id_to_phy_instance line 15 drivers/media/tuners/mt2063.c: function mt2063_init line 81 drivers/nfc/st21nfca/core.c: function st21nfca_hci_im_transceive line 46 arch/sh/boards/mach-landisk/gio.c: function gio_ioctl line 53 drivers/gpu/drm/mgag200/mgag200_mode.c: function mgag200_crtc_set_plls line 11 drivers/gpu/drm/mgag200/mgag200_mode.c: function mgag200_crtc_set_plls line 15 drivers/gpu/drm/mgag200/mgag200_mode.c: function mgag200_crtc_set_plls line 18 drivers/gpu/drm/mgag200/mgag200_mode.c: function mgag200_crtc_set_plls line 22 drivers/gpu/drm/mgag200/mgag200_mode.c: function mgag200_crtc_set_plls line 25 drivers/media/dvb-frontends/cx24117.c: function cx24117_attach line 16 drivers/block/xen-blkback/blkback.c: function dispatch_rw_block_io line 48 drivers/platform/x86/sony-laptop.c: function __sony_nc_gfx_switch_status_get line 16 drivers/platform/x86/sony-laptop.c: function __sony_nc_gfx_switch_status_get line 22 drivers/platform/x86/sony-laptop.c: function __sony_nc_gfx_switch_status_get line 31 drivers/char/mwave/mwavedd.c: function mwave_ioctl line 288 drivers/scsi/be2iscsi/be_mgmt.c: function beiscsi_adap_family_disp line 15 drivers/scsi/be2iscsi/be_mgmt.c: function beiscsi_adap_family_disp line 19 drivers/scsi/be2iscsi/be_mgmt.c: function beiscsi_adap_family_disp line 22 drivers/scsi/be2iscsi/be_mgmt.c: function beiscsi_adap_family_disp line 27 drivers/iio/imu/bmi160/bmi160_core.c: function bmi160_write_raw line 11 drivers/block/z2ram.c: function z2_open line 138 drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hw.c: function _rtl8723e_set_media_status line 38 samples/hidraw/hid-example.c: function bus_str line 6 samples/hidraw/hid-example.c: function bus_str line 9 samples/hidraw/hid-example.c: function bus_str line 12 samples/hidraw/hid-example.c: function bus_str line 15 samples/hidraw/hid-example.c: function bus_str line 18 drivers/scsi/ipr.c: function ipr_pci_error_detected line 10 drivers/gpio/gpio-bd70528.c: function bd70528_gpio_set_config line 11 drivers/gpio/gpio-bd70528.c: function bd70528_gpio_set_config line 17 drivers/gpio/gpio-bd70528.c: function bd70528_gpio_set_config line 21 drivers/pinctrl/pinctrl-rockchip.c: function rockchip_pinconf_get line 71 drivers/pinctrl/pinctrl-rockchip.c: function rockchip_pinconf_set line 74 drivers/gpu/drm/amd/display/include/signal_types.h: function dc_is_dvi_signal line 6 security/keys/trusted-keys/trusted_tpm1.c: function datablob_parse line 63 arch/x86/math-emu/fpu_trig.c: function
[Nouveau] [RFC] treewide: cleanup unreachable breaks
From: Tom Rix
This is a upcoming change to clean up a new warning treewide.
I am wondering if the change could be one mega patch (see below) or
normal patch per file about 100 patches or somewhere half way by collecting
early acks.
clang has a number of useful, new warnings see
https://clang.llvm.org/docs/DiagnosticsReference.html
This change cleans up -Wunreachable-code-break
https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break
for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64.
The method of fixing was to look for warnings where the preceding statement
was a simple statement and by inspection made the subsequent break unneeded.
In order of frequency these look like
return and break
switch (c->x86_vendor) {
case X86_VENDOR_INTEL:
intel_p5_mcheck_init(c);
return 1;
- break;
goto and break
default:
operation = 0; /* make gcc happy */
goto fail_response;
- break;
break and break
case COLOR_SPACE_SRGB:
/* by pass */
REG_SET(OUTPUT_CSC_CONTROL, 0,
OUTPUT_CSC_GRPH_MODE, 0);
break;
- break;
The exception to the simple statement, is a switch case with a block
and the end of block is a return
struct obj_buffer *buff = r->ptr;
return scnprintf(str, PRIV_STR_SIZE,
"size=%u\naddr=0x%X\n", buff->size,
buff->addr);
}
- break;
Not considered obvious and excluded, breaks after
multi level switches
complicated if-else if-else blocks
panic() or similar calls
And there is an odd addition of a 'fallthrough' in drivers/tty/nozomi.c
Tom
---
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 1c08cb9eb9f6..16ce86aed8e2 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -1809,15 +1809,13 @@ static int __mcheck_cpu_ancient_init(struct cpuinfo_x86
*c)
switch (c->x86_vendor) {
case X86_VENDOR_INTEL:
intel_p5_mcheck_init(c);
return 1;
- break;
case X86_VENDOR_CENTAUR:
winchip_mcheck_init(c);
return 1;
- break;
default:
return 0;
}
return 0;
diff --git a/arch/x86/kernel/cpu/microcode/amd.c
b/arch/x86/kernel/cpu/microcode/amd.c
index 3f6b137ef4e6..3d4a48336084 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -213,11 +213,10 @@ static unsigned int __verify_patch_size(u8 family, u32
sh_psize, size_t buf_size
max_size = F14H_MPB_MAX_SIZE;
break;
default:
WARN(1, "%s: WTF family: 0x%x\n", __func__, family);
return 0;
- break;
}
if (sh_psize > min_t(u32, buf_size, max_size))
return 0;
diff --git a/drivers/acpi/utils.c b/drivers/acpi/utils.c
index 838b719ec7ce..d5411a166685 100644
--- a/drivers/acpi/utils.c
+++ b/drivers/acpi/utils.c
@@ -102,11 +102,10 @@ acpi_extract_package(union acpi_object *package,
printk(KERN_WARNING PREFIX "Invalid package
element"
" [%d]: got number, expecting"
" [%c]\n",
i, format_string[i]);
return AE_BAD_DATA;
- break;
}
break;
case ACPI_TYPE_STRING:
case ACPI_TYPE_BUFFER:
@@ -127,11 +126,10 @@ acpi_extract_package(union acpi_object *package,
printk(KERN_WARNING PREFIX "Invalid package
element"
" [%d] got string/buffer,"
" expecting [%c]\n",
i, format_string[i]);
return AE_BAD_DATA;
- break;
}
break;
case ACPI_TYPE_LOCAL_REFERENCE:
switch (format_string[i]) {
case 'R':
@@ -142,22 +140,20 @@ acpi_extract_package(union acpi_object *package,
printk(KERN_WARNING PREFIX "Invalid package
element"
" [%d] got reference,"
" expecting [%c]\n",
i, format_string[i]);
return AE_BAD_DATA;
-
Re: [Nouveau] [RFC] treewide: cleanup unreachable breaks
On Sat, 2020-10-17 at 09:09 -0700, t...@redhat.com wrote: > From: Tom Rix > > This is a upcoming change to clean up a new warning treewide. > I am wondering if the change could be one mega patch (see below) or > normal patch per file about 100 patches or somewhere half way by collecting > early acks. > > clang has a number of useful, new warnings see > https://clang.llvm.org/docs/DiagnosticsReference.html > > This change cleans up -Wunreachable-code-break > https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code-break > for 266 of 485 warnings in this week's linux-next, allyesconfig on x86_64. Early acks/individual patches by subsystem would be good. Better still would be an automated cocci script. The existing checkpatch test for UNNECESSARY_BREAK has a few too many false positives. >From a script run on next on July 28th: arch/arm/mach-s3c24xx/mach-rx1950.c:266: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/arm/nwfpe/fpa11_cprt.c:38: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/arm/nwfpe/fpa11_cprt.c:41: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:684: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:687: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:690: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:693: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:697: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/include/asm/mach-au1x00/au1000.h:700: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:276: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:279: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:282: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:287: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/loongson2ef/common/cs5536/cs5536_isa.c:290: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/rb532/setup.c:76: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/mips/rb532/setup.c:79: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:231: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:234: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:237: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/include/asm/kvm_book3s_64.h:240: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/net/bpf_jit_comp.c:455: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/platforms/cell/spufs/switch.c:2047: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/powerpc/platforms/cell/spufs/switch.c:2077: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/sh/boards/mach-landisk/gio.c:111: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/mce/core.c:1734: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/mce/core.c:1738: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return arch/x86/kernel/cpu/microcode/amd.c:218: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:107: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:132: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:147: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/acpi/utils.c:158: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/ata/libata-scsi.c:3973: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/base/power/main.c:366: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/block/xen-blkback/blkback.c:1272: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/ipmi/ipmi_devintf.c:493: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/lp.c:625: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return drivers/char/mwave/mwavedd.c:406: WARNING:UNNECESSARY_BREAK: break is not useful after a goto or return
Re: [Nouveau] [PATCH v4 09/10] dma-buf-map: Add memcpy and pointer-increment interfaces
Hi Thomas.
On Thu, Oct 15, 2020 at 02:38:05PM +0200, Thomas Zimmermann wrote:
> To do framebuffer updates, one needs memcpy from system memory and a
> pointer-increment function. Add both interfaces with documentation.
>
> Signed-off-by: Thomas Zimmermann
> ---
> include/linux/dma-buf-map.h | 72 +++--
> 1 file changed, 62 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> index 2e8bbecb5091..6ca0f304dda2 100644
> --- a/include/linux/dma-buf-map.h
> +++ b/include/linux/dma-buf-map.h
> @@ -32,6 +32,14 @@
> * accessing the buffer. Use the returned instance and the helper functions
> * to access the buffer's memory in the correct way.
> *
> + * The type :c:type:`struct dma_buf_map ` and its helpers are
> + * actually independent from the dma-buf infrastructure. When sharing buffers
> + * among devices, drivers have to know the location of the memory to access
> + * the buffers in a safe way. :c:type:`struct dma_buf_map `
> + * solves this problem for dma-buf and its users. If other drivers or
> + * sub-systems require similar functionality, the type could be generalized
> + * and moved to a more prominent header file.
> + *
> * Open-coding access to :c:type:`struct dma_buf_map ` is
> * considered bad style. Rather then accessing its fields directly, use one
> * of the provided helper functions, or implement your own. For example,
> @@ -51,6 +59,14 @@
> *
> * dma_buf_map_set_vaddr_iomem( 0xdeadbeaf);
> *
> + * Instances of struct dma_buf_map do not have to be cleaned up, but
> + * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> + * always refer to system memory.
> + *
> + * .. code-block:: c
> + *
> + * dma_buf_map_clear();
> + *
> * Test if a mapping is valid with either dma_buf_map_is_set() or
> * dma_buf_map_is_null().
> *
> @@ -73,17 +89,19 @@
> * if (dma_buf_map_is_equal(_map, _map))
> * // always false
> *
> - * Instances of struct dma_buf_map do not have to be cleaned up, but
> - * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> - * always refer to system memory.
> + * A set up instance of struct dma_buf_map can be used to access or
> manipulate
> + * the buffer memory. Depending on the location of the memory, the provided
> + * helpers will pick the correct operations. Data can be copied into the
> memory
> + * with dma_buf_map_memcpy_to(). The address can be manipulated with
> + * dma_buf_map_incr().
> *
> - * The type :c:type:`struct dma_buf_map ` and its helpers are
> - * actually independent from the dma-buf infrastructure. When sharing buffers
> - * among devices, drivers have to know the location of the memory to access
> - * the buffers in a safe way. :c:type:`struct dma_buf_map `
> - * solves this problem for dma-buf and its users. If other drivers or
> - * sub-systems require similar functionality, the type could be generalized
> - * and moved to a more prominent header file.
> + * .. code-block:: c
> + *
> + * const void *src = ...; // source buffer
> + * size_t len = ...; // length of src
> + *
> + * dma_buf_map_memcpy_to(, src, len);
> + * dma_buf_map_incr(, len); // go to first byte after the memcpy
> */
>
> /**
> @@ -210,4 +228,38 @@ static inline void dma_buf_map_clear(struct dma_buf_map
> *map)
> }
> }
>
> +/**
> + * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
> + * @dst: The dma-buf mapping structure
> + * @src: The source buffer
> + * @len: The number of byte in src
> + *
> + * Copies data into a dma-buf mapping. The source buffer is in system
> + * memory. Depending on the buffer's location, the helper picks the correct
> + * method of accessing the memory.
> + */
> +static inline void dma_buf_map_memcpy_to(struct dma_buf_map *dst, const void
> *src, size_t len)
> +{
> + if (dst->is_iomem)
> + memcpy_toio(dst->vaddr_iomem, src, len);
> + else
> + memcpy(dst->vaddr, src, len);
sparc64 needs "#include " to build as is does not get
this via io.h
Sam
> +}
> +
> +/**
> + * dma_buf_map_incr - Increments the address stored in a dma-buf mapping
> + * @map: The dma-buf mapping structure
> + * @incr:The number of bytes to increment
> + *
> + * Increments the address stored in a dma-buf mapping. Depending on the
> + * buffer's location, the correct value will be updated.
> + */
> +static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
> +{
> + if (map->is_iomem)
> + map->vaddr_iomem += incr;
> + else
> + map->vaddr += incr;
> +}
> +
> #endif /* __DMA_BUF_MAP_H__ */
> --
> 2.28.0
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v4 05/10] drm/ttm: Add vmap/vunmap to TTM and TTM GEM helpers
Am 15.10.20 um 19:52 schrieb Thomas Zimmermann:
Hi
On Thu, 15 Oct 2020 18:49:09 +0200 Daniel Vetter wrote:
On Thu, Oct 15, 2020 at 04:08:13PM +0200, Christian König wrote:
Am 15.10.20 um 14:38 schrieb Thomas Zimmermann:
The new functions ttm_bo_{vmap,vunmap}() map and unmap a TTM BO in
kernel address space. The mapping's address is returned as struct
dma_buf_map. Each function is a simplified version of TTM's existing
kmap code. Both functions respect the memory's location ani/or
writecombine flags.
On top TTM's functions, GEM TTM helpers got drm_gem_ttm_{vmap,vunmap}(),
two helpers that convert a GEM object into the TTM BO and forward the
call to TTM's vmap/vunmap. These helpers can be dropped into the rsp
GEM object callbacks.
v4:
* drop ttm_kmap_obj_to_dma_buf() in favor of vmap helpers
(Daniel, Christian)
Bunch of minor comments below, but over all look very solid to me.
Yeah I think just duplicating the ttm bo map stuff for vmap is indeed the
cleanest. And then we can maybe push the combinatorial monster into
vmwgfx, which I think is the only user after this series. Or perhaps a
dedicated set of helpers to map an invidual page (again using the
dma_buf_map stuff).
From a quick look, I'd say it should be possible to have the same interface
for kmap/kunmap as for vmap/vunmap (i.e., parameters are bo and dma-buf-map).
All mapping state can be deduced from this. And struct ttm_bo_kmap_obj can be
killed off entirely.
Yes, that would be rather nice to have.
Thanks,
Christian.
Best regards
Thomas
I'll let Christian with the details, but at a high level this is
definitely
Acked-by: Daniel Vetter
Thanks a lot for doing all this.
-Daniel
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_gem_ttm_helper.c | 38 +++
drivers/gpu/drm/ttm/ttm_bo_util.c| 72
include/drm/drm_gem_ttm_helper.h | 6 +++
include/drm/ttm/ttm_bo_api.h | 28 +++
include/linux/dma-buf-map.h | 20
5 files changed, 164 insertions(+)
diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c
b/drivers/gpu/drm/drm_gem_ttm_helper.c index 0e4fb9ba43ad..db4c14d78a30
100644 --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
+++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
@@ -49,6 +49,44 @@ void drm_gem_ttm_print_info(struct drm_printer *p,
unsigned int indent, }
EXPORT_SYMBOL(drm_gem_ttm_print_info);
+/**
+ * drm_gem_ttm_vmap() - vmap _buffer_object
+ * @gem: GEM object.
+ * @map: [out] returns the dma-buf mapping.
+ *
+ * Maps a GEM object with ttm_bo_vmap(). This function can be used as
+ * _gem_object_funcs.vmap callback.
+ *
+ * Returns:
+ * 0 on success, or a negative errno code otherwise.
+ */
+int drm_gem_ttm_vmap(struct drm_gem_object *gem,
+struct dma_buf_map *map)
+{
+ struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
+
+ return ttm_bo_vmap(bo, map);
+
+}
+EXPORT_SYMBOL(drm_gem_ttm_vmap);
+
+/**
+ * drm_gem_ttm_vunmap() - vunmap _buffer_object
+ * @gem: GEM object.
+ * @map: dma-buf mapping.
+ *
+ * Unmaps a GEM object with ttm_bo_vunmap(). This function can be used
as
+ * _gem_object_funcs.vmap callback.
+ */
+void drm_gem_ttm_vunmap(struct drm_gem_object *gem,
+ struct dma_buf_map *map)
+{
+ struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
+
+ ttm_bo_vunmap(bo, map);
+}
+EXPORT_SYMBOL(drm_gem_ttm_vunmap);
+
/**
* drm_gem_ttm_mmap() - mmap _buffer_object
* @gem: GEM object.
diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
b/drivers/gpu/drm/ttm/ttm_bo_util.c index bdee4df1f3f2..80c42c774c7d
100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -526,6 +527,77 @@ void ttm_bo_kunmap(struct ttm_bo_kmap_obj *map)
}
EXPORT_SYMBOL(ttm_bo_kunmap);
+int ttm_bo_vmap(struct ttm_buffer_object *bo, struct dma_buf_map *map)
+{
+ struct ttm_resource *mem = >mem;
+ int ret;
+
+ ret = ttm_mem_io_reserve(bo->bdev, mem);
+ if (ret)
+ return ret;
+
+ if (mem->bus.is_iomem) {
+ void __iomem *vaddr_iomem;
+ unsigned long size = bo->num_pages << PAGE_SHIFT;
Please use uint64_t here and make sure to cast bo->num_pages before
shifting.
We have an unit tests of allocating a 8GB BO and that should work on a
32bit machine as well :)
+
+ if (mem->bus.addr)
+ vaddr_iomem = (void *)(((u8 *)mem->bus.addr));
+ else if (mem->placement & TTM_PL_FLAG_WC)
I've just nuked the TTM_PL_FLAG_WC flag in drm-misc-next. There is a new
mem->bus.caching enum as replacement.
+ vaddr_iomem = ioremap_wc(mem->bus.offset,
size);
+ else
+ vaddr_iomem = ioremap(mem->bus.offset, size);
+
+ if (!vaddr_iomem)
+ return -ENOMEM;
Re: [Nouveau] [PATCH v4 10/10] drm/fb_helper: Support framebuffers in I/O memory
Hi Thomas.
On Thu, Oct 15, 2020 at 02:38:06PM +0200, Thomas Zimmermann wrote:
> At least sparc64 requires I/O-specific access to framebuffers. This
> patch updates the fbdev console accordingly.
>
> For drivers with direct access to the framebuffer memory, the callback
> functions in struct fb_ops test for the type of memory and call the rsp
> fb_sys_ of fb_cfb_ functions.
>
> For drivers that employ a shadow buffer, fbdev's blit function retrieves
> the framebuffer address as struct dma_buf_map, and uses dma_buf_map
> interfaces to access the buffer.
>
> The bochs driver on sparc64 uses a workaround to flag the framebuffer as
> I/O memory and avoid a HW exception. With the introduction of struct
> dma_buf_map, this is not required any longer. The patch removes the rsp
> code from both, bochs and fbdev.
>
> v4:
> * move dma_buf_map changes into separate patch (Daniel)
> * TODO list: comment on fbdev updates (Daniel)
I have been offline for a while so have not followed all the threads on
this. So may comments below may well be addressed but I failed to see
it.
If the point about fb_sync is already addressed/considered then:
Reviewed-by: Sam Ravnborg
> Signed-off-by: Thomas Zimmermann
> ---
> Documentation/gpu/todo.rst| 19 ++-
> drivers/gpu/drm/bochs/bochs_kms.c | 1 -
> drivers/gpu/drm/drm_fb_helper.c | 217 --
> include/drm/drm_mode_config.h | 12 --
> 4 files changed, 220 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 7e6fc3c04add..638b7f704339 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup()
>
>
> Most drivers can use drm_fbdev_generic_setup(). Driver have to implement
> -atomic modesetting and GEM vmap support. Current generic fbdev emulation
> -expects the framebuffer in system memory (or system-like memory).
> +atomic modesetting and GEM vmap support. Historically, generic fbdev
> emulation
> +expected the framebuffer in system memory or system-like memory. By employing
> +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported
> +as well.
>
> Contact: Maintainer of the driver you plan to convert
>
> Level: Intermediate
>
> +Reimplement functions in drm_fbdev_fb_ops without fbdev
> +---
> +
> +A number of callback functions in drm_fbdev_fb_ops could benefit from
> +being rewritten without dependencies on the fbdev module. Some of the
> +helpers could further benefit from using struct dma_buf_map instead of
> +raw pointers.
> +
> +Contact: Thomas Zimmermann , Daniel Vetter
> +
> +Level: Advanced
> +
> +
> drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
> -
>
> diff --git a/drivers/gpu/drm/bochs/bochs_kms.c
> b/drivers/gpu/drm/bochs/bochs_kms.c
> index 13d0d04c4457..853081d186d5 100644
> --- a/drivers/gpu/drm/bochs/bochs_kms.c
> +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs)
> bochs->dev->mode_config.preferred_depth = 24;
> bochs->dev->mode_config.prefer_shadow = 0;
> bochs->dev->mode_config.prefer_shadow_fbdev = 1;
> - bochs->dev->mode_config.fbdev_use_iomem = true;
> bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
>
> bochs->dev->mode_config.funcs = _mode_funcs;
Good to see this workaround gone again!
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 6212cd7cde1d..462b0c130ebb 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct
> work_struct *work)
> }
>
> static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper,
> - struct drm_clip_rect *clip)
> + struct drm_clip_rect *clip,
> + struct dma_buf_map *dst)
> {
> struct drm_framebuffer *fb = fb_helper->fb;
> unsigned int cpp = fb->format->cpp[0];
> size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> void *src = fb_helper->fbdev->screen_buffer + offset;
> - void *dst = fb_helper->buffer->map.vaddr + offset;
> size_t len = (clip->x2 - clip->x1) * cpp;
> unsigned int y;
>
> - for (y = clip->y1; y < clip->y2; y++) {
> - if (!fb_helper->dev->mode_config.fbdev_use_iomem)
> - memcpy(dst, src, len);
> - else
> - memcpy_toio((void __iomem *)dst, src, len);
> + dma_buf_map_incr(dst, offset); /* go to first pixel within clip rect */
>
> + for (y = clip->y1; y < clip->y2; y++) {
> +
Re: [Nouveau] [PATCH v4 09/10] dma-buf-map: Add memcpy and pointer-increment interfaces
Hi Thomas.
On Thu, Oct 15, 2020 at 02:38:05PM +0200, Thomas Zimmermann wrote:
> To do framebuffer updates, one needs memcpy from system memory and a
> pointer-increment function. Add both interfaces with documentation.
>
> Signed-off-by: Thomas Zimmermann
Looks good.
Reviewed-by: Sam Ravnborg
> ---
> include/linux/dma-buf-map.h | 72 +++--
> 1 file changed, 62 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> index 2e8bbecb5091..6ca0f304dda2 100644
> --- a/include/linux/dma-buf-map.h
> +++ b/include/linux/dma-buf-map.h
> @@ -32,6 +32,14 @@
> * accessing the buffer. Use the returned instance and the helper functions
> * to access the buffer's memory in the correct way.
> *
> + * The type :c:type:`struct dma_buf_map ` and its helpers are
> + * actually independent from the dma-buf infrastructure. When sharing buffers
> + * among devices, drivers have to know the location of the memory to access
> + * the buffers in a safe way. :c:type:`struct dma_buf_map `
> + * solves this problem for dma-buf and its users. If other drivers or
> + * sub-systems require similar functionality, the type could be generalized
> + * and moved to a more prominent header file.
> + *
> * Open-coding access to :c:type:`struct dma_buf_map ` is
> * considered bad style. Rather then accessing its fields directly, use one
> * of the provided helper functions, or implement your own. For example,
> @@ -51,6 +59,14 @@
> *
> * dma_buf_map_set_vaddr_iomem( 0xdeadbeaf);
> *
> + * Instances of struct dma_buf_map do not have to be cleaned up, but
> + * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> + * always refer to system memory.
> + *
> + * .. code-block:: c
> + *
> + * dma_buf_map_clear();
> + *
> * Test if a mapping is valid with either dma_buf_map_is_set() or
> * dma_buf_map_is_null().
> *
> @@ -73,17 +89,19 @@
> * if (dma_buf_map_is_equal(_map, _map))
> * // always false
> *
> - * Instances of struct dma_buf_map do not have to be cleaned up, but
> - * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> - * always refer to system memory.
> + * A set up instance of struct dma_buf_map can be used to access or
> manipulate
> + * the buffer memory. Depending on the location of the memory, the provided
> + * helpers will pick the correct operations. Data can be copied into the
> memory
> + * with dma_buf_map_memcpy_to(). The address can be manipulated with
> + * dma_buf_map_incr().
> *
> - * The type :c:type:`struct dma_buf_map ` and its helpers are
> - * actually independent from the dma-buf infrastructure. When sharing buffers
> - * among devices, drivers have to know the location of the memory to access
> - * the buffers in a safe way. :c:type:`struct dma_buf_map `
> - * solves this problem for dma-buf and its users. If other drivers or
> - * sub-systems require similar functionality, the type could be generalized
> - * and moved to a more prominent header file.
> + * .. code-block:: c
> + *
> + * const void *src = ...; // source buffer
> + * size_t len = ...; // length of src
> + *
> + * dma_buf_map_memcpy_to(, src, len);
> + * dma_buf_map_incr(, len); // go to first byte after the memcpy
> */
>
> /**
> @@ -210,4 +228,38 @@ static inline void dma_buf_map_clear(struct dma_buf_map
> *map)
> }
> }
>
> +/**
> + * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
> + * @dst: The dma-buf mapping structure
> + * @src: The source buffer
> + * @len: The number of byte in src
> + *
> + * Copies data into a dma-buf mapping. The source buffer is in system
> + * memory. Depending on the buffer's location, the helper picks the correct
> + * method of accessing the memory.
> + */
> +static inline void dma_buf_map_memcpy_to(struct dma_buf_map *dst, const void
> *src, size_t len)
> +{
> + if (dst->is_iomem)
> + memcpy_toio(dst->vaddr_iomem, src, len);
> + else
> + memcpy(dst->vaddr, src, len);
> +}
> +
> +/**
> + * dma_buf_map_incr - Increments the address stored in a dma-buf mapping
> + * @map: The dma-buf mapping structure
> + * @incr:The number of bytes to increment
> + *
> + * Increments the address stored in a dma-buf mapping. Depending on the
> + * buffer's location, the correct value will be updated.
> + */
> +static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
> +{
> + if (map->is_iomem)
> + map->vaddr_iomem += incr;
> + else
> + map->vaddr += incr;
> +}
> +
> #endif /* __DMA_BUF_MAP_H__ */
> --
> 2.28.0
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH v4 09/10] dma-buf-map: Add memcpy and pointer-increment interfaces
Hi Sam
On Fri, 16 Oct 2020 12:08:54 +0200 Sam Ravnborg wrote:
> Hi Thomas.
>
> On Thu, Oct 15, 2020 at 02:38:05PM +0200, Thomas Zimmermann wrote:
> > To do framebuffer updates, one needs memcpy from system memory and a
> > pointer-increment function. Add both interfaces with documentation.
> >
> > Signed-off-by: Thomas Zimmermann
>
> Looks good.
> Reviewed-by: Sam Ravnborg
Thanks. If you have the time, may I ask you to test this patchset on the
bochs/sparc64 system that failed with the original code?
Best regards
Thomas
>
> > ---
> > include/linux/dma-buf-map.h | 72 +++--
> > 1 file changed, 62 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/dma-buf-map.h b/include/linux/dma-buf-map.h
> > index 2e8bbecb5091..6ca0f304dda2 100644
> > --- a/include/linux/dma-buf-map.h
> > +++ b/include/linux/dma-buf-map.h
> > @@ -32,6 +32,14 @@
> > * accessing the buffer. Use the returned instance and the helper
> > functions
> > * to access the buffer's memory in the correct way.
> > *
> > + * The type :c:type:`struct dma_buf_map ` and its helpers
> > are
> > + * actually independent from the dma-buf infrastructure. When sharing
> > buffers
> > + * among devices, drivers have to know the location of the memory to
> > access
> > + * the buffers in a safe way. :c:type:`struct dma_buf_map `
> > + * solves this problem for dma-buf and its users. If other drivers or
> > + * sub-systems require similar functionality, the type could be
> > generalized
> > + * and moved to a more prominent header file.
> > + *
> > * Open-coding access to :c:type:`struct dma_buf_map ` is
> > * considered bad style. Rather then accessing its fields directly, use
> > one
> > * of the provided helper functions, or implement your own. For example,
> > @@ -51,6 +59,14 @@
> > *
> > * dma_buf_map_set_vaddr_iomem( 0xdeadbeaf);
> > *
> > + * Instances of struct dma_buf_map do not have to be cleaned up, but
> > + * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> > + * always refer to system memory.
> > + *
> > + * .. code-block:: c
> > + *
> > + * dma_buf_map_clear();
> > + *
> > * Test if a mapping is valid with either dma_buf_map_is_set() or
> > * dma_buf_map_is_null().
> > *
> > @@ -73,17 +89,19 @@
> > * if (dma_buf_map_is_equal(_map, _map))
> > * // always false
> > *
> > - * Instances of struct dma_buf_map do not have to be cleaned up, but
> > - * can be cleared to NULL with dma_buf_map_clear(). Cleared mappings
> > - * always refer to system memory.
> > + * A set up instance of struct dma_buf_map can be used to access or
> > manipulate
> > + * the buffer memory. Depending on the location of the memory, the
> > provided
> > + * helpers will pick the correct operations. Data can be copied into the
> > memory
> > + * with dma_buf_map_memcpy_to(). The address can be manipulated with
> > + * dma_buf_map_incr().
> > *
> > - * The type :c:type:`struct dma_buf_map ` and its helpers
> > are
> > - * actually independent from the dma-buf infrastructure. When sharing
> > buffers
> > - * among devices, drivers have to know the location of the memory to
> > access
> > - * the buffers in a safe way. :c:type:`struct dma_buf_map `
> > - * solves this problem for dma-buf and its users. If other drivers or
> > - * sub-systems require similar functionality, the type could be
> > generalized
> > - * and moved to a more prominent header file.
> > + * .. code-block:: c
> > + *
> > + * const void *src = ...; // source buffer
> > + * size_t len = ...; // length of src
> > + *
> > + * dma_buf_map_memcpy_to(, src, len);
> > + * dma_buf_map_incr(, len); // go to first byte after the
> > memcpy */
> >
> > /**
> > @@ -210,4 +228,38 @@ static inline void dma_buf_map_clear(struct
> > dma_buf_map *map) }
> > }
> >
> > +/**
> > + * dma_buf_map_memcpy_to - Memcpy into dma-buf mapping
> > + * @dst: The dma-buf mapping structure
> > + * @src: The source buffer
> > + * @len: The number of byte in src
> > + *
> > + * Copies data into a dma-buf mapping. The source buffer is in system
> > + * memory. Depending on the buffer's location, the helper picks the
> > correct
> > + * method of accessing the memory.
> > + */
> > +static inline void dma_buf_map_memcpy_to(struct dma_buf_map *dst, const
> > void *src, size_t len) +{
> > + if (dst->is_iomem)
> > + memcpy_toio(dst->vaddr_iomem, src, len);
> > + else
> > + memcpy(dst->vaddr, src, len);
> > +}
> > +
> > +/**
> > + * dma_buf_map_incr - Increments the address stored in a dma-buf mapping
> > + * @map: The dma-buf mapping structure
> > + * @incr: The number of bytes to increment
> > + *
> > + * Increments the address stored in a dma-buf mapping. Depending on the
> > + * buffer's location, the correct value will be updated.
> > + */
> > +static inline void dma_buf_map_incr(struct dma_buf_map *map, size_t incr)
> > +{
> > + if (map->is_iomem)
> > + map->vaddr_iomem += incr;
>
Re: [Nouveau] [PATCH v4 10/10] drm/fb_helper: Support framebuffers in I/O memory
On Fri, Oct 16, 2020 at 02:19:42PM +0200, Thomas Zimmermann wrote: > Hi > > On Fri, 16 Oct 2020 14:03:47 +0200 Sam Ravnborg wrote: > > > Hi Thomas. > > > > On Thu, Oct 15, 2020 at 02:38:06PM +0200, Thomas Zimmermann wrote: > > > At least sparc64 requires I/O-specific access to framebuffers. This > > > patch updates the fbdev console accordingly. > > > > > > For drivers with direct access to the framebuffer memory, the callback > > > functions in struct fb_ops test for the type of memory and call the rsp > > > fb_sys_ of fb_cfb_ functions. > > > > > > For drivers that employ a shadow buffer, fbdev's blit function retrieves > > > the framebuffer address as struct dma_buf_map, and uses dma_buf_map > > > interfaces to access the buffer. > > > > > > The bochs driver on sparc64 uses a workaround to flag the framebuffer as > > > I/O memory and avoid a HW exception. With the introduction of struct > > > dma_buf_map, this is not required any longer. The patch removes the rsp > > > code from both, bochs and fbdev. > > > > > > v4: > > > * move dma_buf_map changes into separate patch (Daniel) > > > * TODO list: comment on fbdev updates (Daniel) > > > > > > Signed-off-by: Thomas Zimmermann > > > > The original workaround fixed it so we could run qemu with the > > -nographic option. > > > > So I went ahead and tried to run quemu version: > > v5.0.0-1970-g0b100c8e72-dirty. > > And with the BOCHS driver built-in. > > > > With the following command line: > > qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -nographic > > > > Behaviour was the same before and after applying this patch. > > (panic due to VFS: Unable to mount root fs on unknown-block(0,0)) > > So I consider it fixed for real now and not just a workaround. > > > > I also tested with: > > qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -serial > > stdio > > > > and it worked in both cases too. > > FTR, you booted a kernel and got graphics output. The error is simply that > there was no disk to mount? The short version "Yes". The longer version: With "qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -serial stdio" I got graphical output - one penguin. With "qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -nographic" I got no graphical output, as implied by the -nographic option. But the boot continued - where it would panic before when we accessed IO memory as system memory. In both cases I got an error because I had not specified any rootfs, so qemu failed to mount any rootfs. So expected. Sam > > Best regards > Thomas > > > > > All the comments above so future-me have an easier time finding how to > > reproduce. > > > > Tested-by: Sam Ravnborg > > > > Sam > > > > > --- > > > Documentation/gpu/todo.rst| 19 ++- > > > drivers/gpu/drm/bochs/bochs_kms.c | 1 - > > > drivers/gpu/drm/drm_fb_helper.c | 217 -- > > > include/drm/drm_mode_config.h | 12 -- > > > 4 files changed, 220 insertions(+), 29 deletions(-) > > > > > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > > > index 7e6fc3c04add..638b7f704339 100644 > > > --- a/Documentation/gpu/todo.rst > > > +++ b/Documentation/gpu/todo.rst > > > @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup() > > > > > > > > > Most drivers can use drm_fbdev_generic_setup(). Driver have to implement > > > -atomic modesetting and GEM vmap support. Current generic fbdev emulation > > > -expects the framebuffer in system memory (or system-like memory). > > > +atomic modesetting and GEM vmap support. Historically, generic fbdev > > > emulation +expected the framebuffer in system memory or system-like > > > memory. By employing +struct dma_buf_map, drivers with frambuffers in I/O > > > memory can be supported +as well. > > > > > > Contact: Maintainer of the driver you plan to convert > > > > > > Level: Intermediate > > > > > > +Reimplement functions in drm_fbdev_fb_ops without fbdev > > > +--- > > > + > > > +A number of callback functions in drm_fbdev_fb_ops could benefit from > > > +being rewritten without dependencies on the fbdev module. Some of the > > > +helpers could further benefit from using struct dma_buf_map instead of > > > +raw pointers. > > > + > > > +Contact: Thomas Zimmermann , Daniel Vetter > > > + > > > +Level: Advanced > > > + > > > + > > > drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup > > > - > > > > > > diff --git a/drivers/gpu/drm/bochs/bochs_kms.c > > > b/drivers/gpu/drm/bochs/bochs_kms.c index 13d0d04c4457..853081d186d5 > > > 100644 --- a/drivers/gpu/drm/bochs/bochs_kms.c > > > +++ b/drivers/gpu/drm/bochs/bochs_kms.c > > > @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs) > > >
Re: [Nouveau] [PATCH v4 10/10] drm/fb_helper: Support framebuffers in I/O memory
Hi Thomas.
On Thu, Oct 15, 2020 at 02:38:06PM +0200, Thomas Zimmermann wrote:
> At least sparc64 requires I/O-specific access to framebuffers. This
> patch updates the fbdev console accordingly.
>
> For drivers with direct access to the framebuffer memory, the callback
> functions in struct fb_ops test for the type of memory and call the rsp
> fb_sys_ of fb_cfb_ functions.
>
> For drivers that employ a shadow buffer, fbdev's blit function retrieves
> the framebuffer address as struct dma_buf_map, and uses dma_buf_map
> interfaces to access the buffer.
>
> The bochs driver on sparc64 uses a workaround to flag the framebuffer as
> I/O memory and avoid a HW exception. With the introduction of struct
> dma_buf_map, this is not required any longer. The patch removes the rsp
> code from both, bochs and fbdev.
>
> v4:
> * move dma_buf_map changes into separate patch (Daniel)
> * TODO list: comment on fbdev updates (Daniel)
>
> Signed-off-by: Thomas Zimmermann
The original workaround fixed it so we could run qemu with the
-nographic option.
So I went ahead and tried to run quemu version:
v5.0.0-1970-g0b100c8e72-dirty.
And with the BOCHS driver built-in.
With the following command line:
qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -nographic
Behaviour was the same before and after applying this patch.
(panic due to VFS: Unable to mount root fs on unknown-block(0,0))
So I consider it fixed for real now and not just a workaround.
I also tested with:
qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -serial stdio
and it worked in both cases too.
All the comments above so future-me have an easier time finding how to
reproduce.
Tested-by: Sam Ravnborg
Sam
> ---
> Documentation/gpu/todo.rst| 19 ++-
> drivers/gpu/drm/bochs/bochs_kms.c | 1 -
> drivers/gpu/drm/drm_fb_helper.c | 217 --
> include/drm/drm_mode_config.h | 12 --
> 4 files changed, 220 insertions(+), 29 deletions(-)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 7e6fc3c04add..638b7f704339 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup()
>
>
> Most drivers can use drm_fbdev_generic_setup(). Driver have to implement
> -atomic modesetting and GEM vmap support. Current generic fbdev emulation
> -expects the framebuffer in system memory (or system-like memory).
> +atomic modesetting and GEM vmap support. Historically, generic fbdev
> emulation
> +expected the framebuffer in system memory or system-like memory. By employing
> +struct dma_buf_map, drivers with frambuffers in I/O memory can be supported
> +as well.
>
> Contact: Maintainer of the driver you plan to convert
>
> Level: Intermediate
>
> +Reimplement functions in drm_fbdev_fb_ops without fbdev
> +---
> +
> +A number of callback functions in drm_fbdev_fb_ops could benefit from
> +being rewritten without dependencies on the fbdev module. Some of the
> +helpers could further benefit from using struct dma_buf_map instead of
> +raw pointers.
> +
> +Contact: Thomas Zimmermann , Daniel Vetter
> +
> +Level: Advanced
> +
> +
> drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
> -
>
> diff --git a/drivers/gpu/drm/bochs/bochs_kms.c
> b/drivers/gpu/drm/bochs/bochs_kms.c
> index 13d0d04c4457..853081d186d5 100644
> --- a/drivers/gpu/drm/bochs/bochs_kms.c
> +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs)
> bochs->dev->mode_config.preferred_depth = 24;
> bochs->dev->mode_config.prefer_shadow = 0;
> bochs->dev->mode_config.prefer_shadow_fbdev = 1;
> - bochs->dev->mode_config.fbdev_use_iomem = true;
> bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
>
> bochs->dev->mode_config.funcs = _mode_funcs;
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 6212cd7cde1d..462b0c130ebb 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct
> work_struct *work)
> }
>
> static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper *fb_helper,
> - struct drm_clip_rect *clip)
> + struct drm_clip_rect *clip,
> + struct dma_buf_map *dst)
> {
> struct drm_framebuffer *fb = fb_helper->fb;
> unsigned int cpp = fb->format->cpp[0];
> size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> void *src = fb_helper->fbdev->screen_buffer + offset;
> - void *dst = fb_helper->buffer->map.vaddr + offset;
Re: [Nouveau] [PATCH v4 10/10] drm/fb_helper: Support framebuffers in I/O memory
Hi On Fri, 16 Oct 2020 14:03:47 +0200 Sam Ravnborg wrote: > Hi Thomas. > > On Thu, Oct 15, 2020 at 02:38:06PM +0200, Thomas Zimmermann wrote: > > At least sparc64 requires I/O-specific access to framebuffers. This > > patch updates the fbdev console accordingly. > > > > For drivers with direct access to the framebuffer memory, the callback > > functions in struct fb_ops test for the type of memory and call the rsp > > fb_sys_ of fb_cfb_ functions. > > > > For drivers that employ a shadow buffer, fbdev's blit function retrieves > > the framebuffer address as struct dma_buf_map, and uses dma_buf_map > > interfaces to access the buffer. > > > > The bochs driver on sparc64 uses a workaround to flag the framebuffer as > > I/O memory and avoid a HW exception. With the introduction of struct > > dma_buf_map, this is not required any longer. The patch removes the rsp > > code from both, bochs and fbdev. > > > > v4: > > * move dma_buf_map changes into separate patch (Daniel) > > * TODO list: comment on fbdev updates (Daniel) > > > > Signed-off-by: Thomas Zimmermann > > The original workaround fixed it so we could run qemu with the > -nographic option. > > So I went ahead and tried to run quemu version: > v5.0.0-1970-g0b100c8e72-dirty. > And with the BOCHS driver built-in. > > With the following command line: > qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -nographic > > Behaviour was the same before and after applying this patch. > (panic due to VFS: Unable to mount root fs on unknown-block(0,0)) > So I consider it fixed for real now and not just a workaround. > > I also tested with: > qemu-system-sparc64 -m 512 -kernel vmlinux -append console=ttyS0 -serial > stdio > > and it worked in both cases too. FTR, you booted a kernel and got graphics output. The error is simply that there was no disk to mount? Best regards Thomas > > All the comments above so future-me have an easier time finding how to > reproduce. > > Tested-by: Sam Ravnborg > > Sam > > > --- > > Documentation/gpu/todo.rst| 19 ++- > > drivers/gpu/drm/bochs/bochs_kms.c | 1 - > > drivers/gpu/drm/drm_fb_helper.c | 217 -- > > include/drm/drm_mode_config.h | 12 -- > > 4 files changed, 220 insertions(+), 29 deletions(-) > > > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > > index 7e6fc3c04add..638b7f704339 100644 > > --- a/Documentation/gpu/todo.rst > > +++ b/Documentation/gpu/todo.rst > > @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup() > > > > > > Most drivers can use drm_fbdev_generic_setup(). Driver have to implement > > -atomic modesetting and GEM vmap support. Current generic fbdev emulation > > -expects the framebuffer in system memory (or system-like memory). > > +atomic modesetting and GEM vmap support. Historically, generic fbdev > > emulation +expected the framebuffer in system memory or system-like > > memory. By employing +struct dma_buf_map, drivers with frambuffers in I/O > > memory can be supported +as well. > > > > Contact: Maintainer of the driver you plan to convert > > > > Level: Intermediate > > > > +Reimplement functions in drm_fbdev_fb_ops without fbdev > > +--- > > + > > +A number of callback functions in drm_fbdev_fb_ops could benefit from > > +being rewritten without dependencies on the fbdev module. Some of the > > +helpers could further benefit from using struct dma_buf_map instead of > > +raw pointers. > > + > > +Contact: Thomas Zimmermann , Daniel Vetter > > + > > +Level: Advanced > > + > > + > > drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup > > - > > > > diff --git a/drivers/gpu/drm/bochs/bochs_kms.c > > b/drivers/gpu/drm/bochs/bochs_kms.c index 13d0d04c4457..853081d186d5 > > 100644 --- a/drivers/gpu/drm/bochs/bochs_kms.c > > +++ b/drivers/gpu/drm/bochs/bochs_kms.c > > @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs) > > bochs->dev->mode_config.preferred_depth = 24; > > bochs->dev->mode_config.prefer_shadow = 0; > > bochs->dev->mode_config.prefer_shadow_fbdev = 1; > > - bochs->dev->mode_config.fbdev_use_iomem = true; > > bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = > > true; > > bochs->dev->mode_config.funcs = _mode_funcs; > > diff --git a/drivers/gpu/drm/drm_fb_helper.c > > b/drivers/gpu/drm/drm_fb_helper.c index 6212cd7cde1d..462b0c130ebb 100644 > > --- a/drivers/gpu/drm/drm_fb_helper.c > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct > > work_struct *work) } > > > > static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper > > *fb_helper, > > - struct drm_clip_rect *clip) > > +
Re: [Nouveau] [PATCH v4 10/10] drm/fb_helper: Support framebuffers in I/O memory
Hi
On Fri, 16 Oct 2020 12:58:54 +0200 Sam Ravnborg wrote:
> Hi Thomas.
>
> On Thu, Oct 15, 2020 at 02:38:06PM +0200, Thomas Zimmermann wrote:
> > At least sparc64 requires I/O-specific access to framebuffers. This
> > patch updates the fbdev console accordingly.
> >
> > For drivers with direct access to the framebuffer memory, the callback
> > functions in struct fb_ops test for the type of memory and call the rsp
> > fb_sys_ of fb_cfb_ functions.
> >
> > For drivers that employ a shadow buffer, fbdev's blit function retrieves
> > the framebuffer address as struct dma_buf_map, and uses dma_buf_map
> > interfaces to access the buffer.
> >
> > The bochs driver on sparc64 uses a workaround to flag the framebuffer as
> > I/O memory and avoid a HW exception. With the introduction of struct
> > dma_buf_map, this is not required any longer. The patch removes the rsp
> > code from both, bochs and fbdev.
> >
> > v4:
> > * move dma_buf_map changes into separate patch (Daniel)
> > * TODO list: comment on fbdev updates (Daniel)
>
> I have been offline for a while so have not followed all the threads on
> this. So may comments below may well be addressed but I failed to see
> it.
>
> If the point about fb_sync is already addressed/considered then:
> Reviewed-by: Sam Ravnborg
It has not been brought up yet. See below.
>
>
> > Signed-off-by: Thomas Zimmermann
> > ---
> > Documentation/gpu/todo.rst| 19 ++-
> > drivers/gpu/drm/bochs/bochs_kms.c | 1 -
> > drivers/gpu/drm/drm_fb_helper.c | 217 --
> > include/drm/drm_mode_config.h | 12 --
> > 4 files changed, 220 insertions(+), 29 deletions(-)
> >
> > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > index 7e6fc3c04add..638b7f704339 100644
> > --- a/Documentation/gpu/todo.rst
> > +++ b/Documentation/gpu/todo.rst
> > @@ -197,13 +197,28 @@ Convert drivers to use drm_fbdev_generic_setup()
> >
> >
> > Most drivers can use drm_fbdev_generic_setup(). Driver have to implement
> > -atomic modesetting and GEM vmap support. Current generic fbdev emulation
> > -expects the framebuffer in system memory (or system-like memory).
> > +atomic modesetting and GEM vmap support. Historically, generic fbdev
> > emulation +expected the framebuffer in system memory or system-like
> > memory. By employing +struct dma_buf_map, drivers with frambuffers in I/O
> > memory can be supported +as well.
> >
> > Contact: Maintainer of the driver you plan to convert
> >
> > Level: Intermediate
> >
> > +Reimplement functions in drm_fbdev_fb_ops without fbdev
> > +---
> > +
> > +A number of callback functions in drm_fbdev_fb_ops could benefit from
> > +being rewritten without dependencies on the fbdev module. Some of the
> > +helpers could further benefit from using struct dma_buf_map instead of
> > +raw pointers.
> > +
> > +Contact: Thomas Zimmermann , Daniel Vetter
> > +
> > +Level: Advanced
> > +
> > +
> > drm_framebuffer_funcs and drm_mode_config_funcs.fb_create cleanup
> > -
> >
> > diff --git a/drivers/gpu/drm/bochs/bochs_kms.c
> > b/drivers/gpu/drm/bochs/bochs_kms.c index 13d0d04c4457..853081d186d5
> > 100644 --- a/drivers/gpu/drm/bochs/bochs_kms.c
> > +++ b/drivers/gpu/drm/bochs/bochs_kms.c
> > @@ -151,7 +151,6 @@ int bochs_kms_init(struct bochs_device *bochs)
> > bochs->dev->mode_config.preferred_depth = 24;
> > bochs->dev->mode_config.prefer_shadow = 0;
> > bochs->dev->mode_config.prefer_shadow_fbdev = 1;
> > - bochs->dev->mode_config.fbdev_use_iomem = true;
> > bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order =
> > true;
> > bochs->dev->mode_config.funcs = _mode_funcs;
> Good to see this workaround gone again!
>
> > diff --git a/drivers/gpu/drm/drm_fb_helper.c
> > b/drivers/gpu/drm/drm_fb_helper.c index 6212cd7cde1d..462b0c130ebb 100644
> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -372,24 +372,22 @@ static void drm_fb_helper_resume_worker(struct
> > work_struct *work) }
> >
> > static void drm_fb_helper_dirty_blit_real(struct drm_fb_helper
> > *fb_helper,
> > - struct drm_clip_rect *clip)
> > + struct drm_clip_rect *clip,
> > + struct dma_buf_map *dst)
> > {
> > struct drm_framebuffer *fb = fb_helper->fb;
> > unsigned int cpp = fb->format->cpp[0];
> > size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> > void *src = fb_helper->fbdev->screen_buffer + offset;
> > - void *dst = fb_helper->buffer->map.vaddr + offset;
> > size_t len = (clip->x2 - clip->x1) * cpp;
> > unsigned int y;
> >
> > - for (y = clip->y1; y < clip->y2; y++) {
> > - if (!fb_helper->dev->mode_config.fbdev_use_iomem)
> > -
