Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV
Am 05.01.22 um 08:34 schrieb JingWen Chen: On 2022/1/5 上午12:56, Andrey Grodzovsky wrote: On 2022-01-04 6:36 a.m., Christian König wrote: Am 04.01.22 um 11:49 schrieb Liu, Monk: [AMD Official Use Only] See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. No it's not, FLR from hypervisor is just to notify guest the hw VF FLR is about to start or was already executed, but host will do FLR anyway without waiting for guest too long Then we have a major design issue in the SRIOV protocol and really need to question this. How do you want to prevent a race between the hypervisor resetting the hardware and the client trying the same because of a timeout? As far as I can see the procedure should be: 1. We detect that a reset is necessary, either because of a fault a timeout or signal from hypervisor. 2. For each of those potential reset sources a work item is send to the single workqueue. 3. One of those work items execute first and prepares the reset. 4. We either do the reset our self or notify the hypervisor that we are ready for the reset. 5. Cleanup after the reset, eventually resubmit jobs etc.. 6. Cancel work items which might have been scheduled from other reset sources. It does make sense that the hypervisor resets the hardware without waiting for the clients for too long, but if we don't follow this general steps we will always have a race between the different components. Monk, just to add to this - if indeed as you say that 'FLR from hypervisor is just to notify guest the hw VF FLR is about to start or was already executed, but host will do FLR anyway without waiting for guest too long' and there is no strict waiting from the hypervisor for IDH_READY_TO_RESET to be recived from guest before starting the reset then setting in_gpu_reset and locking reset_sem from guest side is not really full proof protection from MMIO accesses by the guest - it only truly helps if hypervisor waits for that message before initiation of HW reset. Hi Andrey, this cannot be done. If somehow guest kernel hangs and never has the chance to send the response back, then other VFs will have to wait it reset. All the vfs will hang in this case. Or sometimes the mailbox has some delay and other VFs will also wait. The user of other VFs will be affected in this case. Yeah, agree completely with JingWen. The hypervisor is the one in charge here, not the guest. What the hypervisor should do (and it already seems to be designed that way) is to send the guest a message that a reset is about to happen and give it some time to response appropriately. The guest on the other hand then tells the hypervisor that all processing has stopped and it is ready to restart. If that doesn't happen in time the hypervisor should eliminate the guest probably trigger even more severe consequences, e.g. restart the whole VM etc... Christian. Andrey Regards, Christian. Am 04.01.22 um 11:49 schrieb Liu, Monk: [AMD Official Use Only] See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. No it's not, FLR from hypervisor is just to notify guest the hw VF FLR is about to start or was already executed, but host will do FLR anyway without waiting for guest too long In other words I strongly think that the current SRIOV reset implementation is severely broken and what Andrey is doing is actually fixing it. It makes the code to crash ... how could it be a fix ? I'm afraid the patch is NAK from me, but it is welcome if the cleanup do not ruin the logic, Andry or jingwen can try it if needed. Thanks --- Monk Liu | Cloud GPU & Virtualization Solution | AMD --- we are hiring software manager for CVS core team --- -Original Message- From: Koenig, Christian Sent: Tuesday, January 4, 2022 6:19 PM To: Chen, JingWen ; Christian König ; Grodzovsky, Andrey ; Deng, Emily ; Liu, Monk ; dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; Chen, Horace ; Chen, JingWen Cc: dan...@ffwll.ch Subject: Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV Hi Jingwen, well what I mean is that we need to adjust the implementation in amdgpu to actually match the requirements. Could be that the reset sequence is questionable in general, but I doubt so at least for now. See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between
[PATCH] drm/imx: imx-ldb: Check for null pointer after calling kmemdup
As the possible failure of the allocation, kmemdup() may return NULL
pointer.
Therefore, it should be better to check the return value of kmemdup()
and return error if fails.
Fixes: dc80d7038883 ("drm/imx-ldb: Add support to drm-bridge")
Signed-off-by: Jiasheng Jiang
---
drivers/gpu/drm/imx/imx-ldb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 53132ddf9587..f9880a779678 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -574,6 +574,8 @@ static int imx_ldb_panel_ddc(struct device *dev,
edidp = of_get_property(child, "edid", _len);
if (edidp) {
channel->edid = kmemdup(edidp, edid_len, GFP_KERNEL);
+ if (!channel->edid)
+ return -ENOMEM;
} else if (!channel->panel) {
/* fallback to display-timings node */
ret = of_get_drm_display_mode(child,
--
2.25.1
Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV
On 2022/1/5 上午12:56, Andrey Grodzovsky wrote: > > On 2022-01-04 6:36 a.m., Christian König wrote: >> Am 04.01.22 um 11:49 schrieb Liu, Monk: >>> [AMD Official Use Only] >>> > See the FLR request from the hypervisor is just another source of > signaling the need for a reset, similar to each job timeout on each > queue. Otherwise you have a race condition between the hypervisor and the > scheduler. >>> No it's not, FLR from hypervisor is just to notify guest the hw VF FLR is >>> about to start or was already executed, but host will do FLR anyway without >>> waiting for guest too long >>> >> >> Then we have a major design issue in the SRIOV protocol and really need to >> question this. >> >> How do you want to prevent a race between the hypervisor resetting the >> hardware and the client trying the same because of a timeout? >> >> As far as I can see the procedure should be: >> 1. We detect that a reset is necessary, either because of a fault a timeout >> or signal from hypervisor. >> 2. For each of those potential reset sources a work item is send to the >> single workqueue. >> 3. One of those work items execute first and prepares the reset. >> 4. We either do the reset our self or notify the hypervisor that we are >> ready for the reset. >> 5. Cleanup after the reset, eventually resubmit jobs etc.. >> 6. Cancel work items which might have been scheduled from other reset >> sources. >> >> It does make sense that the hypervisor resets the hardware without waiting >> for the clients for too long, but if we don't follow this general steps we >> will always have a race between the different components. > > > Monk, just to add to this - if indeed as you say that 'FLR from hypervisor is > just to notify guest the hw VF FLR is about to start or was already executed, > but host will do FLR anyway without waiting for guest too long' > and there is no strict waiting from the hypervisor for IDH_READY_TO_RESET to > be recived from guest before starting the reset then setting in_gpu_reset and > locking reset_sem from guest side is not really full proof > protection from MMIO accesses by the guest - it only truly helps if > hypervisor waits for that message before initiation of HW reset. > Hi Andrey, this cannot be done. If somehow guest kernel hangs and never has the chance to send the response back, then other VFs will have to wait it reset. All the vfs will hang in this case. Or sometimes the mailbox has some delay and other VFs will also wait. The user of other VFs will be affected in this case. > Andrey > > >> >> Regards, >> Christian. >> >> Am 04.01.22 um 11:49 schrieb Liu, Monk: >>> [AMD Official Use Only] >>> > See the FLR request from the hypervisor is just another source of > signaling the need for a reset, similar to each job timeout on each > queue. Otherwise you have a race condition between the hypervisor and the > scheduler. >>> No it's not, FLR from hypervisor is just to notify guest the hw VF FLR is >>> about to start or was already executed, but host will do FLR anyway without >>> waiting for guest too long >>> > In other words I strongly think that the current SRIOV reset > implementation is severely broken and what Andrey is doing is actually > fixing it. >>> It makes the code to crash ... how could it be a fix ? >>> >>> I'm afraid the patch is NAK from me, but it is welcome if the cleanup do >>> not ruin the logic, Andry or jingwen can try it if needed. >>> >>> Thanks >>> --- >>> Monk Liu | Cloud GPU & Virtualization Solution | AMD >>> --- >>> we are hiring software manager for CVS core team >>> --- >>> >>> -Original Message- >>> From: Koenig, Christian >>> Sent: Tuesday, January 4, 2022 6:19 PM >>> To: Chen, JingWen ; Christian König >>> ; Grodzovsky, Andrey >>> ; Deng, Emily ; Liu, Monk >>> ; dri-devel@lists.freedesktop.org; >>> amd-...@lists.freedesktop.org; Chen, Horace ; Chen, >>> JingWen >>> Cc: dan...@ffwll.ch >>> Subject: Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset >>> protection for SRIOV >>> >>> Hi Jingwen, >>> >>> well what I mean is that we need to adjust the implementation in amdgpu to >>> actually match the requirements. >>> >>> Could be that the reset sequence is questionable in general, but I doubt so >>> at least for now. >>> >>> See the FLR request from the hypervisor is just another source of signaling >>> the need for a reset, similar to each job timeout on each queue. Otherwise >>> you have a race condition between the hypervisor and the scheduler. >>> >>> Properly setting in_gpu_reset is indeed mandatory, but should happen at a >>> central place and not in the SRIOV specific code. >>> >>> In other words I strongly think that the current SRIOV reset implementation >>> is severely
Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV
On 2022/1/4 下午7:36, Christian König wrote: > Am 04.01.22 um 11:49 schrieb Liu, Monk: >> [AMD Official Use Only] >> See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. >> No it's not, FLR from hypervisor is just to notify guest the hw VF FLR is >> about to start or was already executed, but host will do FLR anyway without >> waiting for guest too long >> > > Then we have a major design issue in the SRIOV protocol and really need to > question this. > > How do you want to prevent a race between the hypervisor resetting the > hardware and the client trying the same because of a timeout? > > As far as I can see the procedure should be: > 1. We detect that a reset is necessary, either because of a fault a timeout > or signal from hypervisor. > 2. For each of those potential reset sources a work item is send to the > single workqueue. I think Andrey has already used the same ordered work queue to handle the reset from both ring timeout and hypervisor. (Patch 5) So there should be no race between different reset sources. As ring timeout is much longer than the world switch time slice(6ms), we should see a reset from hypervisor queued into reset domain wq first and after the flr work done, then the ring timeout reset queued into reset domain. > 3. One of those work items execute first and prepares the reset. > 4. We either do the reset our self or notify the hypervisor that we are ready > for the reset. > 5. Cleanup after the reset, eventually resubmit jobs etc.. > 6. Cancel work items which might have been scheduled from other reset sources. > > It does make sense that the hypervisor resets the hardware without waiting > for the clients for too long, but if we don't follow this general steps we > will always have a race between the different components. So the reset_sem and in_gpu_reset is to prevent race between reset_domain(mostly hypervisor source) and other user spaces(e.g. kfd). > > Regards, > Christian. > > Am 04.01.22 um 11:49 schrieb Liu, Monk: >> [AMD Official Use Only] >> See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. >> No it's not, FLR from hypervisor is just to notify guest the hw VF FLR is >> about to start or was already executed, but host will do FLR anyway without >> waiting for guest too long >> In other words I strongly think that the current SRIOV reset implementation is severely broken and what Andrey is doing is actually fixing it. >> It makes the code to crash ... how could it be a fix ? >> >> I'm afraid the patch is NAK from me, but it is welcome if the cleanup do >> not ruin the logic, Andry or jingwen can try it if needed. >> >> Thanks >> --- >> Monk Liu | Cloud GPU & Virtualization Solution | AMD >> --- >> we are hiring software manager for CVS core team >> --- >> >> -Original Message- >> From: Koenig, Christian >> Sent: Tuesday, January 4, 2022 6:19 PM >> To: Chen, JingWen ; Christian König >> ; Grodzovsky, Andrey >> ; Deng, Emily ; Liu, Monk >> ; dri-devel@lists.freedesktop.org; >> amd-...@lists.freedesktop.org; Chen, Horace ; Chen, >> JingWen >> Cc: dan...@ffwll.ch >> Subject: Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection >> for SRIOV >> >> Hi Jingwen, >> >> well what I mean is that we need to adjust the implementation in amdgpu to >> actually match the requirements. >> >> Could be that the reset sequence is questionable in general, but I doubt so >> at least for now. >> >> See the FLR request from the hypervisor is just another source of signaling >> the need for a reset, similar to each job timeout on each queue. Otherwise >> you have a race condition between the hypervisor and the scheduler. >> >> Properly setting in_gpu_reset is indeed mandatory, but should happen at a >> central place and not in the SRIOV specific code. >> >> In other words I strongly think that the current SRIOV reset implementation >> is severely broken and what Andrey is doing is actually fixing it. >> >> Regards, >> Christian. >> >> Am 04.01.22 um 10:07 schrieb JingWen Chen: >>> Hi Christian, >>> I'm not sure what do you mean by "we need to change SRIOV not the driver". >>> >>> Do you mean we should change the reset sequence in SRIOV? This will be a >>> huge change for our SRIOV solution. >>> >>> From my point of view, we can directly use amdgpu_device_lock_adev >>> and amdgpu_device_unlock_adev in flr_work instead of try_lock since no one >>> will
Re: [PATCH 1/2] drm/bridge: anx7625: add HDCP support
On Tue, Jan 04, 2022 at 03:50:34PM +0100, Robert Foss wrote:
> Hey Xin,
Hi Robert Foss, thanks for the reply.
As HDCP config interface "anx7625_hdcp_config(..)" need be called in
anx7625_connector_atomic_check(...) interface, so I cannot split out
atomic conversion patch.
Thanks,
Xin
>
> On Tue, 9 Nov 2021 at 03:42, Xin Ji wrote:
> >
> > This patch provides HDCP setting interface for userspace to dynamic
> > enable/disable HDCP function.
> >
> > Signed-off-by: Xin Ji
> > ---
> > drivers/gpu/drm/bridge/analogix/anx7625.c | 368 +-
> > drivers/gpu/drm/bridge/analogix/anx7625.h | 69 +++-
> > 2 files changed, 425 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > index 001fb39d9919..6d93026c2999 100644
> > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > @@ -26,6 +26,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -213,6 +214,60 @@ static int wait_aux_op_finish(struct anx7625_data *ctx)
> > return 0;
> > }
> >
> > +static int anx7625_aux_dpcd_read(struct anx7625_data *ctx,
> > +u8 addrh, u8 addrm, u8 addrl,
> > +u8 len, u8 *buf)
> > +{
> > + struct device *dev = >client->dev;
> > + int ret;
> > + u8 cmd;
> > +
> > + if (len > MAX_DPCD_BUFFER_SIZE) {
> > + DRM_DEV_ERROR(dev, "exceed aux buffer len.\n");
> > + return -EINVAL;
> > + }
> > +
> > + cmd = ((len - 1) << 4) | 0x09;
> > +
> > + /* Set command and length */
> > + ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > + AP_AUX_COMMAND, cmd);
> > +
> > + /* Set aux access address */
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > +AP_AUX_ADDR_7_0, addrl);
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > +AP_AUX_ADDR_15_8, addrm);
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > +AP_AUX_ADDR_19_16, addrh);
> > +
> > + /* Enable aux access */
> > + ret |= anx7625_write_or(ctx, ctx->i2c.rx_p0_client,
> > + AP_AUX_CTRL_STATUS, AP_AUX_CTRL_OP_EN);
> > +
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "cannot access aux related register.\n");
> > + return -EIO;
> > + }
> > +
> > + usleep_range(2000, 2100);
> > +
> > + ret = wait_aux_op_finish(ctx);
> > + if (ret) {
> > + DRM_DEV_ERROR(dev, "aux IO error: wait aux op finish.\n");
> > + return ret;
> > + }
> > +
> > + ret = anx7625_reg_block_read(ctx, ctx->i2c.rx_p0_client,
> > +AP_AUX_BUFF_START, len, buf);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "read dpcd register failed\n");
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int anx7625_video_mute_control(struct anx7625_data *ctx,
> > u8 status)
> > {
> > @@ -669,6 +724,160 @@ static int anx7625_dpi_config(struct anx7625_data
> > *ctx)
> > return ret;
> > }
> >
> > +static int anx7625_read_flash_status(struct anx7625_data *ctx)
> > +{
> > + return anx7625_reg_read(ctx, ctx->i2c.rx_p0_client, R_RAM_CTRL);
> > +}
> > +
> > +static int anx7625_hdcp_key_probe(struct anx7625_data *ctx)
> > +{
> > + int ret, val;
> > + struct device *dev = >client->dev;
> > + u8 ident[32];
>
> Could this hardcoded array length be replaced with FLASH_BUF_LEN?
>
> > +
> > + ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > + FLASH_ADDR_HIGH, 0x91);
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > +FLASH_ADDR_LOW, 0xA0);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "IO error : set key flash address.\n");
> > + return ret;
> > + }
> > +
> > + ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > + FLASH_LEN_HIGH, (FLASH_BUF_LEN - 1) >> 8);
> > + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > +FLASH_LEN_LOW, (FLASH_BUF_LEN - 1) & 0xFF);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "IO error : set key flash len.\n");
> > + return ret;
> > + }
> > +
> > + ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> > + R_FLASH_RW_CTRL, FLASH_READ);
> > + ret |= readx_poll_timeout(anx7625_read_flash_status,
> > + ctx, val,
> > + ((val &
Re: [PATCH v2] dma-buf: dma-heap: Add a size check for allocation
From: Guangming.Cao On Tue, 2022-01-04 at 08:47 +0100, Christian K鰊ig wrote: > Am 03.01.22 um 19:57 schrieb John Stultz: > > On Mon, Dec 27, 2021 at 1:52 AM wrote: > > > From: Guangming > > > > > > > Thanks for submitting this! > > > > > Add a size check for allcation since the allocation size is > > > > nit: "allocation" above. > > > > > always less than the total DRAM size. > > > > In general, it might be good to add more context to the commit > > message > > to better answer *why* this change is needed rather than what the > > change is doing. ie: What negative thing happens without this > > change? > > And so how does this change avoid or improve things? > > Completely agree, just one little addition: Could you also add this > why > as comment to the code? > > When we stumble over this five years from now it is absolutely not > obvious why we do this. > > Thanks, > Christian. > Thanks for your reply! I will update the related reason in the patch later. The reason for adding this check is that we met a case that the user sent an invalid size(It seems it's a negative value, MSB is 0xff, it's larger than DRAM size after convert it to size_t) to dma-heap to alloc memory, and this allocation was running on a process(such as "gralloc" on Android device) can't be killed by OOM flow, and we also couldn't find the related dmabuf in "dma_buf_debug_show" because the related dmabuf was not exported yet since the allocation is still on going. Since this invalid argument case can be prevented at dma-heap side, so, I added this size check, and moreover, to let debug it easily, I also added logs when size is bigger than a threshold we set in mtk system heap. If you think that print logs in dma-heap framework is better, I will update it in next version. If you have better solution(such as dump the size under allocating in "dma_buf_debug_show", which maybe need add global variable to record it), please kindly let me know. Thanks :) Guangming > > > > > > > Signed-off-by: Guangming > > > Signed-off-by: jianjiao zeng > > > --- > > > v2: 1. update size limitation as total_dram page size. > > > 2. update commit message > > > --- > > > drivers/dma-buf/dma-heap.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma- > > > heap.c > > > index 56bf5ad01ad5..e39d2be98d69 100644 > > > --- a/drivers/dma-buf/dma-heap.c > > > +++ b/drivers/dma-buf/dma-heap.c > > > @@ -55,6 +55,8 @@ static int dma_heap_buffer_alloc(struct > > > dma_heap *heap, size_t len, > > > struct dma_buf *dmabuf; > > > int fd; > > > > > > + if (len / PAGE_SIZE > totalram_pages()) > > > + return -EINVAL; > > > > This seems sane. I know ION used to have some 1/2 of memory cap to > > avoid unnecessary memory pressure on crazy allocations. > > > > Could you send again with an improved commit message? > > > > thanks > > -john > >
Re: [PATCH v10 2/6] drm/i915: Use to_gt() helper for GGTT accesses
On Wed, Jan 05, 2022 at 12:35:50AM +0200, Andi Shyti wrote:
> From: Michał Winiarski
>
> GGTT is currently available both through i915->ggtt and gt->ggtt, and we
> eventually want to get rid of the i915->ggtt one.
> Use to_gt() for all i915->ggtt accesses to help with the future
> refactoring.
>
> Signed-off-by: Michał Winiarski
> Cc: Michal Wajdeczko
> Signed-off-by: Andi Shyti
> ---
> Hi Matt,
>
> I'm sending this this v10 as reply-to, if it confuses you I can
> send the whole v10.
>
> The only trivial difference is that in i915_perf.c I'm using
>
> to_gt(perf->i915)
>
> instead of
>
> to_gt(perf->i915)->ggtt->vm.gt
>
Reviewed-by: Matt Roper
> Andi
>
> drivers/gpu/drm/i915/gvt/dmabuf.c| 2 +-
> drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
> drivers/gpu/drm/i915/i915_driver.c | 4 ++--
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/i915_gem.c | 23 ---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 6 +++---
> drivers/gpu/drm/i915/i915_getparam.c | 2 +-
> drivers/gpu/drm/i915/i915_perf.c | 6 +++---
> 8 files changed, 25 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c
> b/drivers/gpu/drm/i915/gvt/dmabuf.c
> index 8e65cd8258b9..94c3eb1586b0 100644
> --- a/drivers/gpu/drm/i915/gvt/dmabuf.c
> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
> @@ -84,7 +84,7 @@ static int vgpu_gem_get_pages(
> kfree(st);
> return ret;
> }
> - gtt_entries = (gen8_pte_t __iomem *)dev_priv->ggtt.gsm +
> + gtt_entries = (gen8_pte_t __iomem *)to_gt(dev_priv)->ggtt->gsm +
> (fb_info->start >> PAGE_SHIFT);
> for_each_sg(st->sgl, sg, page_num, i) {
> dma_addr_t dma_addr =
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index e0e052cdf8b8..6966fe08df92 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -390,9 +390,9 @@ static int i915_swizzle_info(struct seq_file *m, void
> *data)
> intel_wakeref_t wakeref;
>
> seq_printf(m, "bit6 swizzle for X-tiling = %s\n",
> -swizzle_string(dev_priv->ggtt.bit_6_swizzle_x));
> +swizzle_string(to_gt(dev_priv)->ggtt->bit_6_swizzle_x));
> seq_printf(m, "bit6 swizzle for Y-tiling = %s\n",
> -swizzle_string(dev_priv->ggtt.bit_6_swizzle_y));
> +swizzle_string(to_gt(dev_priv)->ggtt->bit_6_swizzle_y));
>
> if (dev_priv->quirks & QUIRK_PIN_SWIZZLED_PAGES)
> seq_puts(m, "L-shaped memory detected\n");
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index 60f8cbf24de7..3c984553d86f 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1146,7 +1146,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>
> /* Must be called before GGTT is suspended. */
> intel_dpt_suspend(dev_priv);
> - i915_ggtt_suspend(_priv->ggtt);
> + i915_ggtt_suspend(to_gt(dev_priv)->ggtt);
>
> i915_save_display(dev_priv);
>
> @@ -1270,7 +1270,7 @@ static int i915_drm_resume(struct drm_device *dev)
> if (ret)
> drm_err(_priv->drm, "failed to re-enable GGTT\n");
>
> - i915_ggtt_resume(_priv->ggtt);
> + i915_ggtt_resume(to_gt(dev_priv)->ggtt);
> /* Must be called after GGTT is resumed. */
> intel_dpt_resume(dev_priv);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index beeb42a14aae..01518ce8f401 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1748,7 +1748,7 @@ static inline bool
> i915_gem_object_needs_bit17_swizzle(struct drm_i915_gem_objec
> {
> struct drm_i915_private *i915 = to_i915(obj->base.dev);
>
> - return i915->ggtt.bit_6_swizzle_x == I915_BIT_6_SWIZZLE_9_10_17 &&
> + return to_gt(i915)->ggtt->bit_6_swizzle_x == I915_BIT_6_SWIZZLE_9_10_17
> &&
> i915_gem_object_is_tiled(obj);
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 915bf431f320..e3730096abd9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -88,7 +88,8 @@ int
> i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
> struct drm_file *file)
> {
> - struct i915_ggtt *ggtt = _i915(dev)->ggtt;
> + struct drm_i915_private *i915 = to_i915(dev);
> + struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
> struct drm_i915_gem_get_aperture *args = data;
> struct i915_vma *vma;
> u64 pinned;
> @@ -289,7 +290,7 @@ static struct i915_vma *i915_gem_gtt_prepare(struct
> drm_i915_gem_object *obj,
>bool write)
> {
> struct drm_i915_private *i915 = to_i915(obj->base.dev);
> - struct i915_ggtt *ggtt = >ggtt;
> + struct
答复: 答复: [PATCH] gpu/drm: fix potential memleak in error branch
-邮件原件-
发件人: bern...@vivo.com 代表 Jani Nikula
发送时间: 2022年1月4日 19:25
收件人: 赵军奎 ; Maarten Lankhorst
; Maxime Ripard ; Thomas
Zimmermann ; David Airlie ; Daniel
Vetter ; dri-devel@lists.freedesktop.org;
linux-ker...@vger.kernel.org
主题: Re: 答复: [PATCH] gpu/drm: fix potential memleak in error branch
On Tue, 04 Jan 2022, 赵军奎 wrote:
> -邮件原件-
> 发件人: bern...@vivo.com 代表 Jani Nikula
> 发送时间: 2021年12月31日 19:09
> 收件人: 赵军奎 ; Maarten Lankhorst
> ; Maxime Ripard
> ; Thomas Zimmermann ; David
> Airlie ; Daniel Vetter ;
> dri-devel@lists.freedesktop.org; linux-ker...@vger.kernel.org
> 抄送: 赵军奎
> 主题: Re: [PATCH] gpu/drm: fix potential memleak in error branch
>
> On Tue, 16 Nov 2021, Bernard Zhao wrote:
>> This patch try to fix potential memleak in error branch.
>
>>Please elaborate.
>
> Hi Jani:
>
> This patch try to fix potential memleak in error branch.
> For example:
> nv50_sor_create ->nv50_mstm_new-> drm_dp_mst_topology_mgr_init In
> function drm_dp_mst_topology_mgr_init, there are five error branches, error
> branch just return error code, no free called.
> And we see that the caller didn`t do the drm_dp_mst_topology_mgr_destroy job.
> I am not sure if there some gap, I think this may bring in the risk of
> memleak issue.
> Thanks!
>This should be part of the commit message.
Hi Jani:
Thanks for your comments, I will resubmit this patch!
BR//Bernard
>
> BR//Bernard
>
>>BR,
>>Jani.
>
>
>>
>> Signed-off-by: Bernard Zhao
>> ---
>> drivers/gpu/drm/drm_dp_mst_topology.c | 22 --
>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>> b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index f3d79eda94bb..f73b180dee73 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -5501,7 +5501,10 @@ int drm_dp_mst_topology_mgr_init(struct
>> drm_dp_mst_topology_mgr *mgr,
>> int max_lane_count, int max_link_rate,
>> int conn_base_id)
>> {
>> -struct drm_dp_mst_topology_state *mst_state;
>> +struct drm_dp_mst_topology_state *mst_state = NULL;
This is superfluous.
Other than that,
Reviewed-by: Jani Nikula
>> +
>> +mgr->payloads = NULL;
>> +mgr->proposed_vcpis = NULL;
>>
>> mutex_init(>lock);
>> mutex_init(>qlock);
>> @@ -5523,7 +5526,7 @@ int drm_dp_mst_topology_mgr_init(struct
>> drm_dp_mst_topology_mgr *mgr,
>> */
>> mgr->delayed_destroy_wq = alloc_ordered_workqueue("drm_dp_mst_wq", 0);
>> if (mgr->delayed_destroy_wq == NULL)
>> -return -ENOMEM;
>> +goto out;
>>
>> INIT_WORK(>work, drm_dp_mst_link_probe_work);
>> INIT_WORK(>tx_work, drm_dp_tx_work); @@ -5539,18 +5542,18 @@
>> int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
>> mgr->conn_base_id = conn_base_id;
>> if (max_payloads + 1 > sizeof(mgr->payload_mask) * 8 ||
>> max_payloads + 1 > sizeof(mgr->vcpi_mask) * 8)
>> -return -EINVAL;
>> +goto failed;
>> mgr->payloads = kcalloc(max_payloads, sizeof(struct drm_dp_payload),
>> GFP_KERNEL);
>> if (!mgr->payloads)
>> -return -ENOMEM;
>> +goto failed;
>> mgr->proposed_vcpis = kcalloc(max_payloads, sizeof(struct drm_dp_vcpi
>> *), GFP_KERNEL);
>> if (!mgr->proposed_vcpis)
>> -return -ENOMEM;
>> +goto failed;
>> set_bit(0, >payload_mask);
>>
>> mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
>> if (mst_state == NULL)
>> -return -ENOMEM;
>> +goto failed;
>>
>> mst_state->total_avail_slots = 63;
>> mst_state->start_slot = 1;
>> @@ -5563,6 +5566,13 @@ int drm_dp_mst_topology_mgr_init(struct
>> drm_dp_mst_topology_mgr *mgr,
>> _dp_mst_topology_state_funcs);
>>
>> return 0;
>> +
>> +failed:
>> +kfree(mgr->proposed_vcpis);
>> +kfree(mgr->payloads);
>> +destroy_workqueue(mgr->delayed_destroy_wq);
>> +out:
>> +return -ENOMEM;
>> }
>> EXPORT_SYMBOL(drm_dp_mst_topology_mgr_init);
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Jani Nikula, Intel Open Source Graphics Center
[PATCH -next] drm/i915/fbc: replace DEFINE_SIMPLE_ATTRIBUTE with DEFINE_DEBUGFS_ATTRIBUTE
Fix the following coccicheck warning: ./drivers/gpu/drm/i915/display/intel_fbc.c:1757:0-23: WARNING: intel_fbc_debugfs_false_color_fops should be defined with DEFINE_DEBUGFS_ATTRIBUTE Reported-by: Abaci Robot Signed-off-by: Yang Li --- drivers/gpu/drm/i915/display/intel_fbc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c index 160fd2bdafe5..a43f5b74d6ac 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -1754,7 +1754,7 @@ static int intel_fbc_debugfs_false_color_set(void *data, u64 val) return 0; } -DEFINE_SIMPLE_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, +DEFINE_DEBUGFS_ATTRIBUTE(intel_fbc_debugfs_false_color_fops, intel_fbc_debugfs_false_color_get, intel_fbc_debugfs_false_color_set, "%llu\n"); -- 2.20.1.7.g153144c
RE: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV
[AMD Official Use Only] I see, I didn't notice you already have this implemented . so the flr_work routine itself is synced now, in this case , I agree it should be safe to remove the in_gpu_reset and reset_semm in the flr_work. Regards Shaoyun.liu -Original Message- From: Grodzovsky, Andrey Sent: Tuesday, January 4, 2022 3:55 PM To: Liu, Shaoyun ; Koenig, Christian ; Liu, Monk ; Chen, JingWen ; Christian König ; Deng, Emily ; dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; Chen, Horace Cc: dan...@ffwll.ch Subject: Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV On 2022-01-04 12:13 p.m., Liu, Shaoyun wrote: > [AMD Official Use Only] > > I mostly agree with the sequences Christian described . Just one thing > might need to discuss here. For FLR notified from host, in new sequenceas > described , driver need to reply the READY_TO_RESET in the workitem from > a reset work queue which means inside flr_work, driver can not directly > reply to host but need to queue another workqueue . Can you clarify why 'driver can not directly reply to host but need to queue another workqueue' ? To my understating all steps 3-6 in Christian's description happen from the same single wq thread serially. > For current code , the flr_work for sriov itself is a work queue queued > from ISR . I think we should try to response to the host driver as soon as > possible. Queue another workqueue inside the workqueue doesn't sounds > efficient to me. Check patch 5 please [1] - I just substituted schedule_work(>virt.flr_work) for queue_work(adev->reset_domain.wq,>virt.flr_work) so no extra requeue here, just instead of sending to system_wq it's sent to dedicated reset wq [1] - https://lore.kernel.org/all/2021121400.790842-1-andrey.grodzov...@amd.com/ Andrey > Anyway, what we need is a working solution for our project. So if we need > to change the sequence, we need to make sure it's been tested first and > won't break the functionality before the code is landed in the branch . > > Regards > Shaoyun.liu > > > -Original Message- > From: amd-gfx On Behalf Of > Christian König > Sent: Tuesday, January 4, 2022 6:36 AM > To: Liu, Monk ; Chen, JingWen > ; Christian König > ; Grodzovsky, Andrey > ; Deng, Emily ; > dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; Chen, > Horace > Cc: dan...@ffwll.ch > Subject: Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset > protection for SRIOV > > Am 04.01.22 um 11:49 schrieb Liu, Monk: >> [AMD Official Use Only] >> See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. >> No it's not, FLR from hypervisor is just to notify guest the hw VF >> FLR is about to start or was already executed, but host will do FLR >> anyway without waiting for guest too long >> > Then we have a major design issue in the SRIOV protocol and really need to > question this. > > How do you want to prevent a race between the hypervisor resetting the > hardware and the client trying the same because of a timeout? > > As far as I can see the procedure should be: > 1. We detect that a reset is necessary, either because of a fault a timeout > or signal from hypervisor. > 2. For each of those potential reset sources a work item is send to the > single workqueue. > 3. One of those work items execute first and prepares the reset. > 4. We either do the reset our self or notify the hypervisor that we are ready > for the reset. > 5. Cleanup after the reset, eventually resubmit jobs etc.. > 6. Cancel work items which might have been scheduled from other reset sources. > > It does make sense that the hypervisor resets the hardware without waiting > for the clients for too long, but if we don't follow this general steps we > will always have a race between the different components. > > Regards, > Christian. > > Am 04.01.22 um 11:49 schrieb Liu, Monk: >> [AMD Official Use Only] >> See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. >> No it's not, FLR from hypervisor is just to notify guest the hw VF >> FLR is about to start or was already executed, but host will do FLR >> anyway without waiting for guest too long >> In other words I strongly think that the current SRIOV reset implementation is severely broken and what Andrey is doing is actually fixing it. >> It makes the code to crash ... how could it be a fix ? >> >> I'm afraid the patch is NAK from me, but it is welcome if the cleanup do >> not ruin the logic, Andry or jingwen can try it if needed. >> >> Thanks >>
[PATCH] drm/msm/dp: stop link training after link training 2 failed
Each DP link training contains link training 1 followed by link
training 2. There is maximum of 5 retries of DP link training
before declared link training failed. It is required to stop link
training at end of link training 2 if it is failed so that next
link training 1 can start freshly. This patch fixes link compliance
test case 4.3.1.13 (Source Device Link Training EQ Fallback Test).
Fixes: 2e0adc765d88 ("drm/msm/dp: do not end dp link training until video is
ready")
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_ctrl.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 39558a2..3e4b3d9 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1745,6 +1745,9 @@ int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl)
/* end with failure */
break; /* lane == 1 already */
}
+
+ /* stop link training before start re training */
+ dp_ctrl_clear_training_pattern(ctrl);
}
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH] drm/i915: Lock timeline mutex directly in error path of eb_pin_timeline
Don't use the interruptable version of the timeline mutex lock in the
error path of eb_pin_timeline as the cleanup must always happen.
v2:
(John Harrison)
- Don't check for interrupt during mutex lock
Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf")
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index e9541244027a..e96e133cbb1f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2516,9 +2516,9 @@ static int eb_pin_timeline(struct i915_execbuffer *eb,
struct intel_context *ce,
timeout) < 0) {
i915_request_put(rq);
- tl = intel_context_timeline_lock(ce);
+ mutex_lock(>timeline->mutex);
intel_context_exit(ce);
- intel_context_timeline_unlock(tl);
+ mutex_unlock(>timeline->mutex);
if (nonblock)
return -EWOULDBLOCK;
--
2.34.1
Re: [PATCH] drm/i915: Check return intel_context_timeline_lock of in eb_pin_timeline
On Tue, Jan 04, 2022 at 03:05:03PM -0800, John Harrison wrote:
> On 1/4/2022 09:31, Matthew Brost wrote:
> > intel_context_timeline_lock can return can error if interrupted by a
> > user when trying to lock the timeline mutex. Check the return value of
> > intel_context_timeline_lock in eb_pin_timeline (execbuf IOCTL).
> >
> > Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf")
> > Signed-off-by: Matthew Brost
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > index e9541244027a..65a078945b00 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > @@ -2517,6 +2517,9 @@ static int eb_pin_timeline(struct i915_execbuffer
> > *eb, struct intel_context *ce,
> > i915_request_put(rq);
> > tl = intel_context_timeline_lock(ce);
> > + if (IS_ERR(tl))
> > + return PTR_ERR(tl);
> > +
> > intel_context_exit(ce);
> Won't this leak the stuff acquired by the intel_context_enter() above if the
> _exit() is now skipped?
>
Yep, this isn't right. I think should just call mutex_lock /
mutex_unlock directly on the timeline mutex.
Matt
> John.
>
> > intel_context_timeline_unlock(tl);
>
Re: [PATCH] drm/i915: Check return intel_context_timeline_lock of in eb_pin_timeline
On 1/4/2022 09:31, Matthew Brost wrote:
intel_context_timeline_lock can return can error if interrupted by a
user when trying to lock the timeline mutex. Check the return value of
intel_context_timeline_lock in eb_pin_timeline (execbuf IOCTL).
Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf")
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index e9541244027a..65a078945b00 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2517,6 +2517,9 @@ static int eb_pin_timeline(struct i915_execbuffer *eb,
struct intel_context *ce,
i915_request_put(rq);
tl = intel_context_timeline_lock(ce);
+ if (IS_ERR(tl))
+ return PTR_ERR(tl);
+
intel_context_exit(ce);
Won't this leak the stuff acquired by the intel_context_enter() above if
the _exit() is now skipped?
John.
intel_context_timeline_unlock(tl);
[PATCH v10 2/6] drm/i915: Use to_gt() helper for GGTT accesses
From: Michał Winiarski
GGTT is currently available both through i915->ggtt and gt->ggtt, and we
eventually want to get rid of the i915->ggtt one.
Use to_gt() for all i915->ggtt accesses to help with the future
refactoring.
Signed-off-by: Michał Winiarski
Cc: Michal Wajdeczko
Signed-off-by: Andi Shyti
---
Hi Matt,
I'm sending this this v10 as reply-to, if it confuses you I can
send the whole v10.
The only trivial difference is that in i915_perf.c I'm using
to_gt(perf->i915)
instead of
to_gt(perf->i915)->ggtt->vm.gt
Andi
drivers/gpu/drm/i915/gvt/dmabuf.c| 2 +-
drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
drivers/gpu/drm/i915/i915_driver.c | 4 ++--
drivers/gpu/drm/i915/i915_drv.h | 2 +-
drivers/gpu/drm/i915/i915_gem.c | 23 ---
drivers/gpu/drm/i915/i915_gem_gtt.c | 6 +++---
drivers/gpu/drm/i915/i915_getparam.c | 2 +-
drivers/gpu/drm/i915/i915_perf.c | 6 +++---
8 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c
b/drivers/gpu/drm/i915/gvt/dmabuf.c
index 8e65cd8258b9..94c3eb1586b0 100644
--- a/drivers/gpu/drm/i915/gvt/dmabuf.c
+++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
@@ -84,7 +84,7 @@ static int vgpu_gem_get_pages(
kfree(st);
return ret;
}
- gtt_entries = (gen8_pte_t __iomem *)dev_priv->ggtt.gsm +
+ gtt_entries = (gen8_pte_t __iomem *)to_gt(dev_priv)->ggtt->gsm +
(fb_info->start >> PAGE_SHIFT);
for_each_sg(st->sgl, sg, page_num, i) {
dma_addr_t dma_addr =
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index e0e052cdf8b8..6966fe08df92 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -390,9 +390,9 @@ static int i915_swizzle_info(struct seq_file *m, void *data)
intel_wakeref_t wakeref;
seq_printf(m, "bit6 swizzle for X-tiling = %s\n",
- swizzle_string(dev_priv->ggtt.bit_6_swizzle_x));
+ swizzle_string(to_gt(dev_priv)->ggtt->bit_6_swizzle_x));
seq_printf(m, "bit6 swizzle for Y-tiling = %s\n",
- swizzle_string(dev_priv->ggtt.bit_6_swizzle_y));
+ swizzle_string(to_gt(dev_priv)->ggtt->bit_6_swizzle_y));
if (dev_priv->quirks & QUIRK_PIN_SWIZZLED_PAGES)
seq_puts(m, "L-shaped memory detected\n");
diff --git a/drivers/gpu/drm/i915/i915_driver.c
b/drivers/gpu/drm/i915/i915_driver.c
index 60f8cbf24de7..3c984553d86f 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1146,7 +1146,7 @@ static int i915_drm_suspend(struct drm_device *dev)
/* Must be called before GGTT is suspended. */
intel_dpt_suspend(dev_priv);
- i915_ggtt_suspend(_priv->ggtt);
+ i915_ggtt_suspend(to_gt(dev_priv)->ggtt);
i915_save_display(dev_priv);
@@ -1270,7 +1270,7 @@ static int i915_drm_resume(struct drm_device *dev)
if (ret)
drm_err(_priv->drm, "failed to re-enable GGTT\n");
- i915_ggtt_resume(_priv->ggtt);
+ i915_ggtt_resume(to_gt(dev_priv)->ggtt);
/* Must be called after GGTT is resumed. */
intel_dpt_resume(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index beeb42a14aae..01518ce8f401 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1748,7 +1748,7 @@ static inline bool
i915_gem_object_needs_bit17_swizzle(struct drm_i915_gem_objec
{
struct drm_i915_private *i915 = to_i915(obj->base.dev);
- return i915->ggtt.bit_6_swizzle_x == I915_BIT_6_SWIZZLE_9_10_17 &&
+ return to_gt(i915)->ggtt->bit_6_swizzle_x == I915_BIT_6_SWIZZLE_9_10_17
&&
i915_gem_object_is_tiled(obj);
}
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 915bf431f320..e3730096abd9 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -88,7 +88,8 @@ int
i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
struct drm_file *file)
{
- struct i915_ggtt *ggtt = _i915(dev)->ggtt;
+ struct drm_i915_private *i915 = to_i915(dev);
+ struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
struct drm_i915_gem_get_aperture *args = data;
struct i915_vma *vma;
u64 pinned;
@@ -289,7 +290,7 @@ static struct i915_vma *i915_gem_gtt_prepare(struct
drm_i915_gem_object *obj,
bool write)
{
struct drm_i915_private *i915 = to_i915(obj->base.dev);
- struct i915_ggtt *ggtt = >ggtt;
+ struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
struct i915_vma *vma;
struct i915_gem_ww_ctx ww;
int ret;
@@ -350,7 +351,7 @@ static void i915_gem_gtt_cleanup(struct drm_i915_gem_object
*obj,
struct i915_vma *vma)
{
Re: [PATCH v9 2/6] drm/i915: Use to_gt() helper for GGTT accesses
Hi guys,
> > > > > struct drm_i915_gem_object *bo;
> > > > > struct i915_vma *vma;
> > > > > const u64 delay_ticks = 0x -
> > > > > -
> > > > > intel_gt_ns_to_clock_interval(stream->perf->i915->ggtt.vm.gt,
> > > > > +
> > > > > intel_gt_ns_to_clock_interval(to_gt(stream->perf->i915)->ggtt->vm.gt,
> > > >
> > > > I'm not too familiar with the perf code, but this looks a bit roundabout
> > > > since we're ultimately trying to get to a GT...do we even need to go
> > > > through the ggtt structure here or can we just pass
> > > > "to_gt(stream->perf->i915)" as the first parameter?
> > > >
> > > > >
> > > > > atomic64_read(>perf->noa_programming_delay));
> > > > > const u32 base = stream->engine->mmio_base;
> > > > > #define CS_GPR(x) GEN8_RING_CS_GPR(base, x)
> > > > > @@ -3542,7 +3542,7 @@ i915_perf_open_ioctl_locked(struct i915_perf
> > > > > *perf,
> > > > >
> > > > > static u64 oa_exponent_to_ns(struct i915_perf *perf, int exponent)
> > > > > {
> > > > > - return intel_gt_clock_interval_to_ns(perf->i915->ggtt.vm.gt,
> > > > > + return
> > > > > intel_gt_clock_interval_to_ns(to_gt(perf->i915)->ggtt->vm.gt,
> > > >
> > > > Ditto; this looks like "to_gt(perf->i915)" might be all we need?
> > >
> > > I think this function is looking for the GT coming from the VM,
> > > otherwise originally it could have taken it from >gt. In my
> > > first version I proposed a wrapper around this but it was
> > > rejected by Lucas.
> > >
> > > Besides, as we discussed earlier when I was proposed the static
> > > allocation, the ggtt might not always be linked to the same gt,
> > > so that I assumed that sometimes:
> > >
> > >to_gt(perf->i915)->ggtt->vm.gt != to_gt(perf->i915)
> > >
> > > if two GTs are sharing the same ggtt, what would the ggtt->vm.gt
> > > link be?
> >
> > From the git history, it doesn't look like this really needs to care
> > about the GGTT at all; I think it was just unintentionally written in a
> > roundabout manner when intel_gt was first being introduced in the code.
> > The reference here first showed up in commit f170523a7b8e ("drm/i915/gt:
> > Consolidate the CS timestamp clocks").
> >
> > Actually the most correct thing to do is probably to use
> > 'stream->engine->gt' to ensure we grab the GT actually associated with
> > the stream's engine.
> >
>
> stream is not yet created at this point, so I would do this:
>
> pass intel_gt to the helper instead of perf:
> static u64 oa_exponent_to_ns(struct intel_gt *gt, int exponent)
> {
> return intel_gt_clock_interval_to_ns(gt, 2ULL << exponent);
> }
>
> caller would then be:
> oa_period = oa_exponent_to_ns(props->engine->gt, value);
thanks for the suggestions, but this is out of the scope of this
patch... I did propose a wrapper but it was rejected because it
was, indeed, out of scope.
I'm going to use to_gt(perf->i915) as Matt suggested originally,
patch is ready.
Thanks,
Andi
Re: [PATCH] Discussion: dt-bindings: display: msm: dsi-controller-main: fix the binding
On Sun, Dec 26, 2021 at 02:34:08AM +0300, Dmitry Baryshkov wrote:
> Hi,
>
> On Sat, 25 Dec 2021 at 23:54, David Heidelberg wrote:
> >
> > This binding is not much validating the old DSI v2.
> >
> > Currently we don't differentiate old v2 from new versions,
> > so we need to figure out how to validate them.
> >
> > I propose specific compatible depending on mdss version, but I would be
> > glad, if someone with deeper knowledge proposed the names.
> >
> > I'm willing to implement it then and back from autodetection.
>
> I'd suggest to use hardware-specific compatible for apq8064 (and maybe
> other v2 hosts if somebody adds support). For example
> "qcom,apq8064-dsi-ctrl" or "qcom,dsi-ctrl-apq8064" (no strong
> preference here).
The former.
> For 6G hosts it will probably make sense to use IP versions instead
> ("qcom-dsi-ctrl-6g-v2.4.1").
Humm, we went down the path of version numbers for QCom blocks, but the
result was not much reuse of same version on more than 2-3 parts if
that. So stick with SoCs for naming unless there's a strong case that
version numbers to SoC parts is 1 to many.
Rob
Re: [i-g-t 00/14] Add IGT support for plane color management
On 2022-01-02 23:11, Modem, Bhanuprakash wrote:
>> From: Harry Wentland
>> Sent: Monday, November 29, 2021 8:50 PM
>> To: Pekka Paalanen
>> Cc: dri-devel@lists.freedesktop.org; Modem, Bhanuprakash
>> ; igt-...@lists.freedesktop.org; Kumar,
>> Mukunda Pramodh ; Juha-Pekka Heikkila
>> ; Shankar, Uma
>> Subject: Re: [i-g-t 00/14] Add IGT support for plane color management
>>
>> On 2021-11-29 04:20, Pekka Paalanen wrote:
>>> On Fri, 26 Nov 2021 11:54:55 -0500
>>> Harry Wentland wrote:
>>>
On 2021-11-18 04:50, Pekka Paalanen wrote:
> On Mon, 15 Nov 2021 15:17:45 +0530
> Bhanuprakash Modem wrote:
>
>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fseries%2F90825%2Fdata=04%7C01%7Charry.wentland%40amd.com%7C6d85b55209204b9b1e6108d9ce6f30f6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637767799222764784%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=j%2BkGUD45bTIke%2FIeCO7GGAM1n%2FCrF2oy6CKfLc6jhzk%3Dreserved=0
>>>
>>> ...
>>>
> I also found some things that looked hardware-specific in this code
> that to my understanding is supposed to be generic, and some concerns
> about UAPI as well.
>
I left some comments on intellisms in these patches.
Do you have any specifics about your concerns about UAPI?
>>>
>>> Yeah, the comments I left in the patches:
>>>
>>> patch 3:
>>>
Having to explicitly special-case index zero feels odd to me. Why does
it need explicit special-casing?
To me it's a hint that the definitions of .start and .end are somehow
inconsistent.
>>>
>>> patch 4 and 8:
>>>
> +static bool is_hdr_plane(const igt_plane_t *plane)
> +{
> + return plane->index >= 0 && plane->index < SDR_PLANE_BASE;
How can this be right for all KMS drivers?
What is a HDR plane?
>>>
>>> patch 12:
>>>
> +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. */
In whose hardware?
Are igt tests not supposed to be generic for everything that exposes
the particular KMS properties?
This also hints that the UAPI design is lacking, because userspace
needs to know hardware specific things out of thin air. Display servers
are not going to have hardware-specific code. They specialise based on
the existence of KMS properties instead.
>>>
>>> ...
>>>
> +void set_advance_gamma(data_t *data, igt_pipe_t *pipe, enum gamma_type
>> type)
> +{
> + igt_display_t *display = >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_MODES, 1);
Is this how we are going to do cross-software DRM-master hand-over or
switching compatibility in general?
Add a new client cap for every new KMS property, and if the KMS client
does not set the property, the kernel will magically reset it to ensure
the client's expectations are met? Is that how it works?
Or why does this exist?
>>>
>>> ...
>>>
> + drmSetClientCap(data->drm_fd, DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES, 0);
I've never seen this done before. I did not know client caps could be
reset.
>>>
>>>
>>> So, patch 12 has the biggest UAPI questions, and patch 3 may need a
>>> UAPI change as well. The comments in patches 4 and 8 could be addressed
>>> with just removing that code since the concept of HDR/SDR planes does
>>> not exist in UAPI. Or, if that concept is needed then it's another UAPI
>>> problem.
>>>
>>
>> Thanks for reiterating your points. I missed your earlier replies and
>> found them in my IGT folder just now.
>>
>> Looks like we're on the same page.
>
> Thanks Pekka & Harry for the review. Apologies for late response. I thought
> that everyone is in holidays . Now I am re-igniting this discussion.
>
No worries.
> I have gone through all review comments and it's make sense to remove hardware
> specific stuff from the helper functions.
>
> Patch 3:
> Intel hardware is expecting first LUT value as 0, still we can remove this
> logic
>
Re: [i-g-t 04/14] tests/kms_color: New subtests for Plane gamma
On 2022-01-02 23:05, Modem, Bhanuprakash wrote:
>> From: Harry Wentland
>> Sent: Friday, November 26, 2021 10:25 PM
>> To: Modem, Bhanuprakash ; igt-
>> d...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
>> Cc: Ville Syrjälä ; Juha-Pekka Heikkila
>> ; Shankar, Uma
>> Subject: Re: [i-g-t 04/14] tests/kms_color: New subtests for Plane gamma
>>
>> On 2021-11-15 04:47, Bhanuprakash Modem wrote:
>>> 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
>>> +*
>>> +*/
>>
>> This seems to be about Intel HW. AMD HW for example does
>> not have HDR or SDR planes, only one generic plane.
>>
>>> + 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 = >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");
>>> +
>>
>> is_hdr_plane is Intel-specific.
>>
>>> + 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,
>>> + ));
>>> + igt_plane_set_fb(plane, );
>>> +
>>> + /* 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;
>>> +
>>
>> It might still make sense to test that this is doing bypass.
>
> As we know gamma_mode->enum[i].name represents the name of the
Re: [PATCH] dt-bindings: display: enable port jdi,lt070me05000
On Fri, 24 Dec 2021 20:53:54 +0100, David Heidelberg wrote: > Enable port inside panel bindings. > > Fixes warnings generated by `make qcom-apq8064-asus-nexus7-flo.dtb` as: > arch/arm/boot/dts/qcom-apq8064-asus-nexus7-flo.dt.yaml: panel@0: 'port' does > not match any of the regexes: 'pinctrl-[0-9]+' > From schema: > Documentation/devicetree/bindings/display/panel/jdi,lt070me05000.yaml > > Signed-off-by: David Heidelberg > --- > .../devicetree/bindings/display/panel/jdi,lt070me05000.yaml | 2 ++ > 1 file changed, 2 insertions(+) > Applied, thanks!
Re: [PATCH] dt-bindings: display/msm: hdmi: split and convert to yaml
On Fri, Dec 24, 2021 at 05:24:57PM +0100, David Heidelberg wrote:
> Convert Qualcomm HDMI binding into HDMI TX and PHY yaml bindings.
>
> Other changes:
> - fixed reg-names numbering to match 0..3 instead 0,1,3,4
>
> Signed-off-by: David Heidelberg
> ---
> .../devicetree/bindings/display/msm/hdmi.txt | 99 -
> .../bindings/display/msm/qcom,hdmi-phy.yaml | 119 +++
PHYs go in bindings/phy/
> .../bindings/display/msm/qcom,hdmi.yaml | 201 ++
> 3 files changed, 320 insertions(+), 99 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/display/msm/hdmi.txt
> create mode 100644
> Documentation/devicetree/bindings/display/msm/qcom,hdmi-phy.yaml
> create mode 100644
> Documentation/devicetree/bindings/display/msm/qcom,hdmi.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/msm/hdmi.txt
> b/Documentation/devicetree/bindings/display/msm/hdmi.txt
> deleted file mode 100644
> index 5f90a40da51b..
> --- a/Documentation/devicetree/bindings/display/msm/hdmi.txt
> +++ /dev/null
> @@ -1,99 +0,0 @@
> -Qualcomm adreno/snapdragon hdmi output
> -
> -Required properties:
> -- compatible: one of the following
> - * "qcom,hdmi-tx-8996"
> - * "qcom,hdmi-tx-8994"
> - * "qcom,hdmi-tx-8084"
> - * "qcom,hdmi-tx-8974"
> - * "qcom,hdmi-tx-8660"
> - * "qcom,hdmi-tx-8960"
> -- reg: Physical base address and length of the controller's registers
> -- reg-names: "core_physical"
> -- interrupts: The interrupt signal from the hdmi block.
> -- power-domains: Should be < MDSS_GDSC>.
> -- clocks: device clocks
> - See ../clocks/clock-bindings.txt for details.
> -- core-vdda-supply: phandle to supply regulator
> -- hdmi-mux-supply: phandle to mux regulator
> -- phys: the phandle for the HDMI PHY device
> -- phy-names: the name of the corresponding PHY device
> -
> -Optional properties:
> -- hpd-gpios: hpd pin
> -- qcom,hdmi-tx-mux-en-gpios: hdmi mux enable pin
> -- qcom,hdmi-tx-mux-sel-gpios: hdmi mux select pin
> -- qcom,hdmi-tx-mux-lpm-gpios: hdmi mux lpm pin
> -- power-domains: reference to the power domain(s), if available.
> -- pinctrl-names: the pin control state names; should contain "default"
> -- pinctrl-0: the default pinctrl state (active)
> -- pinctrl-1: the "sleep" pinctrl state
> -
> -HDMI PHY:
> -Required properties:
> -- compatible: Could be the following
> - * "qcom,hdmi-phy-8660"
> - * "qcom,hdmi-phy-8960"
> - * "qcom,hdmi-phy-8974"
> - * "qcom,hdmi-phy-8084"
> - * "qcom,hdmi-phy-8996"
> -- #phy-cells: Number of cells in a PHY specifier; Should be 0.
> -- reg: Physical base address and length of the registers of the PHY sub
> blocks.
> -- reg-names: The names of register regions. The following regions are
> required:
> - * "hdmi_phy"
> - * "hdmi_pll"
> - For HDMI PHY on msm8996, these additional register regions are required:
> -* "hdmi_tx_l0"
> -* "hdmi_tx_l1"
> -* "hdmi_tx_l3"
> -* "hdmi_tx_l4"
> -- power-domains: Should be < MDSS_GDSC>.
> -- clocks: device clocks
> - See Documentation/devicetree/bindings/clock/clock-bindings.txt for details.
> -- core-vdda-supply: phandle to vdda regulator device node
> -
> -Example:
> -
> -/ {
> - ...
> -
> - hdmi: hdmi@4a0 {
> - compatible = "qcom,hdmi-tx-8960";
> - reg-names = "core_physical";
> - reg = <0x04a0 0x2f0>;
> - interrupts = ;
> - power-domains = < MDSS_GDSC>;
> - clock-names =
> - "core",
> - "master_iface",
> - "slave_iface";
> - clocks =
> - < HDMI_APP_CLK>,
> - < HDMI_M_AHB_CLK>,
> - < HDMI_S_AHB_CLK>;
> - qcom,hdmi-tx-ddc-clk = < 70 GPIO_ACTIVE_HIGH>;
> - qcom,hdmi-tx-ddc-data = < 71 GPIO_ACTIVE_HIGH>;
> - qcom,hdmi-tx-hpd = < 72 GPIO_ACTIVE_HIGH>;
> - core-vdda-supply = <_hdmi_mvs>;
> - hdmi-mux-supply = <_3p3v>;
> - pinctrl-names = "default", "sleep";
> - pinctrl-0 = <_active _active _active>;
> - pinctrl-1 = <_suspend _suspend _suspend>;
> -
> - phys = <_phy>;
> - phy-names = "hdmi_phy";
> - };
> -
> - hdmi_phy: phy@4a00400 {
> - compatible = "qcom,hdmi-phy-8960";
> - reg-names = "hdmi_phy",
> - "hdmi_pll";
> - reg = <0x4a00400 0x60>,
> - <0x4a00500 0x100>;
> - #phy-cells = <0>;
> - power-domains = < MDSS_GDSC>;
> - clock-names = "slave_iface";
> - clocks = < HDMI_S_AHB_CLK>;
> - core-vdda-supply = <_hdmi_mvs>;
> - };
> -};
> diff --git a/Documentation/devicetree/bindings/display/msm/qcom,hdmi-phy.yaml
> b/Documentation/devicetree/bindings/display/msm/qcom,hdmi-phy.yaml
> new file mode 100644
> index ..be08fc767435
> --- /dev/null
> +++
Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV
On 2022-01-04 12:13 p.m., Liu, Shaoyun wrote: [AMD Official Use Only] I mostly agree with the sequences Christian described . Just one thing might need to discuss here. For FLR notified from host, in new sequenceas described , driver need to reply the READY_TO_RESET in the workitem from a reset work queue which means inside flr_work, driver can not directly reply to host but need to queue another workqueue . Can you clarify why 'driver can not directly reply to host but need to queue another workqueue' ? To my understating all steps 3-6 in Christian's description happen from the same single wq thread serially. For current code , the flr_work for sriov itself is a work queue queued from ISR . I think we should try to response to the host driver as soon as possible. Queue another workqueue inside the workqueue doesn't sounds efficient to me. Check patch 5 please [1] - I just substituted schedule_work(>virt.flr_work) for queue_work(adev->reset_domain.wq,>virt.flr_work) so no extra requeue here, just instead of sending to system_wq it's sent to dedicated reset wq [1] - https://lore.kernel.org/all/2021121400.790842-1-andrey.grodzov...@amd.com/ Andrey Anyway, what we need is a working solution for our project. So if we need to change the sequence, we need to make sure it's been tested first and won't break the functionality before the code is landed in the branch . Regards Shaoyun.liu -Original Message- From: amd-gfx On Behalf Of Christian König Sent: Tuesday, January 4, 2022 6:36 AM To: Liu, Monk ; Chen, JingWen ; Christian König ; Grodzovsky, Andrey ; Deng, Emily ; dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; Chen, Horace Cc: dan...@ffwll.ch Subject: Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV Am 04.01.22 um 11:49 schrieb Liu, Monk: [AMD Official Use Only] See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. No it's not, FLR from hypervisor is just to notify guest the hw VF FLR is about to start or was already executed, but host will do FLR anyway without waiting for guest too long Then we have a major design issue in the SRIOV protocol and really need to question this. How do you want to prevent a race between the hypervisor resetting the hardware and the client trying the same because of a timeout? As far as I can see the procedure should be: 1. We detect that a reset is necessary, either because of a fault a timeout or signal from hypervisor. 2. For each of those potential reset sources a work item is send to the single workqueue. 3. One of those work items execute first and prepares the reset. 4. We either do the reset our self or notify the hypervisor that we are ready for the reset. 5. Cleanup after the reset, eventually resubmit jobs etc.. 6. Cancel work items which might have been scheduled from other reset sources. It does make sense that the hypervisor resets the hardware without waiting for the clients for too long, but if we don't follow this general steps we will always have a race between the different components. Regards, Christian. Am 04.01.22 um 11:49 schrieb Liu, Monk: [AMD Official Use Only] See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. No it's not, FLR from hypervisor is just to notify guest the hw VF FLR is about to start or was already executed, but host will do FLR anyway without waiting for guest too long In other words I strongly think that the current SRIOV reset implementation is severely broken and what Andrey is doing is actually fixing it. It makes the code to crash ... how could it be a fix ? I'm afraid the patch is NAK from me, but it is welcome if the cleanup do not ruin the logic, Andry or jingwen can try it if needed. Thanks --- Monk Liu | Cloud GPU & Virtualization Solution | AMD --- we are hiring software manager for CVS core team --- -Original Message- From: Koenig, Christian Sent: Tuesday, January 4, 2022 6:19 PM To: Chen, JingWen ; Christian König ; Grodzovsky, Andrey ; Deng, Emily ; Liu, Monk ; dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; Chen, Horace ; Chen, JingWen Cc: dan...@ffwll.ch Subject: Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV Hi Jingwen, well what I mean is that we need to adjust the implementation in amdgpu to actually match the requirements. Could be that the reset sequence is questionable in general,
Re: [PATCH 3/3] dt-bindings: display: bridge: renesas, lvds: Document r8a77961 bindings
On Fri, 24 Dec 2021 08:23:09 +0300, Nikita Yushchenko wrote: > Document the R-Car M3-W+ (R8A77961) SoC in the R-Car LVDS encoder > bindings. > > Signed-off-by: Nikita Yushchenko > --- > .../devicetree/bindings/display/bridge/renesas,lvds.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring
Re: [PATCH] dt-bindings: display: simple: Add Multi-Inno Technology MI0700S4T-6 panel
On Wed, 22 Dec 2021 14:32:00 +0100, Marek Vasut wrote: > Add Multi-Inno Technology MI0700S4T-6 7" 800x480 DPI panel > compatible string. > > Signed-off-by: Marek Vasut > Cc: Rob Herring > Cc: Sam Ravnborg > Cc: devicet...@vger.kernel.org > To: dri-devel@lists.freedesktop.org > --- > .../devicetree/bindings/display/panel/panel-simple.yaml | 2 ++ > 1 file changed, 2 insertions(+) > Acked-by: Rob Herring
Re: [PATCH] dt-bindings: display: st,stm32-dsi: Fix panel node name in example
On Tue, 21 Dec 2021 08:51:45 -0400, Rob Herring wrote:
> With 'unevaluatedProperties' support enabled, the st,stm32-dsi binding
> has a new warning:
>
> Documentation/devicetree/bindings/display/st,stm32-dsi.example.dt.yaml:
> dsi@5a00: Unevaluated properties are not allowed ('panel-dsi@0' was
> unexpected)
>
> The documented child node name is 'panel', so update the example.
>
> Signed-off-by: Rob Herring
> ---
> Documentation/devicetree/bindings/display/st,stm32-dsi.yaml | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
Applied, thanks!
[PATCH 2/2] drm/mst: use DP_GET_SINK_COUNT() for sink count in ESI
Take bit 7 into account when reading sink count from
DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0.
Signed-off-by: Jani Nikula
---
drivers/gpu/drm/drm_dp_mst_topology.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
b/drivers/gpu/drm/drm_dp_mst_topology.c
index f3d79eda94bb..ab4372e9fe43 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -4196,7 +4196,7 @@ int drm_dp_mst_hpd_irq(struct drm_dp_mst_topology_mgr
*mgr, u8 *esi, bool *handl
int ret = 0;
int sc;
*handled = false;
- sc = esi[0] & 0x3f;
+ sc = DP_GET_SINK_COUNT(esi[0]);
if (sc != mgr->sink_count) {
mgr->sink_count = sc;
--
2.30.2
[PATCH 1/2] drm/dp: note that DPCD 0x2002-0x2003 match 0x200-0x201
DP_SINK_COUNT_ESI and DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0 have the same contents as DP_SINK_COUNT and DP_DEVICE_SERVICE_IRQ_VECTOR, respectively. Signed-off-by: Jani Nikula --- include/drm/drm_dp_helper.h | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index 30359e434c3f..98d020835b49 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -1038,11 +1038,8 @@ struct drm_panel; #define DP_SIDEBAND_MSG_UP_REQ_BASE0x1600 /* 1.2 MST */ /* DPRX Event Status Indicator */ -#define DP_SINK_COUNT_ESI 0x2002 /* 1.2 */ -/* 0-5 sink count */ -# define DP_SINK_COUNT_CP_READY (1 << 6) - -#define DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0 0x2003 /* 1.2 */ +#define DP_SINK_COUNT_ESI 0x2002 /* same as 0x200 */ +#define DP_DEVICE_SERVICE_IRQ_VECTOR_ESI0 0x2003 /* same as 0x201 */ #define DP_DEVICE_SERVICE_IRQ_VECTOR_ESI1 0x2004 /* 1.2 */ # define DP_RX_GTC_MSTR_REQ_STATUS_CHANGE(1 << 0) -- 2.30.2
Re: [PATCH] drm/ttm: Don't inherit GEM object VMAs in child process
[+Adrian] Am 2021-12-23 um 2:05 a.m. schrieb Christian König: > Am 22.12.21 um 21:53 schrieb Daniel Vetter: >> On Mon, Dec 20, 2021 at 01:12:51PM -0500, Bhardwaj, Rajneesh wrote: >> >> [SNIP] >> Still sounds funky. I think minimally we should have an ack from CRIU >> developers that this is officially the right way to solve this >> problem. I >> really don't want to have random one-off hacks that don't work across >> the >> board, for a problem where we (drm subsystem) really shouldn't be the >> only >> one with this problem. Where "this problem" means that the mmap space is >> per file description, and not per underlying inode or real device or >> whatever. That part sounds like a CRIU problem, and I expect CRIU folks >> want a consistent solution across the board for this. Hence please >> grab an >> ack from them. > > Unfortunately it's a KFD design problem. AMD used a single device > node, then mmaped different objects from the same offset to different > processes and expected it to work the rest of the fs subsystem without > churn. This may be true for mmaps in the KFD device, but not for mmaps in the DRM render nodes. > > So yes, this is indeed because the mmap space is per file descriptor > for the use case here. No. This is a different problem. The problem has to do with the way that DRM manages mmap permissions. In order to be able to mmap an offset in the render node, there needs to be a BO that was created in the same render node. If you fork a process, it inherits the VMA. But KFD doesn't know anything about the inherited BOs from the parent process. Therefore those BOs don't get checkpointed and restored in the child process. When the CRIU checkpoint is restored, our CRIU plugin never creates a BO corresponding to the VMA in the child process' render node FD. We've also lost the relationship between the parent and child-process' render node FDs. After "fork" the render node FD points to the same struct file in parent and child. After restoring the CRIU checkpoint, they are separate struct files, created by separate "open" system calls. Therefore the mmap call that restores the VMA fails in the child process. At least for KFD, there is no point inheriting BOs from a child process, because the GPU has no way of accessing the BOs in the child process. The child process has no GPU address space, no user mode queues, no way to do anything with the GPU before it completely reinitializes its KFD context. We can workaround this issue in user mode with madvise(..., MADV_DONTFORK). In fact we've already done this for some BOs to avoid a memory leak in the parent process while a child process exists. But it's slightly racy because there is a short time window where VMA exists without the VM_DONTCOPY flag. A fork during that time window could still create a child process with an inherited VMA. Therefore a safer solution is to set the vm_flags in the VMA in the driver when the VMA is first created. Regards, Felix > > And thanks for pointing this out, this indeed makes the whole change > extremely questionable. > > Regards, > Christian. > >> >> Cheers, Daniel >> >
Re: [PATCH v9 2/6] drm/i915: Use to_gt() helper for GGTT accesses
On Mon, Jan 03, 2022 at 01:17:10PM -0800, Matt Roper wrote:
On Tue, Dec 21, 2021 at 09:46:29PM +0200, Andi Shyti wrote:
Hi Matt,
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c
b/drivers/gpu/drm/i915/i915_perf.c
> > index 170bba913c30..128315aec517 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -1630,7 +1630,7 @@ static int alloc_noa_wait(struct i915_perf_stream
*stream)
> > struct drm_i915_gem_object *bo;
> > struct i915_vma *vma;
> > const u64 delay_ticks = 0x -
> > - intel_gt_ns_to_clock_interval(stream->perf->i915->ggtt.vm.gt,
> > +
intel_gt_ns_to_clock_interval(to_gt(stream->perf->i915)->ggtt->vm.gt,
>
> I'm not too familiar with the perf code, but this looks a bit roundabout
> since we're ultimately trying to get to a GT...do we even need to go
> through the ggtt structure here or can we just pass
> "to_gt(stream->perf->i915)" as the first parameter?
>
> >
atomic64_read(>perf->noa_programming_delay));
> > const u32 base = stream->engine->mmio_base;
> > #define CS_GPR(x) GEN8_RING_CS_GPR(base, x)
> > @@ -3542,7 +3542,7 @@ i915_perf_open_ioctl_locked(struct i915_perf *perf,
> >
> > static u64 oa_exponent_to_ns(struct i915_perf *perf, int exponent)
> > {
> > - return intel_gt_clock_interval_to_ns(perf->i915->ggtt.vm.gt,
> > + return intel_gt_clock_interval_to_ns(to_gt(perf->i915)->ggtt->vm.gt,
>
> Ditto; this looks like "to_gt(perf->i915)" might be all we need?
I think this function is looking for the GT coming from the VM,
otherwise originally it could have taken it from >gt. In my
first version I proposed a wrapper around this but it was
rejected by Lucas.
Besides, as we discussed earlier when I was proposed the static
allocation, the ggtt might not always be linked to the same gt,
so that I assumed that sometimes:
to_gt(perf->i915)->ggtt->vm.gt != to_gt(perf->i915)
if two GTs are sharing the same ggtt, what would the ggtt->vm.gt
link be?
From the git history, it doesn't look like this really needs to care
about the GGTT at all; I think it was just unintentionally written in a
roundabout manner when intel_gt was first being introduced in the code.
The reference here first showed up in commit f170523a7b8e ("drm/i915/gt:
Consolidate the CS timestamp clocks").
Actually the most correct thing to do is probably to use
'stream->engine->gt' to ensure we grab the GT actually associated with
the stream's engine.
stream is not yet created at this point, so I would do this:
pass intel_gt to the helper instead of perf:
static u64 oa_exponent_to_ns(struct intel_gt *gt, int exponent)
{
return intel_gt_clock_interval_to_ns(gt, 2ULL << exponent);
}
caller would then be:
oa_period = oa_exponent_to_ns(props->engine->gt, value);
Thanks,
Umesh
Matt
Thanks,
Andi
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
[PATCH] drm/i915: Check return intel_context_timeline_lock of in eb_pin_timeline
intel_context_timeline_lock can return can error if interrupted by a
user when trying to lock the timeline mutex. Check the return value of
intel_context_timeline_lock in eb_pin_timeline (execbuf IOCTL).
Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf")
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index e9541244027a..65a078945b00 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2517,6 +2517,9 @@ static int eb_pin_timeline(struct i915_execbuffer *eb,
struct intel_context *ce,
i915_request_put(rq);
tl = intel_context_timeline_lock(ce);
+ if (IS_ERR(tl))
+ return PTR_ERR(tl);
+
intel_context_exit(ce);
intel_context_timeline_unlock(tl);
--
2.34.1
RE: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV
[AMD Official Use Only] I mostly agree with the sequences Christian described . Just one thing might need to discuss here. For FLR notified from host, in new sequenceas described , driver need to reply the READY_TO_RESET in the workitem from a reset work queue which means inside flr_work, driver can not directly reply to host but need to queue another workqueue . For current code , the flr_work for sriov itself is a work queue queued from ISR . I think we should try to response to the host driver as soon as possible. Queue another workqueue inside the workqueue doesn't sounds efficient to me. Anyway, what we need is a working solution for our project. So if we need to change the sequence, we need to make sure it's been tested first and won't break the functionality before the code is landed in the branch . Regards Shaoyun.liu -Original Message- From: amd-gfx On Behalf Of Christian König Sent: Tuesday, January 4, 2022 6:36 AM To: Liu, Monk ; Chen, JingWen ; Christian König ; Grodzovsky, Andrey ; Deng, Emily ; dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; Chen, Horace Cc: dan...@ffwll.ch Subject: Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV Am 04.01.22 um 11:49 schrieb Liu, Monk: > [AMD Official Use Only] > >>> See the FLR request from the hypervisor is just another source of signaling >>> the need for a reset, similar to each job timeout on each queue. Otherwise >>> you have a race condition between the hypervisor and the scheduler. > No it's not, FLR from hypervisor is just to notify guest the hw VF FLR > is about to start or was already executed, but host will do FLR anyway > without waiting for guest too long > Then we have a major design issue in the SRIOV protocol and really need to question this. How do you want to prevent a race between the hypervisor resetting the hardware and the client trying the same because of a timeout? As far as I can see the procedure should be: 1. We detect that a reset is necessary, either because of a fault a timeout or signal from hypervisor. 2. For each of those potential reset sources a work item is send to the single workqueue. 3. One of those work items execute first and prepares the reset. 4. We either do the reset our self or notify the hypervisor that we are ready for the reset. 5. Cleanup after the reset, eventually resubmit jobs etc.. 6. Cancel work items which might have been scheduled from other reset sources. It does make sense that the hypervisor resets the hardware without waiting for the clients for too long, but if we don't follow this general steps we will always have a race between the different components. Regards, Christian. Am 04.01.22 um 11:49 schrieb Liu, Monk: > [AMD Official Use Only] > >>> See the FLR request from the hypervisor is just another source of signaling >>> the need for a reset, similar to each job timeout on each queue. Otherwise >>> you have a race condition between the hypervisor and the scheduler. > No it's not, FLR from hypervisor is just to notify guest the hw VF FLR > is about to start or was already executed, but host will do FLR anyway > without waiting for guest too long > >>> In other words I strongly think that the current SRIOV reset implementation >>> is severely broken and what Andrey is doing is actually fixing it. > It makes the code to crash ... how could it be a fix ? > > I'm afraid the patch is NAK from me, but it is welcome if the cleanup do not > ruin the logic, Andry or jingwen can try it if needed. > > Thanks > --- > Monk Liu | Cloud GPU & Virtualization Solution | AMD > --- > we are hiring software manager for CVS core team > --- > > -Original Message- > From: Koenig, Christian > Sent: Tuesday, January 4, 2022 6:19 PM > To: Chen, JingWen ; Christian König > ; Grodzovsky, Andrey > ; Deng, Emily ; Liu, > Monk ; dri-devel@lists.freedesktop.org; > amd-...@lists.freedesktop.org; Chen, Horace ; > Chen, JingWen > Cc: dan...@ffwll.ch > Subject: Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset > protection for SRIOV > > Hi Jingwen, > > well what I mean is that we need to adjust the implementation in amdgpu to > actually match the requirements. > > Could be that the reset sequence is questionable in general, but I doubt so > at least for now. > > See the FLR request from the hypervisor is just another source of signaling > the need for a reset, similar to each job timeout on each queue. Otherwise > you have a race condition between the hypervisor and the scheduler. > > Properly setting in_gpu_reset is indeed mandatory, but should happen at a > central place and not in the SRIOV specific code. > > In other words I strongly think that the current SRIOV reset implementation > is
Re: [PATCH] drm/bridge/tc358775: Fix for dual-link LVDS
On Tue, Jan 4, 2022, 17:56 Vinay Simha B N wrote:
> Robert,
> What is R-b tag?
>
It looks like this Reviewed-by: Name Lastname
Maybe have a quick look at this document.
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html
>
> On Tue, Jan 4, 2022 at 7:21 PM Robert Foss wrote:
> >
> > Jiri: Are you able to test this patch?
> >
> > Vinay: Could you supply a R-b tag, if you feel that it is warranted?
> >
> > On Tue, 14 Dec 2021 at 09:13, Vinay Simha B N
> wrote:
> > >
> > > Robert,
> > > I do not have the hardware to test this feature. Sorry for the late
> response.
> > >
> > > On Thu, Nov 18, 2021 at 8:20 PM Robert Foss
> wrote:
> > >>
> > >> + Zhen & Vinay
> > >>
> > >> This patch looks good in itself, but I would like to see a tested by
> > >> tag. At the very least testing for regression in single-link LVDS but
> > >> ideally some third party verification of this patch.
> > >>
> > >> On Wed, 10 Nov 2021 at 23:01, Jiri Vanek
> wrote:
> > >> >
> > >> > Fixed wrong register shift for single/dual link LVDS output.
> > >> >
> > >> > Signed-off-by: Jiri Vanek
> > >> > ---
> > >> > drivers/gpu/drm/bridge/tc358775.c | 2 +-
> > >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/bridge/tc358775.c
> b/drivers/gpu/drm/bridge/tc358775.c
> > >> > index 2272adcc5b4a..1d6ec1baeff2 100644
> > >> > --- a/drivers/gpu/drm/bridge/tc358775.c
> > >> > +++ b/drivers/gpu/drm/bridge/tc358775.c
> > >> > @@ -241,7 +241,7 @@ static inline u32
> TC358775_LVCFG_PCLKDIV(uint32_t val)
> > >> > }
> > >> >
> > >> > #define TC358775_LVCFG_LVDLINK__MASK
> 0x0002
> > >> > -#define TC358775_LVCFG_LVDLINK__SHIFT0
> > >> > +#define TC358775_LVCFG_LVDLINK__SHIFT1
> > >> > static inline u32 TC358775_LVCFG_LVDLINK(uint32_t val)
> > >> > {
> > >> > return ((val) << TC358775_LVCFG_LVDLINK__SHIFT) &
> > >> > --
> > >> > 2.30.2
> > >> >
> > >
> > >
> > >
> > > --
> > > regards,
> > > vinaysimha
>
>
>
> --
> regards,
> vinaysimha
>
Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV
On 2022-01-04 6:36 a.m., Christian König wrote: Am 04.01.22 um 11:49 schrieb Liu, Monk: [AMD Official Use Only] See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. No it's not, FLR from hypervisor is just to notify guest the hw VF FLR is about to start or was already executed, but host will do FLR anyway without waiting for guest too long Then we have a major design issue in the SRIOV protocol and really need to question this. How do you want to prevent a race between the hypervisor resetting the hardware and the client trying the same because of a timeout? As far as I can see the procedure should be: 1. We detect that a reset is necessary, either because of a fault a timeout or signal from hypervisor. 2. For each of those potential reset sources a work item is send to the single workqueue. 3. One of those work items execute first and prepares the reset. 4. We either do the reset our self or notify the hypervisor that we are ready for the reset. 5. Cleanup after the reset, eventually resubmit jobs etc.. 6. Cancel work items which might have been scheduled from other reset sources. It does make sense that the hypervisor resets the hardware without waiting for the clients for too long, but if we don't follow this general steps we will always have a race between the different components. Monk, just to add to this - if indeed as you say that 'FLR from hypervisor is just to notify guest the hw VF FLR is about to start or was already executed, but host will do FLR anyway without waiting for guest too long' and there is no strict waiting from the hypervisor for IDH_READY_TO_RESET to be recived from guest before starting the reset then setting in_gpu_reset and locking reset_sem from guest side is not really full proof protection from MMIO accesses by the guest - it only truly helps if hypervisor waits for that message before initiation of HW reset. Andrey Regards, Christian. Am 04.01.22 um 11:49 schrieb Liu, Monk: [AMD Official Use Only] See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. No it's not, FLR from hypervisor is just to notify guest the hw VF FLR is about to start or was already executed, but host will do FLR anyway without waiting for guest too long In other words I strongly think that the current SRIOV reset implementation is severely broken and what Andrey is doing is actually fixing it. It makes the code to crash ... how could it be a fix ? I'm afraid the patch is NAK from me, but it is welcome if the cleanup do not ruin the logic, Andry or jingwen can try it if needed. Thanks --- Monk Liu | Cloud GPU & Virtualization Solution | AMD --- we are hiring software manager for CVS core team --- -Original Message- From: Koenig, Christian Sent: Tuesday, January 4, 2022 6:19 PM To: Chen, JingWen ; Christian König ; Grodzovsky, Andrey ; Deng, Emily ; Liu, Monk ; dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; Chen, Horace ; Chen, JingWen Cc: dan...@ffwll.ch Subject: Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV Hi Jingwen, well what I mean is that we need to adjust the implementation in amdgpu to actually match the requirements. Could be that the reset sequence is questionable in general, but I doubt so at least for now. See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. Properly setting in_gpu_reset is indeed mandatory, but should happen at a central place and not in the SRIOV specific code. In other words I strongly think that the current SRIOV reset implementation is severely broken and what Andrey is doing is actually fixing it. Regards, Christian. Am 04.01.22 um 10:07 schrieb JingWen Chen: Hi Christian, I'm not sure what do you mean by "we need to change SRIOV not the driver". Do you mean we should change the reset sequence in SRIOV? This will be a huge change for our SRIOV solution. From my point of view, we can directly use amdgpu_device_lock_adev and amdgpu_device_unlock_adev in flr_work instead of try_lock since no one will conflict with this thread with reset_domain introduced. But we do need the reset_sem and adev->in_gpu_reset to keep device untouched via user space. Best Regards, Jingwen Chen On 2022/1/3 下午6:17, Christian König wrote: Please don't.
RE: [PATCH v3 3/5] drm/i915/panelreplay: Initializaton and compute config for panel replay
> -Original Message-
> From: Souza, Jose
> Sent: Tuesday, January 4, 2022 9:25 PM
> To: dri-devel@lists.freedesktop.org; Manna, Animesh
> ; intel-...@lists.freedesktop.org
> Cc: Mun, Gwan-gyeong ; Nikula, Jani
> ; Kahola, Mika ; Navare,
> Manasi D
> Subject: Re: [PATCH v3 3/5] drm/i915/panelreplay: Initializaton and compute
> config for panel replay
>
> On Tue, 2022-01-04 at 21:21 +0530, Manna, Animesh wrote:
> > Hi,
> >
> > > -Original Message-
> > > From: Souza, Jose
> > > Sent: Wednesday, November 24, 2021 1:19 AM
> > > To: dri-devel@lists.freedesktop.org; Manna, Animesh
> > > ; intel-...@lists.freedesktop.org
> > > Cc: Mun, Gwan-gyeong ; Nikula, Jani
> > > ; Kahola, Mika ;
> > > Navare, Manasi D
> > > Subject: Re: [PATCH v3 3/5] drm/i915/panelreplay: Initializaton and
> > > compute config for panel replay
> > >
> > > On Sun, 2021-10-10 at 17:40 +0530, Animesh Manna wrote:
> > > > As panel replay feature similar to PSR feature of EDP panel, so
> > > > currently utilized existing psr framework for panel replay.
> > > >
> > > > v1: RFC version.
> > > > v2: optimized code, pr_enabled and pr_dpcd variable removed.
> > > > [Jose]
> > > > v3:
> > > > - code comments improved. [Jani]
> > > > - dpcd_readb used instead of dpcd_read. [Jani]
> > > > - panel-repaplay init/compute functions moved inside respective
> > > > psr function. [Jani]
> > > >
> > > > Signed-off-by: Animesh Manna
> > > > ---
> > > > .../drm/i915/display/intel_display_types.h| 2 +
> > > > drivers/gpu/drm/i915/display/intel_dp.c | 43 +
> > > > drivers/gpu/drm/i915/display/intel_psr.c | 48 +++
> > > > drivers/gpu/drm/i915/display/intel_psr.h | 3 ++
> > > > 4 files changed, 87 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > index 39e11eaec1a3..48f7d676ed2c 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > > @@ -1070,6 +1070,7 @@ struct intel_crtc_state {
> > > > bool req_psr2_sdp_prior_scanline;
> > > > u32 dc3co_exitline;
> > > > u16 su_y_granularity;
> > > > + bool has_panel_replay;
> > >
> > > We can drop this and reuse current ones ones, see bellow.
> > >
> > > > struct drm_dp_vsc_sdp psr_vsc;
> > > >
> > > > /*
> > > > @@ -1531,6 +1532,7 @@ struct intel_psr {
> > > > bool irq_aux_error;
> > > > u16 su_w_granularity;
> > > > u16 su_y_granularity;
> > > > + bool sink_panel_replay_support;
> > >
> > > move this closer to has_psr and set both when it is panel replay.
> > > otherwise psr functions will not be executed for panel replay, see
> > > CAN_PSR().
> >
> > AFIU Panel replay do not have any dependency with PSR.
> > So that’s why I have created separate function for panel replay which is
> > doing
> similar thing whatever needed for panel replay.
> > For example intel_panel_replay_compute_config() can be independent of
> intel_psr_compute_config().
> > Do you see any dependency with PSR for panel replay?
>
> There is no dependency but panel replay is PSR for DP so we should re-use PSR
> code as much as possible.
>
> eDP + sink_support = PSR
> DP + sink_support = panel replay
>
> So we can reuse has_psr and all other stuff.
Not sure what to reuse from psr other than selective update part which will not
enable on dg2 for panel replay.
Will check further on this.
Regards,
Animesh
>
> >
> > >
> > > > u32 dc3co_exitline;
> > > > u32 dc3co_exit_delay;
> > > > struct delayed_work dc3co_work;
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index 10fda20a5bd8..f58a7b72be14 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -1587,12 +1587,22 @@ static void
> > > intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
> > > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > >
> > > > - /*
> > > > -* Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> > > > -* VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > > > -* Colorimetry Format indication.
> > > > -*/
> > > > - vsc->revision = 0x5;
> > > > + if (crtc_state->has_panel_replay) {
> > > > + /*
> > > > +* Prepare VSC Header for SU as per DP 2.0 spec, Table
> > > > 2-223
> > > > +* VSC SDP supporting 3D stereo, Panel Replay, and Pixel
> > > > +* Encoding/Colorimetry Format indication.
> > > > +*/
> > > > + vsc->revision = 0x7;
> > > > + } else {
> > > > +
Re: [Intel-gfx] [PATCH v4 2/4] drm/i915: Use the vma resource as argument for gtt binding / unbinding
Hi, Oak, On 1/4/22 16:35, Zeng, Oak wrote: Regards, Oak -Original Message- From: Thomas Hellström Sent: January 4, 2022 3:29 AM To: Zeng, Oak ; intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew Subject: Re: [Intel-gfx] [PATCH v4 2/4] drm/i915: Use the vma resource as argument for gtt binding / unbinding Hi, Oak. On 1/4/22 00:08, Zeng, Oak wrote: Regards, Oak Looks like your emails always start with "Regards, Oak". a misconfiguration? My mail client (outlook) is set to automatically add a regards, when I compose new mail or reply email. Not a big problem -Original Message- From: Thomas Hellström Sent: January 3, 2022 1:58 PM To: Zeng, Oak ; intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Auld, Matthew Subject: Re: [Intel-gfx] [PATCH v4 2/4] drm/i915: Use the vma resource as argument for gtt binding / unbinding Hi, Oak. On 1/3/22 19:17, Zeng, Oak wrote: Regards, Oak -Original Message- From: Intel-gfx On Behalf Of Thomas Hellström Sent: January 3, 2022 7:00 AM To: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Thomas Hellström ; Auld, Matthew Subject: [Intel-gfx] [PATCH v4 2/4] drm/i915: Use the vma resource as argument for gtt binding / unbinding When introducing asynchronous unbinding, the vma itself may no longer be alive when the actual binding or unbinding takes place. Can we take an extra reference counter of the vma to keep the vma alive, until the actual binding/unbinding takes place? The point here is that that's not needed, and should be avoided. Can you explain more why "keeping vma alive until unbinding takes place" should be avoided? As I understand it, your series introduce asynchronized unbinding. But since vma might be no longer alive at the time of unbinding. To overcome this difficulty, you introduce a vma resource structure and you guarantee vma resource is alive at bind/unbind time. So you can use vma resource for the bind/unbind operation. My question is, can we achieve the asynchronized unbinding still using vma structure by keeping vma structure alive ( by ref count it). This way the change should be much smaller (compared to this series). Why it is harmful to keep the vma alive? Maybe you have other reasons to introduce vma resource that I don't see. When we allow asynchronous unbinding, it's allowed to immediately rebind the vma, possibly into the same gpu virtual address, but with different pages. And when doing that we don't want to block waiting for the unbind to execute. Imagine this sequence: 1. Virtual address a1 is bound to physical page p1 2. Unbind a1 from p1, asynchronous so actual unbind not happen yet 3. bind a1 to physical page p2, page table is changed, now a1 pointing to p2 in page table. 4. #2 now take place now - since in page table, a1 points to p2 now, does a1 point to scratch page after #4? Here, 3) will also become async, awaiting any pending unbinds in the address range provided to 3), in particular, the bind in 3) will await 4). See i915_vma_resource_bind_dep_await(), and the discussion on whether or not to use an interval tree for this at the start of i915_vma_resource.c In fact, we could allow a large number of outstanding binds and unbinds for a vma, which makes the vma structure unsuitable to track this, since there will no longer be a single mapping between a set of active pages and a vma, or a virtual gpu range and a vma. I agree that if pages - vma - virtual gpu range is not 1:1:1 mapping, we need introduce a finer-grained vma resource to for the non-1:1 mapping. I also understand the asynchronous unbinding utilize the virtual address space more effectively. But my feeling is that this non-1:1 mapping makes our program hard to understand and maintain. Since this non- 1:1 mapping is introduced by asynchronous binding/unbinding, maybe the real question here is, is it really benefit to introduce asynchronous unbinding? That's a relevant question, which I've asked myself a couple of times. Async unbinding has complicated things like error capture and indeed complicates the understanding of the binding process as well. The main gain is that we avoid a sync point at LMEM eviction, enabling us to pipeline eviction, moving forward it may also find use in the shrinker and for user-space prematurely wanting to re-use softpin addresses. /Thomas I am still not familiar enough to the codes. I suggest other experts to take a look also. @Bloomfield, Jon @Vetter, Daniel @Wilson, Chris P. Regards, Oak Thanks, /Thomas Regards, Oak If the vma is no longer alive, that means nobody uses it anymore, but the GPU may still have work in the pipe that references the GPU virtual address. /Thomas.
Re: [PATCH v3 3/5] drm/i915/panelreplay: Initializaton and compute config for panel replay
On Tue, 2022-01-04 at 21:21 +0530, Manna, Animesh wrote:
> Hi,
>
> > -Original Message-
> > From: Souza, Jose
> > Sent: Wednesday, November 24, 2021 1:19 AM
> > To: dri-devel@lists.freedesktop.org; Manna, Animesh
> > ; intel-...@lists.freedesktop.org
> > Cc: Mun, Gwan-gyeong ; Nikula, Jani
> > ; Kahola, Mika ; Navare,
> > Manasi D
> > Subject: Re: [PATCH v3 3/5] drm/i915/panelreplay: Initializaton and compute
> > config for panel replay
> >
> > On Sun, 2021-10-10 at 17:40 +0530, Animesh Manna wrote:
> > > As panel replay feature similar to PSR feature of EDP panel, so
> > > currently utilized existing psr framework for panel replay.
> > >
> > > v1: RFC version.
> > > v2: optimized code, pr_enabled and pr_dpcd variable removed. [Jose]
> > > v3:
> > > - code comments improved. [Jani]
> > > - dpcd_readb used instead of dpcd_read. [Jani]
> > > - panel-repaplay init/compute functions moved inside respective psr
> > > function. [Jani]
> > >
> > > Signed-off-by: Animesh Manna
> > > ---
> > > .../drm/i915/display/intel_display_types.h| 2 +
> > > drivers/gpu/drm/i915/display/intel_dp.c | 43 +
> > > drivers/gpu/drm/i915/display/intel_psr.c | 48 +++
> > > drivers/gpu/drm/i915/display/intel_psr.h | 3 ++
> > > 4 files changed, 87 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 39e11eaec1a3..48f7d676ed2c 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1070,6 +1070,7 @@ struct intel_crtc_state {
> > > bool req_psr2_sdp_prior_scanline;
> > > u32 dc3co_exitline;
> > > u16 su_y_granularity;
> > > + bool has_panel_replay;
> >
> > We can drop this and reuse current ones ones, see bellow.
> >
> > > struct drm_dp_vsc_sdp psr_vsc;
> > >
> > > /*
> > > @@ -1531,6 +1532,7 @@ struct intel_psr {
> > > bool irq_aux_error;
> > > u16 su_w_granularity;
> > > u16 su_y_granularity;
> > > + bool sink_panel_replay_support;
> >
> > move this closer to has_psr and set both when it is panel replay.
> > otherwise psr functions will not be executed for panel replay, see
> > CAN_PSR().
>
> AFIU Panel replay do not have any dependency with PSR.
> So that’s why I have created separate function for panel replay which is
> doing similar thing whatever needed for panel replay.
> For example intel_panel_replay_compute_config() can be independent of
> intel_psr_compute_config().
> Do you see any dependency with PSR for panel replay?
There is no dependency but panel replay is PSR for DP so we should re-use PSR
code as much as possible.
eDP + sink_support = PSR
DP + sink_support = panel replay
So we can reuse has_psr and all other stuff.
>
> >
> > > u32 dc3co_exitline;
> > > u32 dc3co_exit_delay;
> > > struct delayed_work dc3co_work;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 10fda20a5bd8..f58a7b72be14 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -1587,12 +1587,22 @@ static void
> > intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
> > > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >
> > > - /*
> > > - * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> > > - * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > > - * Colorimetry Format indication.
> > > - */
> > > - vsc->revision = 0x5;
> > > + if (crtc_state->has_panel_replay) {
> > > + /*
> > > + * Prepare VSC Header for SU as per DP 2.0 spec, Table 2-223
> > > + * VSC SDP supporting 3D stereo, Panel Replay, and Pixel
> > > + * Encoding/Colorimetry Format indication.
> > > + */
> > > + vsc->revision = 0x7;
> > > + } else {
> > > + /*
> > > + * Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> > > + * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > > + * Colorimetry Format indication.
> > > + */
> > > + vsc->revision = 0x5;
> > > + }
> > > +
> > > vsc->length = 0x13;
> > >
> > > /* DP 1.4a spec, Table 2-120 */
> > > @@ -1701,6 +1711,21 @@ void intel_dp_compute_psr_vsc_sdp(struct
> > intel_dp *intel_dp,
> > > vsc->revision = 0x4;
> > > vsc->length = 0xe;
> > > }
> > > + } else if (intel_dp->psr.enabled && !intel_dp_is_edp(intel_dp)) {
> > > + if (intel_dp->psr.colorimetry_support &&
> > > + intel_dp_needs_vsc_sdp(crtc_state, conn_state)) {
> > > + /* [Panel Replay with colorimetry info] */
> > > + intel_dp_compute_vsc_colorimetry(crtc_state,
> > conn_state,
> > > +
RE: [PATCH v3 3/5] drm/i915/panelreplay: Initializaton and compute config for panel replay
Hi,
> -Original Message-
> From: Souza, Jose
> Sent: Wednesday, November 24, 2021 1:19 AM
> To: dri-devel@lists.freedesktop.org; Manna, Animesh
> ; intel-...@lists.freedesktop.org
> Cc: Mun, Gwan-gyeong ; Nikula, Jani
> ; Kahola, Mika ; Navare,
> Manasi D
> Subject: Re: [PATCH v3 3/5] drm/i915/panelreplay: Initializaton and compute
> config for panel replay
>
> On Sun, 2021-10-10 at 17:40 +0530, Animesh Manna wrote:
> > As panel replay feature similar to PSR feature of EDP panel, so
> > currently utilized existing psr framework for panel replay.
> >
> > v1: RFC version.
> > v2: optimized code, pr_enabled and pr_dpcd variable removed. [Jose]
> > v3:
> > - code comments improved. [Jani]
> > - dpcd_readb used instead of dpcd_read. [Jani]
> > - panel-repaplay init/compute functions moved inside respective psr
> > function. [Jani]
> >
> > Signed-off-by: Animesh Manna
> > ---
> > .../drm/i915/display/intel_display_types.h| 2 +
> > drivers/gpu/drm/i915/display/intel_dp.c | 43 +
> > drivers/gpu/drm/i915/display/intel_psr.c | 48 +++
> > drivers/gpu/drm/i915/display/intel_psr.h | 3 ++
> > 4 files changed, 87 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> > b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 39e11eaec1a3..48f7d676ed2c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -1070,6 +1070,7 @@ struct intel_crtc_state {
> > bool req_psr2_sdp_prior_scanline;
> > u32 dc3co_exitline;
> > u16 su_y_granularity;
> > + bool has_panel_replay;
>
> We can drop this and reuse current ones ones, see bellow.
>
> > struct drm_dp_vsc_sdp psr_vsc;
> >
> > /*
> > @@ -1531,6 +1532,7 @@ struct intel_psr {
> > bool irq_aux_error;
> > u16 su_w_granularity;
> > u16 su_y_granularity;
> > + bool sink_panel_replay_support;
>
> move this closer to has_psr and set both when it is panel replay.
> otherwise psr functions will not be executed for panel replay, see CAN_PSR().
AFIU Panel replay do not have any dependency with PSR.
So that’s why I have created separate function for panel replay which is doing
similar thing whatever needed for panel replay.
For example intel_panel_replay_compute_config() can be independent of
intel_psr_compute_config().
Do you see any dependency with PSR for panel replay?
>
> > u32 dc3co_exitline;
> > u32 dc3co_exit_delay;
> > struct delayed_work dc3co_work;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 10fda20a5bd8..f58a7b72be14 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -1587,12 +1587,22 @@ static void
> intel_dp_compute_vsc_colorimetry(const struct intel_crtc_state *crtc
> > struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >
> > - /*
> > -* Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> > -* VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > -* Colorimetry Format indication.
> > -*/
> > - vsc->revision = 0x5;
> > + if (crtc_state->has_panel_replay) {
> > + /*
> > +* Prepare VSC Header for SU as per DP 2.0 spec, Table 2-223
> > +* VSC SDP supporting 3D stereo, Panel Replay, and Pixel
> > +* Encoding/Colorimetry Format indication.
> > +*/
> > + vsc->revision = 0x7;
> > + } else {
> > + /*
> > +* Prepare VSC Header for SU as per DP 1.4 spec, Table 2-118
> > +* VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > +* Colorimetry Format indication.
> > +*/
> > + vsc->revision = 0x5;
> > + }
> > +
> > vsc->length = 0x13;
> >
> > /* DP 1.4a spec, Table 2-120 */
> > @@ -1701,6 +1711,21 @@ void intel_dp_compute_psr_vsc_sdp(struct
> intel_dp *intel_dp,
> > vsc->revision = 0x4;
> > vsc->length = 0xe;
> > }
> > + } else if (intel_dp->psr.enabled && !intel_dp_is_edp(intel_dp)) {
> > + if (intel_dp->psr.colorimetry_support &&
> > + intel_dp_needs_vsc_sdp(crtc_state, conn_state)) {
> > + /* [Panel Replay with colorimetry info] */
> > + intel_dp_compute_vsc_colorimetry(crtc_state,
> conn_state,
> > +vsc);
> > + } else {
> > + /*
> > +* [Panel Replay without colorimetry info]
> > +* Prepare VSC Header for SU as per DP 2.0 spec, Table
> 2-223
> > +* VSC SDP supporting 3D stereo + Panel Replay.
> > +*/
> > + vsc->revision = 0x6;
> > +
RE: [PATCH v3 1/5] drm/i915/panelreplay: dpcd register definition for panelreplay
> -Original Message- > From: Souza, Jose > Sent: Wednesday, November 24, 2021 1:07 AM > To: dri-devel@lists.freedesktop.org; Manna, Animesh > ; intel-...@lists.freedesktop.org > Cc: Mun, Gwan-gyeong ; Nikula, Jani > ; Kahola, Mika ; Navare, > Manasi D > Subject: Re: [PATCH v3 1/5] drm/i915/panelreplay: dpcd register definition for > panelreplay > > On Sun, 2021-10-10 at 17:40 +0530, Animesh Manna wrote: > > DPCD register definition added to check and enable panel replay > > capability of the sink. > > > > Signed-off-by: Animesh Manna > > --- > > include/drm/drm_dp_helper.h | 6 ++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h > > index b52df4db3e8f..8a2b929c3f88 100644 > > --- a/include/drm/drm_dp_helper.h > > +++ b/include/drm/drm_dp_helper.h > > @@ -541,6 +541,9 @@ struct drm_panel; > > /* DFP Capability Extension */ > > #define DP_DFP_CAPABILITY_EXTENSION_SUPPORT0x0a3 /* 2.0 */ > > > > +#define DP_PANEL_REPLAY_CAP 0x0b0 > > +# define PANEL_REPLAY_SUPPORT (1 << 0) > > Missing bit 1, that is very important when panel do not support selective > update > panel replay needs to act like PSR1 when it is sets it needs to act like PSR2. > > > + > > /* Link Configuration */ > > #defineDP_LINK_BW_SET 0x100 > > # define DP_LINK_RATE_TABLE0x00/* eDP 1.4 */ > > @@ -709,6 +712,9 @@ struct drm_panel; > > #define DP_BRANCH_DEVICE_CTRL 0x1a1 > > # define DP_BRANCH_DEVICE_IRQ_HPD (1 << 0) > > > > +#define PANEL_REPLAY_CONFIG 0x1b0 > > +# define PANEL_REPLAY_ENABLE(1 << 0) > > All other bits are also important, for the errors ones we have PSR counter > parts > and your are missing the error status register. Thanks for review. Added the suggested changes in current version. Regards, Animesh > > > + > > #define DP_PAYLOAD_ALLOCATE_SET0x1c0 > > #define DP_PAYLOAD_ALLOCATE_START_TIME_SLOT 0x1c1 #define > > DP_PAYLOAD_ALLOCATE_TIME_SLOT_COUNT 0x1c2
RE: [Intel-gfx] [PATCH v4 2/4] drm/i915: Use the vma resource as argument for gtt binding / unbinding
Regards, Oak > -Original Message- > From: Thomas Hellström > Sent: January 4, 2022 3:29 AM > To: Zeng, Oak ; intel-...@lists.freedesktop.org; > dri-devel@lists.freedesktop.org > Cc: Auld, Matthew > Subject: Re: [Intel-gfx] [PATCH v4 2/4] drm/i915: Use the vma resource as > argument for gtt binding / unbinding > > Hi, Oak. > > On 1/4/22 00:08, Zeng, Oak wrote: > > > > Regards, > > Oak > > Looks like your emails always start with "Regards, Oak". a misconfiguration? My mail client (outlook) is set to automatically add a regards, when I compose new mail or reply email. Not a big problem > > > >> -Original Message- > >> From: Thomas Hellström > >> Sent: January 3, 2022 1:58 PM > >> To: Zeng, Oak ; intel-...@lists.freedesktop.org; > >> dri-devel@lists.freedesktop.org > >> Cc: Auld, Matthew > >> Subject: Re: [Intel-gfx] [PATCH v4 2/4] drm/i915: Use the vma resource as > >> argument for gtt binding / unbinding > >> > >> Hi, Oak. > >> > >> On 1/3/22 19:17, Zeng, Oak wrote: > >>> Regards, > >>> Oak > >>> > -Original Message- > From: Intel-gfx On Behalf Of > Thomas Hellström > Sent: January 3, 2022 7:00 AM > To: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Cc: Thomas Hellström ; Auld, Matthew > > Subject: [Intel-gfx] [PATCH v4 2/4] drm/i915: Use the vma resource as > argument for gtt binding / unbinding > > When introducing asynchronous unbinding, the vma itself may no longer > be alive when the actual binding or unbinding takes place. > >>> Can we take an extra reference counter of the vma to keep the vma alive, > >>> until the actual binding/unbinding takes place? > >> The point here is that that's not needed, and should be avoided. > > Can you explain more why "keeping vma alive until unbinding takes place" > > should be avoided? > > > > As I understand it, your series introduce asynchronized unbinding. But > > since vma might be no longer alive at the time of unbinding. > To overcome this difficulty, you introduce a vma resource structure and you > guarantee vma resource is alive at bind/unbind time. So > you can use vma resource for the bind/unbind operation. My question is, can > we achieve the asynchronized unbinding still using vma > structure by keeping vma structure alive ( by ref count it). This way the > change should be much smaller (compared to this series). Why > it is harmful to keep the vma alive? Maybe you have other reasons to > introduce vma resource that I don't see. > > When we allow asynchronous unbinding, it's allowed to immediately rebind > the vma, possibly into the same gpu virtual address, but with different > pages. And when doing that we don't want to block waiting for the unbind > to execute. Imagine this sequence: 1. Virtual address a1 is bound to physical page p1 2. Unbind a1 from p1, asynchronous so actual unbind not happen yet 3. bind a1 to physical page p2, page table is changed, now a1 pointing to p2 in page table. 4. #2 now take place now - since in page table, a1 points to p2 now, does a1 point to scratch page after #4? In fact, we could allow a large number of outstanding binds > and unbinds for a vma, which makes the vma structure unsuitable to track > this, since there will no longer be a single mapping between a set of > active pages and a vma, or a virtual gpu range and a vma. I agree that if pages - vma - virtual gpu range is not 1:1:1 mapping, we need introduce a finer-grained vma resource to for the non-1:1 mapping. I also understand the asynchronous unbinding utilize the virtual address space more effectively. But my feeling is that this non-1:1 mapping makes our program hard to understand and maintain. Since this non- 1:1 mapping is introduced by asynchronous binding/unbinding, maybe the real question here is, is it really benefit to introduce asynchronous unbinding? I am still not familiar enough to the codes. I suggest other experts to take a look also. @Bloomfield, Jon @Vetter, Daniel @Wilson, Chris P. Regards, Oak > > Thanks, > > /Thomas > > > > > Regards, > > Oak > > > > If the > >> vma is no longer alive, that means nobody uses it anymore, but the GPU > >> may still have work in the pipe that references the GPU virtual address. > >> > >> /Thomas. > >>
[Bug 215436] admgpu: suspend and resuming from suspend don't work
https://bugzilla.kernel.org/show_bug.cgi?id=215436 --- Comment #27 from mario.limoncie...@amd.com --- Great! OK, let me send that out for review then and see what folks think. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 215436] admgpu: suspend and resuming from suspend don't work
https://bugzilla.kernel.org/show_bug.cgi?id=215436
--- Comment #26 from spassw...@web.de ---
For the extra call hpd_state = 0:
[ 30.684201] wlp4s0: deauthenticating from 54:67:51:3d:a2:e0 by local choice
(Reason: 3=DEAUTH_LEAVING)
[ 30.794571] amdgpu :08:00.0: amdgpu: Calling optimize_bandwidth from
dc_commit_state_no_check
[ 30.794609] amdgpu :08:00.0: amdgpu: Calling update_clocks from
dcn20_optimize_bandwidth
[ 30.794613] amdgpu :08:00.0: amdgpu: adev->in_s0ix = 0, hpd_state = 1
[ 35.026604] PM: suspend entry (s2idle)
[ 35.256153] Filesystems sync: 0.229 seconds
[ 35.257044] Freezing user space processes ... (elapsed 0.002 seconds) done.
[ 35.259636] OOM killer disabled.
[ 35.259636] Freezing remaining freezable tasks ... (elapsed 0.001 seconds)
done.
[ 35.260642] printk: Suspending console(s) (use no_console_suspend to debug)
[ 35.264212] amdgpu :08:00.0: amdgpu: amdgpu_pmops_suspend: adev->in_s0ix
= 1
[ 35.264369] amdgpu :08:00.0: amdgpu: calling update_clocks from
dcn21_optimize_pwr_state
[ 35.264379] amdgpu :08:00.0: amdgpu: adev->in_s0ix = 1, hpd_state = 0
[ 35.264381] amdgpu :08:00.0: amdgpu: calling
rn_vbios_smu_set_dcn_low_power
So one could go back to the old version of rn_update_clocks with
if (display_count == 0 && !hpd_state) {
--
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 13/24] dma-buf: drop the DAG approach for the dma_resv object
Am 22.12.21 um 22:43 schrieb Daniel Vetter:
On Tue, Dec 07, 2021 at 01:34:00PM +0100, Christian König wrote:
So far we had the approach of using a directed acyclic
graph with the dma_resv obj.
This turned out to have many downsides, especially it means
that every single driver and user of this interface needs
to be aware of this restriction when adding fences. If the
rules for the DAG are not followed then we end up with
potential hard to debug memory corruption, information
leaks or even elephant big security holes because we allow
userspace to access freed up memory.
Since we already took a step back from that by always
looking at all fences we now go a step further and stop
dropping the shared fences when a new exclusive one is
added.
Signed-off-by: Christian König
---
drivers/dma-buf/dma-resv.c | 13 -
1 file changed, 13 deletions(-)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
index 9acceabc9399..ecb2ff606bac 100644
--- a/drivers/dma-buf/dma-resv.c
+++ b/drivers/dma-buf/dma-resv.c
No doc update at all!
Scratching my head I'm pretty sure I've updated at least the kerneldoc
for dma_resv_add_excl_fence(). Must have gone lost in some rebase.
I checked, we're not that shitty with docs,
Well I wouldn't say shitty, but they are not perfect either.
Minimally the DOC: section
header and also the struct dma_resv kerneldoc. Also there's maybe more
references and stuff I've missed on a quick look, please check for them
(e.g. dma_buf.resv kerneldoc is rather important to keep correct too).
Code itself does what it says in the commit message, but we really need
the most accurate docs we can get for this stuff, or the confusion will
persist :-/
Yeah completely agree, going to fix that.
Thanks,
Christian.
Cheers, Daniel
@@ -383,29 +383,16 @@ EXPORT_SYMBOL(dma_resv_replace_fences);
void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence)
{
struct dma_fence *old_fence = dma_resv_excl_fence(obj);
- struct dma_resv_list *old;
- u32 i = 0;
dma_resv_assert_held(obj);
- old = dma_resv_shared_list(obj);
- if (old)
- i = old->shared_count;
-
dma_fence_get(fence);
write_seqcount_begin(>seq);
/* write_seqcount_begin provides the necessary memory barrier */
RCU_INIT_POINTER(obj->fence_excl, fence);
- if (old)
- old->shared_count = 0;
write_seqcount_end(>seq);
- /* inplace update, no shared fences */
- while (i--)
- dma_fence_put(rcu_dereference_protected(old->shared[i],
- dma_resv_held(obj)));
-
dma_fence_put(old_fence);
}
EXPORT_SYMBOL(dma_resv_add_excl_fence);
--
2.25.1
Re: [PATCH 2/2] drm: exynos: dsi: Add mode_set function
Hi Jagan,
On Mon, 22 Nov 2021 at 08:06, Jagan Teki wrote:
>
> Get the display mode settings via mode_set bridge function
> instead of explicitly de-reference.
>
> Signed-off-by: Jagan Teki
> ---
> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 13 -
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 174590f543c3..3d4713346949 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -260,6 +260,7 @@ struct exynos_dsi {
> struct drm_bridge bridge;
> struct drm_bridge *out_bridge;
> struct device *dev;
> + struct drm_display_mode mode;
>
> void __iomem *reg_base;
> struct phy *phy;
> @@ -883,7 +884,7 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
>
> static void exynos_dsi_set_display_mode(struct exynos_dsi *dsi)
> {
> - struct drm_display_mode *m = >encoder.crtc->state->adjusted_mode;
> + struct drm_display_mode *m = >mode;
> unsigned int num_bits_resol = dsi->driver_data->num_bits_resol;
> u32 reg;
>
> @@ -1526,6 +1527,15 @@ static int exynos_dsi_create_connector(struct
> exynos_dsi *dsi)
> return 0;
> }
>
> +static void exynos_dsi_mode_set(struct drm_bridge *bridge,
> + const struct drm_display_mode *mode,
> + const struct drm_display_mode *adjusted_mode)
> +{
> + struct exynos_dsi *dsi = bridge_to_dsi(bridge);
> +
> + drm_mode_copy(>mode, adjusted_mode);
> +}
> +
> static int exynos_dsi_attach(struct drm_bridge *bridge,
> enum drm_bridge_attach_flags flags)
> {
> @@ -1540,6 +1550,7 @@ static const struct drm_bridge_funcs
> exynos_dsi_bridge_funcs = {
> .atomic_reset = drm_atomic_helper_bridge_reset,
> .atomic_enable = exynos_dsi_atomic_enable,
> .atomic_disable = exynos_dsi_atomic_disable,
> + .mode_set = exynos_dsi_mode_set,
> .attach = exynos_dsi_attach,
> };
>
> --
> 2.25.1
>
Reviewed-by: Robert Foss
[Bug 215436] admgpu: suspend and resuming from suspend don't work
https://bugzilla.kernel.org/show_bug.cgi?id=215436 --- Comment #25 from mario.limoncie...@amd.com --- Can you check the state of the other variables though with that extra call? That was in my debugging patch but not inyours. Is the HPD active? -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 215436] admgpu: suspend and resuming from suspend don't work
https://bugzilla.kernel.org/show_bug.cgi?id=215436 --- Comment #24 from spassw...@web.de --- That's it! The extra call to clk_mgr_optimize_pwr_state now leads to rn_update_clock being called with adev->in_s0ix = 1: [ 31.142514] wlp4s0: deauthenticating from 54:67:51:3d:a2:e0 by local choice (Reason: 3=DEAUTH_LEAVING) [ 31.176805] amdgpu :08:00.0: amdgpu: Calling optimize_bandwidth from dc_commit_state_no_check [ 31.176815] amdgpu :08:00.0: amdgpu: Calling update_clocks from dcn20_optimize_bandwidth [ 31.176818] amdgpu :08:00.0: amdgpu: adev->in_s0ix = 0 [ 35.056209] PM: suspend entry (s2idle) [ 36.535688] Filesystems sync: 1.479 seconds [ 36.536404] Freezing user space processes ... (elapsed 0.024 seconds) done. [ 36.561024] OOM killer disabled. [ 36.561025] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. [ 36.562212] printk: Suspending console(s) (use no_console_suspend to debug) [ 36.566381] amdgpu :08:00.0: amdgpu: amdgpu_pmops_suspend: adev->in_s0ix = 1 [ 36.566557] amdgpu :08:00.0: amdgpu: calling update_clocks from dcn21_optimize_pwr_state [ 36.566567] amdgpu :08:00.0: amdgpu: adev->in_s0ix = 1 [ 36.566569] amdgpu :08:00.0: amdgpu: calling rn_vbios_smu_set_dcn_low_power -- 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 2/2] drm/bridge: anx7625: add audio codec .get_eld support
On Tue, 9 Nov 2021 at 03:43, Xin Ji wrote:
>
> Provide .get_eld interface in hdmi_codec_ops for hdmi-codec driver.
>
> Signed-off-by: Xin Ji
> ---
> drivers/gpu/drm/bridge/analogix/anx7625.c | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 6d93026c2999..67a87d21b0ba 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -1821,9 +1821,27 @@ static int anx7625_audio_hook_plugged_cb(struct device
> *dev, void *data,
> return 0;
> }
>
> +static int anx7625_audio_get_eld(struct device *dev, void *data,
> +u8 *buf, size_t len)
> +{
> + struct anx7625_data *ctx = dev_get_drvdata(dev);
> +
> + if (!ctx->connector) {
> + DRM_DEV_DEBUG_DRIVER(dev, "connector not initial\n");
> + return -EINVAL;
> + }
> +
> + DRM_DEV_DEBUG_DRIVER(dev, "audio copy eld\n");
> + memcpy(buf, ctx->connector->eld,
> + min(sizeof(ctx->connector->eld), len));
> +
> + return 0;
> +}
> +
> static const struct hdmi_codec_ops anx7625_codec_ops = {
> .hw_params = anx7625_audio_hw_params,
> .audio_shutdown = anx7625_audio_shutdown,
> + .get_eld= anx7625_audio_get_eld,
> .get_dai_id = anx7625_hdmi_i2s_get_dai_id,
> .hook_plugged_cb = anx7625_audio_hook_plugged_cb,
> };
> --
> 2.25.1
>
Reviewed-by: Robert Foss
Re: [PATCH 1/2] drm/bridge: anx7625: add HDCP support
Hey Xin,
On Tue, 9 Nov 2021 at 03:42, Xin Ji wrote:
>
> This patch provides HDCP setting interface for userspace to dynamic
> enable/disable HDCP function.
>
> Signed-off-by: Xin Ji
> ---
> drivers/gpu/drm/bridge/analogix/anx7625.c | 368 +-
> drivers/gpu/drm/bridge/analogix/anx7625.h | 69 +++-
> 2 files changed, 425 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 001fb39d9919..6d93026c2999 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -26,6 +26,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -213,6 +214,60 @@ static int wait_aux_op_finish(struct anx7625_data *ctx)
> return 0;
> }
>
> +static int anx7625_aux_dpcd_read(struct anx7625_data *ctx,
> +u8 addrh, u8 addrm, u8 addrl,
> +u8 len, u8 *buf)
> +{
> + struct device *dev = >client->dev;
> + int ret;
> + u8 cmd;
> +
> + if (len > MAX_DPCD_BUFFER_SIZE) {
> + DRM_DEV_ERROR(dev, "exceed aux buffer len.\n");
> + return -EINVAL;
> + }
> +
> + cmd = ((len - 1) << 4) | 0x09;
> +
> + /* Set command and length */
> + ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> + AP_AUX_COMMAND, cmd);
> +
> + /* Set aux access address */
> + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +AP_AUX_ADDR_7_0, addrl);
> + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +AP_AUX_ADDR_15_8, addrm);
> + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +AP_AUX_ADDR_19_16, addrh);
> +
> + /* Enable aux access */
> + ret |= anx7625_write_or(ctx, ctx->i2c.rx_p0_client,
> + AP_AUX_CTRL_STATUS, AP_AUX_CTRL_OP_EN);
> +
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "cannot access aux related register.\n");
> + return -EIO;
> + }
> +
> + usleep_range(2000, 2100);
> +
> + ret = wait_aux_op_finish(ctx);
> + if (ret) {
> + DRM_DEV_ERROR(dev, "aux IO error: wait aux op finish.\n");
> + return ret;
> + }
> +
> + ret = anx7625_reg_block_read(ctx, ctx->i2c.rx_p0_client,
> +AP_AUX_BUFF_START, len, buf);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "read dpcd register failed\n");
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> static int anx7625_video_mute_control(struct anx7625_data *ctx,
> u8 status)
> {
> @@ -669,6 +724,160 @@ static int anx7625_dpi_config(struct anx7625_data *ctx)
> return ret;
> }
>
> +static int anx7625_read_flash_status(struct anx7625_data *ctx)
> +{
> + return anx7625_reg_read(ctx, ctx->i2c.rx_p0_client, R_RAM_CTRL);
> +}
> +
> +static int anx7625_hdcp_key_probe(struct anx7625_data *ctx)
> +{
> + int ret, val;
> + struct device *dev = >client->dev;
> + u8 ident[32];
Could this hardcoded array length be replaced with FLASH_BUF_LEN?
> +
> + ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> + FLASH_ADDR_HIGH, 0x91);
> + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +FLASH_ADDR_LOW, 0xA0);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "IO error : set key flash address.\n");
> + return ret;
> + }
> +
> + ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> + FLASH_LEN_HIGH, (FLASH_BUF_LEN - 1) >> 8);
> + ret |= anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +FLASH_LEN_LOW, (FLASH_BUF_LEN - 1) & 0xFF);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "IO error : set key flash len.\n");
> + return ret;
> + }
> +
> + ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> + R_FLASH_RW_CTRL, FLASH_READ);
> + ret |= readx_poll_timeout(anx7625_read_flash_status,
> + ctx, val,
> + ((val & FLASH_DONE) || (val < 0)),
> + 2000,
> + 2000 * 150);
> + if (ret) {
> + DRM_DEV_ERROR(dev, "flash read access fail!\n");
> + return -EIO;
> + }
> +
> + ret = anx7625_reg_block_read(ctx, ctx->i2c.rx_p0_client,
> +FLASH_BUF_BASE_ADDR,
> +FLASH_BUF_LEN, ident);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "read flash data fail!\n");
> +
Re: [PATCH] drm/bridge: parade-ps8640: Link device to ensure suspend/resume order
Il 04/01/22 15:22, Robert Foss ha scritto:
Hey AngeloGioacchino,
On Tue, 2 Nov 2021 at 14:08, AngeloGioacchino Del Regno
wrote:
Entering suspend while the display attached to this bridge is still on
makes the resume sequence to resume the bridge first, display last:
when this happens, we get a timeout while resuming the bridge, as its
MCU will get stuck due to the display being unpowered.
On the other hand, on mt8173-elm, closing the lid makes the display to
get powered off first, bridge last, so at resume time the sequence is
swapped (compared to the first example) and everything just works
as expected.
Add a stateless device link to the DRM device that this bridge belongs
to, ensuring a correct resume sequence and solving the unability to
correctly resume bridge operation in the first mentioned example.
Signed-off-by: AngeloGioacchino Del Regno
---
drivers/gpu/drm/bridge/parade-ps8640.c | 22 --
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
b/drivers/gpu/drm/bridge/parade-ps8640.c
index 45100edd745b..191cc196c9d1 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -100,6 +100,7 @@ struct ps8640 {
struct regulator_bulk_data supplies[2];
struct gpio_desc *gpio_reset;
struct gpio_desc *gpio_powerdown;
+ struct device_link *link;
bool powered;
};
@@ -460,10 +461,23 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
goto err_aux_register;
}
+ ps_bridge->link = device_link_add(bridge->dev->dev, dev,
DL_FLAG_STATELESS);
+ if (!ps_bridge->link) {
+ dev_err(dev, "failed to create device link");
+ ret = -EINVAL;
+ goto err_devlink;
+ }
+
/* Attach the panel-bridge to the dsi bridge */
- return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
+ ret = drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
_bridge->bridge, flags);
+ if (ret)
+ goto err_bridge_attach;
+err_bridge_attach:
+ device_link_del(ps_bridge->link);
+err_devlink:
+ drm_dp_aux_unregister(_bridge->aux);
err_aux_register:
mipi_dsi_detach(dsi);
err_dsi_attach:
@@ -473,7 +487,11 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
static void ps8640_bridge_detach(struct drm_bridge *bridge)
{
- drm_dp_aux_unregister(_to_ps8640(bridge)->aux);
+ struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
+
+ drm_dp_aux_unregister(_bridge->aux);
+ if (ps_bridge->link)
+ device_link_del(ps_bridge->link);
}
static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
--
2.33.1
This patch does not apply on drm-misc-next, could you rebase it on the
current branch?
Sure, I'll rebase it asap.
Re: [PATCH] drm/bridge/tc358775: Fix for dual-link LVDS
Excellent.
Jiri, can you add your Tested-by tag to this patch?
On Tue, 4 Jan 2022 at 15:29, Jiří Vaněk wrote:
>
> Actually, this patch is based on testing with a real HW with dual-link LVDS
> display (full HD) and it also matches with a datasheet. Without this fix it
> does not work at all.
>
> út 4. 1. 2022 v 14:51 odesílatel Robert Foss napsal:
>>
>> Jiri: Are you able to test this patch?
>>
>> Vinay: Could you supply a R-b tag, if you feel that it is warranted?
>>
>> On Tue, 14 Dec 2021 at 09:13, Vinay Simha B N wrote:
>> >
>> > Robert,
>> > I do not have the hardware to test this feature. Sorry for the late
>> > response.
>> >
>> > On Thu, Nov 18, 2021 at 8:20 PM Robert Foss wrote:
>> >>
>> >> + Zhen & Vinay
>> >>
>> >> This patch looks good in itself, but I would like to see a tested by
>> >> tag. At the very least testing for regression in single-link LVDS but
>> >> ideally some third party verification of this patch.
>> >>
>> >> On Wed, 10 Nov 2021 at 23:01, Jiri Vanek wrote:
>> >> >
>> >> > Fixed wrong register shift for single/dual link LVDS output.
>> >> >
>> >> > Signed-off-by: Jiri Vanek
>> >> > ---
>> >> > drivers/gpu/drm/bridge/tc358775.c | 2 +-
>> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/bridge/tc358775.c
>> >> > b/drivers/gpu/drm/bridge/tc358775.c
>> >> > index 2272adcc5b4a..1d6ec1baeff2 100644
>> >> > --- a/drivers/gpu/drm/bridge/tc358775.c
>> >> > +++ b/drivers/gpu/drm/bridge/tc358775.c
>> >> > @@ -241,7 +241,7 @@ static inline u32 TC358775_LVCFG_PCLKDIV(uint32_t
>> >> > val)
>> >> > }
>> >> >
>> >> > #define TC358775_LVCFG_LVDLINK__MASK 0x0002
>> >> > -#define TC358775_LVCFG_LVDLINK__SHIFT0
>> >> > +#define TC358775_LVCFG_LVDLINK__SHIFT1
>> >> > static inline u32 TC358775_LVCFG_LVDLINK(uint32_t val)
>> >> > {
>> >> > return ((val) << TC358775_LVCFG_LVDLINK__SHIFT) &
>> >> > --
>> >> > 2.30.2
>> >> >
>> >
>> >
>> >
>> > --
>> > regards,
>> > vinaysimha
Re: [PATCH] drm/bridge: anx7625: Fix null vs IS_ERR() checking in anx7625_register_i2c_dummy_clients
Applied to drm-misc-next
Re: [PATCH] drm/bridge/tc358775: Fix for dual-link LVDS
Actually, this patch is based on testing with a real HW with dual-link LVDS
display (full HD) and it also matches with a datasheet. Without this fix it
does not work at all.
út 4. 1. 2022 v 14:51 odesílatel Robert Foss
napsal:
> Jiri: Are you able to test this patch?
>
> Vinay: Could you supply a R-b tag, if you feel that it is warranted?
>
> On Tue, 14 Dec 2021 at 09:13, Vinay Simha B N wrote:
> >
> > Robert,
> > I do not have the hardware to test this feature. Sorry for the late
> response.
> >
> > On Thu, Nov 18, 2021 at 8:20 PM Robert Foss
> wrote:
> >>
> >> + Zhen & Vinay
> >>
> >> This patch looks good in itself, but I would like to see a tested by
> >> tag. At the very least testing for regression in single-link LVDS but
> >> ideally some third party verification of this patch.
> >>
> >> On Wed, 10 Nov 2021 at 23:01, Jiri Vanek wrote:
> >> >
> >> > Fixed wrong register shift for single/dual link LVDS output.
> >> >
> >> > Signed-off-by: Jiri Vanek
> >> > ---
> >> > drivers/gpu/drm/bridge/tc358775.c | 2 +-
> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/bridge/tc358775.c
> b/drivers/gpu/drm/bridge/tc358775.c
> >> > index 2272adcc5b4a..1d6ec1baeff2 100644
> >> > --- a/drivers/gpu/drm/bridge/tc358775.c
> >> > +++ b/drivers/gpu/drm/bridge/tc358775.c
> >> > @@ -241,7 +241,7 @@ static inline u32 TC358775_LVCFG_PCLKDIV(uint32_t
> val)
> >> > }
> >> >
> >> > #define TC358775_LVCFG_LVDLINK__MASK
> 0x0002
> >> > -#define TC358775_LVCFG_LVDLINK__SHIFT0
> >> > +#define TC358775_LVCFG_LVDLINK__SHIFT1
> >> > static inline u32 TC358775_LVCFG_LVDLINK(uint32_t val)
> >> > {
> >> > return ((val) << TC358775_LVCFG_LVDLINK__SHIFT) &
> >> > --
> >> > 2.30.2
> >> >
> >
> >
> >
> > --
> > regards,
> > vinaysimha
>
Re: [PATCH] drm/bridge: anx7625: Fix null vs IS_ERR() checking in anx7625_register_i2c_dummy_clients
Hey Miaoqian,
Thanks for submitting this fix.
On Wed, 22 Dec 2021 at 09:33, Miaoqian Lin wrote:
>
> Since i2c_new_client_device() function return error pointers.
> The i2c_new_dummy_device() function does not return NULL, It returns error
> pointers too. Using IS_ERR() to check the return value to fix this.
>
> Fixes: 8bdfc5dae4e3("drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP")
> Signed-off-by: Miaoqian Lin
> ---
> drivers/gpu/drm/bridge/analogix/anx7625.c | 32 ---
> 1 file changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 1a871f6b6822..eb72aa6aedd6 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -1637,40 +1637,54 @@ static const struct drm_bridge_funcs
> anx7625_bridge_funcs = {
> static int anx7625_register_i2c_dummy_clients(struct anx7625_data *ctx,
> struct i2c_client *client)
> {
> + int err = 0;
> +
> ctx->i2c.tx_p0_client = i2c_new_dummy_device(client->adapter,
> TX_P0_ADDR >> 1);
> - if (!ctx->i2c.tx_p0_client)
> - return -ENOMEM;
> + if (IS_ERR(ctx->i2c.tx_p0_client))
> + return PTR_ERR(ctx->i2c.tx_p0_client);
>
> ctx->i2c.tx_p1_client = i2c_new_dummy_device(client->adapter,
> TX_P1_ADDR >> 1);
> - if (!ctx->i2c.tx_p1_client)
> + if (IS_ERR(ctx->i2c.tx_p1_client)) {
> + err = PTR_ERR(ctx->i2c.tx_p1_client);
> goto free_tx_p0;
> + }
>
> ctx->i2c.tx_p2_client = i2c_new_dummy_device(client->adapter,
> TX_P2_ADDR >> 1);
> - if (!ctx->i2c.tx_p2_client)
> + if (IS_ERR(ctx->i2c.tx_p2_client)) {
> + err = PTR_ERR(ctx->i2c.tx_p2_client);
> goto free_tx_p1;
> + }
>
> ctx->i2c.rx_p0_client = i2c_new_dummy_device(client->adapter,
> RX_P0_ADDR >> 1);
> - if (!ctx->i2c.rx_p0_client)
> + if (IS_ERR(ctx->i2c.rx_p0_client)) {
> + err = PTR_ERR(ctx->i2c.rx_p0_client);
> goto free_tx_p2;
> + }
>
> ctx->i2c.rx_p1_client = i2c_new_dummy_device(client->adapter,
> RX_P1_ADDR >> 1);
> - if (!ctx->i2c.rx_p1_client)
> + if (IS_ERR(ctx->i2c.rx_p1_client)) {
> + err = PTR_ERR(ctx->i2c.rx_p1_client);
> goto free_rx_p0;
> + }
>
> ctx->i2c.rx_p2_client = i2c_new_dummy_device(client->adapter,
> RX_P2_ADDR >> 1);
> - if (!ctx->i2c.rx_p2_client)
> + if (IS_ERR(ctx->i2c.rx_p2_client)) {
> + err = PTR_ERR(ctx->i2c.rx_p2_client);
> goto free_rx_p1;
> + }
>
> ctx->i2c.tcpc_client = i2c_new_dummy_device(client->adapter,
> TCPC_INTERFACE_ADDR >> 1);
> - if (!ctx->i2c.tcpc_client)
> + if (IS_ERR(ctx->i2c.tcpc_client)) {
> + err = PTR_ERR(ctx->i2c.tcpc_client);
> goto free_rx_p2;
> + }
>
> return 0;
>
> @@ -1687,7 +1701,7 @@ static int anx7625_register_i2c_dummy_clients(struct
> anx7625_data *ctx,
> free_tx_p0:
> i2c_unregister_device(ctx->i2c.tx_p0_client);
>
> - return -ENOMEM;
> + return err;
> }
>
> static void anx7625_unregister_i2c_dummy_clients(struct anx7625_data *ctx)
> --
> 2.17.1
>
Reviewed-by: Robert Foss
Re: [PATCH] drm/bridge: parade-ps8640: Link device to ensure suspend/resume order
Hey AngeloGioacchino,
On Tue, 2 Nov 2021 at 14:08, AngeloGioacchino Del Regno
wrote:
>
> Entering suspend while the display attached to this bridge is still on
> makes the resume sequence to resume the bridge first, display last:
> when this happens, we get a timeout while resuming the bridge, as its
> MCU will get stuck due to the display being unpowered.
>
> On the other hand, on mt8173-elm, closing the lid makes the display to
> get powered off first, bridge last, so at resume time the sequence is
> swapped (compared to the first example) and everything just works
> as expected.
>
> Add a stateless device link to the DRM device that this bridge belongs
> to, ensuring a correct resume sequence and solving the unability to
> correctly resume bridge operation in the first mentioned example.
>
> Signed-off-by: AngeloGioacchino Del Regno
>
> ---
> drivers/gpu/drm/bridge/parade-ps8640.c | 22 --
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c
> b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 45100edd745b..191cc196c9d1 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -100,6 +100,7 @@ struct ps8640 {
> struct regulator_bulk_data supplies[2];
> struct gpio_desc *gpio_reset;
> struct gpio_desc *gpio_powerdown;
> + struct device_link *link;
> bool powered;
> };
>
> @@ -460,10 +461,23 @@ static int ps8640_bridge_attach(struct drm_bridge
> *bridge,
> goto err_aux_register;
> }
>
> + ps_bridge->link = device_link_add(bridge->dev->dev, dev,
> DL_FLAG_STATELESS);
> + if (!ps_bridge->link) {
> + dev_err(dev, "failed to create device link");
> + ret = -EINVAL;
> + goto err_devlink;
> + }
> +
> /* Attach the panel-bridge to the dsi bridge */
> - return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
> + ret = drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
> _bridge->bridge, flags);
> + if (ret)
> + goto err_bridge_attach;
>
> +err_bridge_attach:
> + device_link_del(ps_bridge->link);
> +err_devlink:
> + drm_dp_aux_unregister(_bridge->aux);
> err_aux_register:
> mipi_dsi_detach(dsi);
> err_dsi_attach:
> @@ -473,7 +487,11 @@ static int ps8640_bridge_attach(struct drm_bridge
> *bridge,
>
> static void ps8640_bridge_detach(struct drm_bridge *bridge)
> {
> - drm_dp_aux_unregister(_to_ps8640(bridge)->aux);
> + struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +
> + drm_dp_aux_unregister(_bridge->aux);
> + if (ps_bridge->link)
> + device_link_del(ps_bridge->link);
> }
>
> static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
> --
> 2.33.1
>
This patch does not apply on drm-misc-next, could you rebase it on the
current branch?
Re: [PATCH 2/4] backlight: qcom-wled: Add PM6150L compatible
On Wed, Dec 29, 2021 at 06:03:56PM +0100, Luca Weiss wrote: > PM6150L contains WLED of version 5. Add support ofr it to the driver. > > Signed-off-by: Luca Weiss Reviewed-by: Daniel Thompson Daniel.
Re: [PATCH] drm: bridge: cdns-mhdp8546: Fix a NULL pointer dereference in cdns_mhdp_atomic_enable()
Hey Zhou,
Thanks for submitting this patch.
On Tue, 30 Nov 2021 at 14:11, Zhou Qingyang wrote:
>
> In cdns_mhdp_atomic_enable(), the return value of drm_mode_duplicate()
> is assigned to mhdp_state->current_mode and used in drm_mode_set_name().
> There is a dereference of it in drm_mode_set_name(), which could lead
> to a NULL pointer dereference on failure of drm_mode_duplicate().
>
> Fix this bug by adding a check of mhdp_state->current_mode.
>
> This bug was found by a static analyzer. The analysis employs
> differential checking to identify inconsistent security operations
> (e.g., checks or kfrees) between two code paths and confirms that the
> inconsistent operations are not recovered in the current function or
> the callers, so they constitute bugs.
>
> Note that, as a bug found by static analysis, it can be a false
> positive or hard to trigger. Multiple researchers have cross-reviewed
> the bug.
>
> Builds with CONFIG_DRM_CDNS_MHDP8546=m show no new warnings,
> and our static analyzer no longer warns about this code.
>
> Fixes: fb43aa0acdfd ("drm: bridge: Add support for Cadence MHDP8546 DPI/DP
> bridge")
> Signed-off-by: Zhou Qingyang
> ---
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 5530fbf64f1e..347fbecf76a4 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2040,6 +2040,11 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge
> *bridge,
> mhdp_state = to_cdns_mhdp_bridge_state(new_state);
>
> mhdp_state->current_mode = drm_mode_duplicate(bridge->dev, mode);
> + if (!mhdp_state->current_mode) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
This appears to be a problem that is handled in other drivers, but the
solution here does strike me as good.
The out-label will schedule modeset_retry_work to be executed if
ret==-ENOMEM. If drm_mode_duplicate() fails, we've had a memory
allocation issue, and failing is probably the correct solution here.
However cdns_mhdp_atomic_enable() does allow for signalling failures.
> drm_mode_set_name(mhdp_state->current_mode);
>
> dev_dbg(mhdp->dev, "%s: Enabling mode %s\n", __func__, mode->name);
> --
> 2.25.1
>
[Bug 215436] admgpu: suspend and resuming from suspend don't work
https://bugzilla.kernel.org/show_bug.cgi?id=215436 --- Comment #23 from mario.limoncie...@amd.com --- Did you try out my patch? It should have given an extra explicit call in the suspend path. -- 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 v6 4/6] drm: implement a method to free unused pages
On 26/12/2021 22:24, Arunpravin wrote:
On contiguous allocation, we round up the size
to the *next* power of 2, implement a function
to free the unused pages after the newly allocate block.
v2(Matthew Auld):
- replace function name 'drm_buddy_free_unused_pages' with
drm_buddy_block_trim
- replace input argument name 'actual_size' with 'new_size'
- add more validation checks for input arguments
- add overlaps check to avoid needless searching and splitting
- merged the below patch to see the feature in action
- add free unused pages support to i915 driver
- lock drm_buddy_block_trim() function as it calls mark_free/mark_split
are all globally visible
v3(Matthew Auld):
- remove trim method error handling as we address the failure case
at drm_buddy_block_trim() function
v4:
- in case of trim, at __alloc_range() split_block failure path
marks the block as free and removes it from the original list,
potentially also freeing it, to overcome this problem, we turn
the drm_buddy_block_trim() input node into a temporary node to
prevent recursively freeing itself, but still retain the
un-splitting/freeing of the other nodes(Matthew Auld)
- modify the drm_buddy_block_trim() function return type
Signed-off-by: Arunpravin
---
drivers/gpu/drm/drm_buddy.c | 61 +++
drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 8 +++
include/drm/drm_buddy.h | 4 ++
3 files changed, 73 insertions(+)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index eddc1eeda02e..855afcaf7edd 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -538,6 +538,67 @@ static int __drm_buddy_alloc_range(struct drm_buddy_mm *mm,
return __alloc_range(mm, , start, size, blocks);
}
+/**
+ * drm_buddy_block_trim - free unused pages
+ *
+ * @mm: DRM buddy manager
+ * @new_size: original size requested
+ * @blocks: output list head to add allocated blocks
+ *
+ * For contiguous allocation, we round up the size to the nearest
+ * power of two value, drivers consume *actual* size, so remaining
+ * portions are unused and it can be freed.
+ */
+void drm_buddy_block_trim(struct drm_buddy_mm *mm,
+ u64 new_size,
+ struct list_head *blocks)
It might be better to just return the error, and let the user decide if
they want to ignore it? Also we might want some kind of unit test for
this, so having an actual return value might be useful there.
+{
+ struct drm_buddy_block *parent;
+ struct drm_buddy_block *block;
+ LIST_HEAD(dfs);
+ u64 new_start;
+ int err;
+
+ if (!list_is_singular(blocks))
+ return;
+
+ block = list_first_entry(blocks,
+struct drm_buddy_block,
+link);
+
+ if (!drm_buddy_block_is_allocated(block))
+ return;
+
+ if (new_size > drm_buddy_block_size(mm, block))
+ return;
+
+ if (!new_size && !IS_ALIGNED(new_size, mm->chunk_size))
+ return;
+
+ if (new_size == drm_buddy_block_size(mm, block))
+ return;
+
+ list_del(>link);
+ mark_free(mm, block);
+ mm->avail += drm_buddy_block_size(mm, block);
+
+ /* Prevent recursively freeing this node */
+ parent = block->parent;
+ block->parent = NULL;
+
+ new_start = drm_buddy_block_offset(block);
+ list_add(>tmp_link, );
+ err = __alloc_range(mm, , new_start, new_size, blocks);
+ if (err) {
+ mark_allocated(block);
+ mm->avail -= drm_buddy_block_size(mm, block);
+ list_add(>link, blocks);
+ }
+
+ block->parent = parent;
+}
+EXPORT_SYMBOL(drm_buddy_block_trim);
+
/**
* drm_buddy_alloc - allocate power-of-two blocks
*
diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
index 7c58efb60dba..05f924f32e96 100644
--- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
+++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
@@ -97,6 +97,14 @@ static int i915_ttm_buddy_man_alloc(struct
ttm_resource_manager *man,
if (unlikely(err))
goto err_free_blocks;
+ if (place->flags & TTM_PL_FLAG_CONTIGUOUS) {
+ mutex_lock(>lock);
+ drm_buddy_block_trim(mm,
+ (u64)n_pages << PAGE_SHIFT,
AFAIK, n_pages has already been rounded up to the next power-of-two
here, so this becomes a noop. I assume we need to use the "original"
size here?
+ _res->blocks);
+ mutex_unlock(>lock);
+ }
+
*res = _res->base;
return 0;
diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
index f573b02304f4..703866a87939 100644
--- a/include/drm/drm_buddy.h
+++
Re: [PATCH v6 2/6] drm: improve drm_buddy_alloc function
On 26/12/2021 22:24, Arunpravin wrote:
- Make drm_buddy_alloc a single function to handle
range allocation and non-range allocation demands
- Implemented a new function alloc_range() which allocates
the requested power-of-two block comply with range limitations
- Moved order computation and memory alignment logic from
i915 driver to drm buddy
v2:
merged below changes to keep the build unbroken
- drm_buddy_alloc_range() becomes obsolete and may be removed
- enable ttm range allocation (fpfn / lpfn) support in i915 driver
- apply enhanced drm_buddy_alloc() function to i915 driver
v3(Matthew Auld):
- Fix alignment issues and remove unnecessary list_empty check
- add more validation checks for input arguments
- make alloc_range() block allocations as bottom-up
- optimize order computation logic
- replace uint64_t with u64, which is preferred in the kernel
v4(Matthew Auld):
- keep drm_buddy_alloc_range() function implementation for generic
actual range allocations
- keep alloc_range() implementation for end bias allocations
Signed-off-by: Arunpravin
@@ -73,34 +83,16 @@ static int i915_ttm_buddy_man_alloc(struct
ttm_resource_manager *man,
n_pages = size >> ilog2(mm->chunk_size);
- do {
- struct drm_buddy_block *block;
- unsigned int order;
-
- order = fls(n_pages) - 1;
- GEM_BUG_ON(order > mm->max_order);
- GEM_BUG_ON(order < min_order);
-
- do {
- mutex_lock(>lock);
- block = drm_buddy_alloc(mm, order);
- mutex_unlock(>lock);
- if (!IS_ERR(block))
- break;
-
- if (order-- == min_order) {
- err = -ENOSPC;
- goto err_free_blocks;
- }
- } while (1);
-
- n_pages -= BIT(order);
-
- list_add_tail(>link, _res->blocks);
-
- if (!n_pages)
- break;
- } while (1);
+ mutex_lock(>lock);
+ err = drm_buddy_alloc(mm, (u64)place->fpfn << PAGE_SHIFT,
+ (u64)place->lpfn << PAGE_SHIFT,
place->lpfn will currently always be zero for i915, AFAIK. I assume here
we want s/place->lpfn/lpfn/?
Also something in this series is preventing i915 from loading on
discrete devices, according to CI. Hopefully that is just the lpfn
issue...which might explain seeing -EINVAL here[1] when allocating some
vram for the firmware.
[1]
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_21904/bat-dg1-6/boot0.txt
+ (u64)n_pages << PAGE_SHIFT,
+min_page_size,
+_res->blocks,
+bman_res->flags);
+ mutex_unlock(>lock);
+ if (unlikely(err))
+ goto err_free_blocks;
*res = _res->base;
return 0;
@@ -266,10 +258,17 @@ int i915_ttm_buddy_man_reserve(struct
ttm_resource_manager *man,
{
struct i915_ttm_buddy_manager *bman = to_buddy_manager(man);
struct drm_buddy_mm *mm = >mm;
+ unsigned long flags = 0;
int ret;
+ flags |= DRM_BUDDY_RANGE_ALLOCATION;
+
mutex_lock(>lock);
- ret = drm_buddy_alloc_range(mm, >reserved, start, size);
+ ret = drm_buddy_alloc(mm, start,
+ start + size,
+ size, mm->chunk_size,
+ >reserved,
+ flags);
mutex_unlock(>lock);
return ret;
diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.h
b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.h
index fa644b512c2e..5ba490875f66 100644
--- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.h
+++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.h
@@ -20,6 +20,7 @@ struct drm_buddy_mm;
*
* @base: struct ttm_resource base class we extend
* @blocks: the list of struct i915_buddy_block for this resource/allocation
+ * @flags: DRM_BUDDY_*_ALLOCATION flags
* @mm: the struct i915_buddy_mm for this resource
*
* Extends the struct ttm_resource to manage an address space allocation with
@@ -28,6 +29,7 @@ struct drm_buddy_mm;
struct i915_ttm_buddy_resource {
struct ttm_resource base;
struct list_head blocks;
+ unsigned long flags;
struct drm_buddy_mm *mm;
};
diff --git a/include/drm/drm_buddy.h b/include/drm/drm_buddy.h
index 09d73328c268..4368acaad222 100644
--- a/include/drm/drm_buddy.h
+++ b/include/drm/drm_buddy.h
@@ -13,15 +13,22 @@
#include
-#define range_overflows(start, size, max) ({ \
+#define check_range_overflow(start, end, size, max) ({ \
typeof(start) start__ = (start); \
+ typeof(end) end__ = (end);\
typeof(size) size__ = (size); \
typeof(max) max__ = (max); \
(void)(__ == __); \
(void)(__ == __); \
-
Re: [PATCH] drm/stm: ltdc: support of new hardware version
On 12/14/21 11:19 AM, Philippe CORNU wrote: On 12/3/21 9:56 AM, Yannick Fertre wrote: Add support of new hardware version 0x40100. Signed-off-by: Yannick Fertre --- drivers/gpu/drm/stm/ltdc.c | 172 ++--- drivers/gpu/drm/stm/ltdc.h | 3 +- 2 files changed, 145 insertions(+), 30 deletions(-) Hi Yannick, This looks great, many thanks for your patch. Acked-by: Philippe Cornu Philippe :-) Applied on drm-misc-next. Many thanks for your patch, Philippe :-)
Re: [PATCH] drm/stm: remove conflicting framebuffers
On 12/14/21 11:15 AM, Philippe CORNU wrote:
On 12/6/21 3:23 PM, Thomas Zimmermann wrote:
Hi
Am 06.12.21 um 14:47 schrieb Yannick Fertre:
In case of using simplefb or another conflicting framebuffer,
call drm_aperture_remove_framebuffers() to remove memory allocated.
Signed-off-by: Yannick Fertre
The patch should have contained a note that this is version 2 of the
change with a short changelog. Anyway
Reviewed-by: Thomas Zimmermann
Best regards
Thomas
---
drivers/gpu/drm/stm/drv.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
index 222869b232ae..9f441aadf2d5 100644
--- a/drivers/gpu/drm/stm/drv.c
+++ b/drivers/gpu/drm/stm/drv.c
@@ -14,6 +14,7 @@
#include
#include
+#include
#include
#include
#include
@@ -183,6 +184,10 @@ static int stm_drm_platform_probe(struct
platform_device *pdev)
DRM_DEBUG("%s\n", __func__);
+ ret = drm_aperture_remove_framebuffers(false, _driver);
+ if (ret)
+ return ret;
+
Hi Yannick,
and many thanks for your patch.
Acked-by: Philippe Cornu
Philippe :-)
dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
ddev = drm_dev_alloc(_driver, dev);
Applied on drm-misc-next.
Many thanks for your patch,
Philippe :-)
Re: [PATCH] drm/bridge/tc358775: Fix for dual-link LVDS
Jiri: Are you able to test this patch?
Vinay: Could you supply a R-b tag, if you feel that it is warranted?
On Tue, 14 Dec 2021 at 09:13, Vinay Simha B N wrote:
>
> Robert,
> I do not have the hardware to test this feature. Sorry for the late response.
>
> On Thu, Nov 18, 2021 at 8:20 PM Robert Foss wrote:
>>
>> + Zhen & Vinay
>>
>> This patch looks good in itself, but I would like to see a tested by
>> tag. At the very least testing for regression in single-link LVDS but
>> ideally some third party verification of this patch.
>>
>> On Wed, 10 Nov 2021 at 23:01, Jiri Vanek wrote:
>> >
>> > Fixed wrong register shift for single/dual link LVDS output.
>> >
>> > Signed-off-by: Jiri Vanek
>> > ---
>> > drivers/gpu/drm/bridge/tc358775.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/bridge/tc358775.c
>> > b/drivers/gpu/drm/bridge/tc358775.c
>> > index 2272adcc5b4a..1d6ec1baeff2 100644
>> > --- a/drivers/gpu/drm/bridge/tc358775.c
>> > +++ b/drivers/gpu/drm/bridge/tc358775.c
>> > @@ -241,7 +241,7 @@ static inline u32 TC358775_LVCFG_PCLKDIV(uint32_t val)
>> > }
>> >
>> > #define TC358775_LVCFG_LVDLINK__MASK 0x0002
>> > -#define TC358775_LVCFG_LVDLINK__SHIFT0
>> > +#define TC358775_LVCFG_LVDLINK__SHIFT1
>> > static inline u32 TC358775_LVCFG_LVDLINK(uint32_t val)
>> > {
>> > return ((val) << TC358775_LVCFG_LVDLINK__SHIFT) &
>> > --
>> > 2.30.2
>> >
>
>
>
> --
> regards,
> vinaysimha
Re: [PATCH v2] drm/bridge: anx7625: Check GPIO description to avoid crash
Applied to drm-misc-next
Re: [PATCH v2] drm/bridge: anx7625: Check GPIO description to avoid crash
On Fri, 19 Nov 2021 at 02:58, Xin Ji wrote:
>
> As GPIO probe function "devm_gpiod_get_optional()" may return error
> code, driver should identify GPIO desc as NULL to avoid crash.
>
> Acked-by: Tzung-Bi Shih
> Signed-off-by: Xin Ji
> ---
> drivers/gpu/drm/bridge/analogix/anx7625.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 001fb39d9919..652ae814246d 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -1098,9 +1098,18 @@ static void anx7625_init_gpio(struct anx7625_data
> *platform)
> /* Gpio for chip power enable */
> platform->pdata.gpio_p_on =
> devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);
> + if (IS_ERR_OR_NULL(platform->pdata.gpio_p_on)) {
> + DRM_DEV_DEBUG_DRIVER(dev, "no enable gpio found\n");
> + platform->pdata.gpio_p_on = NULL;
> + }
> +
> /* Gpio for chip reset */
> platform->pdata.gpio_reset =
> devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR_OR_NULL(platform->pdata.gpio_reset)) {
> + DRM_DEV_DEBUG_DRIVER(dev, "no reset gpio found\n");
> + platform->pdata.gpio_reset = NULL;
> + }
>
> if (platform->pdata.gpio_p_on && platform->pdata.gpio_reset) {
> platform->pdata.low_power_mode = 1;
> --
> 2.25.1
>
Reviewed-by: Robert Foss
Re: [PATCH 1/2] drm/bridge: chipone-icn6211: Switch to atomic operations
Applied to drm-misc-next
Re: [PATCH 2/2] drm/bridge: chipone-icn6211: Add mode_set API
On Fri, 19 Nov 2021 at 15:53, Jagan Teki wrote:
>
> Get the display mode settings via mode_set bridge
> function instead of explicitly de-reference.
>
> Signed-off-by: Jagan Teki
> ---
> drivers/gpu/drm/bridge/chipone-icn6211.c | 18 --
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c
> b/drivers/gpu/drm/bridge/chipone-icn6211.c
> index 77b3e2c29461..e8f36dca56b3 100644
> --- a/drivers/gpu/drm/bridge/chipone-icn6211.c
> +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c
> @@ -31,6 +31,7 @@
> struct chipone {
> struct device *dev;
> struct drm_bridge bridge;
> + struct drm_display_mode mode;
> struct drm_bridge *panel_bridge;
> struct gpio_desc *enable_gpio;
> struct regulator *vdd1;
> @@ -43,11 +44,6 @@ static inline struct chipone *bridge_to_chipone(struct
> drm_bridge *bridge)
> return container_of(bridge, struct chipone, bridge);
> }
>
> -static struct drm_display_mode *bridge_to_mode(struct drm_bridge *bridge)
> -{
> - return >encoder->crtc->state->adjusted_mode;
> -}
> -
> static inline int chipone_dsi_write(struct chipone *icn, const void *seq,
> size_t len)
> {
> @@ -66,7 +62,7 @@ static void chipone_atomic_enable(struct drm_bridge *bridge,
> struct drm_bridge_state *old_bridge_state)
> {
> struct chipone *icn = bridge_to_chipone(bridge);
> - struct drm_display_mode *mode = bridge_to_mode(bridge);
> + struct drm_display_mode *mode = >mode;
>
> ICN6211_DSI(icn, 0x7a, 0xc1);
>
> @@ -165,6 +161,15 @@ static void chipone_atomic_post_disable(struct
> drm_bridge *bridge,
> gpiod_set_value(icn->enable_gpio, 0);
> }
>
> +static void chipone_mode_set(struct drm_bridge *bridge,
> +const struct drm_display_mode *mode,
> +const struct drm_display_mode *adjusted_mode)
> +{
> + struct chipone *icn = bridge_to_chipone(bridge);
> +
> + drm_mode_copy(>mode, adjusted_mode);
> +}
> +
> static int chipone_attach(struct drm_bridge *bridge, enum
> drm_bridge_attach_flags flags)
> {
> struct chipone *icn = bridge_to_chipone(bridge);
> @@ -179,6 +184,7 @@ static const struct drm_bridge_funcs chipone_bridge_funcs
> = {
> .atomic_pre_enable = chipone_atomic_pre_enable,
> .atomic_enable = chipone_atomic_enable,
> .atomic_post_disable= chipone_atomic_post_disable,
> + .mode_set = chipone_mode_set,
> .attach = chipone_attach,
> };
>
> --
> 2.25.1
>
Reviewed-by: Robert Foss
Re: [PATCH 1/2] drm/bridge: chipone-icn6211: Switch to atomic operations
On Sun, 19 Dec 2021 at 17:41, Jagan Teki wrote: > > On Fri, Nov 19, 2021 at 8:23 PM Jagan Teki wrote: > > > > Replace atomic version of the pre_enable/enable/post_disable > > operations to continue the transition to the atomic API. > > > > Also added default drm atomic operations for duplicate, destroy > > and reset state API's in order to have smooth transition on > > atomic API's. > > > > Tested on Allwinner R16/R40 DSI. > > > > Signed-off-by: Jagan Teki > > --- > > Gentle ping! Reviewed-by: Robert Foss
Re: [PATCH] drm/bridge: Fix free wrong object in sii8620_init_rcp_input_dev
Applied to drm-misc-next
Re: [PATCH] drm/bridge: Fix free wrong object in sii8620_init_rcp_input_dev
On Mon, 27 Dec 2021 at 10:25, Miaoqian Lin wrote:
>
> rc_dev is allocated by rc_allocate_device(), and doesn't assigned to
> ctx->rc_dev before calling rc_free_device(ctx->rc_dev).
> So it should call rc_free_device(rc_dev);
>
> Fixes: e25f1f7 ("drm/bridge/sii8620: add remote control support")
> Signed-off-by: Miaoqian Lin
> ---
> drivers/gpu/drm/bridge/sil-sii8620.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
> b/drivers/gpu/drm/bridge/sil-sii8620.c
> index 843265d7f1b1..ec7745c31da0 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -2120,7 +2120,7 @@ static void sii8620_init_rcp_input_dev(struct sii8620
> *ctx)
> if (ret) {
> dev_err(ctx->dev, "Failed to register RC device\n");
> ctx->error = ret;
> - rc_free_device(ctx->rc_dev);
> + rc_free_device(rc_dev);
> return;
> }
> ctx->rc_dev = rc_dev;
> --
Reviewed-by: Robert Foss
[PATCH v5 6/6] drm/i915: Use struct vma_resource instead of struct vma_snapshot
There is always a struct vma_resource guaranteed to be alive when we
access a corresponding struct vma_snapshot.
So ditch the latter and instead of allocating vma_snapshots, reference
the already existning vma_resource.
This requires a couple of extra members in struct vma_resource but that's
a small price to pay for the simplification.
v2:
- Fix a missing include and declaration (kernel test robot )
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/Makefile | 1 -
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 15 +--
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 9 +-
drivers/gpu/drm/i915/i915_gpu_error.c | 87 ++--
drivers/gpu/drm/i915/i915_request.c | 12 +-
drivers/gpu/drm/i915/i915_request.h | 6 +-
drivers/gpu/drm/i915/i915_vma.c | 16 +--
drivers/gpu/drm/i915/i915_vma_resource.c | 4 +
drivers/gpu/drm/i915/i915_vma_resource.h | 28 +++-
drivers/gpu/drm/i915/i915_vma_snapshot.c | 125 --
drivers/gpu/drm/i915/i915_vma_snapshot.h | 101 --
11 files changed, 90 insertions(+), 314 deletions(-)
delete mode 100644 drivers/gpu/drm/i915/i915_vma_snapshot.c
delete mode 100644 drivers/gpu/drm/i915/i915_vma_snapshot.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 98433ad74194..aa86ac33effc 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -175,7 +175,6 @@ i915-y += \
i915_ttm_buddy_manager.o \
i915_vma.o \
i915_vma_resource.o \
- i915_vma_snapshot.o \
intel_wopcm.o
# general-purpose microcontroller (GuC) support
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 72e497745c12..2f85fe557ad2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -29,7 +29,6 @@
#include "i915_gem_ioctls.h"
#include "i915_trace.h"
#include "i915_user_extensions.h"
-#include "i915_vma_snapshot.h"
struct eb_vma {
struct i915_vma *vma;
@@ -1952,7 +1951,6 @@ static void eb_capture_stage(struct i915_execbuffer *eb)
{
const unsigned int count = eb->buffer_count;
unsigned int i = count, j;
- struct i915_vma_snapshot *vsnap;
while (i--) {
struct eb_vma *ev = >vma[i];
@@ -1962,11 +1960,6 @@ static void eb_capture_stage(struct i915_execbuffer *eb)
if (!(flags & EXEC_OBJECT_CAPTURE))
continue;
- vsnap = i915_vma_snapshot_alloc(GFP_KERNEL);
- if (!vsnap)
- continue;
-
- i915_vma_snapshot_init(vsnap, vma, "user");
for_each_batch_create_order(eb, j) {
struct i915_capture_list *capture;
@@ -1975,10 +1968,9 @@ static void eb_capture_stage(struct i915_execbuffer *eb)
continue;
capture->next = eb->capture_lists[j];
- capture->vma_snapshot = i915_vma_snapshot_get(vsnap);
+ capture->vma_res = i915_vma_resource_get(vma->resource);
eb->capture_lists[j] = capture;
}
- i915_vma_snapshot_put(vsnap);
}
}
@@ -3281,9 +3273,8 @@ eb_requests_create(struct i915_execbuffer *eb, struct
dma_fence *in_fence,
* _onstack interface.
*/
if (eb->batches[i]->vma)
-
i915_vma_snapshot_init_onstack(>requests[i]->batch_snapshot,
- eb->batches[i]->vma,
- "batch");
+ eb->requests[i]->batch_res =
+
i915_vma_resource_get(eb->batches[i]->vma->resource);
if (eb->batch_pool) {
GEM_BUG_ON(intel_context_is_parallel(eb->context));
intel_gt_buffer_pool_mark_active(eb->batch_pool,
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 74aa90587061..d1daa4cc2895 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -1708,18 +1708,15 @@ static void intel_engine_print_registers(struct
intel_engine_cs *engine,
static void print_request_ring(struct drm_printer *m, struct i915_request *rq)
{
- struct i915_vma_snapshot *vsnap = >batch_snapshot;
+ struct i915_vma_resource *vma_res = rq->batch_res;
void *ring;
int size;
- if (!i915_vma_snapshot_present(vsnap))
- vsnap = NULL;
-
drm_printf(m,
"[head %04x, postfix %04x, tail %04x, batch 0x%08x_%08x]:\n",
rq->head, rq->postfix, rq->tail,
- vsnap ?
[PATCH v5 5/6] drm/i915: Asynchronous migration selftest
Add a selftest to exercise asynchronous migration and -unbining.
Extend the gem_migrate selftest to perform the migrations while
depending on a spinner and a bound vma set up on the migrated
buffer object.
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/gem/i915_gem_object.c| 12 ++
drivers/gpu/drm/i915/gem/i915_gem_object.h| 3 +
.../drm/i915/gem/selftests/i915_gem_migrate.c | 192 --
3 files changed, 192 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c
b/drivers/gpu/drm/i915/gem/i915_gem_object.c
index d87b508b59b1..1a9e1f940a7d 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
@@ -756,6 +756,18 @@ 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)
+{
+ struct dma_fence **moving = _gem_to_ttm(obj)->moving;
+
+ if (*moving == fence)
+ return;
+
+ dma_fence_put(*moving);
+ *moving = dma_fence_get(fence);
+}
+
/**
* i915_gem_object_wait_moving_fence - Wait for the object's moving fence if
any
* @obj: The object whose moving fence to wait for.
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
b/drivers/gpu/drm/i915/gem/i915_gem_object.h
index f66d46882ea7..1d178236 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
+++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
@@ -524,6 +524,9 @@ i915_gem_object_finish_access(struct drm_i915_gem_object
*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);
diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
index ecb691c81d1e..d534141b2cf7 100644
--- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
+++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_migrate.c
@@ -4,8 +4,13 @@
*/
#include "gt/intel_migrate.h"
+#include "gt/intel_gpu_commands.h"
#include "gem/i915_gem_ttm_move.h"
+#include "i915_deps.h"
+
+#include "selftests/igt_spinner.h"
+
static int igt_fill_check_buffer(struct drm_i915_gem_object *obj,
bool fill)
{
@@ -101,7 +106,8 @@ static int igt_same_create_migrate(void *arg)
}
static int lmem_pages_migrate_one(struct i915_gem_ww_ctx *ww,
- struct drm_i915_gem_object *obj)
+ struct drm_i915_gem_object *obj,
+ struct i915_vma *vma)
{
int err;
@@ -109,6 +115,24 @@ static int lmem_pages_migrate_one(struct i915_gem_ww_ctx
*ww,
if (err)
return err;
+ if (vma) {
+ err = i915_vma_pin_ww(vma, ww, obj->base.size, 0,
+ 0UL | PIN_OFFSET_FIXED |
+ PIN_USER);
+ if (err) {
+ if (err != -EINTR && err != ERESTARTSYS &&
+ err != -EDEADLK)
+ pr_err("Failed to pin vma.\n");
+ return err;
+ }
+
+ i915_vma_unpin(vma);
+ }
+
+ /*
+* Migration will implicitly unbind (asynchronously) any bound
+* vmas.
+*/
if (i915_gem_object_is_lmem(obj)) {
err = i915_gem_object_migrate(obj, ww, INTEL_REGION_SMEM);
if (err) {
@@ -149,11 +173,15 @@ static int lmem_pages_migrate_one(struct i915_gem_ww_ctx
*ww,
return err;
}
-static int igt_lmem_pages_migrate(void *arg)
+static int __igt_lmem_pages_migrate(struct intel_gt *gt,
+ struct i915_address_space *vm,
+ struct i915_deps *deps,
+ struct igt_spinner *spin,
+ struct dma_fence *spin_fence)
{
- struct intel_gt *gt = arg;
struct drm_i915_private *i915 = gt->i915;
struct drm_i915_gem_object *obj;
+ struct i915_vma *vma = NULL;
struct i915_gem_ww_ctx ww;
struct i915_request *rq;
int err;
@@ -165,6 +193,14 @@ static int igt_lmem_pages_migrate(void *arg)
if (IS_ERR(obj))
return PTR_ERR(obj);
+ if (vm) {
+ vma = i915_vma_instance(obj, vm, NULL);
+ if (IS_ERR(vma)) {
+ err = PTR_ERR(vma);
+ goto out_put;
+ }
+ }
+
/* Initial GPU fill, sync, CPU initialization. */
[PATCH v5 4/6] drm/i915: Use vma resources for async unbinding
Implement async (non-blocking) unbinding by not syncing the vma before
calling unbind on the vma_resource.
Add the resulting unbind fence to the object's dma_resv from where it is
picked up by the ttm migration code.
Ideally these unbind fences should be coalesced with the migration blit
fence to avoid stalling the migration blit waiting for unbind, as they
can certainly go on in parallel, but since we don't yet have a
reasonable data structure to use to coalesce fences and attach the
resulting fence to a timeline, we defer that for now.
Note that with async unbinding, even while the unbind waits for the
preceding bind to complete before unbinding, the vma itself might have been
destroyed in the process, clearing the vma pages. Therefore we can
only allow async unbinding if we have a refcounted sg-list and keep a
refcount on that for the vma resource pages to stay intact until
binding occurs. If this condition is not met, a request for an async
unbind is diverted to a sync unbind.
v2:
- Use a separate kmem_cache for vma resources for now to isolate their
memory allocation and aid debugging.
- Move the check for vm closed to the actual unbinding thread. Regardless
of whether the vm is closed, we need the unbind fence to properly wait
for capture.
- Clear vma_res::vm on unbind and update its documentation.
v4:
- Take cache coloring into account when searching for vma resources
pending unbind. (Matthew Auld)
v5:
- Fix timeout and error check in i915_vma_resource_bind_dep_await().
- Avoid taking a reference on the object for async binding if
async unbind capable.
- Fix braces around a single-line if statement.
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 11 +-
drivers/gpu/drm/i915/gt/intel_ggtt.c | 2 +-
drivers/gpu/drm/i915/gt/intel_gtt.c | 4 +
drivers/gpu/drm/i915/gt/intel_gtt.h | 3 +
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_gem.c | 12 +-
drivers/gpu/drm/i915/i915_module.c | 3 +
drivers/gpu/drm/i915/i915_vma.c | 204 +--
drivers/gpu/drm/i915/i915_vma.h | 3 +-
drivers/gpu/drm/i915/i915_vma_resource.c | 354 +--
drivers/gpu/drm/i915/i915_vma_resource.h | 48 +++
11 files changed, 578 insertions(+), 67 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
index ee9612a3ee5e..0f514435b9c5 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -142,7 +142,16 @@ int i915_ttm_move_notify(struct ttm_buffer_object *bo)
struct drm_i915_gem_object *obj = i915_ttm_to_gem(bo);
int ret;
- ret = i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE);
+ /*
+* Note: The async unbinding here will actually transform the
+* blocking wait for unbind into a wait before finally submitting
+* evict / migration blit and thus stall the migration timeline
+* which may not be good for overall throughput. We should make
+* sure we await the unbind fences *after* the migration blit
+* instead of *before* as we currently do.
+*/
+ ret = i915_gem_object_unbind(obj, I915_GEM_OBJECT_UNBIND_ACTIVE |
+I915_GEM_OBJECT_UNBIND_ASYNC);
if (ret)
return ret;
diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
b/drivers/gpu/drm/i915/gt/intel_ggtt.c
index 0137b6af0973..ae7bbd8914c1 100644
--- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
@@ -142,7 +142,7 @@ void i915_ggtt_suspend_vm(struct i915_address_space *vm)
continue;
if (!i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) {
- __i915_vma_evict(vma);
+ __i915_vma_evict(vma, false);
drm_mm_remove_node(>node);
}
}
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.c
b/drivers/gpu/drm/i915/gt/intel_gtt.c
index a94be0306464..46be4197b93f 100644
--- a/drivers/gpu/drm/i915/gt/intel_gtt.c
+++ b/drivers/gpu/drm/i915/gt/intel_gtt.c
@@ -161,6 +161,9 @@ static void __i915_vm_release(struct work_struct *work)
struct i915_address_space *vm =
container_of(work, struct i915_address_space, release_work);
+ /* Synchronize async unbinds. */
+ i915_vma_resource_bind_dep_sync_all(vm);
+
vm->cleanup(vm);
i915_address_space_fini(vm);
@@ -189,6 +192,7 @@ void i915_address_space_init(struct i915_address_space *vm,
int subclass)
if (!kref_read(>resv_ref))
kref_init(>resv_ref);
+ vm->pending_unbind = RB_ROOT_CACHED;
INIT_WORK(>release_work, __i915_vm_release);
atomic_set(>open, 1);
diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
b/drivers/gpu/drm/i915/gt/intel_gtt.h
[PATCH v5 3/6] drm/i915: Don't pin the object pages during pending vma binds
A pin-count is already held by vma->pages so taking an additional pin
during async binds is not necessary.
When we introduce async unbinding we have other means of keeping the
object pages alive.
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/i915_vma.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 1d4e448d22d9..8fa3e0b2fe26 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -305,10 +305,8 @@ static void __vma_release(struct dma_fence_work *work)
{
struct i915_vma_work *vw = container_of(work, typeof(*vw), base);
- if (vw->pinned) {
- __i915_gem_object_unpin_pages(vw->pinned);
+ if (vw->pinned)
i915_gem_object_put(vw->pinned);
- }
i915_vm_free_pt_stash(vw->vm, >stash);
i915_vm_put(vw->vm);
@@ -477,7 +475,6 @@ int i915_vma_bind(struct i915_vma *vma,
work->base.dma.error = 0; /* enable the queue_work() */
- __i915_gem_object_pin_pages(vma->obj);
work->pinned = i915_gem_object_get(vma->obj);
} else {
if (vma->obj) {
--
2.31.1
[PATCH v5 2/6] drm/i915: Use the vma resource as argument for gtt binding / unbinding
When introducing asynchronous unbinding, the vma itself may no longer
be alive when the actual binding or unbinding takes place.
Update the gtt i915_vma_ops accordingly to take a struct i915_vma_resource
instead of a struct i915_vma for the bind_vma() and unbind_vma() ops.
Similarly change the insert_entries() op for struct i915_address_space.
Replace a couple of i915_vma_snapshot members with their newly introduced
i915_vma_resource counterparts, since they have the same lifetime.
Also make sure to avoid changing the struct i915_vma_flags (in particular
the bind flags) async. That should now only be done sync under the
vm mutex.
v2:
- Update the vma_res::bound_flags when binding to the aliased ggtt
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/display/intel_dpt.c | 27 ++---
.../gpu/drm/i915/gem/i915_gem_object_types.h | 27 +
.../gpu/drm/i915/gem/selftests/huge_pages.c | 37 +++
drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 19 ++--
drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 37 +++
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 4 +-
drivers/gpu/drm/i915/gt/intel_ggtt.c | 70 ++---
drivers/gpu/drm/i915/gt/intel_gtt.h | 16 +--
drivers/gpu/drm/i915/gt/intel_ppgtt.c | 22 +++--
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 13 ++-
drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 2 +-
drivers/gpu/drm/i915/i915_debugfs.c | 3 +-
drivers/gpu/drm/i915/i915_gpu_error.c | 6 +-
drivers/gpu/drm/i915/i915_vma.c | 24 -
drivers/gpu/drm/i915/i915_vma.h | 11 +--
drivers/gpu/drm/i915/i915_vma_resource.c | 9 +-
drivers/gpu/drm/i915/i915_vma_resource.h | 99 ++-
drivers/gpu/drm/i915/i915_vma_snapshot.c | 4 -
drivers/gpu/drm/i915/i915_vma_snapshot.h | 8 --
drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 64
drivers/gpu/drm/i915/selftests/mock_gtt.c | 12 +--
21 files changed, 308 insertions(+), 206 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c
b/drivers/gpu/drm/i915/display/intel_dpt.c
index 8f674745e7e0..63a83d5f85a1 100644
--- a/drivers/gpu/drm/i915/display/intel_dpt.c
+++ b/drivers/gpu/drm/i915/display/intel_dpt.c
@@ -48,7 +48,7 @@ static void dpt_insert_page(struct i915_address_space *vm,
}
static void dpt_insert_entries(struct i915_address_space *vm,
- struct i915_vma *vma,
+ struct i915_vma_resource *vma_res,
enum i915_cache_level level,
u32 flags)
{
@@ -64,8 +64,8 @@ static void dpt_insert_entries(struct i915_address_space *vm,
* not to allow the user to override access to a read only page.
*/
- i = vma->node.start / I915_GTT_PAGE_SIZE;
- for_each_sgt_daddr(addr, sgt_iter, vma->pages)
+ i = vma_res->start / I915_GTT_PAGE_SIZE;
+ for_each_sgt_daddr(addr, sgt_iter, vma_res->bi.pages)
gen8_set_pte([i++], pte_encode | addr);
}
@@ -76,35 +76,38 @@ static void dpt_clear_range(struct i915_address_space *vm,
static void dpt_bind_vma(struct i915_address_space *vm,
struct i915_vm_pt_stash *stash,
-struct i915_vma *vma,
+struct i915_vma_resource *vma_res,
enum i915_cache_level cache_level,
u32 flags)
{
- struct drm_i915_gem_object *obj = vma->obj;
u32 pte_flags;
+ if (vma_res->bound_flags)
+ return;
+
/* Applicable to VLV (gen8+ do not support RO in the GGTT) */
pte_flags = 0;
- if (vma->vm->has_read_only && i915_gem_object_is_readonly(obj))
+ if (vm->has_read_only && vma_res->bi.readonly)
pte_flags |= PTE_READ_ONLY;
- if (i915_gem_object_is_lmem(obj))
+ if (vma_res->bi.lmem)
pte_flags |= PTE_LM;
- vma->vm->insert_entries(vma->vm, vma, cache_level, pte_flags);
+ vm->insert_entries(vm, vma_res, cache_level, pte_flags);
- vma->page_sizes.gtt = I915_GTT_PAGE_SIZE;
+ vma_res->page_sizes_gtt = I915_GTT_PAGE_SIZE;
/*
* Without aliasing PPGTT there's no difference between
* GLOBAL/LOCAL_BIND, it's all the same ptes. Hence unconditionally
* upgrade to both bound if we bind either to avoid double-binding.
*/
- atomic_or(I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND, >flags);
+ vma_res->bound_flags = I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND;
}
-static void dpt_unbind_vma(struct i915_address_space *vm, struct i915_vma *vma)
+static void dpt_unbind_vma(struct i915_address_space *vm,
+ struct i915_vma_resource *vma_res)
{
- vm->clear_range(vm, vma->node.start, vma->size);
+ vm->clear_range(vm, vma_res->start, vma_res->vma_size);
}
static void dpt_cleanup(struct
[PATCH v5 1/6] drm/i915: Initial introduction of vma resources
Introduce vma resources, sort of similar to TTM resources, needed for
asynchronous bind management. Initially we will use them to hold
completion of unbinding when we capture data from a vma, but they will
be used extensively in upcoming patches for asynchronous vma unbinding.
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/Makefile | 1 +
.../gpu/drm/i915/gem/i915_gem_execbuffer.c| 2 +-
drivers/gpu/drm/i915/i915_vma.c | 55 +++-
drivers/gpu/drm/i915/i915_vma.h | 19 ++-
drivers/gpu/drm/i915/i915_vma_resource.c | 124 ++
drivers/gpu/drm/i915/i915_vma_resource.h | 70 ++
drivers/gpu/drm/i915/i915_vma_snapshot.c | 15 +--
drivers/gpu/drm/i915/i915_vma_snapshot.h | 7 +-
drivers/gpu/drm/i915/i915_vma_types.h | 5 +
drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 99 --
10 files changed, 334 insertions(+), 63 deletions(-)
create mode 100644 drivers/gpu/drm/i915/i915_vma_resource.c
create mode 100644 drivers/gpu/drm/i915/i915_vma_resource.h
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 1b62b9f65196..98433ad74194 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -174,6 +174,7 @@ i915-y += \
i915_trace_points.o \
i915_ttm_buddy_manager.o \
i915_vma.o \
+ i915_vma_resource.o \
i915_vma_snapshot.o \
intel_wopcm.o
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index e9541244027a..72e497745c12 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1422,7 +1422,7 @@ eb_relocate_entry(struct i915_execbuffer *eb,
mutex_lock(>vm->mutex);
err = i915_vma_bind(target->vma,
target->vma->obj->cache_level,
- PIN_GLOBAL, NULL);
+ PIN_GLOBAL, NULL, NULL);
mutex_unlock(>vm->mutex);
reloc_cache_remap(>reloc_cache, ev->vma->obj);
if (err)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index be208a8f1ed0..7097c5016431 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -37,6 +37,7 @@
#include "i915_sw_fence_work.h"
#include "i915_trace.h"
#include "i915_vma.h"
+#include "i915_vma_resource.h"
static struct kmem_cache *slab_vmas;
@@ -380,6 +381,8 @@ static int i915_vma_verify_bind_complete(struct i915_vma
*vma)
* @cache_level: mapping cache level
* @flags: flags like global or local mapping
* @work: preallocated worker for allocating and binding the PTE
+ * @vma_res: pointer to a preallocated vma resource. The resource is either
+ * consumed or freed.
*
* DMA addresses are taken from the scatter-gather table of this object (or of
* this VMA in case of non-default GGTT views) and PTE entries set up.
@@ -388,7 +391,8 @@ static int i915_vma_verify_bind_complete(struct i915_vma
*vma)
int i915_vma_bind(struct i915_vma *vma,
enum i915_cache_level cache_level,
u32 flags,
- struct i915_vma_work *work)
+ struct i915_vma_work *work,
+ struct i915_vma_resource *vma_res)
{
u32 bind_flags;
u32 vma_flags;
@@ -399,11 +403,15 @@ int i915_vma_bind(struct i915_vma *vma,
if (GEM_DEBUG_WARN_ON(range_overflows(vma->node.start,
vma->node.size,
- vma->vm->total)))
+ vma->vm->total))) {
+ kfree(vma_res);
return -ENODEV;
+ }
- if (GEM_DEBUG_WARN_ON(!flags))
+ if (GEM_DEBUG_WARN_ON(!flags)) {
+ kfree(vma_res);
return -EINVAL;
+ }
bind_flags = flags;
bind_flags &= I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND;
@@ -412,11 +420,21 @@ int i915_vma_bind(struct i915_vma *vma,
vma_flags &= I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND;
bind_flags &= ~vma_flags;
- if (bind_flags == 0)
+ if (bind_flags == 0) {
+ kfree(vma_res);
return 0;
+ }
GEM_BUG_ON(!atomic_read(>pages_count));
+ if (vma->resource || !vma_res) {
+ /* Rebinding with an additional I915_VMA_*_BIND */
+ GEM_WARN_ON(!vma_flags);
+ kfree(vma_res);
+ } else {
+ i915_vma_resource_init(vma_res);
+ vma->resource = vma_res;
+ }
trace_i915_vma_bind(vma, bind_flags);
if (work && bind_flags & vma->vm->bind_async_flags) {
struct dma_fence *prev;
@@ -1279,6
[PATCH v5 0/6] drm/i915: Asynchronous vma unbinding
This patch series introduces infrastructure for asynchronous vma unbinding. The single enabled use-case is initially at buffer object migration where we otherwise sync when unbinding vmas before migration. This in theory allows us to pipeline any number of migrations, but in practice the number is restricted by a sync wait when filling the migration context ring. We might want to look at that moving forward if needed. The other main use-case is to be able to pipeline vma evictions, for example with softpinning where a new vma wants to reuse the vm range of an already active vma. We can't support this just yet because we need dma_resv locking around vma eviction for that, which is under implementation. Patch 1 introduces vma resource first for error capture purposes Patch 2 changes the vm backend interface to take vma resources rather than vmas Patch 3 removes and unneeded page pinning Patch 4 introduces the async unbinding itself, and finally Patch 5 introduces a selftest Patch 6 realizes we have duplicated functionality and removes the vma snapshots v2: -- Some kernel test robot reports addressed. -- kmem cache for vma resources, See patch 4 -- Various fixes all over the place. See separate commit messages. v3: -- Re-add a missing i915_vma_resource_put() -- Remove a stray debug printout v4: -- Patch series split in two. This is the second part. -- Take cache coloring into account when searching for vma_resources pending unbind. (Matthew Auld) v5: -- Add a selftest. -- Remove page pinning while sync binding. -- A couple of fixes in i915_vma_resource_bind_dep_await() Thomas Hellström (6): drm/i915: Initial introduction of vma resources drm/i915: Use the vma resource as argument for gtt binding / unbinding drm/i915: Don't pin the object pages during pending vma binds drm/i915: Use vma resources for async unbinding drm/i915: Asynchronous migration selftest drm/i915: Use struct vma_resource instead of struct vma_snapshot drivers/gpu/drm/i915/Makefile | 2 +- drivers/gpu/drm/i915/display/intel_dpt.c | 27 +- .../gpu/drm/i915/gem/i915_gem_execbuffer.c| 17 +- drivers/gpu/drm/i915/gem/i915_gem_object.c| 12 + drivers/gpu/drm/i915/gem/i915_gem_object.h| 3 + .../gpu/drm/i915/gem/i915_gem_object_types.h | 27 +- drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 11 +- .../gpu/drm/i915/gem/selftests/huge_pages.c | 37 +- .../drm/i915/gem/selftests/i915_gem_migrate.c | 192 +++- drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 19 +- drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 37 +- drivers/gpu/drm/i915/gt/intel_engine_cs.c | 9 +- drivers/gpu/drm/i915/gt/intel_ggtt.c | 72 +-- drivers/gpu/drm/i915/gt/intel_gtt.c | 4 + drivers/gpu/drm/i915/gt/intel_gtt.h | 19 +- drivers/gpu/drm/i915/gt/intel_ppgtt.c | 22 +- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 13 +- drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h | 2 +- drivers/gpu/drm/i915/i915_debugfs.c | 3 +- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 12 +- drivers/gpu/drm/i915/i915_gpu_error.c | 87 ++-- drivers/gpu/drm/i915/i915_module.c| 3 + drivers/gpu/drm/i915/i915_request.c | 12 +- drivers/gpu/drm/i915/i915_request.h | 6 +- drivers/gpu/drm/i915/i915_vma.c | 240 +- drivers/gpu/drm/i915/i915_vma.h | 33 +- drivers/gpu/drm/i915/i915_vma_resource.c | 417 ++ drivers/gpu/drm/i915/i915_vma_resource.h | 235 ++ drivers/gpu/drm/i915/i915_vma_snapshot.c | 134 -- drivers/gpu/drm/i915/i915_vma_snapshot.h | 112 - drivers/gpu/drm/i915/i915_vma_types.h | 5 + drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 159 --- drivers/gpu/drm/i915/selftests/mock_gtt.c | 12 +- 34 files changed, 1415 insertions(+), 581 deletions(-) create mode 100644 drivers/gpu/drm/i915/i915_vma_resource.c create mode 100644 drivers/gpu/drm/i915/i915_vma_resource.h delete mode 100644 drivers/gpu/drm/i915/i915_vma_snapshot.c delete mode 100644 drivers/gpu/drm/i915/i915_vma_snapshot.h -- 2.31.1
Re: [PATCH 1/3] drm/stm: dsi: move lane capability detection in probe()
On Tue, 4 Jan 2022 at 11:47, Philippe CORNU wrote: > > > > On 12/18/21 10:50 PM, Antonio Borneo wrote: > > There is no need to re-compute the dsi lane capability because it > > only depends on dsi hw version. > > Since dsi hw version is detected at probe(), move there also the > > assignment of dsi lane capability. > > > > Signed-off-by: Antonio Borneo > > --- > > To: David Airlie > > To: Daniel Vetter > > To: Andrzej Hajda > > To: Neil Armstrong > > To: Robert Foss > > To: Laurent Pinchart > > To: Jonas Karlman > > To: Jernej Skrabec > > To: Yannick Fertre > > To: Philippe Cornu > > To: Benjamin Gaignard > > To: Maxime Coquelin > > To: Alexandre Torgue > > To: Philipp Zabel > > To: dri-devel@lists.freedesktop.org > > To: linux-st...@st-md-mailman.stormreply.com > > To: linux-arm-ker...@lists.infradead.org > > Cc: linux-ker...@vger.kernel.org > > --- > > drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 16 > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > Hi Antonio, > many thanks for your patch. > Acked-by: Philippe Cornu > Reviewed-by: Philippe Cornu > Philippe :-) Thanks for the series and the Acks. Applied series to drm-misc-next
Re: [PATCH 5/5] drm/vc4: dpi: Support DPI interface in mode3 for RGB565
Hi Chris
Thanks for the patch.
On Mon, 3 Jan 2022 at 17:41, Chris Morgan wrote:
>
> From: Chris Morgan
>
> Add support for the VC4 DPI driver to utilize DPI mode 3. This is
> defined here as xxxRxxGGxxxB:
>
> https://www.raspberrypi.com/documentation/computers/raspberry-pi.html#parallel-display-interface-dpi
>
> This mode is required to use the Geekworm MZP280 DPI display.
>
> Signed-off-by: Chris Morgan
Reviewed-by: Dave Stevenson
The other patches all look valid to me, but I'll leave those for the
more experienced maintainers.
Dave
> ---
> drivers/gpu/drm/vc4/vc4_dpi.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
> index c180eb60bee8..3c58ade2549e 100644
> --- a/drivers/gpu/drm/vc4/vc4_dpi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dpi.c
> @@ -173,6 +173,10 @@ static void vc4_dpi_encoder_enable(struct drm_encoder
> *encoder)
> dpi_c |= VC4_SET_FIELD(DPI_FORMAT_16BIT_565_RGB_3,
>DPI_FORMAT);
> break;
> + case MEDIA_BUS_FMT_RGB565_1X24_CPADHI:
> + dpi_c |= VC4_SET_FIELD(DPI_FORMAT_16BIT_565_RGB_2,
> + DPI_FORMAT);
> + break;
> default:
> DRM_ERROR("Unknown media bus format %d\n",
> bus_format);
> break;
> --
> 2.25.1
>
Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV
Am 04.01.22 um 11:49 schrieb Liu, Monk: [AMD Official Use Only] See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. No it's not, FLR from hypervisor is just to notify guest the hw VF FLR is about to start or was already executed, but host will do FLR anyway without waiting for guest too long Then we have a major design issue in the SRIOV protocol and really need to question this. How do you want to prevent a race between the hypervisor resetting the hardware and the client trying the same because of a timeout? As far as I can see the procedure should be: 1. We detect that a reset is necessary, either because of a fault a timeout or signal from hypervisor. 2. For each of those potential reset sources a work item is send to the single workqueue. 3. One of those work items execute first and prepares the reset. 4. We either do the reset our self or notify the hypervisor that we are ready for the reset. 5. Cleanup after the reset, eventually resubmit jobs etc.. 6. Cancel work items which might have been scheduled from other reset sources. It does make sense that the hypervisor resets the hardware without waiting for the clients for too long, but if we don't follow this general steps we will always have a race between the different components. Regards, Christian. Am 04.01.22 um 11:49 schrieb Liu, Monk: [AMD Official Use Only] See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. No it's not, FLR from hypervisor is just to notify guest the hw VF FLR is about to start or was already executed, but host will do FLR anyway without waiting for guest too long In other words I strongly think that the current SRIOV reset implementation is severely broken and what Andrey is doing is actually fixing it. It makes the code to crash ... how could it be a fix ? I'm afraid the patch is NAK from me, but it is welcome if the cleanup do not ruin the logic, Andry or jingwen can try it if needed. Thanks --- Monk Liu | Cloud GPU & Virtualization Solution | AMD --- we are hiring software manager for CVS core team --- -Original Message- From: Koenig, Christian Sent: Tuesday, January 4, 2022 6:19 PM To: Chen, JingWen ; Christian König ; Grodzovsky, Andrey ; Deng, Emily ; Liu, Monk ; dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; Chen, Horace ; Chen, JingWen Cc: dan...@ffwll.ch Subject: Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV Hi Jingwen, well what I mean is that we need to adjust the implementation in amdgpu to actually match the requirements. Could be that the reset sequence is questionable in general, but I doubt so at least for now. See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. Properly setting in_gpu_reset is indeed mandatory, but should happen at a central place and not in the SRIOV specific code. In other words I strongly think that the current SRIOV reset implementation is severely broken and what Andrey is doing is actually fixing it. Regards, Christian. Am 04.01.22 um 10:07 schrieb JingWen Chen: Hi Christian, I'm not sure what do you mean by "we need to change SRIOV not the driver". Do you mean we should change the reset sequence in SRIOV? This will be a huge change for our SRIOV solution. From my point of view, we can directly use amdgpu_device_lock_adev and amdgpu_device_unlock_adev in flr_work instead of try_lock since no one will conflict with this thread with reset_domain introduced. But we do need the reset_sem and adev->in_gpu_reset to keep device untouched via user space. Best Regards, Jingwen Chen On 2022/1/3 下午6:17, Christian König wrote: Please don't. This patch is vital to the cleanup of the reset procedure. If SRIOV doesn't work with that we need to change SRIOV and not the driver. Christian. Am 30.12.21 um 19:45 schrieb Andrey Grodzovsky: Sure, I guess i can drop this patch then. Andrey On 2021-12-24 4:57 a.m., JingWen Chen wrote: I do agree with shaoyun, if the host find the gpu engine hangs first, and do the flr, guest side thread may not know this and still try to access HW(e.g. kfd is using a lot of amdgpu_in_reset and reset_sem to identify the reset status). And this may lead to very bad result. On 2021/12/24 下午4:58, Deng, Emily wrote: These patches look good to me. JingWen
[Bug 215436] admgpu: suspend and resuming from suspend don't work
https://bugzilla.kernel.org/show_bug.cgi?id=215436 --- Comment #22 from spassw...@web.de --- I get similar message if I just lock the screen which then switches off: [ 1732.526162] amdgpu :08:00.0: amdgpu: Calling dc_commit_state from amdgpu_dm_atomic_commit_tail [ 1732.640992] amdgpu :08:00.0: amdgpu: Calling optimize_bandwidth from dc_commit_state_no_check [ 1732.641005] amdgpu :08:00.0: amdgpu: Calling update_clocks from dcn20_optimize_bandwidth [ 1732.641007] amdgpu :08:00.0: amdgpu: adev->in_s0ix = 0 [ 1732.641008] amdgpu :08:00.0: amdgpu: calling rn_vbios_smu_set_dcn_low_power -- 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] gpu/drm: fix potential memleak in error branch
On Tue, 04 Jan 2022, 赵军奎 wrote:
> -邮件原件-
> 发件人: bern...@vivo.com 代表 Jani Nikula
> 发送时间: 2021年12月31日 19:09
> 收件人: 赵军奎 ; Maarten Lankhorst
> ; Maxime Ripard ;
> Thomas Zimmermann ; David Airlie ;
> Daniel Vetter ; dri-devel@lists.freedesktop.org;
> linux-ker...@vger.kernel.org
> 抄送: 赵军奎
> 主题: Re: [PATCH] gpu/drm: fix potential memleak in error branch
>
> On Tue, 16 Nov 2021, Bernard Zhao wrote:
>> This patch try to fix potential memleak in error branch.
>
>>Please elaborate.
>
> Hi Jani:
>
> This patch try to fix potential memleak in error branch.
> For example:
> nv50_sor_create ->nv50_mstm_new-> drm_dp_mst_topology_mgr_init
> In function drm_dp_mst_topology_mgr_init, there are five error branches,
> error branch just return error code, no free called.
> And we see that the caller didn`t do the drm_dp_mst_topology_mgr_destroy job.
> I am not sure if there some gap, I think this may bring in the risk of
> memleak issue.
> Thanks!
This should be part of the commit message.
>
> BR//Bernard
>
>>BR,
>>Jani.
>
>
>>
>> Signed-off-by: Bernard Zhao
>> ---
>> drivers/gpu/drm/drm_dp_mst_topology.c | 22 --
>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>> b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index f3d79eda94bb..f73b180dee73 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -5501,7 +5501,10 @@ int drm_dp_mst_topology_mgr_init(struct
>> drm_dp_mst_topology_mgr *mgr,
>> int max_lane_count, int max_link_rate,
>> int conn_base_id)
>> {
>> -struct drm_dp_mst_topology_state *mst_state;
>> +struct drm_dp_mst_topology_state *mst_state = NULL;
This is superfluous.
Other than that,
Reviewed-by: Jani Nikula
>> +
>> +mgr->payloads = NULL;
>> +mgr->proposed_vcpis = NULL;
>>
>> mutex_init(>lock);
>> mutex_init(>qlock);
>> @@ -5523,7 +5526,7 @@ int drm_dp_mst_topology_mgr_init(struct
>> drm_dp_mst_topology_mgr *mgr,
>> */
>> mgr->delayed_destroy_wq = alloc_ordered_workqueue("drm_dp_mst_wq", 0);
>> if (mgr->delayed_destroy_wq == NULL)
>> -return -ENOMEM;
>> +goto out;
>>
>> INIT_WORK(>work, drm_dp_mst_link_probe_work);
>> INIT_WORK(>tx_work, drm_dp_tx_work); @@ -5539,18 +5542,18 @@
>> int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
>> mgr->conn_base_id = conn_base_id;
>> if (max_payloads + 1 > sizeof(mgr->payload_mask) * 8 ||
>> max_payloads + 1 > sizeof(mgr->vcpi_mask) * 8)
>> -return -EINVAL;
>> +goto failed;
>> mgr->payloads = kcalloc(max_payloads, sizeof(struct drm_dp_payload),
>> GFP_KERNEL);
>> if (!mgr->payloads)
>> -return -ENOMEM;
>> +goto failed;
>> mgr->proposed_vcpis = kcalloc(max_payloads, sizeof(struct drm_dp_vcpi
>> *), GFP_KERNEL);
>> if (!mgr->proposed_vcpis)
>> -return -ENOMEM;
>> +goto failed;
>> set_bit(0, >payload_mask);
>>
>> mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
>> if (mst_state == NULL)
>> -return -ENOMEM;
>> +goto failed;
>>
>> mst_state->total_avail_slots = 63;
>> mst_state->start_slot = 1;
>> @@ -5563,6 +5566,13 @@ int drm_dp_mst_topology_mgr_init(struct
>> drm_dp_mst_topology_mgr *mgr,
>> _dp_mst_topology_state_funcs);
>>
>> return 0;
>> +
>> +failed:
>> +kfree(mgr->proposed_vcpis);
>> +kfree(mgr->payloads);
>> +destroy_workqueue(mgr->delayed_destroy_wq);
>> +out:
>> +return -ENOMEM;
>> }
>> EXPORT_SYMBOL(drm_dp_mst_topology_mgr_init);
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Jani Nikula, Intel Open Source Graphics Center
[Bug 215436] admgpu: suspend and resuming from suspend don't work
https://bugzilla.kernel.org/show_bug.cgi?id=215436 spassw...@web.de changed: What|Removed |Added Attachment #300223|0 |1 is obsolete|| --- Comment #21 from spassw...@web.de --- Created attachment 300224 --> https://bugzilla.kernel.org/attachment.cgi?id=300224=edit new debuggin patch Seems the call trace of rn_vbios_smu_set_dcn_low_power is amdgpu_dm_atomic_commit_tail -> dc_commit_state -> dc_commit_state_nocheck -> dcn20_optimize_bandwith -> rn_update_clocks -> rn_vbios_smu_set_dcn_low_power 53.562322] amdgpu :08:00.0: amdgpu: Calling dc_commit_state from amdgpu_dm_atomic_commit_tail [ 53.562426] amdgpu :08:00.0: amdgpu: Calling dc_commit_state from amdgpu_dm_atomic_commit_tail [ 53.563183] wlp4s0: deauthenticating from 54:67:51:3d:a2:e0 by local choice (Reason: 3=DEAUTH_LEAVING) [ 53.673655] amdgpu :08:00.0: amdgpu: Calling optimize_bandwidth from dc_commit_state_no_check [ 53.673669] amdgpu :08:00.0: amdgpu: Calling update_clocks from dcn20_optimize_bandwidth [ 53.673672] amdgpu :08:00.0: amdgpu: adev->in_s0ix = 0 [ 53.673673] amdgpu :08:00.0: amdgpu: calling rn_vbios_smu_set_dcn_low_power [ 53.674562] amdgpu :08:00.0: amdgpu: Calling dc_commit_state from amdgpu_dm_atomic_commit_tail This seems to be not really connected to suspend. -- 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 22/22] drm: rockchip: Add VOP2 driver
Hi Sascha:
Please add Series-version for all the patch series. add also Series-changes.
I saw you have Series-changes in cover-letter, but we usually include
them in patch.
Here is a link you can take for reference[0]
[0]https://source.denx.de/u-boot/u-boot/-/tree/master/tools/patman
On 12/20/21 7:06 PM, Sascha Hauer wrote:
From: Andy Yan
The VOP2 unit is found on Rockchip SoCs beginning with rk3566/rk3568.
It replaces the VOP unit found in the older Rockchip SoCs.
This driver has been derived from the downstream Rockchip Kernel and
heavily modified:
- All nonstandard DRM properties have been removed
- dropped struct vop2_plane_state and pass around less data between
functions
- Dropped all DRM_FORMAT_* not known on upstream
- rework register access to get rid of excessively used macros
- Drop all waiting for framesyncs
The driver is tested with HDMI and MIPI-DSI display on a RK3568-EVB
board. Overlay support is tested with the modetest utility. AFBC support
on the cluster windows is tested with weston-simple-dmabuf-egl on
weston using the (yet to be upstreamed) panfrost driver support.
Signed-off-by: Sascha Hauer
---
drivers/gpu/drm/rockchip/Kconfig |6 +
drivers/gpu/drm/rockchip/Makefile|1 +
drivers/gpu/drm/rockchip/rockchip_drm_drv.c |1 +
drivers/gpu/drm/rockchip/rockchip_drm_drv.h |7 +-
drivers/gpu/drm/rockchip/rockchip_drm_fb.c |2 +
drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 15 +
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2768 ++
drivers/gpu/drm/rockchip/rockchip_drm_vop2.h | 480 +++
drivers/gpu/drm/rockchip/rockchip_vop2_reg.c | 285 ++
9 files changed, 3564 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_vop2.h
create mode 100644 drivers/gpu/drm/rockchip/rockchip_vop2_reg.c
diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
index b9b156308460a..4ff0043f0ee70 100644
--- a/drivers/gpu/drm/rockchip/Kconfig
+++ b/drivers/gpu/drm/rockchip/Kconfig
@@ -28,6 +28,12 @@ config ROCKCHIP_VOP
This selects support for the VOP driver. You should enable it
on all older SoCs up to RK3399.
+config ROCKCHIP_VOP2
+ bool "Rockchip VOP2 driver"
+ help
+ This selects support for the VOP2 driver. You should enable it
+ on all newer SoCs beginning form RK3568.
+
config ROCKCHIP_ANALOGIX_DP
bool "Rockchip specific extensions for Analogix DP driver"
depends on ROCKCHIP_VOP
diff --git a/drivers/gpu/drm/rockchip/Makefile
b/drivers/gpu/drm/rockchip/Makefile
index cd6e7bb5ce9c5..29848caef5c21 100644
--- a/drivers/gpu/drm/rockchip/Makefile
+++ b/drivers/gpu/drm/rockchip/Makefile
@@ -7,6 +7,7 @@ rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
rockchip_drm_gem.o
rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
+rockchipdrm-$(CONFIG_ROCKCHIP_VOP2) += rockchip_drm_vop2.o rockchip_vop2_reg.o
rockchipdrm-$(CONFIG_ROCKCHIP_VOP) += rockchip_drm_vop.o rockchip_vop_reg.o
rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index 64fa5fd62c01a..2bd9acb265e5a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -474,6 +474,7 @@ static int __init rockchip_drm_init(void)
num_rockchip_sub_drivers = 0;
ADD_ROCKCHIP_SUB_DRIVER(vop_platform_driver, CONFIG_ROCKCHIP_VOP);
+ ADD_ROCKCHIP_SUB_DRIVER(vop2_platform_driver, CONFIG_ROCKCHIP_VOP2);
ADD_ROCKCHIP_SUB_DRIVER(rockchip_lvds_driver,
CONFIG_ROCKCHIP_LVDS);
ADD_ROCKCHIP_SUB_DRIVER(rockchip_dp_driver,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index aa0909e8edf93..fd6994f21817e 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -18,7 +18,7 @@
#define ROCKCHIP_MAX_FB_BUFFER 3
#define ROCKCHIP_MAX_CONNECTOR2
-#define ROCKCHIP_MAX_CRTC 2
+#define ROCKCHIP_MAX_CRTC 4
struct drm_device;
struct drm_connector;
@@ -31,6 +31,9 @@ struct rockchip_crtc_state {
int output_bpc;
int output_flags;
bool enable_afbc;
+ uint32_t bus_format;
+ u32 bus_flags;
+ int color_space;
};
#define to_rockchip_crtc_state(s) \
container_of(s, struct rockchip_crtc_state, base)
@@ -65,4 +68,6 @@ extern struct platform_driver rockchip_dp_driver;
extern struct platform_driver rockchip_lvds_driver;
extern struct platform_driver vop_platform_driver;
extern struct platform_driver rk3066_hdmi_driver;
+extern struct platform_driver
Re: [PATCH 3/3] drm/stm: dsi: provide the implementation of mode_valid()
On 12/18/21 10:50 PM, Antonio Borneo wrote: The dsi has several constraints on the video modes it can support, mainly due to the frequencies that can be generated by the PLL integrated in the DSI device. Verify that the required HS clock can be generated by the PLL. The dsi clock from the dsi PLL and the ltdc pixel clock are asynchronous. The dsi needs to return in LP mode during HFP or HBP to re-synchronize at each video line. Verify that the duration of HFP and HBP allows the dsi to enter in LP mode. Signed-off-by: Antonio Borneo --- To: David Airlie To: Daniel Vetter To: Andrzej Hajda To: Neil Armstrong To: Robert Foss To: Laurent Pinchart To: Jonas Karlman To: Jernej Skrabec To: Yannick Fertre To: Philippe Cornu To: Benjamin Gaignard To: Maxime Coquelin To: Alexandre Torgue To: Philipp Zabel To: dri-devel@lists.freedesktop.org To: linux-st...@st-md-mailman.stormreply.com To: linux-arm-ker...@lists.infradead.org Cc: linux-ker...@vger.kernel.org --- drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 98 +++ 1 file changed, 98 insertions(+) Hi Antonio, many thanks for your patch. Nice improvement for better filtering supported modes... Acked-by: Philippe Cornu Reviewed-by: Philippe Cornu Philippe :-)
Re: [PATCH 2/3] drm/bridge/synopsys: dsi: extend the prototype of mode_valid()
On 12/18/21 10:50 PM, Antonio Borneo wrote: To evaluate the validity of a video mode, some additional internal value has to be passed to the platform implementation. Extend the prototype of mode_valid(). Signed-off-by: Antonio Borneo --- To: David Airlie To: Daniel Vetter To: Andrzej Hajda To: Neil Armstrong To: Robert Foss To: Laurent Pinchart To: Jonas Karlman To: Jernej Skrabec To: Yannick Fertre To: Philippe Cornu To: Benjamin Gaignard To: Maxime Coquelin To: Alexandre Torgue To: Philipp Zabel To: dri-devel@lists.freedesktop.org To: linux-st...@st-md-mailman.stormreply.com To: linux-arm-ker...@lists.infradead.org Cc: linux-ker...@vger.kernel.org --- drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 5 - include/drm/bridge/dw_mipi_dsi.h | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) Hi Antonio, many thanks for your patch. (I should have done like that from the beginning as validating a mode in dsi requires dsi related information...) Reviewed-by: Philippe Cornu Philippe :-)
RE: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV
[AMD Official Use Only] >> See the FLR request from the hypervisor is just another source of signaling >> the need for a reset, similar to each job timeout on each queue. Otherwise >> you have a race condition between the hypervisor and the scheduler. No it's not, FLR from hypervisor is just to notify guest the hw VF FLR is about to start or was already executed, but host will do FLR anyway without waiting for guest too long >> In other words I strongly think that the current SRIOV reset implementation >> is severely broken and what Andrey is doing is actually fixing it. It makes the code to crash ... how could it be a fix ? I'm afraid the patch is NAK from me, but it is welcome if the cleanup do not ruin the logic, Andry or jingwen can try it if needed. Thanks --- Monk Liu | Cloud GPU & Virtualization Solution | AMD --- we are hiring software manager for CVS core team --- -Original Message- From: Koenig, Christian Sent: Tuesday, January 4, 2022 6:19 PM To: Chen, JingWen ; Christian König ; Grodzovsky, Andrey ; Deng, Emily ; Liu, Monk ; dri-devel@lists.freedesktop.org; amd-...@lists.freedesktop.org; Chen, Horace ; Chen, JingWen Cc: dan...@ffwll.ch Subject: Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV Hi Jingwen, well what I mean is that we need to adjust the implementation in amdgpu to actually match the requirements. Could be that the reset sequence is questionable in general, but I doubt so at least for now. See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. Properly setting in_gpu_reset is indeed mandatory, but should happen at a central place and not in the SRIOV specific code. In other words I strongly think that the current SRIOV reset implementation is severely broken and what Andrey is doing is actually fixing it. Regards, Christian. Am 04.01.22 um 10:07 schrieb JingWen Chen: > Hi Christian, > I'm not sure what do you mean by "we need to change SRIOV not the driver". > > Do you mean we should change the reset sequence in SRIOV? This will be a huge > change for our SRIOV solution. > > From my point of view, we can directly use amdgpu_device_lock_adev > and amdgpu_device_unlock_adev in flr_work instead of try_lock since no one > will conflict with this thread with reset_domain introduced. > But we do need the reset_sem and adev->in_gpu_reset to keep device untouched > via user space. > > Best Regards, > Jingwen Chen > > On 2022/1/3 下午6:17, Christian König wrote: >> Please don't. This patch is vital to the cleanup of the reset procedure. >> >> If SRIOV doesn't work with that we need to change SRIOV and not the driver. >> >> Christian. >> >> Am 30.12.21 um 19:45 schrieb Andrey Grodzovsky: >>> Sure, I guess i can drop this patch then. >>> >>> Andrey >>> >>> On 2021-12-24 4:57 a.m., JingWen Chen wrote: I do agree with shaoyun, if the host find the gpu engine hangs first, and do the flr, guest side thread may not know this and still try to access HW(e.g. kfd is using a lot of amdgpu_in_reset and reset_sem to identify the reset status). And this may lead to very bad result. On 2021/12/24 下午4:58, Deng, Emily wrote: > These patches look good to me. JingWen will pull these patches and do > some basic TDR test on sriov environment, and give feedback. > > Best wishes > Emily Deng > > > >> -Original Message- >> From: Liu, Monk >> Sent: Thursday, December 23, 2021 6:14 PM >> To: Koenig, Christian ; Grodzovsky, >> Andrey ; >> dri-devel@lists.freedesktop.org; amd- g...@lists.freedesktop.org; >> Chen, Horace ; Chen, JingWen >> ; Deng, Emily >> Cc: dan...@ffwll.ch >> Subject: RE: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset >> protection for SRIOV >> >> [AMD Official Use Only] >> >> @Chen, Horace @Chen, JingWen @Deng, Emily >> >> Please take a review on Andrey's patch >> >> Thanks >> - >> -- Monk Liu | Cloud GPU & Virtualization Solution | AMD >> - >> -- we are hiring software manager for CVS core team >> - >> -- >> >> -Original Message- >> From: Koenig, Christian >> Sent: Thursday, December 23, 2021 4:42 PM >> To: Grodzovsky, Andrey ; dri- >> de...@lists.freedesktop.org; amd-...@lists.freedesktop.org >> Cc: dan...@ffwll.ch; Liu, Monk ; Chen, Horace >> >>
Re: [PATCH 1/3] drm/stm: dsi: move lane capability detection in probe()
On 12/18/21 10:50 PM, Antonio Borneo wrote: There is no need to re-compute the dsi lane capability because it only depends on dsi hw version. Since dsi hw version is detected at probe(), move there also the assignment of dsi lane capability. Signed-off-by: Antonio Borneo --- To: David Airlie To: Daniel Vetter To: Andrzej Hajda To: Neil Armstrong To: Robert Foss To: Laurent Pinchart To: Jonas Karlman To: Jernej Skrabec To: Yannick Fertre To: Philippe Cornu To: Benjamin Gaignard To: Maxime Coquelin To: Alexandre Torgue To: Philipp Zabel To: dri-devel@lists.freedesktop.org To: linux-st...@st-md-mailman.stormreply.com To: linux-arm-ker...@lists.infradead.org Cc: linux-ker...@vger.kernel.org --- drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 16 1 file changed, 8 insertions(+), 8 deletions(-) Hi Antonio, many thanks for your patch. Acked-by: Philippe Cornu Reviewed-by: Philippe Cornu Philippe :-)
[PATCH] drivers/video: remove redundant res variable
From: Minghao Chi
Return value from aty_ld_8() directly instead
of taking this in another redundant variable.
Reported-by: Zeal Robot
Signed-off-by: Minghao Chi
Signed-off-by: CGEL ZTE
---
drivers/video/fbdev/aty/mach64_ct.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/aty/mach64_ct.c
b/drivers/video/fbdev/aty/mach64_ct.c
index 011b07e44e0d..e967536af166 100644
--- a/drivers/video/fbdev/aty/mach64_ct.c
+++ b/drivers/video/fbdev/aty/mach64_ct.c
@@ -22,13 +22,11 @@ static u32 aty_pll_to_var_ct(const struct fb_info *info,
const union aty_pll *pl
u8 aty_ld_pll_ct(int offset, const struct atyfb_par *par)
{
- u8 res;
/* write addr byte */
aty_st_8(CLOCK_CNTL_ADDR, (offset << 2) & PLL_ADDR, par);
/* read the register value */
- res = aty_ld_8(CLOCK_CNTL_DATA, par);
- return res;
+ return aty_ld_8(CLOCK_CNTL_DATA, par);
}
static void aty_st_pll_ct(int offset, u8 val, const struct atyfb_par *par)
--
2.25.1
Re: [PATCH 5/5] drm/stm: ltdc: add support of ycbcr pixel formats
On 12/15/21 10:48 PM, Yannick Fertre wrote: This patch adds the following YCbCr input pixel formats on the latest LTDC hardware version: 1 plane (co-planar) : YUYV, YVYU, UYVY, VYUY 2 planes (semi-planar): NV12, NV21 3 planes (full-planar): YU12=I420=DRM YUV420, YV12=DRM YVU420 Signed-off-by: Yannick Fertre --- drivers/gpu/drm/stm/ltdc.c | 251 +++-- drivers/gpu/drm/stm/ltdc.h | 1 + 2 files changed, 245 insertions(+), 7 deletions(-) Hi Yannick, many thanks for your patch. Nice hw features! Acked-by: Philippe Cornu Reviewed-by: Philippe Cornu Philippe :-)
Re: [PATCH 4/5] drm/stm: ltdc: add support of flexible pixel formats
On 12/15/21 10:48 PM, Yannick Fertre wrote: This feature allows the generation of any RGB pixel format. The list of supported formats is no longer linked to the register LXPFCR_PF, that the reason why a list of drm formats is defined for each display controller version. Signed-off-by: Yannick Fertre --- drivers/gpu/drm/stm/ltdc.c | 196 ++--- drivers/gpu/drm/stm/ltdc.h | 5 +- 2 files changed, 145 insertions(+), 56 deletions(-) Hi Yannick, many thanks for your patch. Nice hw feature. Acked-by: Philippe Cornu Reviewed-by: Philippe Cornu Philippe :-)
[Bug 215436] admgpu: suspend and resuming from suspend don't work
https://bugzilla.kernel.org/show_bug.cgi?id=215436 spassw...@web.de changed: What|Removed |Added Attachment #300222|0 |1 is obsolete|| --- Comment #20 from spassw...@web.de --- Created attachment 300223 --> https://bugzilla.kernel.org/attachment.cgi?id=300223=edit corrected debugging patch The patch above contained an error rn_update_clocks is actually called from dcn20_optimize_bandwidth -- 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 3/5] drm/stm: ltdc: add per plane update support
On 12/15/21 10:48 PM, Yannick Fertre wrote: Recent ltdc hardware versions offer the ability to update a plane independently of others planes. This is could be useful especially if a plane is assigned to another OS. Signed-off-by: Yannick Fertre --- drivers/gpu/drm/stm/ltdc.c | 26 +++--- drivers/gpu/drm/stm/ltdc.h | 1 + 2 files changed, 24 insertions(+), 3 deletions(-) Hi Yannick, many thanks for your patch. Acked-by: Philippe Cornu Reviewed-by: Philippe Cornu Philippe :-)
Re: [PATCH 2/5] drm/stm: ltdc: add YCbCr 422 output support
On 12/15/21 10:47 PM, Yannick Fertre wrote: LTDC 40100 hw version supports the YCbCr 422 output, reducing the output pins from 24 to 16. This feature is useful for some external devices like HDMI bridges. Both ITU-R BT.601 & ITU-R BT.709 are supported. It is also possible to choose the chrominance order between * Cb is output first (Y0Cb, then Y1Cr, Y2Cb and so on). * Cr is output first (Y0Cr, then Y1Cb, Y2Cr and so on). Signed-off-by: Yannick Fertre --- drivers/gpu/drm/stm/ltdc.c | 44 +- drivers/gpu/drm/stm/ltdc.h | 1 + 2 files changed, 44 insertions(+), 1 deletion(-) Hi Yannick, many thanks for your patch. Acked-by: Philippe Cornu Reviewed-by: Philippe Cornu Philippe :-)
Re: [PATCH 1/5] drm/stm: ltdc: switch to regmap
On 12/15/21 10:47 PM, Yannick Fertre wrote: Replace the legacy register access by regmap API. Signed-off-by: Yannick Fertre --- drivers/gpu/drm/stm/ltdc.c | 138 ++--- drivers/gpu/drm/stm/ltdc.h | 1 + 2 files changed, 68 insertions(+), 71 deletions(-) Hi Yannick, many thanks for your patch. Acked-by: Philippe Cornu Reviewed-by: Philippe Cornu Philippe :-)
Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV
Hi Jingwen, well what I mean is that we need to adjust the implementation in amdgpu to actually match the requirements. Could be that the reset sequence is questionable in general, but I doubt so at least for now. See the FLR request from the hypervisor is just another source of signaling the need for a reset, similar to each job timeout on each queue. Otherwise you have a race condition between the hypervisor and the scheduler. Properly setting in_gpu_reset is indeed mandatory, but should happen at a central place and not in the SRIOV specific code. In other words I strongly think that the current SRIOV reset implementation is severely broken and what Andrey is doing is actually fixing it. Regards, Christian. Am 04.01.22 um 10:07 schrieb JingWen Chen: Hi Christian, I'm not sure what do you mean by "we need to change SRIOV not the driver". Do you mean we should change the reset sequence in SRIOV? This will be a huge change for our SRIOV solution. From my point of view, we can directly use amdgpu_device_lock_adev and amdgpu_device_unlock_adev in flr_work instead of try_lock since no one will conflict with this thread with reset_domain introduced. But we do need the reset_sem and adev->in_gpu_reset to keep device untouched via user space. Best Regards, Jingwen Chen On 2022/1/3 下午6:17, Christian König wrote: Please don't. This patch is vital to the cleanup of the reset procedure. If SRIOV doesn't work with that we need to change SRIOV and not the driver. Christian. Am 30.12.21 um 19:45 schrieb Andrey Grodzovsky: Sure, I guess i can drop this patch then. Andrey On 2021-12-24 4:57 a.m., JingWen Chen wrote: I do agree with shaoyun, if the host find the gpu engine hangs first, and do the flr, guest side thread may not know this and still try to access HW(e.g. kfd is using a lot of amdgpu_in_reset and reset_sem to identify the reset status). And this may lead to very bad result. On 2021/12/24 下午4:58, Deng, Emily wrote: These patches look good to me. JingWen will pull these patches and do some basic TDR test on sriov environment, and give feedback. Best wishes Emily Deng -Original Message- From: Liu, Monk Sent: Thursday, December 23, 2021 6:14 PM To: Koenig, Christian ; Grodzovsky, Andrey ; dri-devel@lists.freedesktop.org; amd- g...@lists.freedesktop.org; Chen, Horace ; Chen, JingWen ; Deng, Emily Cc: dan...@ffwll.ch Subject: RE: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV [AMD Official Use Only] @Chen, Horace @Chen, JingWen @Deng, Emily Please take a review on Andrey's patch Thanks --- Monk Liu | Cloud GPU & Virtualization Solution | AMD --- we are hiring software manager for CVS core team --- -Original Message- From: Koenig, Christian Sent: Thursday, December 23, 2021 4:42 PM To: Grodzovsky, Andrey ; dri- de...@lists.freedesktop.org; amd-...@lists.freedesktop.org Cc: dan...@ffwll.ch; Liu, Monk ; Chen, Horace Subject: Re: [RFC v2 8/8] drm/amd/virt: Drop concurrent GPU reset protection for SRIOV Am 22.12.21 um 23:14 schrieb Andrey Grodzovsky: Since now flr work is serialized against GPU resets there is no need for this. Signed-off-by: Andrey Grodzovsky Acked-by: Christian König --- drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c | 11 --- drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c | 11 --- 2 files changed, 22 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c index 487cd654b69e..7d59a66e3988 100644 --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_ai.c @@ -248,15 +248,7 @@ static void xgpu_ai_mailbox_flr_work(struct work_struct *work) struct amdgpu_device *adev = container_of(virt, struct amdgpu_device, virt); int timeout = AI_MAILBOX_POLL_FLR_TIMEDOUT; - /* block amdgpu_gpu_recover till msg FLR COMPLETE received, - * otherwise the mailbox msg will be ruined/reseted by - * the VF FLR. - */ - if (!down_write_trylock(>reset_sem)) - return; - amdgpu_virt_fini_data_exchange(adev); - atomic_set(>in_gpu_reset, 1); xgpu_ai_mailbox_trans_msg(adev, IDH_READY_TO_RESET, 0, 0, 0); @@ -269,9 +261,6 @@ static void xgpu_ai_mailbox_flr_work(struct work_struct *work) } while (timeout > 1); flr_done: - atomic_set(>in_gpu_reset, 0); - up_write(>reset_sem); - /* Trigger recovery for world switch failure if no TDR */ if (amdgpu_device_should_recover_gpu(adev) && (!amdgpu_device_has_job_running(adev) || diff --git a/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c b/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c index e3869067a31d..f82c066c8e8d 100644 --- a/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c +++ b/drivers/gpu/drm/amd/amdgpu/mxgpu_nv.c @@
[Bug 215436] admgpu: suspend and resuming from suspend don't work
https://bugzilla.kernel.org/show_bug.cgi?id=215436 --- Comment #19 from spassw...@web.de --- Created attachment 300222 --> https://bugzilla.kernel.org/attachment.cgi?id=300222=edit debugging patch I added some more dev_info calls to figure out where things are called: [ 34.946911] wlp4s0: deauthenticating from 54:67:51:3d:a2:e0 by local choice (Reason: 3=DEAUTH_LEAVING) [ 35.079021] amdgpu :08:00.0: amdgpu: Calling update_clocks from dcn10_set_clock [ 35.079026] amdgpu :08:00.0: amdgpu: adev->in_s0ix = 0 [ 35.079027] amdgpu :08:00.0: amdgpu: calling rn_vbios_smu_set_dcn_low_power [ 39.030889] PM: suspend entry (s2idle) [ 39.270864] Filesystems sync: 0.239 seconds [ 39.271929] Freezing user space processes ... (elapsed 0.002 seconds) done. [ 39.274694] OOM killer disabled. The WLAN message is the first message one gets after initiating suspend. The interesting thing is that rn_vbios_set_dc_low_power is called before suspend entry (and there's also a long 4s gap). I also have no idea where dcn10_set_clock is called. The only place I found is from within dc_set_clock, but dc_set_clock is called nowhere in the entire amdgpu module. -- 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 v2] drm/mediatek: mtk_dsi: Avoid EPROBE_DEFER loop with external bridge
DRM bridge drivers are now attaching their DSI device at probe time,
which requires us to register our DSI host in order to let the bridge
to probe: this recently started producing an endless -EPROBE_DEFER
loop on some machines that are using external bridges, like the
parade-ps8640, found on the ACER Chromebook R13.
Now that the DSI hosts/devices probe sequence is documented, we can
do adjustments to the mtk_dsi driver as to both fix now and make sure
to avoid this situation in the future: for this, following what is
documented in drm_bridge.c, move the mtk_dsi component_add() to the
mtk_dsi_ops.attach callback and delete it in the detach callback;
keeping in mind that we are registering a drm_bridge for our DSI,
which is only used/attached if the DSI Host is bound, it wouldn't
make sense to keep adding our bridge at probe time (as it would
be useless to have it if mtk_dsi_ops.attach() fails!), so also move
that one to the dsi host attach function (and remove it in detach).
Signed-off-by: AngeloGioacchino Del Regno
Reviewed-by: Andrzej Hajda
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 167 +++--
1 file changed, 84 insertions(+), 83 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 5d90d2eb0019..bced4c7d668e 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -786,18 +786,101 @@ void mtk_dsi_ddp_stop(struct device *dev)
mtk_dsi_poweroff(dsi);
}
+static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi)
+{
+ int ret;
+
+ ret = drm_simple_encoder_init(drm, >encoder,
+ DRM_MODE_ENCODER_DSI);
+ if (ret) {
+ DRM_ERROR("Failed to encoder init to drm\n");
+ return ret;
+ }
+
+ dsi->encoder.possible_crtcs = mtk_drm_find_possible_crtc_by_comp(drm,
dsi->host.dev);
+
+ ret = drm_bridge_attach(>encoder, >bridge, NULL,
+ DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+ if (ret)
+ goto err_cleanup_encoder;
+
+ dsi->connector = drm_bridge_connector_init(drm, >encoder);
+ if (IS_ERR(dsi->connector)) {
+ DRM_ERROR("Unable to create bridge connector\n");
+ ret = PTR_ERR(dsi->connector);
+ goto err_cleanup_encoder;
+ }
+ drm_connector_attach_encoder(dsi->connector, >encoder);
+
+ return 0;
+
+err_cleanup_encoder:
+ drm_encoder_cleanup(>encoder);
+ return ret;
+}
+
+static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
+{
+ int ret;
+ struct drm_device *drm = data;
+ struct mtk_dsi *dsi = dev_get_drvdata(dev);
+
+ ret = mtk_dsi_encoder_init(drm, dsi);
+ if (ret)
+ return ret;
+
+ return device_reset_optional(dev);
+}
+
+static void mtk_dsi_unbind(struct device *dev, struct device *master,
+ void *data)
+{
+ struct mtk_dsi *dsi = dev_get_drvdata(dev);
+
+ drm_encoder_cleanup(>encoder);
+}
+
+static const struct component_ops mtk_dsi_component_ops = {
+ .bind = mtk_dsi_bind,
+ .unbind = mtk_dsi_unbind,
+};
+
static int mtk_dsi_host_attach(struct mipi_dsi_host *host,
struct mipi_dsi_device *device)
{
struct mtk_dsi *dsi = host_to_dsi(host);
+ struct device *dev = host->dev;
+ int ret;
dsi->lanes = device->lanes;
dsi->format = device->format;
dsi->mode_flags = device->mode_flags;
+ dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
+ if (IS_ERR(dsi->next_bridge))
+ return PTR_ERR(dsi->next_bridge);
+
+ drm_bridge_add(>bridge);
+
+ ret = component_add(host->dev, _dsi_component_ops);
+ if (ret) {
+ DRM_ERROR("failed to add dsi_host component: %d\n", ret);
+ drm_bridge_remove(>bridge);
+ return ret;
+ }
return 0;
}
+static int mtk_dsi_host_detach(struct mipi_dsi_host *host,
+ struct mipi_dsi_device *device)
+{
+ struct mtk_dsi *dsi = host_to_dsi(host);
+
+ component_del(host->dev, _dsi_component_ops);
+ drm_bridge_remove(>bridge);
+ return 0;
+}
+
static void mtk_dsi_wait_for_idle(struct mtk_dsi *dsi)
{
int ret;
@@ -938,73 +1021,14 @@ static ssize_t mtk_dsi_host_transfer(struct
mipi_dsi_host *host,
static const struct mipi_dsi_host_ops mtk_dsi_ops = {
.attach = mtk_dsi_host_attach,
+ .detach = mtk_dsi_host_detach,
.transfer = mtk_dsi_host_transfer,
};
-static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi)
-{
- int ret;
-
- ret = drm_simple_encoder_init(drm, >encoder,
- DRM_MODE_ENCODER_DSI);
- if (ret) {
- DRM_ERROR("Failed to encoder init to drm\n");
-
Re: [PATCH] drm/mediatek: mtk_dsi: Avoid EPROBE_DEFER loop with external bridge
Hi,
On 10.12.2021 12:36, AngeloGioacchino Del Regno wrote:
DRM bridge drivers are now attaching their DSI device at probe time,
which requires us to register our DSI host in order to let the bridge
to probe: this recently started producing an endless -EPROBE_DEFER
loop on some machines that are using external bridges, like the
parade-ps8640, found on the ACER Chromebook R13.
Now that the DSI hosts/devices probe sequence is documented, we can
do adjustments to the mtk_dsi driver as to both fix now and make sure
to avoid this situation in the future: for this, following what is
documented in drm_bridge.c, move the mtk_dsi component_add() to the
mtk_dsi_ops.attach callback and delete it in the detach callback;
keeping in mind that we are registering a drm_bridge for our DSI,
which is only used/attached if the DSI Host is bound, it wouldn't
make sense to keep adding our bridge at probe time (as it would
be useless to have it if mtk_dsi_ops.attach() fails!), so also move
that one to the dsi host attach function (and remove it in detach).
Signed-off-by: AngeloGioacchino Del Regno
---
drivers/gpu/drm/mediatek/mtk_dsi.c | 167 +++--
1 file changed, 84 insertions(+), 83 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c
b/drivers/gpu/drm/mediatek/mtk_dsi.c
index 5d90d2eb0019..2ff347da35c2 100644
--- a/drivers/gpu/drm/mediatek/mtk_dsi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
@@ -786,18 +786,101 @@ void mtk_dsi_ddp_stop(struct device *dev)
mtk_dsi_poweroff(dsi);
}
+static int mtk_dsi_encoder_init(struct drm_device *drm, struct mtk_dsi *dsi)
+{
+ int ret;
+
+ ret = drm_simple_encoder_init(drm, >encoder,
+ DRM_MODE_ENCODER_DSI);
+ if (ret) {
+ DRM_ERROR("Failed to encoder init to drm\n");
+ return ret;
+ }
+
+ dsi->encoder.possible_crtcs = mtk_drm_find_possible_crtc_by_comp(drm,
dsi->host.dev);
+
+ ret = drm_bridge_attach(>encoder, >bridge, NULL,
+ DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+ if (ret)
+ goto err_cleanup_encoder;
+
+ dsi->connector = drm_bridge_connector_init(drm, >encoder);
+ if (IS_ERR(dsi->connector)) {
+ DRM_ERROR("Unable to create bridge connector\n");
+ ret = PTR_ERR(dsi->connector);
+ goto err_cleanup_encoder;
+ }
+ drm_connector_attach_encoder(dsi->connector, >encoder);
+
+ return 0;
+
+err_cleanup_encoder:
+ drm_encoder_cleanup(>encoder);
+ return ret;
+}
+
+static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
+{
+ int ret;
+ struct drm_device *drm = data;
+ struct mtk_dsi *dsi = dev_get_drvdata(dev);
+
+ ret = mtk_dsi_encoder_init(drm, dsi);
+ if (ret)
+ return ret;
+
+ return device_reset_optional(dev);
+}
+
+static void mtk_dsi_unbind(struct device *dev, struct device *master,
+ void *data)
+{
+ struct mtk_dsi *dsi = dev_get_drvdata(dev);
+
+ drm_encoder_cleanup(>encoder);
+}
+
+static const struct component_ops mtk_dsi_component_ops = {
+ .bind = mtk_dsi_bind,
+ .unbind = mtk_dsi_unbind,
+};
+
static int mtk_dsi_host_attach(struct mipi_dsi_host *host,
struct mipi_dsi_device *device)
{
struct mtk_dsi *dsi = host_to_dsi(host);
+ struct device *dev = host->dev;
+ int ret;
dsi->lanes = device->lanes;
dsi->format = device->format;
dsi->mode_flags = device->mode_flags;
+ dsi->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
+ if (IS_ERR(dsi->next_bridge))
+ return PTR_ERR(dsi->next_bridge);
Here we have implicit assumption that port node (0,0) points always to
the 'device'. I guess it is true for now, but could bite someday. Anyway
improving it would require adding some additional helper for getting
bridge based on mipi_dsi_device and should be done in separate patchset.
+
+ drm_bridge_add(>bridge);
+
+ ret = component_add(host->dev, _dsi_component_ops);
+ if (ret) {
+ DRM_ERROR("failed to add dsi_host component: %d\n", ret);
+ drm_bridge_remove(>bridge);
+ return ret;
+ }
return 0;
}
+static int mtk_dsi_host_detach(struct mipi_dsi_host *host,
+ struct mipi_dsi_device *device)
+{
+ struct mtk_dsi *dsi = host_to_dsi(host);
+
+ drm_bridge_remove(>bridge);
+ component_del(host->dev, _dsi_component_ops);
Order should be reversed.
With this fixed:
Reviewed-by: Andrzej Hajda
Regards
Andrzej
+ return 0;
+}
+
static void mtk_dsi_wait_for_idle(struct mtk_dsi *dsi)
{
int ret;
@@ -938,73 +1021,14 @@ static ssize_t mtk_dsi_host_transfer(struct
mipi_dsi_host *host,
static const struct mipi_dsi_host_ops mtk_dsi_ops = {
答复: [PATCH] gpu/drm: fix potential memleak in error branch
-邮件原件-
发件人: bern...@vivo.com 代表 Jani Nikula
发送时间: 2021年12月31日 19:09
收件人: 赵军奎 ; Maarten Lankhorst
; Maxime Ripard ; Thomas
Zimmermann ; David Airlie ; Daniel
Vetter ; dri-devel@lists.freedesktop.org;
linux-ker...@vger.kernel.org
抄送: 赵军奎
主题: Re: [PATCH] gpu/drm: fix potential memleak in error branch
On Tue, 16 Nov 2021, Bernard Zhao wrote:
> This patch try to fix potential memleak in error branch.
>Please elaborate.
Hi Jani:
This patch try to fix potential memleak in error branch.
For example:
nv50_sor_create ->nv50_mstm_new-> drm_dp_mst_topology_mgr_init
In function drm_dp_mst_topology_mgr_init, there are five error branches, error
branch just return error code, no free called.
And we see that the caller didn`t do the drm_dp_mst_topology_mgr_destroy job.
I am not sure if there some gap, I think this may bring in the risk of memleak
issue.
Thanks!
BR//Bernard
>BR,
>Jani.
>
> Signed-off-by: Bernard Zhao
> ---
> drivers/gpu/drm/drm_dp_mst_topology.c | 22 --
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
> b/drivers/gpu/drm/drm_dp_mst_topology.c
> index f3d79eda94bb..f73b180dee73 100644
> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> @@ -5501,7 +5501,10 @@ int drm_dp_mst_topology_mgr_init(struct
> drm_dp_mst_topology_mgr *mgr,
>int max_lane_count, int max_link_rate,
>int conn_base_id)
> {
> - struct drm_dp_mst_topology_state *mst_state;
> + struct drm_dp_mst_topology_state *mst_state = NULL;
> +
> + mgr->payloads = NULL;
> + mgr->proposed_vcpis = NULL;
>
> mutex_init(>lock);
> mutex_init(>qlock);
> @@ -5523,7 +5526,7 @@ int drm_dp_mst_topology_mgr_init(struct
> drm_dp_mst_topology_mgr *mgr,
>*/
> mgr->delayed_destroy_wq = alloc_ordered_workqueue("drm_dp_mst_wq", 0);
> if (mgr->delayed_destroy_wq == NULL)
> - return -ENOMEM;
> + goto out;
>
> INIT_WORK(>work, drm_dp_mst_link_probe_work);
> INIT_WORK(>tx_work, drm_dp_tx_work); @@ -5539,18 +5542,18 @@
> int drm_dp_mst_topology_mgr_init(struct drm_dp_mst_topology_mgr *mgr,
> mgr->conn_base_id = conn_base_id;
> if (max_payloads + 1 > sizeof(mgr->payload_mask) * 8 ||
> max_payloads + 1 > sizeof(mgr->vcpi_mask) * 8)
> - return -EINVAL;
> + goto failed;
> mgr->payloads = kcalloc(max_payloads, sizeof(struct drm_dp_payload),
> GFP_KERNEL);
> if (!mgr->payloads)
> - return -ENOMEM;
> + goto failed;
> mgr->proposed_vcpis = kcalloc(max_payloads, sizeof(struct drm_dp_vcpi
> *), GFP_KERNEL);
> if (!mgr->proposed_vcpis)
> - return -ENOMEM;
> + goto failed;
> set_bit(0, >payload_mask);
>
> mst_state = kzalloc(sizeof(*mst_state), GFP_KERNEL);
> if (mst_state == NULL)
> - return -ENOMEM;
> + goto failed;
>
> mst_state->total_avail_slots = 63;
> mst_state->start_slot = 1;
> @@ -5563,6 +5566,13 @@ int drm_dp_mst_topology_mgr_init(struct
> drm_dp_mst_topology_mgr *mgr,
> _dp_mst_topology_state_funcs);
>
> return 0;
> +
> +failed:
> + kfree(mgr->proposed_vcpis);
> + kfree(mgr->payloads);
> + destroy_workqueue(mgr->delayed_destroy_wq);
> +out:
> + return -ENOMEM;
> }
> EXPORT_SYMBOL(drm_dp_mst_topology_mgr_init);
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v3, 02/13] media: mtk-vcodec: Using firmware type to separate different firmware architecture
Il 04/01/22 09:01, Yunfei Dong ha scritto: MT8173 platform use vpu firmware, mt8183/mt8192 will use scp firmware instead, using chip name is not reasonable to separate different firmware architecture. Using firmware type is much better. Signed-off-by: Yunfei Dong Reviewed-by: Tzung-Bi Shih Hello Yunfei, I agree with this change, it looks better as it separates behaviors in a more generic way. Anyway, it looks like you're removing all users of `enum mtk_chip` but, in this case, you forgot to also remove the declaration of this enumeration from mtk_vcodec_drv.h! :) Can you please send a v4 with the requested removal? so, for v4... Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH] drm/mediatek: mtk_dsi: Avoid EPROBE_DEFER loop with external bridge
Il 10/12/21 12:36, AngeloGioacchino Del Regno ha scritto: DRM bridge drivers are now attaching their DSI device at probe time, which requires us to register our DSI host in order to let the bridge to probe: this recently started producing an endless -EPROBE_DEFER loop on some machines that are using external bridges, like the parade-ps8640, found on the ACER Chromebook R13. Now that the DSI hosts/devices probe sequence is documented, we can do adjustments to the mtk_dsi driver as to both fix now and make sure to avoid this situation in the future: for this, following what is documented in drm_bridge.c, move the mtk_dsi component_add() to the mtk_dsi_ops.attach callback and delete it in the detach callback; keeping in mind that we are registering a drm_bridge for our DSI, which is only used/attached if the DSI Host is bound, it wouldn't make sense to keep adding our bridge at probe time (as it would be useless to have it if mtk_dsi_ops.attach() fails!), so also move that one to the dsi host attach function (and remove it in detach). Signed-off-by: AngeloGioacchino Del Regno Due to the latest changes in drm bridge, we need this patch to land as soon as possible, otherwise breakages will be seen on all MediaTek powered devices using DSI to eDP/DP bridges... ...hence, friendly ping for a review :)
