[PATCH -next] drm/amd/display: check top_pipe_to_program pointer
Clang static analysis reports this error
drivers/gpu/drm/amd/amdgpu/../display/dc/core/dc.c:2870:7: warning:
Dereference of null pointer [clang-analyzer-core.NullDereference]
if
(top_pipe_to_program->stream_res.tg->funcs->lock_doublebuffer_enable) {
^
top_pipe_to_program being NULL is caught as an error
But then it is used to report the error.
So add a check before using it.
Reported-by: Abaci Robot
Signed-off-by: Yang Li
---
drivers/gpu/drm/amd/display/dc/core/dc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 39ad385..34382d0 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -2867,7 +2867,8 @@ static void commit_planes_for_stream(struct dc *dc,
#endif
if ((update_type != UPDATE_TYPE_FAST) &&
stream->update_flags.bits.dsc_changed)
- if
(top_pipe_to_program->stream_res.tg->funcs->lock_doublebuffer_enable) {
+ if (top_pipe_to_program &&
+
top_pipe_to_program->stream_res.tg->funcs->lock_doublebuffer_enable) {
if (should_use_dmub_lock(stream->link)) {
union dmub_hw_lock_flags hw_locks = { 0 };
struct dmub_hw_lock_inst_flags inst_flags = { 0
};
--
1.8.3.1
Re: [PATCH] fbdev: sh7760fb: document fallthrough cases
On Mon, Nov 15, 2021 at 7:33 AM Randy Dunlap wrote:
> Fix fallthrough warnings in sh776fb.c:
>
> ../drivers/video/fbdev/sh7760fb.c: In function 'sh7760fb_get_color_info':
> ../drivers/video/fbdev/sh7760fb.c:138:23: warning: this statement may fall
> through [-Wimplicit-fallthrough=]
> 138 | lgray = 1;
> ../drivers/video/fbdev/sh7760fb.c:143:23: warning: this statement may fall
> through [-Wimplicit-fallthrough=]
> 143 | lgray = 1;
>
> Just document the current state of code execution/flow.
>
> Fixes: 4a25e41831ee ("video: sh7760fb: SH7760/SH7763 LCDC framebuffer driver")
> Signed-off-by: Randy Dunlap
Section 30.4.4 ("Data Format") of the SH7760 Group Hardware
Manual confirms fall-through is appropriate here (especially for
the odd 6 bpp mode).
Reviewed-by: Geert Uytterhoeven
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: Questions about KMS flip
On 2021-11-15 07:41, Lang Yu wrote: > On Fri, Nov 12, 2021 at 05:10:27PM +0100, Michel DDDnzer wrote: >> On 2021-11-12 16:03, Christian König wrote: >>> Am 12.11.21 um 15:30 schrieb Michel Dänzer: On 2021-11-12 15:29, Michel Dänzer wrote: > On 2021-11-12 13:47, Christian König wrote: >> Anyway this unfortunately turned out to be work for Harray and Nicholas. >> In detail it's about this bug report here: >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D214621&data=04%7C01%7CLang.Yu%40amd.com%7Cca557eab16864ab544a108d9a5f6f288%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637723302338811983%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=9oG6k22BVp%2BkSvRX6uMlGXc6G%2BA5UL0Qy3W5iDTpvzs%3D&reserved=0 >> >> Lang was able to reproduce the issue and narrow it down to the pin in >> amdgpu_display_crtc_page_flip_target(). >> >> In other words we somehow have an unbalanced pinning of the scanout >> buffer in DC. > DC doesn't use amdgpu_display_crtc_page_flip_target AFAICT. The > corresponding pin with DC would be in dm_plane_helper_prepare_fb, paired > with the unpin in > dm_plane_helper_cleanup_fb. > > > With non-DC, the pin in amdgpu_display_crtc_page_flip_target is paired > with the unpin in dm_plane_helper_cleanup_fb This should say amdgpu_display_unpin_work_func. >>> >>> Ah! So that is the classic (e.g. non atomic) path? >> >> Presumably. >> >> > & dce_v*_crtc_disable. One thing I notice is that the pin is guarded by > if (!adev->enable_virtual_display), but the unpins seem unconditional. So > could this be about virtual display, and the problem is actually trying > to unpin a BO that was never pinned? >>> >>> Nope, my educated guess is rather that we free up the BO before >>> amdgpu_display_unpin_work_func is called. >>> >>> E.g. not pin unbalance, but rather use after free. >> >> amdgpu_display_crtc_page_flip_target calls amdgpu_bo_ref(work->old_abo), and >> amdgpu_display_unpin_work_func calls amdgpu_bo_unref(&work->old_abo) only >> after amdgpu_bo_unpin. So what you describe could only happen if there's an >> imbalance elsewhere such that amdgpu_bo_unref is called more often than >> amdgpu_bo_ref, or maybe if amdgpu_bo_reserve fails in >> amdgpu_display_unpin_work_func (in which case the "failed to reserve buffer >> after flip" error message should appear in dmesg). > > > Actually, each call to amdgpu_display_crtc_page_flip_target() will > > 1, init a work(amdgpu_display_unpin_work_func) to unpin an old buffer >(crtc->primary->fb), the work will be queued in dce_vX_0_pageflip_irq(). > > 2, pin a new buffer, assign it to crtc->primary->fb. But how to unpin it? >Next call. > > The problem is the pinned buffer of last call to > amdgpu_display_crtc_page_flip_target() is not unpinned. It's unpinned in dce_v*_0_crtc_disable. I think I've found the problem though: dce_v*_0_crtc_do_set_base pin the BO from target_fb unconditionally, but unpin the BO from the fb parameter only if it's different from the former. So if they're the same, the BO's pin count is incremented by 1. -- Earthling Michel Dänzer| https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
[PATCH 00/11] dmaengine: kill off dma_slave_config->slave_id
From: Arnd Bergmann I recently came across some new uses of the 'slave_id' field that I had (almost) removed a few years ago. There are no legitimate uses of this field in the kernel, only a few stale references and two drivers that abuse the field as a side-channel between the dmaengine driver and its client. Let's change the xilinx and qualcomm drivers to use the documented side-channel (peripheral_data) instead, and remove the remnants of it to prevent new users from coming in. As the last patch in the series depends on all the others, it would be nice have everything merged into the dmaengine tree for v5.17. Arnd Arnd Bergmann (11): ASoC: dai_dma: remove slave_id field spi: pic32: stop setting dma_config->slave_id mmc: bcm2835: stop setting chan_config->slave_id dmaengine: shdma: remove legacy slave_id parsing dmaengine: pxa/mmp: stop referencing config->slave_id dmaengine: sprd: stop referencing config->slave_id dmaengine: qcom-adm: stop abusing slave_id config dmaengine: xilinx_dpdma: stop using slave_id field dmaengine: tegra20-apb: stop checking config->slave_id staging: ralink-gdma: stop using slave_id config dmaengine: remove slave_id config field drivers/dma/mmp_pdma.c| 6 --- drivers/dma/pxa_dma.c | 7 --- drivers/dma/qcom/qcom_adm.c | 56 --- drivers/dma/sh/shdma-base.c | 8 drivers/dma/sprd-dma.c| 3 -- drivers/dma/tegra20-apb-dma.c | 6 --- drivers/dma/xilinx/xilinx_dpdma.c | 12 +++-- drivers/gpu/drm/xlnx/zynqmp_disp.c| 9 +++- drivers/mmc/host/bcm2835.c| 2 - drivers/mtd/nand/raw/qcom_nandc.c | 14 +- drivers/spi/spi-pic32.c | 2 - drivers/staging/ralink-gdma/ralink-gdma.c | 12 ++--- drivers/tty/serial/msm_serial.c | 15 +- include/linux/dma/qcom_adm.h | 12 + include/linux/dma/xilinx_dpdma.h | 11 + include/linux/dmaengine.h | 4 -- include/sound/dmaengine_pcm.h | 2 - sound/core/pcm_dmaengine.c| 5 +- sound/soc/tegra/tegra20_spdif.c | 1 - 19 files changed, 119 insertions(+), 68 deletions(-) create mode 100644 include/linux/dma/qcom_adm.h create mode 100644 include/linux/dma/xilinx_dpdma.h -- 2.30.2 Cc: Andy Gross Cc: Andy Shevchenko Cc: Arnd Bergmann Cc: Baolin Wang Cc: Bjorn Andersson Cc: Chunyan Zhang Cc: Greg Kroah-Hartman Cc: Hyun Kwon Cc: Jaroslav Kysela Cc: Jon Hunter Cc: Lars-Peter Clausen Cc: Laurent Pinchart Cc: Laxman Dewangan Cc: Manivannan Sadhasivam Cc: Mark Brown Cc: Michal Simek Cc: Nicolas Saenz Julienne Cc: Orson Zhai Cc: Robert Jarzmik Cc: Scott Branden Cc: Takashi Iwai Cc: Thierry Reding Cc: Vinod Koul Cc: alsa-de...@alsa-project.org Cc: bcm-kernel-feedback-l...@broadcom.com Cc: dmaeng...@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: linux-arm-ker...@lists.infradead.org Cc: linux-arm-...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux-...@lists.infradead.org Cc: linux-rpi-ker...@lists.infradead.org Cc: linux-ser...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux-stag...@lists.linux.dev Cc: linux-te...@vger.kernel.org
[PATCH 01/11] ASoC: dai_dma: remove slave_id field
From: Arnd Bergmann
This field is never set, and serves no purpose, so remove it.
Signed-off-by: Arnd Bergmann
---
include/sound/dmaengine_pcm.h | 2 --
sound/core/pcm_dmaengine.c | 5 ++---
sound/soc/tegra/tegra20_spdif.c | 1 -
3 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
index 9144bd547851..7403870c28bd 100644
--- a/include/sound/dmaengine_pcm.h
+++ b/include/sound/dmaengine_pcm.h
@@ -58,7 +58,6 @@ struct dma_chan *snd_dmaengine_pcm_get_chan(struct
snd_pcm_substream *substream)
* @maxburst: Maximum number of words(note: words, as in units of the
* src_addr_width member, not bytes) that can be send to or received from the
* DAI in one burst.
- * @slave_id: Slave requester id for the DMA channel.
* @filter_data: Custom DMA channel filter data, this will usually be used when
* requesting the DMA channel.
* @chan_name: Custom channel name to use when requesting DMA channel.
@@ -72,7 +71,6 @@ struct snd_dmaengine_dai_dma_data {
dma_addr_t addr;
enum dma_slave_buswidth addr_width;
u32 maxburst;
- unsigned int slave_id;
void *filter_data;
const char *chan_name;
unsigned int fifo_size;
diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c
index af08bb4bf578..6762fb2083e1 100644
--- a/sound/core/pcm_dmaengine.c
+++ b/sound/core/pcm_dmaengine.c
@@ -91,8 +91,8 @@ EXPORT_SYMBOL_GPL(snd_hwparams_to_dma_slave_config);
* @dma_data: DAI DMA data
* @slave_config: DMA slave configuration
*
- * Initializes the {dst,src}_addr, {dst,src}_maxburst, {dst,src}_addr_width and
- * slave_id fields of the DMA slave config from the same fields of the DAI DMA
+ * Initializes the {dst,src}_addr, {dst,src}_maxburst, {dst,src}_addr_width
+ * fields of the DMA slave config from the same fields of the DAI DMA
* data struct. The src and dst fields will be initialized depending on the
* direction of the substream. If the substream is a playback stream the dst
* fields will be initialized, if it is a capture stream the src fields will be
@@ -124,7 +124,6 @@ void snd_dmaengine_pcm_set_config_from_dai_data(
slave_config->src_addr_width = dma_data->addr_width;
}
- slave_config->slave_id = dma_data->slave_id;
slave_config->peripheral_config = dma_data->peripheral_config;
slave_config->peripheral_size = dma_data->peripheral_size;
}
diff --git a/sound/soc/tegra/tegra20_spdif.c b/sound/soc/tegra/tegra20_spdif.c
index 9fdc82d58db3..1c3385da6f82 100644
--- a/sound/soc/tegra/tegra20_spdif.c
+++ b/sound/soc/tegra/tegra20_spdif.c
@@ -284,7 +284,6 @@ static int tegra20_spdif_platform_probe(struct
platform_device *pdev)
spdif->playback_dma_data.addr = mem->start + TEGRA20_SPDIF_DATA_OUT;
spdif->playback_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
spdif->playback_dma_data.maxburst = 4;
- spdif->playback_dma_data.slave_id = dmareq->start;
pm_runtime_enable(&pdev->dev);
--
2.30.2
[PATCH 02/11] spi: pic32: stop setting dma_config->slave_id
From: Arnd Bergmann
Setting slave_id makes no sense with DT based probing, and
should eventually get removed entirely. Address this driver
by no longer setting the field here.
I could not find which DMA driver is used on PIC32, if it's
in the tree at all, but none of the obvious ones even care
about slave_id any more.
Signed-off-by: Arnd Bergmann
---
drivers/spi/spi-pic32.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/spi/spi-pic32.c b/drivers/spi/spi-pic32.c
index 5eb7b61bbb4d..f86433b29260 100644
--- a/drivers/spi/spi-pic32.c
+++ b/drivers/spi/spi-pic32.c
@@ -370,7 +370,6 @@ static int pic32_spi_dma_config(struct pic32_spi *pic32s,
u32 dma_width)
cfg.src_addr_width = dma_width;
cfg.dst_addr_width = dma_width;
/* tx channel */
- cfg.slave_id = pic32s->tx_irq;
cfg.direction = DMA_MEM_TO_DEV;
ret = dmaengine_slave_config(master->dma_tx, &cfg);
if (ret) {
@@ -378,7 +377,6 @@ static int pic32_spi_dma_config(struct pic32_spi *pic32s,
u32 dma_width)
return ret;
}
/* rx channel */
- cfg.slave_id = pic32s->rx_irq;
cfg.direction = DMA_DEV_TO_MEM;
ret = dmaengine_slave_config(master->dma_rx, &cfg);
if (ret)
--
2.30.2
[PATCH 03/11] mmc: bcm2835: stop setting chan_config->slave_id
From: Arnd Bergmann The field is not interpreted by the DMA engine driver, as all the data is passed from devicetree instead. Remove the assignment so the field can eventually be deleted. Signed-off-by: Arnd Bergmann --- drivers/mmc/host/bcm2835.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/mmc/host/bcm2835.c b/drivers/mmc/host/bcm2835.c index 8c2361e66277..463b707d9e99 100644 --- a/drivers/mmc/host/bcm2835.c +++ b/drivers/mmc/host/bcm2835.c @@ -1293,14 +1293,12 @@ static int bcm2835_add_host(struct bcm2835_host *host) host->dma_cfg_tx.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; host->dma_cfg_tx.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; - host->dma_cfg_tx.slave_id = 13; /* DREQ channel */ host->dma_cfg_tx.direction = DMA_MEM_TO_DEV; host->dma_cfg_tx.src_addr = 0; host->dma_cfg_tx.dst_addr = host->phys_addr + SDDATA; host->dma_cfg_rx.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; host->dma_cfg_rx.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; - host->dma_cfg_rx.slave_id = 13; /* DREQ channel */ host->dma_cfg_rx.direction = DMA_DEV_TO_MEM; host->dma_cfg_rx.src_addr = host->phys_addr + SDDATA; host->dma_cfg_rx.dst_addr = 0; -- 2.30.2
[PATCH 04/11] dmaengine: shdma: remove legacy slave_id parsing
From: Arnd Bergmann The slave device is picked through either devicetree or a filter function, and any remaining out-of-tree drivers would have warned about this usage since 2015. Stop interpreting the field finally so it can be removed from the interface. Signed-off-by: Arnd Bergmann --- drivers/dma/sh/shdma-base.c | 8 1 file changed, 8 deletions(-) diff --git a/drivers/dma/sh/shdma-base.c b/drivers/dma/sh/shdma-base.c index 7f72b3f4cd1a..41c6bc650fa3 100644 --- a/drivers/dma/sh/shdma-base.c +++ b/drivers/dma/sh/shdma-base.c @@ -786,14 +786,6 @@ static int shdma_config(struct dma_chan *chan, if (!config) return -EINVAL; - /* -* overriding the slave_id through dma_slave_config is deprecated, -* but possibly some out-of-tree drivers still do it. -*/ - if (WARN_ON_ONCE(config->slave_id && -config->slave_id != schan->real_slave_id)) - schan->real_slave_id = config->slave_id; - /* * We could lock this, but you shouldn't be configuring the * channel, while using it... -- 2.30.2
[PATCH 05/11] dmaengine: pxa/mmp: stop referencing config->slave_id
From: Arnd Bergmann
The last driver referencing the slave_id on Marvell PXA and MMP platforms
was the SPI driver, but this stopped doing so a long time ago, so the
TODO from the earlier patch can no be removed.
Fixes: b729bf34535e ("spi/pxa2xx: Don't use slave_id of dma_slave_config")
Fixes: 13b3006b8ebd ("dma: mmp_pdma: add filter function")
Signed-off-by: Arnd Bergmann
---
drivers/dma/mmp_pdma.c | 6 --
drivers/dma/pxa_dma.c | 7 ---
2 files changed, 13 deletions(-)
diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c
index a23563cd118b..5a53d7fcef01 100644
--- a/drivers/dma/mmp_pdma.c
+++ b/drivers/dma/mmp_pdma.c
@@ -727,12 +727,6 @@ static int mmp_pdma_config_write(struct dma_chan *dchan,
chan->dir = direction;
chan->dev_addr = addr;
- /* FIXME: drivers should be ported over to use the filter
-* function. Once that's done, the following two lines can
-* be removed.
-*/
- if (cfg->slave_id)
- chan->drcmr = cfg->slave_id;
return 0;
}
diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
index 52d04641e361..6078cc81892e 100644
--- a/drivers/dma/pxa_dma.c
+++ b/drivers/dma/pxa_dma.c
@@ -909,13 +909,6 @@ static void pxad_get_config(struct pxad_chan *chan,
*dcmd |= PXA_DCMD_BURST16;
else if (maxburst == 32)
*dcmd |= PXA_DCMD_BURST32;
-
- /* FIXME: drivers should be ported over to use the filter
-* function. Once that's done, the following two lines can
-* be removed.
-*/
- if (chan->cfg.slave_id)
- chan->drcmr = chan->cfg.slave_id;
}
static struct dma_async_tx_descriptor *
--
2.30.2
[PATCH 06/11] dmaengine: sprd: stop referencing config->slave_id
From: Arnd Bergmann It appears that the code that reads the slave_id from the channel config was copied incorrectly from other drivers. Nothing ever sets this field on platforms that use this driver, so remove the reference. Signed-off-by: Arnd Bergmann --- drivers/dma/sprd-dma.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c index 4357d2395e6b..7f158ef5672d 100644 --- a/drivers/dma/sprd-dma.c +++ b/drivers/dma/sprd-dma.c @@ -795,9 +795,6 @@ static int sprd_dma_fill_desc(struct dma_chan *chan, return dst_datawidth; } - if (slave_cfg->slave_id) - schan->dev_id = slave_cfg->slave_id; - hw->cfg = SPRD_DMA_DONOT_WAIT_BDONE << SPRD_DMA_WAIT_BDONE_OFFSET; /* -- 2.30.2
[PATCH 07/11] dmaengine: qcom-adm: stop abusing slave_id config
From: Arnd Bergmann
The slave_id was previously used to pick one DMA slave instead of another,
but this is now done through the DMA descriptors in device tree.
For the qcom_adm driver, the configuration is documented in the DT
binding to contain a tuple of device identifier and a "crci" field,
but the implementation ends up using only a single cell for identifying
the slave, with the crci getting passed in nonstandard properties of
the device, and passed through the dma driver using the old slave_id
field. Part of the problem apparently is that the nand driver ends up
using only a single DMA request ID, but requires distinct values for
"crci" depending on the type of transfer.
Change both the dmaengine driver and the two slave drivers to allow
the documented binding to work in addition to the ad-hoc passing
of crci values. In order to no longer abuse the slave_id field, pass
the data using the "peripheral_config" mechanism instead.
Signed-off-by: Arnd Bergmann
---
drivers/dma/qcom/qcom_adm.c | 56 +++
drivers/mtd/nand/raw/qcom_nandc.c | 14 ++--
drivers/tty/serial/msm_serial.c | 15 +++--
include/linux/dma/qcom_adm.h | 12 +++
4 files changed, 86 insertions(+), 11 deletions(-)
create mode 100644 include/linux/dma/qcom_adm.h
diff --git a/drivers/dma/qcom/qcom_adm.c b/drivers/dma/qcom/qcom_adm.c
index ee78bed8d60d..bb338b303af6 100644
--- a/drivers/dma/qcom/qcom_adm.c
+++ b/drivers/dma/qcom/qcom_adm.c
@@ -8,6 +8,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -140,6 +141,8 @@ struct adm_chan {
struct adm_async_desc *curr_txd;
struct dma_slave_config slave;
+ u32 crci;
+ u32 mux;
struct list_head node;
int error;
@@ -379,8 +382,8 @@ static struct dma_async_tx_descriptor
*adm_prep_slave_sg(struct dma_chan *chan,
return ERR_PTR(-EINVAL);
}
- crci = achan->slave.slave_id & 0xf;
- if (!crci || achan->slave.slave_id > 0x1f) {
+ crci = achan->crci & 0xf;
+ if (!crci || achan->crci > 0x1f) {
dev_err(adev->dev, "invalid crci value\n");
return ERR_PTR(-EINVAL);
}
@@ -403,9 +406,7 @@ static struct dma_async_tx_descriptor
*adm_prep_slave_sg(struct dma_chan *chan,
if (!async_desc)
return ERR_PTR(-ENOMEM);
- if (crci)
- async_desc->mux = achan->slave.slave_id & ADM_CRCI_MUX_SEL ?
- ADM_CRCI_CTL_MUX_SEL : 0;
+ async_desc->mux = achan->mux ? ADM_CRCI_CTL_MUX_SEL : 0;
async_desc->crci = crci;
async_desc->blk_size = blk_size;
async_desc->dma_len = single_count * sizeof(struct adm_desc_hw_single) +
@@ -488,10 +489,13 @@ static int adm_terminate_all(struct dma_chan *chan)
static int adm_slave_config(struct dma_chan *chan, struct dma_slave_config
*cfg)
{
struct adm_chan *achan = to_adm_chan(chan);
+ struct qcom_adm_peripheral_config *config = cfg->peripheral_config;
unsigned long flag;
spin_lock_irqsave(&achan->vc.lock, flag);
memcpy(&achan->slave, cfg, sizeof(struct dma_slave_config));
+ if (cfg->peripheral_size == sizeof(config))
+ achan->crci = config->crci;
spin_unlock_irqrestore(&achan->vc.lock, flag);
return 0;
@@ -694,6 +698,45 @@ static void adm_channel_init(struct adm_device *adev,
struct adm_chan *achan,
achan->vc.desc_free = adm_dma_free_desc;
}
+/**
+ * adm_dma_xlate
+ * @dma_spec: pointer to DMA specifier as found in the device tree
+ * @ofdma: pointer to DMA controller data
+ *
+ * This can use either 1-cell or 2-cell formats, the first cell
+ * identifies the slave device, while the optional second cell
+ * contains the crci value.
+ *
+ * Returns pointer to appropriate dma channel on success or NULL on error.
+ */
+struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec,
+ struct of_dma *ofdma)
+{
+ struct dma_device *dev = ofdma->of_dma_data;
+ struct dma_chan *chan, *candidate = NULL;
+ struct adm_chan *achan;
+
+ if (!dev || dma_spec->args_count > 2)
+ return NULL;
+
+ list_for_each_entry(chan, &dev->channels, device_node)
+ if (chan->chan_id == dma_spec->args[0]) {
+ candidate = chan;
+ break;
+ }
+
+ if (!candidate)
+ return NULL;
+
+ achan = to_adm_chan(candidate);
+ if (dma_spec->args_count == 2)
+ achan->crci = dma_spec->args[1];
+ else
+ achan->crci = 0;
+
+ return dma_get_slave_channel(candidate);
+}
+
static int adm_dma_probe(struct platform_device *pdev)
{
struct adm_device *adev;
@@ -838,8 +881,7 @@ static int adm_dma_probe(struct platform_device *pdev)
[PATCH 08/11] dmaengine: xilinx_dpdma: stop using slave_id field
From: Arnd Bergmann
The display driver wants to pass a custom flag to the DMA engine driver,
which it started doing by using the slave_id field that was traditionally
used for a different purpose.
As there is no longer a correct use for the slave_id field, it should
really be removed, and the remaining users changed over to something
different.
The new mechanism for passing nonstandard settings is using the
.peripheral_config field, so use that to pass a newly defined structure
here, making it clear that this will not work in portable drivers.
Signed-off-by: Arnd Bergmann
---
drivers/dma/xilinx/xilinx_dpdma.c | 12
drivers/gpu/drm/xlnx/zynqmp_disp.c | 9 +++--
include/linux/dma/xilinx_dpdma.h | 11 +++
3 files changed, 26 insertions(+), 6 deletions(-)
create mode 100644 include/linux/dma/xilinx_dpdma.h
diff --git a/drivers/dma/xilinx/xilinx_dpdma.c
b/drivers/dma/xilinx/xilinx_dpdma.c
index ce5c66e6897d..e2c1ef7a659c 100644
--- a/drivers/dma/xilinx/xilinx_dpdma.c
+++ b/drivers/dma/xilinx/xilinx_dpdma.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -1273,6 +1274,7 @@ static int xilinx_dpdma_config(struct dma_chan *dchan,
struct dma_slave_config *config)
{
struct xilinx_dpdma_chan *chan = to_xilinx_chan(dchan);
+ struct xilinx_dpdma_peripheral_config *pconfig;
unsigned long flags;
/*
@@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan,
spin_lock_irqsave(&chan->lock, flags);
/*
-* Abuse the slave_id to indicate that the channel is part of a video
-* group.
+* Abuse the peripheral_config to indicate that the channel is part
+* of a video group.
*/
- if (chan->id <= ZYNQMP_DPDMA_VIDEO2)
- chan->video_group = config->slave_id != 0;
+ pconfig = config->peripheral_config;
+ if (chan->id <= ZYNQMP_DPDMA_VIDEO2 &&
+ config->peripheral_size == sizeof(*pconfig))
+ chan->video_group = pconfig->video_group;
spin_unlock_irqrestore(&chan->lock, flags);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
b/drivers/gpu/drm/xlnx/zynqmp_disp.c
index ff2b308d8651..11c409cbc88e 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
@@ -24,6 +24,7 @@
#include
#include
+#include
#include
#include
#include
@@ -1058,14 +1059,18 @@ static void zynqmp_disp_layer_set_format(struct
zynqmp_disp_layer *layer,
zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
/*
-* Set slave_id for each DMA channel to indicate they're part of a
+* Set pconfig for each DMA channel to indicate they're part of a
* video group.
*/
for (i = 0; i < info->num_planes; i++) {
struct zynqmp_disp_layer_dma *dma = &layer->dmas[i];
+ struct xilinx_dpdma_peripheral_config pconfig = {
+ .video_group = true,
+ };
struct dma_slave_config config = {
.direction = DMA_MEM_TO_DEV,
- .slave_id = 1,
+ .peripheral_config = &pconfig,
+ .peripheral_size = sizeof(pconfig),
};
dmaengine_slave_config(dma->chan, &config);
diff --git a/include/linux/dma/xilinx_dpdma.h b/include/linux/dma/xilinx_dpdma.h
new file mode 100644
index ..83a1377f03f8
--- /dev/null
+++ b/include/linux/dma/xilinx_dpdma.h
@@ -0,0 +1,11 @@
+// SPDX-License-Identifier: GPL-2.0
+#ifndef __LINUX_DMA_XILINX_DPDMA_H
+#define __LINUX_DMA_XILINX_DPDMA_H
+
+#include
+
+struct xilinx_dpdma_peripheral_config {
+ bool video_group;
+};
+
+#endif /* __LINUX_DMA_XILINX_DPDMA_H */
--
2.30.2
[PATCH 09/11] dmaengine: tegra20-apb: stop checking config->slave_id
From: Arnd Bergmann
Nothing sets the slave_id field any more, so stop accessing
it to allow the removal of this field.
Signed-off-by: Arnd Bergmann
---
drivers/dma/tegra20-apb-dma.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index b7260749e8ee..eaafcbe4ca94 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -343,12 +343,6 @@ static int tegra_dma_slave_config(struct dma_chan *dc,
}
memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig));
- if (tdc->slave_id == TEGRA_APBDMA_SLAVE_ID_INVALID &&
- sconfig->device_fc) {
- if (sconfig->slave_id > TEGRA_APBDMA_CSR_REQ_SEL_MASK)
- return -EINVAL;
- tdc->slave_id = sconfig->slave_id;
- }
tdc->config_init = true;
return 0;
--
2.30.2
[PATCH 10/11] staging: ralink-gdma: stop using slave_id config
From: Arnd Bergmann
Picking the connection between a DMA controller and its attached device
is done through devicetree using the "dmas" property, which is implemented
by the gdma driver, but it also allows overriding the "req" configuration
with the slave_id field, as it was done in some linux-2.6 era drivers.
There is no driver in the tree that sets these values, so stop
interpreting them before anything accidentally starts relying on it.
Rename the field in the channel from "slave_id" to "req" to better match
the purpose and the naming in the hardware.
If any driver actually starts using this DMA engine, it may be necessary
to implement a .xlate callback that sets this field at probe time.
Signed-off-by: Arnd Bergmann
---
drivers/staging/ralink-gdma/ralink-gdma.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/ralink-gdma/ralink-gdma.c
b/drivers/staging/ralink-gdma/ralink-gdma.c
index b5229bc6eae5..f00240e62e1b 100644
--- a/drivers/staging/ralink-gdma/ralink-gdma.c
+++ b/drivers/staging/ralink-gdma/ralink-gdma.c
@@ -106,7 +106,7 @@ struct gdma_dma_desc {
struct gdma_dmaengine_chan {
struct virt_dma_chan vchan;
unsigned int id;
- unsigned int slave_id;
+ unsigned int req;
dma_addr_t fifo_addr;
enum gdma_dma_transfer_size burst_size;
@@ -194,7 +194,6 @@ static int gdma_dma_config(struct dma_chan *c,
dev_err(dma_dev->ddev.dev, "only support 4 byte
buswidth\n");
return -EINVAL;
}
- chan->slave_id = config->slave_id;
chan->fifo_addr = config->dst_addr;
chan->burst_size = gdma_dma_maxburst(config->dst_maxburst);
break;
@@ -203,7 +202,6 @@ static int gdma_dma_config(struct dma_chan *c,
dev_err(dma_dev->ddev.dev, "only support 4 byte
buswidth\n");
return -EINVAL;
}
- chan->slave_id = config->slave_id;
chan->fifo_addr = config->src_addr;
chan->burst_size = gdma_dma_maxburst(config->src_maxburst);
break;
@@ -288,12 +286,12 @@ static int rt305x_gdma_start_transfer(struct
gdma_dmaengine_chan *chan)
dst_addr = chan->fifo_addr;
ctrl0 = GDMA_REG_CTRL0_DST_ADDR_FIXED |
(8 << GDMA_RT305X_CTRL0_SRC_REQ_SHIFT) |
- (chan->slave_id << GDMA_RT305X_CTRL0_DST_REQ_SHIFT);
+ (chan->req << GDMA_RT305X_CTRL0_DST_REQ_SHIFT);
} else if (chan->desc->direction == DMA_DEV_TO_MEM) {
src_addr = chan->fifo_addr;
dst_addr = sg->dst_addr;
ctrl0 = GDMA_REG_CTRL0_SRC_ADDR_FIXED |
- (chan->slave_id << GDMA_RT305X_CTRL0_SRC_REQ_SHIFT) |
+ (chan->req << GDMA_RT305X_CTRL0_SRC_REQ_SHIFT) |
(8 << GDMA_RT305X_CTRL0_DST_REQ_SHIFT);
} else if (chan->desc->direction == DMA_MEM_TO_MEM) {
/*
@@ -365,12 +363,12 @@ static int rt3883_gdma_start_transfer(struct
gdma_dmaengine_chan *chan)
dst_addr = chan->fifo_addr;
ctrl0 = GDMA_REG_CTRL0_DST_ADDR_FIXED;
ctrl1 = (32 << GDMA_REG_CTRL1_SRC_REQ_SHIFT) |
- (chan->slave_id << GDMA_REG_CTRL1_DST_REQ_SHIFT);
+ (chan->req << GDMA_REG_CTRL1_DST_REQ_SHIFT);
} else if (chan->desc->direction == DMA_DEV_TO_MEM) {
src_addr = chan->fifo_addr;
dst_addr = sg->dst_addr;
ctrl0 = GDMA_REG_CTRL0_SRC_ADDR_FIXED;
- ctrl1 = (chan->slave_id << GDMA_REG_CTRL1_SRC_REQ_SHIFT) |
+ ctrl1 = (chan->req << GDMA_REG_CTRL1_SRC_REQ_SHIFT) |
(32 << GDMA_REG_CTRL1_DST_REQ_SHIFT) |
GDMA_REG_CTRL1_COHERENT;
} else if (chan->desc->direction == DMA_MEM_TO_MEM) {
--
2.30.2
[PATCH 11/11] dmaengine: remove slave_id config field
From: Arnd Bergmann
All references to the slave_id field have been removed, so
remove the field as well to prevent new references from
creeping in again.
Signed-off-by: Arnd Bergmann
---
include/linux/dmaengine.h | 4
1 file changed, 4 deletions(-)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index 9000f3ffce8b..0349b35235e6 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -418,9 +418,6 @@ enum dma_slave_buswidth {
* @device_fc: Flow Controller Settings. Only valid for slave channels. Fill
* with 'true' if peripheral should be flow controller. Direction will be
* selected at Runtime.
- * @slave_id: Slave requester id. Only valid for slave channels. The dma
- * slave peripheral will have unique id as dma requester which need to be
- * pass as slave config.
* @peripheral_config: peripheral configuration for programming peripheral
* for dmaengine transfer
* @peripheral_size: peripheral configuration buffer size
@@ -448,7 +445,6 @@ struct dma_slave_config {
u32 src_port_window_size;
u32 dst_port_window_size;
bool device_fc;
- unsigned int slave_id;
void *peripheral_config;
size_t peripheral_size;
};
--
2.30.2
Re: [PATCH 04/11] dmaengine: shdma: remove legacy slave_id parsing
Hi Arnd, Thank you for the patch. On Mon, Nov 15, 2021 at 09:53:56AM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann > > The slave device is picked through either devicetree or a filter > function, and any remaining out-of-tree drivers would have warned > about this usage since 2015. > > Stop interpreting the field finally so it can be removed from > the interface. > > Signed-off-by: Arnd Bergmann Reviewed-by: Laurent Pinchart > --- > drivers/dma/sh/shdma-base.c | 8 > 1 file changed, 8 deletions(-) > > diff --git a/drivers/dma/sh/shdma-base.c b/drivers/dma/sh/shdma-base.c > index 7f72b3f4cd1a..41c6bc650fa3 100644 > --- a/drivers/dma/sh/shdma-base.c > +++ b/drivers/dma/sh/shdma-base.c > @@ -786,14 +786,6 @@ static int shdma_config(struct dma_chan *chan, > if (!config) > return -EINVAL; > > - /* > - * overriding the slave_id through dma_slave_config is deprecated, > - * but possibly some out-of-tree drivers still do it. > - */ > - if (WARN_ON_ONCE(config->slave_id && > - config->slave_id != schan->real_slave_id)) > - schan->real_slave_id = config->slave_id; > - > /* >* We could lock this, but you shouldn't be configuring the >* channel, while using it... -- Regards, Laurent Pinchart
Re: [PATCH 08/11] dmaengine: xilinx_dpdma: stop using slave_id field
Hi Arnd,
Thank you for the patch.
On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> The display driver wants to pass a custom flag to the DMA engine driver,
> which it started doing by using the slave_id field that was traditionally
> used for a different purpose.
>
> As there is no longer a correct use for the slave_id field, it should
> really be removed, and the remaining users changed over to something
> different.
>
> The new mechanism for passing nonstandard settings is using the
> .peripheral_config field, so use that to pass a newly defined structure
> here, making it clear that this will not work in portable drivers.
>
> Signed-off-by: Arnd Bergmann
> ---
> drivers/dma/xilinx/xilinx_dpdma.c | 12
> drivers/gpu/drm/xlnx/zynqmp_disp.c | 9 +++--
> include/linux/dma/xilinx_dpdma.h | 11 +++
> 3 files changed, 26 insertions(+), 6 deletions(-)
> create mode 100644 include/linux/dma/xilinx_dpdma.h
>
> diff --git a/drivers/dma/xilinx/xilinx_dpdma.c
> b/drivers/dma/xilinx/xilinx_dpdma.c
> index ce5c66e6897d..e2c1ef7a659c 100644
> --- a/drivers/dma/xilinx/xilinx_dpdma.c
> +++ b/drivers/dma/xilinx/xilinx_dpdma.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -1273,6 +1274,7 @@ static int xilinx_dpdma_config(struct dma_chan *dchan,
> struct dma_slave_config *config)
> {
> struct xilinx_dpdma_chan *chan = to_xilinx_chan(dchan);
> + struct xilinx_dpdma_peripheral_config *pconfig;
> unsigned long flags;
>
> /*
> @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan,
> spin_lock_irqsave(&chan->lock, flags);
>
> /*
> - * Abuse the slave_id to indicate that the channel is part of a video
> - * group.
> + * Abuse the peripheral_config to indicate that the channel is part
Is it still an abuse, or is this now the right way to pass custom data
to the DMA engine driver ?
> + * of a video group.
>*/
> - if (chan->id <= ZYNQMP_DPDMA_VIDEO2)
> - chan->video_group = config->slave_id != 0;
> + pconfig = config->peripheral_config;
This could be moved to the variable declaration above, up to you.
> + if (chan->id <= ZYNQMP_DPDMA_VIDEO2 &&
> + config->peripheral_size == sizeof(*pconfig))
Silently ignoring a size mismatch isn't nice. Could we validate the size
at the beginning of the function and return an error ?
With these issues addressed,
Reviewed-by: Laurent Pinchart
> + chan->video_group = pconfig->video_group;
>
> spin_unlock_irqrestore(&chan->lock, flags);
>
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index ff2b308d8651..11c409cbc88e 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -24,6 +24,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -1058,14 +1059,18 @@ static void zynqmp_disp_layer_set_format(struct
> zynqmp_disp_layer *layer,
> zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
>
> /*
> - * Set slave_id for each DMA channel to indicate they're part of a
> + * Set pconfig for each DMA channel to indicate they're part of a
>* video group.
>*/
> for (i = 0; i < info->num_planes; i++) {
> struct zynqmp_disp_layer_dma *dma = &layer->dmas[i];
> + struct xilinx_dpdma_peripheral_config pconfig = {
> + .video_group = true,
> + };
> struct dma_slave_config config = {
> .direction = DMA_MEM_TO_DEV,
> - .slave_id = 1,
> + .peripheral_config = &pconfig,
> + .peripheral_size = sizeof(pconfig),
> };
>
> dmaengine_slave_config(dma->chan, &config);
> diff --git a/include/linux/dma/xilinx_dpdma.h
> b/include/linux/dma/xilinx_dpdma.h
> new file mode 100644
> index ..83a1377f03f8
> --- /dev/null
> +++ b/include/linux/dma/xilinx_dpdma.h
> @@ -0,0 +1,11 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#ifndef __LINUX_DMA_XILINX_DPDMA_H
> +#define __LINUX_DMA_XILINX_DPDMA_H
> +
> +#include
> +
> +struct xilinx_dpdma_peripheral_config {
> + bool video_group;
> +};
> +
> +#endif /* __LINUX_DMA_XILINX_DPDMA_H */
--
Regards,
Laurent Pinchart
Re: [PATCH] drm: pre-fill getfb2 modifier array with INVALID
On Thursday, November 11th, 2021 at 13:50, Ville Syrjälä wrote: > On Thu, Nov 11, 2021 at 10:10:54AM +, Simon Ser wrote: > > User-space shouldn't look up the modifier array when the modifier > > flag is missing, but at the moment no docs make this clear (working > > on it). Right now the modifier array is pre-filled with zeroes, aka. > > LINEAR. Instead, pre-fill with INVALID to avoid footguns. > > > > This is a uAPI change, but OTOH any user-space which looks up the > > modifier array without checking the flag is broken already, so > > should be fine. > > > > Signed-off-by: Simon Ser > > Cc: Daniel Vetter > > Cc: Pekka Paalanen > > Cc: Daniel Stone > > Isn't this going to break the test where we pass the get > getfb2 result back into addfb2? Shouldn't be the case, since the kernel will ignore modifiers if the flag isn't set.
Re: [PATCH 11/11] dmaengine: remove slave_id config field
Hi Arnd,
Thank you for the patch.
On Mon, Nov 15, 2021 at 09:54:03AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> All references to the slave_id field have been removed, so
> remove the field as well to prevent new references from
> creeping in again.
A rationale to explain why the slave_id field shouldn't be used would be
nice.
> Signed-off-by: Arnd Bergmann
Reviewed-by: Laurent Pinchart
> ---
> include/linux/dmaengine.h | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
> index 9000f3ffce8b..0349b35235e6 100644
> --- a/include/linux/dmaengine.h
> +++ b/include/linux/dmaengine.h
> @@ -418,9 +418,6 @@ enum dma_slave_buswidth {
> * @device_fc: Flow Controller Settings. Only valid for slave channels. Fill
> * with 'true' if peripheral should be flow controller. Direction will be
> * selected at Runtime.
> - * @slave_id: Slave requester id. Only valid for slave channels. The dma
> - * slave peripheral will have unique id as dma requester which need to be
> - * pass as slave config.
> * @peripheral_config: peripheral configuration for programming peripheral
> * for dmaengine transfer
> * @peripheral_size: peripheral configuration buffer size
> @@ -448,7 +445,6 @@ struct dma_slave_config {
> u32 src_port_window_size;
> u32 dst_port_window_size;
> bool device_fc;
> - unsigned int slave_id;
> void *peripheral_config;
> size_t peripheral_size;
> };
--
Regards,
Laurent Pinchart
Re: Questions about KMS flip
On 2021-11-15 10:04, Lang Yu wrote: > On Mon, Nov 15, 2021 at 09:38:47AM +0100, Michel DDDnzer wrote: >> On 2021-11-15 07:41, Lang Yu wrote: >>> On Fri, Nov 12, 2021 at 05:10:27PM +0100, Michel DDDnzer wrote: On 2021-11-12 16:03, Christian König wrote: > Am 12.11.21 um 15:30 schrieb Michel Dänzer: >> On 2021-11-12 15:29, Michel Dänzer wrote: >>> On 2021-11-12 13:47, Christian König wrote: Anyway this unfortunately turned out to be work for Harray and Nicholas. In detail it's about this bug report here: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D214621&data=04%7C01%7CLang.Yu%40amd.com%7C766e41a1c3052b6b08d9a81358b0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637725623316543180%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=od%2BNksWOff%2FtsuAYZLX7lIJFQJMY2OScVqhLclPYWAQ%3D&reserved=0 Lang was able to reproduce the issue and narrow it down to the pin in amdgpu_display_crtc_page_flip_target(). In other words we somehow have an unbalanced pinning of the scanout buffer in DC. >>> DC doesn't use amdgpu_display_crtc_page_flip_target AFAICT. The >>> corresponding pin with DC would be in dm_plane_helper_prepare_fb, >>> paired with the unpin in >>> dm_plane_helper_cleanup_fb. >>> >>> >>> With non-DC, the pin in amdgpu_display_crtc_page_flip_target is paired >>> with the unpin in dm_plane_helper_cleanup_fb >> This should say amdgpu_display_unpin_work_func. > > Ah! So that is the classic (e.g. non atomic) path? Presumably. >>> & dce_v*_crtc_disable. One thing I notice is that the pin is guarded by >>> if (!adev->enable_virtual_display), but the unpins seem unconditional. >>> So could this be about virtual display, and the problem is actually >>> trying to unpin a BO that was never pinned? > > Nope, my educated guess is rather that we free up the BO before > amdgpu_display_unpin_work_func is called. > > E.g. not pin unbalance, but rather use after free. amdgpu_display_crtc_page_flip_target calls amdgpu_bo_ref(work->old_abo), and amdgpu_display_unpin_work_func calls amdgpu_bo_unref(&work->old_abo) only after amdgpu_bo_unpin. So what you describe could only happen if there's an imbalance elsewhere such that amdgpu_bo_unref is called more often than amdgpu_bo_ref, or maybe if amdgpu_bo_reserve fails in amdgpu_display_unpin_work_func (in which case the "failed to reserve buffer after flip" error message should appear in dmesg). >>> >>> >>> Actually, each call to amdgpu_display_crtc_page_flip_target() will >>> >>> 1, init a work(amdgpu_display_unpin_work_func) to unpin an old buffer >>>(crtc->primary->fb), the work will be queued in dce_vX_0_pageflip_irq(). >>> >>> 2, pin a new buffer, assign it to crtc->primary->fb. But how to unpin it? >>>Next call. >>> >>> The problem is the pinned buffer of last call to >>> amdgpu_display_crtc_page_flip_target() is not unpinned. >> >> It's unpinned in dce_v*_0_crtc_disable. > > I just found crtc->primary->fb is NULL when came in dce_v*_0_crtc_disable(). > So it's not unpinned... __drm_helper_disable_unused_functions sets crtc->primary->fb = NULL only after calling crtc_funcs->disable. Maybe this path can get hit for a CRTC which was already disabled, in which case crtc->primary->fb == NULL in dce_v*_0_crtc_disable is harmless. Have you checked for the issue I described below? Should be pretty easy to catch. >> I think I've found the problem though: dce_v*_0_crtc_do_set_base pin the BO >> from target_fb unconditionally, but unpin the BO from the fb parameter only >> if it's different from the former. So if they're the same, the BO's pin >> count is incremented by 1. -- Earthling Michel Dänzer| https://redhat.com Libre software enthusiast | Mesa and Xwayland developer
[i-g-t 01/14] HAX: Get uapi headers to compile the IGT
Signed-off-by: Bhanuprakash Modem
---
include/drm-uapi/drm.h | 10 ++
include/drm-uapi/drm_mode.h | 28
2 files changed, 38 insertions(+)
diff --git a/include/drm-uapi/drm.h b/include/drm-uapi/drm.h
index 5e54c3aa4c..9ca3dbe8e5 100644
--- a/include/drm-uapi/drm.h
+++ b/include/drm-uapi/drm.h
@@ -830,6 +830,16 @@ struct drm_get_cap {
*/
#define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS5
+/**
+ * DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES
+ *
+ * Add support for advance gamma mode UAPI
+ * If set to 1, DRM will enable advance gamma mode
+ * UAPI to process the gamma mode based on extended
+ * range and segments.
+ */
+#define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES 6
+
/* DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
struct drm_set_client_cap {
__u64 capability;
diff --git a/include/drm-uapi/drm_mode.h b/include/drm-uapi/drm_mode.h
index e4a2570a60..97198609a5 100644
--- a/include/drm-uapi/drm_mode.h
+++ b/include/drm-uapi/drm_mode.h
@@ -817,6 +817,34 @@ struct drm_color_lut {
__u16 reserved;
};
+/*
+ * Creating 64 bit palette entries for better data
+ * precision. This will be required for HDR and
+ * similar color processing usecases.
+ */
+struct drm_color_lut_ext {
+/*
+ * Data is U32.32 fixed point format.
+ */
+__u64 red;
+__u64 green;
+__u64 blue;
+__u64 reserved;
+};
+
+struct drm_color_lut_range {
+/* DRM_MODE_LUT_* */
+__u32 flags;
+/* number of points on the curve */
+__u16 count;
+/* input/output bits per component */
+__u8 input_bpc, output_bpc;
+/* input start/end values */
+__s32 start, end;
+/* output min/max values */
+__s32 min, max;
+};
+
/**
* struct hdr_metadata_infoframe - HDR Metadata Infoframe Data.
*
--
2.32.0
[i-g-t 00/14] Add IGT support for plane color management
>From the Plane Color Management feature design, userspace can take the smart blending decisions based on hardware supported plane color features to obtain an accurate color profile. These IGT patches extend the existing pipe color management tests to the plane level. Kernel implementation: https://patchwork.freedesktop.org/series/90825/ Bhanuprakash Modem (11): HAX: Get uapi headers to compile the IGT lib/igt_kms: Add plane color mgmt properties kms_color_helper: Add helper functions for plane color mgmt tests/kms_color: New subtests for Plane gamma tests/kms_color: New subtests for Plane degamma tests/kms_color: New subtests for Plane CTM tests/kms_color: New negative tests for plane level color mgmt tests/kms_color_chamelium: New subtests for Plane gamma tests/kms_color_chamelium: New subtests for Plane degamma tests/kms_color_chamelium: New subtests for Plane CTM tests/kms_color_chamelium: Extended IGT tests to support logarithmic gamma mode Mukunda Pramodh Kumar (3): lib/igt_kms: Add pipe color mgmt properties kms_color_helper: Add helper functions to support logarithmic gamma mode tests/kms_color: Extended IGT tests to support logarithmic gamma mode include/drm-uapi/drm.h | 10 + include/drm-uapi/drm_mode.h | 28 ++ lib/igt_kms.c | 6 + lib/igt_kms.h | 6 + tests/kms_color.c | 674 +++- tests/kms_color_chamelium.c | 588 ++- tests/kms_color_helper.c| 300 tests/kms_color_helper.h| 45 +++ 8 files changed, 1648 insertions(+), 9 deletions(-) -- 2.32.0
[i-g-t 02/14] lib/igt_kms: Add plane color mgmt properties
Add support for Plane color management properties.
Cc: Harry Wentland
Cc: Ville Syrjälä
Cc: Juha-Pekka Heikkila
Cc: Uma Shankar
Signed-off-by: Bhanuprakash Modem
---
lib/igt_kms.c | 5 +
lib/igt_kms.h | 5 +
2 files changed, 10 insertions(+)
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 34a2aa00ea..fdb83e0f91 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -581,6 +581,11 @@ const char * const
igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
[IGT_PLANE_ALPHA] = "alpha",
[IGT_PLANE_ZPOS] = "zpos",
[IGT_PLANE_FB_DAMAGE_CLIPS] = "FB_DAMAGE_CLIPS",
+ [IGT_PLANE_CTM] = "PLANE_CTM",
+ [IGT_PLANE_GAMMA_MODE] = "PLANE_GAMMA_MODE",
+ [IGT_PLANE_GAMMA_LUT] = "PLANE_GAMMA_LUT",
+ [IGT_PLANE_DEGAMMA_MODE] = "PLANE_DEGAMMA_MODE",
+ [IGT_PLANE_DEGAMMA_LUT] = "PLANE_DEGAMMA_LUT",
};
const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index e9ecd21e98..3a1f7243ad 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -301,6 +301,11 @@ enum igt_atomic_plane_properties {
IGT_PLANE_ALPHA,
IGT_PLANE_ZPOS,
IGT_PLANE_FB_DAMAGE_CLIPS,
+ IGT_PLANE_CTM,
+ IGT_PLANE_GAMMA_MODE,
+ IGT_PLANE_GAMMA_LUT,
+ IGT_PLANE_DEGAMMA_MODE,
+ IGT_PLANE_DEGAMMA_LUT,
IGT_NUM_PLANE_PROPS
};
--
2.32.0
[i-g-t 04/14] tests/kms_color: New subtests for Plane gamma
To verify Plane gamma, draw 3 gradient rectangles in red, green and blue,
with a maxed out gamma LUT and verify we have the same CRC as drawing solid
color rectangles.
Cc: Harry Wentland
Cc: Ville Syrjälä
Cc: Juha-Pekka Heikkila
Cc: Uma Shankar
Signed-off-by: Bhanuprakash Modem
---
tests/kms_color.c | 179 +-
1 file changed, 178 insertions(+), 1 deletion(-)
diff --git a/tests/kms_color.c b/tests/kms_color.c
index 775f35964f..b45d66762f 100644
--- a/tests/kms_color.c
+++ b/tests/kms_color.c
@@ -24,7 +24,34 @@
#include "kms_color_helper.h"
-IGT_TEST_DESCRIPTION("Test Color Features at Pipe level");
+IGT_TEST_DESCRIPTION("Test Color Features at Pipe & Plane level");
+
+#define MAX_SUPPORTED_PLANES 7
+#define SDR_PLANE_BASE 3
+
+typedef bool (*test_t)(data_t*, igt_plane_t*);
+
+static bool is_hdr_plane(const igt_plane_t *plane)
+{
+ return plane->index >= 0 && plane->index < SDR_PLANE_BASE;
+}
+
+static bool is_valid_plane(igt_plane_t *plane)
+{
+ int index = plane->index;
+
+ if (plane->type != DRM_PLANE_TYPE_PRIMARY)
+ return false;
+
+ /*
+* Test 1 HDR plane, 1 SDR plane.
+*
+* 0,1,2 HDR planes
+* 3,4,5,6 SDR planes
+*
+*/
+ return index >= 0 && index < MAX_SUPPORTED_PLANES;
+}
static void test_pipe_degamma(data_t *data,
igt_plane_t *primary)
@@ -638,6 +665,122 @@ static void test_pipe_limited_range_ctm(data_t *data,
}
#endif
+static bool plane_gamma_test(data_t *data, igt_plane_t *plane)
+{
+ igt_output_t *output;
+ igt_display_t *display = &data->display;
+ drmModeModeInfo *mode;
+ struct igt_fb fb;
+ drmModePropertyPtr gamma_mode = NULL;
+ uint32_t i;
+ bool ret = true;
+ igt_pipe_crc_t *pipe_crc = NULL;
+ color_t red_green_blue[] = {
+ { 1.0, 0.0, 0.0 },
+ { 0.0, 1.0, 0.0 },
+ { 0.0, 0.0, 1.0 }
+ };
+
+ igt_info("Plane gamma test is running on pipe-%s plane-%s(%s)\n",
+ kmstest_pipe_name(plane->pipe->pipe),
+ kmstest_plane_type_name(plane->type),
+ is_hdr_plane(plane) ? "hdr":"sdr");
+
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_MODE));
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_LUT));
+
+ pipe_crc = igt_pipe_crc_new(data->drm_fd,
+ plane->pipe->pipe,
+ INTEL_PIPE_CRC_SOURCE_AUTO);
+
+ output = igt_get_single_output_for_pipe(display, plane->pipe->pipe);
+ igt_assert(output);
+
+ igt_output_set_pipe(output, plane->pipe->pipe);
+ mode = igt_output_get_mode(output);
+
+ /* Create a framebuffer at the size of the output. */
+ igt_assert(igt_create_fb(data->drm_fd,
+ mode->hdisplay,
+ mode->vdisplay,
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_MOD_LINEAR,
+ &fb));
+ igt_plane_set_fb(plane, &fb);
+
+ /* Disable Pipe color props. */
+ disable_ctm(plane->pipe);
+ disable_degamma(plane->pipe);
+ disable_gamma(plane->pipe);
+
+ disable_plane_ctm(plane);
+ disable_plane_degamma(plane);
+ igt_display_commit2(display, display->is_atomic ?
+ COMMIT_ATOMIC : COMMIT_LEGACY);
+
+ gamma_mode = get_plane_gamma_degamma_mode(plane, IGT_PLANE_GAMMA_MODE);
+
+ /* Iterate all supported gamma modes. */
+ for (i = 0; i < gamma_mode->count_enums; i++) {
+ igt_crc_t crc_gamma, crc_fullcolors;
+ segment_data_t *segment_info = NULL;
+ struct drm_color_lut_ext *lut = NULL;
+ uint32_t lut_size = 0;
+
+ /* Ignore 'no gamma' from enum list. */
+ if (!strcmp(gamma_mode->enums[i].name, "no gamma"))
+ continue;
+
+ igt_info("Trying to use gamma mode: \'%s\'\n",
gamma_mode->enums[i].name);
+
+ /* Draw solid colors with no gamma transformation. */
+ disable_plane_gamma(plane);
+ paint_rectangles(data, mode, red_green_blue, &fb);
+ igt_plane_set_fb(plane, &fb);
+ igt_display_commit2(display, display->is_atomic ?
+ COMMIT_ATOMIC : COMMIT_LEGACY);
+ igt_wait_for_vblank(data->drm_fd,
+ display->pipes[plane->pipe->pipe].crtc_offset);
+ igt_pipe_crc_collect_crc(pipe_crc, &crc_fullcolors);
+
+ /* Draw gradient colors with gamma LUT to remap all
+* values to max red/green/blue.
+*/
+ segment_info = get_segment_data(data,
gamma_mode->enums[i].value,
+ gamma_mode-
[i-g-t 03/14] kms_color_helper: Add helper functions for plane color mgmt
Add helper functions to support Plane color management properties.
Cc: Harry Wentland
Cc: Ville Syrjälä
Cc: Juha-Pekka Heikkila
Cc: Uma Shankar
Signed-off-by: Bhanuprakash Modem
---
tests/kms_color_helper.c | 173 +++
tests/kms_color_helper.h | 29 +++
2 files changed, 202 insertions(+)
diff --git a/tests/kms_color_helper.c b/tests/kms_color_helper.c
index d71e7bb2e6..c65b7a0f50 100644
--- a/tests/kms_color_helper.c
+++ b/tests/kms_color_helper.c
@@ -241,6 +241,150 @@ void disable_prop(igt_pipe_t *pipe, enum
igt_atomic_crtc_properties prop)
igt_pipe_obj_replace_prop_blob(pipe, prop, NULL, 0);
}
+drmModePropertyPtr get_plane_gamma_degamma_mode(igt_plane_t *plane,
+ enum igt_atomic_plane_properties prop)
+{
+ igt_display_t *display = plane->pipe->display;
+ uint32_t prop_id = plane->props[prop];
+ drmModePropertyPtr drmProp;
+
+ igt_assert(prop_id);
+
+ drmProp = drmModeGetProperty(display->drm_fd, prop_id);
+
+ igt_assert(drmProp);
+ igt_assert(drmProp->count_enums);
+
+ return drmProp;
+}
+
+struct drm_color_lut_ext *create_linear_lut(segment_data_t *info)
+{
+ uint32_t val, segment, entry, index = 0;
+ uint32_t max_val = 0x;
+ struct drm_color_lut_ext *lut = malloc(sizeof(struct drm_color_lut_ext)
* info->entries_count);
+ igt_assert(lut);
+
+ for (segment = 0; segment < info->segment_count; segment++) {
+ uint32_t entry_count = info->segment_data[segment].count;
+ uint32_t start = info->segment_data[segment].start;
+ uint32_t end = info->segment_data[segment].end;
+
+ for (entry = 1; entry <= entry_count; entry++) {
+ val = (index == 0) ? /* First entry is Zero. */
+ 0 : start + entry * ((end - start) * 1.0 /
entry_count);
+
+ lut[index].red = lut[index].green = lut[index].blue =
MIN(val, max_val);
+
+ index++;
+ }
+ }
+
+ return lut;
+}
+
+struct drm_color_lut_ext *create_max_lut(segment_data_t *info)
+{
+ int i;
+ struct drm_color_lut_ext *lut;
+ uint32_t max_val = 0x;
+
+ lut = malloc(sizeof(struct drm_color_lut_ext) * info->entries_count);
+ igt_assert(lut);
+
+ lut[0].red = lut[0].green = lut[0].blue = 0; /* First entry is Zero. */
+
+ for (i = 1; i < info->entries_count; i++)
+ lut[i].red = lut[i].green = lut[i].blue = max_val;
+
+ return lut;
+}
+
+void clear_segment_data(segment_data_t *info)
+{
+ if (!info)
+ return;
+
+ free(info->segment_data);
+ free(info);
+}
+
+segment_data_t *get_segment_data(data_t *data,
+ uint64_t blob_id, char *mode)
+{
+ drmModePropertyBlobPtr blob;
+ struct drm_color_lut_range *lut_range = NULL;
+ segment_data_t *info = NULL;
+ uint32_t i;
+
+ blob = drmModeGetPropertyBlob(data->drm_fd, blob_id);
+ igt_assert(blob);
+ igt_assert(blob->length);
+
+ info = malloc(sizeof(segment_data_t));
+ igt_assert(info);
+
+ lut_range = (struct drm_color_lut_range *) blob->data;
+ info->segment_count = blob->length / sizeof(lut_range[0]);
+ igt_assert(info->segment_count);
+
+ info->segment_data = malloc(sizeof(struct drm_color_lut_range) *
info->segment_count);
+ igt_assert(info->segment_data);
+
+ for (i = 0; i < info->segment_count; i++) {
+ info->entries_count += lut_range[i].count;
+ info->segment_data[i] = lut_range[i];
+ }
+
+ drmModeFreePropertyBlob(blob);
+
+ return info;
+}
+
+void set_plane_gamma(igt_plane_t *plane,
+ char *mode,
+ struct drm_color_lut_ext *lut,
+ uint32_t size)
+{
+ igt_plane_set_prop_enum(plane, IGT_PLANE_GAMMA_MODE, mode);
+ igt_plane_replace_prop_blob(plane, IGT_PLANE_GAMMA_LUT, lut, size);
+}
+
+void set_plane_degamma(igt_plane_t *plane,
+ char *mode,
+ struct drm_color_lut_ext *lut,
+ uint32_t size)
+{
+ igt_plane_set_prop_enum(plane, IGT_PLANE_DEGAMMA_MODE, mode);
+ igt_plane_replace_prop_blob(plane, IGT_PLANE_DEGAMMA_LUT, lut, size);
+}
+
+void set_plane_ctm(igt_plane_t *plane, const double *coefficients)
+{
+ struct drm_color_ctm ctm;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(ctm.matrix); i++) {
+ if (coefficients[i] < 0) {
+ ctm.matrix[i] =
+ (int64_t) (-coefficients[i] *
+ ((int64_t) 1L << 32));
+ ctm.matrix[i] |= 1ULL << 63;
+ } else
+ ctm.matrix[i] =
+ (int64_t) (coefficients[i] *
+ ((int
[i-g-t 05/14] tests/kms_color: New subtests for Plane degamma
To verify Plane degamma, draw 3 gradient rectangles in red, green and blue,
with a maxed out degamma LUT and verify we have the same CRC as drawing solid
color rectangles without degamma.
Cc: Harry Wentland
Cc: Ville Syrjälä
Cc: Juha-Pekka Heikkila
Cc: Uma Shankar
Signed-off-by: Bhanuprakash Modem
---
tests/kms_color.c | 124 ++
1 file changed, 124 insertions(+)
diff --git a/tests/kms_color.c b/tests/kms_color.c
index b45d66762f..920a5eaadd 100644
--- a/tests/kms_color.c
+++ b/tests/kms_color.c
@@ -781,6 +781,125 @@ static bool plane_gamma_test(data_t *data, igt_plane_t
*plane)
return ret;
}
+static bool plane_degamma_test(data_t *data, igt_plane_t *plane)
+{
+ igt_output_t *output;
+ igt_display_t *display = &data->display;
+ drmModeModeInfo *mode;
+ drmModePropertyPtr degamma_mode;
+ struct igt_fb fb;
+ uint32_t i;
+ bool ret = true;
+ igt_pipe_crc_t *pipe_crc = NULL;
+ color_t red_green_blue[] = {
+ { 1.0, 0.0, 0.0 },
+ { 0.0, 1.0, 0.0 },
+ { 0.0, 0.0, 1.0 }
+ };
+
+ igt_info("Plane degamma test is running on pipe-%s plane-%s(%s)\n",
+ kmstest_pipe_name(plane->pipe->pipe),
+ kmstest_plane_type_name(plane->type),
+ is_hdr_plane(plane) ? "hdr":"sdr");
+
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_DEGAMMA_MODE));
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_DEGAMMA_LUT));
+
+ pipe_crc = igt_pipe_crc_new(data->drm_fd,
+ plane->pipe->pipe,
+ INTEL_PIPE_CRC_SOURCE_AUTO);
+
+ output = igt_get_single_output_for_pipe(display, plane->pipe->pipe);
+ igt_assert(output);
+
+ igt_output_set_pipe(output, plane->pipe->pipe);
+ mode = igt_output_get_mode(output);
+
+ /* Create a framebuffer at the size of the output. */
+ igt_assert(igt_create_fb(data->drm_fd,
+ mode->hdisplay,
+ mode->vdisplay,
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_MOD_LINEAR,
+ &fb));
+
+ igt_plane_set_fb(plane, &fb);
+
+ /* Disable Pipe color props. */
+ disable_ctm(plane->pipe);
+ disable_degamma(plane->pipe);
+ disable_gamma(plane->pipe);
+
+ disable_plane_ctm(plane);
+ disable_plane_gamma(plane);
+ igt_display_commit2(display, display->is_atomic ?
+ COMMIT_ATOMIC : COMMIT_LEGACY);
+
+ degamma_mode = get_plane_gamma_degamma_mode(plane,
IGT_PLANE_DEGAMMA_MODE);
+
+ /* Iterate all supported degamma modes. */
+ for (i = 0; i < degamma_mode->count_enums; i++) {
+ igt_crc_t crc_degamma, crc_fullcolors;
+ segment_data_t *degamma_segment_info = NULL;
+ struct drm_color_lut_ext *degamma_lut = NULL;
+ uint32_t degamma_lut_size = 0;
+
+ /* Ignore 'no degamma' from enum list. */
+ if (!strcmp(degamma_mode->enums[i].name, "no degamma"))
+ continue;
+
+ degamma_segment_info = get_segment_data(data,
degamma_mode->enums[i].value,
+ degamma_mode->enums[i].name);
+ degamma_lut_size = sizeof(struct drm_color_lut_ext) *
degamma_segment_info->entries_count;
+ degamma_lut = create_max_lut(degamma_segment_info);
+
+ igt_info("Trying to use degamma mode: \'%s\'\n",
degamma_mode->enums[i].name);
+
+ /* Draw solid colors with no degamma. */
+ disable_plane_degamma(plane);
+ paint_rectangles(data, mode, red_green_blue, &fb);
+ igt_plane_set_fb(plane, &fb);
+ igt_display_commit2(display, display->is_atomic ?
+ COMMIT_ATOMIC : COMMIT_LEGACY);
+ igt_wait_for_vblank(data->drm_fd,
+ display->pipes[plane->pipe->pipe].crtc_offset);
+ igt_pipe_crc_collect_crc(pipe_crc, &crc_fullcolors);
+
+ /* Draw gradient colors with degamma LUT to remap all
+* values to max red/green/blue.
+*/
+ paint_gradient_rectangles(data, mode, red_green_blue, &fb);
+ igt_plane_set_fb(plane, &fb);
+ set_plane_degamma(plane, degamma_mode->enums[i].name,
+ degamma_lut, degamma_lut_size);
+ igt_display_commit2(display, display->is_atomic ?
+ COMMIT_ATOMIC : COMMIT_LEGACY);
+ igt_wait_for_vblank(data->drm_fd,
+ display->pipes[plane->pipe->pipe].crtc_offset);
+ igt_pipe_crc_collect_crc(pipe_crc, &crc_degamma);
+
+ /*
[i-g-t 07/14] tests/kms_color: New negative tests for plane level color mgmt
Negative check for:
* plane gamma lut sizes
* plane degamma lut sizes
* plane ctm matrix sizes
Cc: Harry Wentland
Cc: Ville Syrjälä
Cc: Juha-Pekka Heikkila
Cc: Uma Shankar
Signed-off-by: Bhanuprakash Modem
---
tests/kms_color.c | 127 ++
1 file changed, 127 insertions(+)
diff --git a/tests/kms_color.c b/tests/kms_color.c
index e14b37cb6f..d9fe417ba9 100644
--- a/tests/kms_color.c
+++ b/tests/kms_color.c
@@ -736,6 +736,118 @@ static void test_pipe_limited_range_ctm(data_t *data,
}
#endif
+static bool invalid_plane_gamma_test(data_t *data, igt_plane_t *plane)
+{
+ igt_display_t *display = &data->display;
+ drmModePropertyPtr gamma_mode = NULL;
+ uint32_t i;
+
+ igt_info("Plane invalid gamma test is running on pipe-%s
plane-%s(%s)\n",
+ kmstest_pipe_name(plane->pipe->pipe),
+ kmstest_plane_type_name(plane->type),
+ is_hdr_plane(plane) ? "hdr":"sdr");
+
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_MODE));
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_LUT));
+
+ gamma_mode = get_plane_gamma_degamma_mode(plane, IGT_PLANE_GAMMA_MODE);
+
+ /* Iterate all supported gamma modes. */
+ for (i = 0; i < gamma_mode->count_enums; i++) {
+ segment_data_t *segment_info = NULL;
+ size_t lut_size = 0;
+
+ /* Ignore 'no gamma' from enum list. */
+ if (!strcmp(gamma_mode->enums[i].name, "no gamma"))
+ continue;
+
+ igt_info("Trying to use gamma mode: \'%s\'\n",
gamma_mode->enums[i].name);
+
+ segment_info = get_segment_data(data,
gamma_mode->enums[i].value,
+ gamma_mode->enums[i].name);
+ lut_size = sizeof(struct drm_color_lut_ext) *
segment_info->entries_count;
+
+ igt_plane_set_prop_enum(plane, IGT_PLANE_GAMMA_MODE,
gamma_mode->enums[i].name);
+ invalid_plane_lut_sizes(display, plane,
+ IGT_PLANE_GAMMA_LUT,
+ lut_size);
+
+ clear_segment_data(segment_info);
+
+ /* One enum is enough. */
+ break;
+ }
+
+ drmModeFreeProperty(gamma_mode);
+
+ return true;
+}
+
+static bool invalid_plane_degamma_test(data_t *data, igt_plane_t *plane)
+{
+ igt_display_t *display = &data->display;
+ drmModePropertyPtr degamma_mode = NULL;
+ uint32_t i;
+
+ igt_info("Plane invalid degamma test is running on pipe-%s
plane-%s(%s)\n",
+ kmstest_pipe_name(plane->pipe->pipe),
+ kmstest_plane_type_name(plane->type),
+ is_hdr_plane(plane) ? "hdr":"sdr");
+
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_DEGAMMA_MODE));
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_DEGAMMA_LUT));
+
+ degamma_mode = get_plane_gamma_degamma_mode(plane,
IGT_PLANE_DEGAMMA_MODE);
+
+ /* Iterate all supported degamma modes. */
+ for (i = 0; i < degamma_mode->count_enums; i++) {
+ segment_data_t *segment_info = NULL;
+ size_t lut_size = 0;
+
+ /* Ignore 'no degamma' from enum list. */
+ if (!strcmp(degamma_mode->enums[i].name, "no degamma"))
+ continue;
+
+ igt_info("Trying to use degamma mode: \'%s\'\n",
degamma_mode->enums[i].name);
+
+ segment_info = get_segment_data(data,
+ degamma_mode->enums[i].value,
+ degamma_mode->enums[i].name);
+ lut_size = sizeof(struct drm_color_lut_ext) *
segment_info->entries_count * 2;
+
+ igt_plane_set_prop_enum(plane,
+ IGT_PLANE_DEGAMMA_MODE,
+ degamma_mode->enums[i].name);
+ invalid_plane_lut_sizes(display, plane,
+ IGT_PLANE_DEGAMMA_LUT,
+ lut_size);
+
+ clear_segment_data(segment_info);
+
+ /* One enum is enough. */
+ break;
+ }
+
+ drmModeFreeProperty(degamma_mode);
+
+ return true;
+}
+
+static bool invalid_plane_ctm_test(data_t *data, igt_plane_t *plane)
+{
+ igt_info("Plane invalid CTM test is running on pipe-%s plane-%s(%s)\n",
+ kmstest_pipe_name(plane->pipe->pipe),
+ kmstest_plane_type_name(plane->type),
+ is_hdr_plane(plane) ? "hdr":"sdr");
+
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_CTM));
+ invalid_plane_lut_sizes(&data->display, plane,
+ IGT_PLANE_CTM,
+ sizeof(struct drm_color_ctm));
+
+ return t
[i-g-t 06/14] tests/kms_color: New subtests for Plane CTM
To verify plane CTM, draw 3 rectangles using before colors with the
ctm matrix applied and verify the CRC is equal to using after colors
with an identify ctm matrix.
Cc: Harry Wentland
Cc: Ville Syrjälä
Cc: Juha-Pekka Heikkila
Cc: Uma Shankar
Signed-off-by: Bhanuprakash Modem
---
tests/kms_color.c | 225 ++
1 file changed, 225 insertions(+)
diff --git a/tests/kms_color.c b/tests/kms_color.c
index 920a5eaadd..e14b37cb6f 100644
--- a/tests/kms_color.c
+++ b/tests/kms_color.c
@@ -53,6 +53,77 @@ static bool is_valid_plane(igt_plane_t *plane)
return index >= 0 && index < MAX_SUPPORTED_PLANES;
}
+struct {
+ const char *test_name;
+ int iter;
+ color_t expected_colors[3];
+ double ctm[9];
+} ctm_tests[] = {
+ {"plane-ctm-red-to-blue", 0,
+ {{ 0.0, 0.0, 1.0 },
+{ 0.0, 1.0, 0.0 },
+{ 0.0, 0.0, 1.0 }},
+ { 0.0, 0.0, 0.0,
+ 0.0, 1.0, 0.0,
+ 1.0, 0.0, 1.0 },
+ },
+ {"plane-ctm-green-to-red", 0,
+ {{ 1.0, 0.0, 0.0 },
+{ 1.0, 0.0, 0.0 },
+{ 0.0, 0.0, 1.0 }},
+ { 1.0, 1.0, 0.0,
+ 0.0, 0.0, 0.0,
+ 0.0, 0.0, 1.0 },
+ },
+ {"plane-ctm-blue-to-red", 0,
+ {{ 1.0, 0.0, 0.0 },
+{ 0.0, 1.0, 0.0 },
+{ 1.0, 0.0, 0.0 }},
+ { 1.0, 0.0, 1.0,
+ 0.0, 1.0, 0.0,
+ 0.0, 0.0, 0.0 },
+ },
+ {"plane-ctm-max", 0,
+ {{ 1.0, 0.0, 0.0 },
+{ 0.0, 1.0, 0.0 },
+{ 0.0, 0.0, 1.0 }},
+ { 100.0, 0.0, 0.0,
+ 0.0, 100.0, 0.0,
+ 0.0, 0.0, 100.0 },
+ },
+ {"plane-ctm-negative", 0,
+ {{ 0.0, 0.0, 0.0 },
+{ 0.0, 0.0, 0.0 },
+{ 0.0, 0.0, 0.0 }},
+ { -1.0, 0.0, 0.0,
+ 0.0, -1.0, 0.0,
+ 0.0, 0.0, -1.0 },
+ },
+ /* We tests a few values around the expected result because
+* it depends on the hardware we're dealing with, we can
+* either get clamped or rounded values and we also need to
+* account for odd number of items in the LUTs.
+*/
+ {"plane-ctm-0-25", 5,
+ {{ 0.0, }, { 0.0, }, { 0.0, }},
+ { 0.25, 0.0, 0.0,
+ 0.0, 0.25, 0.0,
+ 0.0, 0.0, 0.25 },
+ },
+ {"plane-ctm-0-50", 5,
+ {{ 0.0, }, { 0.0, }, { 0.0, }},
+ { 0.5, 0.0, 0.0,
+ 0.0, 0.5, 0.0,
+ 0.0, 0.0, 0.5 },
+ },
+ {"plane-ctm-0-75", 7,
+ {{ 0.0, }, { 0.0, }, { 0.0, }},
+ { 0.75, 0.0, 0.0,
+ 0.0, 0.75, 0.0,
+ 0.0, 0.0, 0.75 },
+ },
+};
+
static void test_pipe_degamma(data_t *data,
igt_plane_t *primary)
{
@@ -900,6 +971,99 @@ static bool plane_degamma_test(data_t *data, igt_plane_t
*plane)
return ret;
}
+static bool test_plane_ctm(data_t *data,
+ igt_plane_t *plane,
+ color_t *before,
+ color_t *after,
+ double *ctm_matrix)
+{
+ const double ctm_identity[] = {
+ 1.0, 0.0, 0.0,
+ 0.0, 1.0, 0.0,
+ 0.0, 0.0, 1.0
+ };
+ igt_output_t *output;
+ igt_display_t *display = &data->display;
+ drmModeModeInfo *mode;
+ struct igt_fb fb;
+ igt_crc_t crc_software, crc_hardware;
+ igt_pipe_crc_t *pipe_crc = NULL;
+ bool ret = true;
+
+ igt_info("Plane CTM test is running on pipe-%s plane-%s(%s)\n",
+ kmstest_pipe_name(plane->pipe->pipe),
+ kmstest_plane_type_name(plane->type),
+ is_hdr_plane(plane) ? "hdr":"sdr");
+
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_CTM));
+
+ pipe_crc = igt_pipe_crc_new(data->drm_fd,
+ plane->pipe->pipe,
+ INTEL_PIPE_CRC_SOURCE_AUTO);
+
+ output = igt_get_single_output_for_pipe(display, plane->pipe->pipe);
+ igt_assert(output);
+
+ igt_output_set_pipe(output, plane->pipe->pipe);
+ mode = igt_output_get_mode(output);
+
+ /* Create a framebuffer at the size of the output. */
+ igt_assert(igt_cr
[i-g-t 08/14] tests/kms_color_chamelium: New subtests for Plane gamma
To verify Plane gamma, draw 3 gradient rectangles in red, green and blue,
with a maxed out gamma LUT and verify we have the same frame dump as
drawing solid color rectangles.
Cc: Harry Wentland
Cc: Ville Syrjälä
Cc: Juha-Pekka Heikkila
Cc: Uma Shankar
Cc: Kunal Joshi
Signed-off-by: Bhanuprakash Modem
---
tests/kms_color_chamelium.c | 188 +++-
1 file changed, 187 insertions(+), 1 deletion(-)
diff --git a/tests/kms_color_chamelium.c b/tests/kms_color_chamelium.c
index 76f82d6d35..b506109271 100644
--- a/tests/kms_color_chamelium.c
+++ b/tests/kms_color_chamelium.c
@@ -24,7 +24,34 @@
#include "kms_color_helper.h"
-IGT_TEST_DESCRIPTION("Test Color Features at Pipe level using Chamelium to
verify instead of CRC");
+IGT_TEST_DESCRIPTION("Test Color Features at Pipe & Plane level using
Chamelium to verify instead of CRC");
+
+#define MAX_SUPPORTED_PLANES 7
+#define SDR_PLANE_BASE 3
+
+typedef bool (*test_t)(data_t*, igt_plane_t*);
+
+static bool is_hdr_plane(const igt_plane_t *plane)
+{
+ return plane->index >= 0 && plane->index < SDR_PLANE_BASE;
+}
+
+static bool is_valid_plane(igt_plane_t *plane)
+{
+ int index = plane->index;
+
+ if (plane->type != DRM_PLANE_TYPE_PRIMARY)
+ return false;
+
+ /*
+* Test 1 HDR plane, 1 SDR plane.
+*
+* 0,1,2 HDR planes
+* 3,4,5,6 SDR planes
+*
+*/
+ return index >= 0 && index < MAX_SUPPORTED_PLANES;
+}
/*
* Draw 3 gradient rectangles in red, green and blue, with a maxed out
@@ -723,6 +750,161 @@ run_tests_for_pipe(data_t *data, enum pipe p)
}
}
+static bool plane_gamma_test(data_t *data, igt_plane_t *plane)
+{
+ igt_output_t *output;
+ igt_display_t *display = &data->display;
+ drmModeModeInfo *mode;
+ struct igt_fb fb, fbref;
+ drmModePropertyPtr gamma_mode = NULL;
+ uint32_t i;
+ bool ret = true;
+ struct chamelium_port *port = NULL;
+ color_t red_green_blue[] = {
+ { 1.0, 0.0, 0.0 },
+ { 0.0, 1.0, 0.0 },
+ { 0.0, 0.0, 1.0 }
+ };
+
+ igt_info("Plane gamma test is running on pipe-%s plane-%s(%s)\n",
+ kmstest_pipe_name(plane->pipe->pipe),
+ kmstest_plane_type_name(plane->type),
+ is_hdr_plane(plane) ? "hdr":"sdr");
+
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_MODE));
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_GAMMA_LUT));
+
+ for_each_valid_output_on_pipe(display, plane->pipe->pipe, output) {
+ for (i = 0; i < data->port_count; i++)
+ if (strcmp(output->name,
chamelium_port_get_name(data->ports[i])) == 0) {
+ port = data->ports[i];
+ break;
+ }
+
+ if (port)
+ break;
+ }
+ igt_require(port);
+ igt_assert(output);
+
+ igt_output_set_pipe(output, plane->pipe->pipe);
+ mode = igt_output_get_mode(output);
+
+ /* Create a framebuffer at the size of the output. */
+ igt_assert(igt_create_fb(data->drm_fd,
+ mode->hdisplay,
+ mode->vdisplay,
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_MOD_LINEAR,
+ &fb));
+
+ igt_assert(igt_create_fb(data->drm_fd,
+ mode->hdisplay,
+ mode->vdisplay,
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_MOD_LINEAR,
+ &fbref));
+
+ disable_degamma(plane->pipe);
+ disable_ctm(plane->pipe);
+ disable_gamma(plane->pipe);
+
+ disable_plane_degamma(plane);
+ disable_plane_ctm(plane);
+ disable_plane_gamma(plane);
+
+ igt_plane_set_fb(plane, &fbref);
+ igt_display_commit2(display, display->is_atomic ?
+ COMMIT_ATOMIC : COMMIT_LEGACY);
+
+ /* Draw solid colors with no gamma transformation. */
+ paint_rectangles(data, mode, red_green_blue, &fbref);
+
+ gamma_mode = get_plane_gamma_degamma_mode(plane, IGT_PLANE_GAMMA_MODE);
+ /* Iterate all supported gamma modes. */
+ for (i = 0; i < gamma_mode->count_enums; i++) {
+ struct chamelium_frame_dump *frame_fullcolors;
+ segment_data_t *segment_info = NULL;
+ struct drm_color_lut_ext *lut = NULL;
+ uint32_t lut_size = 0;
+
+ /* Ignore 'no gamma' from enum list. */
+ if (!strcmp(gamma_mode->enums[i].name, "no gamma"))
+ continue;
+
+ igt_info("Trying to use gamma mode: \'%s\'\n",
gamma_mode->enums[i].name);
+
+ segment_info = get_segment_data(data,
gamma_mode->enums[i].v
[i-g-t 09/14] tests/kms_color_chamelium: New subtests for Plane degamma
To verify Plane degamma, draw 3 gradient rectangles in red, green and blue,
with a maxed out degamma LUT and verify we have the same frame dump as
drawing solid color rectangles with linear gamma LUT.
Cc: Harry Wentland
Cc: Ville Syrjälä
Cc: Juha-Pekka Heikkila
Cc: Uma Shankar
Cc: Kunal Joshi
Signed-off-by: Bhanuprakash Modem
---
tests/kms_color_chamelium.c | 131
1 file changed, 131 insertions(+)
diff --git a/tests/kms_color_chamelium.c b/tests/kms_color_chamelium.c
index b506109271..3bcb3ac043 100644
--- a/tests/kms_color_chamelium.c
+++ b/tests/kms_color_chamelium.c
@@ -874,6 +874,132 @@ static bool plane_gamma_test(data_t *data, igt_plane_t
*plane)
return ret;
}
+static bool plane_degamma_test(data_t *data, igt_plane_t *plane)
+{
+ igt_output_t *output;
+ igt_display_t *display = &data->display;
+ drmModeModeInfo *mode;
+ drmModePropertyPtr degamma_mode;
+ struct igt_fb fb, fbref;
+ struct chamelium_port *port;
+ uint32_t i;
+ bool ret = true;
+ color_t red_green_blue[] = {
+ { 1.0, 0.0, 0.0 },
+ { 0.0, 1.0, 0.0 },
+ { 0.0, 0.0, 1.0 }
+ };
+
+ igt_info("Plane degamma test is running on pipe-%s plane-%s(%s)\n",
+ kmstest_pipe_name(plane->pipe->pipe),
+ kmstest_plane_type_name(plane->type),
+ is_hdr_plane(plane) ? "hdr":"sdr");
+
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_DEGAMMA_MODE));
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_DEGAMMA_LUT));
+
+ for_each_valid_output_on_pipe(display, plane->pipe->pipe, output) {
+ for (i = 0; i < data->port_count; i++)
+ if (strcmp(output->name,
chamelium_port_get_name(data->ports[i])) == 0) {
+ port = data->ports[i];
+ break;
+ }
+
+ if (port)
+ break;
+ }
+ igt_require(port);
+ igt_assert(output);
+
+ igt_output_set_pipe(output, plane->pipe->pipe);
+ mode = igt_output_get_mode(output);
+
+ /* Create a framebuffer at the size of the output. */
+ igt_assert(igt_create_fb(data->drm_fd,
+ mode->hdisplay,
+ mode->vdisplay,
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_MOD_LINEAR,
+ &fb));
+
+ igt_assert(igt_create_fb(data->drm_fd,
+ mode->hdisplay,
+ mode->vdisplay,
+ DRM_FORMAT_XRGB,
+ DRM_FORMAT_MOD_LINEAR,
+ &fbref));
+
+ disable_degamma(plane->pipe);
+ disable_ctm(plane->pipe);
+ disable_gamma(plane->pipe);
+
+ disable_plane_degamma(plane);
+ disable_plane_ctm(plane);
+ disable_plane_gamma(plane);
+
+ igt_plane_set_fb(plane, &fbref);
+ igt_display_commit2(display, display->is_atomic ?
+ COMMIT_ATOMIC : COMMIT_LEGACY);
+
+ /* Draw solid colors with no degamma. */
+ paint_rectangles(data, mode, red_green_blue, &fbref);
+
+ degamma_mode = get_plane_gamma_degamma_mode(plane,
IGT_PLANE_DEGAMMA_MODE);
+ /* Iterate all supported degamma modes. */
+ for (i = 0; i < degamma_mode->count_enums; i++) {
+ struct chamelium_frame_dump *frame_fullcolors;
+ segment_data_t *degamma_segment_info = NULL;
+ struct drm_color_lut_ext *degamma_lut = NULL;
+ uint32_t degamma_lut_size = 0;
+
+ /* Ignore 'no degamma' from enum list. */
+ if (!strcmp(degamma_mode->enums[i].name, "no degamma"))
+ continue;
+
+ degamma_segment_info = get_segment_data(data,
degamma_mode->enums[i].value,
+ degamma_mode->enums[i].name);
+ degamma_lut_size = sizeof(struct drm_color_lut_ext) *
degamma_segment_info->entries_count;
+ degamma_lut = create_max_lut(degamma_segment_info);
+
+ igt_info("Trying to use degamma mode: \'%s\'\n",
degamma_mode->enums[i].name);
+
+ /* Draw a gradient with degamma LUT to remap all
+* values to max red/green/blue.
+*/
+ paint_gradient_rectangles(data, mode, red_green_blue, &fb);
+ igt_plane_set_fb(plane, &fb);
+ set_plane_degamma(plane, degamma_mode->enums[i].name,
+ degamma_lut, degamma_lut_size);
+ igt_display_commit2(display, display->is_atomic ?
+ COMMIT_ATOMIC : COMMIT_LEGACY);
+
+ chamelium_capture(data->chamelium, port, 0, 0, 0, 0, 1);
+ fram
[i-g-t 10/14] tests/kms_color_chamelium: New subtests for Plane CTM
To verify plane CTM, draw 3 rectangles using before colors with the
ctm matrix applied and verify the frame dump is equal to using after
colors with an identify ctm matrix.
Cc: Harry Wentland
Cc: Ville Syrjälä
Cc: Juha-Pekka Heikkila
Cc: Uma Shankar
Cc: Kunal Joshi
Signed-off-by: Bhanuprakash Modem
---
tests/kms_color_chamelium.c | 229
1 file changed, 229 insertions(+)
diff --git a/tests/kms_color_chamelium.c b/tests/kms_color_chamelium.c
index 3bcb3ac043..af820565d3 100644
--- a/tests/kms_color_chamelium.c
+++ b/tests/kms_color_chamelium.c
@@ -53,6 +53,77 @@ static bool is_valid_plane(igt_plane_t *plane)
return index >= 0 && index < MAX_SUPPORTED_PLANES;
}
+struct {
+ const char *test_name;
+ int iter;
+ color_t expected_colors[3];
+ double ctm[9];
+} ctm_tests[] = {
+ {"plane-ctm-red-to-blue", 0,
+ {{ 0.0, 0.0, 1.0 },
+{ 0.0, 1.0, 0.0 },
+{ 0.0, 0.0, 1.0 }},
+ { 0.0, 0.0, 0.0,
+ 0.0, 1.0, 0.0,
+ 1.0, 0.0, 1.0 },
+ },
+ {"plane-ctm-green-to-red", 0,
+ {{ 1.0, 0.0, 0.0 },
+{ 1.0, 0.0, 0.0 },
+{ 0.0, 0.0, 1.0 }},
+ { 1.0, 1.0, 0.0,
+ 0.0, 0.0, 0.0,
+ 0.0, 0.0, 1.0 },
+ },
+ {"plane-ctm-blue-to-red", 0,
+ {{ 1.0, 0.0, 0.0 },
+{ 0.0, 1.0, 0.0 },
+{ 1.0, 0.0, 0.0 }},
+ { 1.0, 0.0, 1.0,
+ 0.0, 1.0, 0.0,
+ 0.0, 0.0, 0.0 },
+ },
+ {"plane-ctm-max", 0,
+ {{ 1.0, 0.0, 0.0 },
+{ 0.0, 1.0, 0.0 },
+{ 0.0, 0.0, 1.0 }},
+ { 100.0, 0.0, 0.0,
+ 0.0, 100.0, 0.0,
+ 0.0, 0.0, 100.0 },
+ },
+ {"plane-ctm-negative", 0,
+ {{ 0.0, 0.0, 0.0 },
+{ 0.0, 0.0, 0.0 },
+{ 0.0, 0.0, 0.0 }},
+ { -1.0, 0.0, 0.0,
+ 0.0, -1.0, 0.0,
+ 0.0, 0.0, -1.0 },
+ },
+ /* We tests a few values around the expected result because
+* it depends on the hardware we're dealing with, we can
+* either get clamped or rounded values and we also need to
+* account for odd number of items in the LUTs.
+*/
+ {"plane-ctm-0-25", 5,
+ {{ 0.0, }, { 0.0, }, { 0.0, }},
+ { 0.25, 0.0, 0.0,
+ 0.0, 0.25, 0.0,
+ 0.0, 0.0, 0.25 },
+ },
+ {"plane-ctm-0-50", 5,
+ {{ 0.0, }, { 0.0, }, { 0.0, }},
+ { 0.5, 0.0, 0.0,
+ 0.0, 0.5, 0.0,
+ 0.0, 0.0, 0.5 },
+ },
+ {"plane-ctm-0-75", 7,
+ {{ 0.0, }, { 0.0, }, { 0.0, }},
+ { 0.75, 0.0, 0.0,
+ 0.0, 0.75, 0.0,
+ 0.0, 0.0, 0.75 },
+ },
+};
+
/*
* Draw 3 gradient rectangles in red, green and blue, with a maxed out
* degamma LUT and verify we have the same frame dump as drawing solid color
@@ -1000,6 +1071,103 @@ static bool plane_degamma_test(data_t *data,
igt_plane_t *plane)
return ret;
}
+static bool test_plane_ctm(data_t *data,
+ igt_plane_t *plane,
+ color_t *before,
+ color_t *after,
+ double *ctm_matrix)
+{
+ igt_output_t *output;
+ igt_display_t *display = &data->display;
+ drmModeModeInfo *mode;
+ struct igt_fb fb, fbref;
+ struct chamelium_port *port;
+ struct chamelium_frame_dump *frame_hardware;
+ uint32_t i;
+ bool ret = true;
+
+ igt_info("Plane CTM test is running on pipe-%s plane-%s(%s)\n",
+ kmstest_pipe_name(plane->pipe->pipe),
+ kmstest_plane_type_name(plane->type),
+ is_hdr_plane(plane) ? "hdr":"sdr");
+
+ igt_require(igt_plane_has_prop(plane, IGT_PLANE_CTM));
+
+ for_each_valid_output_on_pipe(display, plane->pipe->pipe, output) {
+ for (i = 0; i < data->port_count; i++)
+ if (strcmp(output->name,
chamelium_port_get_name(data->ports[i])) == 0) {
+ port = data->ports[i];
+ break;
+ }
+
+ if (port)
+ break;
+ }
+ igt_require(port);
+ ig
[i-g-t 12/14] kms_color_helper: Add helper functions to support logarithmic gamma mode
From: Mukunda Pramodh Kumar
Add helper functions to support logarithmic gamma mode
Cc: Harry Wentland
Cc: Ville Syrjälä
Cc: Juha-Pekka Heikkila
Cc: Uma Shankar
Signed-off-by: Mukunda Pramodh Kumar
Signed-off-by: Bhanuprakash Modem
---
tests/kms_color_helper.c | 127 +++
tests/kms_color_helper.h | 16 +
2 files changed, 143 insertions(+)
diff --git a/tests/kms_color_helper.c b/tests/kms_color_helper.c
index c65b7a0f50..7ea8282df3 100644
--- a/tests/kms_color_helper.c
+++ b/tests/kms_color_helper.c
@@ -190,6 +190,33 @@ struct drm_color_lut *coeffs_to_lut(data_t *data,
return lut;
}
+struct drm_color_lut *coeffs_to_logarithmic_lut(data_t *data,
+ const gamma_lut_t *gamma,
+ uint32_t color_depth,
+ int off)
+{
+ struct drm_color_lut *lut;
+ int i, lut_size = gamma->size;
+ /* This is the maximum value due to 16 bit precision in hardware. */
+ uint32_t max_hw_value = (1 << 16) - 1;
+ unsigned int max_segment_value = 1 << 24;
+
+ lut = malloc(sizeof(struct drm_color_lut) * lut_size);
+
+ for (i = 0; i < lut_size; i++) {
+ double scaling_factor = (double)max_hw_value /
(double)max_segment_value;
+ uint32_t r = MIN((gamma->coeffs[i].r * scaling_factor),
max_hw_value);
+ uint32_t g = MIN((gamma->coeffs[i].g * scaling_factor),
max_hw_value);
+ uint32_t b = MIN((gamma->coeffs[i].b * scaling_factor),
max_hw_value);
+
+ lut[i].red = r;
+ lut[i].green = g;
+ lut[i].blue = b;
+ }
+
+ return lut;
+}
+
void set_degamma(data_t *data,
igt_pipe_t *pipe,
const gamma_lut_t *gamma)
@@ -203,6 +230,15 @@ void set_degamma(data_t *data,
free(lut);
}
+void set_pipe_gamma(igt_pipe_t *pipe,
+ uint64_t value,
+ struct drm_color_lut *lut,
+ uint32_t size)
+{
+ igt_pipe_obj_set_prop_value(pipe, IGT_CRTC_GAMMA_MODE, value);
+ igt_pipe_obj_replace_prop_blob(pipe, IGT_CRTC_GAMMA_LUT, lut, size);
+}
+
void set_gamma(data_t *data,
igt_pipe_t *pipe, const gamma_lut_t *gamma)
{
@@ -241,6 +277,51 @@ void disable_prop(igt_pipe_t *pipe, enum
igt_atomic_crtc_properties prop)
igt_pipe_obj_replace_prop_blob(pipe, prop, NULL, 0);
}
+drmModePropertyPtr get_pipe_gamma_degamma_mode(igt_pipe_t *pipe,
+ enum igt_atomic_crtc_properties
prop)
+{
+ igt_display_t *display = pipe->display;
+ uint32_t prop_id = pipe->props[prop];
+ drmModePropertyPtr drmProp;
+
+ igt_assert(prop_id);
+
+ drmProp = drmModeGetProperty(display->drm_fd, prop_id);
+
+ igt_assert(drmProp);
+ igt_assert(drmProp->count_enums);
+
+ return drmProp;
+}
+
+gamma_lut_t *pipe_create_linear_lut(segment_data_t *info)
+{
+ uint32_t segment, entry, index = 0;
+ double val;
+ int i = 0;
+ gamma_lut_t *gamma = alloc_lut(info->entries_count);
+
+ igt_assert(gamma);
+
+ gamma->size = info->entries_count;
+ for (segment = 0; segment < info->segment_count; segment++) {
+ uint32_t entry_count = info->segment_data[segment].count;
+ uint32_t start = (segment == 0) ? 0 : (1 << (segment - 1));
+ uint32_t end = 1 << segment;
+
+ for (entry = 0; entry < entry_count; entry++) {
+ val = (index == 0) ? /* First entry is Zero. */
+ 0 : start + entry *
+ ((end - start) * 1.0 / entry_count);
+
+ set_rgb(&gamma->coeffs[i++], val);
+ index++;
+ }
+ }
+
+ return gamma;
+}
+
drmModePropertyPtr get_plane_gamma_degamma_mode(igt_plane_t *plane,
enum igt_atomic_plane_properties prop)
{
@@ -331,6 +412,7 @@ segment_data_t *get_segment_data(data_t *data,
info->segment_data = malloc(sizeof(struct drm_color_lut_range) *
info->segment_count);
igt_assert(info->segment_data);
+ info->entries_count = 0;
for (i = 0; i < info->segment_count; i++) {
info->entries_count += lut_range[i].count;
info->segment_data[i] = lut_range[i];
@@ -341,6 +423,51 @@ segment_data_t *get_segment_data(data_t *data,
return info;
}
+void set_advance_gamma(data_t *data, igt_pipe_t *pipe, enum gamma_type type)
+{
+ igt_display_t *display = &data->display;
+ gamma_lut_t *gamma_log;
+ drmModePropertyPtr gamma_mode = NULL;
+ segment_data_t *segment_info = NULL;
+ struct drm_color_lut *lut = NULL;
+ int lut_size = 0;
+
+ drmSetClientCap(data->drm_fd, DRM_CLIENT_CAP_ADVANCE_GAMMA_M
[i-g-t 11/14] lib/igt_kms: Add pipe color mgmt properties
From: Mukunda Pramodh Kumar
Add support for Pipe color management properties.
Cc: Harry Wentland
Cc: Ville Syrjälä
Cc: Juha-Pekka Heikkila
Cc: Uma Shankar
Signed-off-by: Mukunda Pramodh Kumar
Signed-off-by: Bhanuprakash Modem
---
lib/igt_kms.c | 1 +
lib/igt_kms.h | 1 +
2 files changed, 2 insertions(+)
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index fdb83e0f91..677d26fedb 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -592,6 +592,7 @@ const char * const igt_crtc_prop_names[IGT_NUM_CRTC_PROPS]
= {
[IGT_CRTC_CTM] = "CTM",
[IGT_CRTC_GAMMA_LUT] = "GAMMA_LUT",
[IGT_CRTC_GAMMA_LUT_SIZE] = "GAMMA_LUT_SIZE",
+ [IGT_CRTC_GAMMA_MODE] = "GAMMA_MODE",
[IGT_CRTC_DEGAMMA_LUT] = "DEGAMMA_LUT",
[IGT_CRTC_DEGAMMA_LUT_SIZE] = "DEGAMMA_LUT_SIZE",
[IGT_CRTC_MODE_ID] = "MODE_ID",
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 3a1f7243ad..5fac651fa3 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -119,6 +119,7 @@ enum igt_atomic_crtc_properties {
IGT_CRTC_CTM = 0,
IGT_CRTC_GAMMA_LUT,
IGT_CRTC_GAMMA_LUT_SIZE,
+ IGT_CRTC_GAMMA_MODE,
IGT_CRTC_DEGAMMA_LUT,
IGT_CRTC_DEGAMMA_LUT_SIZE,
IGT_CRTC_MODE_ID,
--
2.32.0
[i-g-t 13/14] tests/kms_color: Extended IGT tests to support logarithmic gamma mode
From: Mukunda Pramodh Kumar
Extended IGT tests to support logarithmic gamma mode on pipe
Cc: Harry Wentland
Cc: Ville Syrjälä
Cc: Juha-Pekka Heikkila
Cc: Uma Shankar
Signed-off-by: Mukunda Pramodh Kumar
Signed-off-by: Bhanuprakash Modem
---
tests/kms_color.c | 19 +++
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/tests/kms_color.c b/tests/kms_color.c
index d9fe417ba9..00742afaaf 100644
--- a/tests/kms_color.c
+++ b/tests/kms_color.c
@@ -232,8 +232,6 @@ static void test_pipe_gamma(data_t *data,
igt_require(igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_GAMMA_LUT));
- gamma_full = generate_table_max(data->gamma_lut_size);
-
output = igt_get_single_output_for_pipe(&data->display,
primary->pipe->pipe);
igt_require(output);
@@ -258,10 +256,13 @@ static void test_pipe_gamma(data_t *data,
igt_assert(fb_modeset_id);
igt_plane_set_fb(primary, &fb_modeset);
+ /* Reset Color properties */
disable_ctm(primary->pipe);
disable_degamma(primary->pipe);
- set_gamma(data, primary->pipe, gamma_full);
+ disable_gamma(primary->pipe);
igt_display_commit(&data->display);
+ igt_wait_for_vblank(data->drm_fd,
+ display->pipes[primary->pipe->pipe].crtc_offset);
/* Draw solid colors with no gamma transformation. */
paint_rectangles(data, mode, red_green_blue, &fb);
@@ -274,6 +275,13 @@ static void test_pipe_gamma(data_t *data,
/* Draw a gradient with gamma LUT to remap all values
* to max red/green/blue.
*/
+ if (igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_GAMMA_MODE)) {
+ set_advance_gamma(data, primary->pipe, MAX_GAMMA);
+ } else {
+ gamma_full = generate_table_max(data->gamma_lut_size);
+ set_gamma(data, primary->pipe, gamma_full);
+ igt_display_commit(&data->display);
+ }
paint_gradient_rectangles(data, mode, red_green_blue, &fb);
igt_plane_set_fb(primary, &fb);
igt_display_commit(&data->display);
@@ -581,7 +589,10 @@ static bool test_pipe_ctm(data_t *data,
*/
if (memcmp(before, after, sizeof(color_t))) {
set_degamma(data, primary->pipe, degamma_linear);
- set_gamma(data, primary->pipe, gamma_linear);
+ if (igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_GAMMA_MODE))
+ disable_gamma(primary->pipe);
+ else
+ set_gamma(data, primary->pipe, gamma_linear);
} else {
/* Disable Degamma and Gamma for ctm max test */
disable_degamma(primary->pipe);
--
2.32.0
[i-g-t 14/14] tests/kms_color_chamelium: Extended IGT tests to support logarithmic gamma mode
Extended IGT tests to support logarithmic gamma mode on pipe
Cc: Harry Wentland
Cc: Ville Syrjälä
Cc: Juha-Pekka Heikkila
Cc: Kunal Joshi
Cc: Uma Shankar
Signed-off-by: Mukunda Pramodh Kumar
Signed-off-by: Bhanuprakash Modem
---
tests/kms_color_chamelium.c | 40 ++---
1 file changed, 37 insertions(+), 3 deletions(-)
diff --git a/tests/kms_color_chamelium.c b/tests/kms_color_chamelium.c
index af820565d3..26e940f4c1 100644
--- a/tests/kms_color_chamelium.c
+++ b/tests/kms_color_chamelium.c
@@ -317,10 +317,21 @@ static void test_pipe_gamma(data_t *data,
igt_assert(fbref_id);
igt_plane_set_fb(primary, &fb_modeset);
+
+ /* Reset the color properties */
disable_ctm(primary->pipe);
disable_degamma(primary->pipe);
- set_gamma(data, primary->pipe, gamma_full);
+ disable_gamma(primary->pipe);
igt_display_commit(&data->display);
+ igt_wait_for_vblank(data->drm_fd,
+
data->display.pipes[primary->pipe->pipe].crtc_offset);
+
+ if (igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_GAMMA_MODE)) {
+ set_advance_gamma(data, primary->pipe, MAX_GAMMA);
+ } else {
+ set_gamma(data, primary->pipe, gamma_full);
+ igt_display_commit(&data->display);
+ }
/* Draw solid colors with no gamma transformation. */
paint_rectangles(data, mode, red_green_blue, &fbref);
@@ -343,6 +354,7 @@ static void test_pipe_gamma(data_t *data,
frame_fullcolors, &fbref,
CHAMELIUM_CHECK_ANALOG);
+ /* Cleanup */
disable_gamma(primary->pipe);
igt_plane_set_fb(primary, NULL);
igt_output_set_pipe(output, PIPE_NONE);
@@ -431,7 +443,10 @@ static bool test_pipe_ctm(data_t *data,
if (memcmp(before, after, sizeof(color_t))) {
set_degamma(data, primary->pipe, degamma_linear);
- set_gamma(data, primary->pipe, gamma_linear);
+ if (igt_pipe_obj_has_prop(primary->pipe,
IGT_CRTC_GAMMA_MODE))
+ disable_gamma(primary->pipe);
+ else
+ set_gamma(data, primary->pipe, gamma_linear);
} else {
/* Disable Degamma and Gamma for ctm max test */
disable_degamma(primary->pipe);
@@ -465,6 +480,12 @@ static bool test_pipe_ctm(data_t *data,
igt_output_set_pipe(output, PIPE_NONE);
}
+ /* Cleanup */
+ disable_gamma(primary->pipe);
+ disable_degamma(primary->pipe);
+ disable_ctm(primary->pipe);
+ igt_display_commit(&data->display);
+
free_lut(degamma_linear);
free_lut(gamma_linear);
@@ -561,7 +582,14 @@ static void test_pipe_limited_range_ctm(data_t *data,
igt_plane_set_fb(primary, &fb_modeset);
set_degamma(data, primary->pipe, degamma_linear);
- set_gamma(data, primary->pipe, gamma_linear);
+ /*
+* No need of linear gamma for limited range ctm test
+* Not extending for new API interface.
+*/
+ if (igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_GAMMA_MODE))
+ disable_gamma(primary->pipe);
+ else
+ set_gamma(data, primary->pipe, gamma_linear);
set_ctm(primary->pipe, ctm);
igt_output_set_prop_value(output,
@@ -598,6 +626,12 @@ static void test_pipe_limited_range_ctm(data_t *data,
}
+ /* Cleanup */
+ disable_gamma(primary->pipe);
+ disable_degamma(primary->pipe);
+ disable_ctm(primary->pipe);
+ igt_display_commit(&data->display);
+
free_lut(gamma_linear);
free_lut(degamma_linear);
--
2.32.0
Re: (subset) [PATCH] [PATCH v2] drm/sun4i: fix unmet dependency on RESET_CONTROLLER for PHY_SUN6I_MIPI_DPHY
On Mon, 8 Nov 2021 22:23:51 -0500, Julian Braha wrote: > When PHY_SUN6I_MIPI_DPHY is selected, and RESET_CONTROLLER > is not selected, Kbuild gives the following warning: > > WARNING: unmet direct dependencies detected for PHY_SUN6I_MIPI_DPHY > Depends on [n]: (ARCH_SUNXI [=n] || COMPILE_TEST [=y]) && HAS_IOMEM [=y] && > COMMON_CLK [=y] && RESET_CONTROLLER [=n] > Selected by [y]: > - DRM_SUN6I_DSI [=y] && HAS_IOMEM [=y] && DRM_SUN4I [=y] > > [...] Applied to drm/drm-misc (drm-misc-fixes). Thanks! Maxime
Re: [PATCH 03/11] mmc: bcm2835: stop setting chan_config->slave_id
On Mon, 2021-11-15 at 09:53 +0100, Arnd Bergmann wrote: > From: Arnd Bergmann > > The field is not interpreted by the DMA engine driver, as all the data > is passed from devicetree instead. Remove the assignment so the field > can eventually be deleted. > > Signed-off-by: Arnd Bergmann > --- Reviewed-by: Nicolas Saenz Julienne Regards, Nicolas
[Bug 215025] [amdgpu] Thinkpad A275 hangs on shutdown / screen does not turn on after reboot
https://bugzilla.kernel.org/show_bug.cgi?id=215025 --- Comment #3 from Bjoern Franke (b...@nord-west.org) --- mainline 5.16rc1 is also affected. -- 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 0/1] Enable runtime pm autosuspend by default
Enable runtime pm autosuspend by default Signed-off-by: Tilak Tangudu Tilak Tangudu (1): drm/i915/rpm: Enable runtime pm autosuspend by default drivers/gpu/drm/i915/intel_runtime_pm.c | 4 1 file changed, 4 insertions(+) -- 2.25.1
[PATCH 1/1] drm/i915/rpm: Enable runtime pm autosuspend by default
Enable runtime pm autosuspend by default. Signed-off-by: Tilak Tangudu --- drivers/gpu/drm/i915/intel_runtime_pm.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index eaf7688f517d..ef75f24288ef 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -600,6 +600,10 @@ void intel_runtime_pm_enable(struct intel_runtime_pm *rpm) pm_runtime_use_autosuspend(kdev); } + /* XXX: Enable by default only for newer platforms for now */ + if (GRAPHICS_VER(i915) >= 12) + pm_runtime_allow(kdev); + /* * The core calls the driver load handler with an RPM reference held. * We drop that here and will reacquire it during unloading in -- 2.25.1
Re: [PATCH v6 7/7] drm/mediatek: Add mt8195 DisplayPort driver
Hi,
On Wed, Nov 10, 2021 at 02:06:23PM +0100, Guillaume Ranquet wrote:
> From: Markus Schneider-Pargmann
>
> This patch adds a DisplayPort driver for the Mediatek mt8195 SoC and a
> according phy driver mediatek-dp-phy.
>
> It supports both functional units on the mt8195, the embedded
> DisplayPort as well as the external DisplayPort units. It offers
> hot-plug-detection, audio up to 8 channels, and DisplayPort 1.4 with up
> to 4 lanes.
There's a number of checkpatch --strict errors in there, make sure to fix them.
> The driver creates a child device for the phy. The child device will
> never exist without the parent being active. As they are sharing a
> register range, the parent passes a regmap pointer to the child so that
> both can work with the same register range. The phy driver sets device
> data that is read by the parent to get the phy device that can be used
> to control the phy properties.
If the PHY is in the same register space than the DP controller, why do
you need a separate PHY driver in the first place?
> This driver is based on an initial version by
> Jason-JH.Lin .
>
> Signed-off-by: Markus Schneider-Pargmann
> Signed-off-by: Guillaume Ranquet
> Reported-by: kernel test robot
> ---
> drivers/gpu/drm/drm_edid.c |2 +-
> drivers/gpu/drm/mediatek/Kconfig|7 +
> drivers/gpu/drm/mediatek/Makefile |2 +
> drivers/gpu/drm/mediatek/mtk_dp.c | 3094 +++
> drivers/gpu/drm/mediatek/mtk_dp_reg.h | 568 +
> drivers/gpu/drm/mediatek/mtk_dpi.c | 111 +-
> drivers/gpu/drm/mediatek/mtk_dpi_regs.h | 26 +
> drivers/gpu/drm/mediatek/mtk_drm_drv.c |1 +
> drivers/gpu/drm/mediatek/mtk_drm_drv.h |1 +
> 9 files changed, 3799 insertions(+), 13 deletions(-)
> create mode 100644 drivers/gpu/drm/mediatek/mtk_dp.c
> create mode 100644 drivers/gpu/drm/mediatek/mtk_dp_reg.h
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 500279a82167a..bfd98b50ceb5b 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -5183,7 +5183,7 @@ static void drm_parse_hdmi_deep_color_info(struct
> drm_connector *connector,
>* modes and forbids YCRCB422 support for all video modes per
>* HDMI 1.3 spec.
>*/
> - info->color_formats = DRM_COLOR_FORMAT_RGB444;
> + info->color_formats |= DRM_COLOR_FORMAT_RGB444;
>
> /* YCRCB444 is optional according to spec. */
> if (hdmi[6] & DRM_EDID_HDMI_DC_Y444) {
This looks unrelated?
> diff --git a/drivers/gpu/drm/mediatek/Kconfig
> b/drivers/gpu/drm/mediatek/Kconfig
> index 2976d21e9a34a..029b94c716131 100644
> --- a/drivers/gpu/drm/mediatek/Kconfig
> +++ b/drivers/gpu/drm/mediatek/Kconfig
> @@ -28,3 +28,10 @@ config DRM_MEDIATEK_HDMI
> select PHY_MTK_HDMI
> help
> DRM/KMS HDMI driver for Mediatek SoCs
> +
> +config MTK_DPTX_SUPPORT
> + tristate "DRM DPTX Support for Mediatek SoCs"
> + depends on DRM_MEDIATEK
> + select PHY_MTK_DP
> + help
> + DRM/KMS Display Port driver for Mediatek SoCs.
> diff --git a/drivers/gpu/drm/mediatek/Makefile
> b/drivers/gpu/drm/mediatek/Makefile
> index 29098d7c8307c..d86a6406055e6 100644
> --- a/drivers/gpu/drm/mediatek/Makefile
> +++ b/drivers/gpu/drm/mediatek/Makefile
> @@ -21,3 +21,5 @@ mediatek-drm-hdmi-objs := mtk_cec.o \
> mtk_hdmi_ddc.o
>
> obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mediatek-drm-hdmi.o
> +
> +obj-$(CONFIG_MTK_DPTX_SUPPORT) += mtk_dp.o
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c
> b/drivers/gpu/drm/mediatek/mtk_dp.c
> new file mode 100644
> index 0..83087219d5a5e
> --- /dev/null
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> @@ -0,0 +1,3094 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 MediaTek Inc.
> + * Copyright (c) 2021 BayLibre
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "mtk_dp_reg.h"
> +
> +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20
> +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3
> +
> +#define MTK_DP_MAX_LANES 4
> +#define MTK_DP_MAX_LINK_RATE MTK_DP_LINKRATE_HBR3
> +
> +#define MTK_DP_TBC_BUF_READ_START_ADDR 0x08
> +
> +#define MTK_DP_TRAIN_RETRY_LIMIT 8
> +#define MTK_DP_TRAIN_MAX_ITERATIONS 5
> +
> +#define MTK_DP_AUX_WRITE_READ_WAIT_TIME_US 20
> +
> +#define MTK_DP_DP_VERSION_11 0x11
> +
> +enum mtk_dp_state {
> + MTK_DP_STATE_INITIAL,
> + MTK_DP_STATE_IDLE,
> + MTK_DP_STATE_PREPARE,
> + MTK_DP_STATE_NORMAL,
> +};
> +
> +enum mtk_dp_train_state {
> + MTK_DP_TRAIN_STATE_STARTUP = 0,
> + MTK_DP_TRAIN_STATE_CHECKCAP,
> + MTK_DP_TRAIN_STATE_CHECKEDID,
> + MTK_DP_TRAIN_STATE_TRAINING_PRE,
> + MTK_DP_TRAIN_STA
[Bug 215025] [amdgpu] Thinkpad A275 hangs on shutdown / screen does not turn on after reboot
https://bugzilla.kernel.org/show_bug.cgi?id=215025 --- Comment #4 from Bjoern Franke (b...@nord-west.org) --- It seems it is https://gitlab.freedesktop.org/drm/amd/-/issues/1789 -- 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: [PATCH 08/11] dmaengine: xilinx_dpdma: stop using slave_id field
On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart wrote: > On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote: > > @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan > > *dchan, > > spin_lock_irqsave(&chan->lock, flags); > > > > /* > > - * Abuse the slave_id to indicate that the channel is part of a video > > - * group. > > + * Abuse the peripheral_config to indicate that the channel is part > > Is it still an abuse, or is this now the right way to pass custom data > to the DMA engine driver ? It doesn't make the driver any more portable, but it's now being more explicit about it. As far as I can tell, this is the best way to pass data that cannot be expressed through the regular interfaces in DT and the dmaengine API. Ideally there would be a generic way to pass this flag, but I couldn't figure out what this is actually doing, or whether there is a better way. Maybe Vinod has an idea. I'll change s/Abuse/Use/ for the moment until I get a definite answer. > > + * of a video group. > >*/ > > - if (chan->id <= ZYNQMP_DPDMA_VIDEO2) > > - chan->video_group = config->slave_id != 0; > > + pconfig = config->peripheral_config; > > This could be moved to the variable declaration above, up to you. I considered that but since it doesn't fit in a normal 80-column line, it seemed best to do it here. > > + if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && > > + config->peripheral_size == sizeof(*pconfig)) > > Silently ignoring a size mismatch isn't nice. Could we validate the size > at the beginning of the function and return an error ? Yes, good idea. Since this would mean a bug in another driver, I'll add a WARN_ON() as well to make it clear which driver caused it. This is what I have now, let me know if you have any further suggestions: /* * Use the peripheral_config to indicate that the channel is part * of a video group. This requires matching use of the custom * structure in each driver. */ pconfig = config->peripheral_config; if (WARN_ON(config->peripheral_size != 0 && config->peripheral_size != sizeof(*pconfig))) return -EINVAL; spin_lock_irqsave(&chan->lock, flags); if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && config->peripheral_size == sizeof(*pconfig)) chan->video_group = pconfig->video_group; spin_unlock_irqrestore(&chan->lock, flags); return 0; > With these issues addressed, > > Reviewed-by: Laurent Pinchart Thanks, Arnd
Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
On Sat, 13 Nov 2021, Claudio Suarez wrote:
> The prefered way to log connectors is [CONNECTOR:id:name]. Change it in
> drm core programs.
>
> Suggested-by: Ville Syrjälä
> Signed-off-by: Claudio Suarez
> ---
> drivers/gpu/drm/drm_client_modeset.c | 51 ++--
> drivers/gpu/drm/drm_connector.c | 12 ---
> drivers/gpu/drm/drm_edid.c | 36 ++--
> drivers/gpu/drm/drm_edid_load.c | 11 +++---
> drivers/gpu/drm/drm_mode_config.c| 3 +-
> 5 files changed, 59 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_client_modeset.c
> b/drivers/gpu/drm/drm_client_modeset.c
> index ced09c7c06f9..4f35dc375bdd 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -240,7 +240,7 @@ static void drm_client_connectors_enabled(struct
> drm_connector **connectors,
> for (i = 0; i < connector_count; i++) {
> connector = connectors[i];
> enabled[i] = drm_connector_enabled(connector, true);
> - DRM_DEBUG_KMS("connector %d enabled? %s\n", connector->base.id,
> + DRM_DEBUG_KMS("[CONNECTOR:%d;%s] enabled? %s\n",
> connector->base.id, connector->name,
> connector->display_info.non_desktop ? "non
> desktop" : enabled[i] ? "yes" : "no");
You have a semicolon instead of a colon there.
Not to block this patch, but I've wondered about bigger changes such as:
- Adding "debug_name" member or similar in drm_connector, which would be
an allocated string with "[CONNECTOR:id:name]" that you could use with
just %s while debug logging.
- Adding drm_dbg_connector() which would take drm_connector as context,
and do drm_dbg_kms() with the above prefix.
>
> any_enabled |= enabled[i];
> @@ -350,8 +350,8 @@ static int drm_client_get_tile_offsets(struct
> drm_connector **connectors,
> continue;
>
> if (!modes[i] && (h_idx || v_idx)) {
> - DRM_DEBUG_KMS("no modes for connector tiled %d %d\n", i,
> - connector->base.id);
> + DRM_DEBUG_KMS("no modes for [CONNECTOR:%d:%s] tiled
> %d\n",
> + connector->base.id, connector->name, i);
Personally I'd prefer adding [CONNECTOR:id:name] as a prefix in the
beginning, throughout, not in the middle.
BR,
Jani.
> continue;
> }
> if (connector->tile_h_loc < h_idx)
> @@ -419,14 +419,15 @@ static bool drm_client_target_preferred(struct
> drm_connector **connectors,
> drm_client_get_tile_offsets(connectors,
> connector_count, modes, offsets, i,
> connector->tile_h_loc,
> connector->tile_v_loc);
> }
> - DRM_DEBUG_KMS("looking for cmdline mode on connector %d\n",
> - connector->base.id);
> + DRM_DEBUG_KMS("looking for cmdline mode on [CONNECTOR:%d:%s]\n",
> + connector->base.id, connector->name);
>
> /* got for command line mode first */
> modes[i] = drm_connector_pick_cmdline_mode(connector);
> if (!modes[i]) {
> - DRM_DEBUG_KMS("looking for preferred mode on connector
> %d %d\n",
> - connector->base.id, connector->tile_group
> ? connector->tile_group->id : 0);
> + DRM_DEBUG_KMS("looking for preferred mode on
> [CONNECTOR:%d:%s] %d\n",
> + connector->base.id, connector->name,
> + connector->tile_group ?
> connector->tile_group->id : 0);
> modes[i] = drm_connector_has_preferred_mode(connector,
> width, height);
> }
> /* No preferred modes, pick one off the list */
> @@ -448,8 +449,8 @@ static bool drm_client_target_preferred(struct
> drm_connector **connectors,
> (connector->tile_h_loc == 0 &&
>connector->tile_v_loc == 0 &&
>!drm_connector_get_tiled_mode(connector))) {
> - DRM_DEBUG_KMS("Falling back to non tiled mode
> on Connector %d\n",
> - connector->base.id);
> + DRM_DEBUG_KMS("Falling back to non tiled mode
> on [CONNECTOR:%d:%s]\n",
> + connector->base.id,
> connector->name);
> modes[i] =
> drm_connector_fallback_non_tiled_mode(connector);
> } else {
> modes[i] =
> drm_connector_get_tiled_mode(connector);
> @@ -617,15 +618,15 @@ static bool drm_client_firmware_config(struct
> drm_client_dev *client,
> num_connectors_detected++;
>
> if (!
Re: [PATCH 10/11] staging: ralink-gdma: stop using slave_id config
On Mon, Nov 15, 2021 at 10:55 AM Sergio Paracuellos wrote: > On Mon, Nov 15, 2021 at 9:55 AM Arnd Bergmann wrote: > > --- > > drivers/staging/ralink-gdma/ralink-gdma.c | 12 +--- > > 1 file changed, 5 insertions(+), 7 deletions(-) > > This driver has been already deleted from the staging tree. See [0]. Ok, thanks! I'll just leave out the patch from future submissions, and remove it completely once your commit hits mainline. Arnd
Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
On Sun, 14 Nov 2021, Claudio Suarez wrote: > On Sat, Nov 13, 2021 at 09:39:46PM +0100, Sam Ravnborg wrote: >> Hi Claudio, >> >> On Sat, Nov 13, 2021 at 08:27:30PM +0100, Claudio Suarez wrote: >> > The prefered way to log connectors is [CONNECTOR:id:name]. Change it in >> > drm core programs. >> > >> > Suggested-by: Ville Syrjälä >> > Signed-off-by: Claudio Suarez >> >> While touching all these logging calls, could you convernt them to the >> newer drm_dbg_kms variants? >> DRM_DEBUG_* are all deprecated. >> > > Yes, I can, but it is recommended to do it in a different patch: > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes > > C&P: > "Separate your changes > Separate each logical change into a separate patch. > For example, if your changes include both bug fixes and performance > enhancements..." > > > I will study and send a new separate patch with your suggestion. I feel these logging changes are the types of changes where I'd err on the side of fewer patches than strictly following the above guidelines. BR, Jani. > > Best regards, > Claudio Suarez > > > -- Jani Nikula, Intel Open Source Graphics Center
[PATCH] drm/msm/dp: remove deadcode on dpu_encoder_setup
From: Jackie Liu
Nobody care about drm_enc on dpu_encoder_setup, cleanup.
[...]
*** CID 1493979: Possible Control flow issues (DEADCODE)
/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c: 2186 in dpu_encoder_setup()
2180
2181return ret;
2182
2183 fail:
2184DPU_ERROR("failed to create encoder\n");
2185if (drm_enc)
>>> CID 1493979: Possible Control flow issues (DEADCODE)
>>> Execution cannot reach this statement: "dpu_encoder_destroy(drm_enc);".
2186dpu_encoder_destroy(drm_enc);
2187
2188return ret;
2189
2190
2191 }
[...]
Addresses-Coverity: ("Logically dead code")
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Jackie Liu
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 13 +++--
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index e7ee4cfb8461..67c1a979ad98 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2146,15 +2146,16 @@ int dpu_encoder_setup(struct drm_device *dev, struct
drm_encoder *enc,
{
struct msm_drm_private *priv = dev->dev_private;
struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
- struct drm_encoder *drm_enc = NULL;
struct dpu_encoder_virt *dpu_enc = NULL;
int ret = 0;
dpu_enc = to_dpu_encoder_virt(enc);
ret = dpu_encoder_setup_display(dpu_enc, dpu_kms, disp_info);
- if (ret)
+ if (ret) {
+ DPU_ERROR("failed to create encoder\n");
goto fail;
+ }
atomic_set(&dpu_enc->frame_done_timeout_ms, 0);
timer_setup(&dpu_enc->frame_done_timer,
@@ -2178,16 +2179,8 @@ int dpu_encoder_setup(struct drm_device *dev, struct
drm_encoder *enc,
DPU_DEBUG_ENC(dpu_enc, "created\n");
- return ret;
-
fail:
- DPU_ERROR("failed to create encoder\n");
- if (drm_enc)
- dpu_encoder_destroy(drm_enc);
-
return ret;
-
-
}
struct drm_encoder *dpu_encoder_init(struct drm_device *dev,
--
2.25.1
Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
On Monday, November 15th, 2021 at 11:22, Jani Nikula wrote: > - Adding drm_dbg_connector() which would take drm_connector as context, > and do drm_dbg_kms() with the above prefix. This wouldn't work great in case multiple connectors/planes/etc are involved, or when drm_dbg_atomic() is used.
[PATCH v2 0/1] drm/i915/rpm: Enable runtime pm autosuspend by default
Enable runtime pm autosuspend by default for all platforms Signed-off-by: Tilak Tangudu Tilak Tangudu (1): drm/i915/rpm: Enable runtime pm autosuspend by default drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +++ 1 file changed, 3 insertions(+) -- 2.25.1
[PATCH v2 1/1] drm/i915/rpm: Enable runtime pm autosuspend by default
v1: Enable runtime pm autosuspend by default for Gen12 and later versions. v2: Enable runtime pm autosuspend by default for all platforms(Syrjala Ville) Signed-off-by: Tilak Tangudu --- drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index eaf7688f517d..f26ed1427fdc 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -600,6 +600,9 @@ void intel_runtime_pm_enable(struct intel_runtime_pm *rpm) pm_runtime_use_autosuspend(kdev); } + /* Enable by default */ + pm_runtime_allow(kdev); + /* * The core calls the driver load handler with an RPM reference held. * We drop that here and will reacquire it during unloading in -- 2.25.1
Re: [Intel-gfx] [PATCH] drm/i915/execlists: Weak parallel submission support for execlists
On 12/11/2021 17:59, Matthew Brost wrote:
On Fri, Nov 12, 2021 at 02:13:50PM +, Tvrtko Ursulin wrote:
On 11/11/2021 16:49, Matthew Brost wrote:
On Mon, Nov 01, 2021 at 10:35:09AM +, Tvrtko Ursulin wrote:
On 27/10/2021 21:10, Matthew Brost wrote:
On Wed, Oct 27, 2021 at 01:04:49PM -0700, John Harrison wrote:
On 10/27/2021 12:17, Matthew Brost wrote:
On Tue, Oct 26, 2021 at 02:58:00PM -0700, John Harrison wrote:
On 10/20/2021 14:47, Matthew Brost wrote:
A weak implementation of parallel submission (multi-bb execbuf IOCTL) for
execlists. Doing as little as possible to support this interface for
execlists - basically just passing submit fences between each request
generated and virtual engines are not allowed. This is on par with what
is there for the existing (hopefully soon deprecated) bonding interface.
We perma-pin these execlists contexts to align with GuC implementation.
v2:
(John Harrison)
- Drop siblings array as num_siblings must be 1
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 10 +++--
drivers/gpu/drm/i915/gt/intel_context.c | 4 +-
.../drm/i915/gt/intel_execlists_submission.c | 44 ++-
drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 2 -
5 files changed, 52 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index fb33d0322960..35e87a7d0ea9 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -570,10 +570,6 @@ set_proto_ctx_engines_parallel_submit(struct
i915_user_extension __user *base,
struct intel_engine_cs **siblings = NULL;
intel_engine_mask_t prev_mask;
- /* FIXME: This is NIY for execlists */
- if (!(intel_uc_uses_guc_submission(&i915->gt.uc)))
- return -ENODEV;
-
if (get_user(slot, &ext->engine_index))
return -EFAULT;
@@ -583,6 +579,12 @@ set_proto_ctx_engines_parallel_submit(struct
i915_user_extension __user *base,
if (get_user(num_siblings, &ext->num_siblings))
return -EFAULT;
+ if (!intel_uc_uses_guc_submission(&i915->gt.uc) && num_siblings != 1) {
+ drm_dbg(&i915->drm, "Only 1 sibling (%d) supported in non-GuC
mode\n",
+ num_siblings);
+ return -EINVAL;
+ }
+
if (slot >= set->num_engines) {
drm_dbg(&i915->drm, "Invalid placement value, %d >= %d\n",
slot, set->num_engines);
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c
b/drivers/gpu/drm/i915/gt/intel_context.c
index 5634d14052bc..1bec92e1d8e6 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -79,7 +79,8 @@ static int intel_context_active_acquire(struct intel_context
*ce)
__i915_active_acquire(&ce->active);
- if (intel_context_is_barrier(ce) || intel_engine_uses_guc(ce->engine))
+ if (intel_context_is_barrier(ce) || intel_engine_uses_guc(ce->engine) ||
+ intel_context_is_parallel(ce))
return 0;
/* Preallocate tracking nodes */
@@ -563,7 +564,6 @@ void intel_context_bind_parent_child(struct intel_context
*parent,
* Callers responsibility to validate that this function is used
* correctly but we use GEM_BUG_ON here ensure that they do.
*/
- GEM_BUG_ON(!intel_engine_uses_guc(parent->engine));
GEM_BUG_ON(intel_context_is_pinned(parent));
GEM_BUG_ON(intel_context_is_child(parent));
GEM_BUG_ON(intel_context_is_pinned(child));
diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
index bedb80057046..2865b422300d 100644
--- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
+++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
@@ -927,8 +927,7 @@ static void execlists_submit_ports(struct intel_engine_cs
*engine)
static bool ctx_single_port_submission(const struct intel_context *ce)
{
- return (IS_ENABLED(CONFIG_DRM_I915_GVT) &&
- intel_context_force_single_submission(ce));
+ return intel_context_force_single_submission(ce);
I think this is actually going to break GVT.
Not so much this change here but the whole use of single submission outside
of GVT. It looks like the GVT driver overloads the single submission flag to
tag requests that it owns. If we start using that flag elsewhere when GVT is
active, I think that will cause much confusion within the GVT code.
The correct fix would be to create a new flag just for GVT usage alongside
the single submission one. GVT would then set both but only check for its
own private flag. The parallel code would obviously only set the existing
single submission flag.
Ok, see below.
}
stati
Re: [PATCH] drm: change logs to print connectors in the form CONNECTOR:id:name
On Mon, 15 Nov 2021, Simon Ser wrote: > On Monday, November 15th, 2021 at 11:22, Jani Nikula > wrote: > >> - Adding drm_dbg_connector() which would take drm_connector as context, >> and do drm_dbg_kms() with the above prefix. > > This wouldn't work great in case multiple connectors/planes/etc are involved, > or when drm_dbg_atomic() is used. That's a good point, though you could still roll those cases manually. It's also misleading as otherwise drm_dbg_* are categories that can be enabled/disabled via drm.debug. Again, just musing here. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH 01/11] ASoC: dai_dma: remove slave_id field
On Mon, Nov 15, 2021 at 11:14 AM Lars-Peter Clausen wrote: > > On 11/15/21 9:53 AM, Arnd Bergmann wrote: > > From: Arnd Bergmann > > > > This field is never set, and serves no purpose, so remove it. > > I agree that we should remove it. Its been legacy support code for a > while, but the description that there is no user is not right. > > The tegra20_spdif driver obviously uses it and that user is removed in > this patch. I think it makes sense to split that out into a separate > patch with a description why the driver will still work even with > slave_id removed. Maybe the best is to remove the whole tegra20_spdif > driver. Ok, I'll split out the tegra patch and try to come up with a better description for it. What I saw in that driver is it just passes down the slave_id number from a 'struct resource', but there is nothing in the kernel that sets up this resource. Do you or someone else have more information on the state of this driver? I can see that it does not contain any of_device_id based probing, so it seems that this is either dead code, the platform_device gets created by some other code that is no longer compatible with this driver. Arnd
Re: [PATCH v3 3/6] drm/i915/ttm: Move the i915_gem_obj_copy_ttm() function
On 14/11/2021 11:12, Thomas Hellström wrote: Move the i915_gem_obj_copy_ttm() function to i915_gem_ttm_move.h. This will help keep a number of functions static when introducing async moves. Signed-off-by: Thomas Hellström Reviewed-by: Matthew Auld
Re: [PATCH 01/11] ASoC: dai_dma: remove slave_id field
On 11/15/21 9:53 AM, Arnd Bergmann wrote: From: Arnd Bergmann This field is never set, and serves no purpose, so remove it. I agree that we should remove it. Its been legacy support code for a while, but the description that there is no user is not right. The tegra20_spdif driver obviously uses it and that user is removed in this patch. I think it makes sense to split that out into a separate patch with a description why the driver will still work even with slave_id removed. Maybe the best is to remove the whole tegra20_spdif driver. diff --git a/sound/soc/tegra/tegra20_spdif.c b/sound/soc/tegra/tegra20_spdif.c index 9fdc82d58db3..1c3385da6f82 100644 --- a/sound/soc/tegra/tegra20_spdif.c +++ b/sound/soc/tegra/tegra20_spdif.c @@ -284,7 +284,6 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev) spdif->playback_dma_data.addr = mem->start + TEGRA20_SPDIF_DATA_OUT; spdif->playback_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; spdif->playback_dma_data.maxburst = 4; - spdif->playback_dma_data.slave_id = dmareq->start; dmareq is now unused and should be removed as well. pm_runtime_enable(&pdev->dev);
Re: [PATCH v3 4/6] drm/i915/ttm: Break refcounting loops at device region unref time
On 14/11/2021 11:12, Thomas Hellström wrote:
There is an interesting refcounting loop:
struct intel_memory_region has a struct ttm_resource_manager,
ttm_resource_manager->move may hold a reference to i915_request,
i915_request may hold a reference to intel_context,
intel_context may hold a reference to drm_i915_gem_object,
drm_i915_gem_object may hold a reference to intel_memory_region.
Would it help if we drop the per object region refcoutning? IIRC that
was originally added to more cleanly appease some selftest teardown or
something.
Break this loop when we drop the device reference count on the
region by putting the region move fence.
Also hold dropping the device reference count until all objects of
the region has been deleted, to avoid issues if proceeding with the
device takedown while the region is still present.
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 1 +
drivers/gpu/drm/i915/gt/intel_region_lmem.c | 1 +
drivers/gpu/drm/i915/intel_memory_region.c | 5 +++-
drivers/gpu/drm/i915/intel_memory_region.h | 1 +
drivers/gpu/drm/i915/intel_region_ttm.c | 28 +
drivers/gpu/drm/i915/intel_region_ttm.h | 2 ++
6 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
index 537a81445b90..a1df49378a0f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
@@ -1044,6 +1044,7 @@ int __i915_gem_ttm_object_init(struct intel_memory_region
*mem,
static const struct intel_memory_region_ops ttm_system_region_ops = {
.init_object = __i915_gem_ttm_object_init,
+ .disable = intel_region_ttm_disable,
};
struct intel_memory_region *
diff --git a/drivers/gpu/drm/i915/gt/intel_region_lmem.c
b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
index aec838ecb2ef..956916fd21f8 100644
--- a/drivers/gpu/drm/i915/gt/intel_region_lmem.c
+++ b/drivers/gpu/drm/i915/gt/intel_region_lmem.c
@@ -108,6 +108,7 @@ region_lmem_init(struct intel_memory_region *mem)
static const struct intel_memory_region_ops intel_region_lmem_ops = {
.init = region_lmem_init,
.release = region_lmem_release,
+ .disable = intel_region_ttm_disable,
.init_object = __i915_gem_ttm_object_init,
};
diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c
index e7f7e6627750..1f67d2b68c24 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.c
+++ b/drivers/gpu/drm/i915/intel_memory_region.c
@@ -233,8 +233,11 @@ void intel_memory_regions_driver_release(struct
drm_i915_private *i915)
struct intel_memory_region *region =
fetch_and_zero(&i915->mm.regions[i]);
- if (region)
+ if (region) {
+ if (region->ops->disable)
+ region->ops->disable(region);
intel_memory_region_put(region);
+ }
}
}
diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h
index 3feae3353d33..9bb77eacd206 100644
--- a/drivers/gpu/drm/i915/intel_memory_region.h
+++ b/drivers/gpu/drm/i915/intel_memory_region.h
@@ -52,6 +52,7 @@ struct intel_memory_region_ops {
int (*init)(struct intel_memory_region *mem);
void (*release)(struct intel_memory_region *mem);
+ void (*disable)(struct intel_memory_region *mem);
int (*init_object)(struct intel_memory_region *mem,
struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/intel_region_ttm.c
b/drivers/gpu/drm/i915/intel_region_ttm.c
index 2e901a27e259..4219d83a2b19 100644
--- a/drivers/gpu/drm/i915/intel_region_ttm.c
+++ b/drivers/gpu/drm/i915/intel_region_ttm.c
@@ -114,6 +114,34 @@ void intel_region_ttm_fini(struct intel_memory_region *mem)
mem->region_private = NULL;
}
+/**
+ * intel_region_ttm_disable - A TTM region disable callback helper
+ * @mem: The memory region.
+ *
+ * A helper that ensures that nothing any longer references a region at
+ * device takedown. Breaks refcounting loops and waits for objects in the
+ * region to be deleted.
+ */
+void intel_region_ttm_disable(struct intel_memory_region *mem)
+{
+ struct ttm_resource_manager *man = mem->region_private;
+
+ /*
+* Put the region's move fences. This releases requests that
+* may hold on to contexts and vms that may hold on to buffer
+* objects that may have a refcount on the region. :/
+*/
+ if (man)
+ ttm_resource_manager_cleanup(man);
+
+ /* Flush objects that may just have been freed */
+ i915_gem_flush_free_objects(mem->i915);
+
+ /* Wait until the only region reference left is our own. */
+ while (kref_read(&mem->kref) > 1)
+ msleep(20);
If we leak an object, I guess we get an
Re: [PATCH] drm/i915/dp: Perform 30ms delay after source OUI write
On Fri, 12 Nov 2021, Lyude Paul wrote:
> While working on supporting the Intel HDR backlight interface, I noticed
> that there's a couple of laptops that will very rarely manage to boot up
> without detecting Intel HDR backlight support - even though it's supported
> on the system. One example of such a laptop is the Lenovo P17 1st
> generation.
>
> Following some investigation Ville Syrjälä did through the docs they have
> available to them, they discovered that there's actually supposed to be a
> 30ms wait after writing the source OUI before we begin setting up the rest
> of the backlight interface.
>
> This seems to be correct, as adding this 30ms delay seems to have
> completely fixed the probing issues I was previously seeing. So - let's
> start performing a 30ms wait after writing the OUI, which we do in a manner
> similar to how we keep track of PPS delays (e.g. record the timestamp of
> the OUI write, and then wait for however many ms are left since that
> timestamp right before we interact with the backlight) in order to avoid
> waiting any longer then we need to. As well, this also avoids us performing
> this delay on systems where we don't end up using the HDR backlight
> interface.
Ugh. Thanks for digging into this.
The only thing that I dislike with the implementation is splitting the
implementation to two places. See how well we've managed to shove all of
the PPS waits inside intel_pps.c. Almost all of intel_dp->pps is managed
within intel_pps.c.
I think I'd actually add a intel_dp_wait_source_oui() or something in
intel_dp.c, so all of the details about source OUI and
intel_dp->last_oui_write access would be localized.
BR,
Jani.
>
> Signed-off-by: Lyude Paul
> Fixes: 4a8d79901d5b ("drm/i915/dp: Enable Intel's HDR backlight interface
> (only SDR for now)")
> Cc: Ville Syrjälä
> Cc: # v5.12+
> ---
> drivers/gpu/drm/i915/display/intel_display_types.h| 3 +++
> drivers/gpu/drm/i915/display/intel_dp.c | 3 +++
> drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c | 11 +++
> 3 files changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index ea1e8a6e10b0..b9c967837872 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1653,6 +1653,9 @@ struct intel_dp {
> struct intel_dp_pcon_frl frl;
>
> struct intel_psr psr;
> +
> + /* When we last wrote the OUI for eDP */
> + unsigned long last_oui_write;
> };
>
> enum lspcon_vendor {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0a424bf69396..77d9a9390c1e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -2010,6 +2011,8 @@ intel_edp_init_source_oui(struct intel_dp *intel_dp,
> bool careful)
>
> if (drm_dp_dpcd_write(&intel_dp->aux, DP_SOURCE_OUI, oui, sizeof(oui))
> < 0)
> drm_err(&i915->drm, "Failed to write source OUI\n");
> +
> + intel_dp->last_oui_write = jiffies;
> }
>
> /* If the device supports it, try to set the power state appropriately */
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> index 569d17b4d00f..2c35b999ec2c 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
> @@ -96,6 +96,13 @@
> #define INTEL_EDP_BRIGHTNESS_OPTIMIZATION_10x359
>
> /* Intel EDP backlight callbacks */
> +static void
> +wait_for_oui(struct drm_i915_private *i915, struct intel_dp *intel_dp)
> +{
> + drm_dbg_kms(&i915->drm, "Performing OUI wait\n");
> + wait_remaining_ms_from_jiffies(intel_dp->last_oui_write, 30);
> +}
> +
> static bool
> intel_dp_aux_supports_hdr_backlight(struct intel_connector *connector)
> {
> @@ -106,6 +113,8 @@ intel_dp_aux_supports_hdr_backlight(struct
> intel_connector *connector)
> int ret;
> u8 tcon_cap[4];
>
> + wait_for_oui(i915, intel_dp);
> +
> ret = drm_dp_dpcd_read(aux, INTEL_EDP_HDR_TCON_CAP0, tcon_cap,
> sizeof(tcon_cap));
> if (ret != sizeof(tcon_cap))
> return false;
> @@ -204,6 +213,8 @@ intel_dp_aux_hdr_enable_backlight(const struct
> intel_crtc_state *crtc_state,
> int ret;
> u8 old_ctrl, ctrl;
>
> + wait_for_oui(i915, intel_dp);
> +
> ret = drm_dp_dpcd_readb(&intel_dp->aux,
> INTEL_EDP_HDR_GETSET_CTRL_PARAMS, &old_ctrl);
> if (ret != 1) {
> drm_err(&i915->drm, "Failed to read current backlight control
> mode: %d\n", ret);
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v1 1/3] string: Consolidate yesno() helpers under string.h hood
On Tue, Oct 05, 2021 at 02:34:23PM -0700, Lucas De Marchi wrote: > On Mon, Feb 15, 2021 at 04:21:35PM +0200, Andy Shevchenko wrote: > > We have already few similar implementation and a lot of code that can > > benefit > > of the yesno() helper. Consolidate yesno() helpers under string.h hood. > > I was taking a look on i915_utils.h to reduce it and move some of it > elsewhere to be shared with others. I was starting with these helpers > and had [1] done, then Jani pointed me to this thread and also his > previous tentative. I thought the natural place for this would be > include/linux/string_helpers.h, but I will leave it up to you. Seems reasonable to use string_helpers (headers and/or C-file). > After reading the threads, I don't see real opposition to it. > Is there a tree you plan to take this through? I rest my series in favour of Jani's approach, so I suppose there is no go for _this_ series. > [1] > https://lore.kernel.org/lkml/20211005212634.3223113-1-lucas.demar...@intel.com/T/#u -- With Best Regards, Andy Shevchenko
Re: [RESEND PATCH v2 04/13] backlight: qcom-wled: Fix off-by-one maximum with default num_strings
On Fri, Nov 12, 2021 at 10:43:37PM +0100, Marijn Suijten wrote:
> On 2021-11-12 13:35:03, Marijn Suijten wrote:
> > On 2021-11-12 12:08:39, Daniel Thompson wrote:
> > > On Fri, Nov 12, 2021 at 01:26:57AM +0100, Marijn Suijten wrote:
> > > > When not specifying num-strings in the DT the default is used, but +1 is
> > > > added to it which turns WLED3 into 4 and WLED4/5 into 5 strings instead
> > > > of 3 and 4 respectively, causing out-of-bounds reads and register
> > > > read/writes. This +1 exists for a deficiency in the DT parsing code,
> > > > and is simply omitted entirely - solving this oob issue - by parsing the
> > > > property separately much like qcom,enabled-strings.
> > > >
> > > > This also allows more stringent checks on the maximum value when
> > > > qcom,enabled-strings is provided in the DT. Note that num-strings is
> > > > parsed after enabled-strings to give it final sign-off over the length,
> > > > which DT currently utilizes to get around an incorrect fixed read of
> > > > four elements from that array (has been addressed in a prior patch).
> > > >
> > > > Fixes: 93c64f1ea1e8 ("leds: add Qualcomm PM8941 WLED driver")
> > > > Signed-off-by: Marijn Suijten
> > > > Reviewed-By: AngeloGioacchino Del Regno
> > > >
> > > > ---
> > > > drivers/video/backlight/qcom-wled.c | 51 +++--
> > > > 1 file changed, 19 insertions(+), 32 deletions(-)
> > > >
> > > > diff --git a/drivers/video/backlight/qcom-wled.c
> > > > b/drivers/video/backlight/qcom-wled.c
> > > > index 977cd75827d7..c5232478a343 100644
> > > > --- a/drivers/video/backlight/qcom-wled.c
> > > > +++ b/drivers/video/backlight/qcom-wled.c
> > > > @@ -1552,6 +1520,25 @@ static int wled_configure(struct wled *wled)
> > > > }
> > > > }
> > > >
> > > > + rc = of_property_read_u32(dev->of_node, "qcom,num-strings",
> > > > &val);
> > > > + if (!rc) {
> > > > + if (val < 1 || val > wled->max_string_count) {
> > > > + dev_err(dev, "qcom,num-strings must be between
> > > > 1 and %d\n",
> > > > + wled->max_string_count);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + if (string_len > 0) {
> > > > + dev_warn(dev, "qcom,num-strings and
> > > > qcom,enabled-strings are ambiguous\n");
> > >
> > > The warning should also be below the error message on the next if
> > > statement.
> >
> > Agreed.
>
> Thinking about this again while reworking the patches, I initially put
> this above the error to make DT writers aware. There's no point telling
> them that their values are out of sync (num-strings >
> len(enabled-strings)), when they "shouldn't even" (don't need to) set
> both in the first place. They might needlessly fix the discrepancy, see
> the driver finally probe (working backlight) and carry on without
> noticing this warning that now appears.
>
> Sorry for bringing this back up, but I'm curious about your opinion.
With a more helpful warning about how to fix then I think it is OK to
have both the warning and the error.
Daniel.
[PATCH 0/6] drm/vc4: kms: Misc fixes for HVS commits
Hi, The conversion to DRM commit helpers (f3c420fe19f8, "drm/vc4: kms: Convert to atomic helpers") introduced a number of issues in corner cases, most of them showing themselves in the form of either a vblank timeout or use-after-free error. These patches should fix most of them, some of them still being debugged. Maxime Maxime Ripard (6): drm/vc4: kms: Wait for the commit before increasing our clock rate drm/vc4: kms: Fix return code check drm/vc4: kms: Add missing drm_crtc_commit_put drm/vc4: kms: Clear the HVS FIFO commit pointer once done drm/vc4: kms: Don't duplicate pending commit drm/vc4: kms: Fix previous HVS commit wait drivers/gpu/drm/vc4/vc4_kms.c | 36 ++- 1 file changed, 14 insertions(+), 22 deletions(-) -- 2.33.1
[PATCH 2/6] drm/vc4: kms: Fix return code check
The HVS global state functions return an error pointer, but in most
cases we check if it's NULL, possibly resulting in an invalid pointer
dereference.
Fixes: 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a
commit")
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/vc4_kms.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 764ddb41a4ce..3f780c195749 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -354,7 +354,7 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state
*state)
}
old_hvs_state = vc4_hvs_get_old_global_state(state);
- if (!old_hvs_state)
+ if (IS_ERR(old_hvs_state))
return;
for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
@@ -410,8 +410,8 @@ static int vc4_atomic_commit_setup(struct drm_atomic_state
*state)
unsigned int i;
hvs_state = vc4_hvs_get_new_global_state(state);
- if (!hvs_state)
- return -EINVAL;
+ if (WARN_ON(IS_ERR(hvs_state)))
+ return PTR_ERR(hvs_state);
for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
struct vc4_crtc_state *vc4_crtc_state =
@@ -762,8 +762,8 @@ static int vc4_pv_muxing_atomic_check(struct drm_device
*dev,
unsigned int i;
hvs_new_state = vc4_hvs_get_global_state(state);
- if (!hvs_new_state)
- return -EINVAL;
+ if (IS_ERR(hvs_new_state))
+ return PTR_ERR(hvs_new_state);
for (i = 0; i < ARRAY_SIZE(hvs_new_state->fifo_state); i++)
if (!hvs_new_state->fifo_state[i].in_use)
--
2.33.1
[PATCH 1/6] drm/vc4: kms: Wait for the commit before increasing our clock rate
Several DRM/KMS atomic commits can run in parallel if they affect
different CRTC. These commits share the global HVS state, so we have
some code to make sure we run commits in sequence. This synchronization
code is one of the first thing that runs in vc4_atomic_commit_tail().
Another constraints we have is that we need to make sure the HVS clock
gets a boost during the commit. That code relies on clk_set_min_rate and
will remove the old minimum and set a new one. We also need another,
temporary, minimum for the duration of the commit.
The algorithm is thus to set a temporary minimum, drop the previous
one, do the commit, and finally set the minimum for the current mode.
However, the part that sets the temporary minimum and drops the older
one runs before the commit synchronization code.
Thus, under the proper conditions, we can end up mixing up the minimums
and ending up with the wrong one for our current step.
To avoid it, let's move the clock setup in the protected section.
Fixes: d7d96c00e585 ("drm/vc4: hvs: Boost the core clock during modeset")
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/vc4_kms.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index f0b3e4cf5bce..764ddb41a4ce 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -353,9 +353,6 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state
*state)
vc4_hvs_mask_underrun(dev, vc4_crtc_state->assigned_channel);
}
- if (vc4->hvs->hvs5)
- clk_set_min_rate(hvs->core_clk, 5);
-
old_hvs_state = vc4_hvs_get_old_global_state(state);
if (!old_hvs_state)
return;
@@ -377,6 +374,9 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state
*state)
drm_err(dev, "Timed out waiting for commit\n");
}
+ if (vc4->hvs->hvs5)
+ clk_set_min_rate(hvs->core_clk, 5);
+
drm_atomic_helper_commit_modeset_disables(dev, state);
vc4_ctm_commit(vc4, state);
--
2.33.1
[PATCH 3/6] drm/vc4: kms: Add missing drm_crtc_commit_put
Commit 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a
commit") introduced a global state for the HVS, with each FIFO storing
the current CRTC commit so that we can properly synchronize commits.
However, the refcounting was off and we thus ended up leaking the
drm_crtc_commit structure every commit. Add a drm_crtc_commit_put to
prevent the leakage.
Fixes: 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a
commit")
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/vc4_kms.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 3f780c195749..4847d1af399a 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -372,6 +372,8 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state
*state)
ret =
drm_crtc_commit_wait(old_hvs_state->fifo_state[channel].pending_commit);
if (ret)
drm_err(dev, "Timed out waiting for commit\n");
+
+ drm_crtc_commit_put(commit);
}
if (vc4->hvs->hvs5)
--
2.33.1
[PATCH 4/6] drm/vc4: kms: Clear the HVS FIFO commit pointer once done
Commit 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a
commit") introduced a wait on the previous commit done on a given HVS
FIFO.
However, we never cleared that pointer once done. Since
drm_crtc_commit_put can free the drm_crtc_commit structure directly if
we were the last user, this means that it can lead to a use-after free
if we were to duplicate the state, and that stale pointer would even be
copied to the new state.
Set the pointer to NULL once we're done with the wait so that we don't
carry over a pointer to a free'd structure.
Fixes: 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a
commit")
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/vc4_kms.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 4847d1af399a..217a2009c651 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -374,6 +374,7 @@ static void vc4_atomic_commit_tail(struct drm_atomic_state
*state)
drm_err(dev, "Timed out waiting for commit\n");
drm_crtc_commit_put(commit);
+ old_hvs_state->fifo_state[channel].pending_commit = NULL;
}
if (vc4->hvs->hvs5)
--
2.33.1
[PATCH 5/6] drm/vc4: kms: Don't duplicate pending commit
Our HVS global state, when duplicated, will also copy the pointer to the
drm_crtc_commit (and increase the reference count) for each FIFO if the
pointer is not NULL.
However, our atomic_setup function will overwrite that pointer without
putting the reference back leading to a memory leak.
Since the commit is only relevant during the atomic commit process, it
doesn't make sense to duplicate the reference to the commit anyway.
Let's remove it.
Fixes: 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a
commit")
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/vc4_kms.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 217a2009c651..6533f3360e75 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -671,12 +671,6 @@ vc4_hvs_channels_duplicate_state(struct drm_private_obj
*obj)
for (i = 0; i < HVS_NUM_CHANNELS; i++) {
state->fifo_state[i].in_use = old_state->fifo_state[i].in_use;
-
- if (!old_state->fifo_state[i].pending_commit)
- continue;
-
- state->fifo_state[i].pending_commit =
-
drm_crtc_commit_get(old_state->fifo_state[i].pending_commit);
}
return &state->base;
--
2.33.1
[PATCH 6/6] drm/vc4: kms: Fix previous HVS commit wait
Our current code is supposed to serialise the commits by waiting for all
the drm_crtc_commits associated to the previous HVS state.
However, assuming we have two CRTCs running and being configured and we
configure each one alternatively, we end up in a situation where we're
not waiting at all.
Indeed, starting with a state (state 0) where both CRTCs are running,
and doing a commit (state 1) on the first CRTC (CRTC 0), we'll associate
its commit to its assigned FIFO in vc4_hvs_state.
If we get a new commit (state 2), this time affecting the second CRTC
(CRTC 1), the DRM core will allow both commits to execute in parallel
(assuming they don't have any share resources).
Our code in vc4_atomic_commit_tail is supposed to make sure we only get
one commit at a time and serialised by order of submission. It does so
by using for_each_old_crtc_in_state, making sure that the CRTC has a
FIFO assigned, is used, and has a commit pending. If it does, then we'll
wait for the commit before going forward.
During the transition from state 0 to state 1, as our old CRTC state we
get the CRTC 0 state 0, its commit, we wait for it, everything works fine.
During the transition from state 1 to state 2 though, the use of
for_each_old_crtc_in_state is wrong. Indeed, while the code assumes it's
returning the state of the CRTC in the old state (so CRTC 0 state 1), it
actually returns the old state of the CRTC affected by the current
commit, so CRTC 0 state 0 since it wasn't part of state 1.
Due to this, if we alternate between the configuration of CRTC 0 and
CRTC 1, we never actually wait for anything since we should be waiting
on the other every time, but it never is affected by the previous
commit.
Change the logic to, at every commit, look at every FIFO in the previous
HVS state, and if it's in use and has a commit associated to it, wait
for that commit.
Fixes: 9ec03d7f1ed3 ("drm/vc4: kms: Wait on previous FIFO users before a
commit")
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/vc4_kms.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 6533f3360e75..d7a2bcb7d723 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -337,10 +337,10 @@ static void vc4_atomic_commit_tail(struct
drm_atomic_state *state)
struct drm_device *dev = state->dev;
struct vc4_dev *vc4 = to_vc4_dev(dev);
struct vc4_hvs *hvs = vc4->hvs;
- struct drm_crtc_state *old_crtc_state;
struct drm_crtc_state *new_crtc_state;
struct drm_crtc *crtc;
struct vc4_hvs_state *old_hvs_state;
+ unsigned int channel;
int i;
for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
@@ -357,15 +357,10 @@ static void vc4_atomic_commit_tail(struct
drm_atomic_state *state)
if (IS_ERR(old_hvs_state))
return;
- for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
- struct vc4_crtc_state *vc4_crtc_state =
- to_vc4_crtc_state(old_crtc_state);
- unsigned int channel = vc4_crtc_state->assigned_channel;
+ for (channel = 0; channel < HVS_NUM_CHANNELS; channel++) {
+ struct drm_crtc_commit *commit;
int ret;
- if (channel == VC4_HVS_CHANNEL_DISABLED)
- continue;
-
if (!old_hvs_state->fifo_state[channel].in_use)
continue;
--
2.33.1
Re: [BUG] VC4 DRM waiting for flip down makes UI freeze a while with kernel 5.15
Hi,
On Fri, Nov 12, 2021 at 11:22:49AM +0800, Jian-Hong Pan wrote:
> Hi,
>
> I tested Linux mainline kernel 5.15 (aarch64) with enabled VC4 on RPi 4B. I
> notice UI freezes a while (about 10 seconds) some times.
>
> The kernel shows the error message during the time:
>
> [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out
> [drm:drm_atomic_helper_wait_for_flip_done] *ERROR* [CRTC:68:crtc-3]
> flip_done timed out
> [drm:drm_atomic_helper_wait_for_dependencies] *ERROR* [CRTC:68:crtc-3]
> commit wait timed out
> [drm:drm_crtc_commit_wait] *ERROR* flip_done timed out
> vc4-drm gpu: [drm] *ERROR* Timed out waiting for commit
>
> Here is the full log:
> https://github.com/lategoodbye/rpi-zero/files/7518076/dmesg-5.15.log
>
> It is easy to reproduce this issue by invoking GL related things, for
> example "es2gears".
>
> After detail test, I found it is related to these commits:
> * f3c420fe19f8 ("drm/vc4: kms: Convert to atomic helpers")
> * 82faa3276012 ("drm/vc4: kms: Remove async modeset semaphore")
>
> This issue cannot be reproduced after I revert the commits.
I've been working on those regressions recently, and just sent all the
fixes I have so far:
https://lore.kernel.org/dri-devel/2025113105.103275-1-max...@cerno.tech/
Based on the symptoms you provided, I think it should be fixed with
those patches
Let me know how it goes
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v1 1/1] drm: Replace kernel.h with the necessary inclusions
On Wed, Nov 10, 2021 at 05:39:33PM +0100, Thomas Zimmermann wrote: > Am 10.11.21 um 17:34 schrieb Andy Shevchenko: > > On Wed, Nov 10, 2021 at 3:55 PM Thomas Zimmermann > > wrote: > > > Am 10.11.21 um 11:24 schrieb Andy Shevchenko: ... > > > > +#include > > > > > > I built this patch on a recent drm-misc-next, but there's no > > > linux/container_of.h > > > > Thank you for trying. It's in the upstream, whenever drm-misc-next > > switches to newer/newest upstream it will be there. I assume it will > > happen after v5.16-rc1? > > Yes, we'll certainly backmerge soon after rc1 has been released. If I forget > to add the patch then, please send a reminder. > > Once the necessary headers are available, $ git log --oneline v5.16-rc1 -- include/linux/container_of.h e1edc277e6f6 linux/container_of.h: switch to static_assert d2a8ebbf8192 kernel.h: split out container_of() and typeof_member() macros > the patch is > Acked-by: Thomas Zimmermann Thanks! -- With Best Regards, Andy Shevchenko
Re: [PATCH] backlight: ili922x: fix kernel-doc warnings & notation
On Sun, Nov 14, 2021 at 07:39:25PM -0800, Randy Dunlap wrote:
> Convert function-like macro comments to kernel-doc notation and
> fix other kernel-doc warnings:
>
> drivers/video/backlight/ili922x.c:85: warning: This comment starts with
> '/**', but isn't a kernel-doc comment. Refer
> Documentation/doc-guide/kernel-doc.rst
> * START_BYTE(id, rs, rw)
> drivers/video/backlight/ili922x.c:118: warning: expecting prototype for
> CHECK_FREQ_REG(spi_device s, spi_transfer x)(). Prototype was for
> CHECK_FREQ_REG() instead
>
> ili922x.c:92: warning: contents before sections
> ili922x.c:150: warning: No description found for return value of
> 'ili922x_read_status'
> ili922x.c:193: warning: No description found for return value of
> 'ili922x_read'
> ili922x.c:247: warning: No description found for return value of
> 'ili922x_write'
> ili922x.c:353: warning: No description found for return value of
> 'ili922x_poweron'
> ili922x.c:382: warning: No description found for return value of
> 'ili922x_poweroff'
>
> Fixes: 4cfbfa971478 ("video: backlight: add ili922x lcd driver")
> Signed-off-by: Randy Dunlap
> Reported-by: kernel test robot
> Cc: Lee Jones
> Cc: Daniel Thompson
> Cc: Jingoo Han
> Cc: dri-devel@lists.freedesktop.org
> Cc: Stefano Babic
> Cc: Anatolij Gustschin
Thanks for the fixes. Just a could of quibbles about full
stops/periods.
> ---
> drivers/video/backlight/ili922x.c | 29 ++--
> 1 file changed, 19 insertions(+), 10 deletions(-)
>
> --- linux-next-20211102.orig/drivers/video/backlight/ili922x.c
> +++ linux-next-20211102/drivers/video/backlight/ili922x.c
> @@ -82,13 +82,7 @@
> #define START_RW_READ1
>
> /**
> - * START_BYTE(id, rs, rw)
> - *
> - * Set the start byte according to the required operation.
> - * The start byte is defined as:
> - * --
> - * | 0 | 1 | 1 | 1 | 0 | ID | RS | RW |
> - * --
> + * START_BYTE() - Set the start byte according to the required operation
Missing full stop/period.
> * @id: display's id as set by the manufacturer
> * @rs: operation type bit, one of:
> * - START_RS_INDEX set the index register
> @@ -96,14 +90,19 @@
> * @rw: read/write operation
> *- START_RW_WRITE write
> *- START_RW_READread
> + *
> + * The start byte is defined as:
> + * --
> + * | 0 | 1 | 1 | 1 | 0 | ID | RS | RW |
> + * --
> */
> #define START_BYTE(id, rs, rw) \
> (0x70 | (((id) & 0x01) << 2) | (((rs) & 0x01) << 1) | ((rw) & 0x01))
>
> /**
> - * CHECK_FREQ_REG(spi_device s, spi_transfer x) - Check the frequency
> - * for the SPI transfer. According to the datasheet, the controller
> - * accept higher frequency for the GRAM transfer, but it requires
> + * CHECK_FREQ_REG() - Check the frequency for the SPI transfer
> + * According to the datasheet, the controller
> + * accepts higher frequency for the GRAM transfer, but it requires
Also missing the full stop/period in the first sentence of the summary.
Note that here the missing full stop does not benefit from a new line to
conceal it and we will generate bad text as a result.
Check the frequency for the SPI transfer According to the data
sheet, the controller accepts...
Daniel.
Re: [PATCH] dma-buf: add DMA_BUF_IOCTL_SYNC_PARTIAL support
Am 13.11.21 um 07:22 schrieb Jianqun Xu:
Add DMA_BUF_IOCTL_SYNC_PARTIAL support for user to sync dma-buf with
offset and len.
You have not given an use case for this so it is a bit hard to review.
And from the existing use cases I don't see why this should be necessary.
Even worse from the existing backend implementation I don't even see how
drivers should be able to fulfill this semantics.
Please explain further,
Christian.
Change-Id: I03d2d2e10e48d32aa83c31abade57e0931e1be49
Signed-off-by: Jianqun Xu
---
drivers/dma-buf/dma-buf.c| 42
include/uapi/linux/dma-buf.h | 8 +++
2 files changed, 50 insertions(+)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index d9948d58b3f4..78f37f7c3462 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -392,6 +392,7 @@ static long dma_buf_ioctl(struct file *file,
{
struct dma_buf *dmabuf;
struct dma_buf_sync sync;
+ struct dma_buf_sync_partial sync_p;
enum dma_data_direction direction;
int ret;
@@ -430,6 +431,47 @@ static long dma_buf_ioctl(struct file *file,
case DMA_BUF_SET_NAME_B:
return dma_buf_set_name(dmabuf, (const char __user *)arg);
+ case DMA_BUF_IOCTL_SYNC_PARTIAL:
+ if (copy_from_user(&sync_p, (void __user *) arg,
sizeof(sync_p)))
+ return -EFAULT;
+
+ if (sync_p.len == 0)
+ return 0;
+
+ if ((sync_p.offset % cache_line_size()) || (sync_p.len %
cache_line_size()))
+ return -EINVAL;
+
+ if (sync_p.len > dmabuf->size || sync_p.offset > dmabuf->size -
sync_p.len)
+ return -EINVAL;
+
+ if (sync_p.flags & ~DMA_BUF_SYNC_VALID_FLAGS_MASK)
+ return -EINVAL;
+
+ switch (sync_p.flags & DMA_BUF_SYNC_RW) {
+ case DMA_BUF_SYNC_READ:
+ direction = DMA_FROM_DEVICE;
+ break;
+ case DMA_BUF_SYNC_WRITE:
+ direction = DMA_TO_DEVICE;
+ break;
+ case DMA_BUF_SYNC_RW:
+ direction = DMA_BIDIRECTIONAL;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (sync_p.flags & DMA_BUF_SYNC_END)
+ ret = dma_buf_end_cpu_access_partial(dmabuf, direction,
+sync_p.offset,
+sync_p.len);
+ else
+ ret = dma_buf_begin_cpu_access_partial(dmabuf,
direction,
+ sync_p.offset,
+ sync_p.len);
+
+ return ret;
+
default:
return -ENOTTY;
}
diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h
index 7f30393b92c3..6236c644624d 100644
--- a/include/uapi/linux/dma-buf.h
+++ b/include/uapi/linux/dma-buf.h
@@ -47,4 +47,12 @@ struct dma_buf_sync {
#define DMA_BUF_SET_NAME_A_IOW(DMA_BUF_BASE, 1, u32)
#define DMA_BUF_SET_NAME_B_IOW(DMA_BUF_BASE, 1, u64)
+struct dma_buf_sync_partial {
+ __u64 flags;
+ __u32 offset;
+ __u32 len;
+};
+
+#define DMA_BUF_IOCTL_SYNC_PARTIAL _IOW(DMA_BUF_BASE, 2, struct
dma_buf_sync_partial)
+
#endif
Re: [PATCH v1 1/3] string: Consolidate yesno() helpers under string.h hood
On Mon, 15 Nov 2021, Andy Shevchenko wrote: > On Tue, Oct 05, 2021 at 02:34:23PM -0700, Lucas De Marchi wrote: >> On Mon, Feb 15, 2021 at 04:21:35PM +0200, Andy Shevchenko wrote: >> > We have already few similar implementation and a lot of code that can >> > benefit >> > of the yesno() helper. Consolidate yesno() helpers under string.h hood. >> >> I was taking a look on i915_utils.h to reduce it and move some of it >> elsewhere to be shared with others. I was starting with these helpers >> and had [1] done, then Jani pointed me to this thread and also his >> previous tentative. I thought the natural place for this would be >> include/linux/string_helpers.h, but I will leave it up to you. > > Seems reasonable to use string_helpers (headers and/or C-file). > >> After reading the threads, I don't see real opposition to it. >> Is there a tree you plan to take this through? > > I rest my series in favour of Jani's approach, so I suppose there is no go > for _this_ series. If you want to make it happen, please pick it up and drive it. I'm thoroughly had enough of this. BR, Jani. > >> [1] >> https://lore.kernel.org/lkml/20211005212634.3223113-1-lucas.demar...@intel.com/T/#u -- Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH v2 1/1] drm/i915/rpm: Enable runtime pm autosuspend by default
On Mon, 15 Nov 2021, Tilak Tangudu wrote: The actual commit message with explanations why it will work now and didn't work before go here. Just the changelog will not be enough. BR, Jani. > v1: Enable runtime pm autosuspend by default for Gen12 > and later versions. > > v2: Enable runtime pm autosuspend by default for all > platforms(Syrjala Ville) > > Signed-off-by: Tilak Tangudu > --- > drivers/gpu/drm/i915/intel_runtime_pm.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c > b/drivers/gpu/drm/i915/intel_runtime_pm.c > index eaf7688f517d..f26ed1427fdc 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -600,6 +600,9 @@ void intel_runtime_pm_enable(struct intel_runtime_pm *rpm) > pm_runtime_use_autosuspend(kdev); > } > > + /* Enable by default */ > + pm_runtime_allow(kdev); > + > /* >* The core calls the driver load handler with an RPM reference held. >* We drop that here and will reacquire it during unloading in -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH] drm/amd/amdgpu: cleanup the code style a bit
Am 15.11.21 um 08:07 schrieb Bernard Zhao:
This change is to cleanup the code style a bit.
To be honest I think the old style looked better. It took me a moment to
validate this now.
What you could to instead is to have goto style error handling which
would make this a bit more cleaner I think.
Christian.
Signed-off-by: Bernard Zhao
---
drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index 04cf9b207e62..90070b41136a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -286,12 +286,14 @@ static int amdgpu_virt_init_ras_err_handler_data(struct
amdgpu_device *adev)
return -ENOMEM;
bps = kmalloc_array(align_space, sizeof((*data)->bps), GFP_KERNEL);
+ if (!bps) {
+ kfree(*data);
+ return -ENOMEM;
+ }
bps_bo = kmalloc_array(align_space, sizeof((*data)->bps_bo),
GFP_KERNEL);
-
- if (!bps || !bps_bo) {
- kfree(bps);
- kfree(bps_bo);
+ if (!bps_bo) {
kfree(*data);
+ kfree(bps);
return -ENOMEM;
}
Re: [PATCH 08/11] dmaengine: xilinx_dpdma: stop using slave_id field
Hi Arnd, On Mon, Nov 15, 2021 at 11:21:30AM +0100, Arnd Bergmann wrote: > On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart wrote: > > On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote: > > > @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan > > > *dchan, > > > spin_lock_irqsave(&chan->lock, flags); > > > > > > /* > > > - * Abuse the slave_id to indicate that the channel is part of a > > > video > > > - * group. > > > + * Abuse the peripheral_config to indicate that the channel is part > > > > Is it still an abuse, or is this now the right way to pass custom data > > to the DMA engine driver ? > > It doesn't make the driver any more portable, but it's now being > more explicit about it. As far as I can tell, this is the best way > to pass data that cannot be expressed through the regular interfaces > in DT and the dmaengine API. > > Ideally there would be a generic way to pass this flag, but I couldn't > figure out what this is actually doing, or whether there is a better > way. Maybe Vinod has an idea. I don't think we need a generic API in this case. The DMA engine is specific to the display device, I don't foresee a need to mix-n-match. > I'll change s/Abuse/Use/ for the moment until I get a definite answer. > > > > + * of a video group. > > >*/ > > > - if (chan->id <= ZYNQMP_DPDMA_VIDEO2) > > > - chan->video_group = config->slave_id != 0; > > > + pconfig = config->peripheral_config; > > > > This could be moved to the variable declaration above, up to you. > > I considered that but since it doesn't fit in a normal 80-column > line, it seemed best to do it here. > > > > + if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && > > > + config->peripheral_size == sizeof(*pconfig)) > > > > Silently ignoring a size mismatch isn't nice. Could we validate the size > > at the beginning of the function and return an error ? > > Yes, good idea. Since this would mean a bug in another driver, > I'll add a WARN_ON() as well to make it clear which driver caused it. > This is what I have now, let me know if you have any further suggestions: > > /* > * Use the peripheral_config to indicate that the channel is part > * of a video group. This requires matching use of the custom > * structure in each driver. > */ > pconfig = config->peripheral_config; > if (WARN_ON(config->peripheral_size != 0 && > config->peripheral_size != sizeof(*pconfig))) > return -EINVAL; How about if (WARN_ON(config->peripheral_config && config->peripheral_size != sizeof(*pconfig))) > > spin_lock_irqsave(&chan->lock, flags); > if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && > config->peripheral_size == sizeof(*pconfig)) And here you can test pconfig != NULL. > chan->video_group = pconfig->video_group; > spin_unlock_irqrestore(&chan->lock, flags); > > return 0; > > > With these issues addressed, > > > > Reviewed-by: Laurent Pinchart -- Regards, Laurent Pinchart
[PATCH 0/3] drm/i915, agp/intel-ggt: clean up includes
Took Andy's patch [1] and expanded on it a bit. BR, Jani. [1] https://patchwork.freedesktop.org/patch/msgid/2020102857.59604-1-andriy.shevche...@linux.intel.com Cc: Andy Shevchenko Andy Shevchenko (1): agp/intel-gtt: Replace kernel.h with the necessary inclusions Jani Nikula (2): drm/i915: include intel-gtt.h only where needed agp/intel-gtt: reduce intel-gtt dependencies more drivers/char/agp/intel-gtt.c | 1 + drivers/gpu/drm/i915/gt/intel_ggtt.c | 2 ++ drivers/gpu/drm/i915/gt/intel_gt.c | 2 ++ drivers/gpu/drm/i915/i915_drv.h | 1 - include/drm/intel-gtt.h | 8 +--- 5 files changed, 10 insertions(+), 4 deletions(-) -- 2.30.2
[PATCH 1/3] drm/i915: include intel-gtt.h only where needed
Only intel_gt.c and intel_ggtt.c need the interface. Cc: Andy Shevchenko Signed-off-by: Jani Nikula --- drivers/gpu/drm/i915/gt/intel_ggtt.c | 1 + drivers/gpu/drm/i915/gt/intel_gt.c | 2 ++ drivers/gpu/drm/i915/i915_drv.h | 1 - 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c index 1fb4a03d7ac3..0c956e5e7fc7 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c @@ -9,6 +9,7 @@ #include #include +#include #include "gem/i915_gem_lmem.h" diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index 1cb1948ac959..f2422d48be32 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -3,6 +3,8 @@ * Copyright © 2019 Intel Corporation */ +#include + #include "intel_gt_debugfs.h" #include "gem/i915_gem_lmem.h" diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 21ff781b8149..6f9f20a10c0c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -50,7 +50,6 @@ #include #include -#include #include #include #include -- 2.30.2
[PATCH 2/3] agp/intel-gtt: Replace kernel.h with the necessary inclusions
From: Andy Shevchenko When kernel.h is used in the headers it adds a lot into dependency hell, especially when there are circular dependencies are involved. Replace kernel.h inclusion with the list of what is really being used. Signed-off-by: Andy Shevchenko Signed-off-by: Jani Nikula --- include/drm/intel-gtt.h | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/include/drm/intel-gtt.h b/include/drm/intel-gtt.h index abfefaaf897a..4e5f8e7e25d0 100644 --- a/include/drm/intel-gtt.h +++ b/include/drm/intel-gtt.h @@ -6,7 +6,10 @@ #include #include -#include +#include + +struct pci_dev; +struct sg_table; void intel_gtt_get(u64 *gtt_total, phys_addr_t *mappable_base, -- 2.30.2
[PATCH 3/3] agp/intel-gtt: reduce intel-gtt dependencies more
Don't include stuff on behalf of users if they're not strictly necessary for the header. Cc: Andy Shevchenko Signed-off-by: Jani Nikula --- drivers/char/agp/intel-gtt.c | 1 + drivers/gpu/drm/i915/gt/intel_ggtt.c | 1 + include/drm/intel-gtt.h | 3 +-- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c index 5bfdf222d5f9..c53cc9868cd8 100644 --- a/drivers/char/agp/intel-gtt.c +++ b/drivers/char/agp/intel-gtt.c @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include "agp.h" diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c index 0c956e5e7fc7..555111c3bee5 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c @@ -3,6 +3,7 @@ * Copyright © 2020 Intel Corporation */ +#include #include #include diff --git a/include/drm/intel-gtt.h b/include/drm/intel-gtt.h index 4e5f8e7e25d0..67530bfef129 100644 --- a/include/drm/intel-gtt.h +++ b/include/drm/intel-gtt.h @@ -4,10 +4,9 @@ #ifndef _DRM_INTEL_GTT_H #define_DRM_INTEL_GTT_H -#include -#include #include +struct agp_bridge_data; struct pci_dev; struct sg_table; -- 2.30.2
Re: [PATCH 0/3] drm/i915, agp/intel-ggt: clean up includes
On Mon, 15 Nov 2021, Jani Nikula wrote: > Took Andy's patch [1] and expanded on it a bit. > > BR, > Jani. > > > [1] > https://patchwork.freedesktop.org/patch/msgid/2020102857.59604-1-andriy.shevche...@linux.intel.com > > Cc: Andy Shevchenko > > Andy Shevchenko (1): > agp/intel-gtt: Replace kernel.h with the necessary inclusions Forgot to mention, I replaced the drm/i915 prefix with agp/intel-gtt here, no other changes to Andy's patch. > > Jani Nikula (2): > drm/i915: include intel-gtt.h only where needed > agp/intel-gtt: reduce intel-gtt dependencies more > > drivers/char/agp/intel-gtt.c | 1 + > drivers/gpu/drm/i915/gt/intel_ggtt.c | 2 ++ > drivers/gpu/drm/i915/gt/intel_gt.c | 2 ++ > drivers/gpu/drm/i915/i915_drv.h | 1 - > include/drm/intel-gtt.h | 8 +--- > 5 files changed, 10 insertions(+), 4 deletions(-) -- Jani Nikula, Intel Open Source Graphics Center
[PATCH 1/3] drm/cma-helper: Move driver and file ops to the end of header
Restructure the header file for CMA helpers by moving declarations
for driver and file operations to the end of the file. No functional
changes.
Signed-off-by: Thomas Zimmermann
---
include/drm/drm_gem_cma_helper.h | 114 ---
1 file changed, 60 insertions(+), 54 deletions(-)
diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
index cd13508acbc1..e0fb7a0cf03f 100644
--- a/include/drm/drm_gem_cma_helper.h
+++ b/include/drm/drm_gem_cma_helper.h
@@ -32,77 +32,40 @@ struct drm_gem_cma_object {
#define to_drm_gem_cma_obj(gem_obj) \
container_of(gem_obj, struct drm_gem_cma_object, base)
-#ifndef CONFIG_MMU
-#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
- .get_unmapped_area = drm_gem_cma_get_unmapped_area,
-#else
-#define DRM_GEM_CMA_UNMAPPED_AREA_FOPS
-#endif
-
-/**
- * DEFINE_DRM_GEM_CMA_FOPS() - macro to generate file operations for CMA
drivers
- * @name: name for the generated structure
- *
- * This macro autogenerates a suitable &struct file_operations for CMA based
- * drivers, which can be assigned to &drm_driver.fops. Note that this structure
- * cannot be shared between drivers, because it contains a reference to the
- * current module using THIS_MODULE.
- *
- * Note that the declaration is already marked as static - if you need a
- * non-static version of this you're probably doing it wrong and will break the
- * THIS_MODULE reference by accident.
- */
-#define DEFINE_DRM_GEM_CMA_FOPS(name) \
- static const struct file_operations name = {\
- .owner = THIS_MODULE,\
- .open = drm_open,\
- .release= drm_release,\
- .unlocked_ioctl = drm_ioctl,\
- .compat_ioctl = drm_compat_ioctl,\
- .poll = drm_poll,\
- .read = drm_read,\
- .llseek = noop_llseek,\
- .mmap = drm_gem_mmap,\
- DRM_GEM_CMA_UNMAPPED_AREA_FOPS \
- }
-
/* free GEM object */
void drm_gem_cma_free_object(struct drm_gem_object *gem_obj);
-/* create memory region for DRM framebuffer */
-int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
-struct drm_device *drm,
-struct drm_mode_create_dumb *args);
-
-/* create memory region for DRM framebuffer */
-int drm_gem_cma_dumb_create(struct drm_file *file_priv,
- struct drm_device *drm,
- struct drm_mode_create_dumb *args);
-
/* allocate physical memory */
struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
size_t size);
extern const struct vm_operations_struct drm_gem_cma_vm_ops;
-#ifndef CONFIG_MMU
-unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
- unsigned long addr,
- unsigned long len,
- unsigned long pgoff,
- unsigned long flags);
-#endif
-
void drm_gem_cma_print_info(struct drm_printer *p, unsigned int indent,
const struct drm_gem_object *obj);
struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj);
+int drm_gem_cma_vmap(struct drm_gem_object *obj, struct dma_buf_map *map);
+int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
+
+/*
+ * Driver ops
+ */
+
+/* create memory region for DRM framebuffer */
+int drm_gem_cma_dumb_create_internal(struct drm_file *file_priv,
+struct drm_device *drm,
+struct drm_mode_create_dumb *args);
+
+/* create memory region for DRM framebuffer */
+int drm_gem_cma_dumb_create(struct drm_file *file_priv,
+ struct drm_device *drm,
+ struct drm_mode_create_dumb *args);
+
struct drm_gem_object *
drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
struct dma_buf_attachment *attach,
struct sg_table *sgt);
-int drm_gem_cma_vmap(struct drm_gem_object *obj, struct dma_buf_map *map);
-int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
/**
* DRM_GEM_CMA_DRIVER_OPS_WITH_DUMB_CREATE - CMA GEM driver operations
@@ -185,4 +148,47 @@ drm_gem_cma_prime_import_sg_table_vmap(struct drm_device
*drm,
struct dma_buf_attachment *attach,
struct sg_table *sgt);
+/*
+ * File ops
+ */
+
+#ifndef CONFIG_MMU
+unsigned long drm_gem_cma_get_unmapped_area(struct file *filp,
+ unsigned long addr,
+ unsigned long len,
+ unsign
[PATCH 0/3] drm/cma-helper: Clean up public interface
Convert GEM CMA functions to accept struct drm_gem_cma_object, provide small wrappers for GEM object callbacks and update all users. Brings up the interface to the pattern used in SHMEM and VRAM helpers. Converting all GEM object functions to use drm_gem_cma_object enables type checking by the C compiler. Previous callers could have passed any implementation of drm_gem_object to the GEM CMA helpers. It also removes upcasting in the GEM functions and simplifies the caller side. No functional changes. For GEM object callbacks, the CMA helper library now provides a number of small wrappers that do the necessary upcasting. Again no functional changes. Thomas Zimmermann (3): drm/cma-helper: Move driver and file ops to the end of header drm/cma-helper: Export dedicated wrappers for GEM object functions drm/cma-helper: Pass GEM CMA object in public interfaces drivers/gpu/drm/drm_gem_cma_helper.c | 73 +- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 10 +- drivers/gpu/drm/vc4/vc4_bo.c | 8 +- include/drm/drm_gem_cma_helper.h | 189 +++--- 4 files changed, 180 insertions(+), 100 deletions(-) base-commit: 9fccd12cfac1c863fa46d4d17c2d8ac25a44b190 -- 2.33.1
[PATCH 2/3] drm/cma-helper: Export dedicated wrappers for GEM object functions
Wrap GEM CMA functions for struct drm_gem_object_funcs and update
all callers. This will allow for an update of the public interfaces
of the GEM CMA helper library.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_gem_cma_helper.c | 23
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 10 ++--
drivers/gpu/drm/vc4/vc4_bo.c | 4 +-
include/drm/drm_gem_cma_helper.h | 78 +++
4 files changed, 94 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
b/drivers/gpu/drm/drm_gem_cma_helper.c
index 09e2cb80de08..27ccb71e3d66 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -35,11 +35,11 @@
*/
static const struct drm_gem_object_funcs drm_gem_cma_default_funcs = {
- .free = drm_gem_cma_free_object,
- .print_info = drm_gem_cma_print_info,
- .get_sg_table = drm_gem_cma_get_sg_table,
- .vmap = drm_gem_cma_vmap,
- .mmap = drm_gem_cma_mmap,
+ .free = drm_gem_cma_object_free,
+ .print_info = drm_gem_cma_object_print_info,
+ .get_sg_table = drm_gem_cma_object_get_sg_table,
+ .vmap = drm_gem_cma_object_vmap,
+ .mmap = drm_gem_cma_object_mmap,
.vm_ops = &drm_gem_cma_vm_ops,
};
@@ -198,8 +198,6 @@ drm_gem_cma_create_with_handle(struct drm_file *file_priv,
* This function frees the backing memory of the CMA GEM object, cleans up the
* GEM object state and frees the memory used to store the object itself.
* If the buffer is imported and the virtual address is set, it is released.
- * Drivers using the CMA helpers should set this as their
- * &drm_gem_object_funcs.free callback.
*/
void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
{
@@ -393,9 +391,8 @@ EXPORT_SYMBOL(drm_gem_cma_print_info);
* pages for a CMA GEM object
* @obj: GEM object
*
- * This function exports a scatter/gather table by
- * calling the standard DMA mapping API. Drivers using the CMA helpers should
- * set this as their &drm_gem_object_funcs.get_sg_table callback.
+ * This function exports a scatter/gather table by calling the standard
+ * DMA mapping API.
*
* Returns:
* A pointer to the scatter/gather table of pinned pages or NULL on failure.
@@ -475,8 +472,7 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_prime_import_sg_table);
* This function maps a buffer into the kernel's
* virtual address space. Since the CMA buffers are already mapped into the
* kernel virtual address space this simply returns the cached virtual
- * address. Drivers using the CMA helpers should set this as their DRM
- * driver's &drm_gem_object_funcs.vmap callback.
+ * address.
*
* Returns:
* 0 on success, or a negative error code otherwise.
@@ -498,8 +494,7 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_vmap);
*
* This function maps a buffer into a userspace process's address space.
* In addition to the usual GEM VMA setup it immediately faults in the entire
- * object instead of using on-demand faulting. Drivers that use the CMA
- * helpers should set this as their &drm_gem_object_funcs.mmap callback.
+ * object instead of using on-demand faulting.
*
* Returns:
* 0 on success or a negative error code on failure.
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index eacb1f17f747..190dbb7f15dd 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -327,11 +327,11 @@ const struct rcar_du_format_info *rcar_du_format_info(u32
fourcc)
*/
static const struct drm_gem_object_funcs rcar_du_gem_funcs = {
- .free = drm_gem_cma_free_object,
- .print_info = drm_gem_cma_print_info,
- .get_sg_table = drm_gem_cma_get_sg_table,
- .vmap = drm_gem_cma_vmap,
- .mmap = drm_gem_cma_mmap,
+ .free = drm_gem_cma_object_free,
+ .print_info = drm_gem_cma_object_print_info,
+ .get_sg_table = drm_gem_cma_object_get_sg_table,
+ .vmap = drm_gem_cma_object_vmap,
+ .mmap = drm_gem_cma_object_mmap,
.vm_ops = &drm_gem_cma_vm_ops,
};
diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
index fddaeb0b09c1..830756b3159e 100644
--- a/drivers/gpu/drm/vc4/vc4_bo.c
+++ b/drivers/gpu/drm/vc4/vc4_bo.c
@@ -732,8 +732,8 @@ static const struct vm_operations_struct vc4_vm_ops = {
static const struct drm_gem_object_funcs vc4_gem_object_funcs = {
.free = vc4_free_object,
.export = vc4_prime_export,
- .get_sg_table = drm_gem_cma_get_sg_table,
- .vmap = drm_gem_cma_vmap,
+ .get_sg_table = drm_gem_cma_object_get_sg_table,
+ .vmap = drm_gem_cma_object_vmap,
.mmap = vc4_gem_object_mmap,
.vm_ops = &vc4_vm_ops,
};
diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h
index e0fb7a0cf03f..56d2f9fdf9ac 100644
--- a/include/drm/drm_gem_cma_helper.h
+++ b/include/drm/drm_gem_cma_helper.h
@@ -48,6 +48,84 @@ struct sg_table *drm_gem_cma_get_sg_table(stru
[PATCH 3/3] drm/cma-helper: Pass GEM CMA object in public interfaces
Change all GEM CMA object functions that receive a GEM object
of type struct drm_gem_object to expect an object of type
struct drm_gem_cma_object instead.
This change reduces the number of upcasts from struct drm_gem_object
by moving them into callers. The C compiler can now verify that the
GEM CMA functions are called with the correct type.
For consistency, the patch also renames drm_gem_cma_free_object to
drm_gem_cma_free. It further updates documentation for a number of
functions.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_gem_cma_helper.c | 52 +---
drivers/gpu/drm/vc4/vc4_bo.c | 4 +--
include/drm/drm_gem_cma_helper.h | 39 -
3 files changed, 48 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
b/drivers/gpu/drm/drm_gem_cma_helper.c
index 27ccb71e3d66..7d4895de9e0d 100644
--- a/drivers/gpu/drm/drm_gem_cma_helper.c
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -32,6 +32,10 @@
* The DRM GEM/CMA helpers use this allocator as a means to provide buffer
* objects that are physically contiguous in memory. This is useful for
* display drivers that are unable to map scattered buffers via an IOMMU.
+ *
+ * For GEM callback helpers in struct &drm_gem_object functions, see likewise
+ * named functions with an _object_ infix (e.g., drm_gem_cma_object_vmap()
wraps
+ * drm_gem_cma_vmap()). These helpers perform the necessary type conversion.
*/
static const struct drm_gem_object_funcs drm_gem_cma_default_funcs = {
@@ -192,16 +196,16 @@ drm_gem_cma_create_with_handle(struct drm_file *file_priv,
}
/**
- * drm_gem_cma_free_object - free resources associated with a CMA GEM object
- * @gem_obj: GEM object to free
+ * drm_gem_cma_free - free resources associated with a CMA GEM object
+ * @cma_obj: CMA GEM object to free
*
* This function frees the backing memory of the CMA GEM object, cleans up the
* GEM object state and frees the memory used to store the object itself.
* If the buffer is imported and the virtual address is set, it is released.
*/
-void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
+void drm_gem_cma_free(struct drm_gem_cma_object *cma_obj)
{
- struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(gem_obj);
+ struct drm_gem_object *gem_obj = &cma_obj->base;
struct dma_buf_map map = DMA_BUF_MAP_INIT_VADDR(cma_obj->vaddr);
if (gem_obj->import_attach) {
@@ -222,7 +226,7 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
kfree(cma_obj);
}
-EXPORT_SYMBOL_GPL(drm_gem_cma_free_object);
+EXPORT_SYMBOL_GPL(drm_gem_cma_free);
/**
* drm_gem_cma_dumb_create_internal - create a dumb buffer object
@@ -369,18 +373,15 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_get_unmapped_area);
/**
* drm_gem_cma_print_info() - Print &drm_gem_cma_object info for debugfs
+ * @cma_obj: CMA GEM object
* @p: DRM printer
* @indent: Tab indentation level
- * @obj: GEM object
*
- * This function can be used as the &drm_driver->gem_print_info callback.
- * It prints paddr and vaddr for use in e.g. debugfs output.
+ * This function prints paddr and vaddr for use in e.g. debugfs output.
*/
-void drm_gem_cma_print_info(struct drm_printer *p, unsigned int indent,
- const struct drm_gem_object *obj)
+void drm_gem_cma_print_info(const struct drm_gem_cma_object *cma_obj,
+ struct drm_printer *p, unsigned int indent)
{
- const struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
-
drm_printf_indent(p, indent, "paddr=%pad\n", &cma_obj->paddr);
drm_printf_indent(p, indent, "vaddr=%p\n", cma_obj->vaddr);
}
@@ -389,7 +390,7 @@ EXPORT_SYMBOL(drm_gem_cma_print_info);
/**
* drm_gem_cma_get_sg_table - provide a scatter/gather table of pinned
* pages for a CMA GEM object
- * @obj: GEM object
+ * @cma_obj: CMA GEM object
*
* This function exports a scatter/gather table by calling the standard
* DMA mapping API.
@@ -397,9 +398,9 @@ EXPORT_SYMBOL(drm_gem_cma_print_info);
* Returns:
* A pointer to the scatter/gather table of pinned pages or NULL on failure.
*/
-struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj)
+struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_cma_object *cma_obj)
{
- struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);
+ struct drm_gem_object *obj = &cma_obj->base;
struct sg_table *sgt;
int ret;
@@ -465,22 +466,19 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_prime_import_sg_table);
/**
* drm_gem_cma_vmap - map a CMA GEM object into the kernel's virtual
* address space
- * @obj: GEM object
+ * @cma_obj: CMA GEM object
* @map: Returns the kernel virtual address of the CMA GEM object's backing
* store.
*
- * This function maps a buffer into the kernel's
- * virtual address space. Since the CMA buffers are already mapped into the
- * kernel virtual address space this
答复: [PATCH] drm/amd/amdgpu: cleanup the code style a bit
-邮件原件-
发件人: bern...@vivo.com 代表 Christian K?nig
发送时间: 2021年11月15日 19:49
收件人: 赵军奎 ; Alex Deucher ;
Christian König ; Pan, Xinhui ;
David Airlie ; Daniel Vetter ; Jingwen Chen
; Candice Li ; John Clements
; Monk liu ; Peng Ju Zhou
; Jiawei Gu ; Bokun Zhang
; Zhigang Luo ; Lee Jones
; amd-...@lists.freedesktop.org;
dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org
主题: Re: [PATCH] drm/amd/amdgpu: cleanup the code style a bit
Am 15.11.21 um 08:07 schrieb Bernard Zhao:
> This change is to cleanup the code style a bit.
>To be honest I think the old style looked better. It took me a moment to
>validate this now.
>What you could to instead is to have goto style error handling which would
>make this a bit more cleaner I think.
Hi
Looks like a good idea, thank you for your comments!
I will resubmit a version!
BR//Bernard
>Christian.
>
> Signed-off-by: Bernard Zhao
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index 04cf9b207e62..90070b41136a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -286,12 +286,14 @@ static int amdgpu_virt_init_ras_err_handler_data(struct
> amdgpu_device *adev)
> return -ENOMEM;
>
> bps = kmalloc_array(align_space, sizeof((*data)->bps), GFP_KERNEL);
> + if (!bps) {
> + kfree(*data);
> + return -ENOMEM;
> + }
> bps_bo = kmalloc_array(align_space, sizeof((*data)->bps_bo),
> GFP_KERNEL);
> -
> - if (!bps || !bps_bo) {
> - kfree(bps);
> - kfree(bps_bo);
> + if (!bps_bo) {
> kfree(*data);
> + kfree(bps);
> return -ENOMEM;
> }
>
Re: Questions about KMS flip
On 2021-11-15 12:31, Lang Yu wrote: > On Mon, Nov 15, 2021 at 10:49:39AM +0100, Michel DDDnzer wrote: >> On 2021-11-15 10:04, Lang Yu wrote: >>> On Mon, Nov 15, 2021 at 09:38:47AM +0100, Michel DDDnzer wrote: On 2021-11-15 07:41, Lang Yu wrote: > On Fri, Nov 12, 2021 at 05:10:27PM +0100, Michel DDDnzer wrote: >> On 2021-11-12 16:03, Christian König wrote: >>> Am 12.11.21 um 15:30 schrieb Michel Dänzer: On 2021-11-12 15:29, Michel Dänzer wrote: > On 2021-11-12 13:47, Christian König wrote: >> Anyway this unfortunately turned out to be work for Harray and >> Nicholas. In detail it's about this bug report here: >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.kernel.org%2Fshow_bug.cgi%3Fid%3D214621&data=04%7C01%7CLang.Yu%40amd.com%7Cee54c4d055d040ef9f8b08d9a81d3eb9%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637725665833112900%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=7nwIYd1um420XHVpOzeIvz37%2FLQqHF%2F6aRKfzgxUTnM%3D&reserved=0 >> >> Lang was able to reproduce the issue and narrow it down to the pin >> in amdgpu_display_crtc_page_flip_target(). >> >> In other words we somehow have an unbalanced pinning of the scanout >> buffer in DC. > DC doesn't use amdgpu_display_crtc_page_flip_target AFAICT. The > corresponding pin with DC would be in dm_plane_helper_prepare_fb, > paired with the unpin in > dm_plane_helper_cleanup_fb. > > > With non-DC, the pin in amdgpu_display_crtc_page_flip_target is > paired with the unpin in dm_plane_helper_cleanup_fb This should say amdgpu_display_unpin_work_func. >>> >>> Ah! So that is the classic (e.g. non atomic) path? >> >> Presumably. >> >> > & dce_v*_crtc_disable. One thing I notice is that the pin is guarded > by if (!adev->enable_virtual_display), but the unpins seem > unconditional. So could this be about virtual display, and the > problem is actually trying to unpin a BO that was never pinned? >>> >>> Nope, my educated guess is rather that we free up the BO before >>> amdgpu_display_unpin_work_func is called. >>> >>> E.g. not pin unbalance, but rather use after free. >> >> amdgpu_display_crtc_page_flip_target calls amdgpu_bo_ref(work->old_abo), >> and amdgpu_display_unpin_work_func calls amdgpu_bo_unref(&work->old_abo) >> only after amdgpu_bo_unpin. So what you describe could only happen if >> there's an imbalance elsewhere such that amdgpu_bo_unref is called more >> often than amdgpu_bo_ref, or maybe if amdgpu_bo_reserve fails in >> amdgpu_display_unpin_work_func (in which case the "failed to reserve >> buffer after flip" error message should appear in dmesg). > > > Actually, each call to amdgpu_display_crtc_page_flip_target() will > > 1, init a work(amdgpu_display_unpin_work_func) to unpin an old buffer >(crtc->primary->fb), the work will be queued in > dce_vX_0_pageflip_irq(). > > 2, pin a new buffer, assign it to crtc->primary->fb. But how to unpin it? >Next call. > > The problem is the pinned buffer of last call to > amdgpu_display_crtc_page_flip_target() is not unpinned. It's unpinned in dce_v*_0_crtc_disable. >>> >>> I just found crtc->primary->fb is NULL when came in dce_v*_0_crtc_disable(). >>> So it's not unpinned... >> >> __drm_helper_disable_unused_functions sets crtc->primary->fb = NULL only >> after calling crtc_funcs->disable. Maybe this path can get hit for a CRTC >> which was already disabled, in which case crtc->primary->fb == NULL in >> dce_v*_0_crtc_disable is harmless. >> >> Have you checked for the issue I described below? Should be pretty easy to >> catch. >> >> I think I've found the problem though: dce_v*_0_crtc_do_set_base pin the BO from target_fb unconditionally, but unpin the BO from the fb parameter only if it's different from the former. So if they're the same, the BO's pin count is incremented by 1. > > Form my observations, amdgpu_bo_unpin() in dce_v*_0_crtc_disable() is > never called. It would be expected to happen when the screen turns off, e.g. due to DPMS. > Though a single call to dce_v*_0_crtc_do_set_base() will > only pin the BO, I found it will be unpinned in next call to > dce_v*_0_crtc_do_set_base(). Yeah, that's the normal case when the new BO is different from the old one. To catch the case I described, try something like diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c index 18a7b3bd633b..5726bd87a355 100644 --- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c +++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c @@ -1926,6 +1926,7 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
[PATCH v2] drm/amd/amdgpu: cleanup the code style a bit
This change is to cleanup the code style a bit.
Signed-off-by: Bernard Zhao
---
drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 21 +
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index 04cf9b207e62..3fc49823f527 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -283,17 +283,15 @@ static int amdgpu_virt_init_ras_err_handler_data(struct
amdgpu_device *adev)
*data = kmalloc(sizeof(struct amdgpu_virt_ras_err_handler_data),
GFP_KERNEL);
if (!*data)
- return -ENOMEM;
+ goto data_failure;
bps = kmalloc_array(align_space, sizeof((*data)->bps), GFP_KERNEL);
- bps_bo = kmalloc_array(align_space, sizeof((*data)->bps_bo),
GFP_KERNEL);
+ if (!bps)
+ goto bps_failure;
- if (!bps || !bps_bo) {
- kfree(bps);
- kfree(bps_bo);
- kfree(*data);
- return -ENOMEM;
- }
+ bps_bo = kmalloc_array(align_space, sizeof((*data)->bps_bo),
GFP_KERNEL);
+ if (!bps_bo)
+ goto bps_bo_failure;
(*data)->bps = bps;
(*data)->bps_bo = bps_bo;
@@ -303,6 +301,13 @@ static int amdgpu_virt_init_ras_err_handler_data(struct
amdgpu_device *adev)
virt->ras_init_done = true;
return 0;
+
+bps_bo_failure:
+ kfree(bps);
+bps_failure:
+ kfree(*data);
+data_failure:
+ return -ENOMEM;
}
static void amdgpu_virt_ras_release_bp(struct amdgpu_device *adev)
--
2.33.1
Re: [PATCH] drm: document DRM_IOCTL_MODE_GETFB2
On Thursday, November 11th, 2021 at 12:50, Daniel Stone wrote: > In fairness it is perfectly clear, it's just that I never considered > calling it without master/admin because why would you ever do that? FWIW, drm_info does it to display the current buffers metadata. Pretty useful when debugging.
Re: [PATCH 01/11] ASoC: dai_dma: remove slave_id field
On 11/15/21 11:42 AM, Arnd Bergmann wrote: On Mon, Nov 15, 2021 at 11:14 AM Lars-Peter Clausen wrote: On 11/15/21 9:53 AM, Arnd Bergmann wrote: From: Arnd Bergmann This field is never set, and serves no purpose, so remove it. I agree that we should remove it. Its been legacy support code for a while, but the description that there is no user is not right. The tegra20_spdif driver obviously uses it and that user is removed in this patch. I think it makes sense to split that out into a separate patch with a description why the driver will still work even with slave_id removed. Maybe the best is to remove the whole tegra20_spdif driver. Ok, I'll split out the tegra patch and try to come up with a better description for it. What I saw in that driver is it just passes down the slave_id number from a 'struct resource', but there is nothing in the kernel that sets up this resource. Do you or someone else have more information on the state of this driver? I can see that it does not contain any of_device_id based probing, so it seems that this is either dead code, the platform_device gets created by some other code that is no longer compatible with this driver. I've looked into this a while back, when I tried to remove slave_id. And as far as I can tell there were never any in-tree users of this driver, even back when we used platform board files. Maybe somebody from Nvidia knows if there are out-of-tree users. - Lars
Re: [PATCH 02/13] drm/connector: Add helper to check if a mode requires scrambling
On Mon, Nov 08, 2021 at 07:55:00PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 08, 2021 at 04:58:34PM +0100, Maxime Ripard wrote:
> > On Fri, Nov 05, 2021 at 08:14:04PM +0200, Ville Syrjälä wrote:
> > > On Fri, Nov 05, 2021 at 07:02:30PM +0100, Daniel Vetter wrote:
> > > > On Thu, Nov 04, 2021 at 05:37:21PM +0200, Ville Syrjälä wrote:
> > > > > On Tue, Nov 02, 2021 at 03:59:33PM +0100, Maxime Ripard wrote:
> > > > > > --- a/include/drm/drm_modes.h
> > > > > > +++ b/include/drm/drm_modes.h
> > > > > > @@ -424,6 +424,21 @@ static inline bool drm_mode_is_stereo(const
> > > > > > struct drm_display_mode *mode)
> > > > > > return mode->flags & DRM_MODE_FLAG_3D_MASK;
> > > > > > }
> > > > > >
> > > > > > +/**
> > > > > > + * drm_mode_hdmi_requires_scrambling - Checks if a mode requires
> > > > > > HDMI Scrambling
> > > > > > + * @mode: DRM display mode
> > > > > > + *
> > > > > > + * Checks if a given display mode requires the scrambling to be
> > > > > > enabled.
> > > > > > + *
> > > > > > + * Returns:
> > > > > > + * A boolean stating whether it's required or not.
> > > > > > + */
> > > > > > +static inline bool
> > > > > > +drm_mode_hdmi_requires_scrambling(const struct drm_display_mode
> > > > > > *mode)
> > > > > > +{
> > > > > > + return mode->clock > DRM_HDMI_14_MAX_TMDS_CLK_KHZ;
> > > > > > +}
> > > > >
> > > > > That's only correct for 8bpc. The actual limit is on the TMDS clock
> > > > > (or
> > > > > rather TMDS character rate as HDMI 2.0 calls it due to the 1/1 vs. 1/4
> > > > > magic for the actually transmitted TMDS clock).
> > > >
> > > > Yeah we might need to add the bus format for the cable here too, to make
> > > > this complete.
> > >
> > > I don't think we have a usable thing for that on the drm level, so
> > > would need to invent something. Oh, and the mode->clock vs.
> > > mode->crtc_clock funny business also limits the usability of this
> > > approach. So probably just easiest to pass in the the driver calculated
> > > TMDS clock instead.
> >
> > If we look at all (I think?) the existing users of scrambling in KMS as
> > of 5.15, only i915 seems to use crtc_clock (which, in retrospect, seems
> > to be the right thing to do), and only i915 and dw-hdmi use an output
> > format, i915 rolling its own, and dw-hdmi using the mbus formats.
> >
> > I think using the mbus format here makes the most sense: i915 already is
> > rolling a whole bunch of other code, and we use the mbus defines for the
> > bridge format enumeration as well which is probably going to have some
> > interaction.
> >
> > I'm not quite sure what to do next though. The whole point of that
> > series is to streamline as much as possible the scrambling and TMDS
> > ratio setup to avoid the duplication we already have in the drivers that
> > support it, every one using the mostly-the-same-but-slightly-different
> > logic to configure it.
> >
> > The mode is fortunately stored in generic structures so it's easy to
> > make that decision. Having to take into account the output format
> > however makes it mandatory to move the bus format in the
> > drm_connector_state(?) structure too.
>
> I think involving state objects and the like is just going to make
> it harder to share code between all drivers, if that is the goal.
> Just a few tiny helpers I think is what would allow the broadest
> reuse. I guess you could build additional midlayer on top of those
> for some drivers if you wish.
>
> As for the bus_format stuff, that looks at the same time overkill,
> and insufficiently documented. I guess its main purpose is to exactly
> defines how some digtal bus works, which makes sense when you're
> chaining a bunch of random chips together. But looks overly complicated
> to me for defining what to output from a HDMI encoder. Looking at the
> defines I wouldn't even know what to use for HDMI actually. All we
> really want is RGB 4:4:4 vs. YCbCr 4:4:4 vs. YCbCr 4:2:2 vs. YCbCr 4:2:0.
> Beyond that level of detail we don't care how each bit gets transferred
> etc. Hence enum intel_output_format in i915.
I have the same feeling about the mbus formats.
I tried to start a discussion about this some time back, without much
success:
https://lore.kernel.org/all/20210317154352.732095-1-max...@cerno.tech/
The main issue for our current series is that it's tied to the bridges,
while it should work for any HDMI connector, backed by a bridge or not.
However, the main point of this series is to streamline as much as
possible the scrambling setup, including dealing with hotplug properly
just like i915 is doing.
A flag in the connector state to enable the scrambling and high tmds
ratio allows for the helper to perform the disable/enable cycle when we
detected the hotplug. If we wouldn't have it, it wouldn't know what to
do in the first place, and we would end up disabling/enabling the
display every time (which isn't great).
This also allows for less duplication since everyone is using a variant
of the same algorithm to do
Re: [PATCH v3 2/6] drm/i915: Add support for asynchronous moving fence waiting
On 14/11/2021 11:12, Thomas Hellström wrote:
From: Maarten Lankhorst
For now, we will only allow async migration when TTM is used,
so the paths we care about are related to TTM.
The mmap path is handled by having the fence in ttm_bo->moving,
when pinning, the binding only becomes available after the moving
fence is signaled, and pinning a cpu map will only work after
the moving fence signals.
This should close all holes where userspace can read a buffer
before it's fully migrated.
v2:
- Fix a couple of SPARSE warnings
v3:
- Fix a NULL pointer dereference
Co-developed-by: Thomas Hellström
Signed-off-by: Thomas Hellström
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/i915/display/intel_fbdev.c| 7 ++--
drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_pages.c | 6 +++
.../i915/gem/selftests/i915_gem_coherency.c | 4 +-
.../drm/i915/gem/selftests/i915_gem_mman.c| 22 ++-
drivers/gpu/drm/i915/i915_vma.c | 39 ++-
drivers/gpu/drm/i915/i915_vma.h | 3 ++
drivers/gpu/drm/i915/selftests/i915_vma.c | 4 +-
8 files changed, 69 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
b/drivers/gpu/drm/i915/display/intel_fbdev.c
index adc3a81be9f7..5902ad0c2bd8 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -265,11 +265,12 @@ static int intelfb_create(struct drm_fb_helper *helper,
info->fix.smem_len = vma->node.size;
}
- vaddr = i915_vma_pin_iomap(vma);
+ vaddr = i915_vma_pin_iomap_unlocked(vma);
if (IS_ERR(vaddr)) {
- drm_err(&dev_priv->drm,
- "Failed to remap framebuffer into virtual memory\n");
ret = PTR_ERR(vaddr);
+ if (ret != -EINTR && ret != -ERESTARTSYS)
+ drm_err(&dev_priv->drm,
+ "Failed to remap framebuffer into virtual
memory\n");
goto out_unpin;
}
info->screen_base = vaddr;
diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c
b/drivers/gpu/drm/i915/display/intel_overlay.c
index 7e3f5c6ca484..21593f3f2664 100644
--- a/drivers/gpu/drm/i915/display/intel_overlay.c
+++ b/drivers/gpu/drm/i915/display/intel_overlay.c
@@ -1357,7 +1357,7 @@ static int get_registers(struct intel_overlay *overlay,
bool use_phys)
overlay->flip_addr = sg_dma_address(obj->mm.pages->sgl);
else
overlay->flip_addr = i915_ggtt_offset(vma);
- overlay->regs = i915_vma_pin_iomap(vma);
+ overlay->regs = i915_vma_pin_iomap_unlocked(vma);
i915_vma_unpin(vma);
if (IS_ERR(overlay->regs)) {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index c4f684b7cc51..49c6e55c68ce 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -418,6 +418,12 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object
*obj,
}
if (!ptr) {
+ err = i915_gem_object_wait_moving_fence(obj, true);
+ if (err) {
+ ptr = ERR_PTR(err);
+ goto err_unpin;
+ }
+
if (GEM_WARN_ON(type == I915_MAP_WC &&
!static_cpu_has(X86_FEATURE_PAT)))
ptr = ERR_PTR(-ENODEV);
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
index 13b088cc787e..067c512961ba 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
@@ -101,7 +101,7 @@ static int gtt_set(struct context *ctx, unsigned long
offset, u32 v)
intel_gt_pm_get(vma->vm->gt);
- map = i915_vma_pin_iomap(vma);
+ map = i915_vma_pin_iomap_unlocked(vma);
i915_vma_unpin(vma);
if (IS_ERR(map)) {
err = PTR_ERR(map);
@@ -134,7 +134,7 @@ static int gtt_get(struct context *ctx, unsigned long
offset, u32 *v)
intel_gt_pm_get(vma->vm->gt);
- map = i915_vma_pin_iomap(vma);
+ map = i915_vma_pin_iomap_unlocked(vma);
i915_vma_unpin(vma);
if (IS_ERR(map)) {
err = PTR_ERR(map);
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
index 6d30cdfa80f3..5d54181c2145 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
@@ -125,12 +125,13 @@ static int check_partial_mapping(struct
drm_i915_gem_object *obj,
n = page - view.partial.offset;
GEM_BUG_ON(n >= view.partial.size);
- io = i915_vma_pin_iomap(vma);
+ io = i915_vma_pin_iomap_unlocked(vma);
i915_vma_unpin(vma);
if (IS_ERR(io)) {
-
Re: [PATCH 08/11] dmaengine: xilinx_dpdma: stop using slave_id field
On Mon, Nov 15, 2021 at 12:49 PM Laurent Pinchart wrote: > On Mon, Nov 15, 2021 at 11:21:30AM +0100, Arnd Bergmann wrote: > > On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart wrote: > > > On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote: > > > > @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan > > > > *dchan, > > > > spin_lock_irqsave(&chan->lock, flags); > > > > > > > > /* > > > > - * Abuse the slave_id to indicate that the channel is part of a > > > > video > > > > - * group. > > > > + * Abuse the peripheral_config to indicate that the channel is > > > > part > > > > > > Is it still an abuse, or is this now the right way to pass custom data > > > to the DMA engine driver ? > > > > It doesn't make the driver any more portable, but it's now being > > more explicit about it. As far as I can tell, this is the best way > > to pass data that cannot be expressed through the regular interfaces > > in DT and the dmaengine API. > > > > Ideally there would be a generic way to pass this flag, but I couldn't > > figure out what this is actually doing, or whether there is a better > > way. Maybe Vinod has an idea. > > I don't think we need a generic API in this case. The DMA engine is > specific to the display device, I don't foresee a need to mix-n-match. Right. I wonder if there is even a point in using the dmaengine API in that case, I think for other single-purpose drivers we tend to just integrate the functionality in the client driver. No point changing this now of course, but it does feel odd. >From my earlier reading of the driver, my impression was that this is just a memory-to-memory device, so it could be used that way as well, but does need a flag when working on the video memory. I couldn't quite make sense of that though. > > /* > > * Use the peripheral_config to indicate that the channel is part > > * of a video group. This requires matching use of the custom > > * structure in each driver. > > */ > > pconfig = config->peripheral_config; > > if (WARN_ON(config->peripheral_size != 0 && > > config->peripheral_size != sizeof(*pconfig))) > > return -EINVAL; > > How about > > if (WARN_ON(config->peripheral_config && > config->peripheral_size != sizeof(*pconfig))) > > > > > spin_lock_irqsave(&chan->lock, flags); > > if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && > > config->peripheral_size == sizeof(*pconfig)) > > And here you can test pconfig != NULL. Good idea. Changed now, using 'if (pconfig)' without the '!= NULL' in both expressions. Arnd
Re: [PATCH v3 1/6] drm/i915: Add functions to set/get moving fence
On 14/11/2021 11:12, Thomas Hellström wrote:
From: Maarten Lankhorst
We want to get rid of i915_vma tracking to simplify the code and
lifetimes. Add a way to set/put the moving fence, in preparation for
removing the tracking.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 37 ++
drivers/gpu/drm/i915/gem/i915_gem_object.h | 9 ++
2 files changed, 46 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 591ee3cb7275..ec4313836597 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -33,6 +33,7 @@
#include "i915_gem_object.h"
#include "i915_memcpy.h"
#include "i915_trace.h"
+#include "i915_gem_ttm.h"
static struct kmem_cache *slab_objects;
@@ -726,6 +727,42 @@ static const struct drm_gem_object_funcs i915_gem_object_funcs = {
.export = i915_gem_prime_export,
};
+struct dma_fence *
+i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj)
+{
+ return dma_fence_get(i915_gem_to_ttm(obj)->moving);
+}
+
+void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj,
+ struct dma_fence *fence)
+{
+ dma_fence_put(i915_gem_to_ttm(obj)->moving);
+
+ i915_gem_to_ttm(obj)->moving = dma_fence_get(fence);
+}
Are these also assert_object_held()? Should we maybe squash this patch
with the first user?
+
+int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj,
+ bool intr)
+{
+ struct dma_fence *fence = i915_gem_to_ttm(obj)->moving;
+ int ret;
+
+ assert_object_held(obj);
+ if (!fence)
+ return 0;
+
+ ret = dma_fence_wait(fence, intr);
+ if (ret)
+ return ret;
+
+ if (fence->error)
+ return fence->error;
+
+ i915_gem_to_ttm(obj)->moving = NULL;
+ dma_fence_put(fence);
+ return 0;
+}
+
#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
#include "selftests/huge_gem_object.c"
#include "selftests/huge_pages.c"
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index 133963b46135..36bf3e2e602f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -517,6 +517,15 @@ i915_gem_object_finish_access(struct drm_i915_gem_object
*obj)
i915_gem_object_unpin_pages(obj);
}
+struct dma_fence *
+i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj);
+
+void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj,
+ struct dma_fence *fence);
+
+int i915_gem_object_wait_moving_fence(struct drm_i915_gem_object *obj,
+ bool intr);
+
void i915_gem_object_set_cache_coherency(struct drm_i915_gem_object *obj,
unsigned int cache_level);
bool i915_gem_object_can_bypass_llc(struct drm_i915_gem_object *obj);
Re: [PATCH v3 2/6] drm/i915: Add support for asynchronous moving fence waiting
On 11/15/21 13:36, Matthew Auld wrote:
On 14/11/2021 11:12, Thomas Hellström wrote:
From: Maarten Lankhorst
For now, we will only allow async migration when TTM is used,
so the paths we care about are related to TTM.
The mmap path is handled by having the fence in ttm_bo->moving,
when pinning, the binding only becomes available after the moving
fence is signaled, and pinning a cpu map will only work after
the moving fence signals.
This should close all holes where userspace can read a buffer
before it's fully migrated.
v2:
- Fix a couple of SPARSE warnings
v3:
- Fix a NULL pointer dereference
Co-developed-by: Thomas Hellström
Signed-off-by: Thomas Hellström
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/i915/display/intel_fbdev.c | 7 ++--
drivers/gpu/drm/i915/display/intel_overlay.c | 2 +-
drivers/gpu/drm/i915/gem/i915_gem_pages.c | 6 +++
.../i915/gem/selftests/i915_gem_coherency.c | 4 +-
.../drm/i915/gem/selftests/i915_gem_mman.c | 22 ++-
drivers/gpu/drm/i915/i915_vma.c | 39 ++-
drivers/gpu/drm/i915/i915_vma.h | 3 ++
drivers/gpu/drm/i915/selftests/i915_vma.c | 4 +-
8 files changed, 69 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
b/drivers/gpu/drm/i915/display/intel_fbdev.c
index adc3a81be9f7..5902ad0c2bd8 100644
--- a/drivers/gpu/drm/i915/display/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
@@ -265,11 +265,12 @@ static int intelfb_create(struct drm_fb_helper
*helper,
info->fix.smem_len = vma->node.size;
}
- vaddr = i915_vma_pin_iomap(vma);
+ vaddr = i915_vma_pin_iomap_unlocked(vma);
if (IS_ERR(vaddr)) {
- drm_err(&dev_priv->drm,
- "Failed to remap framebuffer into virtual memory\n");
ret = PTR_ERR(vaddr);
+ if (ret != -EINTR && ret != -ERESTARTSYS)
+ drm_err(&dev_priv->drm,
+ "Failed to remap framebuffer into virtual memory\n");
goto out_unpin;
}
info->screen_base = vaddr;
diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c
b/drivers/gpu/drm/i915/display/intel_overlay.c
index 7e3f5c6ca484..21593f3f2664 100644
--- a/drivers/gpu/drm/i915/display/intel_overlay.c
+++ b/drivers/gpu/drm/i915/display/intel_overlay.c
@@ -1357,7 +1357,7 @@ static int get_registers(struct intel_overlay
*overlay, bool use_phys)
overlay->flip_addr = sg_dma_address(obj->mm.pages->sgl);
else
overlay->flip_addr = i915_ggtt_offset(vma);
- overlay->regs = i915_vma_pin_iomap(vma);
+ overlay->regs = i915_vma_pin_iomap_unlocked(vma);
i915_vma_unpin(vma);
if (IS_ERR(overlay->regs)) {
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
index c4f684b7cc51..49c6e55c68ce 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
@@ -418,6 +418,12 @@ void *i915_gem_object_pin_map(struct
drm_i915_gem_object *obj,
}
if (!ptr) {
+ err = i915_gem_object_wait_moving_fence(obj, true);
+ if (err) {
+ ptr = ERR_PTR(err);
+ goto err_unpin;
+ }
+
if (GEM_WARN_ON(type == I915_MAP_WC &&
!static_cpu_has(X86_FEATURE_PAT)))
ptr = ERR_PTR(-ENODEV);
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
index 13b088cc787e..067c512961ba 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_coherency.c
@@ -101,7 +101,7 @@ static int gtt_set(struct context *ctx, unsigned
long offset, u32 v)
intel_gt_pm_get(vma->vm->gt);
- map = i915_vma_pin_iomap(vma);
+ map = i915_vma_pin_iomap_unlocked(vma);
i915_vma_unpin(vma);
if (IS_ERR(map)) {
err = PTR_ERR(map);
@@ -134,7 +134,7 @@ static int gtt_get(struct context *ctx, unsigned
long offset, u32 *v)
intel_gt_pm_get(vma->vm->gt);
- map = i915_vma_pin_iomap(vma);
+ map = i915_vma_pin_iomap_unlocked(vma);
i915_vma_unpin(vma);
if (IS_ERR(map)) {
err = PTR_ERR(map);
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
index 6d30cdfa80f3..5d54181c2145 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c
@@ -125,12 +125,13 @@ static int check_partial_mapping(struct
drm_i915_gem_object *obj,
n = page - view.partial.offset;
GEM_BUG_ON(n >= view.partial.size);
- io = i915_vma_pin_iomap(vma);
+ io = i915_vma_pin_iomap_unlocked(vma);
i915_vma_unpin(vma);
if (IS_ERR(io)) {
- pr_err("Failed to iomap partial view: offset=%lu; err=%d\n",
- page, (int)PTR_ERR(io));
err = PTR_ERR(io);
+ if (err !
Re: [PATCH v3 1/6] drm/i915: Add functions to set/get moving fence
On 11/15/21 13:39, Matthew Auld wrote:
On 14/11/2021 11:12, Thomas Hellström wrote:
From: Maarten Lankhorst
We want to get rid of i915_vma tracking to simplify the code and
lifetimes. Add a way to set/put the moving fence, in preparation for
removing the tracking.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/i915/gem/i915_gem_object.c | 37 ++
drivers/gpu/drm/i915/gem/i915_gem_object.h | 9 ++
2 files changed, 46 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index 591ee3cb7275..ec4313836597 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -33,6 +33,7 @@
#include "i915_gem_object.h"
#include "i915_memcpy.h"
#include "i915_trace.h"
+#include "i915_gem_ttm.h"
static struct kmem_cache *slab_objects;
@@ -726,6 +727,42 @@ static const struct drm_gem_object_funcs
i915_gem_object_funcs = {
.export = i915_gem_prime_export,
};
+struct dma_fence *
+i915_gem_object_get_moving_fence(struct drm_i915_gem_object *obj)
+{
+ return dma_fence_get(i915_gem_to_ttm(obj)->moving);
+}
+
+void i915_gem_object_set_moving_fence(struct drm_i915_gem_object *obj,
+ struct dma_fence *fence)
+{
+ dma_fence_put(i915_gem_to_ttm(obj)->moving);
+
+ i915_gem_to_ttm(obj)->moving = dma_fence_get(fence);
+}
Are these also assert_object_held()? Should we maybe squash this patch
with the first user?
Yes these are also assert_object_held(). We could probably squash these,
yes.
[PATCH 0/3] drm: Make DRM hashtable legacy
Clean up the last non-legacy users of DRM's hashtable code and put the code behind CONFIG_DRM_LEGACY. TTM only includes the header file, but does not use the hashtable. The vmwgfx driver uses the hashtable internally. Copy the DRM code into the driver. A later patchset should probably update vmwgfx to use Linux' hashtable. Finally, make the core hashtable code legacy. Built with/without CONFIG_DRM_LEGACY set. Thomas Zimmermann (3): drm/ttm: Don't include drm_hashtab.h drm/vmwgfx: Copy DRM hash-table code into driver drm: Declare hashtable as legacy drivers/gpu/drm/Makefile | 6 +- drivers/gpu/drm/drm_hashtab.c | 10 +- drivers/gpu/drm/drm_legacy.h | 40 +++- drivers/gpu/drm/vmwgfx/Makefile | 2 +- drivers/gpu/drm/vmwgfx/ttm_object.c | 52 ++--- drivers/gpu/drm/vmwgfx/ttm_object.h | 3 +- drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf_res.c| 24 +-- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +- drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 6 +- drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 2 +- drivers/gpu/drm/vmwgfx/vmwgfx_hashtab.c | 199 ++ .../gpu/drm/vmwgfx/vmwgfx_hashtab.h | 54 ++--- drivers/gpu/drm/vmwgfx/vmwgfx_validation.c| 22 +- drivers/gpu/drm/vmwgfx/vmwgfx_validation.h| 7 +- include/drm/drm_device.h | 5 +- include/drm/drm_legacy.h | 15 +- include/drm/ttm/ttm_bo_api.h | 1 - 17 files changed, 347 insertions(+), 103 deletions(-) create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_hashtab.c rename include/drm/drm_hashtab.h => drivers/gpu/drm/vmwgfx/vmwgfx_hashtab.h (58%) base-commit: 9fccd12cfac1c863fa46d4d17c2d8ac25a44b190 -- 2.33.1
[PATCH 2/3] drm/vmwgfx: Copy DRM hash-table code into driver
Besides some legacy code, vmwgfx is the only user of DRM's hash-
table implementation. Copy the code into the driver, so that the
core code can be retired.
No functional changes. However, the real solution for vmwgfx is to
use Linux' generic hash-table functions.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/vmwgfx/Makefile| 2 +-
drivers/gpu/drm/vmwgfx/ttm_object.c| 52 +++---
drivers/gpu/drm/vmwgfx/ttm_object.h| 3 +-
drivers/gpu/drm/vmwgfx/vmwgfx_cmdbuf_res.c | 24 +--
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c| 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h| 6 +-
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c| 2 +-
drivers/gpu/drm/vmwgfx/vmwgfx_hashtab.c| 199 +
drivers/gpu/drm/vmwgfx/vmwgfx_hashtab.h| 83 +
drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 22 +--
drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 7 +-
11 files changed, 342 insertions(+), 60 deletions(-)
create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_hashtab.c
create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_hashtab.h
diff --git a/drivers/gpu/drm/vmwgfx/Makefile b/drivers/gpu/drm/vmwgfx/Makefile
index bc323f7d4032..e6a6a1edb058 100644
--- a/drivers/gpu/drm/vmwgfx/Makefile
+++ b/drivers/gpu/drm/vmwgfx/Makefile
@@ -1,5 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
-vmwgfx-y := vmwgfx_execbuf.o vmwgfx_gmr.o vmwgfx_kms.o vmwgfx_drv.o \
+vmwgfx-y := vmwgfx_execbuf.o vmwgfx_gmr.o vmwgfx_hashtab.o vmwgfx_kms.o
vmwgfx_drv.o \
vmwgfx_ioctl.o vmwgfx_resource.o vmwgfx_ttm_buffer.o \
vmwgfx_cmd.o vmwgfx_irq.o vmwgfx_ldu.o vmwgfx_ttm_glue.o \
vmwgfx_overlay.o vmwgfx_gmrid_manager.o vmwgfx_fence.o \
diff --git a/drivers/gpu/drm/vmwgfx/ttm_object.c
b/drivers/gpu/drm/vmwgfx/ttm_object.c
index 04789b2bb2a2..123ab2cbec48 100644
--- a/drivers/gpu/drm/vmwgfx/ttm_object.c
+++ b/drivers/gpu/drm/vmwgfx/ttm_object.c
@@ -70,7 +70,7 @@ struct ttm_object_file {
struct ttm_object_device *tdev;
spinlock_t lock;
struct list_head ref_list;
- struct drm_open_hash ref_hash[TTM_REF_NUM];
+ struct vmwgfx_open_hash ref_hash[TTM_REF_NUM];
struct kref refcount;
};
@@ -88,7 +88,7 @@ struct ttm_object_file {
struct ttm_object_device {
spinlock_t object_lock;
- struct drm_open_hash object_hash;
+ struct vmwgfx_open_hash object_hash;
atomic_t object_count;
struct ttm_mem_global *mem_glob;
struct dma_buf_ops ops;
@@ -120,7 +120,7 @@ struct ttm_object_device {
struct ttm_ref_object {
struct rcu_head rcu_head;
- struct drm_hash_item hash;
+ struct vmwgfx_hash_item hash;
struct list_head head;
struct kref kref;
enum ttm_ref_type ref_type;
@@ -244,12 +244,12 @@ void ttm_base_object_unref(struct ttm_base_object
**p_base)
struct ttm_base_object *
ttm_base_object_noref_lookup(struct ttm_object_file *tfile, uint32_t key)
{
- struct drm_hash_item *hash;
- struct drm_open_hash *ht = &tfile->ref_hash[TTM_REF_USAGE];
+ struct vmwgfx_hash_item *hash;
+ struct vmwgfx_open_hash *ht = &tfile->ref_hash[TTM_REF_USAGE];
int ret;
rcu_read_lock();
- ret = drm_ht_find_item_rcu(ht, key, &hash);
+ ret = vmwgfx_ht_find_item_rcu(ht, key, &hash);
if (ret) {
rcu_read_unlock();
return NULL;
@@ -264,12 +264,12 @@ struct ttm_base_object *ttm_base_object_lookup(struct
ttm_object_file *tfile,
uint32_t key)
{
struct ttm_base_object *base = NULL;
- struct drm_hash_item *hash;
- struct drm_open_hash *ht = &tfile->ref_hash[TTM_REF_USAGE];
+ struct vmwgfx_hash_item *hash;
+ struct vmwgfx_open_hash *ht = &tfile->ref_hash[TTM_REF_USAGE];
int ret;
rcu_read_lock();
- ret = drm_ht_find_item_rcu(ht, key, &hash);
+ ret = vmwgfx_ht_find_item_rcu(ht, key, &hash);
if (likely(ret == 0)) {
base = drm_hash_entry(hash, struct ttm_ref_object, hash)->obj;
@@ -309,12 +309,12 @@ ttm_base_object_lookup_for_ref(struct ttm_object_device
*tdev, uint32_t key)
bool ttm_ref_object_exists(struct ttm_object_file *tfile,
struct ttm_base_object *base)
{
- struct drm_open_hash *ht = &tfile->ref_hash[TTM_REF_USAGE];
- struct drm_hash_item *hash;
+ struct vmwgfx_open_hash *ht = &tfile->ref_hash[TTM_REF_USAGE];
+ struct vmwgfx_hash_item *hash;
struct ttm_ref_object *ref;
rcu_read_lock();
- if (unlikely(drm_ht_find_item_rcu(ht, base->handle, &hash) != 0))
+ if (unlikely(vmwgfx_ht_find_item_rcu(ht, base->handle, &hash) != 0))
goto out_false;
/*
@@ -346,9 +346,9 @@ int ttm_ref_object_add(struct ttm_object_file *tfile,
enum ttm_ref_type ref_type, bool *existed,
bool require_existed)
{
- struct drm_o
[PATCH 1/3] drm/ttm: Don't include drm_hashtab.h
Remove the include statement for drm_hashtab.h. It's not required by TTM. Signed-off-by: Thomas Zimmermann --- include/drm/ttm/ttm_bo_api.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index cd785cfa3123..c17b2df9178b 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -32,7 +32,6 @@ #define _TTM_BO_API_H_ #include -#include #include #include #include -- 2.33.1
[PATCH 3/3] drm: Declare hashtable as legacy
The DRM hashtable code is only used by internal functions for legacy
UMS drivers. Move the implementation behind CONFIG_DRM_LEGACY and the
declarations into legacy header files. Unexport the symbols.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/Makefile | 6 +--
drivers/gpu/drm/drm_hashtab.c | 10 +
drivers/gpu/drm/drm_legacy.h | 40 +-
include/drm/drm_device.h | 5 +--
include/drm/drm_hashtab.h | 79 ---
include/drm/drm_legacy.h | 15 ++-
6 files changed, 59 insertions(+), 96 deletions(-)
delete mode 100644 include/drm/drm_hashtab.h
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c41156deb5f..bc5f0db63cba 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -6,7 +6,7 @@
drm-y := drm_aperture.o drm_auth.o drm_cache.o \
drm_file.o drm_gem.o drm_ioctl.o \
drm_drv.o \
- drm_sysfs.o drm_hashtab.o drm_mm.o \
+ drm_sysfs.o drm_mm.o \
drm_crtc.o drm_fourcc.o drm_modes.o drm_edid.o drm_displayid.o \
drm_trace_points.o drm_prime.o \
drm_vma_manager.o \
@@ -20,8 +20,8 @@ drm-y :=drm_aperture.o drm_auth.o drm_cache.o \
drm_managed.o drm_vblank_work.o
drm-$(CONFIG_DRM_LEGACY) += drm_agpsupport.o drm_bufs.o drm_context.o
drm_dma.o \
- drm_irq.o drm_legacy_misc.o drm_lock.o drm_memory.o
\
- drm_scatter.o drm_vm.o
+ drm_hashtab.o drm_irq.o drm_legacy_misc.o
drm_lock.o \
+ drm_memory.o drm_scatter.o drm_vm.o
drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
drm-$(CONFIG_COMPAT) += drm_ioc32.o
drm-$(CONFIG_DRM_PANEL) += drm_panel.o
diff --git a/drivers/gpu/drm/drm_hashtab.c b/drivers/gpu/drm/drm_hashtab.c
index c50fa6f0709f..60afa1865559 100644
--- a/drivers/gpu/drm/drm_hashtab.c
+++ b/drivers/gpu/drm/drm_hashtab.c
@@ -32,16 +32,16 @@
* Thomas Hellstr??m
*/
-#include
#include
#include
#include
#include
#include
-#include
#include
+#include "drm_legacy.h"
+
int drm_ht_create(struct drm_open_hash *ht, unsigned int order)
{
unsigned int size = 1 << order;
@@ -58,7 +58,6 @@ int drm_ht_create(struct drm_open_hash *ht, unsigned int
order)
}
return 0;
}
-EXPORT_SYMBOL(drm_ht_create);
void drm_ht_verbose_list(struct drm_open_hash *ht, unsigned long key)
{
@@ -135,7 +134,6 @@ int drm_ht_insert_item(struct drm_open_hash *ht, struct
drm_hash_item *item)
}
return 0;
}
-EXPORT_SYMBOL(drm_ht_insert_item);
/*
* Just insert an item and return any "bits" bit key that hasn't been
@@ -164,7 +162,6 @@ int drm_ht_just_insert_please(struct drm_open_hash *ht,
struct drm_hash_item *it
}
return 0;
}
-EXPORT_SYMBOL(drm_ht_just_insert_please);
int drm_ht_find_item(struct drm_open_hash *ht, unsigned long key,
struct drm_hash_item **item)
@@ -178,7 +175,6 @@ int drm_ht_find_item(struct drm_open_hash *ht, unsigned
long key,
*item = hlist_entry(list, struct drm_hash_item, head);
return 0;
}
-EXPORT_SYMBOL(drm_ht_find_item);
int drm_ht_remove_key(struct drm_open_hash *ht, unsigned long key)
{
@@ -197,7 +193,6 @@ int drm_ht_remove_item(struct drm_open_hash *ht, struct
drm_hash_item *item)
hlist_del_init_rcu(&item->head);
return 0;
}
-EXPORT_SYMBOL(drm_ht_remove_item);
void drm_ht_remove(struct drm_open_hash *ht)
{
@@ -206,4 +201,3 @@ void drm_ht_remove(struct drm_open_hash *ht)
ht->table = NULL;
}
}
-EXPORT_SYMBOL(drm_ht_remove);
diff --git a/drivers/gpu/drm/drm_legacy.h b/drivers/gpu/drm/drm_legacy.h
index c9206840c87f..70c9dba114a6 100644
--- a/drivers/gpu/drm/drm_legacy.h
+++ b/drivers/gpu/drm/drm_legacy.h
@@ -35,9 +35,47 @@
#include
struct agp_memory;
+struct drm_buf_desc;
struct drm_device;
struct drm_file;
-struct drm_buf_desc;
+struct drm_hash_item;
+struct drm_open_hash;
+
+/*
+ * Hash-table Support
+ */
+
+#define drm_hash_entry(_ptr, _type, _member) container_of(_ptr, _type, _member)
+
+/* drm_hashtab.c */
+#if IS_ENABLED(CONFIG_DRM_LEGACY)
+int drm_ht_create(struct drm_open_hash *ht, unsigned int order);
+int drm_ht_insert_item(struct drm_open_hash *ht, struct drm_hash_item *item);
+int drm_ht_just_insert_please(struct drm_open_hash *ht, struct drm_hash_item
*item,
+ unsigned long seed, int bits, int shift,
+ unsigned long add);
+int drm_ht_find_item(struct drm_open_hash *ht, unsigned long key, struct
drm_hash_item **item);
+
+void drm_ht_verbose_list(struct drm_open_hash *ht, unsigned long key);
+int drm_ht_remove_key(struct drm_open_hash *ht, unsigned long key);
+int drm_ht_remove_item(struct drm_open_hash *ht, struct drm_hash_item *item);
+void drm_ht_remove(struct drm_open_hash *ht);
+#endif
+
+/*
+ * RCU-
Re: [PATCH 08/11] dmaengine: xilinx_dpdma: stop using slave_id field
Hi Arnd, On Mon, Nov 15, 2021 at 01:38:07PM +0100, Arnd Bergmann wrote: > On Mon, Nov 15, 2021 at 12:49 PM Laurent Pinchart wrote: > > On Mon, Nov 15, 2021 at 11:21:30AM +0100, Arnd Bergmann wrote: > > > On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart wrote: > > > > On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote: > > > > > @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct > > > > > dma_chan *dchan, > > > > > spin_lock_irqsave(&chan->lock, flags); > > > > > > > > > > /* > > > > > - * Abuse the slave_id to indicate that the channel is part of a > > > > > video > > > > > - * group. > > > > > + * Abuse the peripheral_config to indicate that the channel is > > > > > part > > > > > > > > Is it still an abuse, or is this now the right way to pass custom data > > > > to the DMA engine driver ? > > > > > > It doesn't make the driver any more portable, but it's now being > > > more explicit about it. As far as I can tell, this is the best way > > > to pass data that cannot be expressed through the regular interfaces > > > in DT and the dmaengine API. > > > > > > Ideally there would be a generic way to pass this flag, but I couldn't > > > figure out what this is actually doing, or whether there is a better > > > way. Maybe Vinod has an idea. > > > > I don't think we need a generic API in this case. The DMA engine is > > specific to the display device, I don't foresee a need to mix-n-match. > > Right. I wonder if there is even a point in using the dmaengine API > in that case, I think for other single-purpose drivers we tend to just > integrate the functionality in the client driver. No point changing this > now of course, but it does feel odd. I agree, and that's what I would have done as well, if it wasn't for the fact that the DMA engine also supports a second client for audio. This isn't supported in upstream yet. We could still have created an ad-hoc solution, possibly based on the components framework, but the DMA engine subsystem wasn't a bad fit. > From my earlier reading of the driver, my impression was that this > is just a memory-to-memory device, so it could be used that way > as well, but does need a flag when working on the video memory. > I couldn't quite make sense of that though. It's only memory-to-device (video and audio). See figures 33-1 and 33-16 in https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf > > > /* > > > * Use the peripheral_config to indicate that the channel is part > > > * of a video group. This requires matching use of the custom > > > * structure in each driver. > > > */ > > > pconfig = config->peripheral_config; > > > if (WARN_ON(config->peripheral_size != 0 && > > > config->peripheral_size != sizeof(*pconfig))) > > > return -EINVAL; > > > > How about > > > > if (WARN_ON(config->peripheral_config && > > config->peripheral_size != sizeof(*pconfig))) > > > > > > > > spin_lock_irqsave(&chan->lock, flags); > > > if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && > > > config->peripheral_size == sizeof(*pconfig)) > > > > And here you can test pconfig != NULL. > > Good idea. Changed now, using 'if (pconfig)' without the '!= NULL' > in both expressions. Sounds good to me. -- Regards, Laurent Pinchart
Re: [PATCH 03/11] mmc: bcm2835: stop setting chan_config->slave_id
On Mon, 15 Nov 2021 at 09:55, Arnd Bergmann wrote: > > From: Arnd Bergmann > > The field is not interpreted by the DMA engine driver, as all the data > is passed from devicetree instead. Remove the assignment so the field > can eventually be deleted. > > Signed-off-by: Arnd Bergmann Acked-by: Ulf Hansson Kind regards Uffe > --- > drivers/mmc/host/bcm2835.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/mmc/host/bcm2835.c b/drivers/mmc/host/bcm2835.c > index 8c2361e66277..463b707d9e99 100644 > --- a/drivers/mmc/host/bcm2835.c > +++ b/drivers/mmc/host/bcm2835.c > @@ -1293,14 +1293,12 @@ static int bcm2835_add_host(struct bcm2835_host *host) > > host->dma_cfg_tx.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > host->dma_cfg_tx.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > - host->dma_cfg_tx.slave_id = 13; /* DREQ channel */ > host->dma_cfg_tx.direction = DMA_MEM_TO_DEV; > host->dma_cfg_tx.src_addr = 0; > host->dma_cfg_tx.dst_addr = host->phys_addr + SDDATA; > > host->dma_cfg_rx.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > host->dma_cfg_rx.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; > - host->dma_cfg_rx.slave_id = 13; /* DREQ channel */ > host->dma_cfg_rx.direction = DMA_DEV_TO_MEM; > host->dma_cfg_rx.src_addr = host->phys_addr + SDDATA; > host->dma_cfg_rx.dst_addr = 0; > -- > 2.30.2 >
