Re: [PATCH v6 0/2] Add p2p via dmabuf to habanalabs
On Thu, Sep 23, 2021 at 12:22 PM Oded Gabbay wrote: > > On Sat, Sep 18, 2021 at 11:38 AM Oded Gabbay wrote: > > > > On Fri, Sep 17, 2021 at 3:30 PM Daniel Vetter wrote: > > > > > > On Thu, Sep 16, 2021 at 10:10:14AM -0300, Jason Gunthorpe wrote: > > > > On Thu, Sep 16, 2021 at 02:31:34PM +0200, Daniel Vetter wrote: > > > > > On Wed, Sep 15, 2021 at 10:45:36AM +0300, Oded Gabbay wrote: > > > > > > On Tue, Sep 14, 2021 at 7:12 PM Jason Gunthorpe > > > > > > wrote: > > > > > > > > > > > > > > On Tue, Sep 14, 2021 at 04:18:31PM +0200, Daniel Vetter wrote: > > > > > > > > On Sun, Sep 12, 2021 at 07:53:07PM +0300, Oded Gabbay wrote: > > > > > > > > > Hi, > > > > > > > > > Re-sending this patch-set following the release of our > > > > > > > > > user-space TPC > > > > > > > > > compiler and runtime library. > > > > > > > > > > > > > > > > > > I would appreciate a review on this. > > > > > > > > > > > > > > > > I think the big open we have is the entire revoke discussions. > > > > > > > > Having the > > > > > > > > option to let dma-buf hang around which map to random local > > > > > > > > memory ranges, > > > > > > > > without clear ownership link and a way to kill it sounds bad to > > > > > > > > me. > > > > > > > > > > > > > > > > I think there's a few options: > > > > > > > > - We require revoke support. But I've heard rdma really doesn't > > > > > > > > like that, > > > > > > > > I guess because taking out an MR while holding the > > > > > > > > dma_resv_lock would > > > > > > > > be an inversion, so can't be done. Jason, can you recap what > > > > > > > > exactly the > > > > > > > > hold-up was again that makes this a no-go? > > > > > > > > > > > > > > RDMA HW can't do revoke. > > > > > > > > > > Like why? I'm assuming when the final open handle or whatever for > > > > > that MR > > > > > is closed, you do clean up everything? Or does that MR still stick > > > > > around > > > > > forever too? > > > > > > > > It is a combination of uAPI and HW specification. > > > > > > > > revoke here means you take a MR object and tell it to stop doing DMA > > > > without causing the MR object to be destructed. > > > > > > > > All the drivers can of course destruct the MR, but doing such a > > > > destruction without explicit synchronization with user space opens > > > > things up to a serious use-after potential that could be a security > > > > issue. > > > > > > > > When the open handle closes the userspace is synchronized with the > > > > kernel and we can destruct the HW objects safely. > > > > > > > > So, the special HW feature required is 'stop doing DMA but keep the > > > > object in an error state' which isn't really implemented, and doesn't > > > > extend very well to other object types beyond simple MRs. > > > > > > Yeah revoke without destroying the MR doesn't work, and it sounds like > > > revoke by destroying the MR just moves the can of worms around to another > > > place. > > > > > > > > 1. User A opens gaudi device, sets up dma-buf export > > > > > > > > > > 2. User A registers that with RDMA, or anything else that doesn't > > > > > support > > > > > revoke. > > > > > > > > > > 3. User A closes gaudi device > > > > > > > > > > 4. User B opens gaudi device, assumes that it has full control over > > > > > the > > > > > device and uploads some secrets, which happen to end up in the dma-buf > > > > > region user A set up > > > > > > > > I would expect this is blocked so long as the DMABUF exists - eg the > > > > DMABUF will hold a fget on the FD of #1 until the DMABUF is closed, so > > > > that #3 can't actually happen. > > > > > > > > > It's not mlocked memory, it's mlocked memory and I can exfiltrate > > > > > it. > > > > > > > > That's just bug, don't make buggy drivers :) > > > > > > Well yeah, but given that habanalabs hand rolled this I can't just check > > > for the usual things we have to enforce this in drm. And generally you can > > > just open chardevs arbitrarily, and multiple users fighting over each > > > another. The troubles only start when you have private state or memory > > > allocations of some kind attached to the struct file (instead of the > > > underlying device), or something else that requires device exclusivity. > > > There's no standard way to do that. > > > > > > Plus in many cases you really want revoke on top (can't get that here > > > unfortunately it seems), and the attempts to get towards a generic > > > revoke() just never went anywhere. So again it's all hand-rolled > > > per-subsystem. *insert lament about us not having done this through a > > > proper subsystem* > > > > > > Anyway it sounds like the code takes care of that. > > > -Daniel > > > > Daniel, Jason, > > Thanks for reviewing this code. > > > > Can I get an R-B / A-B from you for this patch-set ? > > > > Thanks, > > Oded > > A kind reminder. > > Thanks, > Oded Hi, I know last week was LPC and maybe this got lost in the inbox, so I'm sending it again to make sure you got my request for R-B / A-
Re: [PATCH] [RFC] qcom_scm: hide Kconfig symbol
Arnd Bergmann writes: > From: Arnd Bergmann > > Now that SCM can be a loadable module, we have to add another > dependency to avoid link failures when ipa or adreno-gpu are > built-in: > > aarch64-linux-ld: drivers/net/ipa/ipa_main.o: in function `ipa_probe': > ipa_main.c:(.text+0xfc4): undefined reference to `qcom_scm_is_available' > > ld.lld: error: undefined symbol: qcom_scm_is_available referenced by adreno_gpu.c gpu/drm/msm/adreno/adreno_gpu.o:(adreno_zap_shader_load) in archive drivers/built-in.a > > This can happen when CONFIG_ARCH_QCOM is disabled and we don't select > QCOM_MDT_LOADER, but some other module selects QCOM_SCM. Ideally we'd > use a similar dependency here to what we have for QCOM_RPROC_COMMON, > but that causes dependency loops from other things selecting QCOM_SCM. > > This appears to be an endless problem, so try something different this > time: > > - CONFIG_QCOM_SCM becomes a hidden symbol that nothing 'depends on' >but that is simply selected by all of its users > > - All the stubs in include/linux/qcom_scm.h can go away > > - arm-smccc.h needs to provide a stub for __arm_smccc_smc() to >allow compile-testing QCOM_SCM on all architectures. > > - To avoid a circular dependency chain involving RESET_CONTROLLER >and PINCTRL_SUNXI, change the 'depends on RESET_CONTROLLER' in >the latter one to 'select'. > > The last bit is rather annoying, as drivers should generally never > 'select' another subsystem, and about half the users of the reset > controller interface do this anyway. > > Nevertheless, this version seems to pass all my randconfig tests > and is more robust than any of the prior versions. > > Comments? > > Signed-off-by: Arnd Bergmann [...] > diff --git a/drivers/net/wireless/ath/ath10k/Kconfig > b/drivers/net/wireless/ath/ath10k/Kconfig > index 741289e385d5..ca007b800f75 100644 > --- a/drivers/net/wireless/ath/ath10k/Kconfig > +++ b/drivers/net/wireless/ath/ath10k/Kconfig > @@ -44,7 +44,7 @@ config ATH10K_SNOC > tristate "Qualcomm ath10k SNOC support" > depends on ATH10K > depends on ARCH_QCOM || COMPILE_TEST > - depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y > + select QCOM_SCM > select QCOM_QMI_HELPERS > help > This module adds support for integrated WCN3990 chip connected I assume I can continue to build test ATH10K_SNOC with x86 as before? That's important for me. If yes, then: Acked-by: Kalle Valo -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] [RFC] qcom_scm: hide Kconfig symbol
On Tue, Sep 28, 2021 at 9:05 AM Kalle Valo wrote: > Arnd Bergmann writes: > > From: Arnd Bergmann > I assume I can continue to build test ATH10K_SNOC with x86 as before? > That's important for me. If yes, then: > > Acked-by: Kalle Valo > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches Yes, the difference is that this will then also build the qcom_scm module, but that should not cause any problems after the other changes in this patch. Arnd
[PATCH 3/6] drm/aspeed: Add AST2600 support
From: Joel Stanley The values for the threshold and scan line size come from the ASPEED SDK. The DAC register is SCUC0 in the AST2600 datasheet. It has the same layout as the previous generations. Signed-off-by: Joel Stanley Signed-off-by: tommy-huang --- drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c index b53fee6f1c17..ea9cb0a4f16c 100644 --- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c @@ -79,9 +79,16 @@ static const struct aspeed_gfx_config ast2500_config = { .scan_line_max = 128, }; +static const struct aspeed_gfx_config ast2600_config = { + .dac_reg = 0xc0, + .throd_val = CRT_THROD_LOW(0x50) | CRT_THROD_HIGH(0x70), + .scan_line_max = 128, +}; + static const struct of_device_id aspeed_gfx_match[] = { { .compatible = "aspeed,ast2400-gfx", .data = &ast2400_config }, { .compatible = "aspeed,ast2500-gfx", .data = &ast2500_config }, + { .compatible = "aspeed,ast2600-gfx", .data = &ast2600_config }, { }, }; MODULE_DEVICE_TABLE(of, aspeed_gfx_match); -- 2.17.1
[PATCH 4/6] HACK: drm/aspeed: INTR_STS hadndling
From: Joel Stanley The 2600 uses this register differently. THis is a TODO to come up with a method of handling that. Signed-off-by: Joel Stanley Signed-off-by: tommy-huang --- drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c index ea9cb0a4f16c..33095477cc03 100644 --- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c @@ -126,7 +126,8 @@ static irqreturn_t aspeed_gfx_irq_handler(int irq, void *data) if (reg & CRT_CTRL_VERTICAL_INTR_STS) { drm_crtc_handle_vblank(&priv->pipe.crtc); - writel(reg, priv->base + CRT_CTRL1); + /* TODO */ + writel(CRT_CTRL_VERTICAL_INTR_STS, priv->base + CRT_STATUS); return IRQ_HANDLED; } -- 2.17.1
[PATCH] dt-bindings: display: simple: hardware can use ddc-i2c-bus
Both hardware and driver can communicate DDC over i2c bus. Fixes warnings as: arch/arm/boot/dts/tegra20-paz00.dt.yaml: panel: 'ddc-i2c-bus' does not match any of the regexes: 'pinctrl-[0-9]+' From schema: /home/runner/work/linux/linux/Documentation/devicetree/bindings/display/panel/panel-simple.yaml Signed-off-by: David Heidelberg --- .../devicetree/bindings/display/panel/panel-simple.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml index 31f678636717..e4d93e0ddfc3 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml @@ -319,6 +319,7 @@ properties: - yes-optoelectronics,ytc700tlag-05-201c backlight: true + ddc-i2c-bus: true enable-gpios: true port: true power-supply: true -- 2.33.0
[PATCH 1/6] ARM: dts: aspeed: Add GFX node to AST2600
From: Joel Stanley The GFX device is present in the AST2600 SoC. Signed-off-by: Joel Stanley Signed-off-by: tommy-huang --- arch/arm/boot/dts/aspeed-g6.dtsi | 11 +++ 1 file changed, 11 insertions(+) diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed-g6.dtsi index 1b47be1704f8..e38c3742761b 100644 --- a/arch/arm/boot/dts/aspeed-g6.dtsi +++ b/arch/arm/boot/dts/aspeed-g6.dtsi @@ -351,6 +351,17 @@ quality = <100>; }; + gfx: display@1e6e6000 { + compatible = "aspeed,ast2600-gfx", "aspeed,ast2500-gfx", "syscon"; + reg = <0x1e6e6000 0x1000>; + reg-io-width = <4>; + clocks = <&syscon ASPEED_CLK_GATE_D1CLK>; + resets = <&syscon ASPEED_RESET_GRAPHICS>; + syscon = <&syscon>; + status = "disabled"; + interrupts = ; + }; + xdma: xdma@1e6e7000 { compatible = "aspeed,ast2600-xdma"; reg = <0x1e6e7000 0x100>; -- 2.17.1
[PATCH 5/6] HACK: drm/aspeed: Paramterise modes
From: Joel Stanley The AST2600 will run at 1024x868. Signed-off-by: Joel Stanley Signed-off-by: tommy-huang --- drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c index 33095477cc03..11a44b08bd3f 100644 --- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c @@ -99,7 +99,7 @@ static const struct drm_mode_config_funcs aspeed_gfx_mode_config_funcs = { .atomic_commit = drm_atomic_helper_commit, }; -static int aspeed_gfx_setup_mode_config(struct drm_device *drm) +static int aspeed_gfx_setup_mode_config(struct drm_device *drm, int width, int height) { int ret; @@ -109,8 +109,8 @@ static int aspeed_gfx_setup_mode_config(struct drm_device *drm) drm->mode_config.min_width = 0; drm->mode_config.min_height = 0; - drm->mode_config.max_width = 800; - drm->mode_config.max_height = 600; + drm->mode_config.max_width = width; + drm->mode_config.max_height = height; drm->mode_config.funcs = &aspeed_gfx_mode_config_funcs; return ret; @@ -201,7 +201,7 @@ static int aspeed_gfx_load(struct drm_device *drm) writel(0, priv->base + CRT_CTRL1); writel(0, priv->base + CRT_CTRL2); - ret = aspeed_gfx_setup_mode_config(drm); + ret = aspeed_gfx_setup_mode_config(drm, 800, 600); if (ret < 0) return ret; -- 2.17.1
[PATCH 6/6] dt-bindings: gpu: Add ASPEED GFX bindings document
Add ast2600-gfx description for gfx driver. Signed-off-by: tommy-huang --- Documentation/devicetree/bindings/gpu/aspeed-gfx.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/gpu/aspeed-gfx.txt b/Documentation/devicetree/bindings/gpu/aspeed-gfx.txt index 958bdf962339..29ecf119cef2 100644 --- a/Documentation/devicetree/bindings/gpu/aspeed-gfx.txt +++ b/Documentation/devicetree/bindings/gpu/aspeed-gfx.txt @@ -3,6 +3,7 @@ Device tree configuration for the GFX display device on the ASPEED SoCs Required properties: - compatible * Must be one of the following: + + aspeed,ast2600-gfx + aspeed,ast2500-gfx + aspeed,ast2400-gfx * In addition, the ASPEED pinctrl bindings require the 'syscon' property to -- 2.17.1
[PATCH 0/6] *** Add AST2600 GFX node ***
Add AST2600 GFX support first. Joel Stanley (5): ARM: dts: aspeed: Add GFX node to AST2600 ARM: dts: aspeed: ast2600-evb: Enable GFX device drm/aspeed: Add AST2600 support HACK: drm/aspeed: INTR_STS hadndling HACK: drm/aspeed: Paramterise modes tommy-huang (1): dt-bindings: gpu: Add ASPEED GFX bindings document .../devicetree/bindings/gpu/aspeed-gfx.txt | 1 + arch/arm/boot/dts/aspeed-ast2600-evb.dts | 13 + arch/arm/boot/dts/aspeed-g6.dtsi | 11 +++ drivers/gpu/drm/aspeed/aspeed_gfx_drv.c| 18 +- 4 files changed, 38 insertions(+), 5 deletions(-) -- 2.17.1
[PATCH 2/6] ARM: dts: aspeed: ast2600-evb: Enable GFX device
From: Joel Stanley Enable the GFX device with a framebuffer memory region. Signed-off-by: Joel Stanley Signed-off-by: tommy-huang --- arch/arm/boot/dts/aspeed-ast2600-evb.dts | 13 + 1 file changed, 13 insertions(+) diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed-ast2600-evb.dts index b7eb552640cb..195ccd1952da 100644 --- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts @@ -23,6 +23,19 @@ reg = <0x8000 0x8000>; }; + reserved-memory { + #address-cells = <1>; + #size-cells = <1>; + ranges; + + gfx_memory: framebuffer { + size = <0x0100>; + alignment = <0x0100>; + compatible = "shared-dma-pool"; + reusable; + }; + }; + vcc_sdhci0: regulator-vcc-sdhci0 { compatible = "regulator-fixed"; regulator-name = "SDHCI0 Vcc"; -- 2.17.1
Re: [PATCH] dma-buf: move dma-buf symbols into the DMA_BUF module namespace
On Sat, Sep 25, 2021 at 03:47:00PM +0200, Greg Kroah-Hartman wrote: > In order to better track where in the kernel the dma-buf code is used, > put the symbols in the namespace DMA_BUF and modify all users of the > symbols to properly import the namespace to not break the build at the > same time. > > Now the output of modinfo shows the use of these symbols, making it > easier to watch for users over time: > > $ modinfo drivers/misc/fastrpc.ko | grep import > import_ns: DMA_BUF > > Cc: Sumit Semwal > Cc: "Christian König" > Cc: Alex Deucher > Cc: "Pan, Xinhui" > Cc: David Airlie > Cc: Daniel Vetter > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: Mauro Carvalho Chehab > Cc: Arnd Bergmann > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Greg Kroah-Hartman > --- > > The topic of dma-buf came up in the Maintainer's summit yesterday, and > one comment was to put the symbols in their own module namespace, to > make it easier to notice and track who was using them. This patch does > so, and finds some "interesting" users of the api already in the tree. Yeah, the interesting ones is why I added the dma-buf wildcard match a while ago. Since that landed I don't think anything escaped. Should we perhaps also add K: MODULE_IMPORT_NS(DMA_BUF); to the dma-buf MAINATINERS entry? Entirely untested, also no idea whether there's not a better way to match for module namespaces. Either way: Acked-by: Daniel Vetter > > Only test-built on x86 allmodconfig, don't know what other arches will > pick up, will let 0-day run on it for a bit... > > Comments? > > > > drivers/dma-buf/dma-buf.c | 34 +-- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 3 ++ > drivers/gpu/drm/drm_gem_framebuffer_helper.c | 3 ++ > drivers/gpu/drm/drm_prime.c | 3 ++ > drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 3 ++ > drivers/gpu/drm/exynos/exynos_drm_gem.c | 3 ++ > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c| 3 ++ > drivers/gpu/drm/tegra/gem.c | 3 ++ > drivers/gpu/drm/vmwgfx/ttm_object.c | 3 ++ > drivers/infiniband/core/umem_dmabuf.c | 3 ++ > .../media/common/videobuf2/videobuf2-core.c | 1 + > .../common/videobuf2/videobuf2-dma-contig.c | 1 + > .../media/common/videobuf2/videobuf2-dma-sg.c | 1 + > .../common/videobuf2/videobuf2-vmalloc.c | 1 + > drivers/misc/fastrpc.c| 1 + > .../staging/media/tegra-vde/dmabuf-cache.c| 3 ++ > drivers/tee/tee_shm.c | 3 ++ > drivers/virtio/virtio_dma_buf.c | 1 + > drivers/xen/gntdev-dmabuf.c | 3 ++ > samples/vfio-mdev/mbochs.c| 1 + > 20 files changed, 60 insertions(+), 17 deletions(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 63d32261b63f..6c2b5ea828a6 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -610,7 +610,7 @@ struct dma_buf *dma_buf_export(const struct > dma_buf_export_info *exp_info) > module_put(exp_info->owner); > return ERR_PTR(ret); > } > -EXPORT_SYMBOL_GPL(dma_buf_export); > +EXPORT_SYMBOL_NS_GPL(dma_buf_export, DMA_BUF); > > /** > * dma_buf_fd - returns a file descriptor for the given struct dma_buf > @@ -634,7 +634,7 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags) > > return fd; > } > -EXPORT_SYMBOL_GPL(dma_buf_fd); > +EXPORT_SYMBOL_NS_GPL(dma_buf_fd, DMA_BUF); > > /** > * dma_buf_get - returns the struct dma_buf related to an fd > @@ -660,7 +660,7 @@ struct dma_buf *dma_buf_get(int fd) > > return file->private_data; > } > -EXPORT_SYMBOL_GPL(dma_buf_get); > +EXPORT_SYMBOL_NS_GPL(dma_buf_get, DMA_BUF); > > /** > * dma_buf_put - decreases refcount of the buffer > @@ -679,7 +679,7 @@ void dma_buf_put(struct dma_buf *dmabuf) > > fput(dmabuf->file); > } > -EXPORT_SYMBOL_GPL(dma_buf_put); > +EXPORT_SYMBOL_NS_GPL(dma_buf_put, DMA_BUF); > > static void mangle_sg_table(struct sg_table *sg_table) > { > @@ -810,7 +810,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct > device *dev, > dma_buf_detach(dmabuf, attach); > return ERR_PTR(ret); > } > -EXPORT_SYMBOL_GPL(dma_buf_dynamic_attach); > +EXPORT_SYMBOL_NS_GPL(dma_buf_dynamic_attach, DMA_BUF); > > /** > * dma_buf_attach - Wrapper for dma_buf_dynamic_attach > @@ -825,7 +825,7 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf > *dmabuf, > { > return dma_buf_dynamic_attach(dmabuf, dev, NULL, NULL); > } > -EXPORT_SYMBOL_GPL(dma_buf_attach); > +EXPORT_SYMBOL_NS_GPL(dma_buf_attach, DMA_BUF); > > static void __unmap_dma_buf(struct dma_buf_attachment *attach, > struct sg_table *sg_table, > @@ -871,7 +871,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct > dma_buf_attachment *attach) > > kfree(attach); > } > -EXPORT_SYMBOL_GPL(dma_buf_detach)
Re: refactor the i915 GVT support
Hey guys: After some investigation, I found the root cause this problem ("i915" module loading will be stuck with Christoph's refactor patches), which can be reproduced by building both i915 and kvmgt as kernel module and the loading i915. The root cause is: in Linux kernel loading, before a kernel module loading is finished, its symbols can not be reached by other module when resolving the symbols (even they can be found in /proc/kallsyms). Because the status of the kernel module is MODULE_STATE_COMING and resolve_symbol() from another kernel module will check this and return a -EBUSY. In this case, before i915 loading is finished, the requested module "kvmgt" cannot reach the symbols in module i915. Thus it kept waiting and left message like below in the dmesg: [ 644.152021] kvmgt: gave up waiting for init of module i915. [ 644.152039] kvmgt: Unknown symbol i915_gem_object_set_to_cpu_domain (err -16) [ 674.871409] kvmgt: gave up waiting for init of module i915. [ 674.871427] kvmgt: Unknown symbol intel_ring_begin (err -16) [ 705.590586] kvmgt: gave up waiting for init of module i915. [ 705.590604] kvmgt: Unknown symbol i915_vma_move_to_active (err -16) [ 736.310230] kvmgt: gave up waiting for init of module i915. [ 736.310248] kvmgt: Unknown symbol shmem_unpin_map (err -16) ... The error message is from execution path below: kernel/module.c: [i915 module loading] -> request_module("kvmgt")->[modprobe]->init_module("kvmgt")->load_module()->simplify_symbols()->resolve_symbol_wait(): static const struct kernel_symbol * resolve_symbol_wait(struct module *mod, const struct load_info *info, const char *name) { const struct kernel_symbol *ksym; char owner[MODULE_NAME_LEN]; if (wait_event_interruptible_timeout(module_wq, !IS_ERR(ksym = resolve_symbol(mod, info, name, owner)) || PTR_ERR(ksym) != -EBUSY, 30 * HZ) <= 0) { pr_warn("%s: gave up waiting for init of module %s.\n", mod->name, owner); } code: https://github.com/intel/gvt-linux/blob/bd950a66c7919d7121d2530f30984351534a96dc/kernel/module.c#L1452 In resolve_symbol_wait(), it calls resolve_symbol() to resolve the symbols in "i915". In resolve_symbol() -> ref_module() -> strong_try_module_get(), it will check the status of the module which owns the symbol. static inline int strong_try_module_get(struct module *mod) { BUG_ON(mod && mod->state == MODULE_STATE_UNFORMED); if (mod && mod->state == MODULE_STATE_COMING) return -EBUSY; if (try_module_get(mod)) return 0; else return -ENOENT; } code:https://github.com/intel/gvt-linux/blob/bd950a66c7919d7121d2530f30984351534a96dc/kernel/module.c#L318 But unfortunately, this execution path begins in i915 module loading, at this time, the status of kernel module "i915" is MODULE_STATE_COMING until loading of "kvmgt" is finished. Thus a -EBUSY is always returned when kernel is trying to resolve symbols for "kvmgt". This patch below might need re-work: Author: Christoph Hellwig Date: Wed Jul 21 17:53:38 2021 +0200 drm/i915/gvt: move the gvt code into kvmgt.ko Instead of having an option to build the gvt code into the main i915 module, just move it into the kvmgt.ko module. This only requires a new struct with three entries that the main i915 module needs to request before enabling VGPU passthrough operations. This also conveniently streamlines the GVT initialization and avoids the need for the global device pointer. Signed-off-by: Christoph Hellwig Signed-off-by: Zhenyu Wang Link: http://patchwork.freedesktop.org/patch/msgid/20210721155355.173183-5-...@lst.de Acked-by: Zhenyu Wang On 8/26/21 6:12 AM, Zhenyu Wang wrote: > On 2021.08.20 12:56:34 -0700, Luis Chamberlain wrote: >> On Fri, Aug 20, 2021 at 04:17:24PM +0200, Christoph Hellwig wrote: >>> On Thu, Aug 19, 2021 at 04:29:29PM +0800, Zhenyu Wang wrote: I'm working on below patch to resolve this. But I met a weird issue in case when building i915 as module and also kvmgt module, it caused busy wait on request_module("kvmgt") when boot, it doesn't happen if building i915 into kernel. I'm not sure what could be the reason? >>> Luis, do you know if there is a problem with a request_module from >>> a driver ->probe routine that is probably called by a module_init >>> function itself? >> Generally no, but you can easily foot yourself in the feet by creating >> cross dependencies and not dealing with them properly. I'd make sure >> to keep module initialization as simple as possible, and run whatever >> takes more time asynchronously, then use a state machine to allow >> you to verify where you are in the initialization phase or query it >> or wait for a completion with a timeout. >> >> It seems the code in question is getting some spring cleaning, and its >> unclear where the code is I c
[GIT PULL] exynos-drm-fixes
Hi Dave, Just one clean up to use helper function. Please kindly let me know if there is any problem. Thanks, Inki Dae The following changes since commit 6880fa6c56601bb8ed59df6c30fd390cc5f6dd8f: Linux 5.15-rc1 (2021-09-12 16:28:37 -0700) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos tags/exynos-drm-fixes-for-v5.15-rc4 for you to fetch changes up to 17ac76e050c51497e75871a43aa3328ba54cdafd: drm/exynos: Make use of the helper function devm_platform_ioremap_resource() (2021-09-16 14:05:07 +0900) One cleanup - Use devm_platform_ioremap_resource() helper function instead of old one. Cai Huoqing (1): drm/exynos: Make use of the helper function devm_platform_ioremap_resource() drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 4 +--- drivers/gpu/drm/exynos/exynos_drm_dsi.c | 4 +--- drivers/gpu/drm/exynos/exynos_drm_fimc.c | 5 + drivers/gpu/drm/exynos/exynos_drm_fimd.c | 4 +--- drivers/gpu/drm/exynos/exynos_drm_g2d.c | 5 + drivers/gpu/drm/exynos/exynos_drm_gsc.c | 6 +- drivers/gpu/drm/exynos/exynos_drm_rotator.c | 4 +--- drivers/gpu/drm/exynos/exynos_drm_scaler.c| 4 +--- drivers/gpu/drm/exynos/exynos_hdmi.c | 4 +--- 9 files changed, 9 insertions(+), 31 deletions(-)
Re: [PATCH] dma-buf: move dma-buf symbols into the DMA_BUF module namespace
On Tue, Sep 28, 2021 at 09:31:45AM +0200, Daniel Vetter wrote: > On Sat, Sep 25, 2021 at 03:47:00PM +0200, Greg Kroah-Hartman wrote: > > In order to better track where in the kernel the dma-buf code is used, > > put the symbols in the namespace DMA_BUF and modify all users of the > > symbols to properly import the namespace to not break the build at the > > same time. > > > > Now the output of modinfo shows the use of these symbols, making it > > easier to watch for users over time: > > > > $ modinfo drivers/misc/fastrpc.ko | grep import > > import_ns: DMA_BUF > > > > Cc: Sumit Semwal > > Cc: "Christian König" > > Cc: Alex Deucher > > Cc: "Pan, Xinhui" > > Cc: David Airlie > > Cc: Daniel Vetter > > Cc: Maarten Lankhorst > > Cc: Maxime Ripard > > Cc: Thomas Zimmermann > > Cc: Mauro Carvalho Chehab > > Cc: Arnd Bergmann > > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Greg Kroah-Hartman > > --- > > > > The topic of dma-buf came up in the Maintainer's summit yesterday, and > > one comment was to put the symbols in their own module namespace, to > > make it easier to notice and track who was using them. This patch does > > so, and finds some "interesting" users of the api already in the tree. > > Yeah, the interesting ones is why I added the dma-buf wildcard match a > while ago. Since that landed I don't think anything escaped. Should we > perhaps also add > > K: MODULE_IMPORT_NS(DMA_BUF); > > to the dma-buf MAINATINERS entry? Entirely untested, also no idea whether > there's not a better way to match for module namespaces. Either way: I don't know if that would really work, if anything, just make the MAINTAINERS file harder to maintain :) > Acked-by: Daniel Vetter Thanks for the review, I'll send out a v2 later today... greg k-h
[PATCH 2/2] [v2] qcom_scm: hide Kconfig symbol
From: Arnd Bergmann Now that SCM can be a loadable module, we have to add another dependency to avoid link failures when ipa or adreno-gpu are built-in: aarch64-linux-ld: drivers/net/ipa/ipa_main.o: in function `ipa_probe': ipa_main.c:(.text+0xfc4): undefined reference to `qcom_scm_is_available' ld.lld: error: undefined symbol: qcom_scm_is_available >>> referenced by adreno_gpu.c >>> gpu/drm/msm/adreno/adreno_gpu.o:(adreno_zap_shader_load) in >>> archive drivers/built-in.a This can happen when CONFIG_ARCH_QCOM is disabled and we don't select QCOM_MDT_LOADER, but some other module selects QCOM_SCM. Ideally we'd use a similar dependency here to what we have for QCOM_RPROC_COMMON, but that causes dependency loops from other things selecting QCOM_SCM. This appears to be an endless problem, so try something different this time: - CONFIG_QCOM_SCM becomes a hidden symbol that nothing 'depends on' but that is simply selected by all of its users - All the stubs in include/linux/qcom_scm.h can go away - arm-smccc.h needs to provide a stub for __arm_smccc_smc() to allow compile-testing QCOM_SCM on all architectures. - To avoid a circular dependency chain involving RESET_CONTROLLER and PINCTRL_SUNXI, drop the 'select RESET_CONTROLLER' statement. According to my testing this still builds fine, and the QCOM platform selects this symbol already. Acked-by: Kalle Valo Signed-off-by: Arnd Bergmann --- Changes in v2: - drop the 'select RESET_CONTROLLER' line, rather than adding more of the same --- drivers/firmware/Kconfig| 5 +- drivers/gpu/drm/msm/Kconfig | 4 +- drivers/iommu/Kconfig | 2 +- drivers/media/platform/Kconfig | 2 +- drivers/mmc/host/Kconfig| 2 +- drivers/net/ipa/Kconfig | 1 + drivers/net/wireless/ath/ath10k/Kconfig | 2 +- drivers/pinctrl/qcom/Kconfig| 3 +- include/linux/arm-smccc.h | 10 include/linux/qcom_scm.h| 71 - 10 files changed, 20 insertions(+), 82 deletions(-) diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig index 220a58cf0a44..cda7d7162cbb 100644 --- a/drivers/firmware/Kconfig +++ b/drivers/firmware/Kconfig @@ -203,10 +203,7 @@ config INTEL_STRATIX10_RSU Say Y here if you want Intel RSU support. config QCOM_SCM - tristate "Qcom SCM driver" - depends on ARM || ARM64 - depends on HAVE_ARM_SMCCC - select RESET_CONTROLLER + tristate config QCOM_SCM_DOWNLOAD_MODE_DEFAULT bool "Qualcomm download mode enabled by default" diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig index e9c6af78b1d7..3ddf739a6f9b 100644 --- a/drivers/gpu/drm/msm/Kconfig +++ b/drivers/gpu/drm/msm/Kconfig @@ -17,7 +17,7 @@ config DRM_MSM select DRM_SCHED select SHMEM select TMPFS - select QCOM_SCM if ARCH_QCOM + select QCOM_SCM select WANT_DEV_COREDUMP select SND_SOC_HDMI_CODEC if SND_SOC select SYNC_FILE @@ -55,7 +55,7 @@ config DRM_MSM_GPU_SUDO config DRM_MSM_HDMI_HDCP bool "Enable HDMI HDCP support in MSM DRM driver" - depends on DRM_MSM && QCOM_SCM + depends on DRM_MSM default y help Choose this option to enable HDCP state machine diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index 124c41adeca1..989c83acbfee 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -308,7 +308,7 @@ config APPLE_DART config ARM_SMMU tristate "ARM Ltd. System MMU (SMMU) Support" depends on ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64) - depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y + select QCOM_SCM select IOMMU_API select IOMMU_IO_PGTABLE_LPAE select ARM_DMA_USE_IOMMU if ARM diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig index 157c924686e4..80321e03809a 100644 --- a/drivers/media/platform/Kconfig +++ b/drivers/media/platform/Kconfig @@ -565,7 +565,7 @@ config VIDEO_QCOM_VENUS depends on VIDEO_DEV && VIDEO_V4L2 && QCOM_SMEM depends on (ARCH_QCOM && IOMMU_DMA) || COMPILE_TEST select QCOM_MDT_LOADER if ARCH_QCOM - select QCOM_SCM if ARCH_QCOM + select QCOM_SCM select VIDEOBUF2_DMA_CONTIG select V4L2_MEM2MEM_DEV help diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index 71313961cc54..95b3511b0560 100644 --- a/drivers/mmc/host/Kconfig +++ b/drivers/mmc/host/Kconfig @@ -547,7 +547,7 @@ config MMC_SDHCI_MSM depends on MMC_SDHCI_PLTFM select MMC_SDHCI_IO_ACCESSORS select MMC_CQHCI - select QCOM_SCM if MMC_CRYPTO && ARCH_QCOM + select QCOM_SCM if MMC_CRYPTO help This selects the Secure Digital Host Controller Interface (SDHCI) support present in Qualcomm SOCs. The controller
Re: [PATCH v3 8/9] dma-buf/sync_file: Add SET_DEADLINE ioctl
On Mon, 27 Sep 2021 07:36:05 -0700 Rob Clark wrote: > On Mon, Sep 27, 2021 at 1:42 AM Pekka Paalanen wrote: > > > > On Fri, 3 Sep 2021 11:47:59 -0700 > > Rob Clark wrote: > > > > > From: Rob Clark > > > > > > The initial purpose is for igt tests, but this would also be useful for > > > compositors that wait until close to vblank deadline to make decisions > > > about which frame to show. > > > > > > Signed-off-by: Rob Clark > > > --- > > > drivers/dma-buf/sync_file.c| 19 +++ > > > include/uapi/linux/sync_file.h | 20 > > > 2 files changed, 39 insertions(+) ... > > > diff --git a/include/uapi/linux/sync_file.h > > > b/include/uapi/linux/sync_file.h > > > index ee2dcfb3d660..f67d4ffe7566 100644 > > > --- a/include/uapi/linux/sync_file.h > > > +++ b/include/uapi/linux/sync_file.h > > > @@ -67,6 +67,18 @@ struct sync_file_info { > > > __u64 sync_fence_info; > > > }; > > > > > > +/** > > > + * struct sync_set_deadline - set a deadline on a fence > > > + * @tv_sec: seconds elapsed since epoch > > > + * @tv_nsec: nanoseconds elapsed since the time given by the tv_sec > > > + * @pad: must be zero > > > > Hi Rob, > > > > I think you need to specify which clock this timestamp must be in. > > > > Which epoch? Sounds a bit like CLOCK_REALTIME to me which would not > > make sense. > > It should be monotonic.. same clock as is used for vblank timestamps, > which I assume that would be the most straightforward thing for > compositors to use Yes, it would. Just need to document that. :-) Thanks, pq pgpUbtc1YTko6.pgp Description: OpenPGP digital signature
Re: Regression with mainline kernel on rpi4
Hi Daniel, On Sat, Sep 25, 2021 at 12:50:17AM +0200, Daniel Vetter wrote: > On Fri, Sep 24, 2021 at 3:30 PM Maxime Ripard wrote: > > > > On Wed, Sep 22, 2021 at 01:25:21PM -0700, Linus Torvalds wrote: > > > On Wed, Sep 22, 2021 at 1:19 PM Sudip Mukherjee > > > wrote: > > > > > > > > I added some debugs to print the addresses, and I am getting: > > > > [ 38.813809] sudip crtc > > > > > > > > This is from struct drm_crtc *crtc = connector->state->crtc; > > > > > > Yeah, that was my personal suspicion, because while the line number > > > implied "crtc->state" being NULL, the drm data structure documentation > > > and other drivers both imply that "crtc" was the more likely one. > > > > > > I suspect a simple > > > > > > if (!crtc) > > > return; > > > > > > in vc4_hdmi_set_n_cts() is at least part of the fix for this all, but > > > I didn't check if there is possibly something else that needs to be > > > done too. > > > > Thanks for the decode_stacktrace.sh and the follow-up > > > > Yeah, it looks like we have several things wrong here: > > > > * we only check that connector->state is set, and not > > connector->state->crtc indeed. > > > > * We also check only in startup(), so at open() and not later on when > > the sound streaming actually start. This has been there for a while, > > so I guess it's never really been causing a practical issue before. > > You also have no locking Indeed. Do we just need locking to prevent a concurrent audio setup and modeset, or do you have another corner case in mind? Also, generally, what locks should we make sure we have locked when accessing the connector and CRTC state? drm_mode_config.connection_mutex and drm_mode_config.mutex, respectively? > plus looking at ->state objects outside of atomic commit machinery > makes no sense because you're not actually in sync with the hw state. > Relevant bits need to be copied over at commit time, protected by some > spinlock (and that spinlock also needs to be held over whatever other > stuff you're setting to make sure we don't get a funny out-of-sync > state anywhere). If we already have a lock protecting against having both an ASoC and KMS function running, it's not clear to me what the spinlock would prevent here? Maxime
[PATCH 02/10] drm/gma500: Use to_gtt_range() everywhere
Convert upcasts from struct drm_gem_object to struct gtt_range to to_gtt_range(). Some places used container_of() directly. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/gem.c | 4 ++-- drivers/gpu/drm/gma500/gma_display.c | 7 +++ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index 734bcb7a80c8..ff2c1d64689e 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -106,7 +106,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf); static void psb_gem_free_object(struct drm_gem_object *obj) { - struct gtt_range *gtt = container_of(obj, struct gtt_range, gem); + struct gtt_range *gtt = to_gtt_range(obj); /* Remove the list map if one is present */ drm_gem_free_mmap_offset(obj); @@ -256,7 +256,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf) dev = obj->dev; dev_priv = to_drm_psb_private(dev); - r = container_of(obj, struct gtt_range, gem); /* Get the gtt range */ + r = to_gtt_range(obj); /* Make sure we don't parallel update on a fault, nor move or remove something from beneath our feet */ diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c index ecf8153416ac..8285358fac01 100644 --- a/drivers/gpu/drm/gma500/gma_display.c +++ b/drivers/gpu/drm/gma500/gma_display.c @@ -349,8 +349,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, /* Unpin the old GEM object */ if (gma_crtc->cursor_obj) { - gt = container_of(gma_crtc->cursor_obj, - struct gtt_range, gem); + gt = to_gtt_range(gma_crtc->cursor_obj); psb_gtt_unpin(gt); drm_gem_object_put(gma_crtc->cursor_obj); gma_crtc->cursor_obj = NULL; @@ -376,7 +375,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, goto unref_cursor; } - gt = container_of(obj, struct gtt_range, gem); + gt = to_gtt_range(obj); /* Pin the memory into the GTT */ ret = psb_gtt_pin(gt); @@ -426,7 +425,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, /* unpin the old bo */ if (gma_crtc->cursor_obj) { - gt = container_of(gma_crtc->cursor_obj, struct gtt_range, gem); + gt = to_gtt_range(gma_crtc->cursor_obj); psb_gtt_unpin(gt); drm_gem_object_put(gma_crtc->cursor_obj); } -- 2.33.0
[PATCH 04/10] drm/gma500: Allocate GTT ranges in stolen memory with psb_gem_create()
Support private objects for stolen memory in psb_gem_create() and convert users to psb_gem_create(). For stolen memory, psb_gem_create() now initializes the GEM object via drm_gem_private_object_init(). In the fbdev setup, replace the open-coded initialization of struct gtt_range with a call to psb_gem_create(). Use drm_gem_object_put() for release. In the cursor setup, use psb_gem_create() and get a real GEM object. Previously the allocated instance of struct gtt_range was only partially initialized. Release the cursor GEM object in gma_crtc_destroy(). The release was missing from the original code. With the conversion of all callers to psb_gem_create(), the extern declarations of psb_gtt_alloc_range, psb_gtt_free_range and psb_gem_object_func are not required any longer. Declare them as static inline. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/framebuffer.c | 44 ++ drivers/gpu/drm/gma500/gem.c | 22 ++- drivers/gpu/drm/gma500/gem.h | 5 --- drivers/gpu/drm/gma500/gma_display.c | 3 ++ drivers/gpu/drm/gma500/psb_intel_display.c | 5 +-- 5 files changed, 29 insertions(+), 50 deletions(-) diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index ce92d11bd20f..3ea6679ccd38 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -224,31 +224,6 @@ static struct drm_framebuffer *psb_framebuffer_create return fb; } -/** - * psbfb_alloc - allocate frame buffer memory - * @dev: the DRM device - * @aligned_size: space needed - * - * Allocate the frame buffer. In the usual case we get a GTT range that - * is stolen memory backed and life is simple. If there isn't sufficient - * we fail as we don't have the virtual mapping space to really vmap it - * and the kernel console code can't handle non linear framebuffers. - * - * Re-address this as and if the framebuffer layer grows this ability. - */ -static struct gtt_range *psbfb_alloc(struct drm_device *dev, int aligned_size) -{ - struct gtt_range *backing; - /* Begin by trying to use stolen memory backing */ - backing = psb_gtt_alloc_range(dev, aligned_size, "fb", 1, PAGE_SIZE); - if (backing) { - backing->gem.funcs = &psb_gem_object_funcs; - drm_gem_private_object_init(dev, &backing->gem, aligned_size); - return backing; - } - return NULL; -} - /** * psbfb_create- create a framebuffer * @fb_helper: the framebuffer helper @@ -268,6 +243,7 @@ static int psbfb_create(struct drm_fb_helper *fb_helper, int size; int ret; struct gtt_range *backing; + struct drm_gem_object *obj; u32 bpp, depth; mode_cmd.width = sizes->surface_width; @@ -285,24 +261,25 @@ static int psbfb_create(struct drm_fb_helper *fb_helper, size = ALIGN(size, PAGE_SIZE); /* Allocate the framebuffer in the GTT with stolen page backing */ - backing = psbfb_alloc(dev, size); - if (backing == NULL) - return -ENOMEM; + backing = psb_gem_create(dev, size, "fb", true, PAGE_SIZE); + if (IS_ERR(backing)) + return PTR_ERR(backing); + obj = &backing->gem; memset(dev_priv->vram_addr + backing->offset, 0, size); info = drm_fb_helper_alloc_fbi(fb_helper); if (IS_ERR(info)) { ret = PTR_ERR(info); - goto out; + goto err_drm_gem_object_put; } mode_cmd.pixel_format = drm_mode_legacy_fb_format(bpp, depth); - fb = psb_framebuffer_create(dev, &mode_cmd, &backing->gem); + fb = psb_framebuffer_create(dev, &mode_cmd, obj); if (IS_ERR(fb)) { ret = PTR_ERR(fb); - goto out; + goto err_drm_gem_object_put; } fb_helper->fb = fb; @@ -333,8 +310,9 @@ static int psbfb_create(struct drm_fb_helper *fb_helper, dev_dbg(dev->dev, "allocated %dx%d fb\n", fb->width, fb->height); return 0; -out: - psb_gtt_free_range(dev, backing); + +err_drm_gem_object_put: + drm_gem_object_put(obj); return ret; } diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index 8f4bcf9cf912..4acab39a583a 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -90,7 +90,7 @@ void psb_gtt_unpin(struct gtt_range *gt) mutex_unlock(&dev_priv->gtt_mutex); } -void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt) +static void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt) { /* Undo the mmap pin if we are destroying the object */ if (gt->mmapping) { @@ -122,13 +122,13 @@ static const struct vm_operations_struct psb_gem_vm_ops = { .close = drm_gem_vm_close, }; -const struct drm_gem_object_func
[PATCH 01/10] drm/gma500: Move helpers for struct gtt_range from gtt.c to gem.c
Allocation and pinning helpers for struct gtt_range are GEM functions, so move them to gem.c. No functional changes. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/framebuffer.c | 1 - drivers/gpu/drm/gma500/gem.c | 133 +-- drivers/gpu/drm/gma500/gem.h | 8 + drivers/gpu/drm/gma500/gma_display.c | 1 + drivers/gpu/drm/gma500/gtt.c | 190 + drivers/gpu/drm/gma500/gtt.h | 11 +- drivers/gpu/drm/gma500/psb_intel_display.c | 1 + 7 files changed, 136 insertions(+), 209 deletions(-) diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index 321e416489a9..ce92d11bd20f 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -25,7 +25,6 @@ #include "framebuffer.h" #include "gem.h" -#include "gtt.h" #include "psb_drv.h" #include "psb_intel_drv.h" #include "psb_intel_reg.h" diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index 5ae54c9d2819..734bcb7a80c8 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -19,6 +19,89 @@ #include "gem.h" #include "psb_drv.h" +static int psb_gtt_attach_pages(struct gtt_range *gt) +{ + struct page **pages; + + WARN_ON(gt->pages); + + pages = drm_gem_get_pages(>->gem); + if (IS_ERR(pages)) + return PTR_ERR(pages); + + gt->npage = gt->gem.size / PAGE_SIZE; + gt->pages = pages; + + return 0; +} + +static void psb_gtt_detach_pages(struct gtt_range *gt) +{ + drm_gem_put_pages(>->gem, gt->pages, true, false); + gt->pages = NULL; +} + +int psb_gtt_pin(struct gtt_range *gt) +{ + int ret = 0; + struct drm_device *dev = gt->gem.dev; + struct drm_psb_private *dev_priv = to_drm_psb_private(dev); + u32 gpu_base = dev_priv->gtt.gatt_start; + + mutex_lock(&dev_priv->gtt_mutex); + + if (gt->in_gart == 0 && gt->stolen == 0) { + ret = psb_gtt_attach_pages(gt); + if (ret < 0) + goto out; + ret = psb_gtt_insert(dev, gt, 0); + if (ret < 0) { + psb_gtt_detach_pages(gt); + goto out; + } + psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), +gt->pages, (gpu_base + gt->offset), +gt->npage, 0, 0, PSB_MMU_CACHED_MEMORY); + } + gt->in_gart++; +out: + mutex_unlock(&dev_priv->gtt_mutex); + return ret; +} + +void psb_gtt_unpin(struct gtt_range *gt) +{ + struct drm_device *dev = gt->gem.dev; + struct drm_psb_private *dev_priv = to_drm_psb_private(dev); + u32 gpu_base = dev_priv->gtt.gatt_start; + + mutex_lock(&dev_priv->gtt_mutex); + + WARN_ON(!gt->in_gart); + + gt->in_gart--; + if (gt->in_gart == 0 && gt->stolen == 0) { + psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu), +(gpu_base + gt->offset), gt->npage, 0, 0); + psb_gtt_remove(dev, gt); + psb_gtt_detach_pages(gt); + } + + mutex_unlock(&dev_priv->gtt_mutex); +} + +void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt) +{ + /* Undo the mmap pin if we are destroying the object */ + if (gt->mmapping) { + psb_gtt_unpin(gt); + gt->mmapping = 0; + } + WARN_ON(gt->in_gart && !gt->stolen); + release_resource(>->resource); + kfree(gt); +} + static vm_fault_t psb_gem_fault(struct vm_fault *vmf); static void psb_gem_free_object(struct drm_gem_object *obj) @@ -44,19 +127,43 @@ const struct drm_gem_object_funcs psb_gem_object_funcs = { .vm_ops = &psb_gem_vm_ops, }; -/** - * psb_gem_create - create a mappable object - * @file: the DRM file of the client - * @dev: our device - * @size: the size requested - * @handlep: returned handle (opaque number) - * @stolen: unused - * @align: unused - * - * Create a GEM object, fill in the boilerplate and attach a handle to - * it so that userspace can speak about it. This does the core work - * for the various methods that do/will create GEM objects for things - */ +struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len, + const char *name, int backed, u32 align) +{ + struct drm_psb_private *dev_priv = to_drm_psb_private(dev); + struct gtt_range *gt; + struct resource *r = dev_priv->gtt_mem; + int ret; + unsigned long start, end; + + if (backed) { + /* The start of the GTT is the stolen pages */ + start = r->start; + end = r->start + dev_priv->gtt.stolen_size - 1; + } else { + /* The rest we w
[PATCH 00/10] drm/gma500: Refactor GEM code
Bring GEM code up to current standards and untangle the connection to GTT helpers. The allocation and pinning helpers for struct gtt_range are located in the GTT code, but actually part of the GEM implementation. The patchset moves them to GEM code and refactors much of the implementation. Most of the GTT code is then independend from the struct gtt_range, while the GEM code does not contain GTT management. In addition to internal refiactoring, patches 2 to 4 update the rest of the driver to use the GEM interfaces for object allocation and release. Finally, rename struct gtt_range to struct psb_gem_object to designate it as a 'real' GEM object. Future work: with the GEM and GTT code separated, future patchsets can implement on-demand release of GTT entries, or remove the perma-mapping of stolen memory. Dma-buf support might also be added. Tested on Atom N2800 hardware. Thomas Zimmermann (10): drm/gma500: Move helpers for struct gtt_range from gtt.c to gem.c drm/gma500: Use to_gtt_range() everywhere drm/gma500: Reimplement psb_gem_create() drm/gma500: Allocate GTT ranges in stolen memory with psb_gem_create() drm/gma500: Rename psb_gtt_{pin,unpin}() to psb_gem_{pin,unpin}() drm/gma500: Inline psb_gtt_attach_pages() and psb_gtt_detach_pages() drm/gma500: Inline psb_gtt_{alloc,free}_range() into rsp callers drm/gma500: Set page-caching flags in GEM pin/unpin drm/gma500: Rewrite GTT page insert/remove without struct gtt_range drm/gma500: Rename struct gtt_range to struct psb_gem_object drivers/gpu/drm/gma500/framebuffer.c | 52 +--- drivers/gpu/drm/gma500/gem.c | 227 +++ drivers/gpu/drm/gma500/gem.h | 28 +- drivers/gpu/drm/gma500/gma_display.c | 51 ++-- drivers/gpu/drm/gma500/gtt.c | 320 - drivers/gpu/drm/gma500/gtt.h | 29 +- drivers/gpu/drm/gma500/oaktrail_crtc.c | 3 +- drivers/gpu/drm/gma500/psb_intel_display.c | 17 +- drivers/gpu/drm/gma500/psb_intel_drv.h | 2 +- 9 files changed, 310 insertions(+), 419 deletions(-) -- 2.33.0
[PATCH 09/10] drm/gma500: Rewrite GTT page insert/remove without struct gtt_range
struct gtt_range represents a GEM object and should not be used for GTT setup. Change psb_gtt_insert() and psb_gtt_remove() to receive all necessary parameters from their caller. This also eliminates possible failure from psb_gtt_insert(). There's one exception in psb_gtt_restore(), which requires an upcast from struct resource to struct gtt_range when restoring the GTT after hibernation. A possible solution would track the GEM objects that need restoration separately from the GTT resource. Rename the functions to psb_gtt_insert_pages() and psb_gtt_remove_pages() to reflect their similarity to MMU interfaces. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/gem.c | 13 ++ drivers/gpu/drm/gma500/gtt.c | 87 drivers/gpu/drm/gma500/gtt.h | 5 ++- 3 files changed, 35 insertions(+), 70 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index a88d51a3aa2a..fd556ba2c044 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -23,7 +23,6 @@ int psb_gem_pin(struct gtt_range *gt) { - int ret = 0; struct drm_device *dev = gt->gem.dev; struct drm_psb_private *dev_priv = to_drm_psb_private(dev); u32 gpu_base = dev_priv->gtt.gatt_start; @@ -43,10 +42,7 @@ int psb_gem_pin(struct gtt_range *gt) set_pages_array_wc(pages, npages); - ret = psb_gtt_insert(dev, gt); - if (ret) - goto err_drm_gem_put_pages; - + psb_gtt_insert_pages(dev_priv, >->resource, pages); psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), pages, (gpu_base + gt->offset), npages, 0, 0, PSB_MMU_CACHED_MEMORY); @@ -59,10 +55,6 @@ int psb_gem_pin(struct gtt_range *gt) mutex_unlock(&dev_priv->gtt_mutex); return 0; - -err_drm_gem_put_pages: - drm_gem_put_pages(>->gem, pages, true, false); - return ret; } void psb_gem_unpin(struct gtt_range *gt) @@ -82,13 +74,14 @@ void psb_gem_unpin(struct gtt_range *gt) psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu), (gpu_base + gt->offset), gt->npage, 0, 0); - psb_gtt_remove(dev, gt); + psb_gtt_remove_pages(dev_priv, >->resource); /* Reset caching flags */ set_pages_array_wb(gt->pages, gt->npage); drm_gem_put_pages(>->gem, gt->pages, true, false); gt->pages = NULL; + gt->npage = 0; out: mutex_unlock(&dev_priv->gtt_mutex); diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c index 244de618e612..cf71a2396c16 100644 --- a/drivers/gpu/drm/gma500/gtt.c +++ b/drivers/gpu/drm/gma500/gtt.c @@ -66,85 +66,51 @@ static inline uint32_t psb_gtt_mask_pte(uint32_t pfn, int type) return (pfn << PAGE_SHIFT) | mask; } -/** - * psb_gtt_entry - find the GTT entries for a gtt_range - * @dev: our DRM device - * @r: our GTT range - * - * Given a gtt_range object return the GTT offset of the page table - * entries for this gtt_range - */ -static u32 __iomem *psb_gtt_entry(struct drm_device *dev, struct gtt_range *r) +static u32 __iomem *psb_gtt_entry(struct drm_psb_private *pdev, const struct resource *res) { - struct drm_psb_private *dev_priv = to_drm_psb_private(dev); - unsigned long offset; - - offset = r->resource.start - dev_priv->gtt_mem->start; + unsigned long offset = res->start - pdev->gtt_mem->start; - return dev_priv->gtt_map + (offset >> PAGE_SHIFT); + return pdev->gtt_map + (offset >> PAGE_SHIFT); } -/** - * psb_gtt_insert - put an object into the GTT - * @dev: our DRM device - * @r: our GTT range - * - * Take our preallocated GTT range and insert the GEM object into - * the GTT. This is protected via the gtt mutex which the caller - * must hold. - */ -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r) +void psb_gtt_insert_pages(struct drm_psb_private *pdev, const struct resource *res, + struct page **pages) { + resource_size_t npages, i; u32 __iomem *gtt_slot; u32 pte; - int i; - if (r->pages == NULL) { - WARN_ON(1); - return -EINVAL; - } - - WARN_ON(r->stolen); /* refcount these maybe ? */ + /* Write our page entries into the GTT itself */ - gtt_slot = psb_gtt_entry(dev, r); + npages = resource_size(res) >> PAGE_SHIFT; + gtt_slot = psb_gtt_entry(pdev, res); - /* Write our page entries into the GTT itself */ - for (i = 0; i < r->npage; i++) { - pte = psb_gtt_mask_pte(page_to_pfn(r->pages[i]), - PSB_MMU_CACHED_MEMORY); - iowrite32(pte, gtt_slot++); + for (i = 0; i < npages; ++i, ++gtt_slot) { + pte = psb_gtt_mask_pte(page_to_pfn(pages[i]
[PATCH 08/10] drm/gma500: Set page-caching flags in GEM pin/unpin
Caching of the GEM object's backing pages are unrelated to GTT management. Move the respective calls from GTT code to GEM code. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/gem.c | 9 - drivers/gpu/drm/gma500/gtt.c | 17 ++--- drivers/gpu/drm/gma500/gtt.h | 2 +- 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index 46209e10dcc2..a88d51a3aa2a 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -13,6 +13,8 @@ #include +#include + #include #include @@ -39,7 +41,9 @@ int psb_gem_pin(struct gtt_range *gt) npages = gt->gem.size / PAGE_SIZE; - ret = psb_gtt_insert(dev, gt, 0); + set_pages_array_wc(pages, npages); + + ret = psb_gtt_insert(dev, gt); if (ret) goto err_drm_gem_put_pages; @@ -80,6 +84,9 @@ void psb_gem_unpin(struct gtt_range *gt) (gpu_base + gt->offset), gt->npage, 0, 0); psb_gtt_remove(dev, gt); + /* Reset caching flags */ + set_pages_array_wb(gt->pages, gt->npage); + drm_gem_put_pages(>->gem, gt->pages, true, false); gt->pages = NULL; diff --git a/drivers/gpu/drm/gma500/gtt.c b/drivers/gpu/drm/gma500/gtt.c index 5d940fdbe6b8..244de618e612 100644 --- a/drivers/gpu/drm/gma500/gtt.c +++ b/drivers/gpu/drm/gma500/gtt.c @@ -7,10 +7,6 @@ * Alan Cox */ -#include - -#include - #include "psb_drv.h" @@ -92,17 +88,15 @@ static u32 __iomem *psb_gtt_entry(struct drm_device *dev, struct gtt_range *r) * psb_gtt_insert - put an object into the GTT * @dev: our DRM device * @r: our GTT range - * @resume: on resume * * Take our preallocated GTT range and insert the GEM object into * the GTT. This is protected via the gtt mutex which the caller * must hold. */ -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume) +int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r) { u32 __iomem *gtt_slot; u32 pte; - struct page **pages; int i; if (r->pages == NULL) { @@ -113,12 +107,6 @@ int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume) WARN_ON(r->stolen); /* refcount these maybe ? */ gtt_slot = psb_gtt_entry(dev, r); - pages = r->pages; - - if (!resume) { - /* Make sure changes are visible to the GPU */ - set_pages_array_wc(pages, r->npage); - } /* Write our page entries into the GTT itself */ for (i = 0; i < r->npage; i++) { @@ -158,7 +146,6 @@ void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r) for (i = 0; i < r->npage; i++) iowrite32(pte, gtt_slot++); ioread32(gtt_slot - 1); - set_pages_array_wb(r->pages, r->npage); } static void psb_gtt_alloc(struct drm_device *dev) @@ -349,7 +336,7 @@ int psb_gtt_restore(struct drm_device *dev) while (r != NULL) { range = container_of(r, struct gtt_range, resource); if (range->pages) { - psb_gtt_insert(dev, range, 1); + psb_gtt_insert(dev, range); size += range->resource.end - range->resource.start; restored++; } diff --git a/drivers/gpu/drm/gma500/gtt.h b/drivers/gpu/drm/gma500/gtt.h index 459a03141e8b..7af71617e0c5 100644 --- a/drivers/gpu/drm/gma500/gtt.h +++ b/drivers/gpu/drm/gma500/gtt.h @@ -49,7 +49,7 @@ int psb_gtt_allocate_resource(struct drm_psb_private *pdev, struct resource *res const char *name, resource_size_t size, resource_size_t align, bool stolen, u32 offset[static 1]); -int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r, int resume); +int psb_gtt_insert(struct drm_device *dev, struct gtt_range *r); void psb_gtt_remove(struct drm_device *dev, struct gtt_range *r); #endif -- 2.33.0
[PATCH 05/10] drm/gma500: Rename psb_gtt_{pin, unpin}() to psb_gem_{pin, unpin}()
Rename psb_gtt_pin() to psb_gem_pin() to reflect the semantics of the function. Same for psb_gtt_unpin(). No functional changes. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/gem.c | 8 drivers/gpu/drm/gma500/gem.h | 4 ++-- drivers/gpu/drm/gma500/gma_display.c | 12 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index 4acab39a583a..369910d0091e 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -41,7 +41,7 @@ static void psb_gtt_detach_pages(struct gtt_range *gt) gt->pages = NULL; } -int psb_gtt_pin(struct gtt_range *gt) +int psb_gem_pin(struct gtt_range *gt) { int ret = 0; struct drm_device *dev = gt->gem.dev; @@ -69,7 +69,7 @@ int psb_gtt_pin(struct gtt_range *gt) return ret; } -void psb_gtt_unpin(struct gtt_range *gt) +void psb_gem_unpin(struct gtt_range *gt) { struct drm_device *dev = gt->gem.dev; struct drm_psb_private *dev_priv = to_drm_psb_private(dev); @@ -94,7 +94,7 @@ static void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt) { /* Undo the mmap pin if we are destroying the object */ if (gt->mmapping) { - psb_gtt_unpin(gt); + psb_gem_unpin(gt); gt->mmapping = 0; } WARN_ON(gt->in_gart && !gt->stolen); @@ -290,7 +290,7 @@ static vm_fault_t psb_gem_fault(struct vm_fault *vmf) /* For now the mmap pins the object and it stays pinned. As things stand that will do us no harm */ if (r->mmapping == 0) { - err = psb_gtt_pin(r); + err = psb_gem_pin(r); if (err < 0) { dev_err(dev->dev, "gma500: pin failed: %d\n", err); ret = vmf_error(err); diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h index 6b67c58cbed5..21c86df482a6 100644 --- a/drivers/gpu/drm/gma500/gem.h +++ b/drivers/gpu/drm/gma500/gem.h @@ -15,7 +15,7 @@ struct drm_device; struct gtt_range * psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align); -int psb_gtt_pin(struct gtt_range *gt); -void psb_gtt_unpin(struct gtt_range *gt); +int psb_gem_pin(struct gtt_range *gt); +void psb_gem_unpin(struct gtt_range *gt); #endif diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c index 8c95b50034a5..6d0470b27bc5 100644 --- a/drivers/gpu/drm/gma500/gma_display.c +++ b/drivers/gpu/drm/gma500/gma_display.c @@ -75,7 +75,7 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y, /* We are displaying this buffer, make sure it is actually loaded into the GTT */ - ret = psb_gtt_pin(gtt); + ret = psb_gem_pin(gtt); if (ret < 0) goto gma_pipe_set_base_exit; start = gtt->offset; @@ -126,7 +126,7 @@ int gma_pipe_set_base(struct drm_crtc *crtc, int x, int y, gma_pipe_cleaner: /* If there was a previous display we can now unpin it */ if (old_fb) - psb_gtt_unpin(to_gtt_range(old_fb->obj[0])); + psb_gem_unpin(to_gtt_range(old_fb->obj[0])); gma_pipe_set_base_exit: gma_power_end(dev); @@ -350,7 +350,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, /* Unpin the old GEM object */ if (gma_crtc->cursor_obj) { gt = to_gtt_range(gma_crtc->cursor_obj); - psb_gtt_unpin(gt); + psb_gem_unpin(gt); drm_gem_object_put(gma_crtc->cursor_obj); gma_crtc->cursor_obj = NULL; } @@ -378,7 +378,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, gt = to_gtt_range(obj); /* Pin the memory into the GTT */ - ret = psb_gtt_pin(gt); + ret = psb_gem_pin(gt); if (ret) { dev_err(dev->dev, "Can not pin down handle 0x%x\n", handle); goto unref_cursor; @@ -426,7 +426,7 @@ int gma_crtc_cursor_set(struct drm_crtc *crtc, /* unpin the old bo */ if (gma_crtc->cursor_obj) { gt = to_gtt_range(gma_crtc->cursor_obj); - psb_gtt_unpin(gt); + psb_gem_unpin(gt); drm_gem_object_put(gma_crtc->cursor_obj); } @@ -490,7 +490,7 @@ void gma_crtc_disable(struct drm_crtc *crtc) if (crtc->primary->fb) { gt = to_gtt_range(crtc->primary->fb->obj[0]); - psb_gtt_unpin(gt); + psb_gem_unpin(gt); } } -- 2.33.0
[PATCH 06/10] drm/gma500: Inline psb_gtt_attach_pages() and psb_gtt_detach_pages()
psb_gtt_attach_pages() are not GTT functions but deal with the GEM object's SHMEM pages. The only callers of psb_gtt_attach_pages() and psb_gtt_detach_pages() are the GEM pin helpers. Inline the calls and cleanup the resulting code. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/gem.c | 75 +--- 1 file changed, 36 insertions(+), 39 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index 369910d0091e..a48d7d5ed026 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -19,53 +19,45 @@ #include "gem.h" #include "psb_drv.h" -static int psb_gtt_attach_pages(struct gtt_range *gt) +int psb_gem_pin(struct gtt_range *gt) { + int ret = 0; + struct drm_device *dev = gt->gem.dev; + struct drm_psb_private *dev_priv = to_drm_psb_private(dev); + u32 gpu_base = dev_priv->gtt.gatt_start; struct page **pages; + unsigned int npages; - WARN_ON(gt->pages); + mutex_lock(&dev_priv->gtt_mutex); + + if (gt->in_gart || gt->stolen) + goto out; /* already mapped */ pages = drm_gem_get_pages(>->gem); if (IS_ERR(pages)) return PTR_ERR(pages); - gt->npage = gt->gem.size / PAGE_SIZE; - gt->pages = pages; - - return 0; -} + npages = gt->gem.size / PAGE_SIZE; -static void psb_gtt_detach_pages(struct gtt_range *gt) -{ - drm_gem_put_pages(>->gem, gt->pages, true, false); - gt->pages = NULL; -} + ret = psb_gtt_insert(dev, gt, 0); + if (ret) + goto err_drm_gem_put_pages; -int psb_gem_pin(struct gtt_range *gt) -{ - int ret = 0; - struct drm_device *dev = gt->gem.dev; - struct drm_psb_private *dev_priv = to_drm_psb_private(dev); - u32 gpu_base = dev_priv->gtt.gatt_start; + psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), pages, +(gpu_base + gt->offset), npages, 0, 0, +PSB_MMU_CACHED_MEMORY); - mutex_lock(&dev_priv->gtt_mutex); + gt->npage = npages; + gt->pages = pages; - if (gt->in_gart == 0 && gt->stolen == 0) { - ret = psb_gtt_attach_pages(gt); - if (ret < 0) - goto out; - ret = psb_gtt_insert(dev, gt, 0); - if (ret < 0) { - psb_gtt_detach_pages(gt); - goto out; - } - psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), -gt->pages, (gpu_base + gt->offset), -gt->npage, 0, 0, PSB_MMU_CACHED_MEMORY); - } - gt->in_gart++; out: + ++gt->in_gart; mutex_unlock(&dev_priv->gtt_mutex); + + return 0; + +err_drm_gem_put_pages: + drm_gem_put_pages(>->gem, pages, true, false); return ret; } @@ -79,14 +71,19 @@ void psb_gem_unpin(struct gtt_range *gt) WARN_ON(!gt->in_gart); - gt->in_gart--; - if (gt->in_gart == 0 && gt->stolen == 0) { - psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu), + --gt->in_gart; + + if (gt->in_gart || gt->stolen) + goto out; + + psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu), (gpu_base + gt->offset), gt->npage, 0, 0); - psb_gtt_remove(dev, gt); - psb_gtt_detach_pages(gt); - } + psb_gtt_remove(dev, gt); + drm_gem_put_pages(>->gem, gt->pages, true, false); + gt->pages = NULL; + +out: mutex_unlock(&dev_priv->gtt_mutex); } -- 2.33.0
[PATCH 03/10] drm/gma500: Reimplement psb_gem_create()
Implement psb_gem_create() for general use. Create the GEM handle in psb_gem_create_dumb(). Allows to use psb_gem_create() for creating all of the GEM objects. While at it, clean-up drm_gem_dumb_create() to make it more readable. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/gem.c | 93 ++-- drivers/gpu/drm/gma500/gem.h | 4 +- 2 files changed, 59 insertions(+), 38 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index ff2c1d64689e..8f4bcf9cf912 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -164,45 +164,36 @@ struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len, return NULL; } -int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size, - u32 *handlep, int stolen, u32 align) +struct gtt_range * +psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align) { - struct gtt_range *r; + struct gtt_range *gt; + struct drm_gem_object *obj; int ret; - u32 handle; size = roundup(size, PAGE_SIZE); - /* Allocate our object - for now a direct gtt range which is not - stolen memory backed */ - r = psb_gtt_alloc_range(dev, size, "gem", 0, PAGE_SIZE); - if (r == NULL) { + gt = psb_gtt_alloc_range(dev, size, name, stolen, align); + if (!gt) { dev_err(dev->dev, "no memory for %lld byte GEM object\n", size); - return -ENOSPC; + return ERR_PTR(-ENOSPC); } - r->gem.funcs = &psb_gem_object_funcs; - /* Initialize the extra goodies GEM needs to do all the hard work */ - if (drm_gem_object_init(dev, &r->gem, size) != 0) { - psb_gtt_free_range(dev, r); - /* GEM doesn't give an error code so use -ENOMEM */ - dev_err(dev->dev, "GEM init failed for %lld\n", size); - return -ENOMEM; - } - /* Limit the object to 32bit mappings */ - mapping_set_gfp_mask(r->gem.filp->f_mapping, GFP_KERNEL | __GFP_DMA32); - /* Give the object a handle so we can carry it more easily */ - ret = drm_gem_handle_create(file, &r->gem, &handle); - if (ret) { - dev_err(dev->dev, "GEM handle failed for %p, %lld\n", - &r->gem, size); - drm_gem_object_release(&r->gem); - psb_gtt_free_range(dev, r); - return ret; - } - /* We have the initial and handle reference but need only one now */ - drm_gem_object_put(&r->gem); - *handlep = handle; - return 0; + obj = >->gem; + + obj->funcs = &psb_gem_object_funcs; + + ret = drm_gem_object_init(dev, obj, size); + if (ret) + goto err_psb_gtt_free_range; + + /* Limit the object to 32-bit mappings */ + mapping_set_gfp_mask(obj->filp->f_mapping, GFP_KERNEL | __GFP_DMA32); + + return gt; + +err_psb_gtt_free_range: + psb_gtt_free_range(dev, gt); + return ERR_PTR(ret); } /** @@ -218,10 +209,40 @@ int psb_gem_create(struct drm_file *file, struct drm_device *dev, u64 size, int psb_gem_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args) { - args->pitch = ALIGN(args->width * ((args->bpp + 7) / 8), 64); - args->size = args->pitch * args->height; - return psb_gem_create(file, dev, args->size, &args->handle, 0, - PAGE_SIZE); + size_t pitch, size; + struct gtt_range *gt; + struct drm_gem_object *obj; + u32 handle; + int ret; + + pitch = args->width * DIV_ROUND_UP(args->bpp, 8); + pitch = ALIGN(pitch, 64); + + size = pitch * args->height; + size = roundup(size, PAGE_SIZE); + if (!size) + return -EINVAL; + + gt = psb_gem_create(dev, size, "gem", false, PAGE_SIZE); + if (IS_ERR(gt)) + return PTR_ERR(gt); + obj = >->gem; + + ret = drm_gem_handle_create(file, obj, &handle); + if (ret) + goto err_drm_gem_object_put; + + drm_gem_object_put(obj); + + args->pitch = pitch; + args->size = size; + args->handle = handle; + + return 0; + +err_drm_gem_object_put: + drm_gem_object_put(obj); + return ret; } /** diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h index 275494aedd4c..ad76127dc719 100644 --- a/drivers/gpu/drm/gma500/gem.h +++ b/drivers/gpu/drm/gma500/gem.h @@ -14,8 +14,8 @@ struct drm_device; extern const struct drm_gem_object_funcs psb_gem_object_funcs; -extern int psb_gem_create(struct drm_file *file, struct drm_device *dev, - u64 size, u32 *handlep, int stolen, u32 align); +struct gtt_range * +psb_gem_create(struct drm_device *dev, u64 size, con
[PATCH 07/10] drm/gma500: Inline psb_gtt_{alloc, free}_range() into rsp callers
psb_gtt_alloc_range() allocates struct gtt_range, create the GTT resource and performs some half-baked initialization. Inline the function into its only caller psb_gem_create(). For creating the GTT resource, introduce a new helper, psb_gtt_alloc_resource() that hides the details of the GTT. For psb_gtt_free_range(), inline the function into its only caller psb_gem_free_object(). While at it, remove the explicit invocation of drm_gem_free_mmap_offset(). The mmap offset is already released by drm_gem_object_release(). Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/gem.c | 94 drivers/gpu/drm/gma500/gtt.c | 27 +++ drivers/gpu/drm/gma500/gtt.h | 6 +++ 3 files changed, 65 insertions(+), 62 deletions(-) diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index a48d7d5ed026..46209e10dcc2 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -87,30 +87,22 @@ void psb_gem_unpin(struct gtt_range *gt) mutex_unlock(&dev_priv->gtt_mutex); } -static void psb_gtt_free_range(struct drm_device *dev, struct gtt_range *gt) -{ - /* Undo the mmap pin if we are destroying the object */ - if (gt->mmapping) { - psb_gem_unpin(gt); - gt->mmapping = 0; - } - WARN_ON(gt->in_gart && !gt->stolen); - release_resource(>->resource); - kfree(gt); -} - static vm_fault_t psb_gem_fault(struct vm_fault *vmf); static void psb_gem_free_object(struct drm_gem_object *obj) { - struct gtt_range *gtt = to_gtt_range(obj); + struct gtt_range *gt = to_gtt_range(obj); - /* Remove the list map if one is present */ - drm_gem_free_mmap_offset(obj); drm_gem_object_release(obj); - /* This must occur last as it frees up the memory of the GEM object */ - psb_gtt_free_range(obj->dev, gtt); + /* Undo the mmap pin if we are destroying the object */ + if (gt->mmapping) + psb_gem_unpin(gt); + + WARN_ON(gt->in_gart && !gt->stolen); + + release_resource(>->resource); + kfree(gt); } static const struct vm_operations_struct psb_gem_vm_ops = { @@ -124,59 +116,35 @@ static const struct drm_gem_object_funcs psb_gem_object_funcs = { .vm_ops = &psb_gem_vm_ops, }; -static struct gtt_range *psb_gtt_alloc_range(struct drm_device *dev, int len, -const char *name, int backed, u32 align) -{ - struct drm_psb_private *dev_priv = to_drm_psb_private(dev); - struct gtt_range *gt; - struct resource *r = dev_priv->gtt_mem; - int ret; - unsigned long start, end; - - if (backed) { - /* The start of the GTT is the stolen pages */ - start = r->start; - end = r->start + dev_priv->gtt.stolen_size - 1; - } else { - /* The rest we will use for GEM backed objects */ - start = r->start + dev_priv->gtt.stolen_size; - end = r->end; - } - - gt = kzalloc(sizeof(struct gtt_range), GFP_KERNEL); - if (gt == NULL) - return NULL; - gt->resource.name = name; - gt->stolen = backed; - gt->in_gart = backed; - /* Ensure this is set for non GEM objects */ - gt->gem.dev = dev; - ret = allocate_resource(dev_priv->gtt_mem, >->resource, - len, start, end, align, NULL, NULL); - if (ret == 0) { - gt->offset = gt->resource.start - r->start; - return gt; - } - kfree(gt); - return NULL; -} - struct gtt_range * psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, u32 align) { + struct drm_psb_private *dev_priv = to_drm_psb_private(dev); struct gtt_range *gt; struct drm_gem_object *obj; int ret; size = roundup(size, PAGE_SIZE); - gt = psb_gtt_alloc_range(dev, size, name, stolen, align); - if (!gt) { - dev_err(dev->dev, "no memory for %lld byte GEM object\n", size); - return ERR_PTR(-ENOSPC); - } + gt = kzalloc(sizeof(*gt), GFP_KERNEL); + if (!gt) + return ERR_PTR(-ENOMEM); obj = >->gem; + /* GTT resource */ + + ret = psb_gtt_allocate_resource(dev_priv, >->resource, name, size, align, stolen, + >->offset); + if (ret) + goto err_kfree; + + if (stolen) { + gt->stolen = true; + gt->in_gart = 1; + } + + /* GEM object */ + obj->funcs = &psb_gem_object_funcs; if (stolen) { @@ -184,7 +152,7 @@ psb_gem_create(struct drm_device *dev, u64 size, const char *name, bool stolen, } else { ret = drm_gem_object_init(dev, obj, size); if (ret) - goto err_psb_gtt_free_range; +
[PATCH 10/10] drm/gma500: Rename struct gtt_range to struct psb_gem_object
struct gtt_range represents a GEM object. Rename the structure to struct psb_gem_object and update all users. No functional changes. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/gma500/framebuffer.c | 9 +- drivers/gpu/drm/gma500/gem.c | 106 +++-- drivers/gpu/drm/gma500/gem.h | 25 - drivers/gpu/drm/gma500/gma_display.c | 50 +- drivers/gpu/drm/gma500/gtt.c | 15 +-- drivers/gpu/drm/gma500/gtt.h | 15 --- drivers/gpu/drm/gma500/oaktrail_crtc.c | 3 +- drivers/gpu/drm/gma500/psb_intel_display.c | 15 ++- drivers/gpu/drm/gma500/psb_intel_drv.h | 2 +- 9 files changed, 123 insertions(+), 117 deletions(-) diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c index 3ea6679ccd38..45df9de22007 100644 --- a/drivers/gpu/drm/gma500/framebuffer.c +++ b/drivers/gpu/drm/gma500/framebuffer.c @@ -81,14 +81,13 @@ static vm_fault_t psbfb_vm_fault(struct vm_fault *vmf) struct drm_framebuffer *fb = vma->vm_private_data; struct drm_device *dev = fb->dev; struct drm_psb_private *dev_priv = to_drm_psb_private(dev); - struct gtt_range *gtt = to_gtt_range(fb->obj[0]); + struct psb_gem_object *pobj = to_psb_gem_object(fb->obj[0]); int page_num; int i; unsigned long address; vm_fault_t ret = VM_FAULT_SIGBUS; unsigned long pfn; - unsigned long phys_addr = (unsigned long)dev_priv->stolen_base + - gtt->offset; + unsigned long phys_addr = (unsigned long)dev_priv->stolen_base + pobj->offset; page_num = vma_pages(vma); address = vmf->address - (vmf->pgoff << PAGE_SHIFT); @@ -242,7 +241,7 @@ static int psbfb_create(struct drm_fb_helper *fb_helper, struct drm_mode_fb_cmd2 mode_cmd; int size; int ret; - struct gtt_range *backing; + struct psb_gem_object *backing; struct drm_gem_object *obj; u32 bpp, depth; @@ -264,7 +263,7 @@ static int psbfb_create(struct drm_fb_helper *fb_helper, backing = psb_gem_create(dev, size, "fb", true, PAGE_SIZE); if (IS_ERR(backing)) return PTR_ERR(backing); - obj = &backing->gem; + obj = &backing->base; memset(dev_priv->vram_addr + backing->offset, 0, size); diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c index fd556ba2c044..9b7052153cab 100644 --- a/drivers/gpu/drm/gma500/gem.c +++ b/drivers/gpu/drm/gma500/gem.c @@ -21,9 +21,10 @@ #include "gem.h" #include "psb_drv.h" -int psb_gem_pin(struct gtt_range *gt) +int psb_gem_pin(struct psb_gem_object *pobj) { - struct drm_device *dev = gt->gem.dev; + struct drm_gem_object *obj = &pobj->base; + struct drm_device *dev = obj->dev; struct drm_psb_private *dev_priv = to_drm_psb_private(dev); u32 gpu_base = dev_priv->gtt.gatt_start; struct page **pages; @@ -31,57 +32,58 @@ int psb_gem_pin(struct gtt_range *gt) mutex_lock(&dev_priv->gtt_mutex); - if (gt->in_gart || gt->stolen) + if (pobj->in_gart || pobj->stolen) goto out; /* already mapped */ - pages = drm_gem_get_pages(>->gem); + pages = drm_gem_get_pages(obj); if (IS_ERR(pages)) return PTR_ERR(pages); - npages = gt->gem.size / PAGE_SIZE; + npages = obj->size / PAGE_SIZE; set_pages_array_wc(pages, npages); - psb_gtt_insert_pages(dev_priv, >->resource, pages); + psb_gtt_insert_pages(dev_priv, &pobj->resource, pages); psb_mmu_insert_pages(psb_mmu_get_default_pd(dev_priv->mmu), pages, -(gpu_base + gt->offset), npages, 0, 0, +(gpu_base + pobj->offset), npages, 0, 0, PSB_MMU_CACHED_MEMORY); - gt->npage = npages; - gt->pages = pages; + pobj->npage = npages; + pobj->pages = pages; out: - ++gt->in_gart; + ++pobj->in_gart; mutex_unlock(&dev_priv->gtt_mutex); return 0; } -void psb_gem_unpin(struct gtt_range *gt) +void psb_gem_unpin(struct psb_gem_object *pobj) { - struct drm_device *dev = gt->gem.dev; + struct drm_gem_object *obj = &pobj->base; + struct drm_device *dev = obj->dev; struct drm_psb_private *dev_priv = to_drm_psb_private(dev); u32 gpu_base = dev_priv->gtt.gatt_start; mutex_lock(&dev_priv->gtt_mutex); - WARN_ON(!gt->in_gart); + WARN_ON(!pobj->in_gart); - --gt->in_gart; + --pobj->in_gart; - if (gt->in_gart || gt->stolen) + if (pobj->in_gart || pobj->stolen) goto out; psb_mmu_remove_pages(psb_mmu_get_default_pd(dev_priv->mmu), -(gpu_base + gt->offset), gt->npage, 0, 0); - psb_gtt_remove_pages(dev_priv, >->resource); +
Re: [PATCH] drm: rcar-du: Don't create encoder for unconnected LVDS outputs
Hi Laurent, On Mon, Aug 23, 2021 at 4:54 PM Laurent Pinchart wrote: > On Mon, Aug 23, 2021 at 02:25:32PM +0200, Geert Uytterhoeven wrote: > > On Sun, Aug 22, 2021 at 2:36 AM Laurent Pinchart wrote: > > > On R-Car D3 and E3, the LVDS encoders provide the pixel clock to the DU, > > > even when LVDS outputs are not used. For this reason, the rcar-lvds > > > driver probes successfully on those platforms even if no further bridge > > > or panel is connected to the LVDS output, in order to provide the > > > rcar_lvds_clk_enable() and rcar_lvds_clk_disable() functions to the DU > > > driver. > > > > > > If an LVDS output isn't connected, trying to create a DRM connector for > > > the output will fail. Fix this by skipping connector creation in that > > > case, and also skip creation of the DRM encoder as there's no point in > > > an encoder without a connector. > > > > > > Fixes: e9e056949c92 ("drm: rcar-du: lvds: Convert to DRM panel bridge > > > helper") > > > Reported-by: Geert Uytterhoeven > > > > Can you please change that to > > Reported-by: Geert Uytterhoeven > > ? > > Sure thing. Thanks! > > > Signed-off-by: Laurent Pinchart > > > > > > > Thanks, the scary warning on Ebisu-4D is gone, so > > Tested-by: Geert Uytterhoeven > > > > Disclaimer: there are no displays connected to my Ebisu-4D. > > That's the best way to ensure the absence of display issues. It works > great for camera testing too, if you also remove networking and storage > :-) Any chance this fix can make it upstream? The fix was created before the issue entered upstream in v5.15-rc1. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic, jz4780-dw-hdmi DT Schema
Hi, > Am 27.09.2021 um 19:07 schrieb max...@cerno.tech: > > Hi, > > On Mon, Sep 27, 2021 at 06:44:21PM +0200, H. Nikolaus Schaller wrote: >> From: Sam Ravnborg >> >> Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC. >> Based on .txt binding from Zubair Lutfullah Kakakhel >> >> Signed-off-by: Sam Ravnborg >> Signed-off-by: H. Nikolaus Schaller >> Cc: Rob Herring >> Cc: devicet...@vger.kernel.org >> --- >> .../bindings/display/ingenic-jz4780-hdmi.yaml | 85 +++ >> 1 file changed, 85 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml >> >> diff --git >> a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml >> b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml >> new file mode 100644 >> index ..5e60cdac4f63 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml >> @@ -0,0 +1,85 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Bindings for Ingenic JZ4780 HDMI Transmitter >> + >> +maintainers: >> + - H. Nikolaus Schaller >> + >> +description: | >> + The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI >> 1.4 >> + TX controller IP with accompanying PHY IP. >> + >> +allOf: >> + - $ref: panel/panel-common.yaml# > > Is it a panel though? Good question. Appears to have to be changed to - $ref: bridge/synopsys,dw-hdmi.yaml# > >> +properties: >> + compatible: >> +items: >> + - const: ingenic,jz4780-dw-hdmi > > This can just be a const, there's no need for the items Maybe starting with an enum is better if more compatible strings are to be added. > >> + >> + reg: >> +maxItems: 1 >> +description: the address & size of the LCD controller registers > > There's no need for that description, it's obvious enough Indeed. > >> + reg-io-width: >> +const: 4 > > If it's fixed, why do you need it in the first place? There is a fixed default of 1 if not specified. > >> + interrupts: >> +maxItems: 1 >> +description: Specifies the interrupt provided by parent > > There's no need for that description, it's obvious enough Indeed. > >> + clocks: >> +maxItems: 2 >> +description: Clock specifiers for isrf and iahb clocks > > This can be defined as > > clocks: > items: >- description: isrf >- description: iahb > > A better description about what these clocks are would be nice as well Generally I see that this all is nowadays not independent of Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml where there is already a description. On the other hand every SoC specialization runs its own copy. e.g. Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam > >> + clock-names: >> +items: >> + - const: isfr > > Is it isfr or isrf? isfr. Seems to be a typo in the description. See bridge/synopsys,dw-hdmi.yaml# One question to the yaml specialists: since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we have to repeat? Or can we reduce to just the changes? [I am still not familiar enough with the yaml stuff to understand if it has sort of inheritance like device tree include files, so that you just have to change relevant properties] > >> + - const: iahb would it make sense to add additionalItems: false here? In the jz4780 case there are just two clocks while other specializations use more and synopsys,dw-hdmi.yaml# defines additionalItems: true. >> + >> + hdmi-regulator: true >> +description: Optional regulator to provide +5V at the connector > > regulators need to be suffixed by -supply My omission... And, it should be "hdmi-5v-supply" to match driver and device tree. > > You also can just provide the description, you don't need the true there > >> + ddc-i2c-bus: true > > ditto Ok > >> +description: An I2C interface if the internal DDC I2C driver is not to >> be used >> + ports: true > > If there's a single port, you don't need ports There can be two ports - one for input from LCDC and one for output (HDMI connector). But explicitly defining an output port is optional to some extent (depending on driver structure). > > You should also include /schemas/graph.yaml#/$defs/port-base Ok. BR and thanks, Nikolaus
[PATCH] drm: mxsfb: Set proper default bus format when using a bridge
If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in that case. This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") Signed-off-by: Guido Günther --- I'll look at what needs to be done in nwl separately but this also unbreaks other bridge seupts that don't to format negotiation yet. drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index af6c620adf6e..4ef94cf686b0 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, drm_atomic_get_new_bridge_state(state, mxsfb->bridge); bus_format = bridge_state->input_bus_cfg.format; + if (bus_format == MEDIA_BUS_FMT_FIXED) + bus_format = MEDIA_BUS_FMT_RGB888_1X24; } /* If there is no bridge, use bus format from connector */ -- 2.33.0
Re: [PATCH 04/11] ath11: Wstringop-overread warning
Arnd Bergmann wrote: > gcc-11 with the kernel address sanitizer prints a warning for this > driver: > > In function 'ath11k_peer_assoc_h_vht', > inlined from 'ath11k_peer_assoc_prepare' at > drivers/net/wireless/ath/ath11k/mac.c:1632:2: > drivers/net/wireless/ath/ath11k/mac.c:1164:13: error: > 'ath11k_peer_assoc_h_vht_masked' reading 16 bytes from a region of size 4 > [-Werror=stringop-overread] > 1164 | if (ath11k_peer_assoc_h_vht_masked(vht_mcs_mask)) > | ^~~~ > drivers/net/wireless/ath/ath11k/mac.c: In function > 'ath11k_peer_assoc_prepare': > drivers/net/wireless/ath/ath11k/mac.c:1164:13: note: referencing argument 1 > of type 'const u16 *' {aka 'const short unsigned int *'} > drivers/net/wireless/ath/ath11k/mac.c:969:1: note: in a call to function > 'ath11k_peer_assoc_h_vht_masked' > 969 | ath11k_peer_assoc_h_vht_masked(const u16 > vht_mcs_mask[NL80211_VHT_NSS_MAX]) > | ^~ > > According to analysis from gcc developers, this is a glitch in the > way gcc tracks the size of struct members. This should really get > fixed in gcc, but it's also easy to work around this instance > by changing the function prototype to no include the length of > the array. > > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99673 > Signed-off-by: Arnd Bergmann > Signed-off-by: Kalle Valo Patch applied to ath-next branch of ath.git, thanks. eb19efed836a ath11k: Wstringop-overread warning -- https://patchwork.kernel.org/project/linux-wireless/patch/20210322160253.4032422-5-a...@kernel.org/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Re: [PATCH] drm: mxsfb: Set proper default bus format when using a bridge
On 9/28/21 10:55 AM, Guido Günther wrote: If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in that case. This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") Signed-off-by: Guido Günther --- I'll look at what needs to be done in nwl separately but this also unbreaks other bridge seupts that don't to format negotiation yet. drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index af6c620adf6e..4ef94cf686b0 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, drm_atomic_get_new_bridge_state(state, mxsfb->bridge); bus_format = bridge_state->input_bus_cfg.format; + if (bus_format == MEDIA_BUS_FMT_FIXED) + bus_format = MEDIA_BUS_FMT_RGB888_1X24; Shouldn't the NWL bridge return the correct format ?
Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic,jz4780-dw-hdmi DT Schema
On Tue, Sep 28, 2021 at 10:59:45AM +0200, H. Nikolaus Schaller wrote: > >> +properties: > >> + compatible: > >> +items: > >> + - const: ingenic,jz4780-dw-hdmi > > > > This can just be a const, there's no need for the items > > Maybe starting with an enum is better if more compatible strings are to be > added. it's still fairly easy to change if needed, there's no need to confuse anyone. > > > >> + reg-io-width: > >> +const: 4 > > > > If it's fixed, why do you need it in the first place? > > There is a fixed default of 1 if not specified. My point was more about why do you need to have that property at all? Can't you just drop it and assume that the register width is 32 bits if it's all you will ever run on? > >> + clocks: > >> +maxItems: 2 > >> +description: Clock specifiers for isrf and iahb clocks > > > > This can be defined as > > > > clocks: > > items: > >- description: isrf > >- description: iahb > > > > A better description about what these clocks are would be nice as well > > Generally I see that this all is nowadays not independent of > > Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml > > where there is already a description. Ok, good then > On the other hand every SoC specialization runs its own copy. e.g. > > Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml > Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam > > > > >> + clock-names: > >> +items: > >> + - const: isfr > > > > Is it isfr or isrf? > > isfr. Seems to be a typo in the description. See > bridge/synopsys,dw-hdmi.yaml# > > One question to the yaml specialists: > > since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we > have to repeat? Or can we reduce to just the changes? If you add the ref you mentionned above, you don't have to repeat yourself indeed. You can just put clock-names: true > [I am still not familiar enough with the yaml stuff to understand if > it has sort of inheritance like device tree include files, so that you > just have to change relevant properties] Kind of, but not entirely. schemas are all applied separately, unlike DT includes that will just expand to one big DT. In practice, it means that your device must validate against all the schemas, not just the sum of them. For example, if you have a generic schema that has: properties: compatible: const: vendor,my-generic-compatible and your schema that extends the generic binding, with a ref to the generic one that has: properties: compatible: items: - const: other-vendor,my-device-compatible - const: vendor,my-generic-compatible It will still fail since the generic schema expects only a single compatible, whereas your device would have two. > > > >> + - const: iahb > > would it make sense to add additionalItems: false here? > > In the jz4780 case there are just two clocks while other specializations > use more and synopsys,dw-hdmi.yaml# defines additionalItems: true. If you want to refine the generic one, and it's all the clocks you ever expect then there's no need for additionalItems > > > >> +description: An I2C interface if the internal DDC I2C driver is not > >> to be used > >> + ports: true > > > > If there's a single port, you don't need ports > > There can be two ports - one for input from LCDC and one > for output (HDMI connector). But explicitly defining an output > port is optional to some extent (depending on driver structure). This needs to be defined then (and port@0 made mandatory) Maxime
Re: [PATCH] drm: mxsfb: Set proper default bus format when using a bridge
Hi, On Tue, Sep 28, 2021 at 11:08:58AM +0200, Marek Vasut wrote: > On 9/28/21 10:55 AM, Guido Günther wrote: > > If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is > > returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in > > that case. > > > > This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. > > > > Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if > > present") > > > > Signed-off-by: Guido Günther > > --- > > > > I'll look at what needs to be done in nwl separately but this also > > unbreaks other bridge seupts that don't to format negotiation yet. > > > > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > index af6c620adf6e..4ef94cf686b0 100644 > > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc > > *crtc, > > drm_atomic_get_new_bridge_state(state, > > mxsfb->bridge); > > bus_format = bridge_state->input_bus_cfg.format; > > + if (bus_format == MEDIA_BUS_FMT_FIXED) > > + bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > Shouldn't the NWL bridge return the correct format ? Yes it should and I'll send a separate patch for that but we currently don't do anything meaningful at all if the bridge doesn't do format negotiation and then fail setup in mxsfb_set_formats(). I think we should at least preserve the status quo (as we do with the non bridge case in b776b0f00f24 too). We could have a warning to spot drivers that don't do that yet and hence the generic code returns MEDIA_BUS_FMT_FIXED. Cheers, -- Guido
Re: [PATCH] drm: mxsfb: Set proper default bus format when using a bridge
On 9/28/21 11:19 AM, Guido Günther wrote: Hi, On Tue, Sep 28, 2021 at 11:08:58AM +0200, Marek Vasut wrote: On 9/28/21 10:55 AM, Guido Günther wrote: If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in that case. This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") Signed-off-by: Guido Günther --- I'll look at what needs to be done in nwl separately but this also unbreaks other bridge seupts that don't to format negotiation yet. drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index af6c620adf6e..4ef94cf686b0 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, drm_atomic_get_new_bridge_state(state, mxsfb->bridge); bus_format = bridge_state->input_bus_cfg.format; + if (bus_format == MEDIA_BUS_FMT_FIXED) + bus_format = MEDIA_BUS_FMT_RGB888_1X24; Shouldn't the NWL bridge return the correct format ? Yes it should and I'll send a separate patch for that but we currently don't do anything meaningful at all if the bridge doesn't do format negotiation and then fail setup in mxsfb_set_formats(). I think we should at least preserve the status quo (as we do with the non bridge case in b776b0f00f24 too). We could have a warning to spot drivers that don't do that yet and hence the generic code returns MEDIA_BUS_FMT_FIXED. I am not gonna push back against this patch, I think you need feedback from the drm people on this. A warning would indeed be nice. Preserving the old behavior in stable releases would be good (if there are any releases which contain the mxsfb patch this fixes).
Re: [PATCH] drm: mxsfb: Set proper default bus format when using a bridge
Am Dienstag, dem 28.09.2021 um 11:19 +0200 schrieb Guido Günther: > Hi, > On Tue, Sep 28, 2021 at 11:08:58AM +0200, Marek Vasut wrote: > > On 9/28/21 10:55 AM, Guido Günther wrote: > > > If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is > > > returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in > > > that case. > > > > > > This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. > > > > > > Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge > > > if present") > > > > > > Signed-off-by: Guido Günther > > > --- > > > > > > I'll look at what needs to be done in nwl separately but this also > > > unbreaks other bridge seupts that don't to format negotiation yet. > > > > > > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > index af6c620adf6e..4ef94cf686b0 100644 > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc > > > *crtc, > > > drm_atomic_get_new_bridge_state(state, > > > mxsfb->bridge); > > > bus_format = bridge_state->input_bus_cfg.format; > > > + if (bus_format == MEDIA_BUS_FMT_FIXED) > > > + bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > > > Shouldn't the NWL bridge return the correct format ? > > Yes it should and I'll send a separate patch for that but we currently > don't do anything meaningful at all if the bridge doesn't do format > negotiation and then fail setup in mxsfb_set_formats(). > > I think we should at least preserve the status quo (as we do with the > non bridge case in b776b0f00f24 too). > > We could have a warning to spot drivers that don't do that yet and hence > the generic code returns MEDIA_BUS_FMT_FIXED. > That sounds sensible. Using a default format if we don't know what to do is going to be a unpleasant surprise for those with a display pipeline that doesn't work with the default format. So please add a dev_warn when we are doing this fallback. Also I would argue that the NWL fix is the patch that should go in the stable tree. This one should only be a additional safety net, so I would drop the Fixes tag. Regards, Lucas
Re: [PATCH v6 6/6] drm/sprd: add Unisoc's drm mipi dsi&dphy driver
On Sun, Sep 26, 2021 at 10:31:53PM +0800, Kevin Tang wrote: > Maxime Ripard 于2021年9月17日周五 下午11:40写道: > > > +static void sprd_dsi_encoder_mode_set(struct drm_encoder *encoder, > > > + struct drm_display_mode *mode, > > > + struct drm_display_mode *adj_mode) > > > +{ > > > + struct sprd_dsi *dsi = encoder_to_dsi(encoder); > > > + > > > + drm_dbg(dsi->drm, "%s() set mode: %s\n", __func__, dsi->mode->name); > > > +} > > > > You don't need that function? > No need for now. need to delete it? Yes > > > +static int sprd_dsi_encoder_atomic_check(struct drm_encoder *encoder, > > > + struct drm_crtc_state *crtc_state, > > > + struct drm_connector_state *conn_state) > > > +{ > > > + return 0; > > > +} > > > > Ditto > > No need for now. need to delete it? Yep > > > +static int sprd_dsi_find_panel(struct sprd_dsi *dsi) > > > +{ > > > + struct device *dev = dsi->host.dev; > > > + struct device_node *child, *lcds_node; > > > + struct drm_panel *panel; > > > + > > > + /* search /lcds child node first */ > > > + lcds_node = of_find_node_by_path("/lcds"); > > > + for_each_child_of_node(lcds_node, child) { > > > + panel = of_drm_find_panel(child); > > > + if (!IS_ERR(panel)) { > > > + dsi->panel = panel; > > > + return 0; > > > + } > > > + } > > > + > > > + /* > > > + * If /lcds child node search failed, we search > > > + * the child of dsi host node. > > > + */ > > > + for_each_child_of_node(dev->of_node, child) { > > > + panel = of_drm_find_panel(child); > > > + if (!IS_ERR(panel)) { > > > + dsi->panel = panel; > > > + return 0; > > > + } > > > + } > > > + > > > + drm_err(dsi->drm, "of_drm_find_panel() failed\n"); > > > + return -ENODEV; > > > +} > > > > Just use devm_drm_of_get_bridge there > > We use drm_panel_init and drm_panel_add API to add panel, so here is a > panel device, not a bridge. Like Sam said, the panel API is on its way out and is being superseded by bridge_panels. > > > +static int sprd_dsi_host_init(struct sprd_dsi *dsi, struct device *dev) > > > +{ > > > + int ret; > > > + > > > + dsi->host.dev = dev; > > > + dsi->host.ops = &sprd_dsi_host_ops; > > > + > > > + ret = mipi_dsi_host_register(&dsi->host); > > > + if (ret) > > > + drm_err(dsi->drm, "failed to register dsi host\n"); > > > + > > > + return ret; > > > +} > > > > > > [...] > > > > > > +static int sprd_dsi_connector_init(struct drm_device *drm, struct > > > sprd_dsi *dsi) > > > +{ > > > + struct drm_encoder *encoder = &dsi->encoder; > > > + struct drm_connector *connector = &dsi->connector; > > > + int ret; > > > + > > > + connector->polled = DRM_CONNECTOR_POLL_HPD; > > > + > > > + ret = drm_connector_init(drm, connector, > > > + &sprd_dsi_atomic_connector_funcs, > > > + DRM_MODE_CONNECTOR_DSI); > > > + if (ret) { > > > + drm_err(drm, "drm_connector_init() failed\n"); > > > + return ret; > > > + } > > > + > > > + drm_connector_helper_add(connector, > > > + &sprd_dsi_connector_helper_funcs); > > > + > > > + drm_connector_attach_encoder(connector, encoder); > > > + > > > + return 0; > > > +} > > > + > > > +static int sprd_dsi_context_init(struct sprd_dsi *dsi, > > > + struct device *dev) > > > +{ > > > + struct platform_device *pdev = to_platform_device(dev); > > > + struct dsi_context *ctx = &dsi->ctx; > > > + struct resource *res; > > > + > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > + ctx->base = devm_ioremap(dev, res->start, resource_size(res)); > > > + if (!ctx->base) { > > > + drm_err(dsi->drm, "failed to map dsi host registers\n"); > > > + return -ENXIO; > > > + } > > > + > > > + ctx->pll = devm_kzalloc(dev, sizeof(*ctx->pll), GFP_KERNEL); > > > + if (!ctx->pll) > > > + return -ENOMEM; > > > + > > > + ctx->regmap = devm_regmap_init(dev, ®map_tst_io, dsi, > > > &byte_config); > > > + if (IS_ERR(ctx->regmap)) { > > > + drm_err(dsi->drm, "dphy regmap init failed\n"); > > > + return PTR_ERR(ctx->regmap); > > > + } > > > + > > > + ctx->data_hs2lp = 120; > > > + ctx->data_lp2hs = 500; > > > + ctx->clk_hs2lp = 4; > > > + ctx->clk_lp2hs = 15; > > > + ctx->max_rd_time = 6000; > > > + ctx->int0_mask = 0x; > > > + ctx->int1_mask = 0x; > > > + ctx->enabled = true; > > > + > > > + return 0; > > > +} > > > + > > > +static int sprd_dsi_bind(struct device *dev, struct device *master, void > > > *data) > > > +{ > > > + s
Re: [PATCH v4 03/10] dt-bindings: display: Add ingenic, jz4780-dw-hdmi DT Schema
> Am 28.09.2021 um 11:18 schrieb Maxime Ripard : > > On Tue, Sep 28, 2021 at 10:59:45AM +0200, H. Nikolaus Schaller wrote: +properties: + compatible: +items: + - const: ingenic,jz4780-dw-hdmi >>> >>> This can just be a const, there's no need for the items >> >> Maybe starting with an enum is better if more compatible strings are to be >> added. > > it's still fairly easy to change if needed, there's no need to confuse > anyone. > >>> + reg-io-width: +const: 4 >>> >>> If it's fixed, why do you need it in the first place? >> >> There is a fixed default of 1 if not specified. > > My point was more about why do you need to have that property at all? > Can't you just drop it and assume that the register width is 32 bits if > it's all you will ever run on? No, please see bridge/synopsys,dw-hdmi.yaml where it is derived from: reg-io-width: description: Width (in bytes) of the registers specified by the reg property. allOf: - $ref: /schemas/types.yaml#/definitions/uint32 - enum: [1, 4] default: 1 Other bindings define it explicitly to be 4, e.g. Documentation//devicetree/bindings/display/intel,keembay-msscam.yaml: reg-io-width: Documentation//devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml: reg-io-width: Therefore I'd assume that a regmap is not properly set up if we don't require the DTS to include it with const: 4. > + clocks: +maxItems: 2 +description: Clock specifiers for isrf and iahb clocks >>> >>> This can be defined as >>> >>> clocks: >>> items: >>> - description: isrf >>> - description: iahb >>> >>> A better description about what these clocks are would be nice as well >> >> Generally I see that this all is nowadays not independent of >> >> Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml >> >> where there is already a description. > > Ok, good then > >> On the other hand every SoC specialization runs its own copy. e.g. >> >> Documentation/devicetree/bindings/display/imx/fsl,imx6-hdmi.yaml >> Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yam >> >>> + clock-names: +items: + - const: isfr >>> >>> Is it isfr or isrf? >> >> isfr. Seems to be a typo in the description. See >> bridge/synopsys,dw-hdmi.yaml# >> >> One question to the yaml specialists: >> >> since ../bridge/synopsys,dw-hdmi.yaml# already defines this, do we >> have to repeat? Or can we reduce to just the changes? > > If you add the ref you mentionned above, you don't have to repeat Nice. It defines: clocks: minItems: 2 maxItems: 5 items: - description: The bus clock for either AHB and APB - description: The internal register configuration clock additionalItems: true > yourself indeed. You can just put clock-names: true Or should we do clocks: maxItems: 2 additionalItems: false > >> [I am still not familiar enough with the yaml stuff to understand if >> it has sort of inheritance like device tree include files, so that you >> just have to change relevant properties] > > Kind of, but not entirely. schemas are all applied separately, unlike DT > includes that will just expand to one big DT. In practice, it means that > your device must validate against all the schemas, not just the sum of > them. > > For example, if you have a generic schema that has: > > properties: > compatible: >const: vendor,my-generic-compatible > > > and your schema that extends the generic binding, with a ref to the > generic one that has: > > properties: > compatible: >items: > - const: other-vendor,my-device-compatible > - const: vendor,my-generic-compatible > > > It will still fail since the generic schema expects only a single > compatible, whereas your device would have two. Ok, I see. it is not a simple "overwrite" rule. > >>> + - const: iahb >> >> would it make sense to add additionalItems: false here? >> >> In the jz4780 case there are just two clocks while other specializations >> use more and synopsys,dw-hdmi.yaml# defines additionalItems: true. > > If you want to refine the generic one, and it's all the clocks you ever > expect then there's no need for additionalItems Ok. > >>> +description: An I2C interface if the internal DDC I2C driver is not to be used + ports: true >>> >>> If there's a single port, you don't need ports >> >> There can be two ports - one for input from LCDC and one >> for output (HDMI connector). But explicitly defining an output >> port is optional to some extent (depending on driver structure). > > This needs to be defined then (and port@0 made mandatory) Ok. I'll try to make the best out of it for v5 series. Maybe it is still not perfect by then, but close... > > Maxime BR and thanks, Nikolaus
Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output
Hi Nikolaus / Paul, Le lun., sept. 27 2021 at 18:44:20 +0200, H. Nikolaus Schaller a écrit : From: Paul Boddie Add support for the LCD controller present on JZ4780 SoCs. This SoC uses 8-byte descriptors which extend the current 4-byte descriptors used for other Ingenic SoCs. Tested on MIPS Creator CI20 board. Signed-off-by: Paul Boddie Signed-off-by: Ezequiel Garcia Signed-off-by: H. Nikolaus Schaller --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85 +-- drivers/gpu/drm/ingenic/ingenic-drm.h | 42 +++ 2 files changed, 122 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index f73522bdacaa..e2df4b085905 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -6,6 +6,7 @@ #include "ingenic-drm.h" +#include #include #include #include @@ -49,6 +50,11 @@ struct ingenic_dma_hwdesc { u32 addr; u32 id; u32 cmd; + /* extended hw descriptor for jz4780 */ + u32 offsize; + u32 pagewidth; + u32 cpos; + u32 dessize; } __aligned(16); struct ingenic_dma_hwdescs { @@ -60,9 +66,11 @@ struct jz_soc_info { bool needs_dev_clk; bool has_osd; bool map_noncoherent; + bool use_extended_hwdesc; unsigned int max_width, max_height; const u32 *formats_f0, *formats_f1; unsigned int num_formats_f0, num_formats_f1; + unsigned int max_reg; }; struct ingenic_drm_private_state { @@ -168,12 +176,11 @@ static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg) } } -static const struct regmap_config ingenic_drm_regmap_config = { +static struct regmap_config ingenic_drm_regmap_config = { .reg_bits = 32, .val_bits = 32, .reg_stride = 4, - .max_register = JZ_REG_LCD_SIZE1, .writeable_reg = ingenic_drm_writeable_reg, }; @@ -663,6 +670,37 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane, hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4); hwdesc->next = dma_hwdesc_addr(priv, next_id); + if (priv->soc_info->use_extended_hwdesc) { + hwdesc->cmd |= JZ_LCD_CMD_FRM_ENABLE; + + /* Extended 8-byte descriptor */ + hwdesc->cpos = 0; + hwdesc->offsize = 0; + hwdesc->pagewidth = 0; + + switch (newstate->fb->format->format) { + case DRM_FORMAT_XRGB1555: + hwdesc->cpos |= JZ_LCD_CPOS_RGB555; + fallthrough; + case DRM_FORMAT_RGB565: + hwdesc->cpos |= JZ_LCD_CPOS_BPP_15_16; + break; + case DRM_FORMAT_XRGB: + hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24; + break; + } + hwdesc->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD | + (JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1 << +JZ_LCD_CPOS_COEFFICIENT_OFFSET); + + hwdesc->dessize = + (0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) | + FIELD_PREP(JZ_LCD_DESSIZE_HEIGHT_MASK << + JZ_LCD_DESSIZE_HEIGHT_OFFSET, height - 1) | + FIELD_PREP(JZ_LCD_DESSIZE_WIDTH_MASK << + JZ_LCD_DESSIZE_WIDTH_OFFSET, width - 1); + } + if (drm_atomic_crtc_needs_modeset(crtc_state)) { fourcc = newstate->fb->format->format; @@ -694,6 +732,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE; } + /* set use of the 8-word descriptor and OSD foreground usage. */ + if (priv->soc_info->use_extended_hwdesc) + cfg |= JZ_LCD_CFG_DESCRIPTOR_8; + if (mode->flags & DRM_MODE_FLAG_NHSYNC) cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW; if (mode->flags & DRM_MODE_FLAG_NVSYNC) @@ -1010,6 +1052,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) struct drm_encoder *encoder; struct ingenic_drm_bridge *ib; struct drm_device *drm; + struct regmap_config regmap_config; void __iomem *base; long parent_rate; unsigned int i, clone_mask = 0; @@ -1063,8 +1106,10 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) return PTR_ERR(base); } + regmap_config = ingenic_drm_regmap_config; + regmap_config.max_register = soc_i
Re: [PATCH] drm/amdgpu: fix clang out-of-range warning
On 2021-09-27 14:19, Arnd Bergmann wrote: > From: Arnd Bergmann > > clang-14 points out that comparing an 'unsigned int' against a large > 64-bit constantn is pointless: > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c:1206:18: error: result of > comparison of constant 4294967296 with expression of type 'resource_size_t' > (aka 'unsigned int') is always false > [-Werror,-Wtautological-constant-out-of-range-compare] > res->start > 0x1ull) > > Rephrase the comparison using the upper_32_bits() macro, which should > keep it legible while avoiding the warning. > > Fixes: 31b8adab3247 ("drm/amdgpu: require a root bus window above 4GB for BAR > resize") > Signed-off-by: Arnd Bergmann > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index ab3794c42d36..741a55031ca1 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -1203,7 +1203,7 @@ int amdgpu_device_resize_fb_bar(struct amdgpu_device > *adev) > > pci_bus_for_each_resource(root, res, i) { > if (res && res->flags & (IORESOURCE_MEM | IORESOURCE_MEM_64) && > - res->start > 0x1ull) > + upper_32_bits(res->start) != 0) New code matches 1ull << 32, old code didn't. If this difference matters, this would break if res->start is 64 bits? -- Earthling Michel Dänzer| https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
Re: [PATCH v4 10/10] drm/ingenic: add some jz4780 specific features
Hi, Le lun., sept. 27 2021 at 18:44:28 +0200, H. Nikolaus Schaller a écrit : From: Paul Boddie The jz4780 has some features which need initialization according to the vendor kernel. Signed-off-by: Paul Boddie Signed-off-by: H. Nikolaus Schaller --- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 39 +++ 1 file changed, 39 insertions(+) diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c index e2df4b085905..605549b316b5 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c @@ -66,6 +66,10 @@ struct jz_soc_info { bool needs_dev_clk; bool has_osd; bool map_noncoherent; + bool has_alpha; + bool has_pcfg; + bool has_recover; + bool has_rgbc; bool use_extended_hwdesc; unsigned int max_width, max_height; const u32 *formats_f0, *formats_f1; @@ -732,6 +736,9 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder, | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE; } + if (priv->soc_info->has_recover) + cfg |= JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN; Did you actually test this? I know that in theory it sounds like something we'd want, but unless there is a proven use for it, it's better to keep it disabled. + /* set use of the 8-word descriptor and OSD foreground usage. */ if (priv->soc_info->use_extended_hwdesc) cfg |= JZ_LCD_CFG_DESCRIPTOR_8; @@ -1321,6 +1328,25 @@ static int ingenic_drm_bind(struct device *dev, bool has_components) if (soc_info->has_osd) regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN); + if (soc_info->has_alpha) + regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_ALPHAEN); I remember you saying that OSD mode was not yet working on the JZ4780. So I can't see how you could have tested this. + + /* Magic values from the vendor kernel for the priority thresholds. */ + if (soc_info->has_pcfg) + regmap_write(priv->map, JZ_REG_LCD_PCFG, +JZ_LCD_PCFG_PRI_MODE | +JZ_LCD_PCFG_HP_BST_16 | +(511 << JZ_LCD_PCFG_THRESHOLD2_OFFSET) | +(400 << JZ_LCD_PCFG_THRESHOLD1_OFFSET) | +(256 << JZ_LCD_PCFG_THRESHOLD0_OFFSET)); Unless you add a big comment that explains what these values do and why we do want them, I don't want magic values in here. The fact that the kernel vendor sets this doesn't mean it's needed and/or wanted. + + /* RGB output control may be superfluous. */ + if (soc_info->has_rgbc) + regmap_write(priv->map, JZ_REG_LCD_RGBC, +JZ_LCD_RGBC_RGB_FORMAT_ENABLE | +JZ_LCD_RGBC_ODD_RGB | +JZ_LCD_RGBC_EVEN_RGB); ingenic-drm only supports RGB output right now, so I guess the RGB_FORMAT_ENABLE bit needs to be set in patch [2/10], otherwise patch [2/10] cannot state that it adds support for the JZ4780, if it doesn't actually work. The other two bits can be dropped, they are already set in ingenic_drm_encoder_atomic_mode_set(). + mutex_init(&priv->clk_mutex); priv->clock_nb.notifier_call = ingenic_drm_update_pixclk; @@ -1484,6 +1510,9 @@ static const struct jz_soc_info jz4740_soc_info = { .needs_dev_clk = true, .has_osd = false, .map_noncoherent = false, + .has_pcfg = false, + .has_recover = false, + .has_rgbc = false, .max_width = 800, .max_height = 600, .formats_f1 = jz4740_formats, @@ -1496,6 +1525,9 @@ static const struct jz_soc_info jz4725b_soc_info = { .needs_dev_clk = false, .has_osd = true, .map_noncoherent = false, + .has_pcfg = false, + .has_recover = false, + .has_rgbc = false, This is wrong, the JZ4725B and JZ4770 SoCs both have the RGBC register and the RECOVER bit. Cheers, -Paul .max_width = 800, .max_height = 600, .formats_f1 = jz4725b_formats_f1, @@ -1509,6 +1541,9 @@ static const struct jz_soc_info jz4770_soc_info = { .needs_dev_clk = false, .has_osd = true, .map_noncoherent = true, + .has_pcfg = false, + .has_recover = false, + .has_rgbc = false, .max_width = 1280, .max_height = 720, .formats_f1 = jz4770_formats_f1, @@ -1521,6 +1556,10 @@ static const struct jz_soc_info jz4770_soc_info = { static const struct jz_soc_info jz4780_soc_info = { .needs_dev_clk = true, .has_osd = true, + .has_alpha = true, + .has_pcfg = true, + .has_recover = true, + .has_rgbc = true, .use_extended_hwdesc = true, .max_width = 4096, .max_height = 2048, -- 2.31.1
Re: [PATCH v4 10/10] drm/ingenic: add some jz4780 specific features
Hi Paul, > Am 28.09.2021 um 11:58 schrieb Paul Cercueil : > > Hi, > > Le lun., sept. 27 2021 at 18:44:28 +0200, H. Nikolaus Schaller > a écrit : >> From: Paul Boddie >> The jz4780 has some features which need initialization >> according to the vendor kernel. >> Signed-off-by: Paul Boddie >> Signed-off-by: H. Nikolaus Schaller >> --- >> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 39 +++ >> 1 file changed, 39 insertions(+) >> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> index e2df4b085905..605549b316b5 100644 >> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> @@ -66,6 +66,10 @@ struct jz_soc_info { >> bool needs_dev_clk; >> bool has_osd; >> bool map_noncoherent; >> +bool has_alpha; >> +bool has_pcfg; >> +bool has_recover; >> +bool has_rgbc; >> bool use_extended_hwdesc; >> unsigned int max_width, max_height; >> const u32 *formats_f0, *formats_f1; >> @@ -732,6 +736,9 @@ static void ingenic_drm_encoder_atomic_mode_set(struct >> drm_encoder *encoder, >> | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE; >> } >> +if (priv->soc_info->has_recover) >> +cfg |= JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN; > > Did you actually test this? I know that in theory it sounds like something > we'd want, but unless there is a proven use for it, it's better to keep it > disabled. > >> + >> /* set use of the 8-word descriptor and OSD foreground usage. */ >> if (priv->soc_info->use_extended_hwdesc) >> cfg |= JZ_LCD_CFG_DESCRIPTOR_8; >> @@ -1321,6 +1328,25 @@ static int ingenic_drm_bind(struct device *dev, bool >> has_components) >> if (soc_info->has_osd) >> regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN); >> +if (soc_info->has_alpha) >> +regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, >> JZ_LCD_OSDC_ALPHAEN); > > I remember you saying that OSD mode was not yet working on the JZ4780. So I > can't see how you could have tested this. Basically this is all stuff from the vendor kernel under the assumption that they know better than everyone of us. On the other hand this whole patch is sort of optional and we know that the basic milestone to get HDMI working is reached without it. So if you prefer we can drop it for the moment in v5 and leave it for further analysis later. > >> + >> +/* Magic values from the vendor kernel for the priority thresholds. */ >> +if (soc_info->has_pcfg) >> +regmap_write(priv->map, JZ_REG_LCD_PCFG, >> + JZ_LCD_PCFG_PRI_MODE | >> + JZ_LCD_PCFG_HP_BST_16 | >> + (511 << JZ_LCD_PCFG_THRESHOLD2_OFFSET) | >> + (400 << JZ_LCD_PCFG_THRESHOLD1_OFFSET) | >> + (256 << JZ_LCD_PCFG_THRESHOLD0_OFFSET)); > > Unless you add a big comment that explains what these values do and why we do > want them, I don't want magic values in here. The fact that the kernel vendor > sets this doesn't mean it's needed and/or wanted. Well, who has a contact within Ingenic? > >> + >> +/* RGB output control may be superfluous. */ >> +if (soc_info->has_rgbc) >> +regmap_write(priv->map, JZ_REG_LCD_RGBC, >> + JZ_LCD_RGBC_RGB_FORMAT_ENABLE | >> + JZ_LCD_RGBC_ODD_RGB | >> + JZ_LCD_RGBC_EVEN_RGB); > > ingenic-drm only supports RGB output right now, so I guess the > RGB_FORMAT_ENABLE bit needs to be set in patch [2/10], otherwise patch [2/10] > cannot state that it adds support for the JZ4780, if it doesn't actually work. > > The other two bits can be dropped, they are already set in > ingenic_drm_encoder_atomic_mode_set(). Ok. > >> + >> mutex_init(&priv->clk_mutex); >> priv->clock_nb.notifier_call = ingenic_drm_update_pixclk; >> @@ -1484,6 +1510,9 @@ static const struct jz_soc_info jz4740_soc_info = { >> .needs_dev_clk = true, >> .has_osd = false, >> .map_noncoherent = false, >> +.has_pcfg = false, >> +.has_recover = false, >> +.has_rgbc = false, >> .max_width = 800, >> .max_height = 600, >> .formats_f1 = jz4740_formats, >> @@ -1496,6 +1525,9 @@ static const struct jz_soc_info jz4725b_soc_info = { >> .needs_dev_clk = false, >> .has_osd = true, >> .map_noncoherent = false, >> +.has_pcfg = false, >> +.has_recover = false, >> +.has_rgbc = false, > > This is wrong, the JZ4725B and JZ4770 SoCs both have the RGBC register and > the RECOVER bit. Ok, good to know! Will change. BR and thanks, Nikolaus > > Cheers, > -Paul > >> .max_width = 800, >> .max_height = 600, >> .formats_f1 = jz4725b_formats_f1, >> @@ -1509,6 +1541,9 @@ static const struct jz_soc_info jz4770_soc_info = { >> .needs_dev_clk = false, >> .has_osd
Re: [PATCH] drm: mxsfb: Set proper default bus format when using a bridge
On 9/28/21 11:27 AM, Lucas Stach wrote: Am Dienstag, dem 28.09.2021 um 11:19 +0200 schrieb Guido Günther: Hi, On Tue, Sep 28, 2021 at 11:08:58AM +0200, Marek Vasut wrote: On 9/28/21 10:55 AM, Guido Günther wrote: If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in that case. This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") Signed-off-by: Guido Günther --- I'll look at what needs to be done in nwl separately but this also unbreaks other bridge seupts that don't to format negotiation yet. drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index af6c620adf6e..4ef94cf686b0 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, drm_atomic_get_new_bridge_state(state, mxsfb->bridge); bus_format = bridge_state->input_bus_cfg.format; + if (bus_format == MEDIA_BUS_FMT_FIXED) + bus_format = MEDIA_BUS_FMT_RGB888_1X24; Shouldn't the NWL bridge return the correct format ? Yes it should and I'll send a separate patch for that but we currently don't do anything meaningful at all if the bridge doesn't do format negotiation and then fail setup in mxsfb_set_formats(). I think we should at least preserve the status quo (as we do with the non bridge case in b776b0f00f24 too). We could have a warning to spot drivers that don't do that yet and hence the generic code returns MEDIA_BUS_FMT_FIXED. That sounds sensible. Using a default format if we don't know what to do is going to be a unpleasant surprise for those with a display pipeline that doesn't work with the default format. So please add a dev_warn when we are doing this fallback. Also I would argue that the NWL fix is the patch that should go in the stable tree. This one should only be a additional safety net, so I would drop the Fixes tag. Indeed
Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output
Hi, > Am 28.09.2021 um 11:35 schrieb Paul Cercueil : > > Hi Nikolaus / Paul, > > Le lun., sept. 27 2021 at 18:44:20 +0200, H. Nikolaus Schaller > a écrit : >> From: Paul Boddie >> Add support for the LCD controller present on JZ4780 SoCs. >> This SoC uses 8-byte descriptors which extend the current >> 4-byte descriptors used for other Ingenic SoCs. >> Tested on MIPS Creator CI20 board. >> Signed-off-by: Paul Boddie >> Signed-off-by: Ezequiel Garcia >> Signed-off-by: H. Nikolaus Schaller >> --- >> drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 85 +-- >> drivers/gpu/drm/ingenic/ingenic-drm.h | 42 +++ >> 2 files changed, 122 insertions(+), 5 deletions(-) >> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> index f73522bdacaa..e2df4b085905 100644 >> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c >> @@ -6,6 +6,7 @@ >> #include "ingenic-drm.h" >> +#include >> #include >> #include >> #include >> @@ -49,6 +50,11 @@ struct ingenic_dma_hwdesc { >> u32 addr; >> u32 id; >> u32 cmd; >> +/* extended hw descriptor for jz4780 */ >> +u32 offsize; >> +u32 pagewidth; >> +u32 cpos; >> +u32 dessize; >> } __aligned(16); >> struct ingenic_dma_hwdescs { >> @@ -60,9 +66,11 @@ struct jz_soc_info { >> bool needs_dev_clk; >> bool has_osd; >> bool map_noncoherent; >> +bool use_extended_hwdesc; >> unsigned int max_width, max_height; >> const u32 *formats_f0, *formats_f1; >> unsigned int num_formats_f0, num_formats_f1; >> +unsigned int max_reg; >> }; >> struct ingenic_drm_private_state { >> @@ -168,12 +176,11 @@ static bool ingenic_drm_writeable_reg(struct device >> *dev, unsigned int reg) >> } >> } >> -static const struct regmap_config ingenic_drm_regmap_config = { >> +static struct regmap_config ingenic_drm_regmap_config = { >> .reg_bits = 32, >> .val_bits = 32, >> .reg_stride = 4, >> -.max_register = JZ_REG_LCD_SIZE1, >> .writeable_reg = ingenic_drm_writeable_reg, >> }; >> @@ -663,6 +670,37 @@ static void ingenic_drm_plane_atomic_update(struct >> drm_plane *plane, >> hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4); >> hwdesc->next = dma_hwdesc_addr(priv, next_id); >> +if (priv->soc_info->use_extended_hwdesc) { >> +hwdesc->cmd |= JZ_LCD_CMD_FRM_ENABLE; >> + >> +/* Extended 8-byte descriptor */ >> +hwdesc->cpos = 0; >> +hwdesc->offsize = 0; >> +hwdesc->pagewidth = 0; >> + >> +switch (newstate->fb->format->format) { >> +case DRM_FORMAT_XRGB1555: >> +hwdesc->cpos |= JZ_LCD_CPOS_RGB555; >> +fallthrough; >> +case DRM_FORMAT_RGB565: >> +hwdesc->cpos |= JZ_LCD_CPOS_BPP_15_16; >> +break; >> +case DRM_FORMAT_XRGB: >> +hwdesc->cpos |= JZ_LCD_CPOS_BPP_18_24; >> +break; >> +} >> +hwdesc->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD | >> +(JZ_LCD_CPOS_COEFFICIENT_1_ALPHA1 << >> + JZ_LCD_CPOS_COEFFICIENT_OFFSET); >> + >> +hwdesc->dessize = >> +(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) | >> +FIELD_PREP(JZ_LCD_DESSIZE_HEIGHT_MASK << >> + JZ_LCD_DESSIZE_HEIGHT_OFFSET, height >> - 1) | >> +FIELD_PREP(JZ_LCD_DESSIZE_WIDTH_MASK << >> + JZ_LCD_DESSIZE_WIDTH_OFFSET, width - >> 1); >> +} >> + >> if (drm_atomic_crtc_needs_modeset(crtc_state)) { >> fourcc = newstate->fb->format->format; >> @@ -694,6 +732,10 @@ static void ingenic_drm_encoder_atomic_mode_set(struct >> drm_encoder *encoder, >> | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE; >> } >> +/* set use of the 8-word descriptor and OSD foreground usage. */ >> +if (priv->soc_info->use_extended_hwdesc) >> +cfg |= JZ_LCD_CFG_DESCRIPTOR_8; >> + >> if (mode->flags & DRM_MODE_FLAG_NHSYNC) >> cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW; >> if (mode->flags & DRM_MODE_FLAG_NVSYNC) >> @@ -1010,6 +1052,7 @@ static int ingenic_drm_bind(struct device *dev, bool >> has_components) >> struct drm_encoder *encoder; >> struct ingenic_drm_bridge *ib; >> struct drm_device *drm; >> +struct regmap_config regmap_config; >> void __iomem *base; >> long parent_rate; >> unsigned int i, clone_mask = 0; >> @@ -1063,8 +1106,10 @@ static int ingenic_drm_bind(struct de
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] drm/i915/ttm: Rework object initialization slightly
On 27/09/2021 16:10, Thomas Hellström wrote: We may end up in i915_ttm_bo_destroy() in an error path before the object is fully initialized. In that case it's not correct to call __i915_gem_free_object(), because that function a) Assumes the gem object refcount is 0, which it isn't. b) frees the placements which are owned by the caller until the init_object() region ops returns successfully. Fix this by providing a lightweight cleanup function i915_gem_object_fini() which is also called by __i915_gem_free_object(). While doing this, also make sure we call dma_resv_fini() as part of ordinary object destruction and not from the RCU callback that frees the object. This will help track down bugs where the object is incorrectly locked from an RCU lookup. Finally, make sure the object isn't put on the region list until it's either locked or fully initialized in order to block list processing of partially initialized objects. Signed-off-by: Thomas Hellström --- drivers/gpu/drm/i915/gem/i915_gem_object.c | 18 ++-- drivers/gpu/drm/i915/gem/i915_gem_object.h | 3 ++ drivers/gpu/drm/i915/gem/i915_gem_ttm.c| 32 +- 3 files changed, 38 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c index 6fb9afb65034..244e555f9bba 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c @@ -89,6 +89,20 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj, mutex_init(&obj->mm.get_dma_page.lock); } +/** + * i915_gem_object_fini - Clean up a GEM object initialization + * @obj: The gem object cleanup + * + * This function cleans up gem object fields that are set up by + * drm_gem_private_object_init() and i915_gem_object_init(). + */ +void i915_gem_object_fini(struct drm_i915_gem_object *obj) +{ + mutex_destroy(&obj->mm.get_page.lock); + mutex_destroy(&obj->mm.get_dma_page.lock); + dma_resv_fini(&obj->base._resv); +} + /** * Mark up the object's coherency levels for a given cache_level * @obj: #drm_i915_gem_object @@ -174,7 +188,6 @@ void __i915_gem_free_object_rcu(struct rcu_head *head) container_of(head, typeof(*obj), rcu); struct drm_i915_private *i915 = to_i915(obj->base.dev); - dma_resv_fini(&obj->base._resv); i915_gem_object_free(obj); GEM_BUG_ON(!atomic_read(&i915->mm.free_count)); @@ -223,7 +236,6 @@ void __i915_gem_free_object(struct drm_i915_gem_object *obj) obj_link))) { GEM_BUG_ON(vma->obj != obj); spin_unlock(&obj->vma.lock); - __i915_vma_put(vma); Unrelated change? Not seeing any DG1 machines in CI currently, so assuming this was tested locally, Reviewed-by: Matthew Auld
Re: (subset) [PATCH v2 1/3] drm/probe-helper: Document drm_helper_hpd_irq_event() return value
On Tue, 14 Sep 2021 12:17:22 +0200, Maxime Ripard wrote: > The documentation of the drm_helper_hpd_irq_event() function didn't > document the value that function was returning. Add that part as well. > > Applied to drm/drm-misc (drm-misc-next). Thanks! Maxime
Re: (subset) [PATCH v2 2/3] drm/probe-helper: Create a HPD IRQ event helper for a single connector
On Tue, 14 Sep 2021 12:17:23 +0200, Maxime Ripard wrote: > The drm_helper_hpd_irq_event() function is iterating over all the > connectors when an hotplug event is detected. > > During that iteration, it will call each connector detect function and > figure out if its status changed. > > Finally, if any connector changed, it will notify the user-space and the > clients that something changed on the DRM device. > > [...] Applied to drm/drm-misc (drm-misc-next). Thanks! Maxime
Re: (subset) [PATCH v2 3/3] drm/vc4: hdmi: Actually check for the connector status in hotplug
On Tue, 14 Sep 2021 12:17:24 +0200, Maxime Ripard wrote: > The drm_helper_hpd_irq_event() documentation states that this function > is "useful for drivers which can't or don't track hotplug interrupts for > each connector." and that "Drivers which support hotplug interrupts for > each connector individually and which have a more fine-grained detect > logic should bypass this code and directly call > drm_kms_helper_hotplug_event()". This is thus what we ended-up doing. > > [...] Applied to drm/drm-misc (drm-misc-next). Thanks! Maxime
Re: [PATCH v2] component: do not leave master devres group open after bind
Hey, On Tue, 28 Sep 2021, Takashi Iwai wrote: > On Wed, 22 Sep 2021 10:54:32 +0200, Kai Vehmanen wrote: > > --- 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? I wondered about this as well. Functionally it should be equivalent as passing NULL will apply the operation to the latest added group. I noted the practise of passing NULL has been followed in the existing code when referring to groups created within the same function. E.g. » if (!devres_open_group(component->dev, component, GFP_KERNEL)) { [...] » ret = component->ops->bind(component->dev, master->parent, data); » if (!ret) { » » component->bound = true; » » /* » »* Close the component device's group so that resources » »* allocated in the binding are encapsulated for removal » »* at unbind. Remove the group on the DRM device as we » »* can clean those resources up independently. » »*/ » » devres_close_group(component->dev, NULL); ... so I followed this existing practise. I can change and send a V3 if the explicit parameter is preferred. Br, Kai
Re: [PATCH] drm/nouveau: avoid a use-after-free when BO init fails
Reviewed-by: Karol Herbst and queued On Fri, Mar 26, 2021 at 10:41 PM Lyude Paul wrote: > > Reviewed-by: Lyude Paul > > On Wed, 2020-12-02 at 19:02 -0500, Jeremy Cline wrote: > > nouveau_bo_init() is backed by ttm_bo_init() and ferries its return code > > back to the caller. On failures, ttm_bo_init() invokes the provided > > destructor which should de-initialize and free the memory. > > > > Thus, when nouveau_bo_init() returns an error the gem object has already > > been released and the memory freed by nouveau_bo_del_ttm(). > > > > Fixes: 019cbd4a4feb ("drm/nouveau: Initialize GEM object before TTM object") > > Cc: Thierry Reding > > Signed-off-by: Jeremy Cline > > --- > > drivers/gpu/drm/nouveau/nouveau_gem.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c > > b/drivers/gpu/drm/nouveau/nouveau_gem.c > > index 787d05eefd9c..d30157cc7169 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c > > @@ -211,10 +211,8 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int > > align, uint32_t domain, > > } > > > > ret = nouveau_bo_init(nvbo, size, align, domain, NULL, NULL); > > - if (ret) { > > - nouveau_bo_ref(NULL, &nvbo); > > + if (ret) > > return ret; > > - } > > > > /* we restrict allowed domains on nv50+ to only the types > > * that were requested at creation time. not possibly on > > -- > Sincerely, >Lyude Paul (she/her) >Software Engineer at Red Hat > > Note: I deal with a lot of emails and have a lot of bugs on my plate. If > you've > asked me a question, are waiting for a review/merge on a patch, etc. and I > haven't responded in a while, please feel free to send me another email to > check > on my status. I don't bite! > > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH linux-next] drm/nouveau/mmu: drop unneeded assignment in the nvkm_uvmm_mthd_page()
Reviewed-by: Karol Herbst On Sat, Aug 21, 2021 at 10:46 AM CGEL wrote: > > From: Luo penghao > > In order to keep the code style consistency of the whole file, > the 'ret' assignments should be deleted. > > The clang_analyzer complains as follows: > > drivers/gpu/drm/nouveau/nvkm/subdev/mmu/uvmm.c:317:8:warning: > Although the value storedto 'ret' is used in the enclosing expression, > the value is never actually read from 'ret'. > > Reported-by: Zeal Robot > Signed-off-by: Luo penghao > --- > drivers/gpu/drm/nouveau/nvkm/subdev/mmu/uvmm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/uvmm.c > b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/uvmm.c > index c43b824..d9f8e11 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/uvmm.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/uvmm.c > @@ -314,7 +314,7 @@ nvkm_uvmm_mthd_page(struct nvkm_uvmm *uvmm, void *argv, > u32 argc) > page = uvmm->vmm->func->page; > for (nr = 0; page[nr].shift; nr++); > > - if (!(ret = nvif_unpack(ret, &argv, &argc, args->v0, 0, 0, false))) { > + if (!(nvif_unpack(ret, &argv, &argc, args->v0, 0, 0, false))) { > if ((index = args->v0.index) >= nr) > return -EINVAL; > type = page[index].type; > -- > 2.15.2 > >
Re: [PATCH linux-next] drm/nouveau/mmu/gp100-: drop unneeded assignment in the if condition.
Reviewed-by: Karol Herbst but I will remove the unnecessary brackets as well On Sat, Aug 21, 2021 at 10:46 AM CGEL wrote: > > From: Luo penghao > > In order to keep the code style consistency of the whole file, > the 'inst' assignments should be deleted. > > The clang_analyzer complains as follows: > > drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp100.c:499:8: warning: > Although the value storedto 'inst' is used in the enclosing expression, > the value is never actually read from 'inst'. > > Reported-by: Zeal Robot > Signed-off-by: Luo penghao > --- > drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp100.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp100.c > b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp100.c > index f02abd9..5d7766a 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp100.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp100.c > @@ -502,7 +502,7 @@ gp100_vmm_fault_cancel(struct nvkm_vmm *vmm, void *argv, > u32 argc) > args->v0.inst |= 0x8000; > > if (!WARN_ON(nvkm_gr_ctxsw_pause(device))) { > - if ((inst = nvkm_gr_ctxsw_inst(device)) == args->v0.inst) { > + if ((nvkm_gr_ctxsw_inst(device)) == args->v0.inst) { > gf100_vmm_invalidate(vmm, 0x001b > /* CANCEL_TARGETED. */ | > (args->v0.hub<< 20) | > -- > 2.15.2 > >
Re: [PATCH 1/2] drm/nouveau/kms/nv50-: fix file release memory leak
Reviewed-by: Karol Herbst On Sat, Sep 11, 2021 at 9:45 AM Yang Yingliang wrote: > > When using single_open() for opening, single_release() should be > called, otherwise the 'op' allocated in single_open() will be leaked. > > Fixes: 12885ecbfe62 ("drm/nouveau/kms/nvd9-: Add CRC support") > Reported-by: Hulk Robot > Signed-off-by: Yang Yingliang > --- > drivers/gpu/drm/nouveau/dispnv50/crc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/nouveau/dispnv50/crc.c > b/drivers/gpu/drm/nouveau/dispnv50/crc.c > index b8c31b697797..66f32d965c72 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/crc.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/crc.c > @@ -704,6 +704,7 @@ static const struct file_operations > nv50_crc_flip_threshold_fops = { > .open = nv50_crc_debugfs_flip_threshold_open, > .read = seq_read, > .write = nv50_crc_debugfs_flip_threshold_set, > + .release = single_release, > }; > > int nv50_head_crc_late_register(struct nv50_head *head) > -- > 2.25.1 >
Re: [PATCH 2/2] drm/nouveau/debugfs: fix file release memory leak
Reviewed-by: Karol Herbst On Sat, Sep 11, 2021 at 9:45 AM Yang Yingliang wrote: > > When using single_open() for opening, single_release() should be > called, otherwise the 'op' allocated in single_open() will be leaked. > > Fixes: 6e9fc177399f ("drm/nouveau/debugfs: add copy of sysfs pstate interface > ported to debugfs") > Reported-by: Hulk Robot > Signed-off-by: Yang Yingliang > --- > drivers/gpu/drm/nouveau/nouveau_debugfs.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_debugfs.c > b/drivers/gpu/drm/nouveau/nouveau_debugfs.c > index c2bc05eb2e54..1cbe01048b93 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_debugfs.c > +++ b/drivers/gpu/drm/nouveau/nouveau_debugfs.c > @@ -207,6 +207,7 @@ static const struct file_operations nouveau_pstate_fops = > { > .open = nouveau_debugfs_pstate_open, > .read = seq_read, > .write = nouveau_debugfs_pstate_set, > + .release = single_release, > }; > > static struct drm_info_list nouveau_debugfs_list[] = { > -- > 2.25.1 >
Re: [PATCH v4 10/10] drm/ingenic: add some jz4780 specific features
> Am 28.09.2021 um 12:06 schrieb H. Nikolaus Schaller : > >>> >>> + >>> + /* RGB output control may be superfluous. */ >>> + if (soc_info->has_rgbc) >>> + regmap_write(priv->map, JZ_REG_LCD_RGBC, >>> +JZ_LCD_RGBC_RGB_FORMAT_ENABLE | >>> +JZ_LCD_RGBC_ODD_RGB | >>> +JZ_LCD_RGBC_EVEN_RGB); >> >> ingenic-drm only supports RGB output right now, so I guess the >> RGB_FORMAT_ENABLE bit needs to be set in patch [2/10], otherwise patch >> [2/10] cannot state that it adds support for the JZ4780, if it doesn't >> actually work. interestingly it works without setting anything in this register. >> >> The other two bits can be dropped, they are already set in >> ingenic_drm_encoder_atomic_mode_set(). > > Ok. Setting it manually doesn't change anything visible: root@letux:~# devmem2 0x13050090 /dev/mem opened. Memory mapped at address 0x77e14000. Value at address 0x13050090 (0x77e14090): 0x0 root@letux:~# devmem2 0x13050090 w 0x80 /dev/mem opened. Memory mapped at address 0x77e38000. Value at address 0x13050090 (0x77e38090): 0x0 Written 0x80; readback 0x80 root@letux:~# Same for 0x130A0090. Maybe this lcdc register is not used at all - at least for HDMI? So I'd suggest to drop this whole patch from v5. BR and thanks, Nikolaus
Re: [PATCH] drm: rcar-du: Don't create encoder for unconnected LVDS outputs
On 22/08/2021 01:36, Laurent Pinchart wrote: > On R-Car D3 and E3, the LVDS encoders provide the pixel clock to the DU, > even when LVDS outputs are not used. For this reason, the rcar-lvds > driver probes successfully on those platforms even if no further bridge > or panel is connected to the LVDS output, in order to provide the > rcar_lvds_clk_enable() and rcar_lvds_clk_disable() functions to the DU > driver. > > If an LVDS output isn't connected, trying to create a DRM connector for > the output will fail. Fix this by skipping connector creation in that > case, and also skip creation of the DRM encoder as there's no point in > an encoder without a connector. > > Fixes: e9e056949c92 ("drm: rcar-du: lvds: Convert to DRM panel bridge helper") > Reported-by: Geert Uytterhoeven > Signed-off-by: Laurent Pinchart Perhaps this helps it get upstream... Reviewed-by: Kieran Bingham > --- > drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 16 > drivers/gpu/drm/rcar-du/rcar_lvds.c | 11 +++ > drivers/gpu/drm/rcar-du/rcar_lvds.h | 5 + > 3 files changed, 28 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c > b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c > index 0daa8bba50f5..4bf4e25d7f01 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c > @@ -86,12 +86,20 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu, > } > > /* > - * Create and initialize the encoder. On Gen3 skip the LVDS1 output if > + * Create and initialize the encoder. On Gen3, skip the LVDS1 output if >* the LVDS1 encoder is used as a companion for LVDS0 in dual-link > - * mode. > + * mode, or any LVDS output if it isn't connected. The latter may happen > + * on D3 or E3 as the LVDS encoders are needed to provide the pixel > + * clock to the DU, even when the LVDS outputs are not used. >*/ > - if (rcdu->info->gen >= 3 && output == RCAR_DU_OUTPUT_LVDS1) { > - if (rcar_lvds_dual_link(bridge)) > + if (rcdu->info->gen >= 3) { > + if (output == RCAR_DU_OUTPUT_LVDS1 && > + rcar_lvds_dual_link(bridge)) > + return -ENOLINK; > + > + if ((output == RCAR_DU_OUTPUT_LVDS0 || > + output == RCAR_DU_OUTPUT_LVDS1) && > + !rcar_lvds_is_connected(bridge)) > return -ENOLINK; > } > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c > b/drivers/gpu/drm/rcar-du/rcar_lvds.c > index d061b8de748f..b672c5bd72ee 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > @@ -576,6 +576,9 @@ static int rcar_lvds_attach(struct drm_bridge *bridge, > { > struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge); > > + if (!lvds->next_bridge) > + return 0; > + > return drm_bridge_attach(bridge->encoder, lvds->next_bridge, bridge, >flags); > } > @@ -598,6 +601,14 @@ bool rcar_lvds_dual_link(struct drm_bridge *bridge) > } > EXPORT_SYMBOL_GPL(rcar_lvds_dual_link); > > +bool rcar_lvds_is_connected(struct drm_bridge *bridge) > +{ > + struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge); > + > + return lvds->next_bridge != NULL; > +} > +EXPORT_SYMBOL_GPL(rcar_lvds_is_connected); > + > /* > - > * Probe & Remove > */ > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.h > b/drivers/gpu/drm/rcar-du/rcar_lvds.h > index 222ec0e60785..eb7c6ef03b00 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.h > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.h > @@ -16,6 +16,7 @@ struct drm_bridge; > int rcar_lvds_clk_enable(struct drm_bridge *bridge, unsigned long freq); > void rcar_lvds_clk_disable(struct drm_bridge *bridge); > bool rcar_lvds_dual_link(struct drm_bridge *bridge); > +bool rcar_lvds_is_connected(struct drm_bridge *bridge); > #else > static inline int rcar_lvds_clk_enable(struct drm_bridge *bridge, > unsigned long freq) > @@ -27,6 +28,10 @@ static inline bool rcar_lvds_dual_link(struct drm_bridge > *bridge) > { > return false; > } > +static inline bool rcar_lvds_is_connected(struct drm_bridge *bridge) > +{ > + return false; > +} > #endif /* CONFIG_DRM_RCAR_LVDS */ > > #endif /* __RCAR_LVDS_H__ */ >
Re: [PATCH v4 02/10] drm/ingenic: Add support for JZ4780 and HDMI output
Hi Paul, > Am 28.09.2021 um 12:21 schrieb H. Nikolaus Schaller : > >>> @@ -1492,10 +1555,16 @@ static int ingenic_drm_init(void) >>> { >>> int err; >>> + if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) { >>> + err = platform_driver_register(ingenic_dw_hdmi_driver_ptr); >>> + if (err) >>> + return err; >>> + } >> >> I don't see why you need to register the ingenic-dw-hdmi driver here. Just >> register it in the ingenic-dw-hdmi driver. > > Ok, I never though about this (as the code was not from me). We apparently > just followed the IPU code pattern (learning by example). > > It indeed looks not necessary and would also avoid the > ingenic_dw_hdmi_driver_ptr dependency. > > But: what is ingenic_ipu_driver_ptr then good for? > > If we can get rid of this as well, we can drop patch 1/10 ("drm/ingenic: Fix > drm_init error path if IPU was registered") completely. A quick test shows that it *is* required. At least if I configure everything as modules. But like you I can't explain why. Well, just a very rough idea (may be wrong): the bridge chain is not like an i2c bus and clients are not automatically loaded/probed if linked in the device tree. Therefore the consumer (ingenic_drm_drv) must register the "clients" like IPU and HDMI. BR, Nikolaus
[PATCH v1 0/5] mxsfb/nwl/panels: media bus format fixes
commit b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") added bus format probing to mxsfb this exposed several issues in the display stack as used on the Librem 5: The nwl bridge and the panels didn't bother to set any media bus formats and in that case mxsfb would not pick a reasonable default. This series aims to fix this. This series includes the patch from https://lore.kernel.org/dri-devel/yvlyh%2fsgbritg%2...@qwark.sigxcpu.org/ with a `dev_warn` added. The patches are against 5.15-rc3. I've marked a single patch with a 'fixes' which is enough to unbreak the display stack in 5.15. Guido Günther (5): drm: mxsfb: Print failed bus format in hex drm: mxsfb: Set proper default bus format when using a bridge drm/bridge: nwl-dsi: Add atomic_get_input_bus_fmts drm/panel: mantix: Add media bus format drm/panel: st7703: Add media bus format drivers/gpu/drm/bridge/nwl-dsi.c | 35 +++ drivers/gpu/drm/mxsfb/mxsfb_kms.c | 7 +++- .../gpu/drm/panel/panel-mantix-mlaf057we51.c | 9 + drivers/gpu/drm/panel/panel-sitronix-st7703.c | 8 + 4 files changed, 58 insertions(+), 1 deletion(-) -- 2.33.0
[PATCH v1 1/5] drm: mxsfb: Print failed bus format in hex
media-bus-formats.h has them in hexadecimal as well so matching with that file saves one conversion when debugging. Signed-off-by: Guido Günther --- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index af6c620adf6e..d6abd2077114 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -89,7 +89,7 @@ static void mxsfb_set_formats(struct mxsfb_drm_private *mxsfb, ctrl |= CTRL_BUS_WIDTH_24; break; default: - dev_err(drm->dev, "Unknown media bus format %d\n", bus_format); + dev_err(drm->dev, "Unknown media bus format 0x%x\n", bus_format); break; } -- 2.33.0
[PATCH v1 4/5] drm/panel: mantix: Add media bus format
This allows the DSI bridge to detect the correct bus format. We currently only support MEDIA_BUS_FMT_RGB888_1X24. Signed-off-by: Guido Günther --- drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c | 9 + 1 file changed, 9 insertions(+) diff --git a/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c index f0e9bce23c41..d6bcf1045255 100644 --- a/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c +++ b/drivers/gpu/drm/panel/panel-mantix-mlaf057we51.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -232,6 +233,10 @@ static const struct drm_display_mode default_mode_ys = { .height_mm = 130, }; +static const u32 mantix_bus_formats[] = { + MEDIA_BUS_FMT_RGB888_1X24, +}; + static int mantix_get_modes(struct drm_panel *panel, struct drm_connector *connector) { @@ -253,6 +258,10 @@ static int mantix_get_modes(struct drm_panel *panel, connector->display_info.height_mm = mode->height_mm; drm_mode_probed_add(connector, mode); + drm_display_info_set_bus_formats(&connector->display_info, +mantix_bus_formats, +ARRAY_SIZE(mantix_bus_formats)); + return 1; } -- 2.33.0
[PATCH v1 5/5] drm/panel: st7703: Add media bus format
This allows the DSI bridge to detect the correct bus format. We currently only support MEDIA_BUS_FMT_RGB888_1X24. Signed-off-by: Guido Günther --- drivers/gpu/drm/panel/panel-sitronix-st7703.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c index a2c303e5732c..73f69c929a75 100644 --- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c +++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c @@ -453,6 +453,10 @@ static int st7703_prepare(struct drm_panel *panel) return ret; } +static const u32 mantix_bus_formats[] = { + MEDIA_BUS_FMT_RGB888_1X24, +}; + static int st7703_get_modes(struct drm_panel *panel, struct drm_connector *connector) { @@ -474,6 +478,10 @@ static int st7703_get_modes(struct drm_panel *panel, connector->display_info.height_mm = mode->height_mm; drm_mode_probed_add(connector, mode); + drm_display_info_set_bus_formats(&connector->display_info, +mantix_bus_formats, +ARRAY_SIZE(mantix_bus_formats)); + return 1; } -- 2.33.0
[PATCH v1 3/5] drm/bridge: nwl-dsi: Add atomic_get_input_bus_fmts
Components further up in the chain might ask us for supported formats. Without this MEDIA_BUS_FMT_FIXED is assumed which then breaks display output with mxsfb since it can't determine a proper bus format. We handle the bus formats that correspond to the DSI formats the bridge can potentially output (see chapter 13.6 of the i.MX 8MQ reference manual) - which matches what xsfb can input. Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") Signed-off-by: Guido Günther --- drivers/gpu/drm/bridge/nwl-dsi.c | 35 1 file changed, 35 insertions(+) diff --git a/drivers/gpu/drm/bridge/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi.c index a251cc1f088f..27c80d36846b 100644 --- a/drivers/gpu/drm/bridge/nwl-dsi.c +++ b/drivers/gpu/drm/bridge/nwl-dsi.c @@ -1234,6 +1234,40 @@ static void nwl_dsi_bridge_detach(struct drm_bridge *bridge) drm_of_panel_bridge_remove(dsi->dev->of_node, 1, 0); } +static u32 *nwl_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, +struct drm_bridge_state *bridge_state, +struct drm_crtc_state *crtc_state, +struct drm_connector_state *conn_state, +u32 output_fmt, +unsigned int *num_input_fmts) +{ + u32 *input_fmts, input_fmt; + + *num_input_fmts = 0; + + switch (output_fmt) { + /* If MEDIA_BUS_FMT_FIXED is tested, return default bus format */ + case MEDIA_BUS_FMT_FIXED: + input_fmt = MEDIA_BUS_FMT_RGB888_1X24; + break; + case MEDIA_BUS_FMT_RGB888_1X24: + case MEDIA_BUS_FMT_RGB666_1X18: + case MEDIA_BUS_FMT_RGB565_1X16: + input_fmt = output_fmt; + break; + default: + return NULL; + } + + input_fmts = kcalloc(1, sizeof(*input_fmts), GFP_KERNEL); + if (!input_fmts) + return NULL; + input_fmts[0] = input_fmt; + *num_input_fmts = 1; + + return input_fmts; +} + static const struct drm_bridge_funcs nwl_dsi_bridge_funcs = { .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, @@ -1241,6 +1275,7 @@ static const struct drm_bridge_funcs nwl_dsi_bridge_funcs = { .atomic_check = nwl_dsi_bridge_atomic_check, .atomic_enable = nwl_dsi_bridge_atomic_enable, .atomic_disable = nwl_dsi_bridge_atomic_disable, + .atomic_get_input_bus_fmts = nwl_bridge_atomic_get_input_bus_fmts, .mode_set = nwl_dsi_bridge_mode_set, .mode_valid = nwl_dsi_bridge_mode_valid, .attach = nwl_dsi_bridge_attach, -- 2.33.0
[PATCH v1 2/5] drm: mxsfb: Set proper default bus format when using a bridge
If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in that case. This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") Reported-by: Martin Kepplinger Signed-off-by: Guido Günther --- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index d6abd2077114..f4be16f5c20b 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -369,6 +369,11 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, drm_atomic_get_new_bridge_state(state, mxsfb->bridge); bus_format = bridge_state->input_bus_cfg.format; + if (bus_format == MEDIA_BUS_FMT_FIXED) { + dev_warn_once(drm->dev, + "Bridge does not provide bus format. Please fix."); + bus_format = MEDIA_BUS_FMT_RGB888_1X24; + } } /* If there is no bridge, use bus format from connector */ -- 2.33.0
Re: [PATCH] drm: mxsfb: Set proper default bus format when using a bridge
Hi, On Tue, Sep 28, 2021 at 11:27:49AM +0200, Lucas Stach wrote: > Am Dienstag, dem 28.09.2021 um 11:19 +0200 schrieb Guido Günther: > > Hi, > > On Tue, Sep 28, 2021 at 11:08:58AM +0200, Marek Vasut wrote: > > > On 9/28/21 10:55 AM, Guido Günther wrote: > > > > If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is > > > > returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) > > > > in > > > > that case. > > > > > > > > This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. > > > > > > > > Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest > > > > bridge if present") > > > > > > > > Signed-off-by: Guido Günther > > > > --- > > > > > > > > I'll look at what needs to be done in nwl separately but this also > > > > unbreaks other bridge seupts that don't to format negotiation yet. > > > > > > > > drivers/gpu/drm/mxsfb/mxsfb_kms.c | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > > b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > > index af6c620adf6e..4ef94cf686b0 100644 > > > > --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > > +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c > > > > @@ -369,6 +369,8 @@ static void mxsfb_crtc_atomic_enable(struct > > > > drm_crtc *crtc, > > > > drm_atomic_get_new_bridge_state(state, > > > > mxsfb->bridge); > > > > bus_format = bridge_state->input_bus_cfg.format; > > > > + if (bus_format == MEDIA_BUS_FMT_FIXED) > > > > + bus_format = MEDIA_BUS_FMT_RGB888_1X24; > > > > > > Shouldn't the NWL bridge return the correct format ? > > > > Yes it should and I'll send a separate patch for that but we currently > > don't do anything meaningful at all if the bridge doesn't do format > > negotiation and then fail setup in mxsfb_set_formats(). > > > > I think we should at least preserve the status quo (as we do with the > > non bridge case in b776b0f00f24 too). > > > > We could have a warning to spot drivers that don't do that yet and hence > > the generic code returns MEDIA_BUS_FMT_FIXED. > > > That sounds sensible. Using a default format if we don't know what to > do is going to be a unpleasant surprise for those with a display > pipeline that doesn't work with the default format. So please add a > dev_warn when we are doing this fallback. > > Also I would argue that the NWL fix is the patch that should go in the > stable tree. This one should only be a additional safety net, so I > would drop the Fixes tag. Dropped and folded into the small series with other fixes needed to get get the bus format propagated up from the panel. Cheers, -- Guido > > Regards, > Lucas >
Re: [PATCH v1 2/5] drm: mxsfb: Set proper default bus format when using a bridge
On 9/28/21 2:16 PM, Guido Günther wrote: If a bridge doesn't do any bus format handling MEDIA_BUS_FMT_FIXED is returned. Fallback to a reasonable default (MEDIA_BUS_FMT_RGB888_1X24) in that case. This unbreaks e.g. using mxsfb with the nwl bridge and mipi panels. Fixes: b776b0f00f24 ("drm: mxsfb: Use bus_format from the nearest bridge if present") Reported-by: Martin Kepplinger Signed-off-by: Guido Günther --- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/mxsfb/mxsfb_kms.c b/drivers/gpu/drm/mxsfb/mxsfb_kms.c index d6abd2077114..f4be16f5c20b 100644 --- a/drivers/gpu/drm/mxsfb/mxsfb_kms.c +++ b/drivers/gpu/drm/mxsfb/mxsfb_kms.c @@ -369,6 +369,11 @@ static void mxsfb_crtc_atomic_enable(struct drm_crtc *crtc, drm_atomic_get_new_bridge_state(state, mxsfb->bridge); bus_format = bridge_state->input_bus_cfg.format; + if (bus_format == MEDIA_BUS_FMT_FIXED) { + dev_warn_once(drm->dev, + "Bridge does not provide bus format. Please fix."); Fix what ? Oh ... the bridge driver ? Maybe such a hint should be in the warning message. And it might be useful to explain how to fix that bridge driver, or at least provide some reference to an example (like this nwl patch series). + bus_format = MEDIA_BUS_FMT_RGB888_1X24; The warning should also mention that the driver is falling back to this mode.
Re: [PATCH v4 06/10] drm/ingenic: Add dw-hdmi driver for jz4780
On 27/09/2021 18:44, H. Nikolaus Schaller wrote: > From: Paul Boddie > > A specialisation of the generic Synopsys HDMI driver is employed for JZ4780 > HDMI support. This requires a new driver, plus device tree and configuration > modifications. > > Signed-off-by: Paul Boddie > Signed-off-by: Ezequiel Garcia > Signed-off-by: H. Nikolaus Schaller > --- > drivers/gpu/drm/ingenic/Kconfig | 9 ++ > drivers/gpu/drm/ingenic/Makefile | 1 + > drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 142 ++ > 3 files changed, 152 insertions(+) > create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c > > diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig > index 3b57f8be007c..4c7d311fbeff 100644 > --- a/drivers/gpu/drm/ingenic/Kconfig > +++ b/drivers/gpu/drm/ingenic/Kconfig > @@ -25,4 +25,13 @@ config DRM_INGENIC_IPU > > The Image Processing Unit (IPU) will appear as a second primary plane. > > +config DRM_INGENIC_DW_HDMI > + bool "Ingenic specific support for Synopsys DW HDMI" > + depends on MACH_JZ4780 > + select DRM_DW_HDMI > + help > + Choose this option to enable Synopsys DesignWare HDMI based driver. > + If you want to enable HDMI on Ingenic JZ4780 based SoC, you should > + select this option.. > + > endif > diff --git a/drivers/gpu/drm/ingenic/Makefile > b/drivers/gpu/drm/ingenic/Makefile > index d313326bdddb..3db9888a6c04 100644 > --- a/drivers/gpu/drm/ingenic/Makefile > +++ b/drivers/gpu/drm/ingenic/Makefile > @@ -1,3 +1,4 @@ > obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o > ingenic-drm-y = ingenic-drm-drv.o > ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o > +ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o > diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c > b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c > new file mode 100644 > index ..dd9c94ae842e > --- /dev/null > +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c > @@ -0,0 +1,142 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc. > + * Copyright (C) 2019, 2020 Paul Boddie > + * > + * Derived from dw_hdmi-imx.c with i.MX portions removed. > + * Probe and remove operations derived from rcar_dw_hdmi.c. > + */ > + > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = { > + { 4525, { { 0x01e0, 0x }, { 0x21e1, 0x }, { 0x41e2, 0x > } } }, > + { 9250, { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, 0x0005 > } } }, > + { 14850, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, 0x000a > } } }, > + { 21600, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, 0x000f > } } }, > + { ~0UL, { { 0x, 0x }, { 0x, 0x }, { 0x, 0x > } } } > +}; > + > +static const struct dw_hdmi_curr_ctrl ingenic_cur_ctr[] = { > + /*pixelclk bpp8bpp10 bpp12 */ > + { 5400, { 0x091c, 0x091c, 0x06dc } }, > + { 5840, { 0x091c, 0x06dc, 0x06dc } }, > + { 7200, { 0x06dc, 0x06dc, 0x091c } }, > + { 7425, { 0x06dc, 0x0b5c, 0x091c } }, > + { 11880, { 0x091c, 0x091c, 0x06dc } }, > + { 21600, { 0x06dc, 0x0b5c, 0x091c } }, > + { ~0UL, { 0x, 0x, 0x } }, > +}; > + > +/* > + * Resistance term 133Ohm Cfg > + * PREEMP config 0.00 > + * TX/CK level 10 > + */ > +static const struct dw_hdmi_phy_config ingenic_phy_config[] = { > + /*pixelclk symbol term vlev */ > + { 21600, 0x800d, 0x0005, 0x01ad}, > + { ~0UL, 0x, 0x, 0x} > +}; > + > +static enum drm_mode_status > +ingenic_dw_hdmi_mode_valid(struct dw_hdmi *hdmi, void *data, > +const struct drm_display_info *info, > +const struct drm_display_mode *mode) > +{ > + if (mode->clock < 13500) > + return MODE_CLOCK_LOW; > + /* FIXME: Hardware is capable of 270MHz, but setup data is missing. */ > + if (mode->clock > 216000) > + return MODE_CLOCK_HIGH; > + > + return MODE_OK; > +} > + > +static bool > +ingenic_dw_hdmi_mode_fixup(struct drm_bridge *bridge, > +const struct drm_display_mode *mode, > +struct drm_display_mode *adjusted_mode) > +{ > + adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC); > + adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC); > + > + return true; > +} > + > +static const struct drm_bridge_timings ingenic_dw_hdmi_timings = { > + .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE, > +}; These should go in the intermediate encoder bridge callbacks Paul introduces in his patch at [1]. With that patch 4 can be dropped. [1] https://lore.kernel.org/r/2021092220.496871-7-p...@crapouillou.net Neil > + > +static struct dw_hdmi_plat_data ingenic_
[Bug 212425] Kernel warning at drivers/gpu/drm/ttm/ttm_bo.c:517
https://bugzilla.kernel.org/show_bug.cgi?id=212425 Grzegorz Kowal (custos.men...@gmail.com) changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |CODE_FIX -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[PATCH v3 0/3] MSM8953 MDP/DSI PHY enablement
This patch series adds support for the MDP and DSI PHY as found on the MSM8953 platform (APQ8053, SDM450, SDA450, SDM625, SDM626). All the SoCs on this platform use the adreno 506 GPU. Changes since v2: - Changed dt-bindings to use enum instead of oneOf Changes since v1: - Split the dsi phy doc changes into a separate commit - Add Reviewed-by tag to patch 2 Sireesh Kodali (1): dt-bindings: msm: dsi: Add MSM8953 dsi phy Vladimir Lypak (2): drm/msm/dsi: Add phy configuration for MSM8953 drm/msm/mdp5: Add configuration for MDP v1.16 .../bindings/display/msm/dsi-phy-14nm.yaml| 1 + drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 89 +++ drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 + drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 1 + drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c| 21 + 5 files changed, 114 insertions(+) -- 2.33.0
[PATCH v3 1/3] dt-bindings: msm: dsi: Add MSM8953 dsi phy
SoCs based on the MSM8953 platform use the 14nm DSI PHY driver Signed-off-by: Sireesh Kodali --- Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml b/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml index 064df50e21a5..81dbee4803c0 100644 --- a/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml @@ -17,6 +17,7 @@ properties: enum: - qcom,dsi-phy-14nm - qcom,dsi-phy-14nm-660 + - qcom,dsi-phy-14nm-8953 reg: items: -- 2.33.0
[PATCH v3 2/3] drm/msm/dsi: Add phy configuration for MSM8953
From: Vladimir Lypak Add phy configuration for 14nm dsi phy found on MSM8953 SoC. Only difference from existing configurations are io_start addresses. Signed-off-by: Vladimir Lypak Reviewed-by: Dmitry Baryshkov Signed-off-by: Sireesh Kodali --- drivers/gpu/drm/msm/dsi/phy/dsi_phy.c | 2 ++ drivers/gpu/drm/msm/dsi/phy/dsi_phy.h | 1 + drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c | 21 + 3 files changed, 24 insertions(+) diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c index 8c65ef6968ca..9842e04b5858 100644 --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.c @@ -627,6 +627,8 @@ static const struct of_device_id dsi_phy_dt_match[] = { .data = &dsi_phy_14nm_cfgs }, { .compatible = "qcom,dsi-phy-14nm-660", .data = &dsi_phy_14nm_660_cfgs }, + { .compatible = "qcom,dsi-phy-14nm-8953", + .data = &dsi_phy_14nm_8953_cfgs }, #endif #ifdef CONFIG_DRM_MSM_DSI_10NM_PHY { .compatible = "qcom,dsi-phy-10nm", diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h index b91303ada74f..4c8257581bfc 100644 --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy.h @@ -48,6 +48,7 @@ extern const struct msm_dsi_phy_cfg dsi_phy_20nm_cfgs; extern const struct msm_dsi_phy_cfg dsi_phy_28nm_8960_cfgs; extern const struct msm_dsi_phy_cfg dsi_phy_14nm_cfgs; extern const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs; +extern const struct msm_dsi_phy_cfg dsi_phy_14nm_8953_cfgs; extern const struct msm_dsi_phy_cfg dsi_phy_10nm_cfgs; extern const struct msm_dsi_phy_cfg dsi_phy_10nm_8998_cfgs; extern const struct msm_dsi_phy_cfg dsi_phy_7nm_cfgs; diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c index d13552b2213b..9a6b1f0cbbaf 100644 --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_14nm.c @@ -1065,3 +1065,24 @@ const struct msm_dsi_phy_cfg dsi_phy_14nm_660_cfgs = { .io_start = { 0xc994400, 0xc996000 }, .num_dsi_phy = 2, }; + +const struct msm_dsi_phy_cfg dsi_phy_14nm_8953_cfgs = { + .has_phy_lane = true, + .reg_cfg = { + .num = 1, + .regs = { + {"vcca", 17000, 32}, + }, + }, + .ops = { + .enable = dsi_14nm_phy_enable, + .disable = dsi_14nm_phy_disable, + .pll_init = dsi_pll_14nm_init, + .save_pll_state = dsi_14nm_pll_save_state, + .restore_pll_state = dsi_14nm_pll_restore_state, + }, + .min_pll_rate = VCO_MIN_RATE, + .max_pll_rate = VCO_MAX_RATE, + .io_start = { 0x1a94400, 0x1a96400 }, + .num_dsi_phy = 2, +}; -- 2.33.0
[PATCH v3 3/3] drm/msm/mdp5: Add configuration for MDP v1.16
From: Vladimir Lypak MDP version v1.16 is almost identical to v1.15 with most significant difference being presence of second DSI interface. MDP v1.16 is found on SoCs such as MSM8x53, SDM450, SDM632 (All with Adreno 506). Signed-off-by: Vladimir Lypak Signed-off-by: Sireesh Kodali --- drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c | 89 1 file changed, 89 insertions(+) diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c index 9741544ffc35..0d28c8ff4009 100644 --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cfg.c @@ -752,6 +752,94 @@ const struct mdp5_cfg_hw msm8x76_config = { .max_clk = 36000, }; +static const struct mdp5_cfg_hw msm8x53_config = { + .name = "msm8x53", + .mdp = { + .count = 1, + .caps = MDP_CAP_CDM | + MDP_CAP_SRC_SPLIT, + }, + .ctl = { + .count = 3, + .base = { 0x01000, 0x01200, 0x01400 }, + .flush_hw_mask = 0x, + }, + .pipe_vig = { + .count = 1, + .base = { 0x04000 }, + .caps = MDP_PIPE_CAP_HFLIP | + MDP_PIPE_CAP_VFLIP | + MDP_PIPE_CAP_SCALE | + MDP_PIPE_CAP_CSC| + MDP_PIPE_CAP_DECIMATION | + MDP_PIPE_CAP_SW_PIX_EXT | + 0, + }, + .pipe_rgb = { + .count = 2, + .base = { 0x14000, 0x16000 }, + .caps = MDP_PIPE_CAP_HFLIP | + MDP_PIPE_CAP_VFLIP | + MDP_PIPE_CAP_DECIMATION | + MDP_PIPE_CAP_SW_PIX_EXT | + 0, + }, + .pipe_dma = { + .count = 1, + .base = { 0x24000 }, + .caps = MDP_PIPE_CAP_HFLIP | + MDP_PIPE_CAP_VFLIP | + MDP_PIPE_CAP_SW_PIX_EXT | + 0, + }, + .pipe_cursor = { + .count = 1, + .base = { 0x34000 }, + .caps = MDP_PIPE_CAP_HFLIP | + MDP_PIPE_CAP_VFLIP | + MDP_PIPE_CAP_SW_PIX_EXT | + MDP_PIPE_CAP_CURSOR | + 0, + }, + + .lm = { + .count = 3, + .base = { 0x44000, 0x45000 }, + .instances = { + { .id = 0, .pp = 0, .dspp = 0, + .caps = MDP_LM_CAP_DISPLAY | + MDP_LM_CAP_PAIR }, + { .id = 1, .pp = 1, .dspp = -1, + .caps = MDP_LM_CAP_DISPLAY }, +}, + .nb_stages = 5, + .max_width = 2048, + .max_height = 0x, + }, + .dspp = { + .count = 1, + .base = { 0x54000 }, + + }, + .pp = { + .count = 2, + .base = { 0x7, 0x70800 }, + }, + .cdm = { + .count = 1, + .base = { 0x79200 }, + }, + .intf = { + .base = { 0x6a000, 0x6a800, 0x6b000 }, + .connect = { + [0] = INTF_DISABLED, + [1] = INTF_DSI, + [2] = INTF_DSI, + }, + }, + .max_clk = 4, +}; + static const struct mdp5_cfg_hw msm8917_config = { .name = "msm8917", .mdp = { @@ -1151,6 +1239,7 @@ static const struct mdp5_cfg_handler cfg_handlers_v1[] = { { .revision = 7, .config = { .hw = &msm8x96_config } }, { .revision = 11, .config = { .hw = &msm8x76_config } }, { .revision = 15, .config = { .hw = &msm8917_config } }, + { .revision = 16, .config = { .hw = &msm8x53_config } }, }; static const struct mdp5_cfg_handler cfg_handlers_v3[] = { -- 2.33.0
Re: [PATCH 2/2] [v2] qcom_scm: hide Kconfig symbol
On 9/28/21 2:50 AM, Arnd Bergmann wrote: From: Arnd Bergmann Now that SCM can be a loadable module, we have to add another dependency to avoid link failures when ipa or adreno-gpu are built-in: aarch64-linux-ld: drivers/net/ipa/ipa_main.o: in function `ipa_probe': ipa_main.c:(.text+0xfc4): undefined reference to `qcom_scm_is_available' ld.lld: error: undefined symbol: qcom_scm_is_available referenced by adreno_gpu.c gpu/drm/msm/adreno/adreno_gpu.o:(adreno_zap_shader_load) in archive drivers/built-in.a This can happen when CONFIG_ARCH_QCOM is disabled and we don't select QCOM_MDT_LOADER, but some other module selects QCOM_SCM. Ideally we'd use a similar dependency here to what we have for QCOM_RPROC_COMMON, but that causes dependency loops from other things selecting QCOM_SCM. This appears to be an endless problem, so try something different this time: - CONFIG_QCOM_SCM becomes a hidden symbol that nothing 'depends on' but that is simply selected by all of its users - All the stubs in include/linux/qcom_scm.h can go away - arm-smccc.h needs to provide a stub for __arm_smccc_smc() to allow compile-testing QCOM_SCM on all architectures. - To avoid a circular dependency chain involving RESET_CONTROLLER and PINCTRL_SUNXI, drop the 'select RESET_CONTROLLER' statement. According to my testing this still builds fine, and the QCOM platform selects this symbol already. Acked-by: Kalle Valo Signed-off-by: Arnd Bergmann --- Changes in v2: - drop the 'select RESET_CONTROLLER' line, rather than adding more of the same --- drivers/firmware/Kconfig| 5 +- drivers/gpu/drm/msm/Kconfig | 4 +- drivers/iommu/Kconfig | 2 +- drivers/media/platform/Kconfig | 2 +- drivers/mmc/host/Kconfig| 2 +- drivers/net/ipa/Kconfig | 1 + For drivers/net/ipa/Kconfig, looks good to me. Nice simplification. Acked-by: Alex Elder drivers/net/wireless/ath/ath10k/Kconfig | 2 +- drivers/pinctrl/qcom/Kconfig| 3 +- include/linux/arm-smccc.h | 10 include/linux/qcom_scm.h| 71 - 10 files changed, 20 insertions(+), 82 deletions(-) . . .
Re: [PATCH v4 06/10] drm/ingenic: Add dw-hdmi driver for jz4780
Hi Neil, > Am 28.09.2021 um 15:02 schrieb Neil Armstrong : > > On 27/09/2021 18:44, H. Nikolaus Schaller wrote: >> From: Paul Boddie >> >> A specialisation of the generic Synopsys HDMI driver is employed for JZ4780 >> HDMI support. This requires a new driver, plus device tree and configuration >> modifications. >> >> Signed-off-by: Paul Boddie >> Signed-off-by: Ezequiel Garcia >> Signed-off-by: H. Nikolaus Schaller >> --- >> drivers/gpu/drm/ingenic/Kconfig | 9 ++ >> drivers/gpu/drm/ingenic/Makefile | 1 + >> drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 142 ++ >> 3 files changed, 152 insertions(+) >> create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c >> >> diff --git a/drivers/gpu/drm/ingenic/Kconfig >> b/drivers/gpu/drm/ingenic/Kconfig >> index 3b57f8be007c..4c7d311fbeff 100644 >> --- a/drivers/gpu/drm/ingenic/Kconfig >> +++ b/drivers/gpu/drm/ingenic/Kconfig >> @@ -25,4 +25,13 @@ config DRM_INGENIC_IPU >> >>The Image Processing Unit (IPU) will appear as a second primary plane. >> >> +config DRM_INGENIC_DW_HDMI >> +bool "Ingenic specific support for Synopsys DW HDMI" >> +depends on MACH_JZ4780 >> +select DRM_DW_HDMI >> +help >> + Choose this option to enable Synopsys DesignWare HDMI based driver. >> + If you want to enable HDMI on Ingenic JZ4780 based SoC, you should >> + select this option.. >> + >> endif >> diff --git a/drivers/gpu/drm/ingenic/Makefile >> b/drivers/gpu/drm/ingenic/Makefile >> index d313326bdddb..3db9888a6c04 100644 >> --- a/drivers/gpu/drm/ingenic/Makefile >> +++ b/drivers/gpu/drm/ingenic/Makefile >> @@ -1,3 +1,4 @@ >> obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o >> ingenic-drm-y = ingenic-drm-drv.o >> ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o >> +ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o >> diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c >> b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c >> new file mode 100644 >> index ..dd9c94ae842e >> --- /dev/null >> +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c >> @@ -0,0 +1,142 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc. >> + * Copyright (C) 2019, 2020 Paul Boddie >> + * >> + * Derived from dw_hdmi-imx.c with i.MX portions removed. >> + * Probe and remove operations derived from rcar_dw_hdmi.c. >> + */ >> + >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> + >> +static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = { >> +{ 4525, { { 0x01e0, 0x }, { 0x21e1, 0x }, { 0x41e2, 0x >> } } }, >> +{ 9250, { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, 0x0005 >> } } }, >> +{ 14850, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, 0x000a >> } } }, >> +{ 21600, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, 0x000f >> } } }, >> +{ ~0UL, { { 0x, 0x }, { 0x, 0x }, { 0x, 0x >> } } } >> +}; >> + >> +static const struct dw_hdmi_curr_ctrl ingenic_cur_ctr[] = { >> +/*pixelclk bpp8bpp10 bpp12 */ >> +{ 5400, { 0x091c, 0x091c, 0x06dc } }, >> +{ 5840, { 0x091c, 0x06dc, 0x06dc } }, >> +{ 7200, { 0x06dc, 0x06dc, 0x091c } }, >> +{ 7425, { 0x06dc, 0x0b5c, 0x091c } }, >> +{ 11880, { 0x091c, 0x091c, 0x06dc } }, >> +{ 21600, { 0x06dc, 0x0b5c, 0x091c } }, >> +{ ~0UL, { 0x, 0x, 0x } }, >> +}; >> + >> +/* >> + * Resistance term 133Ohm Cfg >> + * PREEMP config 0.00 >> + * TX/CK level 10 >> + */ >> +static const struct dw_hdmi_phy_config ingenic_phy_config[] = { >> +/*pixelclk symbol term vlev */ >> +{ 21600, 0x800d, 0x0005, 0x01ad}, >> +{ ~0UL, 0x, 0x, 0x} >> +}; >> + >> +static enum drm_mode_status >> +ingenic_dw_hdmi_mode_valid(struct dw_hdmi *hdmi, void *data, >> + const struct drm_display_info *info, >> + const struct drm_display_mode *mode) >> +{ >> +if (mode->clock < 13500) >> +return MODE_CLOCK_LOW; >> +/* FIXME: Hardware is capable of 270MHz, but setup data is missing. */ >> +if (mode->clock > 216000) >> +return MODE_CLOCK_HIGH; >> + >> +return MODE_OK; >> +} >> + >> +static bool >> +ingenic_dw_hdmi_mode_fixup(struct drm_bridge *bridge, >> + const struct drm_display_mode *mode, >> + struct drm_display_mode *adjusted_mode) >> +{ >> +adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC); >> +adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC); >> + >> +return true; >> +} >> + >> +static const struct drm_bridge_timings ingenic_dw_hdmi_timings = { >> +.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE, >> +}; > > These should go in the intermediate encoder bridge callbacks Paul introduces > in his patch at [1]. > > Wit
Re: refactor the i915 GVT support
On Tue, Sep 28, 2021 at 07:41:00AM +, Wang, Zhi A wrote: > Hey guys: > > After some investigation, I found the root cause this problem ("i915" > module loading will be stuck with Christoph's refactor patches), which > can be reproduced by building both i915 and kvmgt as kernel module and > the loading i915. Thanks for looking into this! > The root cause is: in Linux kernel loading, before a kernel module > loading is finished, its symbols can not be reached by other module when > resolving the symbols (even they can be found in /proc/kallsyms). > Because the status of the kernel module is MODULE_STATE_COMING and > resolve_symbol() from another kernel module will check this and return a > -EBUSY. Well, it would seem that way but... > In this case, before i915 loading is finished, the requested module > "kvmgt" cannot reach the symbols in module i915. Thus it kept waiting > and left message like below in the dmesg: > > [ 644.152021] kvmgt: gave up waiting for init of module i915. > [ 644.152039] kvmgt: Unknown symbol i915_gem_object_set_to_cpu_domain > (err -16) > [ 674.871409] kvmgt: gave up waiting for init of module i915. > [ 674.871427] kvmgt: Unknown symbol intel_ring_begin (err -16) > [ 705.590586] kvmgt: gave up waiting for init of module i915. > [ 705.590604] kvmgt: Unknown symbol i915_vma_move_to_active (err -16) > [ 736.310230] kvmgt: gave up waiting for init of module i915. > [ 736.310248] kvmgt: Unknown symbol shmem_unpin_map (err -16) > ... > > The error message is from execution path below: > > kernel/module.c: > > [i915 module loading] -> > request_module("kvmgt")->[modprobe]->init_module("kvmgt")->load_module()->simplify_symbols()->resolve_symbol_wait(): > > static const struct kernel_symbol * > resolve_symbol_wait(struct module *mod, > const struct load_info *info, > const char *name) > { > const struct kernel_symbol *ksym; > char owner[MODULE_NAME_LEN]; > > if (wait_event_interruptible_timeout(module_wq, > !IS_ERR(ksym = resolve_symbol(mod, info, name, owner)) > || PTR_ERR(ksym) != -EBUSY, > 30 * HZ) <= 0) { > pr_warn("%s: gave up waiting for init of module %s.\n", > mod->name, owner); > > } Commit 9bea7f23952d5 ("module: fix bne2 "gave up waiting for init of module libcrc32c") is worth reviewing. It dealt with a similar issue, and in particular it addressed the issue with -EBUSY being returned by ref_module(). And so, in theory that case should be dealt with in resolve_symbol_wait() already. And so can you try this just to verify something: diff --git a/kernel/module.c b/kernel/module.c index 40ec9a030eec..98f87cbb37de 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1459,7 +1459,7 @@ resolve_symbol_wait(struct module *mod, if (wait_event_interruptible_timeout(module_wq, !IS_ERR(ksym = resolve_symbol(mod, info, name, owner)) || PTR_ERR(ksym) != -EBUSY, -30 * HZ) <= 0) { +160 * HZ) <= 0) { pr_warn("%s: gave up waiting for init of module %s.\n", mod->name, owner); } > code: > https://github.com/intel/gvt-linux/blob/bd950a66c7919d7121d2530f30984351534a96dc/kernel/module.c#L1452 > > In resolve_symbol_wait(), it calls resolve_symbol() to resolve the > symbols in "i915". In resolve_symbol() -> ref_module() -> > strong_try_module_get(), it will check the status of the module which > owns the symbol. > > static inline int strong_try_module_get(struct module *mod) > { > BUG_ON(mod && mod->state == MODULE_STATE_UNFORMED); > if (mod && mod->state == MODULE_STATE_COMING) > return -EBUSY; > if (try_module_get(mod)) > return 0; > else > return -ENOENT; > } > > code:https://github.com/intel/gvt-linux/blob/bd950a66c7919d7121d2530f30984351534a96dc/kernel/module.c#L318 > > But unfortunately, this execution path begins in i915 module loading, at > this time, the status of kernel module "i915" is MODULE_STATE_COMING > until loading of "kvmgt" is finished. Thus a -EBUSY is always returned > when kernel is trying to resolve symbols for "kvmgt". > > > This patch below might need re-work: If the above test patch still fails, well.. that might be telling of another issue which is perhaps difficult to see at first glance. If resolve_symbol_wait() won't succeed until request_module("kvmgt") completes and if this means having kvmgt's init routine complete, that could end up in some longer chain or in the worst case a sort of circular dependency which is only implicated by module loading. It'd be really odd... but I cannot rule it out. This is one reason I hinted that you should strive to not do much on a module's init. If you can punt work off for later that's best. Luis > > Author: Christoph Hellwig > Da
Re: [PATCH v4 06/10] drm/ingenic: Add dw-hdmi driver for jz4780
Hi Nikolaus & Paul, Le lun., sept. 27 2021 at 18:44:24 +0200, H. Nikolaus Schaller a écrit : From: Paul Boddie A specialisation of the generic Synopsys HDMI driver is employed for JZ4780 HDMI support. This requires a new driver, plus device tree and configuration modifications. Signed-off-by: Paul Boddie Signed-off-by: Ezequiel Garcia Signed-off-by: H. Nikolaus Schaller --- drivers/gpu/drm/ingenic/Kconfig | 9 ++ drivers/gpu/drm/ingenic/Makefile | 1 + drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 142 ++ 3 files changed, 152 insertions(+) create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig index 3b57f8be007c..4c7d311fbeff 100644 --- a/drivers/gpu/drm/ingenic/Kconfig +++ b/drivers/gpu/drm/ingenic/Kconfig @@ -25,4 +25,13 @@ config DRM_INGENIC_IPU The Image Processing Unit (IPU) will appear as a second primary plane. +config DRM_INGENIC_DW_HDMI + bool "Ingenic specific support for Synopsys DW HDMI" + depends on MACH_JZ4780 + select DRM_DW_HDMI + help + Choose this option to enable Synopsys DesignWare HDMI based driver. + If you want to enable HDMI on Ingenic JZ4780 based SoC, you should + select this option.. + endif diff --git a/drivers/gpu/drm/ingenic/Makefile b/drivers/gpu/drm/ingenic/Makefile index d313326bdddb..3db9888a6c04 100644 --- a/drivers/gpu/drm/ingenic/Makefile +++ b/drivers/gpu/drm/ingenic/Makefile @@ -1,3 +1,4 @@ obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o ingenic-drm-y = ingenic-drm-drv.o ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o +ingenic-drm-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c new file mode 100644 index ..dd9c94ae842e --- /dev/null +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c @@ -0,0 +1,142 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc. + * Copyright (C) 2019, 2020 Paul Boddie + * + * Derived from dw_hdmi-imx.c with i.MX portions removed. + * Probe and remove operations derived from rcar_dw_hdmi.c. + */ + +#include +#include +#include + +#include +#include +#include + +static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = { + { 4525, { { 0x01e0, 0x }, { 0x21e1, 0x }, { 0x41e2, 0x } } }, + { 9250, { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, 0x0005 } } }, + { 14850, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, 0x000a } } }, + { 21600, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, 0x000f } } }, + { ~0UL, { { 0x, 0x }, { 0x, 0x }, { 0x, 0x } } } +}; + +static const struct dw_hdmi_curr_ctrl ingenic_cur_ctr[] = { + /*pixelclk bpp8bpp10 bpp12 */ + { 5400, { 0x091c, 0x091c, 0x06dc } }, + { 5840, { 0x091c, 0x06dc, 0x06dc } }, + { 7200, { 0x06dc, 0x06dc, 0x091c } }, + { 7425, { 0x06dc, 0x0b5c, 0x091c } }, + { 11880, { 0x091c, 0x091c, 0x06dc } }, + { 21600, { 0x06dc, 0x0b5c, 0x091c } }, + { ~0UL, { 0x, 0x, 0x } }, +}; + +/* + * Resistance term 133Ohm Cfg + * PREEMP config 0.00 + * TX/CK level 10 + */ +static const struct dw_hdmi_phy_config ingenic_phy_config[] = { + /*pixelclk symbol term vlev */ + { 21600, 0x800d, 0x0005, 0x01ad}, + { ~0UL, 0x, 0x, 0x} +}; + +static enum drm_mode_status +ingenic_dw_hdmi_mode_valid(struct dw_hdmi *hdmi, void *data, + const struct drm_display_info *info, + const struct drm_display_mode *mode) +{ + if (mode->clock < 13500) + return MODE_CLOCK_LOW; + /* FIXME: Hardware is capable of 270MHz, but setup data is missing. */ + if (mode->clock > 216000) + return MODE_CLOCK_HIGH; + + return MODE_OK; +} + +static bool +ingenic_dw_hdmi_mode_fixup(struct drm_bridge *bridge, + const struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode) +{ + adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC); + adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC); Why do these flags need to be cleared? The LCDC can work with negative v/h sync, does the HDMI core not work with that? + + return true; +} + +static const struct drm_bridge_timings ingenic_dw_hdmi_timings = { + .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE, That info should be provided in the EDID, is this really needed here? Cheers, -Paul +}; + +static struct dw_hdmi_plat_data ingenic_dw_hdmi_plat_data = { + .mpll_cfg = ingenic_mpll_cfg, + .cur_ctr= ingenic_cur_ctr, + .phy_config = ingenic_phy_config, + .mode_valid = ingenic_dw_hdmi_mode_
Re: [PATCH] drm/msm: Switch ordering of runpm put vs devfreq_idle
On 9/27/2021 8:59 PM, Rob Clark wrote: From: Rob Clark I've seen a few crashes like: Internal error: synchronous external abort: 9610 [#1] PREEMPT SMP Modules linked in: snd_seq_dummy snd_seq snd_seq_device bridge stp llc tun nf_nat_tftp nf_conntrack_tftp nf_nat_ftp nf_conntrack_ftp esp6 ah6 ip6t_REJECT ip6t_ipv6header vhost_vsock vhost vmw_vsock_virtio_transport_common vsock rfcomm algif_hash algif_skcipher af_alg uinput veth xt_cgroup xt_MASQUERADE venus_enc venus_dec videobuf2_dma_contig qcom_spmi_adc5 qcom_spmi_adc_tm5 hci_uart qcom_vadc_common cros_ec_typec qcom_spmi_temp_alarm typec btqca snd_soc_rt5682_i2c snd_soc_rt5682 snd_soc_sc7180 bluetooth snd_soc_qcom_common snd_soc_rl6231 ecdh_generic ecc venus_core v4l2_mem2mem snd_soc_lpass_sc7180 snd_soc_lpass_hdmi snd_soc_lpass_cpu snd_soc_lpass_platform snd_soc_max98357a ip6table_nat fuse iio_trig_sysfs cros_ec_lid_angle cros_ec_sensors cros_ec_sensors_core industrialio_triggered_buffer kfifo_buf cros_ec_sensorhub lzo_rle ath10k_snoc lzo_compress ath10k_core ath zram mac80211 cfg80211 ax88179_178a usbnet mii uvcvideo videobuf2_vmalloc joydev CPU: 3 PID: 212 Comm: A618-worker Tainted: G W 5.4.139-16300-g88d8e1285982 #1 Hardware name: Google Pompom (rev1) with LTE (DT) pstate: 60c9 (nZCv daif +PAN +UAO) pc : a6xx_gmu_set_oob+0x114/0x200 lr : a6xx_gmu_set_oob+0x10c/0x200 sp : ffc011b7bc20 x29: ffc011b7bc20 x28: ffdad27c5000 x27: 0001 x26: ffdad1521044 x25: ffbef7498338 x24: 0018 x23: 0002 x22: 00014648 x21: 033732fe638b x20: 8000 x19: ffbef7433bc8 x18: 4000 x17: 00243508d982 x16: b67e x15: 90d4 x14: 0024 x13: 0024 x12: 00017521 x11: 0b48 x10: 00326a48 x9 : 1a130d33f6371600 x8 : ffc011e54648 x7 : 614948e5003c x6 : ffbe3cd17e60 x5 : 0040 x4 : 0004 x3 : x2 : ffbef7488000 x1 : ffbef7488000 x0 : Call trace: a6xx_gmu_set_oob+0x114/0x200 a6xx_gmu_set_freq+0xe0/0x1fc msm_devfreq_target+0x80/0x13c msm_devfreq_idle+0x54/0x94 retire_submit+0x170/0x254 retire_submits+0xa4/0xdc retire_worker+0x1c/0x28 kthread_worker_fn+0xf4/0x1bc kthread+0x140/0x158 ret_from_fork+0x10/0x18 Code: 52800c81 9415bbe5 f9400a68 8b160108 (b9400108) ---[ end trace 16b871df2482cd61 ]--- Kernel panic - not syncing: Fatal exception SMP: stopping secondary CPUs Kernel Offset: 0x1ac140 from 0xffc01000 PHYS_OFFSET: 0xffc28000 CPU features: 0x88102e,2a80aa38 Memory Limit: none Which smells a lot like touching hw after power collapse. I'm not *entirely* sure how it could have taken 66ms (the autosuspend delay) before we get to a6xx_gmu_set_oob(), but to be safe we should move the pm_runtime_put_autosuspend() after msm_devfreq_idle(). https://elixir.bootlin.com/linux/v5.15-rc1/source/drivers/gpu/drm/msm/adreno/a6xx_gmu.c#L132 We have this check in the gmu freq set path which should avoid this scenario. I might be a bit pedantic here, but I feel that the original code is more accurate. We should immediately mark last busy and put runtime_pm refcount. -Akhil. Fixes: 9bc95570175a ("drm/msm: Devfreq tuning") Signed-off-by: Rob Clark --- drivers/gpu/drm/msm/msm_gpu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index d1a16642ecd5..2b2bbe7499e6 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -667,9 +667,6 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring, msm_submit_retire(submit); - pm_runtime_mark_last_busy(&gpu->pdev->dev); - pm_runtime_put_autosuspend(&gpu->pdev->dev); - spin_lock_irqsave(&ring->submit_lock, flags); list_del(&submit->node); spin_unlock_irqrestore(&ring->submit_lock, flags); @@ -683,6 +680,9 @@ static void retire_submit(struct msm_gpu *gpu, struct msm_ringbuffer *ring, mutex_unlock(&gpu->active_lock); msm_gem_submit_put(submit); + + pm_runtime_mark_last_busy(&gpu->pdev->dev); + pm_runtime_put_autosuspend(&gpu->pdev->dev); } static void retire_submits(struct msm_gpu *gpu)
[PATCH] [SUBMITTED 20210721] fbdev: simplefb: fix Kconfig dependencies
From: Arnd Bergmann Configurations with both CONFIG_FB_SIMPLE=y and CONFIG_DRM_SIMPLEDRM=m are allowed by Kconfig because the 'depends on !DRM_SIMPLEDRM' dependency does not disallow FB_SIMPLE as long as SIMPLEDRM is not built-in. This can however result in a build failure when cfb_fillrect() etc are then also in loadable modules: x86_64-linux-ld: drivers/video/fbdev/simplefb.o:(.rodata+0x1f8): undefined reference to `cfb_fillrect' x86_64-linux-ld: drivers/video/fbdev/simplefb.o:(.rodata+0x200): undefined reference to `cfb_copyarea' x86_64-linux-ld: drivers/video/fbdev/simplefb.o:(.rodata+0x208): undefined reference to `cfb_imageblit' To work around this, change FB_SIMPLE to be a 'tristate' symbol, which still allows both to be =m together, but not one of them to be =y if the other one is =m. If a distro kernel picks this configuration, it can be determined by local policy which of the two modules gets loaded. The 'of_chosen' export is needed as this is the first loadable module referencing it. Alternatively, the Kconfig dependency could be changed to 'depends on DRM_SIMPLEDRM=n', which would forbid the configuration with both drivers. Fixes: 11e8f5fd223b ("drm: Add simpledrm driver") Acked-by: Rob Herring # for drivers/of/ Link: https://lore.kernel.org/all/20210721151839.2484245-1-a...@kernel.org/ Signed-off-by: Arnd Bergmann --- drivers/of/base.c | 1 + drivers/video/fbdev/Kconfig | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/of/base.c b/drivers/of/base.c index f720c0d246f2..0ac17256258d 100644 --- a/drivers/of/base.c +++ b/drivers/of/base.c @@ -36,6 +36,7 @@ LIST_HEAD(aliases_lookup); struct device_node *of_root; EXPORT_SYMBOL(of_root); struct device_node *of_chosen; +EXPORT_SYMBOL(of_chosen); struct device_node *of_aliases; struct device_node *of_stdout; static const char *of_stdout_options; diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index b26b79dfcac9..6ed5e608dd04 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -2193,8 +2193,9 @@ config FB_HYPERV This framebuffer driver supports Microsoft Hyper-V Synthetic Video. config FB_SIMPLE - bool "Simple framebuffer support" - depends on (FB = y) && !DRM_SIMPLEDRM + tristate "Simple framebuffer support" + depends on FB + depends on !DRM_SIMPLEDRM select FB_CFB_FILLRECT select FB_CFB_COPYAREA select FB_CFB_IMAGEBLIT -- 2.29.2
Re: refactor the i915 GVT support
On 9/28/21 2:00 PM, Luis Chamberlain wrote: > On Tue, Sep 28, 2021 at 07:41:00AM +, Wang, Zhi A wrote: >> Hey guys: >> >> After some investigation, I found the root cause this problem ("i915" >> module loading will be stuck with Christoph's refactor patches), which >> can be reproduced by building both i915 and kvmgt as kernel module and >> the loading i915. > Thanks for looking into this! > >> The root cause is: in Linux kernel loading, before a kernel module >> loading is finished, its symbols can not be reached by other module when >> resolving the symbols (even they can be found in /proc/kallsyms). >> Because the status of the kernel module is MODULE_STATE_COMING and >> resolve_symbol() from another kernel module will check this and return a >> -EBUSY. > Well, it would seem that way but... > >> In this case, before i915 loading is finished, the requested module >> "kvmgt" cannot reach the symbols in module i915. Thus it kept waiting >> and left message like below in the dmesg: >> >> [ 644.152021] kvmgt: gave up waiting for init of module i915. >> [ 644.152039] kvmgt: Unknown symbol i915_gem_object_set_to_cpu_domain >> (err -16) >> [ 674.871409] kvmgt: gave up waiting for init of module i915. >> [ 674.871427] kvmgt: Unknown symbol intel_ring_begin (err -16) >> [ 705.590586] kvmgt: gave up waiting for init of module i915. >> [ 705.590604] kvmgt: Unknown symbol i915_vma_move_to_active (err -16) >> [ 736.310230] kvmgt: gave up waiting for init of module i915. >> [ 736.310248] kvmgt: Unknown symbol shmem_unpin_map (err -16) >> ... >> >> The error message is from execution path below: >> >> kernel/module.c: >> >> [i915 module loading] -> >> request_module("kvmgt")->[modprobe]->init_module("kvmgt")->load_module()->simplify_symbols()->resolve_symbol_wait(): >> >> static const struct kernel_symbol * >> resolve_symbol_wait(struct module *mod, >> const struct load_info *info, >> const char *name) >> { >> const struct kernel_symbol *ksym; >> char owner[MODULE_NAME_LEN]; >> >> if (wait_event_interruptible_timeout(module_wq, >> !IS_ERR(ksym = resolve_symbol(mod, info, name, owner)) >> || PTR_ERR(ksym) != -EBUSY, >> 30 * HZ) <= 0) { >> pr_warn("%s: gave up waiting for init of module %s.\n", >> mod->name, owner); >> >> } > Commit 9bea7f23952d5 ("module: fix bne2 "gave up waiting for init of > module libcrc32c") is worth reviewing. It dealt with a similar issue, > and in particular it addressed the issue with -EBUSY being returned > by ref_module(). > > And so, in theory that case should be dealt with in resolve_symbol_wait() > already. And so can you try this just to verify something: > > diff --git a/kernel/module.c b/kernel/module.c > index 40ec9a030eec..98f87cbb37de 100644 > --- a/kernel/module.c > +++ b/kernel/module.c > @@ -1459,7 +1459,7 @@ resolve_symbol_wait(struct module *mod, > if (wait_event_interruptible_timeout(module_wq, > !IS_ERR(ksym = resolve_symbol(mod, info, name, owner)) > || PTR_ERR(ksym) != -EBUSY, > - 30 * HZ) <= 0) { > + 160 * HZ) <= 0) { > pr_warn("%s: gave up waiting for init of module %s.\n", > mod->name, owner); > } > Hi Luis: Thanks so much for the reply and patch.:) I am afraid that this patch wouldn't work in this case as the request_module("kvmgt") happens in the init_module of i915. Before the initialization path is finished in i915, the i915 symbols are not available to be referenced. Unfortunately, It's matter of sequence, not of delay. :( >> code: >> https://github.com/intel/gvt-linux/blob/bd950a66c7919d7121d2530f30984351534a96dc/kernel/module.c#L1452 >> >> In resolve_symbol_wait(), it calls resolve_symbol() to resolve the >> symbols in "i915". In resolve_symbol() -> ref_module() -> >> strong_try_module_get(), it will check the status of the module which >> owns the symbol. >> >> static inline int strong_try_module_get(struct module *mod) >> { >> BUG_ON(mod && mod->state == MODULE_STATE_UNFORMED); >> if (mod && mod->state == MODULE_STATE_COMING) >> return -EBUSY; >> if (try_module_get(mod)) >> return 0; >> else >> return -ENOENT; >> } >> >> code:https://github.com/intel/gvt-linux/blob/bd950a66c7919d7121d2530f30984351534a96dc/kernel/module.c#L318 >> >> But unfortunately, this execution path begins in i915 module loading, at >> this time, the status of kernel module "i915" is MODULE_STATE_COMING >> until loading of "kvmgt" is finished. Thus a -EBUSY is always returned >> when kernel is trying to resolve symbols for "kvmgt". >> >> >> This patch below might need re-work: > If the above test patch still fails, well.. that might be telling of > another issue which is perhaps difficult to see at first glance. If > resolve_sy
Re: refactor the i915 GVT support
On Tue, Sep 28, 2021 at 02:35:06PM +, Wang, Zhi A wrote: > Yes. I was thinking of the possibility of putting off some work later so > that we don't need to make a lot of changes. GVT-g needs to take a > snapshot of GPU registers as the initial virtual states for other vGPUs, > which requires the initialization happens at a certain early time of > initialization of i915. I was thinking maybe we can take other patches > from Christoph like "de-virtualize*" except this one because currently > we have to maintain a TEST-ONLY patch on our tree to prevent i915 built > as kernel module. How about just capture these registers in the main module/device and not try so hard to isolate it to the gvt stuff? Jason
Re: [PATCH v3] drm/dp: Add Additional DP2 Headers
On 2021-09-27 15:23, Fangzhi Zuo wrote: > Include FEC, DSC, Link Training related headers. > > Change since v2 > - Align with the spec for DP_DSC_SUPPORT_AND_DSC_DECODER_COUNT > > Signed-off-by: Fangzhi Zuo Reviewed-by: Harry Wentland Harry > --- > This patch is based on top of the other DP2.0 work in > "drm/dp: add LTTPR DP 2.0 DPCD addresses" > --- > include/drm/drm_dp_helper.h | 20 > 1 file changed, 20 insertions(+) > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > index 1d5b3dbb6e56..a1df35aa6e68 100644 > --- a/include/drm/drm_dp_helper.h > +++ b/include/drm/drm_dp_helper.h > @@ -453,6 +453,7 @@ struct drm_panel; > # define DP_FEC_UNCORR_BLK_ERROR_COUNT_CAP (1 << 1) > # define DP_FEC_CORR_BLK_ERROR_COUNT_CAP(1 << 2) > # define DP_FEC_BIT_ERROR_COUNT_CAP (1 << 3) > +#define DP_FEC_CAPABILITY_1 0x091 /* 2.0 */ > > /* DP-HDMI2.1 PCON DSC ENCODER SUPPORT */ > #define DP_PCON_DSC_ENCODER_CAP_SIZE0xC /* 0x9E - 0x92 */ > @@ -537,6 +538,9 @@ struct drm_panel; > #define DP_DSC_BRANCH_OVERALL_THROUGHPUT_1 0x0a1 > #define DP_DSC_BRANCH_MAX_LINE_WIDTH0x0a2 > > +/* DFP Capability Extension */ > +#define DP_DFP_CAPABILITY_EXTENSION_SUPPORT 0x0a3 /* 2.0 */ > + > /* Link Configuration */ > #define DP_LINK_BW_SET 0x100 > # define DP_LINK_RATE_TABLE 0x00/* eDP 1.4 */ > @@ -688,6 +692,7 @@ struct drm_panel; > > #define DP_DSC_ENABLE 0x160 /* DP 1.4 */ > # define DP_DECOMPRESSION_EN(1 << 0) > +#define DP_DSC_CONFIGURATION 0x161 /* DP 2.0 */ > > #define DP_PSR_EN_CFG0x170 /* XXX 1.2? */ > # define DP_PSR_ENABLE BIT(0) > @@ -743,6 +748,7 @@ struct drm_panel; > # define DP_RECEIVE_PORT_0_STATUS(1 << 0) > # define DP_RECEIVE_PORT_1_STATUS(1 << 1) > # define DP_STREAM_REGENERATION_STATUS (1 << 2) /* 2.0 */ > +# define DP_INTRA_HOP_AUX_REPLY_INDICATION (1 << 3) /* 2.0 */ > > #define DP_ADJUST_REQUEST_LANE0_10x206 > #define DP_ADJUST_REQUEST_LANE2_30x207 > @@ -865,6 +871,8 @@ struct drm_panel; > # define DP_PHY_TEST_PATTERN_80BIT_CUSTOM 0x4 > # define DP_PHY_TEST_PATTERN_CP2520 0x5 > > +#define DP_PHY_SQUARE_PATTERN0x249 > + > #define DP_TEST_HBR2_SCRAMBLER_RESET0x24A > #define DP_TEST_80BIT_CUSTOM_PATTERN_7_00x250 > #define DP_TEST_80BIT_CUSTOM_PATTERN_15_8 0x251 > @@ -1109,6 +1117,18 @@ struct drm_panel; > #define DP_128B132B_TRAINING_AUX_RD_INTERVAL 0x2216 /* 2.0 */ > # define DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK 0x7f > > +#define DP_TEST_264BIT_CUSTOM_PATTERN_7_00x2230 > +#define DP_TEST_264BIT_CUSTOM_PATTERN_263_2560x2250 > + > +/* DSC Extended Capability Branch Total DSC Resources */ > +#define DP_DSC_SUPPORT_AND_DSC_DECODER_COUNT 0x2260 /* 2.0 */ > +# define DP_DSC_DECODER_COUNT_MASK (0b111 << 5) > +# define DP_DSC_DECODER_COUNT_SHIFT 5 > +#define DP_DSC_MAX_SLICE_COUNT_AND_AGGREGATION_0 0x2270 /* 2.0 */ > +# define DP_DSC_DECODER_0_MAXIMUM_SLICE_COUNT_MASK (1 << 0) > +# define DP_DSC_DECODER_0_AGGREGATION_SUPPORT_MASK (0b111 << 1) > +# define DP_DSC_DECODER_0_AGGREGATION_SUPPORT_SHIFT 1 > + > /* Protocol Converter Extension */ > /* HDMI CEC tunneling over AUX DP 1.3 section 5.3.3.3.1 DPCD 1.4+ */ > #define DP_CEC_TUNNELING_CAPABILITY0x3000 >
[Bug 214029] [bisected] [amdgpu] Several memory leaks in amdgpu and ttm
https://bugzilla.kernel.org/show_bug.cgi?id=214029 Erhard F. (erhar...@mailbox.org) changed: What|Removed |Added CC||christian.koe...@amd.com --- Comment #15 from Erhard F. (erhar...@mailbox.org) --- I got around skipping commits by cherry-picking 9551158069ba8fcc893798d42dc4f978b62ef60f (kfence: make compatible with kmemleak) and finally was able to complete the bisect. The offending commit was: # git bisect good d02117f8efaa5fbc37437df1ae955a147a2a424a is the first bad commit commit d02117f8efaa5fbc37437df1ae955a147a2a424a Author: Christian König Date: Sat Apr 17 19:09:30 2021 +0200 drm/ttm: remove special handling for non GEM drivers vmwgfx is the only driver actually using this. Move the handling into the driver instead. Signed-off-by: Christian König Acked-by: Huang Rui Reviewed-by: Zack Rusin Link: https://patchwork.freedesktop.org/patch/msgid/20210419092853.1605-1-christian.koe...@amd.com drivers/gpu/drm/ttm/ttm_bo.c | 11 --- drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 10 ++ include/drm/ttm/ttm_bo_api.h | 19 --- 3 files changed, 10 insertions(+), 30 deletions(-) -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 214029] [bisected] [amdgpu] Several memory leaks in amdgpu and ttm
https://bugzilla.kernel.org/show_bug.cgi?id=214029 --- Comment #16 from Erhard F. (erhar...@mailbox.org) --- Created attachment 299007 --> https://bugzilla.kernel.org/attachment.cgi?id=299007&action=edit final bisect.log -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [Freedreno] [PATCH] drm/msm: Switch ordering of runpm put vs devfreq_idle
On Tue, Sep 28, 2021 at 7:52 AM Akhil P Oommen wrote: > > On 9/27/2021 8:59 PM, Rob Clark wrote: > > From: Rob Clark > > > > I've seen a few crashes like: > > > > Internal error: synchronous external abort: 9610 [#1] PREEMPT SMP > > Modules linked in: snd_seq_dummy snd_seq snd_seq_device bridge stp llc > > tun nf_nat_tftp nf_conntrack_tftp nf_nat_ftp nf_conntrack_ftp esp6 ah6 > > ip6t_REJECT ip6t_ipv6header vhost_vsock vhost > > vmw_vsock_virtio_transport_common vsock rfcomm algif_hash algif_skcipher > > af_alg uinput veth xt_cgroup xt_MASQUERADE venus_enc venus_dec > > videobuf2_dma_contig qcom_spmi_adc5 qcom_spmi_adc_tm5 hci_uart > > qcom_vadc_common cros_ec_typec qcom_spmi_temp_alarm typec btqca > > snd_soc_rt5682_i2c snd_soc_rt5682 snd_soc_sc7180 bluetooth > > snd_soc_qcom_common snd_soc_rl6231 ecdh_generic ecc venus_core v4l2_mem2mem > > snd_soc_lpass_sc7180 snd_soc_lpass_hdmi snd_soc_lpass_cpu > > snd_soc_lpass_platform snd_soc_max98357a ip6table_nat fuse iio_trig_sysfs > > cros_ec_lid_angle cros_ec_sensors cros_ec_sensors_core > > industrialio_triggered_buffer kfifo_buf cros_ec_sensorhub lzo_rle > > ath10k_snoc lzo_compress ath10k_core ath zram mac80211 cfg80211 > > ax88179_178a usbnet mii uvcvideo videobuf2_vmalloc joydev > > CPU: 3 PID: 212 Comm: A618-worker Tainted: G W > > 5.4.139-16300-g88d8e1285982 #1 > > Hardware name: Google Pompom (rev1) with LTE (DT) > > pstate: 60c9 (nZCv daif +PAN +UAO) > > pc : a6xx_gmu_set_oob+0x114/0x200 > > lr : a6xx_gmu_set_oob+0x10c/0x200 > > sp : ffc011b7bc20 > > x29: ffc011b7bc20 x28: ffdad27c5000 > > x27: 0001 x26: ffdad1521044 > > x25: ffbef7498338 x24: 0018 > > x23: 0002 x22: 00014648 > > x21: 033732fe638b x20: 8000 > > x19: ffbef7433bc8 x18: 4000 > > x17: 00243508d982 x16: b67e > > x15: 90d4 x14: 0024 > > x13: 0024 x12: 00017521 > > x11: 0b48 x10: 00326a48 > > x9 : 1a130d33f6371600 x8 : ffc011e54648 > > x7 : 614948e5003c x6 : ffbe3cd17e60 > > x5 : 0040 x4 : 0004 > > x3 : x2 : ffbef7488000 > > x1 : ffbef7488000 x0 : > > Call trace: > > a6xx_gmu_set_oob+0x114/0x200 > > a6xx_gmu_set_freq+0xe0/0x1fc > > msm_devfreq_target+0x80/0x13c > > msm_devfreq_idle+0x54/0x94 > > retire_submit+0x170/0x254 > > retire_submits+0xa4/0xdc > > retire_worker+0x1c/0x28 > > kthread_worker_fn+0xf4/0x1bc > > kthread+0x140/0x158 > > ret_from_fork+0x10/0x18 > > Code: 52800c81 9415bbe5 f9400a68 8b160108 (b9400108) > > ---[ end trace 16b871df2482cd61 ]--- > > Kernel panic - not syncing: Fatal exception > > SMP: stopping secondary CPUs > > Kernel Offset: 0x1ac140 from 0xffc01000 > > PHYS_OFFSET: 0xffc28000 > > CPU features: 0x88102e,2a80aa38 > > Memory Limit: none > > > > Which smells a lot like touching hw after power collapse. I'm not > > *entirely* sure how it could have taken 66ms (the autosuspend delay) > > before we get to a6xx_gmu_set_oob(), but to be safe we should move > > the pm_runtime_put_autosuspend() after msm_devfreq_idle(). > https://elixir.bootlin.com/linux/v5.15-rc1/source/drivers/gpu/drm/msm/adreno/a6xx_gmu.c#L132 > We have this check in the gmu freq set path which should avoid this > scenario. I might be a bit pedantic here, but I feel that the original > code is more accurate. We should immediately mark last busy and put > runtime_pm refcount. The problem is that get_if_in_use(gmu->dev) doesn't prevent the _gpu_ device from suspending and racing down the _set_oob()/_clear_oob() path in parallel with set_freq().. I think that (plus the potential for either of those paths to race with get_timestamp()) is related to the gmu OOB crashes I'm seeing from the field. A better plan is probably something along the lines of [1].. maybe re-worked a bit if it is not acceptable to start using c99. Which we'll also need if we wanted to defer slightly clamping to idle. (Or if we had IFPC working we could just drop the whole freq change on idle<->active transitions ;-)) [1] https://patchwork.freedesktop.org/patch/455928/?series=95119&rev=1 > > -Akhil. > > > > > Fixes: 9bc95570175a ("drm/msm: Devfreq tuning") > > Signed-off-by: Rob Clark > > --- > > drivers/gpu/drm/msm/msm_gpu.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c > > index d1a16642ecd5..2b2bbe7499e6 100644 > > --- a/drivers/gpu/drm/msm/msm_gpu.c > > +++ b/drivers/gpu/drm/msm/msm_gpu.c > > @@ -667,9 +667,6 @@ static void retire_submit(struct msm_gpu *gpu, struct > > msm_ringbuffer *ring, > > > > msm_submit_retire
Re: linux-next: Tree for Sep 28 [drivers/gpu/drm/vc4/vc4.ko]
On 9/28/21 12:23 AM, Stephen Rothwell wrote: Hi all, Changes since 20210927: on x86_64: ERROR: modpost: "devm_drm_of_get_bridge" [drivers/gpu/drm/vc4/vc4.ko] undefined! Full randconfig file is attached. -- ~Randy config-r5051.gz Description: application/gzip
[PATCH] drm/msm/dp: only signal audio when disconnected detected at dp_pm_resume
Only signal audio when disconnected detected at dp_pm_resume since connected status will be signaled to audio at next plugin handler. Fixes: 078867ce04ed ("drm/msm/dp: signal audio plugged change at dp_pm_resume") Signed-off-by: Kuogee Hsieh --- drivers/gpu/drm/msm/dp/dp_display.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 0e543a03..6f13008 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1356,14 +1356,14 @@ static int dp_pm_resume(struct device *dev) * can not declared display is connected unless * HDMI cable is plugged in and sink_count of * dongle become 1 +* also only signal audio when disconnected */ - if (dp->link->sink_count) + if (dp->link->sink_count) { dp->dp_display.is_connected = true; - else + } else { dp->dp_display.is_connected = false; - - dp_display_handle_plugged_change(g_dp_display, - dp->dp_display.is_connected); + dp_display_handle_plugged_change(g_dp_display, false); + } DRM_DEBUG_DP("After, sink_count=%d is_connected=%d core_inited=%d power_on=%d\n", dp->link->sink_count, dp->dp_display.is_connected, -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH] drm/msm/dpu: Remove some nonsense
From: Rob Clark These aren't used. And if we add use for them later, we should probably do something a bit more structured than string parsing. Signed-off-by: Rob Clark --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 6 -- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 8 2 files changed, 14 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c index b131fd376192..e32dbb06aad1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c @@ -958,12 +958,6 @@ static const struct dpu_perf_cfg sdm845_perf_data = { .min_core_ib = 240, .min_llcc_ib = 80, .min_dram_ib = 80, - .core_ib_ff = "6.0", - .core_clk_ff = "1.0", - .comp_ratio_rt = - "NV12/5/1/1.23 AB24/5/1/1.23 XB24/5/1/1.23", - .comp_ratio_nrt = - "NV12/5/1/1.25 AB24/5/1/1.25 XB24/5/1/1.25", .undersized_prefill_lines = 2, .xtra_prefill_lines = 2, .dest_scale_prefill_lines = 3, diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h index d2a945a27cfa..4ade44bbd37e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h @@ -676,10 +676,6 @@ struct dpu_perf_cdp_cfg { * @min_core_ibminimum mnoc ib vote in kbps * @min_llcc_ibminimum llcc ib vote in kbps * @min_dram_ibminimum dram ib vote in kbps - * @core_ib_ff core instantaneous bandwidth fudge factor - * @core_clk_ffcore clock fudge factor - * @comp_ratio_rt string of 0 or more of /// - * @comp_ratio_nrt string of 0 or more of /// * @undersized_prefill_lines undersized prefill in lines * @xtra_prefill_lines extra prefill latency in lines * @dest_scale_prefill_lines destination scaler latency in lines @@ -702,10 +698,6 @@ struct dpu_perf_cfg { u32 min_core_ib; u32 min_llcc_ib; u32 min_dram_ib; - const char *core_ib_ff; - const char *core_clk_ff; - const char *comp_ratio_rt; - const char *comp_ratio_nrt; u32 undersized_prefill_lines; u32 xtra_prefill_lines; u32 dest_scale_prefill_lines; -- 2.31.1
Re: [PATCH 1/3] drm/bridge: Add a function to abstract away panels
W dniu 23.09.2021 o 02:29, Laurent Pinchart pisze: > Hi Maxime, > > Thank you for the patch. > > I know this has already been merged, but I have a question. > > On Fri, Sep 10, 2021 at 03:09:39PM +0200, Maxime Ripard wrote: >> Display drivers so far need to have a lot of boilerplate to first >> retrieve either the panel or bridge that they are connected to using >> drm_of_find_panel_or_bridge(), and then either deal with each with ad-hoc >> functions or create a drm panel bridge through drm_panel_bridge_add. >> >> In order to reduce the boilerplate and hopefully create a path of least >> resistance towards using the DRM panel bridge layer, let's create the >> function devm_drm_of_get_next to reduce that boilerplate. >> >> Signed-off-by: Maxime Ripard >> --- >> drivers/gpu/drm/drm_bridge.c | 42 >> drivers/gpu/drm/drm_of.c | 3 +++ >> include/drm/drm_bridge.h | 2 ++ >> 3 files changed, 43 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c >> index a8ed66751c2d..10ddca4638b0 100644 >> --- a/drivers/gpu/drm/drm_bridge.c >> +++ b/drivers/gpu/drm/drm_bridge.c >> @@ -28,6 +28,7 @@ >> #include >> #include >> #include >> +#include >> #include >> >> #include "drm_crtc_internal.h" >> @@ -51,10 +52,8 @@ >>* >>* Display drivers are responsible for linking encoders with the first >> bridge >>* in the chains. This is done by acquiring the appropriate bridge with >> - * of_drm_find_bridge() or drm_of_find_panel_or_bridge(), or creating it >> for a >> - * panel with drm_panel_bridge_add_typed() (or the managed version >> - * devm_drm_panel_bridge_add_typed()). Once acquired, the bridge shall be >> - * attached to the encoder with a call to drm_bridge_attach(). >> + * devm_drm_of_get_bridge(). Once acquired, the bridge shall be attached to >> the >> + * encoder with a call to drm_bridge_attach(). >>* >>* Bridges are responsible for linking themselves with the next bridge in >> the >>* chain, if any. This is done the same way as for encoders, with the call >> to >> @@ -1233,6 +1232,41 @@ struct drm_bridge *of_drm_find_bridge(struct >> device_node *np) >> return NULL; >> } >> EXPORT_SYMBOL(of_drm_find_bridge); >> + >> +/** >> + * devm_drm_of_get_bridge - Return next bridge in the chain >> + * @dev: device to tie the bridge lifetime to >> + * @np: device tree node containing encoder output ports >> + * @port: port in the device tree node >> + * @endpoint: endpoint in the device tree node >> + * >> + * Given a DT node's port and endpoint number, finds the connected node >> + * and returns the associated bridge if any, or creates and returns a >> + * drm panel bridge instance if a panel is connected. >> + * >> + * Returns a pointer to the bridge if successful, or an error pointer >> + * otherwise. >> + */ >> +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, >> + struct device_node *np, >> + unsigned int port, >> + unsigned int endpoint) >> +{ >> +struct drm_bridge *bridge; >> +struct drm_panel *panel; >> +int ret; >> + >> +ret = drm_of_find_panel_or_bridge(np, port, endpoint, >> + &panel, &bridge); >> +if (ret) >> +return ERR_PTR(ret); >> + >> +if (panel) >> +bridge = devm_drm_panel_bridge_add(dev, panel); >> + >> +return bridge; > I really like the idea, I've wanted to do something like this for a long > time. I however wonder if this is the best approach, or if we could get > the panel core to register the bridge itself. The part that bothers me > here is the assymetry in the lifetime of the bridges, the returned > pointer is either looked up or allocated. When drm_panel_bridge was proposed 1st time I suggested that instead of it we should just merge panels with bridges (maybe with new name, my favorite is drm_sink), but I was outvoted :) Apparently with this patch we go to the same direction: since panels will be accessed only via drm_panel_bridge they will become actually bridges :) Somebody could then clean-up the whole intermediate code. Regards Andrzej > > Bridge lifetime is such a mess that it may not make a big difference, > but eventually we'll have to address that problem globally. > >> +} >> +EXPORT_SYMBOL(devm_drm_of_get_bridge); >> #endif >> >> MODULE_AUTHOR("Ajay Kumar "); >> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c >> index 997b8827fed2..37c34146eea8 100644 >> --- a/drivers/gpu/drm/drm_of.c >> +++ b/drivers/gpu/drm/drm_of.c >> @@ -231,6 +231,9 @@ EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint); >>* return either the associated struct drm_panel or drm_bridge device. >> Either >>* @panel or @bridge must not be NULL. >>* >> + * This function is deprecated and should not be used in
Re: [PATCH] drm/msm/dpu: Remove some nonsense
On 28/09/2021 19:28, Rob Clark wrote: From: Rob Clark These aren't used. And if we add use for them later, we should probably do something a bit more structured than string parsing. Signed-off-by: Rob Clark Reviewed-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 6 -- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 8 2 files changed, 14 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c index b131fd376192..e32dbb06aad1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c @@ -958,12 +958,6 @@ static const struct dpu_perf_cfg sdm845_perf_data = { .min_core_ib = 240, .min_llcc_ib = 80, .min_dram_ib = 80, - .core_ib_ff = "6.0", - .core_clk_ff = "1.0", - .comp_ratio_rt = - "NV12/5/1/1.23 AB24/5/1/1.23 XB24/5/1/1.23", - .comp_ratio_nrt = - "NV12/5/1/1.25 AB24/5/1/1.25 XB24/5/1/1.25", .undersized_prefill_lines = 2, .xtra_prefill_lines = 2, .dest_scale_prefill_lines = 3, diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h index d2a945a27cfa..4ade44bbd37e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h @@ -676,10 +676,6 @@ struct dpu_perf_cdp_cfg { * @min_core_ibminimum mnoc ib vote in kbps * @min_llcc_ibminimum llcc ib vote in kbps * @min_dram_ibminimum dram ib vote in kbps - * @core_ib_ff core instantaneous bandwidth fudge factor - * @core_clk_ffcore clock fudge factor - * @comp_ratio_rt string of 0 or more of /// - * @comp_ratio_nrt string of 0 or more of /// * @undersized_prefill_lines undersized prefill in lines * @xtra_prefill_lines extra prefill latency in lines * @dest_scale_prefill_lines destination scaler latency in lines @@ -702,10 +698,6 @@ struct dpu_perf_cfg { u32 min_core_ib; u32 min_llcc_ib; u32 min_dram_ib; - const char *core_ib_ff; - const char *core_clk_ff; - const char *comp_ratio_rt; - const char *comp_ratio_nrt; u32 undersized_prefill_lines; u32 xtra_prefill_lines; u32 dest_scale_prefill_lines; -- With best wishes Dmitry
Re: [PATCH v5 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
On Sat 25 Sep 16:25 CDT 2021, Uwe Kleine-K?nig wrote: > Hello Bjorn, > > On Fri, Sep 24, 2021 at 05:04:41PM -0500, Bjorn Andersson wrote: > > On Fri 24 Sep 02:54 CDT 2021, Uwe Kleine-K?nig wrote: > > > > +static int ti_sn65dsi86_read_u16(struct ti_sn65dsi86 *pdata, > > > > +unsigned int reg, u16 *val) > > > > +{ > > > > + unsigned int tmp; > > > > + int ret; > > > > + > > > > + ret = regmap_read(pdata->regmap, reg, &tmp); > > > > + if (ret) > > > > + return ret; > > > > + *val = tmp; > > > > + > > > > + ret = regmap_read(pdata->regmap, reg + 1, &tmp); > > > > + if (ret) > > > > + return ret; > > > > + *val |= tmp << 8; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static void ti_sn65dsi86_write_u16(struct ti_sn65dsi86 *pdata, > > > >unsigned int reg, u16 val) > > > > { > > > > - regmap_write(pdata->regmap, reg, val & 0xFF); > > > > - regmap_write(pdata->regmap, reg + 1, val >> 8); > > > > + u8 buf[2] = { val & 0xff, val >> 8 }; > > > > + > > > > + regmap_bulk_write(pdata->regmap, reg, &buf, ARRAY_SIZE(buf)); > > > > > > Given that ti_sn65dsi86_write_u16 uses regmap_bulk_write I wonder why > > > ti_sn65dsi86_read_u16 doesn't use regmap_bulk_read. > > > > Seems reasonable to make that use the bulk interface as well and this > > would look better in its own patch. > > Not sure I understand you here. My position here would be to introduce > these two functions with a consistent behaviour. If both use bulk or > both don't use it (and you introduce it later in a separate patch) > doesn't matter to me. > We're in agreement :) > > > > static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn65dsi86 *pdata) > > > > @@ -253,6 +298,12 @@ static void ti_sn_bridge_set_refclk_freq(struct > > > > ti_sn65dsi86 *pdata) > > > > > > > > regmap_update_bits(pdata->regmap, SN_DPPLL_SRC_REG, > > > > REFCLK_FREQ_MASK, > > > >REFCLK_FREQ(i)); > > > > + > > > > + /* > > > > +* The PWM refclk is based on the value written to > > > > SN_DPPLL_SRC_REG, > > > > +* regardless of its actual sourcing. > > > > +*/ > > > > + pdata->pwm_refclk_freq = ti_sn_bridge_refclk_lut[i]; > > > > } > > > > > > > > static void ti_sn65dsi86_enable_comms(struct ti_sn65dsi86 *pdata) > > > > @@ -1259,9 +1310,283 @@ static struct auxiliary_driver > > > > ti_sn_bridge_driver = { > > > > }; > > > > > > > > /* > > > > - > > > > - * GPIO Controller > > > > + * PWM Controller > > > > + */ > > > > +#if defined(CONFIG_PWM) > > > > +static int ti_sn_pwm_pin_request(struct ti_sn65dsi86 *pdata) > > > > +{ > > > > + return atomic_xchg(&pdata->pwm_pin_busy, 1) ? -EBUSY : 0; > > > > +} > > > > + > > > > +static void ti_sn_pwm_pin_release(struct ti_sn65dsi86 *pdata) > > > > +{ > > > > + atomic_set(&pdata->pwm_pin_busy, 0); > > > > +} > > > > + > > > > +static struct ti_sn65dsi86 *pwm_chip_to_ti_sn_bridge(struct pwm_chip > > > > *chip) > > > > +{ > > > > + return container_of(chip, struct ti_sn65dsi86, pchip); > > > > +} > > > > + > > > > +static int ti_sn_pwm_request(struct pwm_chip *chip, struct pwm_device > > > > *pwm) > > > > +{ > > > > + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip); > > > > + > > > > + return ti_sn_pwm_pin_request(pdata); > > > > +} > > > > + > > > > +static void ti_sn_pwm_free(struct pwm_chip *chip, struct pwm_device > > > > *pwm) > > > > +{ > > > > + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip); > > > > + > > > > + ti_sn_pwm_pin_release(pdata); > > > > +} > > > > + > > > > +/* > > > > + * Limitations: > > > > + * - The PWM signal is not driven when the chip is powered down, or in > > > > its > > > > + * reset state and the driver does not implement the "suspend state" > > > > + * described in the documentation. In order to save power, > > > > state->enabled is > > > > + * interpreted as denoting if the signal is expected to be valid, > > > > and is used > > > > + * to determine if the chip needs to be kept powered. > > > > + * - Changing both period and duty_cycle is not done atomically, > > > > neither is the > > > > + * multi-byte register updates, so the output might briefly be > > > > undefined > > > > + * during update. > > > > */ > > > > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device > > > > *pwm, > > > > + const struct pwm_state *state) > > > > +{ > > > > + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip); > > > > + unsigned int pwm_en_inv; > > > > + unsigned int backlight; > > > > + unsigned int pre_div; > > > > + unsigned int scale; > > > > + u64 period_max; > > > > + u64 period; > > > > + int ret
[PATCH v2] drm/amd/display: Only define DP 2.0 symbols if not already defined
[Why] For some reason we're defining DP 2.0 definitions inside our driver. Now that patches to introduce relevant definitions are slated to be merged into drm-next this is causing conflicts. In file included from drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c:33: In file included from ./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgpu.h:70: In file included from ./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgpu_mode.h:36: ./include/drm/drm_dp_helper.h:1322:9: error: 'DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER' macro redefined [-Werror,-Wmacro-redefined] ^ ./drivers/gpu/drm/amd/amdgpu/../display/dc/dc_dp_types.h:881:9: note: previous definition is here ^ 1 error generated. v2: Add one missing endif [How] Guard all display driver defines with #ifndef for now. Once we pull in the new definitions into amd-staging-drm-next we will follow up and drop definitions from our driver and provide follow-up header updates for any addition DP 2.0 definitions required by our driver. Signed-off-by: Harry Wentland --- drivers/gpu/drm/amd/display/dc/dc_dp_types.h | 54 ++-- 1 file changed, 49 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/amd/display/dc/dc_dp_types.h b/drivers/gpu/drm/amd/display/dc/dc_dp_types.h index a5e798b5da79..9de86ff5ef1b 100644 --- a/drivers/gpu/drm/amd/display/dc/dc_dp_types.h +++ b/drivers/gpu/drm/amd/display/dc/dc_dp_types.h @@ -860,28 +860,72 @@ struct psr_caps { }; #if defined(CONFIG_DRM_AMD_DC_DCN) +#ifndef DP_MAIN_LINK_CHANNEL_CODING_CAP #define DP_MAIN_LINK_CHANNEL_CODING_CAP0x006 +#endif +#ifndef DP_SINK_VIDEO_FALLBACK_FORMATS #define DP_SINK_VIDEO_FALLBACK_FORMATS 0x020 +#endif +#ifndef DP_FEC_CAPABILITY_1 #define DP_FEC_CAPABILITY_10x091 +#endif +#ifndef DP_DFP_CAPABILITY_EXTENSION_SUPPORT #define DP_DFP_CAPABILITY_EXTENSION_SUPPORT0x0A3 +#endif +#ifndef DP_DSC_CONFIGURATION #define DP_DSC_CONFIGURATION 0x161 +#endif +#ifndef DP_PHY_SQUARE_PATTERN #define DP_PHY_SQUARE_PATTERN 0x249 +#endif +#ifndef DP_128b_132b_SUPPORTED_LINK_RATES #define DP_128b_132b_SUPPORTED_LINK_RATES 0x2215 +#endif +#ifndef DP_128b_132b_TRAINING_AUX_RD_INTERVAL #define DP_128b_132b_TRAINING_AUX_RD_INTERVAL 0x2216 +#endif +#ifndef DP_TEST_264BIT_CUSTOM_PATTERN_7_0 #define DP_TEST_264BIT_CUSTOM_PATTERN_7_0 0X2230 +#endif +#ifndef DP_TEST_264BIT_CUSTOM_PATTERN_263_256 #define DP_TEST_264BIT_CUSTOM_PATTERN_263_256 0X2250 +#endif +#ifndef DP_DSC_SUPPORT_AND_DECODER_COUNT #define DP_DSC_SUPPORT_AND_DECODER_COUNT 0x2260 +#endif +#ifndef DP_DSC_MAX_SLICE_COUNT_AND_AGGREGATION_0 #define DP_DSC_MAX_SLICE_COUNT_AND_AGGREGATION_0 0x2270 -# define DP_DSC_DECODER_0_MAXIMUM_SLICE_COUNT_MASK (1 << 0) -# define DP_DSC_DECODER_0_AGGREGATION_SUPPORT_MASK (0b111 << 1) -# define DP_DSC_DECODER_0_AGGREGATION_SUPPORT_SHIFT1 -# define DP_DSC_DECODER_COUNT_MASK (0b111 << 5) -# define DP_DSC_DECODER_COUNT_SHIFT5 +#endif +#ifndef DP_DSC_DECODER_0_MAXIMUM_SLICE_COUNT_MASK +#define DP_DSC_DECODER_0_MAXIMUM_SLICE_COUNT_MASK (1 << 0) +#endif +#ifndef DP_DSC_DECODER_0_AGGREGATION_SUPPORT_MASK +#define DP_DSC_DECODER_0_AGGREGATION_SUPPORT_MASK (0b111 << 1) +#endif +#ifndef DP_DSC_DECODER_0_AGGREGATION_SUPPORT_SHIFT +#define DP_DSC_DECODER_0_AGGREGATION_SUPPORT_SHIFT 1 +#endif +#ifndef DP_DSC_DECODER_COUNT_MASK +#define DP_DSC_DECODER_COUNT_MASK (0b111 << 5) +#endif +#ifndef DP_DSC_DECODER_COUNT_SHIFT +#define DP_DSC_DECODER_COUNT_SHIFT 5 +#endif +#ifndef DP_MAIN_LINK_CHANNEL_CODING_SET #define DP_MAIN_LINK_CHANNEL_CODING_SET0x108 +#endif +#ifndef DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER #define DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER 0xF0006 +#endif +#ifndef DP_PHY_REPEATER_128b_132b_RATES #define DP_PHY_REPEATER_128b_132b_RATES0xF0007 +#endif +#ifndef DP_128b_132b_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER1 #define DP_128b_132b_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER10xF0022 +#endif +#ifndef DP_INTRA_HOP_AUX_REPLY_INDICATION #define DP_INTRA_HOP_AUX_REPLY_INDICATION (1 << 3) +#endif /* TODO - Use DRM header to replace above once available */ union dp_main_line_channel_coding_cap { -- 2.33.0
Re: [PATCH v6 1/2] habanalabs: define uAPI to export FD for DMA-BUF
On Sun, Sep 12, 2021 at 07:53:08PM +0300, Oded Gabbay wrote: > /* HL_MEM_OP_* */ > __u32 op; > - /* HL_MEM_* flags */ > + /* HL_MEM_* flags. > + * For the HL_MEM_OP_EXPORT_DMABUF_FD opcode, this field holds the > + * DMA-BUF file/FD flags. > + */ > __u32 flags; > /* Context ID - Currently not in use */ > __u32 ctx_id; > @@ -1072,6 +1091,13 @@ struct hl_mem_out { > > __u32 pad; > }; > + > + /* Returned in HL_MEM_OP_EXPORT_DMABUF_FD. Represents the > + * DMA-BUF object that was created to describe a memory > + * allocation on the device's memory space. The FD should be > + * passed to the importer driver > + */ > + __u64 fd; fd's should be a s32 type in a fixed width uapi. I usually expect to see the uapi changes inside the commit that consumes them, splitting the patch like this seems strange but harmless. Jason
Re: [PATCH v2 3/5] drm/msm/dp: Support up to 3 DP controllers
On 2021-08-27 10:14, Bjorn Andersson wrote: On Fri 27 Aug 00:20 CDT 2021, Stephen Boyd wrote: Quoting Bjorn Andersson (2021-08-25 16:42:31) > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 2c7de43f655a..4a6132c18e57 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -78,6 +78,8 @@ struct dp_display_private { > char *name; > int irq; > > + int id; > + > /* state variables */ > bool core_initialized; > bool hpd_irq_on; > @@ -115,8 +117,19 @@ struct dp_display_private { > struct dp_audio *audio; > }; > > + > +struct msm_dp_config { > + phys_addr_t io_start[3]; Can this be made into another struct, like msm_dp_desc, that also indicates what type of DP connector it is, i.e. eDP vs DP? That would help me understand in modetest and /sys/class/drm what sort of connector is probing. dp_drm_connector_init() would need to pass the type of connector appropriately. Right now, eDP connectors still show up as DP instead of eDP in sysfs. I like it, will spin a v3 with this. Regards, Bjorn Hi Bjorn, Have you spin off V3 yet? When you expect your patches related to DP be up streamed? Thanks, kuogee > + size_t num_dp; > +}; > + > +static const struct msm_dp_config sc7180_dp_cfg = { > + .io_start = { 0x0ae9 }, > + .num_dp = 1, > +}; > + > static const struct of_device_id dp_dt_match[] = { > - {.compatible = "qcom,sc7180-dp"}, > + { .compatible = "qcom,sc7180-dp", .data = &sc7180_dp_cfg }, > {} > }; >
Re: [Freedreno] [PATCH v2 04/13] drm/hdcp: Expand HDCP helper library for enable/disable/check
On Tue, Sep 21, 2021 at 04:34:59PM -0700, abhin...@codeaurora.org wrote: > On 2021-09-15 13:38, Sean Paul wrote: > > From: Sean Paul > > > > This patch expands upon the HDCP helper library to manage HDCP > > enable, disable, and check. > > > > Previous to this patch, the majority of the state management and sink > > interaction is tucked inside the Intel driver with the understanding > > that once a new platform supported HDCP we could make good decisions > > about what should be centralized. With the addition of HDCP support > > for Qualcomm, it's time to migrate the protocol-specific bits of HDCP > > authentication, key exchange, and link checks to the HDCP helper. > > > > In terms of functionality, this migration is 1:1 with the Intel driver, > > however things are laid out a bit differently than with intel_hdcp.c, > > which is why this is a separate patch from the i915 transition to the > > helper. On i915, the "shim" vtable is used to account for HDMI vs. DP > > vs. DP-MST differences whereas the helper library uses a LUT to > > account for the register offsets and a remote read function to route > > the messages. On i915, storing the sink information in the source is > > done inline whereas now we use the new drm_hdcp_helper_funcs vtable > > to store and fetch information to/from source hw. Finally, instead of > > calling enable/disable directly from the driver, we'll leave that > > decision to the helper and by calling drm_hdcp_helper_atomic_commit() > > from the driver. All told, this will centralize the protocol and state > > handling in the helper, ensuring we collect all of our bugs^Wlogic > > in one place. > > > > Signed-off-by: Sean Paul > > Link: > > https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-5-s...@poorly.run > > #v1 > > > > Changes in v2: > > -Fixed set-but-unused variable identified by 0-day > > --- > > drivers/gpu/drm/drm_hdcp.c | 1103 > > include/drm/drm_hdcp.h | 191 +++ > > 2 files changed, 1294 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_hdcp.c b/drivers/gpu/drm/drm_hdcp.c > > index 742313ce8f6f..47c6e6923a76 100644 > > --- a/drivers/gpu/drm/drm_hdcp.c > > +++ b/drivers/gpu/drm/drm_hdcp.c /snip > > +static void drm_hdcp_helper_check_work(struct work_struct *work) > > +{ > > + struct drm_hdcp_helper_data *data = > > container_of(to_delayed_work(work), > > +struct > > drm_hdcp_helper_data, > > +check_work); > > + unsigned long check_link_interval; > > + > > Does this SW polling for Ri' mismatch need to be done even if the HW is > capable of doing it > on its own? > MSM HDCP 1x HW can periodically check Ri' mismatches and issue an interrupt > if there is a mismatch. > In that case this SW polling is not needed. So maybe check if the HW > supports polling and if so > skip this SW polling? > One could certainly change this to be HW driven. There is also an interrupt on Intel for DP links which [re]schedules link check in the interrupt handler, something similar could be done for msm. Note that even on these Intel links which support the CP interrupt, the worker still runs on the normal cadence. I haven't considered relying solely on the interrupt since I want to be sure we didn't miss anything. Sean > > + mutex_lock(&data->mutex); > > + if (data->value != DRM_MODE_CONTENT_PROTECTION_ENABLED) > > + goto out_data_mutex; > > + > > + drm_hdcp_helper_driver_lock(data); > > + > > + if (data->enabled_type == DRM_MODE_HDCP_CONTENT_TYPE1) { > > + if (drm_hdcp_hdcp2_check_link(data)) > > + goto out; > > + check_link_interval = DRM_HDCP2_CHECK_PERIOD_MS; > > + } else { > > + if (drm_hdcp_hdcp1_check_link(data)) > > + goto out; > > + check_link_interval = DRM_HDCP_CHECK_PERIOD_MS; > > + } > > + schedule_delayed_work(&data->check_work, check_link_interval); > > + > > +out: > > + drm_hdcp_helper_driver_unlock(data); > > +out_data_mutex: > > + mutex_unlock(&data->mutex); > > +} /snip -- Sean Paul, Software Engineer, Google / Chromium OS
Re: [PATCH v6 2/2] habanalabs: add support for dma-buf exporter
On Sun, Sep 12, 2021 at 07:53:09PM +0300, Oded Gabbay wrote: > From: Tomer Tayar > > Implement the calls to the dma-buf kernel api to create a dma-buf > object backed by FD. > > We block the option to mmap the DMA-BUF object because we don't support > DIRECT_IO and implicit P2P. This statement doesn't make sense, you can mmap your dmabuf if you like. All dmabuf mmaps are supposed to set the special bit/etc to exclude them from get_user_pages() anyhow - and since this is BAR memory not struct page memory this driver would be doing it anyhow. > We check the p2p distance using pci_p2pdma_distance_many() and refusing > to map dmabuf in case the distance doesn't allow p2p. Does this actually allow the p2p transfer for your intended use cases? > diff --git a/drivers/misc/habanalabs/common/memory.c > b/drivers/misc/habanalabs/common/memory.c > index 33986933aa9e..8cf5437c0390 100644 > +++ b/drivers/misc/habanalabs/common/memory.c > @@ -1,7 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0 > > /* > - * Copyright 2016-2019 HabanaLabs, Ltd. > + * Copyright 2016-2021 HabanaLabs, Ltd. > * All Rights Reserved. > */ > > @@ -11,11 +11,13 @@ > > #include > #include > +#include > > #define HL_MMU_DEBUG 0 > > /* use small pages for supporting non-pow2 (32M/40M/48M) DRAM phys page > sizes */ > -#define DRAM_POOL_PAGE_SIZE SZ_8M > +#define DRAM_POOL_PAGE_SIZE SZ_8M > + ?? > /* > * The va ranges in context object contain a list with the available chunks > of > @@ -347,6 +349,13 @@ static int free_device_memory(struct hl_ctx *ctx, struct > hl_mem_in *args) > return -EINVAL; > } > > + if (phys_pg_pack->exporting_cnt) { > + dev_err(hdev->dev, > + "handle %u is exported, cannot free\n", handle); > + spin_unlock(&vm->idr_lock); Don't write to the kernel log from user space triggered actions > +static int alloc_sgt_from_device_pages(struct hl_device *hdev, > + struct sg_table **sgt, u64 *pages, > + u64 npages, u64 page_size, > + struct device *dev, > + enum dma_data_direction dir) Why doesn't this return a sg_table * and an ERR_PTR? > +{ > + u64 chunk_size, bar_address, dma_max_seg_size; > + struct asic_fixed_properties *prop; > + int rc, i, j, nents, cur_page; > + struct scatterlist *sg; > + > + prop = &hdev->asic_prop; > + > + dma_max_seg_size = dma_get_max_seg_size(dev); > + > + /* We would like to align the max segment size to PAGE_SIZE, so the > + * SGL will contain aligned addresses that can be easily mapped to > + * an MMU > + */ > + dma_max_seg_size = ALIGN_DOWN(dma_max_seg_size, PAGE_SIZE); > + if (dma_max_seg_size < PAGE_SIZE) { > + dev_err_ratelimited(hdev->dev, > + "dma_max_seg_size %llu can't be smaller than > PAGE_SIZE\n", > + dma_max_seg_size); > + return -EINVAL; > + } > + > + *sgt = kzalloc(sizeof(**sgt), GFP_KERNEL); > + if (!*sgt) > + return -ENOMEM; > + > + /* If the size of each page is larger than the dma max segment size, > + * then we can't combine pages and the number of entries in the SGL > + * will just be the > + * * > + */ > + if (page_size > dma_max_seg_size) > + nents = npages * DIV_ROUND_UP_ULL(page_size, dma_max_seg_size); > + else > + /* Get number of non-contiguous chunks */ > + for (i = 1, nents = 1, chunk_size = page_size ; i < npages ; > i++) { > + if (pages[i - 1] + page_size != pages[i] || > + chunk_size + page_size > > dma_max_seg_size) { > + nents++; > + chunk_size = page_size; > + continue; > + } > + > + chunk_size += page_size; > + } > + > + rc = sg_alloc_table(*sgt, nents, GFP_KERNEL | __GFP_ZERO); > + if (rc) > + goto error_free; > + > + /* Because we are not going to include a CPU list we want to have some > + * chance that other users will detect this by setting the orig_nents > + * to 0 and using only nents (length of DMA list) when going over the > + * sgl > + */ > + (*sgt)->orig_nents = 0; Maybe do this at the end so you'd have to undo it on the error path? > + cur_page = 0; > + > + if (page_size > dma_max_seg_size) { > + u64 size_left, cur_device_address = 0; > + > + size_left = page_size; > + > + /* Need to split each page into the number of chunks of > + * dma_max_seg_size > + */ > + for_each_sgtable_dma_sg((*sgt), sg, i) { > +
RE: [PATCH v2] drm/amd/display: Only define DP 2.0 symbols if not already defined
[AMD Official Use Only] > -Original Message- > From: Harry Wentland > Sent: September 28, 2021 1:08 PM > To: Deucher, Alexander ; amd- > g...@lists.freedesktop.org; Zuo, Jerry > Cc: jani.nik...@intel.com; Li, Sun peng (Leo) ; > nat...@kernel.org; intel-...@lists.freedesktop.org; dri- > de...@lists.freedesktop.org; ville.syrj...@linux.intel.com; > manasi.d.nav...@intel.com; Koenig, Christian ; > Pan, Xinhui ; s...@canb.auug.org.au; linux- > n...@vger.kernel.org; airl...@gmail.com; daniel.vet...@ffwll.ch; Wentland, > Harry > Subject: [PATCH v2] drm/amd/display: Only define DP 2.0 symbols if not > already defined > > [Why] > For some reason we're defining DP 2.0 definitions inside our driver. Now that > patches to introduce relevant definitions are slated to be merged into drm- > next this is causing conflicts. > > In file included from drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c:33: > In file included > from ./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgpu.h:70: > In file included > from ./drivers/gpu/drm/amd/amdgpu/../amdgpu/amdgpu_mode.h:36: > ./include/drm/drm_dp_helper.h:1322:9: error: > 'DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER' macro redefined [- > Werror,-Wmacro-redefined] > ^ > ./drivers/gpu/drm/amd/amdgpu/../display/dc/dc_dp_types.h:881:9: note: > previous definition is here > ^ > 1 error generated. > > v2: Add one missing endif > > [How] > Guard all display driver defines with #ifndef for now. Once we pull in the new > definitions into amd-staging-drm-next we will follow up and drop definitions > from our driver and provide follow-up header updates for any addition DP > 2.0 definitions required by our driver. > > Signed-off-by: Harry Wentland Reviewed-by: Fangzhi Zuo > --- > drivers/gpu/drm/amd/display/dc/dc_dp_types.h | 54 > ++-- > 1 file changed, 49 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/dc/dc_dp_types.h > b/drivers/gpu/drm/amd/display/dc/dc_dp_types.h > index a5e798b5da79..9de86ff5ef1b 100644 > --- a/drivers/gpu/drm/amd/display/dc/dc_dp_types.h > +++ b/drivers/gpu/drm/amd/display/dc/dc_dp_types.h > @@ -860,28 +860,72 @@ struct psr_caps { > }; > > #if defined(CONFIG_DRM_AMD_DC_DCN) > +#ifndef DP_MAIN_LINK_CHANNEL_CODING_CAP > #define DP_MAIN_LINK_CHANNEL_CODING_CAP 0x006 > +#endif > +#ifndef DP_SINK_VIDEO_FALLBACK_FORMATS > #define DP_SINK_VIDEO_FALLBACK_FORMATS 0x020 > +#endif > +#ifndef DP_FEC_CAPABILITY_1 > #define DP_FEC_CAPABILITY_1 0x091 > +#endif > +#ifndef DP_DFP_CAPABILITY_EXTENSION_SUPPORT > #define DP_DFP_CAPABILITY_EXTENSION_SUPPORT 0x0A3 > +#endif > +#ifndef DP_DSC_CONFIGURATION > #define DP_DSC_CONFIGURATION 0x161 > +#endif > +#ifndef DP_PHY_SQUARE_PATTERN > #define DP_PHY_SQUARE_PATTERN0x249 > +#endif > +#ifndef DP_128b_132b_SUPPORTED_LINK_RATES > #define DP_128b_132b_SUPPORTED_LINK_RATES0x2215 > +#endif > +#ifndef DP_128b_132b_TRAINING_AUX_RD_INTERVAL > #define DP_128b_132b_TRAINING_AUX_RD_INTERVAL > 0x2216 > +#endif > +#ifndef DP_TEST_264BIT_CUSTOM_PATTERN_7_0 > #define DP_TEST_264BIT_CUSTOM_PATTERN_7_00X2230 > +#endif > +#ifndef DP_TEST_264BIT_CUSTOM_PATTERN_263_256 > #define DP_TEST_264BIT_CUSTOM_PATTERN_263_256 > 0X2250 > +#endif > +#ifndef DP_DSC_SUPPORT_AND_DECODER_COUNT > #define DP_DSC_SUPPORT_AND_DECODER_COUNT 0x2260 > +#endif > +#ifndef DP_DSC_MAX_SLICE_COUNT_AND_AGGREGATION_0 > #define DP_DSC_MAX_SLICE_COUNT_AND_AGGREGATION_0 > 0x2270 > -# define DP_DSC_DECODER_0_MAXIMUM_SLICE_COUNT_MASK (1 << > 0) > -# define DP_DSC_DECODER_0_AGGREGATION_SUPPORT_MASK > (0b111 << 1) > -# define DP_DSC_DECODER_0_AGGREGATION_SUPPORT_SHIFT 1 > -# define DP_DSC_DECODER_COUNT_MASK (0b111 << 5) > -# define DP_DSC_DECODER_COUNT_SHIFT 5 > +#endif > +#ifndef DP_DSC_DECODER_0_MAXIMUM_SLICE_COUNT_MASK > +#define DP_DSC_DECODER_0_MAXIMUM_SLICE_COUNT_MASK(1 << > 0) > +#endif > +#ifndef DP_DSC_DECODER_0_AGGREGATION_SUPPORT_MASK > +#define DP_DSC_DECODER_0_AGGREGATION_SUPPORT_MASK > (0b111 << 1) > +#endif > +#ifndef DP_DSC_DECODER_0_AGGREGATION_SUPPORT_SHIFT > +#define DP_DSC_DECODER_0_AGGREGATION_SUPPORT_SHIFT 1 > +#endif > +#ifndef DP_DSC_DECODER_COUNT_MASK > +#define DP_DSC_DECODER_COUNT_MASK(0b111 << 5) > +#endif > +#ifndef DP_DSC_DECODER_COUNT_SHIFT > +#define DP_DSC_DECODER_COUNT_SHIFT 5 > +#endif > +#ifndef DP_MAIN_LINK_CHANNEL_CODING_SET > #define DP_MAIN_LINK_CHANNEL_CODING_SET 0x108 > +#endif > +#ifndef DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER > #define DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER 0xF0006 > +#endif > +#ifndef DP_PHY_REPEATER_128b_132b_RATES > #define DP_PHY_REPEATER_128b_132b_RATES > 0xF0007 > +#endif > +#ifndef DP_128b_132b_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER1 > #define
Re: [Freedreno] [PATCH v2 13/13] drm/msm: Implement HDCP 1.x using the new drm HDCP helpers
On Tue, Sep 21, 2021 at 07:25:41PM -0700, abhin...@codeaurora.org wrote: > On 2021-09-15 13:38, Sean Paul wrote: > > From: Sean Paul > > > > This patch adds HDCP 1.x support to msm DP connectors using the new HDCP > > helpers. > > > > Cc: Stephen Boyd > > Signed-off-by: Sean Paul > > Link: > > https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-15-s...@poorly.run > > #v1 > > > > Changes in v2: > > -Squash [1] into this patch with the following changes (Stephen) > > -Update the sc7180 dtsi file > > -Remove resource names and just use index (Stephen) > > > > > > [1] > > https://patchwork.freedesktop.org/patch/msgid/20210913175747.47456-14-s...@poorly.run > > --- /snip > > diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile > > index 904535eda0c4..98731fd262d6 100644 > > --- a/drivers/gpu/drm/msm/Makefile > > +++ b/drivers/gpu/drm/msm/Makefile > > @@ -109,6 +109,7 @@ msm-$(CONFIG_DRM_MSM_DP)+= dp/dp_aux.o \ > > dp/dp_ctrl.o \ > > dp/dp_display.o \ > > dp/dp_drm.o \ > > + dp/dp_hdcp.o \ > > dp/dp_hpd.o \ > > dp/dp_link.o \ > > dp/dp_panel.o \ > > diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c > > b/drivers/gpu/drm/msm/dp/dp_debug.c > > index 2f6247e80e9d..de16fca8782a 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_debug.c > > +++ b/drivers/gpu/drm/msm/dp/dp_debug.c /snip > > +static ssize_t dp_hdcp_key_write(struct file *file, const char __user > > *ubuf, > > +size_t len, loff_t *offp) > > +{ > > + char *input_buffer; > > + int ret = 0; > > + struct dp_debug_private *debug = file->private_data; > > + struct drm_device *dev; > > + > > + dev = debug->drm_dev; > > + > > + if (len != (DRM_HDCP_KSV_LEN + DP_HDCP_NUM_KEYS * DP_HDCP_KEY_LEN)) > > + return -EINVAL; > > + > > + if (!debug->hdcp) > > + return -ENOENT; > > + > > + input_buffer = memdup_user_nul(ubuf, len); > > + if (IS_ERR(input_buffer)) > > + return PTR_ERR(input_buffer); > > + > > + ret = dp_hdcp_ingest_key(debug->hdcp, input_buffer, len); > > + > > + kfree(input_buffer); > > + if (ret < 0) { > > + DRM_ERROR("Could not ingest HDCP key, ret=%d\n", ret); > > + return ret; > > + } > > + > > + *offp += len; > > + return len; > > +} > > It seems like the HDCP keys written using debugfs, just for my > understanding, > are you storing this in some secure partition and the usermode reads from it > and writes them here? > We have not sorted out the userspace side of HDCP enablement yet, so it remains to be seen whether the keys will be injected via debugfs/firmware file/property. /snip > > +static int dp_connector_atomic_check(struct drm_connector *connector, > > +struct drm_atomic_state *state) > > +{ > > + struct drm_connector_state *conn_state; > > + struct dp_connector_state *dp_state; > > + > > + conn_state = drm_atomic_get_new_connector_state(state, connector); > > + dp_state = to_dp_connector_state(conn_state); > > + > > + dp_state->hdcp_transition = drm_hdcp_atomic_check(connector, state); > > I have a general question related to the transition flag and overall tying > the HDCP > enable and authentication to the commit. > So lets say there is a case where the driver needs to disable HDCP. It could > be due > to link integrity failure OR some other error condition which usermode is > not aware of. > In that case, we will set this hdcp_transition to true but in the next > commit we will > actually do the authentication. What if usermode doesnt issue a new frame? > This question arises because currently the link intergrity check is done > using SW polling > in the previous patchset. But as I had commented there, this occurs in HW > for us. > I dont see that isr itself in this patchset. So wanted to understand if > thats part of this > approach to still tie it with commit. > > So if we go with the HW polling based approach which is the preferred > method, we need to > untie this from the commit. > In the case of error, the worker will detect it and try to re-authenticate. If the re-authentication is successful, userspace will continue to be unaware and everything will keep working. If re-authentication is unsuccessful, the worker will update the property value and issue a uevent to userspace. So HDCP enablement is only tied to commits when the property value is changing as a result of userspace. Regarding SW vs HW link checks, I don't think there's any difference in efficacy between them. If HW can be relied on to issue an interrupt in failure cases, a follow-up set allowing for this seems like a great idea. > > + > > + return 0; > > +} /snip > > +static int dp_hdcp_load_keys(struct drm_connector *connector) > > +{ > > + struct dp_hdcp *hdcp = dp_display_connector_to_hdcp(connector); > > + struct dp_hdcp_key *key; > > + int i, ret = 0; > > + > > + mutex_lock(&hdcp->key_lock); > > + > > + key = hdcp->key; > > + >
Re: [Freedreno] [PATCH v2 00/13] drm/hdcp: Pull HDCP auth/exchange/check into helpers
On Tue, Sep 21, 2021 at 07:30:29PM -0700, abhin...@codeaurora.org wrote: > Hi Sean > > On 2021-09-15 13:38, Sean Paul wrote: > > From: Sean Paul > > > > Hello again, > > This is the second version of the HDCP helper patchset. See version 1 > > here: https://patchwork.freedesktop.org/series/94623/ > > > > In this second version, I've fixed up the oopsies exposed by 0-day and > > yamllint and incorporated early review feedback from the dt/dts reviews. > > > > Please take a look, > > > > Sean > > One question overall on the series: > > 1) Regarding validation, did you run any secure video to check the > transitions? Yep, the transitions look good, no visual artifacts. Unplug/replug/suspend/resume all seem to be behaving as expected. > 2) Is running HDCP 1x compliance also part of the validation efforts? If Qualcomm has the ability to run validation, I'd be very keen to get some help in that regard. Sean > > Thanks > > Abhinav > > > > > Sean Paul (13): > > drm/hdcp: Add drm_hdcp_atomic_check() > > drm/hdcp: Avoid changing crtc state in hdcp atomic check > > drm/hdcp: Update property value on content type and user changes > > drm/hdcp: Expand HDCP helper library for enable/disable/check > > drm/i915/hdcp: Consolidate HDCP setup/state cache > > drm/i915/hdcp: Retain hdcp_capable return codes > > drm/i915/hdcp: Use HDCP helpers for i915 > > drm/msm/dpu_kms: Re-order dpu includes > > drm/msm/dpu: Remove useless checks in dpu_encoder > > drm/msm/dpu: Remove encoder->enable() hack > > drm/msm/dp: Re-order dp_audio_put in deinit_sub_modules > > dt-bindings: msm/dp: Add bindings for HDCP registers > > drm/msm: Implement HDCP 1.x using the new drm HDCP helpers > > > > .../bindings/display/msm/dp-controller.yaml |7 +- > > arch/arm64/boot/dts/qcom/sc7180.dtsi |4 +- > > drivers/gpu/drm/drm_hdcp.c| 1197 - > > drivers/gpu/drm/i915/display/intel_atomic.c |7 +- > > drivers/gpu/drm/i915/display/intel_ddi.c | 29 +- > > .../drm/i915/display/intel_display_debugfs.c | 11 +- > > .../drm/i915/display/intel_display_types.h| 58 +- > > drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 345 ++--- > > drivers/gpu/drm/i915/display/intel_dp_mst.c | 17 +- > > drivers/gpu/drm/i915/display/intel_hdcp.c | 1011 +++--- > > drivers/gpu/drm/i915/display/intel_hdcp.h | 35 +- > > drivers/gpu/drm/i915/display/intel_hdmi.c | 256 ++-- > > drivers/gpu/drm/msm/Makefile |1 + > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 17 +- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 30 +- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h |2 - > > drivers/gpu/drm/msm/disp/dpu1/dpu_trace.h |4 - > > drivers/gpu/drm/msm/dp/dp_debug.c | 49 +- > > drivers/gpu/drm/msm/dp/dp_debug.h |6 +- > > drivers/gpu/drm/msm/dp/dp_display.c | 47 +- > > drivers/gpu/drm/msm/dp/dp_display.h |5 + > > drivers/gpu/drm/msm/dp/dp_drm.c | 68 +- > > drivers/gpu/drm/msm/dp/dp_drm.h |5 + > > drivers/gpu/drm/msm/dp/dp_hdcp.c | 433 ++ > > drivers/gpu/drm/msm/dp/dp_hdcp.h | 27 + > > drivers/gpu/drm/msm/dp/dp_parser.c| 22 +- > > drivers/gpu/drm/msm/dp/dp_parser.h|4 + > > drivers/gpu/drm/msm/dp/dp_reg.h | 44 +- > > drivers/gpu/drm/msm/msm_atomic.c | 15 + > > include/drm/drm_hdcp.h| 194 +++ > > 30 files changed, 2561 insertions(+), 1389 deletions(-) > > create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.c > > create mode 100644 drivers/gpu/drm/msm/dp/dp_hdcp.h -- Sean Paul, Software Engineer, Google / Chromium OS