[PATCH] drivers: gpu: drm: Remove repeated declaration
struct drm_i915_private, struct intel_crtc_state and struct intel_crtc have been declared before. Remove the duplicate. Signed-off-by: Wan Jiabing --- drivers/gpu/drm/i915/display/intel_crt.h | 1 - drivers/gpu/drm/i915/display/intel_display.h | 1 - drivers/gpu/drm/i915/display/intel_vrr.h | 1 - 3 files changed, 3 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_crt.h b/drivers/gpu/drm/i915/display/intel_crt.h index 1b3fba359efc..6c5c44600cbd 100644 --- a/drivers/gpu/drm/i915/display/intel_crt.h +++ b/drivers/gpu/drm/i915/display/intel_crt.h @@ -11,7 +11,6 @@ enum pipe; struct drm_encoder; struct drm_i915_private; -struct drm_i915_private; bool intel_crt_port_enabled(struct drm_i915_private *dev_priv, i915_reg_t adpa_reg, enum pipe *pipe); diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h index 76f8a805b0a3..29cb6d84ed70 100644 --- a/drivers/gpu/drm/i915/display/intel_display.h +++ b/drivers/gpu/drm/i915/display/intel_display.h @@ -48,7 +48,6 @@ struct i915_ggtt_view; struct intel_atomic_state; struct intel_crtc; struct intel_crtc_state; -struct intel_crtc_state; struct intel_digital_port; struct intel_dp; struct intel_encoder; diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h index fac01bf4ab50..96f9c9c27ab9 100644 --- a/drivers/gpu/drm/i915/display/intel_vrr.h +++ b/drivers/gpu/drm/i915/display/intel_vrr.h @@ -15,7 +15,6 @@ struct intel_crtc; struct intel_crtc_state; struct intel_dp; struct intel_encoder; -struct intel_crtc; bool intel_vrr_is_capable(struct drm_connector *connector); void intel_vrr_check_modeset(struct intel_atomic_state *state); -- 2.25.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drivers: gpu: drm: Remove duplicate declaration
struct dss_device has been declared at 51st line. Remove the duplicate. Signed-off-by: Wan Jiabing --- drivers/gpu/drm/omapdrm/dss/omapdss.h | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h index a40abeafd2e9..2658aadee09a 100644 --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h @@ -52,7 +52,6 @@ struct dss_device; struct omap_drm_private; struct omap_dss_device; struct dispc_device; -struct dss_device; struct dss_lcd_mgr_config; struct snd_aes_iec958; struct snd_cea_861_aud_if; -- 2.25.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] gpu/drm/msm: fix shutdown hook in case GPU components failed to bind
Quoting Rob Clark (2021-03-24 20:09:37)
> On Wed, Mar 24, 2021 at 6:49 PM Stephen Boyd wrote:
> >
> > Quoting Dmitry Baryshkov (2021-03-18 13:05:44)
> > > if GPU components have failed to bind, shutdown callback would fail with
> > > the following backtrace. Add safeguard check to stop that oops from
> > > happening and allow the board to reboot.
> > [...]
> > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > index 94525ac76d4e..fd2ac54caf9f 100644
> > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > @@ -1311,6 +1311,10 @@ static int msm_pdev_remove(struct platform_device
> > > *pdev)
> > > static void msm_pdev_shutdown(struct platform_device *pdev)
> > > {
> > > struct drm_device *drm = platform_get_drvdata(pdev);
> > > + struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
> > > +
> > > + if (!priv || !priv->kms)
> > > + return;
> > >
> >
> > I see a problem where if I don't get a backlight probing then my
> > graphics card doesn't appear but this driver is still bound. I was
> > hoping this patch would fix it but it doesn't. I have slab poisoning
> > enabled so sometimes the 'priv' pointer is 0x6b6b6b6b6b6b6b6b meaning it
> > got all freed.
> >
> > I found that the 'drm' pointer here is pointing at junk. The
> > msm_drm_init() function calls drm_dev_put() on the error path and that
> > will destroy the drm pointer but it doesn't update this platform drivers
> > drvdata. Do we need another patch that sets the drvdata to NULL on
> > msm_drm_init() failing? One last note, I'm seeing this on 5.4 so maybe I
> > missed something and the drvdata has been set to NULL somewhere else
> > upstream. I sort of doubt it though.
>
> the hw that I guess you are running on should work pretty well w/
> upstream kernel.. but I don't think there is any important delta
> between upstream and the 5.4 based kernel that you are running that
> would fix this..
>
> so *probably* you are right..
linux-next is failing like this today for me on Lazor right after the
screen turns on. I'll have to figure out what's wrong before checking
upstream.
[ 10.734752] Unable to handle kernel NULL pointer dereference at virtual
address 0080
[ 10.744482] Mem abort info:
[ 10.747462] ESR = 0x9606
[ 10.750644] EC = 0x25: DABT (current EL), IL = 32 bits
[ 10.756125] SET = 0, FnV = 0
[ 10.759290] EA = 0, S1PTW = 0
[ 10.762543] Data abort info:
[ 10.765519] ISV = 0, ISS = 0x0006
[ 10.769485] CM = 0, WnR = 0
[ 10.772553] user pgtable: 4k pages, 39-bit VAs, pgdp=000123474000
[ 10.779212] [0080] pgd=080123475003, p4d=080123475003,
pud=0800000123475003, pmd=
[ 10.790128] Internal error: Oops: 9606 [#1] PREEMPT SMP
[ 10.795856] Modules linked in: ath10k_snoc qmi_helpers ath10k_core ath
mac80211 cfg80211 r8152 mii joydev
[ 10.805705] CPU: 5 PID: 1576 Comm: DrmThread Not tainted
5.12.0-rc4-next-20210324+ #13
[ 10.813832] Hardware name: Google Lazor (rev3+) with KB Backlight (DT)
[ 10.820535] pstate: 8049 (Nzcv daif +PAN -UAO -TCO BTYPE=--)
[ 10.826703] pc : dpu_plane_atomic_update+0x80/0xcb8
[ 10.831730] lr : dpu_plane_restore+0x5c/0x88
[ 10.836117] sp : ffc012963920
[ 10.839521] x29: ffc0129639c0 x28: ffed5c9ad000
[ 10.844979] x27: ffed5c736000 x26: ffed5ca3f000
[ 10.850443] x25: ffed5c736000 x24:
[ 10.855903] x23: x22: ff80ad007400
[ 10.861361] x21: ff8085193808 x20:
[ 10.866818] x19: ff8085193800 x18: 0008
[ 10.872274] x17: 0080 x16: 2000
[ 10.877738] x15: 0001 x14:
[ 10.883201] x13: ff80852324a8 x12: 0008
[ 10.888657] x11: ffed5c3b7890 x10:
[ 10.894112] x9 : x8 :
[ 10.899570] x7 : 4000 x6 : 0001
[ 10.905026] x5 : 0004 x4 : 0800
[ 10.910482] x3 : x2 : 00020041
[ 10.915946] x1 : ff80ad2e2600 x0 : ff8085193800
[ 10.921402] Call trace:
[ 10.923923] dpu_plane_atomic_update+0x80/0xcb8
[ 10.928585] dpu_plane_restore+0x5c/0x88
[ 10.932620] dpu_crtc_atomic_flush+0xd4/0x1a0
[ 10.937105] drm_atomic_helper_commit_planes+0x1b4/0x1e0
[ 10.942565] msm_atomic_commit_tail+0x2d4/0x670
[ 10.947223] commit_tail+0xac/0x148
[ 10.950814] drm_atomic_helper_commit+0x104/0x10c
[ 10.955653] drm_atomic_commit+0x58/0x68
[ 10.959686] drm_mode_atomic_ioctl+0x438/0x51c
[ 10.964261] dr
Re: [PATCH v2] gpu/drm/msm: fix shutdown hook in case GPU components failed to bind
On Wed, Mar 24, 2021 at 6:49 PM Stephen Boyd wrote:
>
> Quoting Dmitry Baryshkov (2021-03-18 13:05:44)
> > if GPU components have failed to bind, shutdown callback would fail with
> > the following backtrace. Add safeguard check to stop that oops from
> > happening and allow the board to reboot.
> [...]
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 94525ac76d4e..fd2ac54caf9f 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -1311,6 +1311,10 @@ static int msm_pdev_remove(struct platform_device
> > *pdev)
> > static void msm_pdev_shutdown(struct platform_device *pdev)
> > {
> > struct drm_device *drm = platform_get_drvdata(pdev);
> > + struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
> > +
> > + if (!priv || !priv->kms)
> > + return;
> >
>
> I see a problem where if I don't get a backlight probing then my
> graphics card doesn't appear but this driver is still bound. I was
> hoping this patch would fix it but it doesn't. I have slab poisoning
> enabled so sometimes the 'priv' pointer is 0x6b6b6b6b6b6b6b6b meaning it
> got all freed.
>
> I found that the 'drm' pointer here is pointing at junk. The
> msm_drm_init() function calls drm_dev_put() on the error path and that
> will destroy the drm pointer but it doesn't update this platform drivers
> drvdata. Do we need another patch that sets the drvdata to NULL on
> msm_drm_init() failing? One last note, I'm seeing this on 5.4 so maybe I
> missed something and the drvdata has been set to NULL somewhere else
> upstream. I sort of doubt it though.
the hw that I guess you are running on should work pretty well w/
upstream kernel.. but I don't think there is any important delta
between upstream and the 5.4 based kernel that you are running that
would fix this..
so *probably* you are right..
BR,
-R
>
> ---8<
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index c842a270806d..895d74aa8834 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -577,6 +577,7 @@ static int msm_drm_init(struct device *dev, struct
> drm_driver *drv)
> kfree(priv);
> err_put_drm_dev:
> drm_dev_put(ddev);
> + platform_set_drvdata(pdev, NULL);
> return ret;
> }
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/doc: Add RFC section
Acked-by: Jason Ekstrand On March 24, 2021 16:10:48 Daniel Vetter wrote: Motivated by the pre-review process for i915 gem/gt features, but probably useful in general for complex stuff. v2: Add reminder to not forget userspace projects in the discussion (Simon, Jason) Cc: Simon Ser Cc: Jani Nikula Cc: Joonas Lahtinen Cc: Rodrigo Vivi Cc: Jason Ekstrand Cc: Dave Airlie Acked-by: Rodrigo Vivi Acked-by: Dave Airlie Signed-off-by: Daniel Vetter --- Documentation/gpu/index.rst | 1 + Documentation/gpu/rfc.rst | 17 + 2 files changed, 18 insertions(+) create mode 100644 Documentation/gpu/rfc.rst diff --git a/Documentation/gpu/index.rst b/Documentation/gpu/index.rst index c9a51e3bfb5a..df58cb826d68 100644 --- a/Documentation/gpu/index.rst +++ b/Documentation/gpu/index.rst @@ -16,6 +16,7 @@ Linux GPU Driver Developer's Guide vga-switcheroo vgaarbiter todo + rfc .. only:: subproject and html diff --git a/Documentation/gpu/rfc.rst b/Documentation/gpu/rfc.rst new file mode 100644 index ..a8621f7dab8b --- /dev/null +++ b/Documentation/gpu/rfc.rst @@ -0,0 +1,17 @@ +=== +GPU RFC Section +=== + +For complex work, especially new uapi, it is often good to nail the high level +design issues before getting lost in the code details. This section is meant to +host such documentation: + +* Each RFC should be a section in this file, explaining the goal and main design + considerations. Especially for uapi make sure you Cc: all relevant project + mailing lists and involved people outside of dri-devel. + +* For uapi structures add a file to this directory with and then pull the + kerneldoc in like with real uapi headers. + +* Once the code has landed move all the documentation to the right places in + the main core, helper or driver sections. -- 2.31.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] include: linux: host1x: Remove duplicate declaration
struct host1x is declared at 20th line. Remove the duplicate. Signed-off-by: Wan Jiabing --- include/linux/host1x.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/linux/host1x.h b/include/linux/host1x.h index ce59a6a6a008..462f0bc7a703 100644 --- a/include/linux/host1x.h +++ b/include/linux/host1x.h @@ -140,7 +140,6 @@ static inline void host1x_bo_munmap(struct host1x_bo *bo, void *addr) struct host1x_syncpt_base; struct host1x_syncpt; -struct host1x; struct host1x_syncpt *host1x_syncpt_get(struct host1x *host, u32 id); u32 host1x_syncpt_id(struct host1x_syncpt *sp); -- 2.25.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] [v3] drm/imx: imx-ldb: fix out of bounds array access warning
On Wed, 2021-03-24 at 17:47 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> When CONFIG_OF is disabled, building with 'make W=1' produces warnings
> about out of bounds array access:
>
> drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_set_clock.constprop':
> drivers/gpu/drm/imx/imx-ldb.c:186:8: error: array subscript -22 is below
> array bounds of 'struct clk *[4]' [-Werror=array-bounds]
>
> Add an error check before the index is used, which helps with the
> warning, as well as any possible other error condition that may be
> triggered at runtime.
>
> The warning could be fixed by adding a Kconfig depedency on CONFIG_OF,
> but Liu Ying points out that the driver may hit the out-of-bounds
> problem at runtime anyway.
Almost impossible to hit the out-of-bounds problem at runtime, unless
something wrong happens and makes unexpected parameters(node and/or
encoder) be handed over to drm_of_encoder_active_port_id(). Anyway, an
error check on return value from drm_of_encoder_active_port_id() looks
ok to me.
>
> Signed-off-by: Arnd Bergmann
Reviewed-by: Liu Ying
Thanks,
Liu Ying
> ---
> v3: fix build regression from v2
> v2: fix subject line
> expand patch description
> print mux number
> check upper bound as well
> ---
> drivers/gpu/drm/imx/imx-ldb.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index dbfe39e2f7f6..565482e2b816 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -197,6 +197,11 @@ static void imx_ldb_encoder_enable(struct drm_encoder
> *encoder)
> int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
>
> + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> + dev_warn(ldb->dev, "%s: invalid mux %d\n", __func__, mux);
> + return;
> + }
> +
> drm_panel_prepare(imx_ldb_ch->panel);
>
> if (dual) {
> @@ -255,6 +260,11 @@ imx_ldb_encoder_atomic_mode_set(struct drm_encoder
> *encoder,
> int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
> u32 bus_format = imx_ldb_ch->bus_format;
>
> + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> + dev_warn(ldb->dev, "%s: invalid mux %d\n", __func__, mux);
> + return;
> + }
> +
> if (mode->clock > 17) {
> dev_warn(ldb->dev,
>"%s: mode exceeds 170 MHz pixel clock\n", __func__);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] gpu/drm/msm: fix shutdown hook in case GPU components failed to bind
Quoting Dmitry Baryshkov (2021-03-18 13:05:44)
> if GPU components have failed to bind, shutdown callback would fail with
> the following backtrace. Add safeguard check to stop that oops from
> happening and allow the board to reboot.
[...]
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 94525ac76d4e..fd2ac54caf9f 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1311,6 +1311,10 @@ static int msm_pdev_remove(struct platform_device
> *pdev)
> static void msm_pdev_shutdown(struct platform_device *pdev)
> {
> struct drm_device *drm = platform_get_drvdata(pdev);
> + struct msm_drm_private *priv = drm ? drm->dev_private : NULL;
> +
> + if (!priv || !priv->kms)
> + return;
>
I see a problem where if I don't get a backlight probing then my
graphics card doesn't appear but this driver is still bound. I was
hoping this patch would fix it but it doesn't. I have slab poisoning
enabled so sometimes the 'priv' pointer is 0x6b6b6b6b6b6b6b6b meaning it
got all freed.
I found that the 'drm' pointer here is pointing at junk. The
msm_drm_init() function calls drm_dev_put() on the error path and that
will destroy the drm pointer but it doesn't update this platform drivers
drvdata. Do we need another patch that sets the drvdata to NULL on
msm_drm_init() failing? One last note, I'm seeing this on 5.4 so maybe I
missed something and the drvdata has been set to NULL somewhere else
upstream. I sort of doubt it though.
---8<
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index c842a270806d..895d74aa8834 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -577,6 +577,7 @@ static int msm_drm_init(struct device *dev, struct
drm_driver *drv)
kfree(priv);
err_put_drm_dev:
drm_dev_put(ddev);
+ platform_set_drvdata(pdev, NULL);
return ret;
}
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/msm: Add param for userspace to query suspend count
From: Rob Clark
Performance counts, and ALWAYS_ON counters used for capturing GPU
timestamps, lose their state across suspend/resume cycles. Userspace
tooling for performance monitoring needs to be aware of this. For
example, after a suspend userspace needs to recalibrate it's offset
between CPU and GPU time.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 3 +++
drivers/gpu/drm/msm/msm_drv.c | 1 +
drivers/gpu/drm/msm/msm_gpu.c | 2 ++
drivers/gpu/drm/msm/msm_gpu.h | 2 ++
include/uapi/drm/msm_drm.h | 1 +
5 files changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index f09175698827..e473b7c9ff7f 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -280,6 +280,9 @@ int adreno_get_param(struct msm_gpu *gpu, uint32_t param,
uint64_t *value)
case MSM_PARAM_FAULTS:
*value = gpu->global_faults;
return 0;
+ case MSM_PARAM_SUSPENDS:
+ *value = gpu->suspend_count;
+ return 0;
default:
DBG("%s: invalid param: %u", gpu->name, param);
return -EINVAL;
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index b29e439eb299..4f9fa0189a07 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -39,6 +39,7 @@
* GEM object's debug name
* - 1.5.0 - Add SUBMITQUERY_QUERY ioctl
* - 1.6.0 - Syncobj support
+ * - 1.7.0 - Add MSM_PARAM_SUSPENDS to access suspend count
*/
#define MSM_VERSION_MAJOR 1
#define MSM_VERSION_MINOR 6
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 7bdb01f202f4..ab888d83b887 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -256,6 +256,8 @@ int msm_gpu_pm_suspend(struct msm_gpu *gpu)
if (ret)
return ret;
+ gpu->suspend_count++;
+
return 0;
}
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index d7cd02cd2109..18baf935e143 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -152,6 +152,8 @@ struct msm_gpu {
ktime_t time;
} devfreq;
+ uint32_t suspend_count;
+
struct msm_gpu_state *crashstate;
/* True if the hardware supports expanded apriv (a650 and newer) */
bool hw_apriv;
diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h
index a6c1f3eb2623..5596d7c37f9e 100644
--- a/include/uapi/drm/msm_drm.h
+++ b/include/uapi/drm/msm_drm.h
@@ -76,6 +76,7 @@ struct drm_msm_timespec {
#define MSM_PARAM_NR_RINGS 0x07
#define MSM_PARAM_PP_PGTABLE 0x08 /* => 1 for per-process pagetables, else 0
*/
#define MSM_PARAM_FAULTS 0x09
+#define MSM_PARAM_SUSPENDS 0x0a
struct drm_msm_param {
__u32 pipe; /* in, MSM_PIPE_x */
--
2.29.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/msm: Fix a5xx/a6xx timestamps
From: Rob Clark
They were reading a counter that was configured to ALWAYS_COUNT (ie.
cycles that the GPU is doing something) rather than ALWAYS_ON. This
isn't the thing that userspace is looking for.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++--
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index a5af223eaf50..bb82fcd9df81 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1241,8 +1241,8 @@ static int a5xx_pm_suspend(struct msm_gpu *gpu)
static int a5xx_get_timestamp(struct msm_gpu *gpu, uint64_t *value)
{
- *value = gpu_read64(gpu, REG_A5XX_RBBM_PERFCTR_CP_0_LO,
- REG_A5XX_RBBM_PERFCTR_CP_0_HI);
+ *value = gpu_read64(gpu, REG_A5XX_RBBM_ALWAYSON_COUNTER_LO,
+ REG_A5XX_RBBM_ALWAYSON_COUNTER_HI);
return 0;
}
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 130661898546..59718c304488 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1173,8 +1173,8 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu,
uint64_t *value)
/* Force the GPU power on so we can read this register */
a6xx_gmu_set_oob(_gpu->gmu, GMU_OOB_GPU_SET);
- *value = gpu_read64(gpu, REG_A6XX_RBBM_PERFCTR_CP_0_LO,
- REG_A6XX_RBBM_PERFCTR_CP_0_HI);
+ *value = gpu_read64(gpu, REG_A6XX_CP_ALWAYS_ON_COUNTER_LO,
+ REG_A6XX_CP_ALWAYS_ON_COUNTER_HI);
a6xx_gmu_clear_oob(_gpu->gmu, GMU_OOB_GPU_SET);
return 0;
--
2.29.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/2] drm/msm: Fixes/updates for perfetto profiling
From: Rob Clark A couple kernel side things I realized I needed in the process of implementing performance-counter and render-stage support for perfetto, the first patch fixes the MSM_PARAM_TIMESTAMP query which was just wrong on a5xx/a6xx (ALWAYS_COUNT vs ALWAYS_ON). The second adds a way for userspace to determine whether the device has suspended since the last sampling period (which means counters have lost their state and configuration). I am a bit tempted to add a way that a CAP_SYS_ADMIN user could ask the GPU to not suspend (until the drm_file is closed), but so far I've managed to avoid needing this. Rob Clark (2): drm/msm: Fix a5xx/a6xx timestamps drm/msm: Add param for userspace to query suspend count drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++-- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 ++-- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 3 +++ drivers/gpu/drm/msm/msm_drv.c | 1 + drivers/gpu/drm/msm/msm_gpu.c | 2 ++ drivers/gpu/drm/msm/msm_gpu.h | 2 ++ include/uapi/drm/msm_drm.h | 1 + 7 files changed, 13 insertions(+), 4 deletions(-) -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 212427] [AMDGPU] Multiple ttm_bo_release warnings
https://bugzilla.kernel.org/show_bug.cgi?id=212427 Alex Deucher (alexdeuc...@gmail.com) changed: What|Removed |Added CC||alexdeuc...@gmail.com --- Comment #2 from Alex Deucher (alexdeuc...@gmail.com) --- Duplicate of https://bugzilla.kernel.org/show_bug.cgi?id=212425 -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PULL] nouveau-fixes 5.12
Single regression fix. Thanks, Ben. The following changes since commit d27ce83fa4baa5cb908a42e9878564cad6ea0eb3: Merge tag 'du-fixes-20210316' of git://linuxtv.org/pinchartl/media into drm-fixes (2021-03-22 13:49:55 +1000) are available in the Git repository at: git://github.com/skeggsb/linux linux-5.12 for you to fetch changes up to d3999c1f7bbbc100c167d7ad3cd79c1d10446ba2: drm/nouveau/kms/nve4-nv108: Limit cursors to 128x128 (2021-03-25 10:00:04 +1000) Lyude Paul (1): drm/nouveau/kms/nve4-nv108: Limit cursors to 128x128 drivers/gpu/drm/nouveau/dispnv50/disp.c | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 212425] Kernel warning at drivers/gpu/drm/ttm/ttm_bo.c:517
https://bugzilla.kernel.org/show_bug.cgi?id=212425 Erhard F. (erhar...@mailbox.org) changed: What|Removed |Added CC||erhar...@mailbox.org --- Comment #2 from Erhard F. (erhar...@mailbox.org) --- Created attachment 296039 --> https://bugzilla.kernel.org/attachment.cgi?id=296039=edit dmesg (kernel 5.11.9, Talos II) Yep, getting these too on my Talos II (ppc64) on kernel 5.11.9. Card is a Radeon HD6670. [...] [ cut here ] WARNING: CPU: 8 PID: 1229 at drivers/gpu/drm/ttm/ttm_bo.c:517 .ttm_bo_release+0x298/0x810 [ttm] Modules linked in: input_leds led_class joydev hid_generic usbhid hid fuse rfkill sd_mod uas usb_storage scsi_mod ecb xts ctr cbc radeon aes_generic libaes evdev snd_hda_codec_hdmi> CPU: 8 PID: 1229 Comm: X Tainted: GW 5.11.9-gentoo-TalosII #1 NIP: c0081a1a7278 LR: c0081a1a7264 CTR: REGS: c00144763370 TRAP: 0700 Tainted: GW (5.11.9-gentoo-TalosII) MSR: 90029032 CR: 44244248 XER: 2004 CFAR: c0bcd1f4 IRQMASK: 0 GPR00: c0081a1a7264 c00144763610 c0081a1ba900 c0081a1b47e8 GPR04: 28b1 722135af 0008 GPR08: 0001 0001 0001 c155d3f8 GPR12: 44244248 c007d000 3fff 0313 GPR16: 0010 3ed97f00 0001467b5a30 GPR20: c1206d10 c0004b96 c1206d18 0001 GPR24: c101deb8 c0002d5166c0 c0081a1b47e8 c0002d516400 GPR28: c0002a668a28 c0081a1b41d0 c0002d516650 NIP [c0081a1a7278] .ttm_bo_release+0x298/0x810 [ttm] LR [c0081a1a7264] .ttm_bo_release+0x284/0x810 [ttm] Call Trace: [c00144763610] [c0081a1a7264] .ttm_bo_release+0x284/0x810 [ttm] (unreliable) [c001447636e0] [c0081a1ab154] .ttm_bo_move_accel_cleanup+0x2a4/0x570 [ttm] [c001447637a0] [c0081bcdea3c] .radeon_bo_move+0x40c/0x610 [radeon] [c00144763870] [c0081a1a621c] .ttm_bo_handle_move_mem+0xac/0x1e0 [ttm] [c00144763920] [c0081a1a9570] .ttm_bo_validate+0x1b0/0x260 [ttm] [c00144763a20] [c0081bce1480] .radeon_bo_fault_reserve_notify+0x130/0x260 [radeon] [c00144763ae0] [c0081bcde518] .radeon_ttm_fault+0x98/0x1b0 [radeon] [c00144763b70] [c0347cf8] .__do_fault+0x58/0x120 [c00144763bf0] [c034f32c] .handle_mm_fault+0x15ec/0x1f80 Modules linked in: auth_rpcgss nfsv4 dns_resolver nfs nfs_ssc lockd grace sunrpc input_leds led_class joydev hid_generic usbhid hid fuse rfkill sd_mod uas usb_storage scsi_mod ecb > CPU: 1 PID: 1229 Comm: X Tainted: G D W 5.11.9-gentoo-TalosII #1 NIP: c0081a1a7278 LR: c0081a1a7264 CTR: c0bcd190 REGS: c00144763370 TRAP: 0700 Tainted: G D W (5.11.9-gentoo-TalosII) MSR: 90029032 CR: 44044244 XER: 2004 CFAR: c0bcd1f4 IRQMASK: 0 GPR00: c0081a1a7264 c00144763610 c0081a1ba900 c0081a1b47e8 GPR04: 0001 GPR08: 0001 0001 0001 7fff GPR12: c0bcd190 c007ec00 0080 000146899640 GPR16: 3fffa7cf1208 3fffa7c8d440 000146899840 3fffa7c889f8 GPR20: c1206d10 c0004b96 c1206d18 0001 GPR24: c101deb8 c000bd492ac0 c0081a1b47e8 c000bd492800 GPR28: c0002a668a28 c000bd492a50 NIP [c0081a1a7278] .ttm_bo_release+0x298/0x810 [ttm] LR [c0081a1a7264] .ttm_bo_release+0x284/0x810 [ttm] Call Trace: [c00144763610] [c0081a1a7264] .ttm_bo_release+0x284/0x810 [ttm] (unreliable) [c001447636e0] [c0081a1ab154] .ttm_bo_move_accel_cleanup+0x2a4/0x570 [ttm] [c001447637a0] [c0081bcdea3c] .radeon_bo_move+0x40c/0x610 [radeon] [c00144763870] [c0081a1a621c] .ttm_bo_handle_move_mem+0xac/0x1e0 [ttm] [c00144763920] [c0081a1a9570] .ttm_bo_validate+0x1b0/0x260 [ttm] [c00144763a20] [c0081bce1480] .radeon_bo_fault_reserve_notify+0x130/0x260 [radeon] [c00144763ae0] [c0081bcde518] .radeon_ttm_fault+0x98/0x1b0 [radeon] [c00144763b70] [c0347cf8] .__do_fault+0x58/0x120 [c00144763bf0] [c034f32c] .handle_mm_fault+0x15ec/0x1f80 [c00144763d40] [c0063e84] .do_page_fault+0x2b4/0xa00 [c00144763e10] [c000c218] handle_page_fault+0x10/0x2c --- interrupt: 300 at 0x3fffa7ca6d24 NIP: 3fffa7ca6d24 LR: 3fffa7ca6d00 CTR: 3fffb21186bc REGS: c00144763e80 TRAP: 0300 Tainted: G D W (5.11.9-gentoo-TalosII) MSR: 9280f032 CR: 44028842 XER: CFAR: 3fffb3ac5a58 DAR:
Re: [PATCH V2] drm/radeon/r600_cs: Few typo fixes
On 3/24/21 4:29 PM, Bhaskar Chowdhury wrote:
> s/miror/mirror/
> s/needind/needing/
> s/informations/information/
>
> Signed-off-by: Bhaskar Chowdhury
Acked-by: Randy Dunlap
Thanks.
> ---
> Changes from V1:
> Randy's finding incorporated ,i.e in one place,informations->information
> Adjusted the subject line accordingly
>
> drivers/gpu/drm/radeon/r600_cs.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/r600_cs.c
> b/drivers/gpu/drm/radeon/r600_cs.c
> index 34b7c6f16479..8be4799a98ef 100644
> --- a/drivers/gpu/drm/radeon/r600_cs.c
> +++ b/drivers/gpu/drm/radeon/r600_cs.c
> @@ -38,7 +38,7 @@ extern void r600_cs_legacy_get_tiling_conf(struct
> drm_device *dev, u32 *npipes,
>
>
> struct r600_cs_track {
> - /* configuration we miror so that we use same code btw kms/ums */
> + /* configuration we mirror so that we use same code btw kms/ums */
> u32 group_size;
> u32 nbanks;
> u32 npipes;
> @@ -963,7 +963,7 @@ static int r600_cs_parse_packet0(struct radeon_cs_parser
> *p,
> *
> * This function will test against r600_reg_safe_bm and return 0
> * if register is safe. If register is not flag as safe this function
> - * will test it against a list of register needind special handling.
> + * will test it against a list of register needing special handling.
> */
> static int r600_cs_check_reg(struct radeon_cs_parser *p, u32 reg, u32 idx)
> {
> @@ -2336,7 +2336,7 @@ int r600_cs_parse(struct radeon_cs_parser *p)
> /**
> * r600_dma_cs_next_reloc() - parse next reloc
> * @p: parser structure holding parsing context.
> - * @cs_reloc:reloc informations
> + * @cs_reloc:reloc information
> *
> * Return the next reloc, do bo validation and compute
> * GPU offset using the provided start.
> --
--
~Randy
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Nouveau] [PATCH] drm/nouveau/kms/nv04: use vzalloc for nv04_display
On Mon, 8 Mar 2021 at 03:49, Ilia Mirkin wrote: > > The struct is giant, and triggers an order-7 allocation (512K). There is > no reason for this to be kmalloc-type memory, so switch to vmalloc. This > should help loading nouveau on low-memory and/or long-running systems. > > Reported-by: Nathan E. Egge > Signed-off-by: Ilia Mirkin > Cc: sta...@vger.kernel.org Thanks! > --- > drivers/gpu/drm/nouveau/dispnv04/disp.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c > b/drivers/gpu/drm/nouveau/dispnv04/disp.c > index 7739f46470d3..99fee4d8cd31 100644 > --- a/drivers/gpu/drm/nouveau/dispnv04/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv04/disp.c > @@ -205,7 +205,7 @@ nv04_display_destroy(struct drm_device *dev) > nvif_notify_dtor(>flip); > > nouveau_display(dev)->priv = NULL; > - kfree(disp); > + vfree(disp); > > nvif_object_unmap(>client.device.object); > } > @@ -223,7 +223,7 @@ nv04_display_create(struct drm_device *dev) > struct nv04_display *disp; > int i, ret; > > - disp = kzalloc(sizeof(*disp), GFP_KERNEL); > + disp = vzalloc(sizeof(*disp)); > if (!disp) > return -ENOMEM; > > -- > 2.26.2 > > ___ > Nouveau mailing list > nouv...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/radeon/r600_cs: Couple of typo fixes
On 14:48 Wed 24 Mar 2021, Randy Dunlap wrote:
On 3/24/21 6:50 AM, Bhaskar Chowdhury wrote:
s/miror/mirror/
s/needind/needing/
Signed-off-by: Bhaskar Chowdhury
---
drivers/gpu/drm/radeon/r600_cs.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c
index 34b7c6f16479..aded1f2264e0 100644
--- a/drivers/gpu/drm/radeon/r600_cs.c
+++ b/drivers/gpu/drm/radeon/r600_cs.c
@@ -38,7 +38,7 @@ extern void r600_cs_legacy_get_tiling_conf(struct drm_device
*dev, u32 *npipes,
struct r600_cs_track {
- /* configuration we miror so that we use same code btw kms/ums */
+ /* configuration we mirror so that we use same code btw kms/ums */
u32 group_size;
u32 nbanks;
u32 npipes;
@@ -963,7 +963,7 @@ static int r600_cs_parse_packet0(struct radeon_cs_parser *p,
*
* This function will test against r600_reg_safe_bm and return 0
* if register is safe. If register is not flag as safe this function
- * will test it against a list of register needind special handling.
+ * will test it against a list of register needing special handling.
*/
static int r600_cs_check_reg(struct radeon_cs_parser *p, u32 reg, u32 idx)
{
--
Those 2 LGTM, but please fix this while you are touching this file:
drivers/gpu/drm/radeon/r600_cs.c:2339: informations ==> information
Thanks, I have sent a V2 with the change...
thanks.
--
~Randy
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH V2] drm/radeon/r600_cs: Few typo fixes
s/miror/mirror/
s/needind/needing/
s/informations/information/
Signed-off-by: Bhaskar Chowdhury
---
Changes from V1:
Randy's finding incorporated ,i.e in one place,informations->information
Adjusted the subject line accordingly
drivers/gpu/drm/radeon/r600_cs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c
index 34b7c6f16479..8be4799a98ef 100644
--- a/drivers/gpu/drm/radeon/r600_cs.c
+++ b/drivers/gpu/drm/radeon/r600_cs.c
@@ -38,7 +38,7 @@ extern void r600_cs_legacy_get_tiling_conf(struct drm_device
*dev, u32 *npipes,
struct r600_cs_track {
- /* configuration we miror so that we use same code btw kms/ums */
+ /* configuration we mirror so that we use same code btw kms/ums */
u32 group_size;
u32 nbanks;
u32 npipes;
@@ -963,7 +963,7 @@ static int r600_cs_parse_packet0(struct radeon_cs_parser *p,
*
* This function will test against r600_reg_safe_bm and return 0
* if register is safe. If register is not flag as safe this function
- * will test it against a list of register needind special handling.
+ * will test it against a list of register needing special handling.
*/
static int r600_cs_check_reg(struct radeon_cs_parser *p, u32 reg, u32 idx)
{
@@ -2336,7 +2336,7 @@ int r600_cs_parse(struct radeon_cs_parser *p)
/**
* r600_dma_cs_next_reloc() - parse next reloc
* @p: parser structure holding parsing context.
- * @cs_reloc: reloc informations
+ * @cs_reloc: reloc information
*
* Return the next reloc, do bo validation and compute
* GPU offset using the provided start.
--
2.30.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] nouveau/nvkm/subdev/devinit/mcp89.c:Unneeded variable
On Wed, 17 Mar 2021 at 19:51, ChunyouTang wrote:
>
> From: tangchunyou
>
> disable,delete disable and return 0
>
> Signed-off-by: tangchunyou
Thanks!
> ---
> drivers/gpu/drm/nouveau/nvkm/subdev/devinit/mcp89.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/devinit/mcp89.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/devinit/mcp89.c
> index fb90d47..a9cdf24 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/devinit/mcp89.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/devinit/mcp89.c
> @@ -32,7 +32,6 @@
> struct nvkm_device *device = init->subdev.device;
> u32 r001540 = nvkm_rd32(device, 0x001540);
> u32 r00154c = nvkm_rd32(device, 0x00154c);
> - u64 disable = 0;
>
> if (!(r001540 & 0x4000)) {
> nvkm_subdev_disable(device, NVKM_ENGINE_MSPDEC, 0);
> @@ -48,7 +47,7 @@
> if (!(r00154c & 0x0200))
> nvkm_subdev_disable(device, NVKM_ENGINE_CE, 0);
>
> - return disable;
> + return 0;
> }
>
> static const struct nvkm_devinit_func
> --
> 1.9.1
>
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/nouveau/kms/nv50-: Correct size checks for cursors
On Fri, 19 Mar 2021 at 09:04, Lyude Paul wrote:
>
> Found this while trying to make some changes to the kms_cursor_crc test.
> curs507a_acquire checks that the width and height of the cursor framebuffer
> are equal (asyw->image.{w,h}). This isn't entirely correct though, as the
> height of the cursor can be larger than the size of the cursor, as long as
> the width is the same as the cursor size and there's no framebuffer offset.
>
> Note that I'm not entirely sure why this wasn't previously breaking
> kms_cursor_crc tests - they all set up cursors with the height being one
> pixel larger than the actual size of the cursor. But this seems to fix
> things, and the code before was definitely incorrect - so it's not really
> worth looking into further imho.
>
> Changes since v1:
> * Don't use crtc_w everywhere for determining cursor layout, just use fb
> size again
> * Change check so that we only check that the w/h of the cursor plane is
> the same, the width of the scanout surface is the same as the framebuffer
> width, and that there's no offset being used for the cursor surface.
>
> Signed-off-by: Lyude Paul
> Cc: Martin Peres
> Cc: Jeremy Cline
Thanks Lyude!
> ---
> drivers/gpu/drm/nouveau/dispnv50/curs507a.c | 15 ++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> index 54fbd6fe751d..00e19fd959ea 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/curs507a.c
> @@ -98,6 +98,7 @@ static int
> curs507a_acquire(struct nv50_wndw *wndw, struct nv50_wndw_atom *asyw,
> struct nv50_head_atom *asyh)
> {
> + struct nouveau_drm *drm = nouveau_drm(wndw->plane.dev);
> struct nv50_head *head = nv50_head(asyw->state.crtc);
> int ret;
>
> @@ -109,8 +110,20 @@ curs507a_acquire(struct nv50_wndw *wndw, struct
> nv50_wndw_atom *asyw,
> if (ret || !asyh->curs.visible)
> return ret;
>
> - if (asyw->image.w != asyw->image.h)
> + if (asyw->state.crtc_w != asyw->state.crtc_h) {
> + NV_ATOMIC(drm, "Plane width/height must be equal for
> cursors\n");
> return -EINVAL;
> + }
> +
> + if (asyw->image.w != asyw->state.crtc_w) {
> + NV_ATOMIC(drm, "Plane width must be equal to fb width for
> cursors (height can be larger though)\n");
> + return -EINVAL;
> + }
> +
> + if (asyw->state.src_x || asyw->state.src_y) {
> + NV_ATOMIC(drm, "Cursor planes do not support framebuffer
> offsets\n");
> + return -EINVAL;
> + }
>
> ret = head->func->curs_layout(head, asyw, asyh);
> if (ret)
> --
> 2.29.2
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Nouveau] [PATCH -next] drm/nouveau/core/client: Mark nvkm_uclient_sclass with static keyword
On Tue, 23 Mar 2021 at 17:03, Zou Wei wrote:
>
> Fix the following sparse warning:
>
> drivers/gpu/drm/nouveau/nvkm/core/client.c:64:1: warning: symbol
> 'nvkm_uclient_sclass' was not declared. Should it be static?
>
> Signed-off-by: Zou Wei
Applied, thanks.
> ---
> drivers/gpu/drm/nouveau/nvkm/core/client.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/core/client.c
> b/drivers/gpu/drm/nouveau/nvkm/core/client.c
> index ac671202919e..0c8c55c73b12 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/core/client.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/core/client.c
> @@ -60,7 +60,7 @@ nvkm_uclient_new(const struct nvkm_oclass *oclass, void
> *argv, u32 argc,
> return 0;
> }
>
> -const struct nvkm_sclass
> +static const struct nvkm_sclass
> nvkm_uclient_sclass = {
> .oclass = NVIF_CLASS_CLIENT,
> .minver = 0,
> --
> 2.17.1
>
> ___
> Nouveau mailing list
> nouv...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/2] mm,drm/ttm: Block fast GUP to TTM huge pages
On Wed, Mar 24, 2021 at 09:07:53PM +0100, Thomas Hellström (Intel) wrote: > > On 3/24/21 7:31 PM, Christian König wrote: > > > > > > Am 24.03.21 um 17:38 schrieb Jason Gunthorpe: > > > On Wed, Mar 24, 2021 at 04:50:14PM +0100, Thomas Hellström (Intel) > > > wrote: > > > > On 3/24/21 2:48 PM, Jason Gunthorpe wrote: > > > > > On Wed, Mar 24, 2021 at 02:35:38PM +0100, Thomas Hellström > > > > > (Intel) wrote: > > > > > > > > > > > > In an ideal world the creation/destruction of page > > > > > > > table levels would > > > > > > > by dynamic at this point, like THP. > > > > > > Hmm, but I'm not sure what problem we're trying to solve > > > > > > by changing the > > > > > > interface in this way? > > > > > We are trying to make a sensible driver API to deal with huge pages. > > > > > > Currently if the core vm requests a huge pud, we give it > > > > > > one, and if we > > > > > > can't or don't want to (because of dirty-tracking, for > > > > > > example, which is > > > > > > always done on 4K page-level) we just return > > > > > > VM_FAULT_FALLBACK, and the > > > > > > fault is retried at a lower level. > > > > > Well, my thought would be to move the pte related stuff into > > > > > vmf_insert_range instead of recursing back via VM_FAULT_FALLBACK. > > > > > > > > > > I don't know if the locking works out, but it feels cleaner that the > > > > > driver tells the vmf how big a page it can stuff in, not the vm > > > > > telling the driver to stuff in a certain size page which it might not > > > > > want to do. > > > > > > > > > > Some devices want to work on a in-between page size like 64k so they > > > > > can't form 2M pages but they can stuff 64k of 4K pages in a batch on > > > > > every fault. > > > > Hmm, yes, but we would in that case be limited anyway to insert ranges > > > > smaller than and equal to the fault size to avoid extensive and > > > > possibly > > > > unnecessary checks for contigous memory. > > > Why? The insert function is walking the page tables, it just updates > > > things as they are. It learns the arragement for free while doing the > > > walk. > > > > > > The device has to always provide consistent data, if it overlaps into > > > pages that are already populated that is fine so long as it isn't > > > changing their addresses. > > > > > > > And then if we can't support the full fault size, we'd need to > > > > either presume a size and alignment of the next level or search for > > > > contigous memory in both directions around the fault address, > > > > perhaps unnecessarily as well. > > > You don't really need to care about levels, the device should be > > > faulting in the largest memory regions it can within its efficiency. > > > > > > If it works on 4M pages then it should be faulting 4M pages. The page > > > size of the underlying CPU doesn't really matter much other than some > > > tuning to impact how the device's allocator works. > > Yes, but then we'd be adding a lot of complexity into this function that is > already provided by the current interface for DAX, for little or no gain, at > least in the drm/ttm setting. Please think of the following situation: You > get a fault, you do an extensive time-consuming scan of your VRAM buffer > object into which the fault goes and determine you can fault 1GB. Now you > hand it to vmf_insert_range() and because the user-space address is > misaligned, or already partly populated because of a previous eviction, you > can only fault single pages, and you end up faulting a full GB of single > pages perhaps for a one-time small update. Why would "you can only fault single pages" ever be true? If you have 1GB of pages then the vmf_insert_range should allocate enough page table entries to consume it, regardless of alignment. And why shouldn't DAX switch to this kind of interface anyhow? It is basically exactly the same problem. The underlying filesystem block size is *not* necessarily aligned to the CPU page table sizes and DAX would benefit from better handling of this mismatch. > On top of this, unless we want to do the walk trying increasingly smaller > sizes of vmf_insert_xxx(), we'd have to use apply_to_page_range() and teach > it about transhuge page table entries, because pagewalk.c can't be used (It > can't populate page tables). That also means apply_to_page_range() needs to > be complicated with page table locks since transhuge pages aren't stable and > can be zapped and refaulted under us while we do the walk. I didn't say it would be simple :) But we also need to stop hacking around the sides of all this huge page stuff and come up with sensible APIs that drivers can actually implement correctly. Exposing drivers to specific kinds of page levels really feels like the wrong level of abstraction. Once we start doing this we should do it everywhere, the io_remap_pfn stuff should be able to create huge special IO pages as well, for instance. > On top of this, the user-space address allocator needs to know how large gpu >
[Bug 212427] [AMDGPU] Multiple ttm_bo_release warnings
https://bugzilla.kernel.org/show_bug.cgi?id=212427
Chris Rankin (ranki...@yahoo.com) changed:
What|Removed |Added
Regression|No |Yes
--- Comment #1 from Chris Rankin (ranki...@yahoo.com) ---
I am assuming that these warnings are all due to this one patch:
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 22073e77fdf9a..a76eb2c14e8c5 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -514,7 +514,7 @@ static void ttm_bo_release(struct kref *kref)
* shrinkers, now that they are queued for
* destruction.
*/
- if (bo->pin_count) {
+ if (WARN_ON(bo->pin_count)) {
bo->pin_count = 0;
ttm_bo_del_from_lru(bo);
ttm_bo_add_mem_to_lru(bo, >mem);
which would imply that the only real difference between 5.11.8 and 5.11.9 is
"noise". Is this warning highlighting a real problem?
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 11/11] drm/bridge: ti-sn65dsi86: Support hotplug detection
Hi,
On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart
wrote:
>
> When the SN65DSI86 is used in DisplayPort mode, its output is likely
> routed to a DisplayPort connector, which can benefit from hotplug
> detection. Support it in such cases, with polling mode only for now.
>
> The implementation is limited to the bridge operations, as the connector
> operations are legacy and new users should use
> DRM_BRIDGE_ATTACH_NO_CONNECTOR.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 46 +++
> 1 file changed, 33 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index f792227142a7..72f6362adf44 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -167,6 +167,8 @@ struct ti_sn_bridge {
> struct gpio_chipgchip;
> DECLARE_BITMAP(gchip_output, SN_NUM_GPIOS);
> #endif
> +
> + boolno_hpd;
This structure is documented by kernel-doc, but you didn't add your new member.
> };
>
> static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> @@ -862,23 +864,28 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge
> *bridge)
> ti_sn_bridge_set_refclk_freq(pdata);
>
> /*
> -* HPD on this bridge chip is a bit useless. This is an eDP bridge
> -* so the HPD is an internal signal that's only there to signal that
> -* the panel is done powering up. ...but the bridge chip debounces
> -* this signal by between 100 ms and 400 ms (depending on process,
> -* voltage, and temperate--I measured it at about 200 ms). One
> +* As this is an eDP bridge, the output will be connected to a fixed
> +* panel in most systems. HPD is in that case only an internal signal
> +* to signal that the panel is done powering up. The bridge chip
> +* debounces this signal by between 100 ms and 400 ms (depending on
> +* process, voltage, and temperate--I measured it at about 200 ms).
> One
> * particular panel asserted HPD 84 ms after it was powered on meaning
> * that we saw HPD 284 ms after power on. ...but the same panel said
> * that instead of looking at HPD you could just hardcode a delay of
> -* 200 ms. We'll assume that the panel driver will have the hardcoded
> -* delay in its prepare and always disable HPD.
> +* 200 ms. HPD is thus a bit useless. For this type of use cases,
> we'll
> +* assume that the panel driver will have the hardcoded delay in its
> +* prepare and always disable HPD.
> *
> -* If HPD somehow makes sense on some future panel we'll have to
> -* change this to be conditional on someone specifying that HPD should
> -* be used.
> +* However, on some systems, the output is connected to a DisplayPort
> +* connector. HPD is needed in such cases. To accommodate both use
> +* cases, enable HPD only when requested.
> */
> - regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
> - HPD_DISABLE);
> + if (pdata->no_hpd)
> + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> + HPD_DISABLE, HPD_DISABLE);
> + else
> + regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> + HPD_DISABLE, 0);
Optionally you could skip the "else". HPD enabled is the default state
and, in general, we don't exhaustively init all registers and rely on
the power-on defaults for ones we don't explicitly control.
> }
>
> static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> @@ -890,6 +897,15 @@ static void ti_sn_bridge_post_disable(struct drm_bridge
> *bridge)
> pm_runtime_put_sync(pdata->dev);
> }
>
> +static enum drm_connector_status ti_sn_bridge_detect(struct drm_bridge
> *bridge)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> + int val;
> +
> + regmap_read(pdata->regmap, SN_HPD_DISABLE_REG, );
> + return val ? connector_status_connected :
> connector_status_disconnected;
I would have expected that you would have used the interrupt signal,
but I guess it just polls in this case. I suppose polling has the
advantage that it's simpler... Maybe throw in a comment about why IRQ
isn't being used?
> +}
> +
> static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> struct drm_connector *connector)
> {
> @@ -904,6 +920,7 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs =
> {
> .enable = ti_sn_bridge_enable,
> .disable = ti_sn_bridge_disable,
> .post_disable = ti_sn_bridge_post_disable,
> + .detect = ti_sn_bridge_detect,
> .get_edid =
Re: [RFC PATCH 10/11] drm/bridge: ti-sn65dsi86: Support DisplayPort (non-eDP) mode
Hi, On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote: > > Despite the SN65DSI86 being an eDP bridge, on some systems its output is > routed to a DisplayPort connector. Enable DisplayPort mode when the next > component in the display pipeline is not a panel, and disable eDP > features in that case. > > Signed-off-by: Laurent Pinchart > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 --- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index e2527d597ccb..f792227142a7 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -55,6 +55,7 @@ > #define SN_LN_ASSIGN_REG 0x59 > #define LN_ASSIGN_WIDTH 2 > #define SN_ENH_FRAME_REG 0x5A > +#define ASSR_CONTROL BIT(0) > #define VSTREAM_ENABLEBIT(3) > #define LN_POLRS_OFFSET 4 > #define LN_POLRS_MASK 0xf0 > @@ -86,6 +87,8 @@ > #define SN_DATARATE_CONFIG_REG 0x94 > #define DP_DATARATE_MASK GENMASK(7, 5) > #define DP_DATARATE(x)((x) << 5) > +#define SN_TRAINING_SETTING_REG0x95 > +#define SCRAMBLE_DISABLE BIT(4) > #define SN_ML_TX_MODE_REG 0x96 > #define ML_TX_MAIN_LINK_OFF 0 > #define ML_TX_NORMAL_MODE BIT(0) > @@ -723,6 +726,11 @@ static int ti_sn_link_training(struct ti_sn_bridge > *pdata, int dp_rate_idx, > regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG, >DP_DATARATE_MASK, DP_DATARATE(dp_rate_idx)); > > + /* For DisplayPort, use the standard DP scrambler seed. */ > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > + regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, > + ASSR_CONTROL, 0); I don't actually know anything about DP scrambler seeds. However: 1. From reading the docs, this field seems to be documented to be "read only" unless: 1a) The "TEST2" pin is pulled high when you power on the bridge. 1b) You set "ASSR_OVERRIDE" (page select to page 7, write to register 0x16, page select back to page 0). I don't know if TEST2 is being pulled high in your hardware, but at least I can see that 1b) isn't done. So I'm guessing that this line is a no-op? If I had to guess from all the hoops they're making you jump through there's some sort of errata around standard scrambling on this bridge chip. Are you sure it works OK? 2. The docs I see claim that this field is 2 bits big. It seems like it would be nice to honor. Yeah, it's silly because 0x11 and 0x10 are "reserved" so it's really more like a 1-bit field, but still seems like it would be better to set both bits, or at least add a comment explaining why you're not matching the datasheet. 3. Your patch doesn't seem to touch the bit of code in ti_sn_bridge_enable() that says this: /** * The SN65DSI86 only supports ASSR Display Authentication method and * this method is enabled by default. An eDP panel must support this * authentication method. We need to enable this method in the eDP panel * at DisplayPort address 0x0010A prior to link training. */ drm_dp_dpcd_writeb(>aux, DP_EDP_CONFIGURATION_SET, DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); Won't that be a problem? > + > /* enable DP PLL */ > regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1); > > @@ -734,6 +742,11 @@ static int ti_sn_link_training(struct ti_sn_bridge > *pdata, int dp_rate_idx, > goto exit; > } > > + /* For DisplayPort, disable scrambling mode. */ > + if (pdata->bridge.type == DRM_MODE_CONNECTOR_DisplayPort) > + regmap_update_bits(pdata->regmap, SN_TRAINING_SETTING_REG, > + SCRAMBLE_DISABLE, SCRAMBLE_DISABLE); I'm assuming that this is the important part of your patch? Would be sorta nice to include the "why" in your comment. Why do you want to disable scrambling mode for DP but not for eDP? Maybe you care about compatibility but not EMI if you're hooking up to random DP things? -Doug ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 08/11] drm/bridge: ti-sn65dsi86: Implement bridge connector operations
Hi,
On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart
wrote:
>
> Implement the bridge connector-related .get_edid() operation, and report
> the related bridge capabilities and type. The .get_edid() operation is
> implemented with the same backend as the EDID retrieval from the
> connector .get_modes() operation.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 30 ++-
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index dc300fab4319..6f6e075544e8 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -261,6 +261,18 @@ static void ti_sn_debugfs_remove(struct ti_sn_bridge
> *pdata)
> pdata->debugfs = NULL;
> }
>
> +static struct edid *__ti_sn_bridge_get_edid(struct ti_sn_bridge *pdata,
> + struct drm_connector *connector)
> +{
> + struct edid *edid;
> +
> + pm_runtime_get_sync(pdata->dev);
> + edid = drm_get_edid(connector, >aux.ddc);
> + pm_runtime_put(pdata->dev);
As mentioned in my patch [1], the above is a bit iffy for eDP.
Specifically just doing a pm_runtime_get_sync() isn't enough to
actually be able to read the EDID. We also need to do any panel power
sequencing and potentially set the "ignore HPD" bit in the bridge to
actually read the EDID.
I'll try to find some time to make this better--let's see if I get
distracted before I manage it.
> +
> + return edid;
> +}
> +
> /*
> -
> * DRM Connector Operations
> */
> @@ -277,11 +289,8 @@ static int ti_sn_bridge_connector_get_modes(struct
> drm_connector *connector)
> struct edid *edid = pdata->edid;
> int num, ret;
>
> - if (!edid) {
> - pm_runtime_get_sync(pdata->dev);
> - edid = pdata->edid = drm_get_edid(connector, >aux.ddc);
> - pm_runtime_put(pdata->dev);
> - }
> + if (!edid)
> + edid = pdata->edid = __ti_sn_bridge_get_edid(pdata,
> connector);
It feels weird to me that we are now exposing the get_edid() function
directly but we still need to implement get_modes(). I guess this is
because we might need to fallback to the hardcoded modes? ...but that
seems like it would be a common pattern for eDP bridges...
> if (edid && drm_edid_is_valid(edid)) {
> ret = drm_connector_update_edid_property(connector, edid);
> @@ -871,12 +880,21 @@ static void ti_sn_bridge_post_disable(struct drm_bridge
> *bridge)
> pm_runtime_put_sync(pdata->dev);
> }
>
> +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> + struct drm_connector *connector)
> +{
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> + return __ti_sn_bridge_get_edid(pdata, connector);
> +}
> +
> static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> .attach = ti_sn_bridge_attach,
> .pre_enable = ti_sn_bridge_pre_enable,
> .enable = ti_sn_bridge_enable,
> .disable = ti_sn_bridge_disable,
> .post_disable = ti_sn_bridge_post_disable,
> + .get_edid = ti_sn_bridge_get_edid,
> };
>
> /*
> -
> @@ -1335,6 +1353,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>
> pdata->bridge.funcs = _sn_bridge_funcs;
> pdata->bridge.of_node = client->dev.of_node;
> + pdata->bridge.ops = DRM_BRIDGE_OP_EDID;
> + pdata->bridge.type = DRM_MODE_CONNECTOR_eDP;
Even with the few comments above, I think this is still fine to move
us in the right direction. Unless I manage to fix up the EDID reading
(in which case your patch would conflict and would need to be
tweaked), then:
Reviewed-by: Douglas Anderson
[1]
https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 23:36 +0100, Rasmus Villemoes wrote: > On 24/03/2021 23.18, Joe Perches wrote: > > There's no silly game here. %pe would either print a string or a value. > > A hashed value, that is, never the raw value. There is value in printing the raw value. As discussed, it can simplify the code. The worry about exposing a ptr value is IMO overstated. It's trivial to inspect the uses and _all_ %p uses need inspection and validation at acceptance anyway. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 212427] New: [AMDGPU] Multiple ttm_bo_release warnings
https://bugzilla.kernel.org/show_bug.cgi?id=212427 Bug ID: 212427 Summary: [AMDGPU] Multiple ttm_bo_release warnings Product: Drivers Version: 2.5 Kernel Version: 5.11.9 Hardware: x86-64 OS: Linux Tree: Mainline Status: NEW Severity: normal Priority: P1 Component: Video(DRI - non Intel) Assignee: drivers_video-...@kernel-bugs.osdl.org Reporter: ranki...@yahoo.com Regression: No Created attachment 296037 --> https://bugzilla.kernel.org/attachment.cgi?id=296037=edit Full dmesg log from 5.11.9 After upgrading from 5.11.8 to 5.11.9, I am seeing warnings like this in my dmesg log: [ 43.393574] WARNING: CPU: 2 PID: 1268 at drivers/gpu/drm/ttm/ttm_bo.c:517 ttm_bo_release+0x172/0x282 [ttm] [ 43.401940] Modules linked in: nf_nat_ftp nf_conntrack_ftp cfg80211 af_packet nft_ct nft_chain_nat nf_tables ebtable_nat ebtable_broute ip6table_nat ip6table_mangle ip6table_raw iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 libcrc32c iptable_mangle iptable_raw nfnetlink ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter bnep it87 hwmon_vid snd_hda_codec_realtek dm_mod dax snd_hda_codec_generic ledtrig_audio snd_hda_codec_hdmi snd_hda_intel btusb btbcm btintel bluetooth uvcvideo videobuf2_vmalloc snd_usb_audio ecdh_generic videobuf2_memops videobuf2_v4l2 videobuf2_common input_leds led_class rfkill videodev snd_usbmidi_lib mc joydev ecc coretemp kvm_intel kvm snd_intel_dspcfg irqbypass snd_hda_codec snd_virtuoso snd_oxygen_lib snd_hwdep snd_hda_core snd_mpu401_uart snd_rawmidi snd_seq snd_seq_device snd_pcm r8169 mxm_wmi snd_hrtimer iTCO_wdt gpio_ich psmouse realtek i2c_i801 pcspkr tiny_power_button snd_timer mdio_devres libphy i2c_smbus wmi snd i7core_edac [ 43.402051] button soundcore acpi_cpufreq lpc_ich binfmt_misc nfsd auth_rpcgss nfs_acl lockd fuse grace configfs sunrpc zram zsmalloc ip_tables x_tables ext4 crc32c_generic crc16 mbcache jbd2 sr_mod cdrom sd_mod usbhid uhci_hcd ehci_pci ehci_hcd amdgpu xhci_pci xhci_hcd drm_ttm_helper pata_jmicron ttm ahci mfd_core gpu_sched libahci firewire_ohci i2c_algo_bit firewire_core libata crc_itu_t drm_kms_helper cfbfillrect syscopyarea cfbimgblt sysfillrect sysimgblt fb_sys_fops crc32c_intel serio_raw cfbcopyarea scsi_mod usbcore usb_common drm drm_panel_orientation_quirks msr sha256_ssse3 sha256_generic libsha256 ipv6 [ 43.541960] CPU: 2 PID: 1268 Comm: gnome-shell Tainted: G I 5.11.9 #1 [ 43.548677] Hardware name: Gigabyte Technology Co., Ltd. EX58-UD3R/EX58-UD3R, BIOS FB 05/04/2009 [ 43.556556] RIP: 0010:ttm_bo_release+0x172/0x282 [ttm] [ 43.560568] Code: 75 05 e8 dd e0 e6 e0 41 ff c5 eb d4 e8 0f c9 b9 e0 c6 43 58 01 48 c7 c7 68 2a 50 a0 e8 08 8e fa e0 83 bb 9c 00 00 00 00 74 20 <0f> 0b c7 83 9c 00 00 00 00 00 00 00 4c 89 e7 e8 29 fc ff ff 48 8d [ 43.578854] RSP: :c900017e3bc8 EFLAGS: 00010202 [ 43.582956] RAX: RBX: 8881076ca580 RCX: 0007 [ 43.589041] RDX: 0001 RSI: 0246 RDI: a0502a68 [ 43.595121] RBP: 88810b525638 R08: 888101f84f78 R09: c900017e3a80 [ 43.601213] R10: 40042000 R11: R12: 8881076ca400 [ 43.607331] R13: R14: R15: [ 43.613469] FS: 7fb19706e480() GS:888343c8() knlGS: [ 43.620568] CS: 0010 DS: ES: CR0: 80050033 [ 43.625221] CR2: 7fb1940ff2e0 CR3: 0001291fe000 CR4: 06e0 [ 43.631329] Call Trace: [ 43.632488] ttm_bo_move_accel_cleanup+0x109/0x238 [ttm] [ 43.636662] amdgpu_bo_move+0x3c3/0x507 [amdgpu] [ 43.640332] ttm_bo_handle_move_mem+0x73/0xf9 [ttm] [ 43.644141] ttm_bo_validate+0xfa/0x16d [ttm] [ 43.647303] amdgpu_bo_fault_reserve_notify+0xb7/0x134 [amdgpu] [ 43.652246] amdgpu_ttm_fault+0x27/0x7c [amdgpu] [ 43.655730] __do_fault+0x49/0x64 [ 43.657826] handle_mm_fault+0xb19/0xbf8 [ 43.660564] exc_page_fault+0x226/0x32b [ 43.663243] ? asm_exc_page_fault+0x8/0x30 [ 43.666128] asm_exc_page_fault+0x1e/0x30 [ 43.668951] RIP: 0033:0x7fb19b11b1ee [ 43.671328] Code: 4d 29 c1 4c 29 c2 48 3b 15 27 dc 11 00 0f 87 af 00 00 00 0f 10 01 0f 10 49 f0 0f 10 51 e0 0f 10 59 d0 48 83 e9 40 48 83 ea 40 <41> 0f 29 01 41 0f 29 49 f0 41 0f 29 51 e0 41 0f 29 59 d0 49 83 e9 [ 43.689572] RSP: 002b:7fffc11ae478 EFLAGS: 00010202 [ 43.693662] RAX: 7fb1940ff200 RBX: 00f8 RCX: 55886456a1e4 [ 43.699826] RDX: 00b0 RSI: 55886456a144 RDI: 7fb1940ff200 [ 43.705952] RBP: 558864563380 R08: 0008 R09: 7fb1940ff2e0 [ 43.712042] R10: 0088 R11: 7fb1940ff2e8 R12: 55886456a144 [ 43.718128] R13: 7fb17ae55b00 R14: 558864563380 R15: 558864565d98 I have a "BONNAIRE" R7 360: 02:00.0 VGA compatible
Re: [RFC PATCH 07/11] drm/bridge: ti-sn65dsi86: Split connector creation to a function
Hi, On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote: > > To prepare for making connector creation option, move connector creation > out of ti_sn_bridge_attach to a separate function. > > No functional change intended. > > Signed-off-by: Laurent Pinchart > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 31 ++- > 1 file changed, 21 insertions(+), 10 deletions(-) Reviewed-by: Douglas Anderson ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 06/11] drm/bridge: ti-sn65dsi86: Group code in sections
Hi, On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart wrote: > > Reorganize the functions in sections, related to connector operations, > bridge operations, AUX adapter, GPIO controller and probe & remove. > > This prepares for proper support of DRM_BRIDGE_ATTACH_NO_CONNECTOR that > will add more functions, to ensure that the code will stay readable. > > No functional change intended. > > Signed-off-by: Laurent Pinchart > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 75 +-- > 1 file changed, 47 insertions(+), 28 deletions(-) Reviewed-by: Douglas Anderson ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 05/11] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge
Hi,
On Sun, Mar 21, 2021 at 8:02 PM Laurent Pinchart
wrote:
>
> To simplify interfacing with the panel, wrap it in a panel-bridge and
> let the DRM bridge helpers handle chaining of operations.
>
> This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> requires all components in the display pipeline to be represented by
> bridges.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 30 +++
> 1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 1d1be791d5ba..c21a7f7d452b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -124,6 +124,7 @@
> * @edid: Detected EDID of eDP panel.
> * @refclk: Our reference clock.
> * @panel:Our panel.
> + * @next_bridge: The next bridge.
To make it easier for folks who don't work with DRM all day, could you
somehow clarify which direction "next" is talking about. AKA the next
"outward" (towards the sink) or the next "inward" (toward the source)?
> * @enable_gpio: The GPIO we toggle to enable the bridge.
> * @supplies: Data for bulk enabling/disabling our regulators.
> * @dp_lanes: Count of dp_lanes we're using.
> @@ -152,6 +153,7 @@ struct ti_sn_bridge {
> struct mipi_dsi_device *dsi;
> struct clk *refclk;
> struct drm_panel*panel;
> + struct drm_bridge *next_bridge;
There's no reason to store the "panel" pointer anymore, right? It can
just be a local variable in probe?
> @@ -850,8 +856,6 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge
> *bridge)
> */
> regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE,
>HPD_DISABLE);
> -
> - drm_panel_prepare(pdata->panel);
Ugh, I guess conflicts with my EDID patch [1] which assumes that this
function will directly turn the panel on. I'll see if I can find some
solution...
[1]
https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On 24/03/2021 23.18, Joe Perches wrote: > On Wed, 2021-03-24 at 22:27 +0100, Rasmus Villemoes wrote: >> On 24/03/2021 20.24, Joe Perches wrote: >>> On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote: On 24/03/2021 18.20, Joe Perches wrote: > > Maybe it's better to output non PTR_ERR %pe uses as decimal so this > sort of code would work. No, because that would leak the pointer value when somebody has accidentally passed a real kernel pointer to %pe. >>> >>> I think it's not really an issue. >>> >>> _All_ code that uses %p extensions need inspection anyway. >> >> There are now a bunch of sanity checks in place that catch e.g. an >> ERR_PTR passed to an extension that would derefence the pointer; >> enforcing that only ERR_PTRs are passed to %pe (or falling back to %p) >> is another of those safeguards. >> >>> It's already possible to intentionally 'leak' the ptr value >>> by using %pe, -ptr so I think that's not really an issue. >>> >> >> Huh, what? I assume -ptr is shorthand for (void*)-(unsigned long)ptr. >> How would that leak the value if ptr is an ordinary kernel pointer? >> That's not an ERR_PTR unless (unsigned long)ptr is < 4095 or so. > > You are confusing ERR_PTR with IS_ERR No I'm not, I'm just being slightly sloppy - obviously when I say "not an ERR_PTR" I mean "not the result of ERR_PTR applied to a negative errno value", or "not the result of a valid invocation of ERR_PTR". But yes, feel free to read "not an ERR_PTR" as "something for which IS_ERR is false". Can you expand on why you think %pe, -ptr would leak the value of ptr? >> If you want to print the pointer value just do %px. No need for silly >> games. > > There's no silly game here. %pe would either print a string or a value. A hashed value, that is, never the raw value. > It already does that in 2 cases. Yes, if you pass it ERR_PTR(-1234) (where no E symbol exists) or ERR_PTR(-EINVAL) but CONFIG_SYMBOLIC_ERRNAME=n, it prints the value in decimal, because people will probably recognize "-22" and values in that range don't reveal anything about the kernel image. Anything outside [-4095,0] or so is hashed. Rasmus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
linux-next: build warning after merge of the drm-intel-fixes tree
Hi all,
After merging the drm-intel-fixes tree, today's linux-next build
(htmldocs) produced this warning:
Documentation/gpu/i915:22:
/home/sfr/next/next/drivers/gpu/drm/i915/intel_runtime_pm.c:423: WARNING:
Inline strong start-string without end-string.
Introduced by commit
8840e3bd981f ("drm/i915: Fix the GT fence revocation runtime PM logic")
--
Cheers,
Stephen Rothwell
pgpjQjUc6ht3y.pgp
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 22:27 +0100, Rasmus Villemoes wrote:
> On 24/03/2021 20.24, Joe Perches wrote:
> > On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
> > > On 24/03/2021 18.20, Joe Perches wrote:
> > >
> > > >
> > > > Maybe it's better to output non PTR_ERR %pe uses as decimal so this
> > > > sort of code would work.
> > >
> > > No, because that would leak the pointer value when somebody has
> > > accidentally passed a real kernel pointer to %pe.
> >
> > I think it's not really an issue.
> >
> > _All_ code that uses %p extensions need inspection anyway.
>
> There are now a bunch of sanity checks in place that catch e.g. an
> ERR_PTR passed to an extension that would derefence the pointer;
> enforcing that only ERR_PTRs are passed to %pe (or falling back to %p)
> is another of those safeguards.
>
> > It's already possible to intentionally 'leak' the ptr value
> > by using %pe, -ptr so I think that's not really an issue.
> >
>
> Huh, what? I assume -ptr is shorthand for (void*)-(unsigned long)ptr.
> How would that leak the value if ptr is an ordinary kernel pointer?
> That's not an ERR_PTR unless (unsigned long)ptr is < 4095 or so.
You are confusing ERR_PTR with IS_ERR
ERR_PTR is just
include/linux/err.h:static inline void * __must_check ERR_PTR(long error)
include/linux/err.h-{
include/linux/err.h-return (void *) error;
include/linux/err.h-}f
> If you want to print the pointer value just do %px. No need for silly
> games.
There's no silly game here. %pe would either print a string or a value.
It already does that in 2 cases.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vkms: fix misuse of WARN_ON
On 03/20, Dmitry Vyukov wrote:
> vkms_vblank_simulate() uses WARN_ON for timing-dependent condition
> (timer overrun). This is a mis-use of WARN_ON, WARN_ON must be used
> to denote kernel bugs. Use pr_warn() instead.
>
> Signed-off-by: Dmitry Vyukov
> Reported-by: syzbot+4fc21a003c8332eb0...@syzkaller.appspotmail.com
> Cc: Rodrigo Siqueira
> Cc: Melissa Wen
> Cc: Haneen Mohammed
> Cc: Daniel Vetter
> Cc: David Airlie
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-ker...@vger.kernel.org
> Change-Id: I7f01c288092bc7e472ec63af198f93ce3d8c49f7
> ---
> drivers/gpu/drm/vkms/vkms_crtc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c
> b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 0443b7deeaef6..758d8a98d96b3 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -18,7 +18,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct
> hrtimer *timer)
>
> ret_overrun = hrtimer_forward_now(>vblank_hrtimer,
> output->period_ns);
> - WARN_ON(ret_overrun != 1);
> + if (ret_overrun != 1)
> + pr_warn("%s: vblank timer overrun\n", __func__);
Hi Dmitry,
Thanks for your patch.
Looks good to me.
The Change-Id tag just seems a little noisy to me, but can be
fixed while applying.
Acked-by: Melissa Wen
>
> spin_lock(>lock);
> ret = drm_crtc_handle_vblank(crtc);
>
> base-commit: e94c55b8e0a0bbe9a026250cf31e2fa45957d776
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/radeon/r600_cs: Couple of typo fixes
On 3/24/21 6:50 AM, Bhaskar Chowdhury wrote:
>
> s/miror/mirror/
> s/needind/needing/
>
> Signed-off-by: Bhaskar Chowdhury
> ---
> drivers/gpu/drm/radeon/r600_cs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/r600_cs.c
> b/drivers/gpu/drm/radeon/r600_cs.c
> index 34b7c6f16479..aded1f2264e0 100644
> --- a/drivers/gpu/drm/radeon/r600_cs.c
> +++ b/drivers/gpu/drm/radeon/r600_cs.c
> @@ -38,7 +38,7 @@ extern void r600_cs_legacy_get_tiling_conf(struct
> drm_device *dev, u32 *npipes,
>
>
> struct r600_cs_track {
> - /* configuration we miror so that we use same code btw kms/ums */
> + /* configuration we mirror so that we use same code btw kms/ums */
> u32 group_size;
> u32 nbanks;
> u32 npipes;
> @@ -963,7 +963,7 @@ static int r600_cs_parse_packet0(struct radeon_cs_parser
> *p,
> *
> * This function will test against r600_reg_safe_bm and return 0
> * if register is safe. If register is not flag as safe this function
> - * will test it against a list of register needind special handling.
> + * will test it against a list of register needing special handling.
> */
> static int r600_cs_check_reg(struct radeon_cs_parser *p, u32 reg, u32 idx)
> {
> --
Those 2 LGTM, but please fix this while you are touching this file:
drivers/gpu/drm/radeon/r600_cs.c:2339: informations ==> information
thanks.
--
~Randy
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/nouveau/bios/init: A typo fix
On 3/23/21 11:34 PM, Bhaskar Chowdhury wrote: > > s/conditon/condition/ > > Signed-off-by: Bhaskar Chowdhury Acked-by: Randy Dunlap > --- > drivers/gpu/drm/nouveau/nvkm/subdev/bios/init.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/init.c > b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/init.c > index 9de74f41dcd2..142079403864 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/bios/init.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bios/init.c > @@ -401,7 +401,7 @@ init_table_(struct nvbios_init *init, u16 offset, const > char *name) > #define init_macro_table(b) init_table_((b), 0x04, "macro table") > #define init_condition_table(b) init_table_((b), 0x06, "condition table") > #define init_io_condition_table(b) init_table_((b), 0x08, "io condition > table") > -#define init_io_flag_condition_table(b) init_table_((b), 0x0a, "io flag > conditon table") > +#define init_io_flag_condition_table(b) init_table_((b), 0x0a, "io flag > condition table") > #define init_function_table(b) init_table_((b), 0x0c, "function table") > #define init_xlat_table(b) init_table_((b), 0x10, "xlat table"); > > -- -- ~Randy ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 212077] AMD GPU discrete card memory at highest frequency even while not in use
https://bugzilla.kernel.org/show_bug.cgi?id=212077 --- Comment #13 from Alex Deucher (alexdeuc...@gmail.com) --- Created attachment 296035 --> https://bugzilla.kernel.org/attachment.cgi?id=296035=edit possible fix This patch should fix it. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On 24/03/2021 20.24, Joe Perches wrote: > On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote: >> On 24/03/2021 18.20, Joe Perches wrote: >> >>> >>> Maybe it's better to output non PTR_ERR %pe uses as decimal so this >>> sort of code would work. >> >> No, because that would leak the pointer value when somebody has >> accidentally passed a real kernel pointer to %pe. > > I think it's not really an issue. > > _All_ code that uses %p extensions need inspection anyway. There are now a bunch of sanity checks in place that catch e.g. an ERR_PTR passed to an extension that would derefence the pointer; enforcing that only ERR_PTRs are passed to %pe (or falling back to %p) is another of those safeguards. > It's already possible to intentionally 'leak' the ptr value > by using %pe, -ptr so I think that's not really an issue. > Huh, what? I assume -ptr is shorthand for (void*)-(unsigned long)ptr. How would that leak the value if ptr is an ordinary kernel pointer? That's not an ERR_PTR unless (unsigned long)ptr is < 4095 or so. If you want to print the pointer value just do %px. No need for silly games. What I'm talking about is preventing _un_intentionally leaking a valid kernel pointer value. So no, a non-ERR_PTR passed to %pe is not going to be printed as-is, not in decimal or hexadecimal or roman numerals. >> If the code wants a cute -EFOO string explaining what's wrong, what >> about "%pe", ERR_PTR(mux < 0 : mux : -ERANGE)? Or two separate error >> messages >> >> if (mux < 0) >> ... >> else if (mux >= ARRAY_SIZE()) >> ... > > Multiple tests, more unnecessary code, multiple format strings, etc... Agreed, I'm not really advocating for the latter; the former suggestion is IMO a pretty concise way of providing useful information in dmesg. Rasmus ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/doc: Add RFC section
Motivated by the pre-review process for i915 gem/gt features, but probably useful in general for complex stuff. v2: Add reminder to not forget userspace projects in the discussion (Simon, Jason) Cc: Simon Ser Cc: Jani Nikula Cc: Joonas Lahtinen Cc: Rodrigo Vivi Cc: Jason Ekstrand Cc: Dave Airlie Acked-by: Rodrigo Vivi Acked-by: Dave Airlie Signed-off-by: Daniel Vetter --- Documentation/gpu/index.rst | 1 + Documentation/gpu/rfc.rst | 17 + 2 files changed, 18 insertions(+) create mode 100644 Documentation/gpu/rfc.rst diff --git a/Documentation/gpu/index.rst b/Documentation/gpu/index.rst index c9a51e3bfb5a..df58cb826d68 100644 --- a/Documentation/gpu/index.rst +++ b/Documentation/gpu/index.rst @@ -16,6 +16,7 @@ Linux GPU Driver Developer's Guide vga-switcheroo vgaarbiter todo + rfc .. only:: subproject and html diff --git a/Documentation/gpu/rfc.rst b/Documentation/gpu/rfc.rst new file mode 100644 index ..a8621f7dab8b --- /dev/null +++ b/Documentation/gpu/rfc.rst @@ -0,0 +1,17 @@ +=== +GPU RFC Section +=== + +For complex work, especially new uapi, it is often good to nail the high level +design issues before getting lost in the code details. This section is meant to +host such documentation: + +* Each RFC should be a section in this file, explaining the goal and main design + considerations. Especially for uapi make sure you Cc: all relevant project + mailing lists and involved people outside of dri-devel. + +* For uapi structures add a file to this directory with and then pull the + kerneldoc in like with real uapi headers. + +* Once the code has landed move all the documentation to the right places in + the main core, helper or driver sections. -- 2.31.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/i915: add gem/gt TODO
We've discussed a bit how to get the gem/gt team better integrated and collaborate more with the wider community and agreed to the following: - all gem/gt patches are reviewed on dri-devel for now. That's overkill, but in the past there was definitely too little of that. - i915-gem folks are encouraged to cross review core patches from other teams - big features (especially uapi changes) need to be discussed in an rfc patch that documents the interface and big picture design, before we get lost in the details of the code - Also a rough TODO (can be refined as we go ofc) to get gem/gt back on track, like we've e.g. done with DAL/DC to get that in shape. v2: - add dma_fence annotations (Dave) - tasklet helpers (Jani on irc) There was also a discussion about moving these into gitlab issues, or gitlab issues as additional discussion place at least. For now it's just the TODO file Cc: Jani Nikula Cc: Joonas Lahtinen Cc: Rodrigo Vivi Cc: Jason Ekstrand Cc: Dave Airlie Acked-by: Dave Airlie Acked-by: Rodrigo Vivi Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/TODO.txt | 41 +++ 1 file changed, 41 insertions(+) create mode 100644 drivers/gpu/drm/i915/TODO.txt diff --git a/drivers/gpu/drm/i915/TODO.txt b/drivers/gpu/drm/i915/TODO.txt new file mode 100644 index ..81a82c9c203f --- /dev/null +++ b/drivers/gpu/drm/i915/TODO.txt @@ -0,0 +1,41 @@ +gem/gt TODO items +- + +- For discrete memory manager, merge enough dg1 to be able to refactor it to + TTM. Then land pci ids (just in case that turns up an uapi problem). TTM has + improved a lot the past 2 years, there's no reason anymore not to use it. + +- Come up with a plan what to do with drm/scheduler and how to get there. + +- Roll out dma_fence critical section annotations. + +- There's a lot of complexity added past few years to make relocations faster. + That doesn't make sense given hw and gpu apis moved away from this model years + ago: + 1. Land a modern pre-bound uapi like VM_BIND + 2. Any complexity added in this area past few years which can't be justified + with VM_BIND using userspace should be removed. Looking at amdgpu dma_resv on + the bo and vm, plus some lru locks is all that needed. No complex rcu, + refcounts, caching, ... on everything. + This is the matching task on the vm side compared to ttm/dma_resv on the + backing storage side. + +- i915_sw_fence seems to be the main structure for the i915-gem dma_fence model. + How-to-dma_fence is core and drivers really shouldn't build their own world + here, treating everything else as a fixed platform. i915_sw_fence concepts + should be moved to dma_fence, drm/scheduler or atomic commit helpers. Or + removed if dri-devel consensus is that it's not a good idea. Once that's done + maybe even remove it if there's nothing left. + +Smaller things: +- i915_utils.h needs to be moved to the right places. + +- dma_fence_work should be in drivers/dma-buf + +- i915_mm.c should be moved to the right places. Some of the helpers also look a + bit fishy: + + https://lore.kernel.org/linux-mm/20210301083320.943079-1-...@lst.de/ + +- tasklet helpers in i915_gem.h also look a bit misplaced and should + probably be moved to tasklet headers. -- 2.31.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[pull] amdgpu drm-fixes-5.12
Hi Dave, Daniel, Fixes for 5.12. The following changes since commit d27ce83fa4baa5cb908a42e9878564cad6ea0eb3: Merge tag 'du-fixes-20210316' of git://linuxtv.org/pinchartl/media into drm-fixes (2021-03-22 13:49:55 +1000) are available in the Git repository at: https://gitlab.freedesktop.org/agd5f/linux.git tags/amd-drm-fixes-5.12-2021-03-24 for you to fetch changes up to 5c458585c0141754cdcbf25feebb547dd671b559: drm/amdgpu/display: restore AUX_DPHY_TX_CONTROL for DCN2.x (2021-03-24 00:30:57 -0400) amd-drm-fixes-5.12-2021-03-24: amdgpu: - S0ix fixes - Add PCI ID - Polaris PCIe DPM fix - Display fix for high refresh rate monitors Alex Deucher (11): drm/amdgpu: rework S3/S4/S0ix state handling drm/amdgpu: don't evict vram on APUs for suspend to ram (v4) drm/amdgpu: clean up non-DC suspend/resume handling drm/amdgpu: move s0ix check into amdgpu_device_ip_suspend_phase2 (v3) drm/amdgpu: re-enable suspend phase 2 for S0ix drm/amdgpu/swsmu: skip gfx cgpg on s0ix suspend drm/amdgpu: update comments about s0ix suspend/resume drm/amdgpu: drop S0ix checks around CG/PG in suspend drm/amdgpu: skip kfd suspend/resume for S0ix drm/amdgpu: Add additional Sienna Cichlid PCI ID drm/amdgpu/display: restore AUX_DPHY_TX_CONTROL for DCN2.x Kenneth Feng (1): drm/amd/pm: workaround for audio noise issue Pratik Vishwakarma (1): drm/amdgpu: skip CG/PG for gfx during S0ix Prike Liang (1): drm/amdgpu: fix the hibernation suspend with s0ix drivers/gpu/drm/amd/amdgpu/amdgpu.h| 10 +- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 132 ++--- drivers/gpu/drm/amd/amdgpu/amdgpu_display.c| 89 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_display.h| 3 + drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 31 +++-- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 7 +- drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 9 +- drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 9 +- drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 8 +- drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 9 +- drivers/gpu/drm/amd/amdgpu/dce_virtual.c | 15 ++- .../drm/amd/display/dc/dcn20/dcn20_link_encoder.c | 3 +- .../gpu/drm/amd/pm/powerplay/hwmgr/smu7_hwmgr.c| 54 + .../gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c | 74 ++-- .../gpu/drm/amd/pm/powerplay/hwmgr/vega12_hwmgr.c | 24 .../gpu/drm/amd/pm/powerplay/hwmgr/vega20_hwmgr.c | 25 drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 5 +- 17 files changed, 365 insertions(+), 142 deletions(-) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 212397] Resume from suspend (S3) does not bring back video anymore
https://bugzilla.kernel.org/show_bug.cgi?id=212397 Alex Deucher (alexdeuc...@gmail.com) changed: What|Removed |Added CC||alexdeuc...@gmail.com --- Comment #1 from Alex Deucher (alexdeuc...@gmail.com) --- Can you bisect (https://www.kernel.org/doc/html/latest/admin-guide/bug-bisect.html)? The radeon driver has not been changed much in the last few years so the problem may be outside of the driver. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/2] mm,drm/ttm: Block fast GUP to TTM huge pages
On 3/24/21 1:22 PM, Thomas Hellström (Intel) wrote: >> We also have not been careful at *all* about how _PAGE_BIT_SOFTW* are >> used. It's quite possible we can encode another use even in the >> existing bits. >> >> Personally, I'd just try: >> >> #define _PAGE_BIT_SOFTW5 57 /* available for programmer */ >> > OK, I'll follow your advise here. FWIW I grepped for SW1 and it seems > used in a selftest, but only for PTEs AFAICT. > > Oh, and we don't care about 32-bit much anymore? On x86, we have 64-bit PTEs when running 32-bit kernels if PAE is enabled. IOW, we can handle the majority of 32-bit CPUs out there. But, yeah, we don't care about 32-bit. :) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/display: Try YCbCr420 color when YCbCr444 fails
On Wed, Mar 17, 2021 at 11:25 AM Werner Sembach
wrote:
>
> When encoder validation of a display mode fails, retry with less bandwidth
> heavy YCbCr420 color mode, if available. This enables some HDMI 1.4 setups
> to support 4k60Hz output, which previously failed silently.
>
> On some setups, while the monitor and the gpu support display modes with
> pixel clocks of up to 600MHz, the link encoder might not. This prevents
> YCbCr444 and RGB encoding for 4k60Hz, but YCbCr420 encoding might still be
> possible. However, which color mode is used is decided before the link
> encoder capabilities are checked. This patch fixes the problem by retrying
> to find a display mode with YCbCr420 enforced and using it, if it is
> valid.
>
> Signed-off-by: Werner Sembach
> Cc:
This seems reasonable to me. Harry, Leo, Any objections?
Alex
> ---
>
> From c9398160caf4ff20e63b8ba3a4366d6ef95c4ac3 Mon Sep 17 00:00:00 2001
> From: Werner Sembach
> Date: Wed, 17 Mar 2021 12:52:22 +0100
> Subject: [PATCH] Retry forcing YCbCr420 color on failed encoder validation
>
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 961abf1cf040..2d16389b5f1e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5727,6 +5727,15 @@ create_validate_stream_for_sink(struct
> amdgpu_dm_connector *aconnector,
>
> } while (stream == NULL && requested_bpc >= 6);
>
> + if (dc_result == DC_FAIL_ENC_VALIDATE &&
> !aconnector->force_yuv420_output) {
> + DRM_DEBUG_KMS("Retry forcing YCbCr420 encoding\n");
> +
> + aconnector->force_yuv420_output = true;
> + stream = create_validate_stream_for_sink(aconnector, drm_mode,
> + dm_state, old_stream);
> + aconnector->force_yuv420_output = false;
> + }
> +
> return stream;
> }
>
> --
> 2.25.1
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/2] mm,drm/ttm: Block fast GUP to TTM huge pages
On 3/24/21 5:34 PM, Dave Hansen wrote: On 3/24/21 3:05 AM, Thomas Hellström (Intel) wrote: Yes, I agree. Seems like the special (SW1) is available also for huge page table entries on x86 AFAICT, although just not implemented. Otherwise the SW bits appear completely used up. Although the _PAGE_BIT_SOFTW* bits are used up, there's plenty of room in the hardware PTEs. Bits 52->58 are software-available, and we're only using 58 at the moment. We also have not been careful at *all* about how _PAGE_BIT_SOFTW* are used. It's quite possible we can encode another use even in the existing bits. Personally, I'd just try: #define _PAGE_BIT_SOFTW557 /* available for programmer */ OK, I'll follow your advise here. FWIW I grepped for SW1 and it seems used in a selftest, but only for PTEs AFAICT. Oh, and we don't care about 32-bit much anymore? /Thomas ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 21/21] drm/msm/dpu: call hw_intr ops directly
Hi Dmitry,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.12-rc4 next-20210324]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Dmitry-Baryshkov/drm-msm-dpu-cleanup-callbacks-resource-manager/20210324-230347
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
7acac4b3196caee5e21fb5ea53f8bc124e6a16fc
config: arm64-randconfig-r036-20210324 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project
a4fb88669cd98db6fef7dcac88e3ec425d40c00d)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
#
https://github.com/0day-ci/linux/commit/8f82b58643355f9e0d03c022b66e276c252e633a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Dmitry-Baryshkov/drm-msm-dpu-cleanup-callbacks-resource-manager/20210324-230347
git checkout 8f82b58643355f9e0d03c022b66e276c252e633a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c:889:5: warning: no
>> previous prototype for function 'dpu_hw_intr_disable_irq_nolock'
>> [-Wmissing-prototypes]
int dpu_hw_intr_disable_irq_nolock(struct dpu_hw_intr *intr, int irq_idx)
^
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c:889:1: note: declare
'static' if the function is not intended to be used outside of this translation
unit
int dpu_hw_intr_disable_irq_nolock(struct dpu_hw_intr *intr, int irq_idx)
^
static
1 warning generated.
vim +/dpu_hw_intr_disable_irq_nolock +889
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
888
> 889 int dpu_hw_intr_disable_irq_nolock(struct dpu_hw_intr *intr, int
> irq_idx)
890 {
891 int reg_idx;
892 const struct dpu_intr_reg *reg;
893 const struct dpu_irq_type *irq;
894 const char *dbgstr = NULL;
895 uint32_t cache_irq_mask;
896
897 if (!intr)
898 return -EINVAL;
899
900 if (irq_idx < 0 || irq_idx >= ARRAY_SIZE(dpu_irq_map)) {
901 pr_err("invalid IRQ index: [%d]\n", irq_idx);
902 return -EINVAL;
903 }
904
905 irq = _irq_map[irq_idx];
906 reg_idx = irq->reg_idx;
907 reg = _intr_set[reg_idx];
908
909 cache_irq_mask = intr->cache_irq_mask[reg_idx];
910 if ((cache_irq_mask & irq->irq_mask) == 0) {
911 dbgstr = "DPU IRQ is already cleared:";
912 } else {
913 dbgstr = "DPU IRQ mask disable:";
914
915 cache_irq_mask &= ~irq->irq_mask;
916 /* Disable interrupts based on the new mask */
917 DPU_REG_WRITE(>hw, reg->en_off, cache_irq_mask);
918 /* Cleaning any pending interrupt */
919 DPU_REG_WRITE(>hw, reg->clr_off, irq->irq_mask);
920
921 /* ensure register write goes through */
922 wmb();
923
924 intr->cache_irq_mask[reg_idx] = cache_irq_mask;
925 }
926
927 pr_debug("%s MASK:0x%.8x, CACHE-MASK:0x%.8x\n", dbgstr,
928 irq->irq_mask, cache_irq_mask);
929
930 return 0;
931 }
932
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/amdgpu: Ensure that the modifier requested is supported by plane.
From: Mark Yacoub
On initializing the framebuffer, call drm_any_plane_has_format to do a
check if the modifier is supported. drm_any_plane_has_format calls
dm_plane_format_mod_supported which is extended to validate that the
modifier is on the list of the plane's supported modifiers.
The bug was caught using igt-gpu-tools test:
kms_addfb_basic.addfb25-bad-modifier
Tested on ChromeOS Zork by turning on the display, running an overlay
test, and running a YT video.
=== Changes from v1 ===
Explicitly handle DRM_FORMAT_MOD_INVALID modifier.
Cc: Alex Deucher
Cc: Bas Nieuwenhuizen
Signed-off-by: default avatarMark Yacoub
---
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c| 13 +
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 18 +++---
2 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index afa5f8ad0f563..a947b5aa420d2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -908,6 +908,19 @@ int amdgpu_display_gem_fb_verify_and_init(
_fb_funcs);
if (ret)
goto err;
+ /* Verify that the modifier is supported. */
+ if (!drm_any_plane_has_format(dev, mode_cmd->pixel_format,
+ mode_cmd->modifier[0])) {
+ struct drm_format_name_buf format_name;
+ drm_dbg_kms(dev,
+ "unsupported pixel format %s / modifier 0x%llx\n",
+ drm_get_format_name(mode_cmd->pixel_format,
+ _name),
+ mode_cmd->modifier[0]);
+
+ ret = -EINVAL;
+ goto err;
+ }
ret = amdgpu_display_framebuffer_init(dev, rfb, mode_cmd, obj);
if (ret)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 961abf1cf040c..6fc746996c24f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3939,6 +3939,7 @@ static bool dm_plane_format_mod_supported(struct
drm_plane *plane,
{
struct amdgpu_device *adev = drm_to_adev(plane->dev);
const struct drm_format_info *info = drm_format_info(format);
+ int i;
enum dm_micro_swizzle microtile = modifier_gfx9_swizzle_mode(modifier)
& 3;
@@ -3946,11 +3947,22 @@ static bool dm_plane_format_mod_supported(struct
drm_plane *plane,
return false;
/*
-* We always have to allow this modifier, because core DRM still
-* checks LINEAR support if userspace does not provide modifers.
+* We always have to allow these modifiers:
+* 1. Core DRM checks for LINEAR support if userspace does not provide
modifiers.
+* 2. Not passing any modifiers is the same as explicitly passing
INVALID.
*/
- if (modifier == DRM_FORMAT_MOD_LINEAR)
+ if (modifier == DRM_FORMAT_MOD_LINEAR ||
+ modifier == DRM_FORMAT_MOD_INVALID) {
return true;
+ }
+
+ /* Check that the modifier is on the list of the plane's supported
modifiers. */
+ for (i = 0; i < plane->modifier_count; i++) {
+ if (modifier == plane->modifiers[i])
+ break;
+ }
+ if (i == plane->modifier_count)
+ return false;
/*
* The arbitrary tiling support for multiplane formats has not been
hooked
--
2.31.0.291.g576ba9dcdaf-goog
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 212425] Kernel warning at drivers/gpu/drm/ttm/ttm_bo.c:517
https://bugzilla.kernel.org/show_bug.cgi?id=212425 Alex Deucher (alexdeuc...@gmail.com) changed: What|Removed |Added CC||alexdeuc...@gmail.com --- Comment #1 from Alex Deucher (alexdeuc...@gmail.com) --- I don't know that this should have gone to stable. Looks like Sasha's bot picked it up. I think it may depend on other ttm changes that are not in stable. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/2] mm,drm/ttm: Block fast GUP to TTM huge pages
On 3/24/21 7:31 PM, Christian König wrote: Am 24.03.21 um 17:38 schrieb Jason Gunthorpe: On Wed, Mar 24, 2021 at 04:50:14PM +0100, Thomas Hellström (Intel) wrote: On 3/24/21 2:48 PM, Jason Gunthorpe wrote: On Wed, Mar 24, 2021 at 02:35:38PM +0100, Thomas Hellström (Intel) wrote: In an ideal world the creation/destruction of page table levels would by dynamic at this point, like THP. Hmm, but I'm not sure what problem we're trying to solve by changing the interface in this way? We are trying to make a sensible driver API to deal with huge pages. Currently if the core vm requests a huge pud, we give it one, and if we can't or don't want to (because of dirty-tracking, for example, which is always done on 4K page-level) we just return VM_FAULT_FALLBACK, and the fault is retried at a lower level. Well, my thought would be to move the pte related stuff into vmf_insert_range instead of recursing back via VM_FAULT_FALLBACK. I don't know if the locking works out, but it feels cleaner that the driver tells the vmf how big a page it can stuff in, not the vm telling the driver to stuff in a certain size page which it might not want to do. Some devices want to work on a in-between page size like 64k so they can't form 2M pages but they can stuff 64k of 4K pages in a batch on every fault. Hmm, yes, but we would in that case be limited anyway to insert ranges smaller than and equal to the fault size to avoid extensive and possibly unnecessary checks for contigous memory. Why? The insert function is walking the page tables, it just updates things as they are. It learns the arragement for free while doing the walk. The device has to always provide consistent data, if it overlaps into pages that are already populated that is fine so long as it isn't changing their addresses. And then if we can't support the full fault size, we'd need to either presume a size and alignment of the next level or search for contigous memory in both directions around the fault address, perhaps unnecessarily as well. You don't really need to care about levels, the device should be faulting in the largest memory regions it can within its efficiency. If it works on 4M pages then it should be faulting 4M pages. The page size of the underlying CPU doesn't really matter much other than some tuning to impact how the device's allocator works. Yes, but then we'd be adding a lot of complexity into this function that is already provided by the current interface for DAX, for little or no gain, at least in the drm/ttm setting. Please think of the following situation: You get a fault, you do an extensive time-consuming scan of your VRAM buffer object into which the fault goes and determine you can fault 1GB. Now you hand it to vmf_insert_range() and because the user-space address is misaligned, or already partly populated because of a previous eviction, you can only fault single pages, and you end up faulting a full GB of single pages perhaps for a one-time small update. On top of this, unless we want to do the walk trying increasingly smaller sizes of vmf_insert_xxx(), we'd have to use apply_to_page_range() and teach it about transhuge page table entries, because pagewalk.c can't be used (It can't populate page tables). That also means apply_to_page_range() needs to be complicated with page table locks since transhuge pages aren't stable and can be zapped and refaulted under us while we do the walk. On top of this, the user-space address allocator needs to know how large gpu pages are aligned in buffer objects to have a reasonable chance of aligning with CPU huge page boundaries which is a requirement to be able to insert a huge CPU page table entry, so the driver would basically need the drm helper that can do this alignment anyway. All this makes me think we should settle for the current interface for now, and if someone feels like refining it, I'm fine with that. After all, this isn't a strange drm/ttm invention, it's a pre-existing interface that we reuse. I agree with Jason here. We get the best efficiency when we look at the what the GPU driver provides and make sure that we handle one GPU page at once instead of looking to much into what the CPU is doing with it's page tables. At least one AMD GPUs the GPU page size can be anything between 4KiB and 2GiB and if we will in a 2GiB chunk at once this can in theory be handled by just two giant page table entries on the CPU side. Yes, but I fail to see why, with the current code, we can't do this (save the refcounting bug)? /Thomas ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/4] drm/i915: missing workarounds and refactors
Missing WAs and related refactors. Caz Yokoyama (2): drm/i915/gen12: Add recommended hardware tuning value drm/i915/icl: add Wa_22010271021 for all gen11 José Roberto de Souza (1): drm/i915: Move Wa_16011163337 to gen12_ctx_workarounds_init() Swathi Dhanavanthri (1): drm/i915: Add Wa_14011060649 drivers/gpu/drm/i915/gt/intel_workarounds.c | 97 ++--- drivers/gpu/drm/i915/i915_reg.h | 3 + 2 files changed, 67 insertions(+), 33 deletions(-) -- 2.31.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/4] drm/i915/gen12: Add recommended hardware tuning value
From: Caz Yokoyama
Follow Bspec 31870 to set recommended tuning values for certain GT
register. These values aren't workarounds per-se, but it's best to
handle them in the same general area of the driver, especially since
there may be real workarounds that update other bits of the same
registers.
At the moment the only value we need to worry about is the
TDS_TIMER setting in FF_MODE2. This setting was previously
described as "Wa_1604555607" on some platforms, but the spec
tells us that we should continue to program this on all current
gen12 platforms, even those that do not have that WA.
Bspec: 31870
v2: Rephrase some comments to make them clearer (Matt)
Cc: Clinton Taylor
Signed-off-by: Caz Yokoyama
Signed-off-by: Lucas De Marchi
Reviewed-by: Matt Roper
---
drivers/gpu/drm/i915/gt/intel_workarounds.c | 48 -
1 file changed, 37 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 3b4a7da60f0b..2e367f95b989 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -646,9 +646,38 @@ static void icl_ctx_workarounds_init(struct
intel_engine_cs *engine,
wa_masked_en(wal, GEN9_ROW_CHICKEN4, GEN11_DIS_PICK_2ND_EU);
}
+/*
+ * These settings aren't actually workarounds, but general tuning settings that
+ * need to be programmed on several platforms.
+ */
+static void gen12_ctx_gt_tuning_init(struct intel_engine_cs *engine,
+struct i915_wa_list *wal)
+{
+ /*
+* Although some platforms refer to it as Wa_1604555607, we need to
+* program it even on those that don't explicitly list that
+* workaround.
+*
+* Note that the programming of this register is further modified
+* according to the FF_MODE2 guidance given by Wa_1608008084:gen12.
+* Wa_1608008084 tells us the FF_MODE2 register will return the wrong
+* value when read. The default value for this register is zero for all
+* fields and there are no bit masks. So instead of doing a RMW we
+* should just write TDS timer value. For the same reason read
+* verification is ignored.
+*/
+ wa_add(wal,
+ FF_MODE2,
+ FF_MODE2_TDS_TIMER_MASK,
+ FF_MODE2_TDS_TIMER_128,
+ 0);
+}
+
static void gen12_ctx_workarounds_init(struct intel_engine_cs *engine,
struct i915_wa_list *wal)
{
+ gen12_ctx_gt_tuning_init(engine, wal);
+
/*
* Wa_1409142259:tgl
* Wa_1409347922:tgl
@@ -675,19 +704,15 @@ static void tgl_ctx_workarounds_init(struct
intel_engine_cs *engine,
gen12_ctx_workarounds_init(engine, wal);
/*
-* Wa_1604555607:tgl,rkl
+* Wa_16011163337
*
-* Note that the implementation of this workaround is further modified
-* according to the FF_MODE2 guidance given by Wa_1608008084:gen12.
-* FF_MODE2 register will return the wrong value when read. The default
-* value for this register is zero for all fields and there are no bit
-* masks. So instead of doing a RMW we should just write the GS Timer
-* and TDS timer values for Wa_1604555607 and Wa_16011163337.
+* Like in gen12_ctx_gt_tuning_init(), read verification is ignored due
+* to Wa_1608008084.
*/
wa_add(wal,
FF_MODE2,
- FF_MODE2_GS_TIMER_MASK | FF_MODE2_TDS_TIMER_MASK,
- FF_MODE2_GS_TIMER_224 | FF_MODE2_TDS_TIMER_128,
+ FF_MODE2_GS_TIMER_MASK,
+ FF_MODE2_GS_TIMER_224,
0);
}
@@ -707,12 +732,13 @@ static void dg1_ctx_workarounds_init(struct
intel_engine_cs *engine,
/*
* Wa_16011163337
*
-* Like in tgl_ctx_workarounds_init(), read verification is ignored due
+* Like in gen12_ctx_gt_tuning_init(), read verification is ignored due
* to Wa_1608008084.
*/
wa_add(wal,
FF_MODE2,
- FF_MODE2_GS_TIMER_MASK, FF_MODE2_GS_TIMER_224, 0);
+ FF_MODE2_GS_TIMER_MASK,
+ FF_MODE2_GS_TIMER_224, 0);
}
static void
--
2.31.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/4] drm/i915/icl: add Wa_22010271021 for all gen11
From: Caz Yokoyama
Wa_22010271021 does not apply only to EHL, but to all gen11 and other
gen12 platforms. Gen12 is already covered in another code path, but we
need to stop checking for EHL when handling gen11.
Bspec: 33450, 52887
v2: Remove "gen11" suffix as it also applies to gen12 platforms
Cc: Clinton Taylor
Signed-off-by: Caz Yokoyama
Signed-off-by: Lucas De Marchi
Reviewed-by: Matt Roper
---
drivers/gpu/drm/i915/gt/intel_workarounds.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 2e367f95b989..3c609adca2ee 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -1820,11 +1820,10 @@ rcs_engine_wa_init(struct intel_engine_cs *engine,
struct i915_wa_list *wal)
GEN7_FF_THREAD_MODE,
GEN12_FF_TESSELATION_DOP_GATE_DISABLE);
- /* Wa_22010271021:ehl */
- if (IS_JSL_EHL(i915))
- wa_masked_en(wal,
-GEN9_CS_DEBUG_MODE1,
-FF_DOP_CLOCK_GATE_DISABLE);
+ /* Wa_22010271021 */
+ wa_masked_en(wal,
+GEN9_CS_DEBUG_MODE1,
+FF_DOP_CLOCK_GATE_DISABLE);
}
if (IS_GEN_RANGE(i915, 9, 12)) {
--
2.31.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/4] drm/i915: Move Wa_16011163337 to gen12_ctx_workarounds_init()
From: José Roberto de Souza
This WA is needed in all gen12 platforms, moving it to
gen12_ctx_workarounds_init() allow us to remove the duplicated
implementation.
Also allow us to remove the tgl_ctx_workarounds_init() that after the
WA move above was empty.
Signed-off-by: José Roberto de Souza
Reviewed-by: Lucas De Marchi
---
drivers/gpu/drm/i915/gt/intel_workarounds.c | 20
1 file changed, 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index 3c609adca2ee..aeb5fb54fb0a 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -696,12 +696,6 @@ static void gen12_ctx_workarounds_init(struct
intel_engine_cs *engine,
wa_masked_field_set(wal, GEN8_CS_CHICKEN1,
GEN9_PREEMPT_GPGPU_LEVEL_MASK,
GEN9_PREEMPT_GPGPU_THREAD_GROUP_LEVEL);
-}
-
-static void tgl_ctx_workarounds_init(struct intel_engine_cs *engine,
-struct i915_wa_list *wal)
-{
- gen12_ctx_workarounds_init(engine, wal);
/*
* Wa_16011163337
@@ -728,17 +722,6 @@ static void dg1_ctx_workarounds_init(struct
intel_engine_cs *engine,
/* Wa_22010493298 */
wa_masked_en(wal, HIZ_CHICKEN,
DG1_HZ_READ_SUPPRESSION_OPTIMIZATION_DISABLE);
-
- /*
-* Wa_16011163337
-*
-* Like in gen12_ctx_gt_tuning_init(), read verification is ignored due
-* to Wa_1608008084.
-*/
- wa_add(wal,
- FF_MODE2,
- FF_MODE2_GS_TIMER_MASK,
- FF_MODE2_GS_TIMER_224, 0);
}
static void
@@ -755,9 +738,6 @@ __intel_engine_init_ctx_wa(struct intel_engine_cs *engine,
if (IS_DG1(i915))
dg1_ctx_workarounds_init(engine, wal);
- else if (IS_ALDERLAKE_S(i915) || IS_ROCKETLAKE(i915) ||
-IS_TIGERLAKE(i915))
- tgl_ctx_workarounds_init(engine, wal);
else if (IS_GEN(i915, 12))
gen12_ctx_workarounds_init(engine, wal);
else if (IS_GEN(i915, 11))
--
2.31.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/4] drm/i915: Add Wa_14011060649
From: Swathi Dhanavanthri
This is a permanent workaround for TGL,RKL,DG1 and ADLS.
Signed-off-by: Swathi Dhanavanthri
Reviewed-by: Lucas De Marchi
---
drivers/gpu/drm/i915/gt/intel_workarounds.c | 26 +
drivers/gpu/drm/i915/i915_reg.h | 3 +++
2 files changed, 29 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c
b/drivers/gpu/drm/i915/gt/intel_workarounds.c
index aeb5fb54fb0a..3678f6fbee46 100644
--- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
@@ -1123,11 +1123,37 @@ icl_gt_workarounds_init(struct drm_i915_private *i915,
struct i915_wa_list *wal)
L3_CLKGATE_DIS | L3_CR2X_CLKGATE_DIS);
}
+/*
+ * Though there are per-engine instances of these registers,
+ * they retain their value through engine resets and should
+ * only be provided on the GT workaround list rather than
+ * the engine-specific workaround list.
+ */
+static void
+wa_14011060649(struct drm_i915_private *i915, struct i915_wa_list *wal)
+{
+ struct intel_engine_cs *engine;
+ struct intel_gt *gt = >gt;
+ int id;
+
+ for_each_engine(engine, gt, id) {
+ if (engine->class != VIDEO_DECODE_CLASS ||
+ (engine->instance % 2))
+ continue;
+
+ wa_write_or(wal, VDBOX_CGCTL3F10(engine->mmio_base),
+ IECPUNIT_CLKGATE_DIS);
+ }
+}
+
static void
gen12_gt_workarounds_init(struct drm_i915_private *i915,
struct i915_wa_list *wal)
{
wa_init_mcr(i915, wal);
+
+ /* Wa_14011060649:tgl,rkl,dg1,adls */
+ wa_14011060649(i915, wal);
}
static void
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cbf7a60afe54..e087bcd21911 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2715,6 +2715,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
#define RING_INDIRECT_CTX_OFFSET(base) _MMIO((base) + 0x1c8) /* gen8+ */
#define RING_CTX_TIMESTAMP(base) _MMIO((base) + 0x3a8) /* gen8+ */
+#define VDBOX_CGCTL3F10(base) _MMIO((base) + 0x3f10)
+#define IECPUNIT_CLKGATE_DIS REG_BIT(22)
+
#define ERROR_GEN6 _MMIO(0x40a0)
#define GEN7_ERR_INT _MMIO(0x44040)
#define ERR_INT_POISON (1 << 31)
--
2.31.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 212425] New: Kernel warning at drivers/gpu/drm/ttm/ttm_bo.c:517
https://bugzilla.kernel.org/show_bug.cgi?id=212425 Bug ID: 212425 Summary: Kernel warning at drivers/gpu/drm/ttm/ttm_bo.c:517 Product: Drivers Version: 2.5 Kernel Version: 5.11.9 Hardware: x86-64 OS: Linux Tree: Mainline Status: NEW Severity: normal Priority: P1 Component: Video(DRI - non Intel) Assignee: drivers_video-...@kernel-bugs.osdl.org Reporter: custos.men...@gmail.com Regression: No After installing today's release of kernel 5.11.9 I am getting a bunch of kernel warnings like: [ 70.902471] WARNING: CPU: 6 PID: 2147 at drivers/gpu/drm/ttm/ttm_bo.c:517 ttm_bo_release+0x2b1/0x300 [ 70.902481] Modules linked in: nls_iso8859_2 nls_cp852 vfat fat uinput ipt_REJECT nf_reject_ipv4 iptable_filter ip_tables ip6t_REJECT nf_reject_ipv6 xt_tcpudp xt_conntrack nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 ip6table_filter ip6_tables x_tables kvm_amd kvm pcspkr irqbypass sp5100_tco snd_hda_codec_realtek snd_hda_codec_hdmi snd_hda_codec_generic ledtrig_audio snd_hda_intel snd_intel_dspcfg snd_hda_codec snd_hwdep snd_hda_core snd_pcm snd_timer lm92 lm63 it87 hwmon_vid fam15h_power nfsd k10temp auth_rpcgss fuse oid_registry lockd grace hid_logitech_hidpp hid_logitech_dj hid_generic sr_mod cdrom sd_mod megaraid_sas 8250 8250_base serial_core usbhid sunrpc dm_mod dax [ 70.902541] CPU: 6 PID: 2147 Comm: kmail Tainted: GW 5.11.9-acrux #9 [ 70.902554] Hardware name: To be filled by O.E.M. To be filled by O.E.M./SABERTOOTH 990FX R2.0, BIOS 2901 05/04/2016 [ 70.902556] RIP: 0010:ttm_bo_release+0x2b1/0x300 [ 70.902559] Code: e8 74 2b 2f 00 e9 d9 fd ff ff 48 8b 7d 88 b9 30 75 00 00 31 d2 be 01 00 00 00 e8 3a 52 2f 00 48 8b 45 d8 eb 9d 4c 89 e0 eb 98 <0f> 0b c7 85 9c 00 00 00 00 00 00 00 4c 89 ef e8 4b f2 ff ff 48 8d [ 70.902561] RSP: 0018:af4a83313bb8 EFLAGS: 00010202 [ 70.902566] RAX: 0001 RBX: 961b7553c500 RCX: 0008 [ 70.902568] RDX: 0001 RSI: 0246 RDI: a1db5248 [ 70.902570] RBP: 961b7553c570 R08: 961ba14f7a38 R09: 9621dedaa3f8 [ 70.902571] R10: 961ac60c1250 R11: 961ac60c1240 R12: 961ac5fe5588 [ 70.902572] R13: 961b7553c400 R14: R15: 961ac5fe5f48 [ 70.902573] FS: 7ff78c2f5f00() GS:9621ded8() knlGS: [ 70.902575] CS: 0010 DS: ES: CR0: 80050033 [ 70.902576] CR2: 7ff69182 CR3: 000203d5d000 CR4: 000406e0 [ 70.902578] Call Trace: [ 70.902580] ttm_bo_move_accel_cleanup+0x19d/0x398 [ 70.902583] amdgpu_bo_move+0x15c/0x698 [ 70.902586] ? amdgpu_vram_mgr_new+0x373/0x3d8 [ 70.902587] ttm_bo_handle_move_mem+0x8c/0x170 [ 70.902590] ttm_bo_validate+0x147/0x178 [ 70.902592] amdgpu_bo_fault_reserve_notify+0xbf/0x148 [ 70.902594] amdgpu_ttm_fault+0x33/0x80 [ 70.902596] __do_fault+0x33/0x90 [ 70.902599] handle_mm_fault+0xdff/0x1498 [ 70.902601] exc_page_fault+0x1a5/0x500 [ 70.902604] ? exit_to_user_mode_prepare+0x5d/0x118 [ 70.902607] ? asm_exc_page_fault+0x8/0x30 [ 70.902609] asm_exc_page_fault+0x1e/0x30 [ 70.902611] RIP: 0033:0x7ff78d8f290d [ 70.902612] Code: 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 f3 0f 6f 46 bc f3 0f 6f 4e cc 4c 8b 4e dc 4c 8b 56 e4 4c 8b 5e ec 48 8b 4e f4 8b 56 fc 0f 7f 47 bc f3 0f 7f 4f cc 4c 89 4f dc 4c 89 57 e4 4c 89 5f ec [ 70.902614] RSP: 002b:7ffdbd5bb838 EFLAGS: 00010207 [ 70.902616] RAX: 7ff69182 RBX: 0044 RCX: 3f80 [ 70.902617] RDX: RSI: 55d1049acad4 RDI: 7ff691820044 [ 70.902618] RBP: 55d1049aca90 R08: 55d103fd9a30 R09: [ 70.902619] R10: 3f80 R11: 3f80bf80 R12: [ 70.902620] R13: 55d103f58320 R14: R15: 0044 Apparently the warnings show after commit 7d09e9725b5dcc8d14e101de931e4969d033a6ad, which explains that the warning is triggered by "very likely a driver bug". -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] dt-bindings: msm: Couple of spelling fixes
On Sun, 21 Mar 2021 00:55:53 +0530, Bhaskar Chowdhury wrote: > > s/Subsytem/Subsystem/ > s/contoller/controller/ > > Signed-off-by: Bhaskar Chowdhury > --- > Documentation/devicetree/bindings/display/msm/dpu.txt | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Applied, thanks! ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] dt-bindings: msm: Couple of spelling fixes
On Sun, Mar 21, 2021 at 12:55:53AM +0530, Bhaskar Chowdhury wrote: > > s/Subsytem/Subsystem/ > s/contoller/controller/ Rather than worry about trivial fixes, please convert .txt bindings to DT schema instead. > > Signed-off-by: Bhaskar Chowdhury > --- > Documentation/devicetree/bindings/display/msm/dpu.txt | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt > b/Documentation/devicetree/bindings/display/msm/dpu.txt > index 551ae26f60da..586e6eac5b08 100644 > --- a/Documentation/devicetree/bindings/display/msm/dpu.txt > +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt > @@ -2,14 +2,14 @@ Qualcomm Technologies, Inc. DPU KMS > > Description: > > -Device tree bindings for MSM Mobile Display Subsytem(MDSS) that encapsulates > +Device tree bindings for MSM Mobile Display Subsystem(MDSS) that encapsulates > sub-blocks like DPU display controller, DSI and DP interfaces etc. > The DPU display controller is found in SDM845 SoC. > > MDSS: > Required properties: > - compatible: "qcom,sdm845-mdss", "qcom,sc7180-mdss" > -- reg: physical base address and length of contoller's registers. > +- reg: physical base address and length of controller's registers. > - reg-names: register region names. The following region is required: >* "mdss" > - power-domains: a power domain consumer specifier according to > -- > 2.26.2 > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/ttm: switch back to static allocation limits for now
On Wed, Mar 24, 2021 at 02:48:45PM +0100, Christian König wrote:
> The shrinker based approach still has some flaws. Especially that we need
> temporary pages to free up the pages allocated to the driver is problematic
> in a shrinker.
>
> Signed-off-by: Christian König
> ---
> drivers/gpu/drm/ttm/ttm_device.c | 14 ++--
> drivers/gpu/drm/ttm/ttm_tt.c | 112 ---
> include/drm/ttm/ttm_tt.h | 3 +-
> 3 files changed, 53 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_device.c
> b/drivers/gpu/drm/ttm/ttm_device.c
> index 95e1b7b1f2e6..388da2a7f0bb 100644
> --- a/drivers/gpu/drm/ttm/ttm_device.c
> +++ b/drivers/gpu/drm/ttm/ttm_device.c
> @@ -53,7 +53,6 @@ static void ttm_global_release(void)
> goto out;
>
> ttm_pool_mgr_fini();
> - ttm_tt_mgr_fini();
>
> __free_page(glob->dummy_read_page);
> memset(glob, 0, sizeof(*glob));
> @@ -64,7 +63,7 @@ static void ttm_global_release(void)
> static int ttm_global_init(void)
> {
> struct ttm_global *glob = _glob;
> - unsigned long num_pages;
> + unsigned long num_pages, num_dma32;
> struct sysinfo si;
> int ret = 0;
> unsigned i;
> @@ -79,8 +78,15 @@ static int ttm_global_init(void)
>* system memory.
>*/
> num_pages = ((u64)si.totalram * si.mem_unit) >> PAGE_SHIFT;
> - ttm_pool_mgr_init(num_pages * 50 / 100);
> - ttm_tt_mgr_init();
> + num_pages /= 2;
> +
> + /* But for DMA32 we limit ourself to only use 2GiB maximum. */
> + num_dma32 = (u64)(si.totalram - si.totalhigh) * si.mem_unit
> + >> PAGE_SHIFT;
> + num_dma32 = min(num_dma32, 2UL << (30 - PAGE_SHIFT));
> +
> + ttm_pool_mgr_init(num_pages);
> + ttm_tt_mgr_init(num_pages, num_dma32);
>
> spin_lock_init(>lru_lock);
> glob->dummy_read_page = alloc_page(__GFP_ZERO | GFP_DMA32);
> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
> index 2f0833c98d2c..5d8820725b75 100644
> --- a/drivers/gpu/drm/ttm/ttm_tt.c
> +++ b/drivers/gpu/drm/ttm/ttm_tt.c
> @@ -40,8 +40,18 @@
>
> #include "ttm_module.h"
>
> -static struct shrinker mm_shrinker;
> -static atomic_long_t swapable_pages;
> +static unsigned long ttm_pages_limit;
> +
> +MODULE_PARM_DESC(pages_limit, "Limit for the allocated pages");
> +module_param_named(pages_limit, ttm_pages_limit, ulong, 0644);
> +
> +static unsigned long ttm_dma32_pages_limit;
> +
> +MODULE_PARM_DESC(dma32_pages_limit, "Limit for the allocated DMA32 pages");
> +module_param_named(dma32_pages_limit, ttm_dma32_pages_limit, ulong, 0644);
> +
> +static atomic_long_t ttm_pages_allocated;
> +static atomic_long_t ttm_dma32_pages_allocated;
Making this configurable looks an awful lot like "job done, move on". Just
the revert to hardcoded 50% (or I guess just revert the shrinker patch at
that point) for -fixes is imo better.
Then I guess retry again for 5.14 or so.
-Daniel
>
> /*
> * Allocates a ttm structure for the given BO.
> @@ -294,8 +304,6 @@ static void ttm_tt_add_mapping(struct ttm_device *bdev,
> struct ttm_tt *ttm)
>
> for (i = 0; i < ttm->num_pages; ++i)
> ttm->pages[i]->mapping = bdev->dev_mapping;
> -
> - atomic_long_add(ttm->num_pages, _pages);
> }
>
> int ttm_tt_populate(struct ttm_device *bdev,
> @@ -309,12 +317,25 @@ int ttm_tt_populate(struct ttm_device *bdev,
> if (ttm_tt_is_populated(ttm))
> return 0;
>
> + atomic_long_add(ttm->num_pages, _pages_allocated);
> + if (bdev->pool.use_dma32)
> + atomic_long_add(ttm->num_pages, _dma32_pages_allocated);
> +
> + while (atomic_long_read(_pages_allocated) > ttm_pages_limit ||
> +atomic_long_read(_dma32_pages_allocated) >
> +ttm_dma32_pages_limit) {
> +
> + ret = ttm_bo_swapout(ctx, GFP_KERNEL);
> + if (ret)
> + goto error;
> + }
> +
> if (bdev->funcs->ttm_tt_populate)
> ret = bdev->funcs->ttm_tt_populate(bdev, ttm, ctx);
> else
> ret = ttm_pool_alloc(>pool, ttm, ctx);
> if (ret)
> - return ret;
> + goto error;
>
> ttm_tt_add_mapping(bdev, ttm);
> ttm->page_flags |= TTM_PAGE_FLAG_PRIV_POPULATED;
> @@ -327,6 +348,12 @@ int ttm_tt_populate(struct ttm_device *bdev,
> }
>
> return 0;
> +
> +error:
> + atomic_long_sub(ttm->num_pages, _pages_allocated);
> + if (bdev->pool.use_dma32)
> + atomic_long_sub(ttm->num_pages, _dma32_pages_allocated);
> + return ret;
> }
> EXPORT_SYMBOL(ttm_tt_populate);
>
> @@ -342,12 +369,9 @@ static void ttm_tt_clear_mapping(struct ttm_tt *ttm)
> (*page)->mapping = NULL;
> (*page++)->index = 0;
> }
> -
> - atomic_long_sub(ttm->num_pages, _pages);
> }
>
> -void ttm_tt_unpopulate(struct ttm_device *bdev,
> -struct ttm_tt *ttm)
> +void
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
> On 24/03/2021 18.20, Joe Perches wrote:
> > On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
> > > On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
> > > > On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
> > > []
> > > > > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c
> > > > > > b/drivers/gpu/drm/imx/imx-ldb.c
> > > > > []
> > > > > > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> > > > > > drm_encoder *encoder)
> > > > > > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > > > > > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child,
> > > > > > encoder);
> > > > > >
> > > > > > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > > > > > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > > > > + __func__, ERR_PTR(mux));
> > > > >
> > > > > This does not compile without warnings.
> > > > >
> > > > > drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> > > > > drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects
> > > > > argument of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> > > > > 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > > > | ^~
> > > > >
> > > > > If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> > > > > is converting an int a void * to decode the error type and
> > > > > emit it as a string.
> > > >
> > > > Sorry about that.
> > > >
> > > > I decided against using ERR_PTR() in order to also check for
> > > > positive array overflow, but the version I tested was different from
> > > > the version I sent.
> > > >
> > > > v3 coming.
> > >
> > > Thanks. No worries.
> > >
> > > Up to you, vsprintf would emit the positive mux as a funky hashed
> > > hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
> > > perhaps %d without the ERR_PTR use makes the most sense.
> > >
>
> >
> > Maybe it's better to output non PTR_ERR %pe uses as decimal so this
> > sort of code would work.
>
> No, because that would leak the pointer value when somebody has
> accidentally passed a real kernel pointer to %pe.
I think it's not really an issue.
_All_ code that uses %p extensions need inspection anyway.
It's already possible to intentionally 'leak' the ptr value
by using %pe, -ptr so I think that's not really an issue.
>
> If the code wants a cute -EFOO string explaining what's wrong, what
> about "%pe", ERR_PTR(mux < 0 : mux : -ERANGE)? Or two separate error
> messages
>
> if (mux < 0)
> ...
> else if (mux >= ARRAY_SIZE())
> ...
Multiple tests, more unnecessary code, multiple format strings, etc...
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/ttm: stop warning on TT shrinker failure
On Wed, Mar 24, 2021 at 01:07:44PM +0100, Christian König wrote: > > > Am 24.03.21 um 13:01 schrieb Daniel Vetter: > > On Wed, Mar 24, 2021 at 01:00:28PM +0100, Christian König wrote: > > > Am 24.03.21 um 12:55 schrieb Daniel Vetter: > > > > On Wed, Mar 24, 2021 at 11:19:13AM +0100, Thomas Hellström (Intel) > > > > wrote: > > > > > On 3/23/21 4:45 PM, Christian König wrote: > > > > > > Am 23.03.21 um 16:13 schrieb Michal Hocko: > > > > > > > On Tue 23-03-21 14:56:54, Christian König wrote: > > > > > > > > Am 23.03.21 um 14:41 schrieb Michal Hocko: > > > > > > > [...] > > > > > > > > > Anyway, I am wondering whether the overall approach is > > > > > > > > > sound. Why don't > > > > > > > > > you simply use shmem as your backing storage from the > > > > > > > > > beginning and pin > > > > > > > > > those pages if they are used by the device? > > > > > > > > Yeah, that is exactly what the Intel guys are doing for their > > > > > > > > integrated > > > > > > > > GPUs :) > > > > > > > > > > > > > > > > Problem is for TTM I need to be able to handle dGPUs and those > > > > > > > > have all > > > > > > > > kinds of funny allocation restrictions. In other words I need to > > > > > > > > guarantee > > > > > > > > that the allocated memory is coherent accessible to the GPU > > > > > > > > without using > > > > > > > > SWIOTLB. > > > > > > > > > > > > > > > > The simple case is that the device can only do DMA32, but you > > > > > > > > also got > > > > > > > > device which can only do 40bits or 48bits. > > > > > > > > > > > > > > > > On top of that you also got AGP, CMA and stuff like CPU cache > > > > > > > > behavior > > > > > > > > changes (write back vs. write through, vs. uncached). > > > > > > > OK, so the underlying problem seems to be that gfp mask (thus > > > > > > > mapping_gfp_mask) cannot really reflect your requirements, right? > > > > > > > Would > > > > > > > it help if shmem would allow to provide an allocation callback to > > > > > > > override alloc_page_vma which is used currently? I am pretty sure > > > > > > > there > > > > > > > will be more to handle but going through shmem for the whole life > > > > > > > time > > > > > > > is just so much easier to reason about than some tricks to abuse > > > > > > > shmem > > > > > > > just for the swapout path. > > > > > > Well it's a start, but the pages can have special CPU cache > > > > > > settings. So > > > > > > direct IO from/to them usually doesn't work as expected. > > > > > > > > > > > > Additional to that for AGP and CMA I need to make sure that I give > > > > > > those > > > > > > pages back to the relevant subsystems instead of just dropping the > > > > > > page > > > > > > reference. > > > > > > > > > > > > So I would need to block for the swapio to be completed. > > > > > > > > > > > > Anyway I probably need to revert those patches for now since this > > > > > > isn't > > > > > > working as we hoped it would. > > > > > > > > > > > > Thanks for the explanation how stuff works here. > > > > > Another alternative here that I've tried before without being > > > > > successful > > > > > would perhaps be to drop shmem completely and, if it's a normal page > > > > > (no dma > > > > > or funny caching attributes) just use add_to_swap_cache()? If it's > > > > > something > > > > > else, try alloc a page with relevant gfp attributes, copy and > > > > > add_to_swap_cache()? Or perhaps that doesn't work well from a shrinker > > > > > either? > > > > So before we toss everything and go an a great rewrite-the-world tour, > > > > what if we just try to split up big objects. So for objects which are > > > > bigger than e.g. 10mb > > > > > > > > - move them to a special "under eviction" list > > > > - keep a note how far we evicted thus far > > > > - interleave allocating shmem pages, copying data and releasing the ttm > > > > backing store on a chunk basis (maybe 10mb or whatever, tuning tbh) > > > > > > > > If that's not enough, occasionally break out of the shrinker entirely so > > > > other parts of reclaim can reclaim the shmem stuff. But just releasing > > > > our > > > > own pages as we go should help a lot I think. > > > Yeah, the later is exactly what I was currently prototyping. > > > > > > I just didn't used a limit but rather a only partially evicted BOs list > > > which is used when we fail to allocate a page. > > > > > > For the 5.12 cycle I think we should just go back to a hard 50% limit for > > > now and then resurrect this when we have solved the issues. > > Can we do the 50% limit without tossing out all the code we've done thus > > far? Just so this doesn't get too disruptive. > > Yeah, I just need to get back to v1 of this patch. Before you convinced me > that the shrinker is the better approach .) I don't think there's anything else than the shrinker if you want dynamically sized memory usage. Or pinning it all. Implementing our own kswapd and not tying into direct reclaim does not sound like a good
Re: [PATCH 3/3] mm: unexport follow_pfn
On Wed, Mar 24, 2021 at 09:52:11AM -0300, Jason Gunthorpe wrote:
> On Tue, Mar 16, 2021 at 04:33:03PM +0100, Daniel Vetter wrote:
> > Both kvm (in bd2fae8da794 ("KVM: do not assume PTE is writable after
> > follow_pfn")) and vfio (in 07956b6269d3 ("vfio/type1: Use
> > follow_pte()")) have lost their callsites of follow_pfn(). All the
> > other ones have been switched over to unsafe_follow_pfn because they
> > cannot be fixed without breaking userspace api.
> >
> > Argueably the vfio code is still racy, but that's kinda a bigger
> > picture. But since it does leak the pte beyond where it drops the pt
> > lock, without anything else like an mmu notifier guaranteeing
> > coherence, the problem is at least clearly visible in the vfio code.
> > So good enough with me.
> >
> > I've decided to keep the explanation that after dropping the pt lock
> > you must have an mmu notifier if you keep using the pte somehow by
> > adjusting it and moving it into the kerneldoc for the new follow_pte()
> > function.
> >
> > Cc: 3...@google.com
> > Cc: Jann Horn
> > Cc: Paolo Bonzini
> > Cc: Jason Gunthorpe
> > Cc: Cornelia Huck
> > Cc: Peter Xu
> > Cc: Alex Williamson
> > Cc: linux...@kvack.org
> > Cc: linux-arm-ker...@lists.infradead.org
> > Cc: linux-samsung-...@vger.kernel.org
> > Cc: linux-me...@vger.kernel.org
> > Cc: k...@vger.kernel.org
> > Signed-off-by: Daniel Vetter
> > ---
> > include/linux/mm.h | 2 --
> > mm/memory.c| 26 +-
> > mm/nommu.c | 13 +
> > 3 files changed, 6 insertions(+), 35 deletions(-)
>
> I think this is the right thing to do.
Was just about to smash this into the topic branch for testing in
linux-next. Feel like an ack on the series, or at least the two mm
patches?
-Daniel
>
> Alex is working on fixing VFIO and while kvm is still racy using
> follow pte, I think they are working on it too?
>
> Jason
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/mediatek: Switch the hdmi bridge ops to the atomic versions
The bridge operation '.enable' and the audio cb '.get_eld'
access hdmi->conn. In the future we will want to support
the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR and then we will
not have direct access to the connector.
The atomic version '.atomic_enable' allows accessing the
current connector from the state.
This patch switches the bridge to the atomic version and
saves the current connector in a new field 'curr_conn'.
Signed-off-by: Dafna Hirschfeld
---
drivers/gpu/drm/mediatek/mtk_hdmi.c | 41 -
1 file changed, 29 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c
b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 8ee55f9e2954..9f415c508b33 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -154,6 +154,7 @@ struct mtk_hdmi {
struct drm_bridge bridge;
struct drm_bridge *next_bridge;
struct drm_connector conn;
+ struct drm_connector *curr_conn;
struct device *dev;
const struct mtk_hdmi_conf *conf;
struct phy *phy;
@@ -974,7 +975,7 @@ static int mtk_hdmi_setup_avi_infoframe(struct mtk_hdmi
*hdmi,
ssize_t err;
err = drm_hdmi_avi_infoframe_from_display_mode(,
- >conn, mode);
+ hdmi->curr_conn, mode);
if (err < 0) {
dev_err(hdmi->dev,
"Failed to get AVI infoframe from mode: %zd\n", err);
@@ -1054,7 +1055,7 @@ static int
mtk_hdmi_setup_vendor_specific_infoframe(struct mtk_hdmi *hdmi,
ssize_t err;
err = drm_hdmi_vendor_infoframe_from_display_mode(,
- >conn, mode);
+ hdmi->curr_conn,
mode);
if (err) {
dev_err(hdmi->dev,
"Failed to get vendor infoframe from mode: %zd\n", err);
@@ -1357,7 +1358,8 @@ static bool mtk_hdmi_bridge_mode_fixup(struct drm_bridge
*bridge,
return true;
}
-static void mtk_hdmi_bridge_disable(struct drm_bridge *bridge)
+static void mtk_hdmi_bridge_atomic_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_bridge_state)
{
struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
@@ -1368,10 +1370,13 @@ static void mtk_hdmi_bridge_disable(struct drm_bridge
*bridge)
clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);
clk_disable_unprepare(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]);
+ hdmi->curr_conn = NULL;
+
hdmi->enabled = false;
}
-static void mtk_hdmi_bridge_post_disable(struct drm_bridge *bridge)
+static void mtk_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_state)
{
struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
@@ -1406,7 +1411,8 @@ static void mtk_hdmi_bridge_mode_set(struct drm_bridge
*bridge,
drm_mode_copy(>mode, adjusted_mode);
}
-static void mtk_hdmi_bridge_pre_enable(struct drm_bridge *bridge)
+static void mtk_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state
*old_state)
{
struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
@@ -1426,10 +1432,16 @@ static void mtk_hdmi_send_infoframe(struct mtk_hdmi
*hdmi,
mtk_hdmi_setup_vendor_specific_infoframe(hdmi, mode);
}
-static void mtk_hdmi_bridge_enable(struct drm_bridge *bridge)
+static void mtk_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_state)
{
+ struct drm_atomic_state *state = old_state->base.state;
struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
+ /* Retrieve the connector through the atomic state. */
+ hdmi->curr_conn = drm_atomic_get_new_connector_for_encoder(state,
+
bridge->encoder);
+
mtk_hdmi_output_set_display_mode(hdmi, >mode);
clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PLL]);
clk_prepare_enable(hdmi->clk[MTK_HDMI_CLK_HDMI_PIXEL]);
@@ -1440,13 +1452,16 @@ static void mtk_hdmi_bridge_enable(struct drm_bridge
*bridge)
}
static const struct drm_bridge_funcs mtk_hdmi_bridge_funcs = {
+ .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+ .atomic_reset = drm_atomic_helper_bridge_reset,
.attach = mtk_hdmi_bridge_attach,
.mode_fixup = mtk_hdmi_bridge_mode_fixup,
- .disable = mtk_hdmi_bridge_disable,
- .post_disable = mtk_hdmi_bridge_post_disable,
+ .atomic_disable = mtk_hdmi_bridge_atomic_disable,
+ .atomic_post_disable = mtk_hdmi_bridge_atomic_post_disable,
[PATCH 2/2] drm/mediatek: Don't support hdmi connector creation
commit f01195148967 ("drm/mediatek: mtk_dpi: Create connector for bridges")
broke the display support for elm device since mtk_dpi calls
drm_bridge_attach with the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR
while mtk_hdmi does not yet support this flag.
Fix this by accepting DRM_BRIDGE_ATTACH_NO_CONNECTOR in bridge attachment.
Implement the drm_bridge_funcs .detect() and .get_edid() operations, and
call drm_bridge_hpd_notify() to report HPD. This provides the
necessary API to support disabling connector creation.
This patch is inspired by a similar patch for bridge/synopsys/dw-hdmi.c:
commit ec971aaa6775 ("drm: bridge: dw-hdmi: Make connector creation optional")
But with the difference that in mtk-hdmi only the option of not creating
a connector is supported.
Fixes: f01195148967 ("drm/mediatek: mtk_dpi: Create connector for bridges")
Signed-off-by: Dafna Hirschfeld
---
drivers/gpu/drm/mediatek/mtk_hdmi.c | 130 ++--
1 file changed, 44 insertions(+), 86 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c
b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 9f415c508b33..9da5dfb7c7fb 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -153,7 +153,6 @@ struct mtk_hdmi_conf {
struct mtk_hdmi {
struct drm_bridge bridge;
struct drm_bridge *next_bridge;
- struct drm_connector conn;
struct drm_connector *curr_conn;
struct device *dev;
const struct mtk_hdmi_conf *conf;
@@ -187,11 +186,6 @@ static inline struct mtk_hdmi *hdmi_ctx_from_bridge(struct
drm_bridge *b)
return container_of(b, struct mtk_hdmi, bridge);
}
-static inline struct mtk_hdmi *hdmi_ctx_from_conn(struct drm_connector *c)
-{
- return container_of(c, struct mtk_hdmi, conn);
-}
-
static u32 mtk_hdmi_read(struct mtk_hdmi *hdmi, u32 offset)
{
return readl(hdmi->regs + offset);
@@ -1202,48 +1196,30 @@ mtk_hdmi_update_plugged_status(struct mtk_hdmi *hdmi)
connector_status_connected : connector_status_disconnected;
}
-static enum drm_connector_status hdmi_conn_detect(struct drm_connector *conn,
- bool force)
-{
- struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn);
- return mtk_hdmi_update_plugged_status(hdmi);
-}
-
-static void hdmi_conn_destroy(struct drm_connector *conn)
+static struct edid *mtk_hdmi_get_edid(struct mtk_hdmi *hdmi,
+ struct drm_connector *connector)
{
- struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn);
-
- mtk_cec_set_hpd_event(hdmi->cec_dev, NULL, NULL);
-
- drm_connector_cleanup(conn);
-}
-
-static int mtk_hdmi_conn_get_modes(struct drm_connector *conn)
-{
- struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn);
struct edid *edid;
- int ret;
if (!hdmi->ddc_adpt)
- return -ENODEV;
-
- edid = drm_get_edid(conn, hdmi->ddc_adpt);
+ return NULL;
+ edid = drm_get_edid(connector, hdmi->ddc_adpt);
if (!edid)
- return -ENODEV;
-
+ return NULL;
hdmi->dvi_mode = !drm_detect_monitor_audio(edid);
+ return edid;
+}
- drm_connector_update_edid_property(conn, edid);
-
- ret = drm_add_edid_modes(conn, edid);
- kfree(edid);
- return ret;
+static enum drm_connector_status mtk_hdmi_detect(struct mtk_hdmi *hdmi)
+{
+ return mtk_hdmi_update_plugged_status(hdmi);
}
-static int mtk_hdmi_conn_mode_valid(struct drm_connector *conn,
- struct drm_display_mode *mode)
+static int mtk_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
+ const struct drm_display_info *info,
+ const struct drm_display_mode *mode)
{
- struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn);
+ struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
struct drm_bridge *next_bridge;
dev_dbg(hdmi->dev, "xres=%d, yres=%d, refresh=%d, intl=%d clock=%d\n",
@@ -1268,74 +1244,50 @@ static int mtk_hdmi_conn_mode_valid(struct
drm_connector *conn,
return drm_mode_validate_size(mode, 0x1fff, 0x1fff);
}
-static struct drm_encoder *mtk_hdmi_conn_best_enc(struct drm_connector *conn)
-{
- struct mtk_hdmi *hdmi = hdmi_ctx_from_conn(conn);
-
- return hdmi->bridge.encoder;
-}
-
-static const struct drm_connector_funcs mtk_hdmi_connector_funcs = {
- .detect = hdmi_conn_detect,
- .fill_modes = drm_helper_probe_single_connector_modes,
- .destroy = hdmi_conn_destroy,
- .reset = drm_atomic_helper_connector_reset,
- .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
- .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
-static const struct drm_connector_helper_funcs
- mtk_hdmi_connector_helper_funcs = {
- .get_modes = mtk_hdmi_conn_get_modes,
-
[PATCH 2/2] drm/mediatek: Don't support hdmi connector creation
commit f01195148967 ("drm/mediatek: mtk_dpi: Create connector for bridges")
broke the display support for elm device since mtk_dpi calls
drm_bridge_attach with the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR
while mtk_hdmi does not yet support this flag.
These two patches fix that by adding support for DRM_BRIDGE_ATTACH_NO_CONNECTOR
in mtk_hdmi bridge attachment. The first patch moves the mkt-hdmi bridge
to the atomic API in order to be able to access the connector through the
global state.
Dafna Hirschfeld (2):
drm/mediatek: Switch the hdmi bridge ops to the atomic versions
drm/mediatek: Don't support hdmi connector creation
drivers/gpu/drm/mediatek/mtk_hdmi.c | 171
1 file changed, 73 insertions(+), 98 deletions(-)
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/2] mm,drm/ttm: Block fast GUP to TTM huge pages
Am 24.03.21 um 17:38 schrieb Jason Gunthorpe: On Wed, Mar 24, 2021 at 04:50:14PM +0100, Thomas Hellström (Intel) wrote: On 3/24/21 2:48 PM, Jason Gunthorpe wrote: On Wed, Mar 24, 2021 at 02:35:38PM +0100, Thomas Hellström (Intel) wrote: In an ideal world the creation/destruction of page table levels would by dynamic at this point, like THP. Hmm, but I'm not sure what problem we're trying to solve by changing the interface in this way? We are trying to make a sensible driver API to deal with huge pages. Currently if the core vm requests a huge pud, we give it one, and if we can't or don't want to (because of dirty-tracking, for example, which is always done on 4K page-level) we just return VM_FAULT_FALLBACK, and the fault is retried at a lower level. Well, my thought would be to move the pte related stuff into vmf_insert_range instead of recursing back via VM_FAULT_FALLBACK. I don't know if the locking works out, but it feels cleaner that the driver tells the vmf how big a page it can stuff in, not the vm telling the driver to stuff in a certain size page which it might not want to do. Some devices want to work on a in-between page size like 64k so they can't form 2M pages but they can stuff 64k of 4K pages in a batch on every fault. Hmm, yes, but we would in that case be limited anyway to insert ranges smaller than and equal to the fault size to avoid extensive and possibly unnecessary checks for contigous memory. Why? The insert function is walking the page tables, it just updates things as they are. It learns the arragement for free while doing the walk. The device has to always provide consistent data, if it overlaps into pages that are already populated that is fine so long as it isn't changing their addresses. And then if we can't support the full fault size, we'd need to either presume a size and alignment of the next level or search for contigous memory in both directions around the fault address, perhaps unnecessarily as well. You don't really need to care about levels, the device should be faulting in the largest memory regions it can within its efficiency. If it works on 4M pages then it should be faulting 4M pages. The page size of the underlying CPU doesn't really matter much other than some tuning to impact how the device's allocator works. I agree with Jason here. We get the best efficiency when we look at the what the GPU driver provides and make sure that we handle one GPU page at once instead of looking to much into what the CPU is doing with it's page tables. At least one AMD GPUs the GPU page size can be anything between 4KiB and 2GiB and if we will in a 2GiB chunk at once this can in theory be handled by just two giant page table entries on the CPU side. On the other hand I'm not sure how filling in the CPU page tables work in detail. Christian. Jason ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v9 63/70] drm/i915: Move gt_revoke() slightly
On Wed, Mar 24, 2021 at 06:16:44PM +0100, Daniel Vetter wrote:
> On Wed, Mar 24, 2021 at 07:15:36PM +0200, Ville Syrjälä wrote:
> > On Wed, Mar 24, 2021 at 06:00:12PM +0100, Daniel Vetter wrote:
> > > On Tue, Mar 23, 2021 at 04:50:52PM +0100, Maarten Lankhorst wrote:
> > > > We get a lockdep splat when the reset mutex is held, because it can be
> > > > taken from fence_wait. This conflicts with the mmu notifier we have,
> > > > because we recurse between reset mutex and mmap lock -> mmu notifier.
> > > >
> > > > Remove this recursion by calling revoke_mmaps before taking the lock.
> > > >
> > > > The reset code still needs fixing, as taking mmap locks during reset
> > > > is not allowed.
> > > >
> > > > Signed-off-by: Maarten Lankhorst
> > > > Reviewed-by: Thomas Hellström
> > > > ---
> > > > drivers/gpu/drm/i915/gt/intel_reset.c | 5 +++--
> > > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c
> > > > b/drivers/gpu/drm/i915/gt/intel_reset.c
> > > > index 990cb4adbb9a..447f589750c2 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> > > > @@ -970,8 +970,6 @@ static int do_reset(struct intel_gt *gt,
> > > > intel_engine_mask_t stalled_mask)
> > > > {
> > > > int err, i;
> > > >
> > > > - gt_revoke(gt);
> > > > -
> > > > err = __intel_gt_reset(gt, ALL_ENGINES);
> > > > for (i = 0; err && i < RESET_MAX_RETRIES; i++) {
> > > > msleep(10 * (i + 1));
> > > > @@ -1026,6 +1024,9 @@ void intel_gt_reset(struct intel_gt *gt,
> > > >
> > > > might_sleep();
> > > > GEM_BUG_ON(!test_bit(I915_RESET_BACKOFF, >reset.flags));
> > > > +
> > >
> > > I've added a FIXME comment here just so we don't totally forget. This will
> > > also blow up again when we wrap the entire reset path into a dma_fence
> > > critical section annotation (at least going forward, we can't do that on
> > > hw that needs display reset with the current code unfortunately).
> > >
> > > But I did look at the code which originally added this in
> > >
> > > commit 2caffbf1176256cc4f8d4e5c3c524fc689cb9876
> > > Author: Chris Wilson
> > > Date: Fri Feb 8 15:37:03 2019 +
> > >
> > > drm/i915: Revoke mmaps and prevent access to fence registers across
> > > reset
> > >
> > > and noped right out.
> > >
> > > I think this complexity needs to go entirely, and instead we just protect
> > > the fence register state to make sure that after reset they are all good
> > > again:
> > > - add a new mutex for low level fence register state
> > > - hold that mutex around fence register writes (really just the low level
> > > fence writes)
> > > - hold it in the reset path when we restore fence registers
> > >
> > > This means that a global reset also thrashes mmaps, but it's a global
> > > reset we're talking about here, everything is thrash anyway. Plus/minus
> > > fenced gtt mmaps really doesn't change the tally.
> >
> > My recollection is that GPU reset doesn't actually clobber the fence
> > registers. Though not 100% sure I can trust my brain on this. Also
> > dunno if it actually matter here or not, but figured I'd point it out.
>
> I think on gen2/3 it does, because there everything goes over. But yeah
> maybe on gen4+ it's all fine, would be worth to check that.
Right you are. Gave it a quick test on my 945gm and the fence
registers did get zeroed out. I guess it was snb+ where it didn't
happen. Well, could be some of the earlier platforms too I guess.
--
Ville Syrjälä
Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 21/21] drm/msm/dpu: call hw_intr ops directly
Hi Dmitry,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.12-rc4 next-20210324]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Dmitry-Baryshkov/drm-msm-dpu-cleanup-callbacks-resource-manager/20210324-230347
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
7acac4b3196caee5e21fb5ea53f8bc124e6a16fc
config: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
#
https://github.com/0day-ci/linux/commit/8f82b58643355f9e0d03c022b66e276c252e633a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review
Dmitry-Baryshkov/drm-msm-dpu-cleanup-callbacks-resource-manager/20210324-230347
git checkout 8f82b58643355f9e0d03c022b66e276c252e633a
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c:889:5: warning: no
>> previous prototype for 'dpu_hw_intr_disable_irq_nolock'
>> [-Wmissing-prototypes]
889 | int dpu_hw_intr_disable_irq_nolock(struct dpu_hw_intr *intr, int
irq_idx)
| ^~
vim +/dpu_hw_intr_disable_irq_nolock +889
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c
888
> 889 int dpu_hw_intr_disable_irq_nolock(struct dpu_hw_intr *intr, int
> irq_idx)
890 {
891 int reg_idx;
892 const struct dpu_intr_reg *reg;
893 const struct dpu_irq_type *irq;
894 const char *dbgstr = NULL;
895 uint32_t cache_irq_mask;
896
897 if (!intr)
898 return -EINVAL;
899
900 if (irq_idx < 0 || irq_idx >= ARRAY_SIZE(dpu_irq_map)) {
901 pr_err("invalid IRQ index: [%d]\n", irq_idx);
902 return -EINVAL;
903 }
904
905 irq = _irq_map[irq_idx];
906 reg_idx = irq->reg_idx;
907 reg = _intr_set[reg_idx];
908
909 cache_irq_mask = intr->cache_irq_mask[reg_idx];
910 if ((cache_irq_mask & irq->irq_mask) == 0) {
911 dbgstr = "DPU IRQ is already cleared:";
912 } else {
913 dbgstr = "DPU IRQ mask disable:";
914
915 cache_irq_mask &= ~irq->irq_mask;
916 /* Disable interrupts based on the new mask */
917 DPU_REG_WRITE(>hw, reg->en_off, cache_irq_mask);
918 /* Cleaning any pending interrupt */
919 DPU_REG_WRITE(>hw, reg->clr_off, irq->irq_mask);
920
921 /* ensure register write goes through */
922 wmb();
923
924 intr->cache_irq_mask[reg_idx] = cache_irq_mask;
925 }
926
927 pr_debug("%s MASK:0x%.8x, CACHE-MASK:0x%.8x\n", dbgstr,
928 irq->irq_mask, cache_irq_mask);
929
930 return 0;
931 }
932
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RE: [PATCH v2] video: hyperv_fb: Fix a double free in hvfb_probe
From: Lv Yunlong Sent: Wednesday, March 24, 2021
3:37 AM
>
> In function hvfb_probe in hyperv_fb.c, it calls hvfb_getmem(hdev, info)
> and return err when info->apertures is freed.
>
> In the error1 label of hvfb_probe, info->apertures will be freed for the
> second time in framebuffer_release(info).
>
> My patch removes all kfree(info->apertures) instead of set info->apertures
> to NULL. It is because that let framebuffer_release() handle freeing the
> memory flows the fbdev pattern, and less code overall.
Let me suggest some clarifications in the commit message. It's probably
better not to reference the initial approach of setting info->apertures to
NULL, since there won't be any record of that approach in the commit
history. Here's what I would suggest:
Function hvfb_probe() calls hvfb_getmem(), expecting upon return that
info->apertures is either NULL or points to memory that should be freed
by framebuffer_release(). But hvfb_getmem() is freeing the memory and
leaving the pointer non-NULL, resulting in a double free if an error
occurs or later if hvfb_remove() is called.
Fix this by removing all kfree(info->apertures) calls in hvfb_getmem().
This will allow framebuffer_release() to free the memory, which follows
the pattern of other fbdev drivers.
Modulo this revision to the commit message, which Wei Liu can
probably incorporate,
Reviewed-by: Michael Kelley
>
> Signed-off-by: Lv Yunlong
> ---
> drivers/video/fbdev/hyperv_fb.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index c8b0ae676809..4dc9077dd2ac 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -1031,7 +1031,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct
> fb_info
> *info)
> PCI_DEVICE_ID_HYPERV_VIDEO, NULL);
> if (!pdev) {
> pr_err("Unable to find PCI Hyper-V video\n");
> - kfree(info->apertures);
> return -ENODEV;
> }
>
> @@ -1129,7 +1128,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct
> fb_info
> *info)
> } else {
> pci_dev_put(pdev);
> }
> - kfree(info->apertures);
>
> return 0;
>
> @@ -1141,7 +1139,6 @@ static int hvfb_getmem(struct hv_device *hdev, struct
> fb_info
> *info)
> err1:
> if (!gen2vm)
> pci_dev_put(pdev);
> - kfree(info->apertures);
>
> return -ENOMEM;
> }
> --
> 2.25.1
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 00/25] Rid W=1 warnings from HID
This set is part of a larger effort attempting to clean-up W=1 kernel builds, which are currently overwhelmingly riddled with niggly little warnings. Lee Jones (25): HID: intel-ish-hid: Remove unused variable 'err' HID: ishtp-hid-client: Move variable to where it's actually used HID: intel-ish-hid: pci-ish: Remove unused variable 'ret' HID: intel-ish: Supply some missing param descriptions HID: intel-ish: Fix a naming disparity and a formatting error HID: usbhid: Repair a formatting issue in a struct description HID: intel-ish-hid: Fix a little doc-rot HID: usbhid: hid-pidff: Demote a couple kernel-doc abuses HID: hid-alps: Correct struct misnaming HID: intel-ish-hid: Fix potential copy/paste error HID: hid-core: Fix incorrect function name in header HID: intel-ish-hid: ipc: Correct fw_reset_work_fn() function name in header HID: ishtp-hid-client: Fix incorrect function name report_bad_packet() HID: hid-kye: Fix incorrect function name for kye_tablet_enable() HID: hid-picolcd_core: Remove unused variable 'ret' HID: hid-logitech-hidpp: Fix conformant kernel-doc header and demote abuses HID: hid-uclogic-rdesc: Kernel-doc is for functions and structs HID: hid-thrustmaster: Demote a bunch of kernel-doc abuses HID: hid-uclogic-params: Ensure function names are present and correct in kernel-doc headers HID: hid-sensor-custom: Remove unused variable 'ret' HID: wacom_sys: Demote kernel-doc abuse HID: hid-sensor-hub: Remove unused struct member 'quirks' HID: hid-sensor-hub: Move 'hsdev' description to correct struct definition HID: intel-ish-hid: ishtp-fw-loader: Fix a bunch of formatting issues HID: ishtp-hid-client: Fix 'suggest-attribute=format' compiler warning drivers/hid/hid-alps.c | 2 +- drivers/hid/hid-core.c | 2 +- drivers/hid/hid-kye.c| 2 +- drivers/hid/hid-logitech-hidpp.c | 7 +-- drivers/hid/hid-picolcd_core.c | 5 +-- drivers/hid/hid-sensor-custom.c | 5 +-- drivers/hid/hid-sensor-hub.c | 4 +- drivers/hid/hid-thrustmaster.c | 24 +-- drivers/hid/hid-uclogic-params.c | 8 ++-- drivers/hid/hid-uclogic-rdesc.c | 2 +- drivers/hid/intel-ish-hid/ipc/ipc.c | 2 +- drivers/hid/intel-ish-hid/ipc/pci-ish.c | 3 +- drivers/hid/intel-ish-hid/ishtp-fw-loader.c | 45 ++-- drivers/hid/intel-ish-hid/ishtp-hid-client.c | 11 +++-- drivers/hid/intel-ish-hid/ishtp-hid.c| 2 +- drivers/hid/intel-ish-hid/ishtp-hid.h| 9 +--- drivers/hid/intel-ish-hid/ishtp/bus.c| 9 +++- drivers/hid/intel-ish-hid/ishtp/client.c | 5 +-- drivers/hid/intel-ish-hid/ishtp/hbm.c| 4 +- drivers/hid/intel-ish-hid/ishtp/ishtp-dev.h | 4 +- drivers/hid/usbhid/hid-pidff.c | 4 +- drivers/hid/usbhid/usbkbd.c | 2 +- drivers/hid/wacom_sys.c | 2 +- include/linux/intel-ish-client-if.h | 8 +++- 24 files changed, 90 insertions(+), 81 deletions(-) Cc: Alexandre Torgue Cc: Anssi Hannula Cc: Benjamin Tissoires Cc: "Bruno Prémont" Cc: "Christian König" Cc: Daniel Drubin Cc: Dario Pagani Cc: dri-devel@lists.freedesktop.org Cc: Henrik Rydberg Cc: Jiri Kosina Cc: Jonathan Cameron Cc: Kai-Heng Feng Cc: Kim Kuparinen Cc: "Krzysztof Wilczyński" Cc: Lee Jones Cc: linaro-mm-...@lists.linaro.org Cc: linux-arm-ker...@lists.infradead.org Cc: linux-...@vger.kernel.org Cc: linux-in...@vger.kernel.org Cc: linux-me...@vger.kernel.org Cc: linux-st...@st-md-mailman.stormreply.com Cc: linux-...@vger.kernel.org Cc: Lopez Casado Cc: "L. Vinyard, Jr" Cc: Masaki Ota Cc: Maxime Coquelin Cc: message to Cc: Michael Haboustak Cc: Rushikesh S Kadam Cc: Srinivas Pandruvada Cc: Sumit Semwal Cc: "Uwe Kleine-König" Cc: Vojtech Pavlik Cc: Zhang Lixu -- 2.27.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On 24/03/2021 18.20, Joe Perches wrote:
> On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
>> On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
>>> On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
>> []
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
[]
> @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> drm_encoder *encoder)
> int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
>
> + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> + __func__, ERR_PTR(mux));
This does not compile without warnings.
drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects
argument of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
| ^~
If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
is converting an int a void * to decode the error type and
emit it as a string.
>>>
>>> Sorry about that.
>>>
>>> I decided against using ERR_PTR() in order to also check for
>>> positive array overflow, but the version I tested was different from
>>> the version I sent.
>>>
>>> v3 coming.
>>
>> Thanks. No worries.
>>
>> Up to you, vsprintf would emit the positive mux as a funky hashed
>> hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
>> perhaps %d without the ERR_PTR use makes the most sense.
>>
>
> Maybe it's better to output non PTR_ERR %pe uses as decimal so this
> sort of code would work.
No, because that would leak the pointer value when somebody has
accidentally passed a real kernel pointer to %pe.
If the code wants a cute -EFOO string explaining what's wrong, what
about "%pe", ERR_PTR(mux < 0 : mux : -ERANGE)? Or two separate error
messages
if (mux < 0)
...
else if (mux >= ARRAY_SIZE())
...
Rasmus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 10/11] drm/i915: avoid stringop-overread warning on pri_latency
On Wed, Mar 24, 2021 at 05:30:24PM +0200, Jani Nikula wrote:
> On Mon, 22 Mar 2021, Arnd Bergmann wrote:
> > From: Arnd Bergmann
> >
> > gcc-11 warns about what appears to be an out-of-range array access:
> >
> > In function ‘snb_wm_latency_quirk’,
> > inlined from ‘ilk_setup_wm_latency’ at
> > drivers/gpu/drm/i915/intel_pm.c:3108:3:
> > drivers/gpu/drm/i915/intel_pm.c:3057:9: error: ‘intel_print_wm_latency’
> > reading 16 bytes from a region of size 10 [-Werror=stringop-overread]
> > 3057 | intel_print_wm_latency(dev_priv, "Primary",
> > dev_priv->wm.pri_latency);
> > |
> > ^
> > drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
> > drivers/gpu/drm/i915/intel_pm.c:3057:9: note: referencing argument 3 of
> > type ‘const u16 *’ {aka ‘const short unsigned int *’}
> > drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
> > ‘intel_print_wm_latency’
> > 2994 | static void intel_print_wm_latency(struct drm_i915_private
> > *dev_priv,
> > | ^~
> >
> > My guess is that this code is actually safe because the size of the
> > array depends on the hardware generation, and the function checks for
> > that, but at the same time I would not expect the compiler to work it
> > out correctly, and the code seems a little fragile with regards to
> > future changes. Simply increasing the size of the array should help.
>
> Agreed, I don't think there's an issue, but the code could use a bunch
> of improvements.
>
> Like, we have intel_print_wm_latency() for debug logging and
> wm_latency_show() for debugfs, and there's a bunch of duplication and
> ugh.
There is all this ancient stuff in review limbo...
https://patchwork.freedesktop.org/series/50802/
--
Ville Syrjälä
Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 4/7] drm/i915: Handle async cancellation in sentinel assert
On Wed, 24 Mar 2021 at 12:14, Tvrtko Ursulin wrote: > > From: Tvrtko Ursulin > > With the watchdog cancelling requests asynchronously to preempt-to-busy we > need to relax one assert making it apply only to requests not in error. > > v2: > * Check against the correct request! > > v3: > * Simplify the check to avoid the question of when to sample the fence >error vs sentinel bit. > > Signed-off-by: Tvrtko Ursulin Reviewed-by: Matthew Auld ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v9 66/70] drm/i915: Add ww parameter to get_pages() callback
On Tue, Mar 23, 2021 at 04:50:55PM +0100, Maarten Lankhorst wrote:
> We will need this to support eviction with lmem, so
> explicitly pass ww as a parameter.
>
> Signed-off-by: Maarten Lankhorst
I'm leaving this and later patches out for now. Would be good to
fast-track the bsw fix, but the others are for lmem enabling, so imo can
go in through the usual way.
-Daniel
> ---
> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 3 ++-
> drivers/gpu/drm/i915/gem/i915_gem_internal.c | 3 ++-
> drivers/gpu/drm/i915/gem/i915_gem_object_types.h | 3 ++-
> drivers/gpu/drm/i915/gem/i915_gem_pages.c| 2 +-
> drivers/gpu/drm/i915/gem/i915_gem_region.c | 3 ++-
> drivers/gpu/drm/i915/gem/i915_gem_region.h | 4 +++-
> drivers/gpu/drm/i915/gem/i915_gem_shmem.c| 3 ++-
> drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 3 ++-
> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 3 ++-
> drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c | 3 ++-
> drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 9 ++---
> drivers/gpu/drm/i915/gvt/dmabuf.c| 3 ++-
> drivers/gpu/drm/i915/selftests/i915_gem_gtt.c| 3 ++-
> 13 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> index 0926e0895ee6..1b3998c066a7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> @@ -199,7 +199,8 @@ struct dma_buf *i915_gem_prime_export(struct
> drm_gem_object *gem_obj, int flags)
> return drm_gem_dmabuf_export(gem_obj->dev, _info);
> }
>
> -static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
> +static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj,
> + struct i915_gem_ww_ctx *ww)
> {
> struct sg_table *pages;
> unsigned int sg_page_sizes;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> index 21cc40897ca8..90777fb5f5e0 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_internal.c
> @@ -30,7 +30,8 @@ static void internal_free_pages(struct sg_table *st)
> kfree(st);
> }
>
> -static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object
> *obj)
> +static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object
> *obj,
> + struct i915_gem_ww_ctx *ww)
> {
> struct drm_i915_private *i915 = to_i915(obj->base.dev);
> struct sg_table *st;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> index a5bc42c7087a..280f54a75ab1 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h
> @@ -50,7 +50,8 @@ struct drm_i915_gem_object_ops {
>* being released or under memory pressure (where we attempt to
>* reap pages for the shrinker).
>*/
> - int (*get_pages)(struct drm_i915_gem_object *obj);
> + int (*get_pages)(struct drm_i915_gem_object *obj,
> + struct i915_gem_ww_ctx *ww);
> void (*put_pages)(struct drm_i915_gem_object *obj,
> struct sg_table *pages);
> void (*truncate)(struct drm_i915_gem_object *obj);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> index aed8a37ccdc9..58e222030e10 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -100,7 +100,7 @@ int i915_gem_object_get_pages(struct
> drm_i915_gem_object *obj)
> return -EFAULT;
> }
>
> - err = obj->ops->get_pages(obj);
> + err = obj->ops->get_pages(obj, NULL);
> GEM_BUG_ON(!err && !i915_gem_object_has_pages(obj));
>
> return err;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.c
> b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> index 6a84fb6dde24..6cb8b70c19bf 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_region.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.c
> @@ -20,7 +20,8 @@ i915_gem_object_put_pages_buddy(struct drm_i915_gem_object
> *obj,
> }
>
> int
> -i915_gem_object_get_pages_buddy(struct drm_i915_gem_object *obj)
> +i915_gem_object_get_pages_buddy(struct drm_i915_gem_object *obj,
> + struct i915_gem_ww_ctx *ww)
> {
> const u64 max_segment = i915_sg_segment_size();
> struct intel_memory_region *mem = obj->mm.region;
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_region.h
> b/drivers/gpu/drm/i915/gem/i915_gem_region.h
> index ebddc86d78f7..c6f250aac925 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_region.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_region.h
> @@ -9,10 +9,12 @@
>
[RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
> On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
> > On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
> []
> > > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c
> > > > b/drivers/gpu/drm/imx/imx-ldb.c
> > > []
> > > > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> > > > drm_encoder *encoder)
> > > > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > > > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child,
> > > > encoder);
> > > >
> > > > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > > > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > > + __func__, ERR_PTR(mux));
> > >
> > > This does not compile without warnings.
> > >
> > > drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> > > drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects
> > > argument of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> > > 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > | ^~
> > >
> > > If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> > > is converting an int a void * to decode the error type and
> > > emit it as a string.
> >
> > Sorry about that.
> >
> > I decided against using ERR_PTR() in order to also check for
> > positive array overflow, but the version I tested was different from
> > the version I sent.
> >
> > v3 coming.
>
> Thanks. No worries.
>
> Up to you, vsprintf would emit the positive mux as a funky hashed
> hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
> perhaps %d without the ERR_PTR use makes the most sense.
>
>
Maybe it's better to output non PTR_ERR %pe uses as decimal so this
sort of code would work.
---
lib/vsprintf.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3600db686fa4..debdd1c62038 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -619,19 +619,23 @@ static char *string_nocheck(char *buf, char *end, const
char *s,
return widen_string(buf, len, end, spec);
}
-static char *err_ptr(char *buf, char *end, void *ptr,
-struct printf_spec spec)
+static noinline_for_stack
+char *err_ptr(char *buf, char *end, void *ptr, struct printf_spec spec)
{
int err = PTR_ERR(ptr);
- const char *sym = errname(err);
- if (sym)
- return string_nocheck(buf, end, sym, spec);
+ if (IS_ERR(ptr)) {
+ const char *sym = errname(err);
+
+ if (sym)
+ return string_nocheck(buf, end, sym, spec);
+ }
/*
-* Somebody passed ERR_PTR(-1234) or some other non-existing
-* Efoo - or perhaps CONFIG_SYMBOLIC_ERRNAME=n. Fall back to
-* printing it as its decimal representation.
+* Somebody passed ERR_PTR(-1234) or some other non-existing -E
+* or perhaps CONFIG_SYMBOLIC_ERRNAME=n
+* or perhaps a positive number like an array index
+* Fall back to printing it as its decimal representation.
*/
spec.flags |= SIGN;
spec.base = 10;
@@ -2407,9 +2411,7 @@ char *pointer(const char *fmt, char *buf, char *end, void
*ptr,
case 'x':
return pointer_string(buf, end, ptr, spec);
case 'e':
- /* %pe with a non-ERR_PTR gets treated as plain %p */
- if (!IS_ERR(ptr))
- break;
+ /* %pe with a non-ERR_PTR(ptr) gets treated as %ld */
return err_ptr(buf, end, ptr, spec);
case 'u':
case 'k':
---
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v9 70/70] drm/i915: Remove asynchronous vma binding
On Tue, Mar 23, 2021 at 04:50:59PM +0100, Maarten Lankhorst wrote:
> The current asynchronous work is done in a dma_fence_work, which
> has signalling annotations, because dma_fence_work signals on completion.
>
> On braswell, we can call stop_machine inside fence_work, causing a splat
> because memory allocation inside stop_machine is allowed.
>
> Strictly speaking this is a false positive, but go for safe and remove
> asynchronous vma binding entirely.
Please add at least the condensed version of the full dependency cycle
here. I'm also not sure we can call it a false positive, that's always a
bit a bold statement for lockdep splats which needs extroadinary evidence.
So maybe drop that part.
Also maybe good to reference the commit which added async ggtt pte writes
for reference here.
With those things on the commit message addressed:
Acked-by: Daniel Vetter
>
> Signed-off-by: Maarten Lankhorst
> ---
> drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 3 -
> drivers/gpu/drm/i915/gt/gen6_ppgtt.c | 1 -
> drivers/gpu/drm/i915/gt/gen8_ppgtt.c | 1 -
> drivers/gpu/drm/i915/gt/intel_ggtt.c | 4 -
> drivers/gpu/drm/i915/gt/intel_gtt.h| 2 -
> drivers/gpu/drm/i915/i915_gem.c| 6 -
> drivers/gpu/drm/i915/i915_vma.c| 174 +++--
> drivers/gpu/drm/i915/i915_vma.h| 5 +-
> drivers/gpu/drm/i915/i915_vma_types.h | 3 -
> 9 files changed, 21 insertions(+), 178 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> index 48b2258091c3..52b35b1a14f1 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> @@ -505,9 +505,6 @@ static void dbg_poison(struct i915_ggtt *ggtt,
> if (!drm_mm_node_allocated(>error_capture))
> return;
>
> - if (ggtt->vm.bind_async_flags & I915_VMA_GLOBAL_BIND)
> - return; /* beware stop_machine() inversion */
> -
> GEM_BUG_ON(!IS_ALIGNED(size, PAGE_SIZE));
>
> mutex_lock(>error_mutex);
> diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> index e08dff376339..0c5a9a767849 100644
> --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> @@ -436,7 +436,6 @@ struct i915_ppgtt *gen6_ppgtt_create(struct intel_gt *gt)
> ppgtt->base.vm.pd_shift = ilog2(SZ_4K * SZ_4K / sizeof(gen6_pte_t));
> ppgtt->base.vm.top = 1;
>
> - ppgtt->base.vm.bind_async_flags = I915_VMA_LOCAL_BIND;
> ppgtt->base.vm.allocate_va_range = gen6_alloc_va_range;
> ppgtt->base.vm.clear_range = gen6_ppgtt_clear_range;
> ppgtt->base.vm.insert_entries = gen6_ppgtt_insert_entries;
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> index 176c19633412..92f8a23e66cc 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> @@ -736,7 +736,6 @@ struct i915_ppgtt *gen8_ppgtt_create(struct intel_gt *gt)
> goto err_free_pd;
> }
>
> - ppgtt->vm.bind_async_flags = I915_VMA_LOCAL_BIND;
> ppgtt->vm.insert_entries = gen8_ppgtt_insert;
> ppgtt->vm.allocate_va_range = gen8_ppgtt_alloc;
> ppgtt->vm.clear_range = gen8_ppgtt_clear;
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index c2fc49495029..2a36b09a3761 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -127,7 +127,6 @@ void i915_ggtt_suspend(struct i915_ggtt *ggtt)
>
> list_for_each_entry_safe(vma, vn, >vm.bound_list, vm_link) {
> GEM_BUG_ON(!drm_mm_node_allocated(>node));
> - i915_vma_wait_for_bind(vma);
>
> if (i915_vma_is_pinned(vma))
> continue;
> @@ -671,7 +670,6 @@ static int init_aliasing_ppgtt(struct i915_ggtt *ggtt)
> ppgtt->vm.allocate_va_range(>vm, , 0, ggtt->vm.total);
>
> ggtt->alias = ppgtt;
> - ggtt->vm.bind_async_flags |= ppgtt->vm.bind_async_flags;
>
> GEM_BUG_ON(ggtt->vm.vma_ops.bind_vma != ggtt_bind_vma);
> ggtt->vm.vma_ops.bind_vma = aliasing_gtt_bind_vma;
> @@ -911,8 +909,6 @@ static int gen8_gmch_probe(struct i915_ggtt *ggtt)
> IS_CHERRYVIEW(i915) /* fails with concurrent use/update */) {
> ggtt->vm.insert_entries = bxt_vtd_ggtt_insert_entries__BKL;
> ggtt->vm.insert_page= bxt_vtd_ggtt_insert_page__BKL;
> - ggtt->vm.bind_async_flags =
> - I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND;
> }
>
> ggtt->invalidate = gen8_ggtt_invalidate;
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h
> b/drivers/gpu/drm/i915/gt/intel_gtt.h
> index e67e34e17913..d9d2ca8b4b61 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> @@ -230,8 +230,6 @@
Re: [Intel-gfx] [PATCH] drm/i915: Implement SINGLE_TIMELINE with a syncobj (v2)
On Wed, Mar 24, 2021 at 6:36 AM Tvrtko Ursulin wrote: > > > On 24/03/2021 09:52, Daniel Vetter wrote: > > On Wed, Mar 24, 2021 at 09:28:58AM +, Tvrtko Ursulin wrote: > >> > >> On 23/03/2021 17:51, Jason Ekstrand wrote: > >>> This API is entirely unnecessary and I'd love to get rid of it. If > >>> userspace wants a single timeline across multiple contexts, they can > >>> either use implicit synchronization or a syncobj, both of which existed > >>> at the time this feature landed. The justification given at the time > >>> was that it would help GL drivers which are inherently single-timeline. > >>> However, neither of our GL drivers actually wanted the feature. i965 > >>> was already in maintenance mode at the time and iris uses syncobj for > >>> everything. > >>> > >>> Unfortunately, as much as I'd love to get rid of it, it is used by the > >>> media driver so we can't do that. We can, however, do the next-best > >>> thing which is to embed a syncobj in the context and do exactly what > >>> we'd expect from userspace internally. This isn't an entirely identical > >>> implementation because it's no longer atomic if userspace races with > >>> itself by calling execbuffer2 twice simultaneously from different > >>> threads. It won't crash in that case; it just doesn't guarantee any > >>> ordering between those two submits. > >>> > >>> Moving SINGLE_TIMELINE to a syncobj emulation has a couple of technical > >>> advantages beyond mere annoyance. One is that intel_timeline is no > >>> longer an api-visible object and can remain entirely an implementation > >>> detail. This may be advantageous as we make scheduler changes going > >>> forward. Second is that, together with deleting the CLONE_CONTEXT API, > >>> we should now have a 1:1 mapping between intel_context and > >>> intel_timeline which may help us reduce locking. > >> > >> Much, much better commit message although I still fail to understand where > >> do you see implementation details leaking out. So for me this is still > >> something I'd like to get to the bottom of. I didn't say "leaking". I said it's no longer API-visible. That's just true. It used to be a kernel object that userspace was unaware of, then we added SINGLE_TIMELINE and userspace now has some influence over the object. With this patch, it's hidden again. I don't get why that's confusing. > >> I would also mention the difference regarding fence context change. There are no fence context changes. The fence that we stuff in the syncobj is an i915 fence and the fence we pull back out is an i915 fence. A syncobj is just a fancy wrapper for a dma_buf pointer. > >> And in general I would maintain this patch as part of a series which ends > >> up > >> demonstrating the "mays" and "shoulds". > > > > I disagree. The past few years we've merged way too much patches and > > features without carefully answering the high level questions of > > - do we really need to solve this problem > > - and if so, are we really solving this problem in the right place > > > > Now we're quite in a hole, and we're not going to get out of this hole if > > we keep applying the same standards that got us here. Anything that does > > not clearly and without reservation the above two questions with "yes" > > needs to be removed or walled off, just so we can eventually see which > > complexity we really need, and what is actually superflous. > > I understand your general point but when I apply it to this specific > patch, even if it is simple, it is neither removing the uapi or walling > it off. So I see it as the usual review standard to ask to see what > "mays" and "shoulds" actually get us in concrete terms. Instead of focusing on the term "leak", let's focus on this line of the commit message instead: > Second is that, together with deleting the CLONE_CONTEXT API, > we should now have a 1:1 mapping between intel_context and > intel_timeline which may help us reduce locking. Now, I've not written any patches yet which actually reduce the locking. I can try to look into that today. I CC'd Maarten on the first send of this because I was hoping he would have good intuition about this. It may be that this object will always have to require some amount of locking if the scheduler has to touch them in parallel with other stuff. What I can say concretely, however, is that this does reduce the sharing of an internal object even if it doesn't get rid of it completely. The one thing that is shared all over the place with this patch is a syncobj which is explicitly designed for exactly this sort of case and can be used and abused by as many threads as you'd like. That seems like it's going in the right direction. I can further weasel-word the commit message to make the above more prominent if you'd like. > I would be able to understand putting the uapi behind the "if gen > 12 > || is_discrete EINVAL", or whatever, since it is fair game to deprecate > for any new platform or say GuC submission.
Re: [Intel-gfx] [PATCH v9 63/70] drm/i915: Move gt_revoke() slightly
On Wed, Mar 24, 2021 at 07:15:36PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 24, 2021 at 06:00:12PM +0100, Daniel Vetter wrote:
> > On Tue, Mar 23, 2021 at 04:50:52PM +0100, Maarten Lankhorst wrote:
> > > We get a lockdep splat when the reset mutex is held, because it can be
> > > taken from fence_wait. This conflicts with the mmu notifier we have,
> > > because we recurse between reset mutex and mmap lock -> mmu notifier.
> > >
> > > Remove this recursion by calling revoke_mmaps before taking the lock.
> > >
> > > The reset code still needs fixing, as taking mmap locks during reset
> > > is not allowed.
> > >
> > > Signed-off-by: Maarten Lankhorst
> > > Reviewed-by: Thomas Hellström
> > > ---
> > > drivers/gpu/drm/i915/gt/intel_reset.c | 5 +++--
> > > 1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c
> > > b/drivers/gpu/drm/i915/gt/intel_reset.c
> > > index 990cb4adbb9a..447f589750c2 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> > > @@ -970,8 +970,6 @@ static int do_reset(struct intel_gt *gt,
> > > intel_engine_mask_t stalled_mask)
> > > {
> > > int err, i;
> > >
> > > - gt_revoke(gt);
> > > -
> > > err = __intel_gt_reset(gt, ALL_ENGINES);
> > > for (i = 0; err && i < RESET_MAX_RETRIES; i++) {
> > > msleep(10 * (i + 1));
> > > @@ -1026,6 +1024,9 @@ void intel_gt_reset(struct intel_gt *gt,
> > >
> > > might_sleep();
> > > GEM_BUG_ON(!test_bit(I915_RESET_BACKOFF, >reset.flags));
> > > +
> >
> > I've added a FIXME comment here just so we don't totally forget. This will
> > also blow up again when we wrap the entire reset path into a dma_fence
> > critical section annotation (at least going forward, we can't do that on
> > hw that needs display reset with the current code unfortunately).
> >
> > But I did look at the code which originally added this in
> >
> > commit 2caffbf1176256cc4f8d4e5c3c524fc689cb9876
> > Author: Chris Wilson
> > Date: Fri Feb 8 15:37:03 2019 +
> >
> > drm/i915: Revoke mmaps and prevent access to fence registers across
> > reset
> >
> > and noped right out.
> >
> > I think this complexity needs to go entirely, and instead we just protect
> > the fence register state to make sure that after reset they are all good
> > again:
> > - add a new mutex for low level fence register state
> > - hold that mutex around fence register writes (really just the low level
> > fence writes)
> > - hold it in the reset path when we restore fence registers
> >
> > This means that a global reset also thrashes mmaps, but it's a global
> > reset we're talking about here, everything is thrash anyway. Plus/minus
> > fenced gtt mmaps really doesn't change the tally.
>
> My recollection is that GPU reset doesn't actually clobber the fence
> registers. Though not 100% sure I can trust my brain on this. Also
> dunno if it actually matter here or not, but figured I'd point it out.
I think on gen2/3 it does, because there everything goes over. But yeah
maybe on gen4+ it's all fine, would be worth to check that.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v9 63/70] drm/i915: Move gt_revoke() slightly
On Wed, Mar 24, 2021 at 06:00:12PM +0100, Daniel Vetter wrote:
> On Tue, Mar 23, 2021 at 04:50:52PM +0100, Maarten Lankhorst wrote:
> > We get a lockdep splat when the reset mutex is held, because it can be
> > taken from fence_wait. This conflicts with the mmu notifier we have,
> > because we recurse between reset mutex and mmap lock -> mmu notifier.
> >
> > Remove this recursion by calling revoke_mmaps before taking the lock.
> >
> > The reset code still needs fixing, as taking mmap locks during reset
> > is not allowed.
> >
> > Signed-off-by: Maarten Lankhorst
> > Reviewed-by: Thomas Hellström
> > ---
> > drivers/gpu/drm/i915/gt/intel_reset.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c
> > b/drivers/gpu/drm/i915/gt/intel_reset.c
> > index 990cb4adbb9a..447f589750c2 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> > @@ -970,8 +970,6 @@ static int do_reset(struct intel_gt *gt,
> > intel_engine_mask_t stalled_mask)
> > {
> > int err, i;
> >
> > - gt_revoke(gt);
> > -
> > err = __intel_gt_reset(gt, ALL_ENGINES);
> > for (i = 0; err && i < RESET_MAX_RETRIES; i++) {
> > msleep(10 * (i + 1));
> > @@ -1026,6 +1024,9 @@ void intel_gt_reset(struct intel_gt *gt,
> >
> > might_sleep();
> > GEM_BUG_ON(!test_bit(I915_RESET_BACKOFF, >reset.flags));
> > +
>
> I've added a FIXME comment here just so we don't totally forget. This will
> also blow up again when we wrap the entire reset path into a dma_fence
> critical section annotation (at least going forward, we can't do that on
> hw that needs display reset with the current code unfortunately).
>
> But I did look at the code which originally added this in
>
> commit 2caffbf1176256cc4f8d4e5c3c524fc689cb9876
> Author: Chris Wilson
> Date: Fri Feb 8 15:37:03 2019 +
>
> drm/i915: Revoke mmaps and prevent access to fence registers across reset
>
> and noped right out.
>
> I think this complexity needs to go entirely, and instead we just protect
> the fence register state to make sure that after reset they are all good
> again:
> - add a new mutex for low level fence register state
> - hold that mutex around fence register writes (really just the low level
> fence writes)
> - hold it in the reset path when we restore fence registers
>
> This means that a global reset also thrashes mmaps, but it's a global
> reset we're talking about here, everything is thrash anyway. Plus/minus
> fenced gtt mmaps really doesn't change the tally.
My recollection is that GPU reset doesn't actually clobber the fence
registers. Though not 100% sure I can trust my brain on this. Also
dunno if it actually matter here or not, but figured I'd point it out.
--
Ville Syrjälä
Intel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v9 65/70] drm/i915: Fix pin_map in scheduler selftests
On Tue, Mar 23, 2021 at 04:50:54PM +0100, Maarten Lankhorst wrote:
> Signed-off-by: Maarten Lankhorst
Fixes a commit I'll drop anyway, so didn't bother with this one.
-Daniel
> ---
> drivers/gpu/drm/i915/selftests/i915_scheduler.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/selftests/i915_scheduler.c
> b/drivers/gpu/drm/i915/selftests/i915_scheduler.c
> index f54bdbeaa48b..4c306e40c416 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_scheduler.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_scheduler.c
> @@ -645,7 +645,7 @@ static int __igt_schedule_cycle(struct drm_i915_private
> *i915,
> if (IS_ERR(obj))
> return PTR_ERR(obj);
>
> - time = i915_gem_object_pin_map(obj, I915_MAP_WC);
> + time = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WC);
> if (IS_ERR(time)) {
> err = PTR_ERR(time);
> goto out_obj;
> --
> 2.31.0
>
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v9 64/70] drm/i915: Add missing -EDEADLK path in execbuffer ggtt pinning.
On Tue, Mar 23, 2021 at 04:50:53PM +0100, Maarten Lankhorst wrote:
> In reloc_iomap we swallow the -EDEADLK error, but this needs to
> be returned for -EDEADLK handling. Add the missing check to
> make bsw pass again.
>
> Testcase: gem_exec_fence.basic-await
>
> Signed-off-by: Maarten Lankhorst
This partially reverts
https://lore.kernel.org/intel-gfx/cam0jshohkzhivgi-x37w2rxyqhm1vbqb8uzavyexejuwe0l...@mail.gmail.com/
which I'm going to throw out anyway. So not merged this one here.
-Daniel
> ---
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 37fecd295eb6..f85ca10bf7f3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -1216,6 +1216,8 @@ static void *reloc_iomap(struct drm_i915_gem_object
> *obj,
> PIN_MAPPABLE |
> PIN_NONBLOCK /*
> NOWARN */ |
> PIN_NOEVICT);
> + if (vma == ERR_PTR(-EDEADLK))
> + return vma;
> if (IS_ERR(vma)) {
> memset(>node, 0, sizeof(cache->node));
> mutex_lock(>vm.mutex);
> --
> 2.31.0
>
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v9 63/70] drm/i915: Move gt_revoke() slightly
On Tue, Mar 23, 2021 at 04:50:52PM +0100, Maarten Lankhorst wrote:
> We get a lockdep splat when the reset mutex is held, because it can be
> taken from fence_wait. This conflicts with the mmu notifier we have,
> because we recurse between reset mutex and mmap lock -> mmu notifier.
>
> Remove this recursion by calling revoke_mmaps before taking the lock.
>
> The reset code still needs fixing, as taking mmap locks during reset
> is not allowed.
>
> Signed-off-by: Maarten Lankhorst
> Reviewed-by: Thomas Hellström
> ---
> drivers/gpu/drm/i915/gt/intel_reset.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c
> b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 990cb4adbb9a..447f589750c2 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -970,8 +970,6 @@ static int do_reset(struct intel_gt *gt,
> intel_engine_mask_t stalled_mask)
> {
> int err, i;
>
> - gt_revoke(gt);
> -
> err = __intel_gt_reset(gt, ALL_ENGINES);
> for (i = 0; err && i < RESET_MAX_RETRIES; i++) {
> msleep(10 * (i + 1));
> @@ -1026,6 +1024,9 @@ void intel_gt_reset(struct intel_gt *gt,
>
> might_sleep();
> GEM_BUG_ON(!test_bit(I915_RESET_BACKOFF, >reset.flags));
> +
I've added a FIXME comment here just so we don't totally forget. This will
also blow up again when we wrap the entire reset path into a dma_fence
critical section annotation (at least going forward, we can't do that on
hw that needs display reset with the current code unfortunately).
But I did look at the code which originally added this in
commit 2caffbf1176256cc4f8d4e5c3c524fc689cb9876
Author: Chris Wilson
Date: Fri Feb 8 15:37:03 2019 +
drm/i915: Revoke mmaps and prevent access to fence registers across reset
and noped right out.
I think this complexity needs to go entirely, and instead we just protect
the fence register state to make sure that after reset they are all good
again:
- add a new mutex for low level fence register state
- hold that mutex around fence register writes (really just the low level
fence writes)
- hold it in the reset path when we restore fence registers
This means that a global reset also thrashes mmaps, but it's a global
reset we're talking about here, everything is thrash anyway. Plus/minus
fenced gtt mmaps really doesn't change the tally.
The real solution is per-engine reset here, and making sure that works as
well as absolutely possible.
Maarten, can you pls take care of this in a follow-up? We have to do this
anyway when we roll out more dma_fence annotations.
Thanks, Daniel
> + gt_revoke(gt);
> +
> mutex_lock(>reset.mutex);
>
> /* Clear any previous failed attempts at recovery. Time to try again. */
> --
> 2.31.0
>
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
> On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
[]
> > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> > []
> > > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> > > drm_encoder *encoder)
> > > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
> > >
> > > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > + __func__, ERR_PTR(mux));
> >
> > This does not compile without warnings.
> >
> > drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> > drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects argument
> > of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> > 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > | ^~
> >
> > If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> > is converting an int a void * to decode the error type and
> > emit it as a string.
>
> Sorry about that.
>
> I decided against using ERR_PTR() in order to also check for
> positive array overflow, but the version I tested was different from
> the version I sent.
>
> v3 coming.
Thanks. No worries.
Up to you, vsprintf would emit the positive mux as a funky hashed
hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
perhaps %d without the ERR_PTR use makes the most sense.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 6/9] drm/tegra: gem: Add a clarifying comment
24.03.2021 19:42, Thierry Reding пишет: > On Wed, Mar 24, 2021 at 06:45:30PM +0300, Dmitry Osipenko wrote: >> 24.03.2021 18:02, Thierry Reding пишет: >>> On Wed, Mar 24, 2021 at 05:41:08PM +0300, Dmitry Osipenko wrote: 23.03.2021 18:54, Thierry Reding пишет: > From: Thierry Reding > > Clarify when a fixed IOV address can be used and when a buffer has to > be mapped before the IOVA can be used. > > Signed-off-by: Thierry Reding > --- > drivers/gpu/drm/tegra/plane.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/tegra/plane.c b/drivers/gpu/drm/tegra/plane.c > index 19e8847a164b..793da5d675d2 100644 > --- a/drivers/gpu/drm/tegra/plane.c > +++ b/drivers/gpu/drm/tegra/plane.c > @@ -119,6 +119,14 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct > tegra_plane_state *state) > dma_addr_t phys_addr, *phys; > struct sg_table *sgt; > > + /* > + * If we're not attached to a domain, we already stored the > + * physical address when the buffer was allocated. If we're > + * part of a group that's shared between all display > + * controllers, we've also already mapped the framebuffer > + * through the SMMU. In both cases we can short-circuit the > + * code below and retrieve the stored IOV address. > + */ > if (!domain || dc->client.group) > phys = _addr; > else > This comment is correct, but the logic feels a bit lame because it should be wasteful to re-map DMA on each FB flip. Personally I don't care much about this since older Tegras use pinned buffers by default, but this shouldn't be good for T124+ users. >>> >>> I'm not terribly thrilled by this either, but it's the only way to do >>> this when using the DMA API because we don't know at allocation time (or >>> import time for that matter) which of the (up to) 4 display controllers >>> a framebuffer will be shown on. tegra_dc_pin() is the earliest where >>> this is known and worst case that's called once per flip. >>> >>> When the IOMMU API is used explicitly, we always map framebuffers into >>> the IOMMU domain shared by all display controllers at allocation or >>> import time and then we don't need to pin at flip time anymore. >>> >>> I do have a work-in-progress patch somewhere that creates a mapping >>> cache to mitigate this problem to some degree. I need to dig that up and >>> do a few measurements because I vaguely recall this speeding up flips by >>> quite a bit (well, except for the very first mapping, obviously). >>> Perhaps dumb buffers should be pinned to display by default and then we should extend the Tegra UAPI to support BO mapping to display client(?). >>> >>> That would kind of defeat the purpose of a generic KMS UAPI. >> >> Couldn't the BOs be mapped when FB is created, i.e. by tegra_fb_create? > > I suppose that would be possible. However, tegra_fb_create() doesn't > know a thing about display controllers, so we'd have to add extra code > to it to iterate over all display controllers and do a dma_map_sg() of > the GEM object for each of them. > > It's also somewhat wasteful because now we get a mapping for each > framebuffer for each display controller. So if you've got, say, a four > UHD screen setup (which is something that Tegra194 supports), you could > end up with 8 UHD framebuffers (two for each display, for double- > buffering) at 32 MiB each for a whopping 256 MiB of memory that needs to > be mapped for each of the four display controllers. That 1 GiB worth of > page table updates, whereas you really only need one fourth of that. > > Granted, this will make flipping a bit faster, and IOVA space isn't > really a problem on Tegra194. It would still waste a bit of RAM for all > those page table entries that we don't really need, though. > > A mapping cache seems like a much better compromise because the cache > lookup should be quite fast compared to a mapping operation and we waste > just a couple dozen bytes per mapping perhaps as opposed to a few > megabytes for the gratuitous, preemptive mappings. Isn't it really possible to put displays into the same IOMMU group on T194? It doesn't make much sense to have them in a separate groups on Linux. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] [v3] drm/imx: imx-ldb: fix out of bounds array access warning
From: Arnd Bergmann
When CONFIG_OF is disabled, building with 'make W=1' produces warnings
about out of bounds array access:
drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_set_clock.constprop':
drivers/gpu/drm/imx/imx-ldb.c:186:8: error: array subscript -22 is below array
bounds of 'struct clk *[4]' [-Werror=array-bounds]
Add an error check before the index is used, which helps with the
warning, as well as any possible other error condition that may be
triggered at runtime.
The warning could be fixed by adding a Kconfig depedency on CONFIG_OF,
but Liu Ying points out that the driver may hit the out-of-bounds
problem at runtime anyway.
Signed-off-by: Arnd Bergmann
---
v3: fix build regression from v2
v2: fix subject line
expand patch description
print mux number
check upper bound as well
---
drivers/gpu/drm/imx/imx-ldb.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index dbfe39e2f7f6..565482e2b816 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -197,6 +197,11 @@ static void imx_ldb_encoder_enable(struct drm_encoder
*encoder)
int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
+ if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
+ dev_warn(ldb->dev, "%s: invalid mux %d\n", __func__, mux);
+ return;
+ }
+
drm_panel_prepare(imx_ldb_ch->panel);
if (dual) {
@@ -255,6 +260,11 @@ imx_ldb_encoder_atomic_mode_set(struct drm_encoder
*encoder,
int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
u32 bus_format = imx_ldb_ch->bus_format;
+ if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
+ dev_warn(ldb->dev, "%s: invalid mux %d\n", __func__, mux);
+ return;
+ }
+
if (mode->clock > 17) {
dev_warn(ldb->dev,
"%s: mode exceeds 170 MHz pixel clock\n", __func__);
--
2.29.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
>
> On Wed, 2021-03-24 at 13:17 +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann
> >
> > When CONFIG_OF is disabled, building with 'make W=1' produces warnings
> > about out of bounds array access:
> >
> > drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_set_clock.constprop':
> > drivers/gpu/drm/imx/imx-ldb.c:186:8: error: array subscript -22 is below
> > array bounds of 'struct clk *[4]' [-Werror=array-bounds]
> >
> > Add an error check before the index is used, which helps with the
> > warning, as well as any possible other error condition that may be
> > triggered at runtime.
> >
> > The warning could be fixed by adding a Kconfig depedency on CONFIG_OF,
> > but Liu Ying points out that the driver may hit the out-of-bounds
> > problem at runtime anyway.
> >
> > Signed-off-by: Arnd Bergmann
> > ---
> > v2: fix subject line
> > expand patch description
> > print mux number
> > check upper bound as well
> []
> > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> []
> > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct drm_encoder
> > *encoder)
> > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
> >
> > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > + __func__, ERR_PTR(mux));
>
> This does not compile without warnings.
>
> drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects argument
> of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> | ^~
>
> If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> is converting an int a void * to decode the error type and
> emit it as a string.
Sorry about that.
I decided against using ERR_PTR() in order to also check for
positive array overflow, but the version I tested was different from
the version I sent.
v3 coming.
Arnd
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 6/9] drm/tegra: gem: Add a clarifying comment
On Wed, Mar 24, 2021 at 06:45:30PM +0300, Dmitry Osipenko wrote: > 24.03.2021 18:02, Thierry Reding пишет: > > On Wed, Mar 24, 2021 at 05:41:08PM +0300, Dmitry Osipenko wrote: > >> 23.03.2021 18:54, Thierry Reding пишет: > >>> From: Thierry Reding > >>> > >>> Clarify when a fixed IOV address can be used and when a buffer has to > >>> be mapped before the IOVA can be used. > >>> > >>> Signed-off-by: Thierry Reding > >>> --- > >>> drivers/gpu/drm/tegra/plane.c | 8 > >>> 1 file changed, 8 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/tegra/plane.c b/drivers/gpu/drm/tegra/plane.c > >>> index 19e8847a164b..793da5d675d2 100644 > >>> --- a/drivers/gpu/drm/tegra/plane.c > >>> +++ b/drivers/gpu/drm/tegra/plane.c > >>> @@ -119,6 +119,14 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct > >>> tegra_plane_state *state) > >>> dma_addr_t phys_addr, *phys; > >>> struct sg_table *sgt; > >>> > >>> + /* > >>> + * If we're not attached to a domain, we already stored the > >>> + * physical address when the buffer was allocated. If we're > >>> + * part of a group that's shared between all display > >>> + * controllers, we've also already mapped the framebuffer > >>> + * through the SMMU. In both cases we can short-circuit the > >>> + * code below and retrieve the stored IOV address. > >>> + */ > >>> if (!domain || dc->client.group) > >>> phys = _addr; > >>> else > >>> > >> > >> This comment is correct, but the logic feels a bit lame because it > >> should be wasteful to re-map DMA on each FB flip. Personally I don't > >> care much about this since older Tegras use pinned buffers by default, > >> but this shouldn't be good for T124+ users. > > > > I'm not terribly thrilled by this either, but it's the only way to do > > this when using the DMA API because we don't know at allocation time (or > > import time for that matter) which of the (up to) 4 display controllers > > a framebuffer will be shown on. tegra_dc_pin() is the earliest where > > this is known and worst case that's called once per flip. > > > > When the IOMMU API is used explicitly, we always map framebuffers into > > the IOMMU domain shared by all display controllers at allocation or > > import time and then we don't need to pin at flip time anymore. > > > > I do have a work-in-progress patch somewhere that creates a mapping > > cache to mitigate this problem to some degree. I need to dig that up and > > do a few measurements because I vaguely recall this speeding up flips by > > quite a bit (well, except for the very first mapping, obviously). > > > >> Perhaps dumb buffers should be pinned to display by default and then we > >> should extend the Tegra UAPI to support BO mapping to display client(?). > > > > That would kind of defeat the purpose of a generic KMS UAPI. > > Couldn't the BOs be mapped when FB is created, i.e. by tegra_fb_create? I suppose that would be possible. However, tegra_fb_create() doesn't know a thing about display controllers, so we'd have to add extra code to it to iterate over all display controllers and do a dma_map_sg() of the GEM object for each of them. It's also somewhat wasteful because now we get a mapping for each framebuffer for each display controller. So if you've got, say, a four UHD screen setup (which is something that Tegra194 supports), you could end up with 8 UHD framebuffers (two for each display, for double- buffering) at 32 MiB each for a whopping 256 MiB of memory that needs to be mapped for each of the four display controllers. That 1 GiB worth of page table updates, whereas you really only need one fourth of that. Granted, this will make flipping a bit faster, and IOVA space isn't really a problem on Tegra194. It would still waste a bit of RAM for all those page table entries that we don't really need, though. A mapping cache seems like a much better compromise because the cache lookup should be quite fast compared to a mapping operation and we waste just a couple dozen bytes per mapping perhaps as opposed to a few megabytes for the gratuitous, preemptive mappings. Thierry signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v9 44/70] drm/i915/selftests: Prepare context tests for obj->mm.lock removal.
On Tue, Mar 23, 2021 at 04:50:33PM +0100, Maarten Lankhorst wrote:
> Straightforward conversion, just convert a bunch of calls to
> unlocked versions.
>
> Signed-off-by: Maarten Lankhorst
> Reviewed-by: Thomas Hellström
Another one where I just picked your old version:
https://lore.kernel.org/intel-gfx/20210128162612.927917-45-maarten.lankho...@linux.intel.com/
There was some functional changes in the test, so I figured that's the
safer path.
-Daniel
> ---
> drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> index df949320f2b5..82d5d37e9b66 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> @@ -1092,7 +1092,7 @@ __read_slice_count(struct intel_context *ce,
> if (ret < 0)
> return ret;
>
> - buf = i915_gem_object_pin_map(obj, I915_MAP_WB);
> + buf = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
> if (IS_ERR(buf)) {
> ret = PTR_ERR(buf);
> return ret;
> @@ -1509,7 +1509,7 @@ static int write_to_scratch(struct i915_gem_context
> *ctx,
> if (IS_ERR(obj))
> return PTR_ERR(obj);
>
> - cmd = i915_gem_object_pin_map(obj, I915_MAP_WB);
> + cmd = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
> if (IS_ERR(cmd)) {
> err = PTR_ERR(cmd);
> goto out;
> @@ -1620,7 +1620,7 @@ static int read_from_scratch(struct i915_gem_context
> *ctx,
> if (err)
> goto out_vm;
>
> - cmd = i915_gem_object_pin_map(obj, I915_MAP_WC);
> + cmd = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WC);
> if (IS_ERR(cmd)) {
> err = PTR_ERR(cmd);
> goto out;
> @@ -1656,7 +1656,7 @@ static int read_from_scratch(struct i915_gem_context
> *ctx,
> if (err)
> goto out_vm;
>
> - cmd = i915_gem_object_pin_map(obj, I915_MAP_WC);
> + cmd = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WC);
> if (IS_ERR(cmd)) {
> err = PTR_ERR(cmd);
> goto out;
> @@ -1715,7 +1715,7 @@ static int read_from_scratch(struct i915_gem_context
> *ctx,
> }
> i915_request_put(rq);
>
> - cmd = i915_gem_object_pin_map(obj, I915_MAP_WC);
> + cmd = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WC);
> if (IS_ERR(cmd)) {
> err = PTR_ERR(cmd);
> goto out_vm;
> --
> 2.31.0
>
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/2] mm,drm/ttm: Block fast GUP to TTM huge pages
On Wed, Mar 24, 2021 at 04:50:14PM +0100, Thomas Hellström (Intel) wrote: > > On 3/24/21 2:48 PM, Jason Gunthorpe wrote: > > On Wed, Mar 24, 2021 at 02:35:38PM +0100, Thomas Hellström (Intel) wrote: > > > > > > In an ideal world the creation/destruction of page table levels would > > > > by dynamic at this point, like THP. > > > Hmm, but I'm not sure what problem we're trying to solve by changing the > > > interface in this way? > > We are trying to make a sensible driver API to deal with huge pages. > > > Currently if the core vm requests a huge pud, we give it one, and if we > > > can't or don't want to (because of dirty-tracking, for example, which is > > > always done on 4K page-level) we just return VM_FAULT_FALLBACK, and the > > > fault is retried at a lower level. > > Well, my thought would be to move the pte related stuff into > > vmf_insert_range instead of recursing back via VM_FAULT_FALLBACK. > > > > I don't know if the locking works out, but it feels cleaner that the > > driver tells the vmf how big a page it can stuff in, not the vm > > telling the driver to stuff in a certain size page which it might not > > want to do. > > > > Some devices want to work on a in-between page size like 64k so they > > can't form 2M pages but they can stuff 64k of 4K pages in a batch on > > every fault. > > Hmm, yes, but we would in that case be limited anyway to insert ranges > smaller than and equal to the fault size to avoid extensive and possibly > unnecessary checks for contigous memory. Why? The insert function is walking the page tables, it just updates things as they are. It learns the arragement for free while doing the walk. The device has to always provide consistent data, if it overlaps into pages that are already populated that is fine so long as it isn't changing their addresses. > And then if we can't support the full fault size, we'd need to > either presume a size and alignment of the next level or search for > contigous memory in both directions around the fault address, > perhaps unnecessarily as well. You don't really need to care about levels, the device should be faulting in the largest memory regions it can within its efficiency. If it works on 4M pages then it should be faulting 4M pages. The page size of the underlying CPU doesn't really matter much other than some tuning to impact how the device's allocator works. Jason ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/2] mm,drm/ttm: Block fast GUP to TTM huge pages
On 3/24/21 3:05 AM, Thomas Hellström (Intel) wrote: > Yes, I agree. Seems like the special (SW1) is available also for huge > page table entries on x86 AFAICT, although just not implemented. > Otherwise the SW bits appear completely used up. Although the _PAGE_BIT_SOFTW* bits are used up, there's plenty of room in the hardware PTEs. Bits 52->58 are software-available, and we're only using 58 at the moment. We also have not been careful at *all* about how _PAGE_BIT_SOFTW* are used. It's quite possible we can encode another use even in the existing bits. Personally, I'd just try: #define _PAGE_BIT_SOFTW557 /* available for programmer */ ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v9 34/70] drm/i915: Add ww locking around vm_access()
On Tue, Mar 23, 2021 at 04:50:23PM +0100, Maarten Lankhorst wrote:
> i915_gem_object_pin_map potentially needs a ww context, so ensure we
> have one we can revoke.
>
> Signed-off-by: Maarten Lankhorst
> Reviewed-by: Thomas Hellström
We shouldn't hand-roll our own vm_access callback, generic_access_phys
should be used here instead.
I've applied this, but can you pls do a follow up patch here?
Thanks, Daniel
> ---
> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 24 ++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 163208a6260d..2561a2f1e54f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -421,7 +421,9 @@ vm_access(struct vm_area_struct *area, unsigned long addr,
> {
> struct i915_mmap_offset *mmo = area->vm_private_data;
> struct drm_i915_gem_object *obj = mmo->obj;
> + struct i915_gem_ww_ctx ww;
> void *vaddr;
> + int err = 0;
>
> if (i915_gem_object_is_readonly(obj) && write)
> return -EACCES;
> @@ -430,10 +432,18 @@ vm_access(struct vm_area_struct *area, unsigned long
> addr,
> if (addr >= obj->base.size)
> return -EINVAL;
>
> + i915_gem_ww_ctx_init(, true);
> +retry:
> + err = i915_gem_object_lock(obj, );
> + if (err)
> + goto out;
> +
> /* As this is primarily for debugging, let's focus on simplicity */
> vaddr = i915_gem_object_pin_map(obj, I915_MAP_FORCE_WC);
> - if (IS_ERR(vaddr))
> - return PTR_ERR(vaddr);
> + if (IS_ERR(vaddr)) {
> + err = PTR_ERR(vaddr);
> + goto out;
> + }
>
> if (write) {
> memcpy(vaddr + addr, buf, len);
> @@ -443,6 +453,16 @@ vm_access(struct vm_area_struct *area, unsigned long
> addr,
> }
>
> i915_gem_object_unpin_map(obj);
> +out:
> + if (err == -EDEADLK) {
> + err = i915_gem_ww_ctx_backoff();
> + if (!err)
> + goto retry;
> + }
> + i915_gem_ww_ctx_fini();
> +
> + if (err)
> + return err;
>
> return len;
> }
> --
> 2.31.0
>
> ___
> Intel-gfx mailing list
> intel-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH v9 31/70] drm/i915: Fix workarounds selftest, part 1
On Tue, Mar 23, 2021 at 04:50:20PM +0100, Maarten Lankhorst wrote:
> pin_map needs the ww lock, so ensure we pin both before submission.
>
> Signed-off-by: Maarten Lankhorst
> Reviewed-by: Thomas Hellström
Another one where I picked an older version:
https://lore.kernel.org/intel-gfx/20210128162612.927917-32-maarten.lankho...@linux.intel.com/
Cheers, Daniel
> ---
> drivers/gpu/drm/i915/gem/i915_gem_object.h| 3 +
> drivers/gpu/drm/i915/gem/i915_gem_pages.c | 12 +++
> .../gpu/drm/i915/gt/selftest_workarounds.c| 95 +--
> 3 files changed, 80 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> index 6c3f75adb53c..983f2d4b2a85 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> @@ -437,6 +437,9 @@ void i915_gem_object_writeback(struct drm_i915_gem_object
> *obj);
> void *__must_check i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
> enum i915_map_type type);
>
> +void *__must_check i915_gem_object_pin_map_unlocked(struct
> drm_i915_gem_object *obj,
> + enum i915_map_type type);
> +
> void __i915_gem_object_flush_map(struct drm_i915_gem_object *obj,
>unsigned long offset,
>unsigned long size);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> index e947d4c0da1f..a24617af3c93 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> @@ -400,6 +400,18 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object
> *obj,
> goto out_unlock;
> }
>
> +void *i915_gem_object_pin_map_unlocked(struct drm_i915_gem_object *obj,
> +enum i915_map_type type)
> +{
> + void *ret;
> +
> + i915_gem_object_lock(obj, NULL);
> + ret = i915_gem_object_pin_map(obj, type);
> + i915_gem_object_unlock(obj);
> +
> + return ret;
> +}
> +
> void __i915_gem_object_flush_map(struct drm_i915_gem_object *obj,
>unsigned long offset,
>unsigned long size)
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index de6136bd10ac..a508614b2fd5 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -103,15 +103,13 @@ read_nonprivs(struct intel_context *ce, struct i915_vma
> *result)
> int err;
> int i;
>
> - rq = intel_context_create_request(ce);
> + rq = i915_request_create(ce);
> if (IS_ERR(rq))
> return rq;
>
> - i915_vma_lock(result);
> err = i915_request_await_object(rq, result->obj, true);
> if (err == 0)
> err = i915_vma_move_to_active(result, rq, EXEC_OBJECT_WRITE);
> - i915_vma_unlock(result);
> if (err)
> goto err_rq;
>
> @@ -176,10 +174,11 @@ static int check_whitelist(struct intel_context *ce)
> u32 *vaddr;
> int i;
>
> - result = __vm_create_scratch_for_read(>gt->ggtt->vm, PAGE_SIZE);
> + result = __vm_create_scratch_for_read_pinned(>gt->ggtt->vm,
> PAGE_SIZE);
> if (IS_ERR(result))
> return PTR_ERR(result);
>
> + i915_gem_object_lock(result->obj, NULL);
> vaddr = i915_gem_object_pin_map(result->obj, I915_MAP_WB);
> if (IS_ERR(vaddr)) {
> err = PTR_ERR(vaddr);
> @@ -219,6 +218,8 @@ static int check_whitelist(struct intel_context *ce)
> out_map:
> i915_gem_object_unpin_map(result->obj);
> out_put:
> + i915_vma_unpin(result);
> + i915_gem_object_unlock(result->obj);
> i915_vma_put(result);
> return err;
> }
> @@ -279,10 +280,14 @@ static int check_whitelist_across_reset(struct
> intel_engine_cs *engine,
> if (IS_ERR(ce))
> return PTR_ERR(ce);
>
> - err = igt_spinner_init(, engine->gt);
> + err = intel_context_pin(ce);
> if (err)
> goto out_ctx;
>
> + err = igt_spinner_init(, engine->gt);
> + if (err)
> + goto out_unpin;
> +
> err = check_whitelist(ce);
> if (err) {
> pr_err("Invalid whitelist *before* %s reset!\n", name);
> @@ -315,6 +320,13 @@ static int check_whitelist_across_reset(struct
> intel_engine_cs *engine,
> err = PTR_ERR(tmp);
> goto out_spin;
> }
> + err = intel_context_pin(tmp);
> + if (err) {
> + intel_context_put(tmp);
> + goto out_spin;
> + }
> +
> + intel_context_unpin(ce);
> intel_context_put(ce);
> ce = tmp;
>
> @@ -327,6 +339,8 @@ static int check_whitelist_across_reset(struct
> intel_engine_cs *engine,
>
> out_spin:
>
Re: [PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
Hi Arnd, I love your patch! Perhaps something to improve: [auto build test WARNING on shawnguo/for-next] [also build test WARNING on pza/reset/next drm-intel/for-linux-next drm-tip/drm-tip v5.12-rc4 next-20210324] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Arnd-Bergmann/drm-imx-imx-ldb-fix-out-of-bounds-array-access-warning/20210324-202112 base: https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git for-next config: ia64-randconfig-r021-20210323 (attached as .config) compiler: ia64-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/1921451dcfc3ce8072884c286da96759e18ad102 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Arnd-Bergmann/drm-imx-imx-ldb-fix-out-of-bounds-array-access-warning/20210324-202112 git checkout 1921451dcfc3ce8072884c286da96759e18ad102 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): In file included from arch/ia64/include/asm/pgtable.h:154, from include/linux/pgtable.h:6, from arch/ia64/include/asm/uaccess.h:40, from include/linux/uaccess.h:11, from arch/ia64/include/asm/sections.h:11, from include/linux/interrupt.h:20, from include/linux/trace_recursion.h:5, from include/linux/ftrace.h:10, from include/linux/kprobes.h:29, from include/linux/kgdb.h:19, from include/linux/fb.h:5, from include/drm/drm_crtc.h:31, from include/drm/drm_atomic.h:31, from drivers/gpu/drm/imx/imx-ldb.c:21: arch/ia64/include/asm/mmu_context.h: In function 'reload_context': arch/ia64/include/asm/mmu_context.h:127:41: warning: variable 'old_rr4' set but not used [-Wunused-but-set-variable] 127 | unsigned long rr0, rr1, rr2, rr3, rr4, old_rr4; | ^~~ In file included from include/linux/device.h:15, from include/linux/node.h:18, from include/linux/cpu.h:17, from include/linux/of_device.h:5, from drivers/gpu/drm/imx/imx-ldb.c:13: drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_encoder_enable': >> drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format '%d' expects argument >> of type 'int', but argument 4 has type 'void *' [-Wformat=] 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n", | ^~ include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt' 19 | #define dev_fmt(fmt) fmt | ^~~ drivers/gpu/drm/imx/imx-ldb.c:201:3: note: in expansion of macro 'dev_warn' 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n", | ^~~~ drivers/gpu/drm/imx/imx-ldb.c:201:40: note: format string is defined here 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n", | ~^ || |int | %p In file included from include/linux/device.h:15, from include/linux/node.h:18, from include/linux/cpu.h:17, from include/linux/of_device.h:5, from drivers/gpu/drm/imx/imx-ldb.c:13: drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_encoder_atomic_mode_set': drivers/gpu/drm/imx/imx-ldb.c:265:22: warning: format '%d' expects argument of type 'int', but argument 4 has type 'void *' [-Wformat=] 265 | dev_warn(ldb->dev, "%s: invalid mux %d\n", | ^~ include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt' 19 | #define dev_fmt(fmt) fmt | ^~~ drivers/gpu/drm/imx/imx-ldb.c:265:3: note: in expansion of macro 'dev_warn' 265 | dev_warn(ldb->dev, "%s: invalid mux %d\n", | ^~~~ drivers/gpu/drm/imx/imx-ldb.c:265:40: note:
Re: [RFC PATCH 1/2] mm,drm/ttm: Block fast GUP to TTM huge pages
On 3/24/21 2:48 PM, Jason Gunthorpe wrote: On Wed, Mar 24, 2021 at 02:35:38PM +0100, Thomas Hellström (Intel) wrote: In an ideal world the creation/destruction of page table levels would by dynamic at this point, like THP. Hmm, but I'm not sure what problem we're trying to solve by changing the interface in this way? We are trying to make a sensible driver API to deal with huge pages. Currently if the core vm requests a huge pud, we give it one, and if we can't or don't want to (because of dirty-tracking, for example, which is always done on 4K page-level) we just return VM_FAULT_FALLBACK, and the fault is retried at a lower level. Well, my thought would be to move the pte related stuff into vmf_insert_range instead of recursing back via VM_FAULT_FALLBACK. I don't know if the locking works out, but it feels cleaner that the driver tells the vmf how big a page it can stuff in, not the vm telling the driver to stuff in a certain size page which it might not want to do. Some devices want to work on a in-between page size like 64k so they can't form 2M pages but they can stuff 64k of 4K pages in a batch on every fault. Hmm, yes, but we would in that case be limited anyway to insert ranges smaller than and equal to the fault size to avoid extensive and possibly unnecessary checks for contigous memory. And then if we can't support the full fault size, we'd need to either presume a size and alignment of the next level or search for contigous memory in both directions around the fault address, perhaps unnecessarily as well. I do think the current interface works ok, as we're just acting on what the core vm tells us to do. /Thomas That idea doesn't fit naturally if the VM is driving the size. Jason ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 6/9] drm/tegra: gem: Add a clarifying comment
24.03.2021 18:02, Thierry Reding пишет: > On Wed, Mar 24, 2021 at 05:41:08PM +0300, Dmitry Osipenko wrote: >> 23.03.2021 18:54, Thierry Reding пишет: >>> From: Thierry Reding >>> >>> Clarify when a fixed IOV address can be used and when a buffer has to >>> be mapped before the IOVA can be used. >>> >>> Signed-off-by: Thierry Reding >>> --- >>> drivers/gpu/drm/tegra/plane.c | 8 >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/tegra/plane.c b/drivers/gpu/drm/tegra/plane.c >>> index 19e8847a164b..793da5d675d2 100644 >>> --- a/drivers/gpu/drm/tegra/plane.c >>> +++ b/drivers/gpu/drm/tegra/plane.c >>> @@ -119,6 +119,14 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct >>> tegra_plane_state *state) >>> dma_addr_t phys_addr, *phys; >>> struct sg_table *sgt; >>> >>> + /* >>> +* If we're not attached to a domain, we already stored the >>> +* physical address when the buffer was allocated. If we're >>> +* part of a group that's shared between all display >>> +* controllers, we've also already mapped the framebuffer >>> +* through the SMMU. In both cases we can short-circuit the >>> +* code below and retrieve the stored IOV address. >>> +*/ >>> if (!domain || dc->client.group) >>> phys = _addr; >>> else >>> >> >> This comment is correct, but the logic feels a bit lame because it >> should be wasteful to re-map DMA on each FB flip. Personally I don't >> care much about this since older Tegras use pinned buffers by default, >> but this shouldn't be good for T124+ users. > > I'm not terribly thrilled by this either, but it's the only way to do > this when using the DMA API because we don't know at allocation time (or > import time for that matter) which of the (up to) 4 display controllers > a framebuffer will be shown on. tegra_dc_pin() is the earliest where > this is known and worst case that's called once per flip. > > When the IOMMU API is used explicitly, we always map framebuffers into > the IOMMU domain shared by all display controllers at allocation or > import time and then we don't need to pin at flip time anymore. > > I do have a work-in-progress patch somewhere that creates a mapping > cache to mitigate this problem to some degree. I need to dig that up and > do a few measurements because I vaguely recall this speeding up flips by > quite a bit (well, except for the very first mapping, obviously). > >> Perhaps dumb buffers should be pinned to display by default and then we >> should extend the Tegra UAPI to support BO mapping to display client(?). > > That would kind of defeat the purpose of a generic KMS UAPI. Couldn't the BOs be mapped when FB is created, i.e. by tegra_fb_create? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu: Ensure that the modifier requested is supported by plane.
On Wed, Mar 24, 2021 at 11:25 AM Daniel Stone wrote: > > Hi Mark, > > On Wed, 24 Mar 2021 at 14:58, Mark Yacoub wrote: >> >> So you mean it would make more sense to be more explicit in handling >> DRM_FORMAT_MOD_INVALID as an incoming modifier (which will, just like >> DRM_FORMAT_MOD_LINEAR, will return true in >> dm_plane_format_mod_supported)? > > > That's correct. Not passing any modifiers is the same as explicitly passing > INVALID, both of which mean 'the driver will figure it out somehow'; that > driver-specific determination is not the same as explicit LINEAR. > > (I cannot regret enough that INVALID is not 0.) I feel you. When I tested it on a board that doesn't support modifiers, the modifier value was Zero. when I checked it, it was basically LINEAR. I'll amend my changes to explicitly handle INVALID. > > Cheers, > Daniel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 10/11] drm/i915: avoid stringop-overread warning on pri_latency
On Mon, 22 Mar 2021, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> gcc-11 warns about what appears to be an out-of-range array access:
>
> In function ‘snb_wm_latency_quirk’,
> inlined from ‘ilk_setup_wm_latency’ at
> drivers/gpu/drm/i915/intel_pm.c:3108:3:
> drivers/gpu/drm/i915/intel_pm.c:3057:9: error: ‘intel_print_wm_latency’
> reading 16 bytes from a region of size 10 [-Werror=stringop-overread]
> 3057 | intel_print_wm_latency(dev_priv, "Primary",
> dev_priv->wm.pri_latency);
> |
> ^
> drivers/gpu/drm/i915/intel_pm.c: In function ‘ilk_setup_wm_latency’:
> drivers/gpu/drm/i915/intel_pm.c:3057:9: note: referencing argument 3 of type
> ‘const u16 *’ {aka ‘const short unsigned int *’}
> drivers/gpu/drm/i915/intel_pm.c:2994:13: note: in a call to function
> ‘intel_print_wm_latency’
> 2994 | static void intel_print_wm_latency(struct drm_i915_private *dev_priv,
> | ^~
>
> My guess is that this code is actually safe because the size of the
> array depends on the hardware generation, and the function checks for
> that, but at the same time I would not expect the compiler to work it
> out correctly, and the code seems a little fragile with regards to
> future changes. Simply increasing the size of the array should help.
Agreed, I don't think there's an issue, but the code could use a bunch
of improvements.
Like, we have intel_print_wm_latency() for debug logging and
wm_latency_show() for debugfs, and there's a bunch of duplication and
ugh.
But this seems like the easiest fix for the warning.
Reviewed-by: Jani Nikula
> Signed-off-by: Arnd Bergmann
> ---
> drivers/gpu/drm/i915/i915_drv.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 26d69d06aa6d..3567602e0a35 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1095,11 +1095,11 @@ struct drm_i915_private {
>* in 0.5us units for WM1+.
>*/
> /* primary */
> - u16 pri_latency[5];
> + u16 pri_latency[8];
> /* sprite */
> - u16 spr_latency[5];
> + u16 spr_latency[8];
> /* cursor */
> - u16 cur_latency[5];
> + u16 cur_latency[8];
> /*
>* Raw watermark memory latency values
>* for SKL for all 8 levels
--
Jani Nikula, Intel Open Source Graphics Center
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/7] drm/i915: Restrict sentinel requests further
On Wed, 24 Mar 2021 at 12:14, Tvrtko Ursulin wrote: > > From: Tvrtko Ursulin > > Disallow sentinel requests follow previous sentinels to make request > cancellation work better when faced with a chain of requests which have > all been marked as in error. > > Because in cases where we end up with a stream of cancelled requests we > want to turn of request coalescing so they each will get individually turn off > skipped by the execlists_schedule_in (which is called per ELSP port, not > per request). > > Signed-off-by: Tvrtko Ursulin Reviewed-by: Matthew Auld ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu: Ensure that the modifier requested is supported by plane.
Hi Mark, On Wed, 24 Mar 2021 at 14:58, Mark Yacoub wrote: > So you mean it would make more sense to be more explicit in handling > DRM_FORMAT_MOD_INVALID as an incoming modifier (which will, just like > DRM_FORMAT_MOD_LINEAR, will return true in > dm_plane_format_mod_supported)? > That's correct. Not passing any modifiers is the same as explicitly passing INVALID, both of which mean 'the driver will figure it out somehow'; that driver-specific determination is not the same as explicit LINEAR. (I cannot regret enough that INVALID is not 0.) Cheers, Daniel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH 2/7] drm/i915: Individual request cancellation
On Wed, 24 Mar 2021 at 12:13, Tvrtko Ursulin wrote: > > From: Chris Wilson > > Currently, we cancel outstanding requests within a context when the > context is closed. We may also want to cancel individual requests using > the same graceful preemption mechanism. > > v2 (Tvrtko): > * Cancel waiters carefully considering no timeline lock and RCU. > * Fixed selftests. > > v3 (Tvrtko): > * Remove error propagation to waiters for now. > > v4 (Tvrtko): > * Rebase for extracted i915_request_active_engine. (Matt) > > Signed-off-by: Chris Wilson > Signed-off-by: Tvrtko Ursulin > Reviewed-by: Matthew Auld # v3 Reviewed-by: Matthew Auld ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
