Re: Split render/display SoCs, Mesa's renderonly, and Wayland dmabuf hints
On Tue, Apr 20, 2021 at 3:18 AM Daniel Stone wrote: > > Hi, > > On Mon, 19 Apr 2021 at 13:06, Simon Ser wrote: >> >> I'm working on a Wayland extension [1] that, among other things, allows >> compositors to advertise the preferred device to be used by Wayland >> clients. >> >> In general, compositors will send a render node. However, in the case >> of split render/display SoCs, things get a little bit complicated. >> >> [...] > > > Thanks for the write-up Simon! > >> >> There are a few solutions: >> >> 1. Require compositors to discover the render device by trying to import >>a buffer. For each available render device, the compositor would >>allocate a buffer, export it as a DMA-BUF, import it to the >>display-only device, then try to drmModeAddFB. > > > I don't think this is actually tractable? Assuming that 'allocate a buffer' > means 'obtain a gbm_device for the render node directly and allocate a gbm_bo > from it', even with compatible formats and modifiers this will fail for more > restrictive display hardware. imx-drm and pl111 (combined with vc4 on some > Raspberry Pis) will fail this, since they'll take different allocation paths > when they're bound through kmsro vs. directly, accounting for things like > contiguous allocation. So we'd get false negatives on at least some platforms. > >> >> 2. Allow compositors to query the render device magically opened by >>kmsro. This could be done either via EGL_EXT_device_drm, or via a >>new EGL extension. > > > This would be my strong preference, and I don't entirely understand anholt's > pushback here. The way I see it, GBM is about allocation for scanout, and EGL > is about rendering. If, on a split GPU/display system, we create a gbm_device > from a KMS display-only device node, then creating an EGLDisplay from that > magically binds us to a completely different DRM GPU node, and anything using > that EGLDisplay will use that GPU device to render. > > Being able to discover the GPU device node through the device query is really > useful, because it tells us exactly what implicit magic EGL did under the > hood, and about the device that EGL will use. Being able to discover the > display node is much less useful; it does tell us how GBM will allocate > buffers, but the user already knows which device is in use because they > supplied it to GBM. I see the display node as a property of GBM, and the GPU > node as a property of EGL, even if EGL does do (*waves hands*) stuff under > the hood to ensure the two are compatible. I guess if we're assuming that the caller definitely knows about the display device and is asking EGL for the render node in order to do smarter buffer sharing between display and render, I can see it. My objection was that getting the render node in that discussion was apparently some workaround for other brokenness, and was going to result in software that didn't work on pl111 and vc4 displays because it was trying to dodge kmsro. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] iommu/arm-smmu-qcom: Skip the TTBR1 quirk for db820c.
On Mon, Mar 29, 2021 at 7:47 AM Will Deacon wrote: > > On Fri, Mar 26, 2021 at 04:13:02PM -0700, Eric Anholt wrote: > > db820c wants to use the qcom smmu path to get HUPCF set (which keeps > > the GPU from wedging and then sometimes wedging the kernel after a > > page fault), but it doesn't have separate pagetables support yet in > > drm/msm so we can't go all the way to the TTBR1 path. > > What do you mean by "doesn't have separate pagetables support yet"? The > compatible string doesn't feel like the right way to determine this. In my past experience with DT, software looking at the (existing) board-specific compatibles has been a typical mechanism used to resolve something like this "ok, but you need to actually get down to what board is involved here to figure out how to play along with the rest of Linux that later attaches to other DT nodes". Do you have a preferred mechanism here? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] arm64: dts: msm8996: Mark the GPU's SMMU as an adreno one.
This enables the adreno-specific SMMU path that sets HUPCF so
(user-managed) page faults don't wedge the GPU.
Signed-off-by: Eric Anholt
---
We've been seeing a flaky test per day or so in Mesa CI where the
kernel gets wedged after an iommu fault turns into CP errors. With
this patch, the CI isn't throwing the string of CP errors on the
faults in any of the ~10 jobs I've run so far.
arch/arm64/boot/dts/qcom/msm8996.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi
b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 6de136e3add9..432b87ec9c5e 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -1127,7 +1127,7 @@ cci_i2c1: i2c-bus@1 {
};
adreno_smmu: iommu@b4 {
- compatible = "qcom,msm8996-smmu-v2", "qcom,smmu-v2";
+ compatible = "qcom,msm8996-smmu-v2",
"qcom,adreno-smmu", "qcom,smmu-v2";
reg = <0x00b4 0x1>;
#global-interrupts = <1>;
--
2.31.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] iommu/arm-smmu-qcom: Skip the TTBR1 quirk for db820c.
db820c wants to use the qcom smmu path to get HUPCF set (which keeps
the GPU from wedging and then sometimes wedging the kernel after a
page fault), but it doesn't have separate pagetables support yet in
drm/msm so we can't go all the way to the TTBR1 path.
Signed-off-by: Eric Anholt
---
We've been seeing a flaky test per day or so in Mesa CI where the
kernel gets wedged after an iommu fault turns into CP errors. With
this patch, the CI isn't throwing the string of CP errors on the
faults in any of the ~10 jobs I've run so far.
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index bcda17012aee..51f22193e456 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -130,6 +130,16 @@ static int qcom_adreno_smmu_alloc_context_bank(struct
arm_smmu_domain *smmu_doma
return __arm_smmu_alloc_bitmap(smmu->context_map, start, count);
}
+static bool qcom_adreno_can_do_ttbr1(struct arm_smmu_device *smmu)
+{
+ const struct device_node *np = smmu->dev->of_node;
+
+ if (of_device_is_compatible(np, "qcom,msm8996-smmu-v2"))
+ return false;
+
+ return true;
+}
+
static int qcom_adreno_smmu_init_context(struct arm_smmu_domain *smmu_domain,
struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
{
@@ -144,7 +154,8 @@ static int qcom_adreno_smmu_init_context(struct
arm_smmu_domain *smmu_domain,
* be AARCH64 stage 1 but double check because the arm-smmu code assumes
* that is the case when the TTBR1 quirk is enabled
*/
- if ((smmu_domain->stage == ARM_SMMU_DOMAIN_S1) &&
+ if (qcom_adreno_can_do_ttbr1(smmu_domain->smmu) &&
+ (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) &&
(smmu_domain->cfg.fmt == ARM_SMMU_CTX_FMT_AARCH64))
pgtbl_cfg->quirks |= IO_PGTABLE_QUIRK_ARM_TTBR1;
--
2.31.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/msm: fix a6xx_gmu_clear_oob
On Tue, Feb 9, 2021 at 5:24 PM Jordan Crouse wrote:
>
> On Mon, Feb 08, 2021 at 01:55:54PM -0500, Jonathan Marek wrote:
> > The cleanup patch broke a6xx_gmu_clear_oob, fix it by adding the missing
> > bitshift operation.
> >
> > Fixes: 555c50a4a19b ("drm/msm: Clean up GMU OOB set/clear handling")
> > Signed-off-by: Jonathan Marek
>
> Thanks. I feel silly that I missed that.
>
> Reviewed-by: Jordan Crouse
Yeah, oops.
Reviewed-by: Eric Anholt
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/3] drm/v3d: Add job timeout module param
On Thu, Feb 4, 2021 at 10:09 AM Chema Casanova wrote: > > On 3/9/20 18:48, Yukimasa Sugizaki wrote: > > From: Yukimasa Sugizaki > > > > The default timeout is 500 ms which is too short for some workloads > > including Piglit. Adding this parameter will help users to run heavier > > tasks. > > > > Signed-off-by: Yukimasa Sugizaki > > --- > > drivers/gpu/drm/v3d/v3d_sched.c | 24 +--- > > 1 file changed, 13 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/v3d/v3d_sched.c > > b/drivers/gpu/drm/v3d/v3d_sched.c > > index feef0c749e68..983efb018560 100644 > > --- a/drivers/gpu/drm/v3d/v3d_sched.c > > +++ b/drivers/gpu/drm/v3d/v3d_sched.c > > @@ -19,11 +19,17 @@ > >*/ > > > > #include > > +#include > > > > #include "v3d_drv.h" > > #include "v3d_regs.h" > > #include "v3d_trace.h" > > > > +static uint timeout = 500; > > +module_param(timeout, uint, 0444); > > +MODULE_PARM_DESC(timeout, > > + "Timeout for a job in ms (0 means infinity and default is 500 ms)"); > > + > > I think that parameter definition could be included at v3d_drv.c as > other drivers do. Then we would have all future parameters together. In > that case we would need also to include the extern definition at > v3d_drv.h for the driver variable. > > The param name could be more descriptive like "sched_timeout_ms" in the > lima driver. > > I wonder if it would make sense to have different timeouts parameters > for different type of jobs we have. At least this series come from the > need additional time to complete compute jobs. This is what amdgpu does, > but we probably don't need something such complex. > > I think it would also make sense to increase our default value for the > compute jobs. > > What do you think? > > In any case I think that having this general scheduling timeout > parameter as other drivers do is a good idea. Having a param for being able to test if the job completes eventually is a good idea, but if tests are triggering timeouts, then you probably need to investigate what's going on in the driver -- it's not like you want .5second unpreemptible jobs to be easy to produce. Also, for CS, I wonder if we have another reg besides CSD_CURRENT_CFG4 we could be looking at for "we're making progress on the job". Half a second with no discernible progress sounds like a bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 3/3] drm/msm: Clean up GMU OOB set/clear handling.
Now that the bug is fixed in the minimal way for stable, go make the
code table-driven.
Signed-off-by: Eric Anholt
Reviewed-by: Jordan Crouse
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 124 +-
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 55
2 files changed, 77 insertions(+), 102 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index b3318f86aabc..9066e98eb8ef 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -245,47 +245,66 @@ static int a6xx_gmu_hfi_start(struct a6xx_gmu *gmu)
return ret;
}
+struct a6xx_gmu_oob_bits {
+ int set, ack, set_new, ack_new;
+ const char *name;
+};
+
+/* These are the interrupt / ack bits for each OOB request that are set
+ * in a6xx_gmu_set_oob and a6xx_clear_oob
+ */
+static const struct a6xx_gmu_oob_bits a6xx_gmu_oob_bits[] = {
+ [GMU_OOB_GPU_SET] = {
+ .name = "GPU_SET",
+ .set = 16,
+ .ack = 24,
+ .set_new = 30,
+ .ack_new = 31,
+ },
+
+ [GMU_OOB_PERFCOUNTER_SET] = {
+ .name = "PERFCOUNTER",
+ .set = 17,
+ .ack = 25,
+ .set_new = 28,
+ .ack_new = 30,
+ },
+
+ [GMU_OOB_BOOT_SLUMBER] = {
+ .name = "BOOT_SLUMBER",
+ .set = 22,
+ .ack = 30,
+ },
+
+ [GMU_OOB_DCVS_SET] = {
+ .name = "GPU_DCVS",
+ .set = 23,
+ .ack = 31,
+ },
+};
+
/* Trigger a OOB (out of band) request to the GMU */
int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state)
{
int ret;
u32 val;
int request, ack;
- const char *name;
- switch (state) {
- case GMU_OOB_GPU_SET:
- if (gmu->legacy) {
- request = GMU_OOB_GPU_SET_REQUEST;
- ack = GMU_OOB_GPU_SET_ACK;
- } else {
- request = GMU_OOB_GPU_SET_REQUEST_NEW;
- ack = GMU_OOB_GPU_SET_ACK_NEW;
- }
- name = "GPU_SET";
- break;
- case GMU_OOB_PERFCOUNTER_SET:
- if (gmu->legacy) {
- request = GMU_OOB_PERFCOUNTER_REQUEST;
- ack = GMU_OOB_PERFCOUNTER_ACK;
- } else {
- request = GMU_OOB_PERFCOUNTER_REQUEST_NEW;
- ack = GMU_OOB_PERFCOUNTER_ACK_NEW;
- }
- name = "PERFCOUNTER";
- break;
- case GMU_OOB_BOOT_SLUMBER:
- request = GMU_OOB_BOOT_SLUMBER_REQUEST;
- ack = GMU_OOB_BOOT_SLUMBER_ACK;
- name = "BOOT_SLUMBER";
- break;
- case GMU_OOB_DCVS_SET:
- request = GMU_OOB_DCVS_REQUEST;
- ack = GMU_OOB_DCVS_ACK;
- name = "GPU_DCVS";
- break;
- default:
+ if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits))
return -EINVAL;
+
+ if (gmu->legacy) {
+ request = a6xx_gmu_oob_bits[state].set;
+ ack = a6xx_gmu_oob_bits[state].ack;
+ } else {
+ request = a6xx_gmu_oob_bits[state].set_new;
+ ack = a6xx_gmu_oob_bits[state].ack_new;
+ if (!request || !ack) {
+ DRM_DEV_ERROR(gmu->dev,
+ "Invalid non-legacy GMU request %s\n",
+ a6xx_gmu_oob_bits[state].name);
+ return -EINVAL;
+ }
}
/* Trigger the equested OOB operation */
@@ -298,7 +317,7 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
if (ret)
DRM_DEV_ERROR(gmu->dev,
"Timeout waiting for GMU OOB set %s: 0x%x\n",
- name,
+ a6xx_gmu_oob_bits[state].name,
gmu_read(gmu, REG_A6XX_GMU_GMU2HOST_INTR_INFO));
/* Clear the acknowledge interrupt */
@@ -310,36 +329,17 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
/* Clear a pending OOB state in the GMU */
void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state)
{
- if (!gmu->legacy) {
- if (state == GMU_OOB_GPU_SET) {
- gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
- 1 << GMU_OOB_GPU_SET_CLEAR_NEW);
- } else {
- WARN_ON(state != GMU_OOB_PERFCOUNTER_SET);
- gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
-
[PATCH v3 1/3] drm/msm: Fix race of GPU init vs timestamp power management.
We were using the same force-poweron bit in the two codepaths, so they
could race to have one of them lose GPU power early.
freedreno CI was seeing intermittent errors like:
[drm:_a6xx_gmu_set_oob] *ERROR* Timeout waiting for GMU OOB set GPU_SET: 0x0
and this issue could have contributed to it.
Signed-off-by: Eric Anholt
Fixes: 4b565ca5a2cb ("drm/msm: Add A6XX device support")
Reviewed-by: Jordan Crouse
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 25 ++---
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 8
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 ++--
3 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index e6703ae98760..b3318f86aabc 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -264,6 +264,16 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
}
name = "GPU_SET";
break;
+ case GMU_OOB_PERFCOUNTER_SET:
+ if (gmu->legacy) {
+ request = GMU_OOB_PERFCOUNTER_REQUEST;
+ ack = GMU_OOB_PERFCOUNTER_ACK;
+ } else {
+ request = GMU_OOB_PERFCOUNTER_REQUEST_NEW;
+ ack = GMU_OOB_PERFCOUNTER_ACK_NEW;
+ }
+ name = "PERFCOUNTER";
+ break;
case GMU_OOB_BOOT_SLUMBER:
request = GMU_OOB_BOOT_SLUMBER_REQUEST;
ack = GMU_OOB_BOOT_SLUMBER_ACK;
@@ -301,9 +311,14 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state)
{
if (!gmu->legacy) {
- WARN_ON(state != GMU_OOB_GPU_SET);
- gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
- 1 << GMU_OOB_GPU_SET_CLEAR_NEW);
+ if (state == GMU_OOB_GPU_SET) {
+ gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
+ 1 << GMU_OOB_GPU_SET_CLEAR_NEW);
+ } else {
+ WARN_ON(state != GMU_OOB_PERFCOUNTER_SET);
+ gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
+ 1 << GMU_OOB_PERFCOUNTER_CLEAR_NEW);
+ }
return;
}
@@ -312,6 +327,10 @@ void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
1 << GMU_OOB_GPU_SET_CLEAR);
break;
+ case GMU_OOB_PERFCOUNTER_SET:
+ gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
+ 1 << GMU_OOB_PERFCOUNTER_CLEAR);
+ break;
case GMU_OOB_BOOT_SLUMBER:
gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
1 << GMU_OOB_BOOT_SLUMBER_CLEAR);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index c6d2bced8e5d..9fa278de2106 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -156,6 +156,7 @@ enum a6xx_gmu_oob_state {
GMU_OOB_BOOT_SLUMBER = 0,
GMU_OOB_GPU_SET,
GMU_OOB_DCVS_SET,
+ GMU_OOB_PERFCOUNTER_SET,
};
/* These are the interrupt / ack bits for each OOB request that are set
@@ -190,6 +191,13 @@ enum a6xx_gmu_oob_state {
#define GMU_OOB_GPU_SET_ACK_NEW31
#define GMU_OOB_GPU_SET_CLEAR_NEW 31
+#define GMU_OOB_PERFCOUNTER_REQUEST17
+#define GMU_OOB_PERFCOUNTER_ACK25
+#define GMU_OOB_PERFCOUNTER_CLEAR 25
+
+#define GMU_OOB_PERFCOUNTER_REQUEST_NEW28
+#define GMU_OOB_PERFCOUNTER_ACK_NEW30
+#define GMU_OOB_PERFCOUNTER_CLEAR_NEW 30
void a6xx_hfi_init(struct a6xx_gmu *gmu);
int a6xx_hfi_start(struct a6xx_gmu *gmu, int boot_state);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index c8a9010c1a1d..7424a70b9d35 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1177,12 +1177,12 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu,
uint64_t *value)
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
/* Force the GPU power on so we can read this register */
- a6xx_gmu_set_oob(_gpu->gmu, GMU_OOB_GPU_SET);
+ a6xx_gmu_set_oob(_gpu->gmu, GMU_OOB_PERFCOUNTER_SET);
*value = gpu_read64(gpu, REG_A6XX_RBBM_PERFCTR_CP_0_LO,
REG_A6XX_RBBM_PERFCTR_CP_0_HI);
- a6xx_gmu_clear_oob(_gpu->gmu, GMU_OOB_GPU_SET);
+ a6xx_gmu_clear_oob(_gpu->gmu, GMU_OOB_PERFCOUNTER_SET);
return 0;
}
--
2.30.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 2/3] drm/msm: Fix races managing the OOB state for timestamp vs timestamps.
Now that we're not racing with GPU setup, also fix races of timestamps
against other timestamps. In freedreno CI, we were seeing this path trigger
timeouts on setting the GMU bit, producing:
[drm:_a6xx_gmu_set_oob] *ERROR* Timeout waiting for GMU OOB set GPU_SET: 0x0
and this triggered especially on the first set of tests right after
boot (it's probably easier to lose the race than one might think,
given that we start many tests in parallel, and waiting for NFS to
page in code probably means that lots of tests hit the same point of
screen init at the same time). As of this patch, the message seems to
have completely gone away.
Signed-off-by: Eric Anholt
Fixes: 4b565ca5a2cb ("drm/msm: Add A6XX device support")
Reviewed-by: Jordan Crouse
---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 7424a70b9d35..e8f0b5325a7f 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1175,6 +1175,9 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu,
uint64_t *value)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
+ static DEFINE_MUTEX(perfcounter_oob);
+
+ mutex_lock(_oob);
/* Force the GPU power on so we can read this register */
a6xx_gmu_set_oob(_gpu->gmu, GMU_OOB_PERFCOUNTER_SET);
@@ -1183,6 +1186,7 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu,
uint64_t *value)
REG_A6XX_RBBM_PERFCTR_CP_0_HI);
a6xx_gmu_clear_oob(_gpu->gmu, GMU_OOB_PERFCOUNTER_SET);
+ mutex_unlock(_oob);
return 0;
}
--
2.30.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 0/3] drm/msm: fix for "Timeout waiting for GMU OOB set GPU_SET: 0x0"
Updated commit messages over v2, no code changes. Eric Anholt (3): drm/msm: Fix race of GPU init vs timestamp power management. drm/msm: Fix races managing the OOB state for timestamp vs timestamps. drm/msm: Clean up GMU OOB set/clear handling. drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 105 +++--- drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 49 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 8 +- 3 files changed, 84 insertions(+), 78 deletions(-) -- 2.30.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/msm: Fix race of GPU init vs timestamp power management.
On Thu, Jan 28, 2021 at 10:52 AM Jordan Crouse wrote:
>
> On Wed, Jan 27, 2021 at 03:39:44PM -0800, Eric Anholt wrote:
> > We were using the same force-poweron bit in the two codepaths, so they
> > could race to have one of them lose GPU power early.
> >
> > Signed-off-by: Eric Anholt
> > Cc: sta...@vger.kernel.org # v5.9
>
> You can add:
> Fixes: 4b565ca5a2cb ("drm/msm: Add A6XX device support")
>
> Because that was my ugly.
>
> Reviewed-by: Jordan Crouse
I only pointed it at 5.9 because it looked like it would probably
conflict against older branches. I can add the fixes tag if you'd
like, though.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/3 v2] drm/msm: Clean up GMU OOB set/clear handling.
Now that the bug is fixed in the minimal way for stable, go make the
code table-driven.
Signed-off-by: Eric Anholt
---
Previous version hadn't been rebased off of a bit of debug code I had,
so it wouldn't cleanly apply.
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 124 +-
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 55
2 files changed, 77 insertions(+), 102 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index b3318f86aabc..9066e98eb8ef 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -245,47 +245,66 @@ static int a6xx_gmu_hfi_start(struct a6xx_gmu *gmu)
return ret;
}
+struct a6xx_gmu_oob_bits {
+ int set, ack, set_new, ack_new;
+ const char *name;
+};
+
+/* These are the interrupt / ack bits for each OOB request that are set
+ * in a6xx_gmu_set_oob and a6xx_clear_oob
+ */
+static const struct a6xx_gmu_oob_bits a6xx_gmu_oob_bits[] = {
+ [GMU_OOB_GPU_SET] = {
+ .name = "GPU_SET",
+ .set = 16,
+ .ack = 24,
+ .set_new = 30,
+ .ack_new = 31,
+ },
+
+ [GMU_OOB_PERFCOUNTER_SET] = {
+ .name = "PERFCOUNTER",
+ .set = 17,
+ .ack = 25,
+ .set_new = 28,
+ .ack_new = 30,
+ },
+
+ [GMU_OOB_BOOT_SLUMBER] = {
+ .name = "BOOT_SLUMBER",
+ .set = 22,
+ .ack = 30,
+ },
+
+ [GMU_OOB_DCVS_SET] = {
+ .name = "GPU_DCVS",
+ .set = 23,
+ .ack = 31,
+ },
+};
+
/* Trigger a OOB (out of band) request to the GMU */
int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state)
{
int ret;
u32 val;
int request, ack;
- const char *name;
- switch (state) {
- case GMU_OOB_GPU_SET:
- if (gmu->legacy) {
- request = GMU_OOB_GPU_SET_REQUEST;
- ack = GMU_OOB_GPU_SET_ACK;
- } else {
- request = GMU_OOB_GPU_SET_REQUEST_NEW;
- ack = GMU_OOB_GPU_SET_ACK_NEW;
- }
- name = "GPU_SET";
- break;
- case GMU_OOB_PERFCOUNTER_SET:
- if (gmu->legacy) {
- request = GMU_OOB_PERFCOUNTER_REQUEST;
- ack = GMU_OOB_PERFCOUNTER_ACK;
- } else {
- request = GMU_OOB_PERFCOUNTER_REQUEST_NEW;
- ack = GMU_OOB_PERFCOUNTER_ACK_NEW;
- }
- name = "PERFCOUNTER";
- break;
- case GMU_OOB_BOOT_SLUMBER:
- request = GMU_OOB_BOOT_SLUMBER_REQUEST;
- ack = GMU_OOB_BOOT_SLUMBER_ACK;
- name = "BOOT_SLUMBER";
- break;
- case GMU_OOB_DCVS_SET:
- request = GMU_OOB_DCVS_REQUEST;
- ack = GMU_OOB_DCVS_ACK;
- name = "GPU_DCVS";
- break;
- default:
+ if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits))
return -EINVAL;
+
+ if (gmu->legacy) {
+ request = a6xx_gmu_oob_bits[state].set;
+ ack = a6xx_gmu_oob_bits[state].ack;
+ } else {
+ request = a6xx_gmu_oob_bits[state].set_new;
+ ack = a6xx_gmu_oob_bits[state].ack_new;
+ if (!request || !ack) {
+ DRM_DEV_ERROR(gmu->dev,
+ "Invalid non-legacy GMU request %s\n",
+ a6xx_gmu_oob_bits[state].name);
+ return -EINVAL;
+ }
}
/* Trigger the equested OOB operation */
@@ -298,7 +317,7 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
if (ret)
DRM_DEV_ERROR(gmu->dev,
"Timeout waiting for GMU OOB set %s: 0x%x\n",
- name,
+ a6xx_gmu_oob_bits[state].name,
gmu_read(gmu, REG_A6XX_GMU_GMU2HOST_INTR_INFO));
/* Clear the acknowledge interrupt */
@@ -310,36 +329,17 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
/* Clear a pending OOB state in the GMU */
void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state)
{
- if (!gmu->legacy) {
- if (state == GMU_OOB_GPU_SET) {
- gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
- 1 << GMU_OOB_GPU_SET_CLEAR_NEW);
- } else {
- WARN_ON(state != GMU_OOB_PERFCOUNTER_SET);
-
[PATCH 3/3] drm/msm: Clean up GMU OOB set/clear handling.
Now that the bug is fixed in the minimal way for stable, go make the
code table-driven.
Signed-off-by: Eric Anholt
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 124 +-
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 55
2 files changed, 77 insertions(+), 102 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 378dc7f190c3..c497e0942141 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -245,47 +245,66 @@ static int a6xx_gmu_hfi_start(struct a6xx_gmu *gmu)
return ret;
}
+struct a6xx_gmu_oob_bits {
+ int set, ack, set_new, ack_new;
+ const char *name;
+};
+
+/* These are the interrupt / ack bits for each OOB request that are set
+ * in a6xx_gmu_set_oob and a6xx_clear_oob
+ */
+static const struct a6xx_gmu_oob_bits a6xx_gmu_oob_bits[] = {
+ [GMU_OOB_GPU_SET] = {
+ .name = "GPU_SET",
+ .set = 16,
+ .ack = 24,
+ .set_new = 30,
+ .ack_new = 31,
+ },
+
+ [GMU_OOB_PERFCOUNTER_SET] = {
+ .name = "PERFCOUNTER",
+ .set = 17,
+ .ack = 25,
+ .set_new = 28,
+ .ack_new = 30,
+ },
+
+ [GMU_OOB_BOOT_SLUMBER] = {
+ .name = "BOOT_SLUMBER",
+ .set = 22,
+ .ack = 30,
+ },
+
+ [GMU_OOB_DCVS_SET] = {
+ .name = "GPU_DCVS",
+ .set = 23,
+ .ack = 31,
+ },
+};
+
/* Trigger a OOB (out of band) request to the GMU */
int _a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state,
char *file, int line)
{
int ret;
u32 val;
int request, ack;
- const char *name;
- switch (state) {
- case GMU_OOB_GPU_SET:
- if (gmu->legacy) {
- request = GMU_OOB_GPU_SET_REQUEST;
- ack = GMU_OOB_GPU_SET_ACK;
- } else {
- request = GMU_OOB_GPU_SET_REQUEST_NEW;
- ack = GMU_OOB_GPU_SET_ACK_NEW;
- }
- name = "GPU_SET";
- break;
- case GMU_OOB_PERFCOUNTER_SET:
- if (gmu->legacy) {
- request = GMU_OOB_PERFCOUNTER_REQUEST;
- ack = GMU_OOB_PERFCOUNTER_ACK;
- } else {
- request = GMU_OOB_PERFCOUNTER_REQUEST_NEW;
- ack = GMU_OOB_PERFCOUNTER_ACK_NEW;
- }
- name = "PERFCOUNTER";
- break;
- case GMU_OOB_BOOT_SLUMBER:
- request = GMU_OOB_BOOT_SLUMBER_REQUEST;
- ack = GMU_OOB_BOOT_SLUMBER_ACK;
- name = "BOOT_SLUMBER";
- break;
- case GMU_OOB_DCVS_SET:
- request = GMU_OOB_DCVS_REQUEST;
- ack = GMU_OOB_DCVS_ACK;
- name = "GPU_DCVS";
- break;
- default:
+ if (state >= ARRAY_SIZE(a6xx_gmu_oob_bits))
return -EINVAL;
+
+ if (gmu->legacy) {
+ request = a6xx_gmu_oob_bits[state].set;
+ ack = a6xx_gmu_oob_bits[state].ack;
+ } else {
+ request = a6xx_gmu_oob_bits[state].set_new;
+ ack = a6xx_gmu_oob_bits[state].ack_new;
+ if (!request || !ack) {
+ DRM_DEV_ERROR(gmu->dev,
+ "Invalid non-legacy GMU request %s\n",
+ a6xx_gmu_oob_bits[state].name);
+ return -EINVAL;
+ }
}
/* Trigger the equested OOB operation */
@@ -299,7 +318,7 @@ int _a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state, char
DRM_DEV_ERROR(gmu->dev,
"%s:%d Timeout waiting for GMU OOB set %s: 0x%x\n",
file, line,
- name,
+ a6xx_gmu_oob_bits[state].name,
gmu_read(gmu, REG_A6XX_GMU_GMU2HOST_INTR_INFO));
/* Clear the acknowledge interrupt */
@@ -311,36 +330,17 @@ int _a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state, char
/* Clear a pending OOB state in the GMU */
void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state)
{
- if (!gmu->legacy) {
- if (state == GMU_OOB_GPU_SET) {
- gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
- 1 << GMU_OOB_GPU_SET_CLEAR_NEW);
- } else {
- WARN_ON(state != GMU_OOB_PERFCOUNTER_SET);
- gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
-
[PATCH 2/3] drm/msm: Fix races managing the OOB state for timestamp vs timestamps.
Now that we're not racing with GPU setup, also fix races of timestamps
against other timestamps. In CI, we were seeing this path trigger
timeouts on setting the GMU bit, especially on the first set of tests
right after boot (it's probably easier to lose the race than one might
think, given that we start many tests in parallel, and waiting for NFS
to page in code probably means that lots of tests hit the same point
of screen init at the same time).
Signed-off-by: Eric Anholt
Cc: sta...@vger.kernel.org # v5.9
---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 7424a70b9d35..e8f0b5325a7f 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1175,6 +1175,9 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu,
uint64_t *value)
{
struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
+ static DEFINE_MUTEX(perfcounter_oob);
+
+ mutex_lock(_oob);
/* Force the GPU power on so we can read this register */
a6xx_gmu_set_oob(_gpu->gmu, GMU_OOB_PERFCOUNTER_SET);
@@ -1183,6 +1186,7 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu,
uint64_t *value)
REG_A6XX_RBBM_PERFCTR_CP_0_HI);
a6xx_gmu_clear_oob(_gpu->gmu, GMU_OOB_PERFCOUNTER_SET);
+ mutex_unlock(_oob);
return 0;
}
--
2.30.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] drm/msm: Fix race of GPU init vs timestamp power management.
We were using the same force-poweron bit in the two codepaths, so they
could race to have one of them lose GPU power early.
Signed-off-by: Eric Anholt
Cc: sta...@vger.kernel.org # v5.9
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 25 ++---
drivers/gpu/drm/msm/adreno/a6xx_gmu.h | 8
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 ++--
3 files changed, 32 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 78836b4fb98e..378dc7f190c3 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -264,6 +264,16 @@ int _a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state, char
}
name = "GPU_SET";
break;
+ case GMU_OOB_PERFCOUNTER_SET:
+ if (gmu->legacy) {
+ request = GMU_OOB_PERFCOUNTER_REQUEST;
+ ack = GMU_OOB_PERFCOUNTER_ACK;
+ } else {
+ request = GMU_OOB_PERFCOUNTER_REQUEST_NEW;
+ ack = GMU_OOB_PERFCOUNTER_ACK_NEW;
+ }
+ name = "PERFCOUNTER";
+ break;
case GMU_OOB_BOOT_SLUMBER:
request = GMU_OOB_BOOT_SLUMBER_REQUEST;
ack = GMU_OOB_BOOT_SLUMBER_ACK;
@@ -302,9 +312,14 @@ int _a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state, char
void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state)
{
if (!gmu->legacy) {
- WARN_ON(state != GMU_OOB_GPU_SET);
- gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
- 1 << GMU_OOB_GPU_SET_CLEAR_NEW);
+ if (state == GMU_OOB_GPU_SET) {
+ gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
+ 1 << GMU_OOB_GPU_SET_CLEAR_NEW);
+ } else {
+ WARN_ON(state != GMU_OOB_PERFCOUNTER_SET);
+ gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
+ 1 << GMU_OOB_PERFCOUNTER_CLEAR_NEW);
+ }
return;
}
@@ -313,6 +328,10 @@ void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum
a6xx_gmu_oob_state state)
gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
1 << GMU_OOB_GPU_SET_CLEAR);
break;
+ case GMU_OOB_PERFCOUNTER_SET:
+ gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
+ 1 << GMU_OOB_PERFCOUNTER_CLEAR);
+ break;
case GMU_OOB_BOOT_SLUMBER:
gmu_write(gmu, REG_A6XX_GMU_HOST2GMU_INTR_SET,
1 << GMU_OOB_BOOT_SLUMBER_CLEAR);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
index c6d2bced8e5d..9fa278de2106 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.h
@@ -156,6 +156,7 @@ enum a6xx_gmu_oob_state {
GMU_OOB_BOOT_SLUMBER = 0,
GMU_OOB_GPU_SET,
GMU_OOB_DCVS_SET,
+ GMU_OOB_PERFCOUNTER_SET,
};
/* These are the interrupt / ack bits for each OOB request that are set
@@ -190,6 +191,13 @@ enum a6xx_gmu_oob_state {
#define GMU_OOB_GPU_SET_ACK_NEW31
#define GMU_OOB_GPU_SET_CLEAR_NEW 31
+#define GMU_OOB_PERFCOUNTER_REQUEST17
+#define GMU_OOB_PERFCOUNTER_ACK25
+#define GMU_OOB_PERFCOUNTER_CLEAR 25
+
+#define GMU_OOB_PERFCOUNTER_REQUEST_NEW28
+#define GMU_OOB_PERFCOUNTER_ACK_NEW30
+#define GMU_OOB_PERFCOUNTER_CLEAR_NEW 30
void a6xx_hfi_init(struct a6xx_gmu *gmu);
int a6xx_hfi_start(struct a6xx_gmu *gmu, int boot_state);
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index c8a9010c1a1d..7424a70b9d35 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1177,12 +1177,12 @@ static int a6xx_get_timestamp(struct msm_gpu *gpu,
uint64_t *value)
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
/* Force the GPU power on so we can read this register */
- a6xx_gmu_set_oob(_gpu->gmu, GMU_OOB_GPU_SET);
+ a6xx_gmu_set_oob(_gpu->gmu, GMU_OOB_PERFCOUNTER_SET);
*value = gpu_read64(gpu, REG_A6XX_RBBM_PERFCTR_CP_0_LO,
REG_A6XX_RBBM_PERFCTR_CP_0_HI);
- a6xx_gmu_clear_oob(_gpu->gmu, GMU_OOB_GPU_SET);
+ a6xx_gmu_clear_oob(_gpu->gmu, GMU_OOB_PERFCOUNTER_SET);
return 0;
}
--
2.30.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/vc4: replace idr_init() by idr_init_base()
On Thu, Nov 5, 2020 at 12:21 PM Deepak R Varma wrote:
>
> idr_init() uses base 0 which is an invalid identifier for this driver.
> The idr_alloc for this driver uses VC4_PERFMONID_MIN as start value for
> ID range and it is #defined to 1. The new function idr_init_base allows
> IDR to set the ID lookup from base 1. This avoids all lookups that
> otherwise starts from 0 since 0 is always unused / available.
>
> References: commit 6ce711f27500 ("idr: Make 1-based IDRs more efficient")
>
> Signed-off-by: Deepak R Varma
> ---
> Changes since v1:
>- Change suggested by Eric Anholt
> 1. Use VC4_PERFMONID_MIN instead of magic number 1
>
> drivers/gpu/drm/vc4/vc4_perfmon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_perfmon.c
> b/drivers/gpu/drm/vc4/vc4_perfmon.c
> index f4aa75efd16b..18abc06335c1 100644
> --- a/drivers/gpu/drm/vc4/vc4_perfmon.c
> +++ b/drivers/gpu/drm/vc4/vc4_perfmon.c
> @@ -77,7 +77,7 @@ struct vc4_perfmon *vc4_perfmon_find(struct vc4_file
> *vc4file, int id)
> void vc4_perfmon_open_file(struct vc4_file *vc4file)
> {
> mutex_init(>perfmon.lock);
> - idr_init(>perfmon.idr);
> + idr_init_base(>perfmon.idr, VC4_PERFMONID_MIN);
> }
>
> static int vc4_perfmon_idr_del(int id, void *elem, void *data)
> --
> 2.25.1
Reviewed-by: Eric Anholt
hopefully Maxime can apply it.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vc4: replace idr_init() by idr_init_base()
On Thu, Nov 5, 2020 at 10:25 AM Deepak R Varma wrote:
>
> idr_init() uses base 0 which is an invalid identifier for this driver.
> The idr_alloc for this driver uses VC4_PERFMONID_MIN as start value for
> ID range and it is #defined to 1. The new function idr_init_base allows
> IDR to set the ID lookup from base 1. This avoids all lookups that
> otherwise starts from 0 since 0 is always unused / available.
>
> References: commit 6ce711f27500 ("idr: Make 1-based IDRs more efficient")
>
> Signed-off-by: Deepak R Varma
> ---
> drivers/gpu/drm/vc4/vc4_perfmon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_perfmon.c
> b/drivers/gpu/drm/vc4/vc4_perfmon.c
> index f4aa75efd16b..7d40f421d922 100644
> --- a/drivers/gpu/drm/vc4/vc4_perfmon.c
> +++ b/drivers/gpu/drm/vc4/vc4_perfmon.c
> @@ -77,7 +77,7 @@ struct vc4_perfmon *vc4_perfmon_find(struct vc4_file
> *vc4file, int id)
> void vc4_perfmon_open_file(struct vc4_file *vc4file)
> {
> mutex_init(>perfmon.lock);
> - idr_init(>perfmon.idr);
> + idr_init_base(>perfmon.idr, 1);
> }
Sounds like you should use VC4_PERFMONID_MIN instead of a magic 1 here.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] MAINTAINERS: Add myself as a maintainer for vc4
Reviewed-by: Eric Anholt I'd be fine with retiring from being maintainer, too. I'm definitely not active. On Fri, Oct 9, 2020 at 12:50 AM Maxime Ripard wrote: > > Eric isn't working on vc4 anymore and I've been working on it, as well as > merging patches for it, recently so let's make it official so I don't miss > patches. > > Signed-off-by: Maxime Ripard > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 91d46806e511..a248e3a2b537 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5957,6 +5957,7 @@ F:include/uapi/drm/v3d_drm.h > > DRM DRIVERS FOR VC4 > M: Eric Anholt > +M: Maxime Ripard > S: Supported > T: git git://github.com/anholt/linux > T: git git://anongit.freedesktop.org/drm/drm-misc > -- > 2.26.2 > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH -next] drm/v3d: Remove set but not used variable
On Wed, Sep 23, 2020 at 6:13 AM Dave Stevenson
wrote:
>
> Hi
>
> On Wed, 23 Sep 2020 at 08:53, Li Heng wrote:
> >
> > This addresses the following gcc warning with "make W=1":
> >
> > drivers/gpu/drm/v3d/v3d_drv.c:73:32: warning:
> > ‘v3d_v3d_pm_ops’ defined but not used [-Wunused-const-variable=]
> >
> > Reported-by: Hulk Robot
> > Signed-off-by: Li Heng
> > ---
> > drivers/gpu/drm/v3d/v3d_drv.c | 4
> > 1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
> > index 9f7c261..05140db 100644
> > --- a/drivers/gpu/drm/v3d/v3d_drv.c
> > +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> > @@ -70,10 +70,6 @@ static int v3d_runtime_resume(struct device *dev)
> > }
> > #endif
> >
> > -static const struct dev_pm_ops v3d_v3d_pm_ops = {
> > - SET_RUNTIME_PM_OPS(v3d_runtime_suspend, v3d_runtime_resume, NULL)
> > -};
> > -
>
> This looks to be the wrong approach, and I think a patch has got
> dropped somewhere.
>
> On our Raspberry Pi downstream vendor tree we have a patch [1] from
> Eric that renames v3d_v3d_pm_ops to v3d_pm_ops (don't need the
> duplicated suffix), and adds it to v3d_platform_driver. Why that never
> made it through the mainline trees I don't know.
>
> Eric: How good's your memory on this one?
The RPM stuff ended up abandoned because I didn't have any support in
debugging the power domain driver and I punted for a downstream hack.
We should at least be using these ops, though.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH AUTOSEL 5.4 001/330] drm/v3d: don't leak bin job if v3d_job_init fails.
On Thu, Sep 17, 2020 at 7:01 PM Sasha Levin wrote:
>
> From: Iago Toral Quiroga
>
> [ Upstream commit 0d352a3a8a1f26168d09f7073e61bb4b328e3bb9 ]
>
> If the initialization of the job fails we need to kfree() it
> before returning.
>
> Signed-off-by: Iago Toral Quiroga
> Signed-off-by: Eric Anholt
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20190916071125.5255-1-ito...@igalia.com
> Fixes: a783a09ee76d ("drm/v3d: Refactor job management.")
> Reviewed-by: Eric Anholt
> Signed-off-by: Sasha Levin
You're double freeing with this patch, the bug is already solved.
> ---
> drivers/gpu/drm/v3d/v3d_gem.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> index 19c092d75266b..6316bf3646af5 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -565,6 +565,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
> ret = v3d_job_init(v3d, file_priv, >base,
>v3d_job_free, args->in_sync_bcl);
> if (ret) {
> + kfree(bin);
> v3d_job_put(>base);
> kfree(bin);
> return ret;
> --
> 2.25.1
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/v3d: Use platform_get_irq_optional() to get optional IRQs
On Thu, Jul 16, 2020 at 7:51 AM Nicolas Saenz Julienne
wrote:
>
> Aside from being more correct, the non optional version of the function
> prints an error when failing to find the IRQ.
>
> Fixes: eea9b97b4504 ("drm/v3d: Add support for V3D v4.2")
> Signed-off-by: Nicolas Saenz Julienne
> ---
> drivers/gpu/drm/v3d/v3d_irq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
> index c88686489b88..0be2eb7876be 100644
> --- a/drivers/gpu/drm/v3d/v3d_irq.c
> +++ b/drivers/gpu/drm/v3d/v3d_irq.c
> @@ -217,7 +217,7 @@ v3d_irq_init(struct v3d_dev *v3d)
> V3D_CORE_WRITE(core, V3D_CTL_INT_CLR, V3D_CORE_IRQS);
> V3D_WRITE(V3D_HUB_INT_CLR, V3D_HUB_IRQS);
>
> - irq1 = platform_get_irq(v3d_to_pdev(v3d), 1);
> + irq1 = platform_get_irq_optional(v3d_to_pdev(v3d), 1);
> if (irq1 == -EPROBE_DEFER)
> return irq1;
> if (irq1 > 0) {
> --
Reviewed-by: Eric Anholt
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vc4: dsi: Only register our component once a DSI device is attached
On Tue, Jul 7, 2020 at 3:26 AM Maxime Ripard wrote: > > If the DSI driver is the last to probe, component_add will try to run all > the bind callbacks straight away and return the error code. > > However, since we depend on a power domain, we're pretty much guaranteed to > be in that case on the BCM2711, and are just lucky on the previous SoCs > since the v3d also depends on that power domain and is further in the probe > order. > > In that case, the DSI host will not stick around in the system: the DSI > bind callback will be executed, will not find any DSI device attached and > will return EPROBE_DEFER, and we will then remove the DSI host and ask to > be probed later on. > > But since that host doesn't stick around, DSI devices like the RaspberryPi > touchscreen whose probe is not linked to the DSI host (unlike the usual DSI > devices that will be probed through the call to mipi_dsi_host_register) > cannot attach to the DSI host, and we thus end up in a situation where the > DSI host cannot probe because the panel hasn't probed yet, and the panel > cannot probe because the DSI host hasn't yet. > > In order to break this cycle, let's wait until there's a DSI device that > attaches to the DSI host to register the component and allow to progress > further. > > Suggested-by: Andrzej Hajda > Signed-off-by: Maxime Ripard I feel like I've written this patch before, but I've thankfully forgotten most of my battle with DSI probing. As long as this still lets vc4 probe in the absence of a DSI panel in the DT as well, then this is enthusiastically acked. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 0/9] drm/vc4: Turn the TXP into a CRTC
On Tue, Jun 30, 2020 at 1:25 AM Maxime Ripard wrote: > > Hi Eric, > > On Thu, Jun 11, 2020 at 03:36:45PM +0200, Maxime Ripard wrote: > > Hi, > > > > This is another part of the rpi4 HDMI series that got promoted to a > > series of its own to try to reduce the main one. > > > > This rework is needed since the bcm2711 used in the rpi4 has a more > > complex routing in the HVS that doesn't allow to use a fairly simple > > mapping like what was used before. > > > > Since that mapping affects both the pixelvalves and the TXP, turning the > > TXP into a CRTC just like pixelvalves are allows to deal with the > > mapping in the CRTC states, which turns out to be pretty convenient. > > > > Let me know what you think, > > Maxime > > > > Changes from v3: > > - Stripped the patches out of the main HDMI series > > - Change the bind order of the HVS to avoid a compatible check > > - Added Eric's tags > > - Rebased on top of drm-misc-next > > > > Maxime Ripard (9): > > drm/vc4: Reorder the bind order of the devices > > drm/vc4: crtc: Move HVS setup code to the HVS driver > > Could you review those two patches? You haven't reviewed them yet and > it's holding back the rest of the patches. LKML email workflow is the worst, the patches never came through to me so I didn't see them until you linked me the patchwork. Patch 2, commit message should say "We'll need the HVS to be bound before the TXP", but other than that, r-b. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vc4: Convert register accessors to FIELD_*
On Fri, Jul 3, 2020 at 6:57 AM Maxime Ripard wrote: > > The VC4_SET_FIELD and VC4_GET_FIELD are reimplementing most of the logic > already defined in FIELD_SET and FIELD_GET. Let's convert the vc4 macros to > use the FIELD_* macros. > > Signed-off-by: Maxime Ripard > --- Reviewed-by: Eric Anholt ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/msm: Quiet error during failure in optional resource mappings.
We don't expect to find vbif_nrt or regdma on cheza, but were clogging
up dmesg with errors about it.
Signed-off-by: Eric Anholt
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 ++--
drivers/gpu/drm/msm/msm_drv.c | 22 ++
drivers/gpu/drm/msm/msm_drv.h | 2 ++
3 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index a4ab802fee6d..d9aef2b5e930 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -838,13 +838,13 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
dpu_kms->vbif[VBIF_RT] = NULL;
goto error;
}
- dpu_kms->vbif[VBIF_NRT] = msm_ioremap(dpu_kms->pdev, "vbif_nrt",
"vbif_nrt");
+ dpu_kms->vbif[VBIF_NRT] = msm_ioremap_quiet(dpu_kms->pdev, "vbif_nrt",
"vbif_nrt");
if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
dpu_kms->vbif[VBIF_NRT] = NULL;
DPU_DEBUG("VBIF NRT is not defined");
}
- dpu_kms->reg_dma = msm_ioremap(dpu_kms->pdev, "regdma", "regdma");
+ dpu_kms->reg_dma = msm_ioremap_quiet(dpu_kms->pdev, "regdma", "regdma");
if (IS_ERR(dpu_kms->reg_dma)) {
dpu_kms->reg_dma = NULL;
DPU_DEBUG("REG_DMA is not defined");
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index f6ce40bf3699..df4a3c6a49cd 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -120,8 +120,8 @@ struct clk *msm_clk_get(struct platform_device *pdev, const
char *name)
return clk;
}
-void __iomem *msm_ioremap(struct platform_device *pdev, const char *name,
- const char *dbgname)
+void __iomem *_msm_ioremap(struct platform_device *pdev, const char *name,
+ const char *dbgname, bool quiet)
{
struct resource *res;
unsigned long size;
@@ -133,7 +133,8 @@ void __iomem *msm_ioremap(struct platform_device *pdev,
const char *name,
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res) {
- DRM_DEV_ERROR(>dev, "failed to get memory resource:
%s\n", name);
+ if (!quiet)
+ DRM_DEV_ERROR(>dev, "failed to get memory
resource: %s\n", name);
return ERR_PTR(-EINVAL);
}
@@ -141,7 +142,8 @@ void __iomem *msm_ioremap(struct platform_device *pdev,
const char *name,
ptr = devm_ioremap(>dev, res->start, size);
if (!ptr) {
- DRM_DEV_ERROR(>dev, "failed to ioremap: %s\n", name);
+ if (!quiet)
+ DRM_DEV_ERROR(>dev, "failed to ioremap: %s\n",
name);
return ERR_PTR(-ENOMEM);
}
@@ -151,6 +153,18 @@ void __iomem *msm_ioremap(struct platform_device *pdev,
const char *name,
return ptr;
}
+void __iomem *msm_ioremap(struct platform_device *pdev, const char *name,
+ const char *dbgname)
+{
+ return _msm_ioremap(pdev, name, dbgname, false);
+}
+
+void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name,
+ const char *dbgname)
+{
+ return _msm_ioremap(pdev, name, dbgname, true);
+}
+
void msm_writel(u32 data, void __iomem *addr)
{
if (reglog)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index e2d6a6056418..2687f7a42c15 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -411,6 +411,8 @@ struct clk *msm_clk_bulk_get_clock(struct clk_bulk_data
*bulk, int count,
const char *name);
void __iomem *msm_ioremap(struct platform_device *pdev, const char *name,
const char *dbgname);
+void __iomem *msm_ioremap_quiet(struct platform_device *pdev, const char *name,
+ const char *dbgname);
void msm_writel(u32 data, void __iomem *addr);
u32 msm_readl(const void __iomem *addr);
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/msm: Garbage collect unused resource _len fields.
Nothing was using the lengths of these ioremaps.
Signed-off-by: Eric Anholt
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 -
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 1 -
drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c | 9 -
3 files changed, 31 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 680527e28d09..a4ab802fee6d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -45,20 +45,6 @@
static int dpu_kms_hw_init(struct msm_kms *kms);
static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms);
-static unsigned long dpu_iomap_size(struct platform_device *pdev,
- const char *name)
-{
- struct resource *res;
-
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, name);
- if (!res) {
- DRM_ERROR("failed to get memory resource: %s\n", name);
- return 0;
- }
-
- return resource_size(res);
-}
-
#ifdef CONFIG_DEBUG_FS
static int _dpu_danger_signal_status(struct seq_file *s,
bool danger_status)
@@ -844,7 +830,6 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
goto error;
}
DRM_DEBUG("mapped dpu address space @%pK\n", dpu_kms->mmio);
- dpu_kms->mmio_len = dpu_iomap_size(dpu_kms->pdev, "mdp");
dpu_kms->vbif[VBIF_RT] = msm_ioremap(dpu_kms->pdev, "vbif", "vbif");
if (IS_ERR(dpu_kms->vbif[VBIF_RT])) {
@@ -853,22 +838,16 @@ static int dpu_kms_hw_init(struct msm_kms *kms)
dpu_kms->vbif[VBIF_RT] = NULL;
goto error;
}
- dpu_kms->vbif_len[VBIF_RT] = dpu_iomap_size(dpu_kms->pdev, "vbif");
dpu_kms->vbif[VBIF_NRT] = msm_ioremap(dpu_kms->pdev, "vbif_nrt",
"vbif_nrt");
if (IS_ERR(dpu_kms->vbif[VBIF_NRT])) {
dpu_kms->vbif[VBIF_NRT] = NULL;
DPU_DEBUG("VBIF NRT is not defined");
- } else {
- dpu_kms->vbif_len[VBIF_NRT] = dpu_iomap_size(dpu_kms->pdev,
-"vbif_nrt");
}
dpu_kms->reg_dma = msm_ioremap(dpu_kms->pdev, "regdma", "regdma");
if (IS_ERR(dpu_kms->reg_dma)) {
dpu_kms->reg_dma = NULL;
DPU_DEBUG("REG_DMA is not defined");
- } else {
- dpu_kms->reg_dma_len = dpu_iomap_size(dpu_kms->pdev, "regdma");
}
pm_runtime_get_sync(_kms->pdev->dev);
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index 4e32d040f1e6..13034cdb8665 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -100,7 +100,6 @@ struct dpu_kms {
/* io/register spaces: */
void __iomem *mmio, *vbif[VBIF_MAX], *reg_dma;
- unsigned long mmio_len, vbif_len[VBIF_MAX], reg_dma_len;
struct regulator *vdd;
struct regulator *mmagic;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
index 80d3cfc14007..9f20b84d5c0a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_mdss.c
@@ -37,7 +37,6 @@ struct dpu_mdss_hw_init_handler {
struct dpu_mdss {
struct msm_mdss base;
void __iomem *mmio;
- unsigned long mmio_len;
struct dss_module_power mp;
struct dpu_irq_controller irq_controller;
struct icc_path *path[2];
@@ -292,7 +291,6 @@ int dpu_mdss_init(struct drm_device *dev)
{
struct platform_device *pdev = to_platform_device(dev->dev);
struct msm_drm_private *priv = dev->dev_private;
- struct resource *res;
struct dpu_mdss *dpu_mdss;
struct dss_module_power *mp;
int ret = 0;
@@ -308,13 +306,6 @@ int dpu_mdss_init(struct drm_device *dev)
DRM_DEBUG("mapped mdss address space @%pK\n", dpu_mdss->mmio);
- res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mdss");
- if (!res) {
- DRM_ERROR("failed to get memory resource for mdss\n");
- return -ENOMEM;
- }
- dpu_mdss->mmio_len = resource_size(res);
-
ret = dpu_mdss_parse_data_bus_icc_path(dev, dpu_mdss);
if (ret)
return ret;
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/msm: Fix address space size after refactor.
Previously the address space went from 16M to ~0u, but with the
refactor one of the 'f's was dropped, limiting us to 256MB.
Additionally, the new interface takes a start and size, not start and
end, so we can't just copy and paste.
Fixes regressions in dEQP-VK.memory.allocation.random.*
Fixes: ccac7ce373c1 ("drm/msm: Refactor address space initialization")
Signed-off-by: Eric Anholt
---
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 89673c7ed473..5db06b590943 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -194,7 +194,7 @@ adreno_iommu_create_address_space(struct msm_gpu *gpu,
struct msm_gem_address_space *aspace;
aspace = msm_gem_address_space_create(mmu, "gpu", SZ_16M,
- 0xfff);
+ 0x - SZ_16M);
if (IS_ERR(aspace) && !IS_ERR(mmu))
mmu->funcs->destroy(mmu);
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/msm: Fix setup of a6xx create_address_space.
We don't want it under CONFIG_DRM_MSM_GPU_STATE, we need it all the
time (like the other GPUs do).
Fixes: ccac7ce373c1 ("drm/msm: Refactor address space initialization")
Signed-off-by: Eric Anholt
---
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index a1589e040c57..7768557cdfb2 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -893,8 +893,8 @@ static const struct adreno_gpu_funcs funcs = {
#if defined(CONFIG_DRM_MSM_GPU_STATE)
.gpu_state_get = a6xx_gpu_state_get,
.gpu_state_put = a6xx_gpu_state_put,
- .create_address_space = adreno_iommu_create_address_space,
#endif
+ .create_address_space = adreno_iommu_create_address_space,
},
.get_timestamp = a6xx_get_timestamp,
};
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 4/5] drm/msm: Refactor address space initialization
On Wed, Jun 17, 2020 at 1:16 PM Eric Anholt wrote: > > On Thu, Apr 9, 2020 at 4:34 PM Jordan Crouse wrote: > > > > Refactor how address space initialization works. Instead of having the > > address space function create the MMU object (and thus require separate but > > equal functions for gpummu and iommu) use a single function and pass the > > MMU struct in. Make the generic code cleaner by using target specific > > functions to create the address space so a2xx can do its own thing in its > > own space. For all the other targets use a generic helper to initialize > > IOMMU but leave the door open for newer targets to use customization > > if they need it. > > I'm seeing regressions in dEQP-VK.memory.allocation.random.* on cheza > after this commit. The symptom is that large allocations fail with > -ENOSPC from MSM_GEM_INFO(IOVA). > > Possibly relevant change from having stuffed some debug info in: > > before: > [3.791436] [drm:msm_gem_address_space_create] *ERROR* msmgem > address space create: 0x100 + 0xfeff > [3.801672] platform 506a000.gmu: Adding to iommu group 6 > [3.807359] [drm:msm_gem_address_space_create] *ERROR* msmgem > address space create: 0x0 + 0x7fff > [3.817140] msm ae0.mdss: bound 500.gpu (ops a3xx_ops) > [3.823212] msm_dpu ae01000.mdp: [drm:msm_ioremap] *ERROR* failed > to get memory resource: vbif_nrt > [3.832429] msm_dpu ae01000.mdp: [drm:msm_ioremap] *ERROR* failed > to get memory resource: regdma > [3.841478] [drm:dpu_kms_hw_init:878] dpu hardware revision:0x4000 > [3.848193] [drm:msm_gem_address_space_create] *ERROR* msmgem > address space create: 0x1000 + 0xefff > > after: > > [3.798707] [drm:msm_gem_address_space_create] *ERROR* msmgem > address space create: 0x100 + 0xfff > [3.808731] platform 506a000.gmu: Adding to iommu group 6 > [3.814440] [drm:msm_gem_address_space_create] *ERROR* msmgem > address space create: 0x0 + 0x7fff > [3.820494] hub 2-1:1.0: USB hub found > [3.824108] msm ae0.mdss: bound 500.gpu (ops a3xx_ops) > [3.828554] hub 2-1:1.0: 4 ports detected > [3.833756] msm_dpu ae01000.mdp: [drm:msm_ioremap] *ERROR* failed > to get memory resource: vbif_nrt > [3.847038] msm_dpu ae01000.mdp: [drm:msm_ioremap] *ERROR* failed > to get memory resource: regdma > [3.856095] [drm:dpu_kms_hw_init:878] dpu hardware revision:0x4000 > [3.862840] [drm:msm_gem_address_space_create] *ERROR* msmgem > address space create: 0x1000 + 0xfff > > 256MB for GMU address space? Found the bug, fixes at the last 2 commits of https://github.com/anholt/linux/tree/drm-msm-address-space I'm going to try having another go at convincing gmail to let git send-email through, but all the howtos in the world didn't work last time (gsuite has different behavior from normal gmail). ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6 4/5] drm/msm: Refactor address space initialization
On Thu, Apr 9, 2020 at 4:34 PM Jordan Crouse wrote: > > Refactor how address space initialization works. Instead of having the > address space function create the MMU object (and thus require separate but > equal functions for gpummu and iommu) use a single function and pass the > MMU struct in. Make the generic code cleaner by using target specific > functions to create the address space so a2xx can do its own thing in its > own space. For all the other targets use a generic helper to initialize > IOMMU but leave the door open for newer targets to use customization > if they need it. I'm seeing regressions in dEQP-VK.memory.allocation.random.* on cheza after this commit. The symptom is that large allocations fail with -ENOSPC from MSM_GEM_INFO(IOVA). Possibly relevant change from having stuffed some debug info in: before: [3.791436] [drm:msm_gem_address_space_create] *ERROR* msmgem address space create: 0x100 + 0xfeff [3.801672] platform 506a000.gmu: Adding to iommu group 6 [3.807359] [drm:msm_gem_address_space_create] *ERROR* msmgem address space create: 0x0 + 0x7fff [3.817140] msm ae0.mdss: bound 500.gpu (ops a3xx_ops) [3.823212] msm_dpu ae01000.mdp: [drm:msm_ioremap] *ERROR* failed to get memory resource: vbif_nrt [3.832429] msm_dpu ae01000.mdp: [drm:msm_ioremap] *ERROR* failed to get memory resource: regdma [3.841478] [drm:dpu_kms_hw_init:878] dpu hardware revision:0x4000 [3.848193] [drm:msm_gem_address_space_create] *ERROR* msmgem address space create: 0x1000 + 0xefff after: [3.798707] [drm:msm_gem_address_space_create] *ERROR* msmgem address space create: 0x100 + 0xfff [3.808731] platform 506a000.gmu: Adding to iommu group 6 [3.814440] [drm:msm_gem_address_space_create] *ERROR* msmgem address space create: 0x0 + 0x7fff [3.820494] hub 2-1:1.0: USB hub found [3.824108] msm ae0.mdss: bound 500.gpu (ops a3xx_ops) [3.828554] hub 2-1:1.0: 4 ports detected [3.833756] msm_dpu ae01000.mdp: [drm:msm_ioremap] *ERROR* failed to get memory resource: vbif_nrt [3.847038] msm_dpu ae01000.mdp: [drm:msm_ioremap] *ERROR* failed to get memory resource: regdma [3.856095] [drm:dpu_kms_hw_init:878] dpu hardware revision:0x4000 [3.862840] [drm:msm_gem_address_space_create] *ERROR* msmgem address space create: 0x1000 + 0xfff 256MB for GMU address space? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/4] drm: pl111: Update documentation
On Tue, Jun 9, 2020 at 1:17 PM Linus Walleij wrote: > > On Tue, Jun 9, 2020 at 10:11 PM Eric Anholt wrote: > > > FWIW, series is Reviewed-by: Eric Anholt > > Thanks Eric, do I remember correct that you were using this > driver for something like a clock display? Are you still using it > for that? > > Nowadays the biggest user is arguably the ARM FVP emulator > which is running a full Android stack using this driver. I've since moved on from Broadcom (now working at Google on open source qualcomm graphics) but as far as I know the HW team at broadcom making those phones will still be using this driver in combination with vc4. I had ported their phone as a vehicle to get vc4 onto more versions of the graphics chip and hopefully get more of bcm invested in open source graphics. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/4] drm: pl111: Update documentation
On Tue, Jun 9, 2020 at 1:08 PM Linus Walleij wrote: > > Remove notes about migrating from the old driver which is > retired as all users are now migrated. > > Update the text to reflect that we support PL110 and PL111 > alike. > > Drop the bullet on memory bandwidth scaling: this has been > implemented. > > Cc: Russell King > Signed-off-by: Linus Walleij FWIW, series is Reviewed-by: Eric Anholt ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 004/105] clk: bcm: Add BCM2711 DVP driver
On Wed, May 27, 2020 at 8:49 AM Maxime Ripard wrote: > > The HDMI block has a block that controls clocks and reset signals to the > HDMI0 and HDMI1 controllers. > > Let's expose that through a clock driver implementing a clock and reset > provider. > > Cc: Michael Turquette > Cc: Stephen Boyd > Cc: Rob Herring > Cc: linux-...@vger.kernel.org > Cc: devicet...@vger.kernel.org > Reviewed-by: Stephen Boyd > Signed-off-by: Maxime Ripard > --- > drivers/clk/bcm/Kconfig | 11 +++- > drivers/clk/bcm/Makefile | 1 +- > drivers/clk/bcm/clk-bcm2711-dvp.c | 127 +++- > 3 files changed, 139 insertions(+) > create mode 100644 drivers/clk/bcm/clk-bcm2711-dvp.c > > diff --git a/drivers/clk/bcm/Kconfig b/drivers/clk/bcm/Kconfig > index 8c83977a7dc4..784f12c72365 100644 > --- a/drivers/clk/bcm/Kconfig > +++ b/drivers/clk/bcm/Kconfig > @@ -1,4 +1,15 @@ > # SPDX-License-Identifier: GPL-2.0-only > + > +config CLK_BCM2711_DVP > + tristate "Broadcom BCM2711 DVP support" > + depends on ARCH_BCM2835 ||COMPILE_TEST > + depends on COMMON_CLK > + default ARCH_BCM2835 > + select RESET_SIMPLE > + help > + Enable common clock framework support for the Broadcom BCM2711 > + DVP Controller. > + > config CLK_BCM2835 > bool "Broadcom BCM2835 clock support" > depends on ARCH_BCM2835 || ARCH_BRCMSTB || COMPILE_TEST > diff --git a/drivers/clk/bcm/Makefile b/drivers/clk/bcm/Makefile > index 0070ddf6cdd2..2c1349062147 100644 > --- a/drivers/clk/bcm/Makefile > +++ b/drivers/clk/bcm/Makefile > @@ -6,6 +6,7 @@ obj-$(CONFIG_CLK_BCM_KONA) += clk-kona-setup.o > obj-$(CONFIG_CLK_BCM_KONA) += clk-bcm281xx.o > obj-$(CONFIG_CLK_BCM_KONA) += clk-bcm21664.o > obj-$(CONFIG_COMMON_CLK_IPROC) += clk-iproc-armpll.o clk-iproc-pll.o > clk-iproc-asiu.o > +obj-$(CONFIG_CLK_BCM2835) += clk-bcm2711-dvp.o > obj-$(CONFIG_CLK_BCM2835) += clk-bcm2835.o > obj-$(CONFIG_CLK_BCM2835) += clk-bcm2835-aux.o > obj-$(CONFIG_CLK_RASPBERRYPI) += clk-raspberrypi.o I do think that single driver is the right model here, but I noticed that you're not using your new CONFIG_ symbol. With that fixed, r-b from me. (though I'd also recommend devm_platform_get_and_ioremap_resource and devm_reset_controller_register()) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 032/105] drm/vc4: crtc: Enable and disable the PV in atomic_enable / disable
On Tue, Jun 2, 2020 at 8:02 AM Dave Stevenson wrote: > > Hi Maxime and Eric > > On Tue, 2 Jun 2020 at 15:12, Maxime Ripard wrote: > > > > Hi Eric > > > > On Wed, May 27, 2020 at 09:54:44AM -0700, Eric Anholt wrote: > > > On Wed, May 27, 2020 at 8:50 AM Maxime Ripard wrote: > > > > > > > > The VIDEN bit in the pixelvalve currently being used to enable or > > > > disable > > > > the pixelvalve seems to not be enough in some situations, which whill > > > > end > > > > up with the pixelvalve stalling. > > > > > > > > In such a case, even re-enabling VIDEN doesn't bring it back and we > > > > need to > > > > clear the FIFO. This can only be done if the pixelvalve is disabled > > > > though. > > > > > > > > In order to overcome this, we can configure the pixelvalve during > > > > mode_set_no_fb, but only enable it in atomic_enable and flush the FIFO > > > > there, and in atomic_disable disable the pixelvalve again. > > > > > > What displays has this been tested with? Getting this sequencing > > > right is so painful, and things like DSI are tricky to get to light > > > up. > > > > That FIFO is between the HVS and the HDMI PVs, so this was obviously > > tested against that. Dave also tested the DSI output IIRC, so we should > > be covered here. > > DSI wasn't working on the first patch set that Maxime sent - I haven't > tested this one as yet but will do so. > DPI was working early on to both an Adafruit 800x480 DPI panel, and > via a VGA666 as VGA. > HDMI is obviously working. > VEC is being ignored now. The clock structure is more restricted than > earlier chips, so to get the required clocks for the VEC without using > fractional divides it compromises the clock that other parts of the > system can run at (IIRC including the ARM). That's why the VEC has to > be explicitly enabled for the firmware to enable it as the only > output. It's annoying, but that's just a restriction of the chip. I'm more concerned with "make sure we don't regress pre-pi4 with this series" than "pi4 displays all work from the beginning" ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 015/105] drm/vc4: hvs: Boost the core clock during modeset
On Tue, Jun 2, 2020 at 5:52 AM Maxime Ripard wrote: > > Hi Eric, > > On Wed, May 27, 2020 at 09:33:44AM -0700, Eric Anholt wrote: > > On Wed, May 27, 2020 at 8:49 AM Maxime Ripard wrote: > > > > > > In order to prevent timeouts and stalls in the pipeline, the core clock > > > needs to be maxed at 500MHz during a modeset on the BCM2711. > > > > Like, the whole system's core clock? > > Yep, unfortunately... > > > How is it reasonable for some device driver to crank the system's core > > clock up and back down to some fixed-in-the-driver frequency? Sounds > > like you need some sort of opp thing here. > > That frequency is the minimum rate of that clock. However, since other > devices have similar requirements (unicam in particular) with different > minimum requirements, we will switch to setting a minimum rate instead > of enforcing a particular rate, so that patch would be essentially > s/clk_set_rate/clk_set_min_rate/. clk_set_min_rate makes a lot more sense to me. r-b with that obvious change. Thanks! ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6] drm/ioctl: Add a ioctl to set and get a label on GEM objects
On Fri, May 29, 2020 at 6:44 AM Rohan Garg wrote:
>
> Hey Eric!
>
> On jueves, 28 de mayo de 2020 20:45:24 (CEST) Eric Anholt wrote:
> > On Thu, May 28, 2020 at 10:06 AM Rohan Garg
> wrote:
> > > DRM_IOCTL_HANDLE_SET_LABEL lets you label buffers associated
> > > with a handle, making it easier to debug issues in userspace
> > > applications.
> > >
> > > DRM_IOCTL_HANDLE_GET_LABEL lets you read the label associated
> > > with a buffer.
> > >
> > > Changes in v2:
> > > - Hoist the IOCTL up into the drm_driver framework
> > >
> > > Changes in v3:
> > > - Introduce a drm_gem_set_label for drivers to use internally
> > >
> > > in order to label a GEM object
> > >
> > > - Hoist string copying up into the IOCTL
> > > - Fix documentation
> > > - Move actual gem labeling into drm_gem_adopt_label
> > >
> > > Changes in v4:
> > > - Refactor IOCTL call to only perform string duplication and move
> > >
> > > all gem lookup logic into GEM specific call
> > >
> > > Changes in v5:
> > > - Fix issues pointed out by kbuild test robot
> > > - Cleanup minor issues around kfree and out/err labels
> > > - Fixed API documentation issues
> > > - Rename to DRM_IOCTL_HANDLE_SET_LABEL
> > > - Introduce a DRM_IOCTL_HANDLE_GET_LABEL to read labels
> > > - Added some documentation for consumers of this IOCTL
> > > - Ensure that label's cannot be longer than PAGE_SIZE
> > > - Set a default label value
> > > - Drop useless warning
> > > - Properly return length of label to userspace even if
> > >
> > > userspace did not allocate memory for label.
> > >
> > > Changes in v6:
> > > - Wrap code to make better use of 80 char limit
> > > - Drop redundant copies of the label
> > > - Protect concurrent access to labels
> > > - Improved documentation
> > > - Refactor setter/getter ioctl's to be static
> > > - Return EINVAL when label length > PAGE_SIZE
> > > - Simplify code by calling the default GEM label'ing
> > >
> > > function for all drivers using GEM
> > >
> > > - Do not error out when fetching empty labels
> > > - Refactor flags to the u32 type and add documentation
> > > - Refactor ioctls to use correct DRM_IOCTL{R,W,WR} macros
> > > - Return length of copied label to userspace
> > >
> > > Signed-off-by: Rohan Garg
> >
> > I don't think we should land this until it actually does something
> > with the label, that feels out of the spirit of our uapi merge rules.
> > I would suggest looking at dma_buf_set_name(), which would produce
> > useful output in debugfs's /dma_buf/buf_info. But also presumably you
> > have something in panfrost using this?
> >
>
> My current short term plan is to hook up glLabel to the labeling functionality
> in order for userspace applications to debug exactly which buffer objects
> could be causing faults in the kernel for speedier debugging.
So, something like "when a fault happens, dump/capture names of nearby
objects"? That would certainly be nice to have!
> The more long term plan is to label each buffer with a unique id that we can
> correlate to the GL calls being flushed in order to be able to profile (a set
> of) GL calls on various platforms in order to aid driver developers with
> performance work. This could be something that we corelate on the userspace
> side with the help of perfetto by using this [1] patch that emits ftrace
> events.
I'm curious how BO names are part of profiling? Do you have the
ability to sample instruction IP and use that to figure out where time
is spent in shaders, or something?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v6] drm/ioctl: Add a ioctl to set and get a label on GEM objects
On Thu, May 28, 2020 at 10:06 AM Rohan Garg wrote:
>
> DRM_IOCTL_HANDLE_SET_LABEL lets you label buffers associated
> with a handle, making it easier to debug issues in userspace
> applications.
>
> DRM_IOCTL_HANDLE_GET_LABEL lets you read the label associated
> with a buffer.
>
> Changes in v2:
> - Hoist the IOCTL up into the drm_driver framework
>
> Changes in v3:
> - Introduce a drm_gem_set_label for drivers to use internally
> in order to label a GEM object
> - Hoist string copying up into the IOCTL
> - Fix documentation
> - Move actual gem labeling into drm_gem_adopt_label
>
> Changes in v4:
> - Refactor IOCTL call to only perform string duplication and move
> all gem lookup logic into GEM specific call
>
> Changes in v5:
> - Fix issues pointed out by kbuild test robot
> - Cleanup minor issues around kfree and out/err labels
> - Fixed API documentation issues
> - Rename to DRM_IOCTL_HANDLE_SET_LABEL
> - Introduce a DRM_IOCTL_HANDLE_GET_LABEL to read labels
> - Added some documentation for consumers of this IOCTL
> - Ensure that label's cannot be longer than PAGE_SIZE
> - Set a default label value
> - Drop useless warning
> - Properly return length of label to userspace even if
> userspace did not allocate memory for label.
>
> Changes in v6:
> - Wrap code to make better use of 80 char limit
> - Drop redundant copies of the label
> - Protect concurrent access to labels
> - Improved documentation
> - Refactor setter/getter ioctl's to be static
> - Return EINVAL when label length > PAGE_SIZE
> - Simplify code by calling the default GEM label'ing
> function for all drivers using GEM
> - Do not error out when fetching empty labels
> - Refactor flags to the u32 type and add documentation
> - Refactor ioctls to use correct DRM_IOCTL{R,W,WR} macros
> - Return length of copied label to userspace
>
> Signed-off-by: Rohan Garg
I don't think we should land this until it actually does something
with the label, that feels out of the spirit of our uapi merge rules.
I would suggest looking at dma_buf_set_name(), which would produce
useful output in debugfs's /dma_buf/buf_info. But also presumably you
have something in panfrost using this?
> +int drm_gem_get_label(struct drm_device *dev, struct drm_file *file_priv,
> + struct drm_handle_label *args)
> +{
> + struct drm_gem_object *gem_obj;
> + int len, ret;
> +
> + gem_obj = drm_gem_object_lookup(file_priv, args->handle);
> + if (!gem_obj) {
> + DRM_DEBUG("Failed to look up GEM BO %d\n", args->handle);
> + return -ENOENT;
> + }
> +
> + if (!gem_obj->label) {
> + args->label = NULL;
> + args->len = 0;
> + return 0;
> + }
> +
> + mutex_lock(_obj->bo_lock);
> + len = strlen(gem_obj->label);
> + ret = copy_to_user(u64_to_user_ptr(args->label), gem_obj->label,
> + min(args->len, len));
> + mutex_unlock(_obj->bo_lock);
> + args->len = len;
> + drm_gem_object_put(gem_obj);
> + return ret;
> +}
I think the !gem_obj->label code path also needs to be under the lock,
otherwise you could be racing to copy_to_user from a NULL pointer,
right?
> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index bb924cddc09c..6fcb7b9ff322 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -540,6 +540,36 @@ struct drm_driver {
> struct drm_device *dev,
> uint32_t handle);
>
> + /**
> +* @set_label:
> +*
> +* Label a buffer object
> +*
> +* Called by the user via ioctl.
> +*
> +* Returns:
> +*
> +* Length of label on success, negative errno on failure.
> +*/
> + int (*set_label)(struct drm_device *dev,
> +struct drm_file *file_priv,
> +struct drm_handle_label *args);
> +
> + /**
> +* @get_label:
> +*
> +* Read the label of a buffer object.
> +*
> +* Called by the user via ioctl.
> +*
> +* Returns:
> +*
> +* Length of label on success, negative errno on failiure.
> +*/
> + char *(*get_label)(struct drm_device *dev,
> + struct drm_file *file_priv,
> + struct drm_handle_label *args);
> +
I think the documentation should note that these are optional.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 055/105] drm/vc4: hvs: Introduce a function to get the assigned FIFO
On Wed, May 27, 2020 at 8:50 AM Maxime Ripard wrote:
>
> At boot time, if we detect that a pixelvalve has been enabled, we need to
> be able to retrieve the HVS channel it has been assigned to so that we can
> disable that channel too. Let's create that function that returns the FIFO
> or an error from a given output.
>
> Signed-off-by: Maxime Ripard
> ---
> +int vc4_hvs_get_fifo_from_output(struct drm_device *dev, unsigned int output)
> +{
> + struct vc4_dev *vc4 = to_vc4_dev(dev);
> + u32 reg;
> + int ret;
> +
> + switch (output) {
> + case 0:
> + return 0;
> +
> + case 1:
> + return 1;
> +
> + case 2:
> + reg = HVS_READ(SCALER_DISPECTRL);
> + ret = FIELD_GET(SCALER_DISPECTRL_DSP2_MUX_MASK, reg);
> + if (ret == 0)
> + return 2;
> +
> + return 0;
> +
> + case 3:
> + reg = HVS_READ(SCALER_DISPCTRL);
> + ret = FIELD_GET(SCALER_DISPCTRL_DSP3_MUX_MASK, reg);
> + if (ret == 3)
> + return -EPIPE;
> +
> + return ret;
> +
> + case 4:
> + reg = HVS_READ(SCALER_DISPEOLN);
> + ret = FIELD_GET(SCALER_DISPEOLN_DSP4_MUX_MASK, reg);
> + if (ret == 3)
> + return -EPIPE;
> +
> + return ret;
> +
> + case 5:
> + reg = HVS_READ(SCALER_DISPDITHER);
> + ret = FIELD_GET(SCALER_DISPDITHER_DSP5_MUX_MASK, reg);
> + if (ret == 3)
> + return -EPIPE;
Oh, FIELD_GET is new to me. Looks like we should replace
VC4_GET_FIELD usage with just using that header, and also
VC4_SET_FIELD with WARN_ON(!FIELD_FIT()); FIELD_PREP.
Could you follow up with that? Other than that, 54-67 r-b.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 059/105] drm/vc4: crtc: Add BCM2711 pixelvalves
On Wed, May 27, 2020 at 8:50 AM Maxime Ripard wrote:
>
> The BCM2711 has 5 pixelvalves, so now that our driver is ready, let's add
> support for them.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/vc4/vc4_crtc.c | 84 ++-
> drivers/gpu/drm/vc4/vc4_regs.h | 6 +++-
> 2 files changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 9efd7cb25590..a577ed8f929f 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -229,6 +229,13 @@ static u32 vc4_get_fifo_full_level(struct vc4_crtc
> *vc4_crtc, u32 format)
> case PV_CONTROL_FORMAT_24:
> case PV_CONTROL_FORMAT_DSIV_24:
> default:
> + /*
> +* For some reason, the pixelvalve4 doesn't work with
> +* the usual formula and will only work with 32.
> +*/
> + if (vc4_crtc->data->hvs_output == 5)
> + return 32;
> +
> return fifo_len_bytes - 3 * HVS_FIFO_LATENCY_PIX;
> }
> }
> @@ -237,9 +244,14 @@ static u32 vc4_crtc_get_fifo_full_level_bits(struct
> vc4_crtc *vc4_crtc,
> u32 format)
> {
> u32 level = vc4_get_fifo_full_level(vc4_crtc, format);
> + u32 ret = 0;
>
> - return VC4_SET_FIELD(level & 0x3f,
> -PV_CONTROL_FIFO_LEVEL);
> + if (level > 0x3f)
> + ret |= VC4_SET_FIELD((level >> 6) & 0x3,
> +PV5_CONTROL_FIFO_LEVEL_HIGH);
> +
I would drop the conditional here (ORing in zero is fine), and also
the & 3 because it would be good to get a warning if you picked a fifo
full level that doesn't fit in the field.
> + return ret | VC4_SET_FIELD(level & 0x3f,
> + PV_CONTROL_FIFO_LEVEL);
> }
>
> /*
> @@ -277,6 +289,8 @@ static void vc4_crtc_pixelvalve_reset(struct drm_crtc
> *crtc)
>
> static void vc4_crtc_config_pv(struct drm_crtc *crtc)
> {
> + struct drm_device *dev = crtc->dev;
> + struct vc4_dev *vc4 = to_vc4_dev(dev);
> struct drm_encoder *encoder = vc4_get_crtc_encoder(crtc);
> struct vc4_encoder *vc4_encoder = to_vc4_encoder(encoder);
> struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> @@ -356,6 +370,10 @@ static void vc4_crtc_config_pv(struct drm_crtc *crtc)
> if (is_dsi)
> CRTC_WRITE(PV_HACT_ACT, mode->hdisplay * pixel_rep);
>
> + if (vc4->hvs->hvs5)
> + CRTC_WRITE(PV_MUX_CFG,
> + VC4_SET_FIELD(8, PV_MUX_CFG_RGB_PIXEL_MUX_MODE));
Can we get some #defines in the reg header instead of a magic value?
Other than that, r-b.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 070/105] drm/vc4: hdmi: rework connectors and encoders
On Wed, May 27, 2020 at 8:51 AM Maxime Ripard wrote: > > the vc4_hdmi driver has some custom structures to hold the data it needs to > associate with the drm_encoder and drm_connector structures. > > However, it allocates them separately from the vc4_hdmi structure which > makes it more complicated than it needs to be. > > Move those structures to be contained by vc4_hdmi and update the code > accordingly. > @@ -1220,7 +1219,7 @@ static int vc4_hdmi_bind(struct device *dev, struct > device *master, void *data) > struct drm_device *drm = dev_get_drvdata(master); > struct vc4_dev *vc4 = drm->dev_private; > struct vc4_hdmi *hdmi; > - struct vc4_hdmi_encoder *vc4_hdmi_encoder; > + struct drm_encoder *encoder; > struct device_node *ddc_node; > u32 value; > int ret; > @@ -1229,14 +1228,10 @@ static int vc4_hdmi_bind(struct device *dev, struct > device *master, void *data) > if (!hdmi) > return -ENOMEM; > > - vc4_hdmi_encoder = devm_kzalloc(dev, sizeof(*vc4_hdmi_encoder), > - GFP_KERNEL); > - if (!vc4_hdmi_encoder) > - return -ENOMEM; > - vc4_hdmi_encoder->base.type = VC4_ENCODER_TYPE_HDMI0; > - hdmi->encoder = _hdmi_encoder->base.base; > - > hdmi->pdev = pdev; > + encoder = >encoder.base.base; > + encoder->base.type = VC4_ENCODER_TYPE_HDMI0; Wait, does this patch build? setting struct drm_encoder->base.type = VC4_* seems very wrong, when previously we were setting struct vc4_hdmi_encoder->base.type (struct vc4_encoder->type). Other than this, patch 68-78 r-b. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 041/105] drm/vc4: crtc: Move HVS mode config to HVS file
On Wed, May 27, 2020 at 8:50 AM Maxime Ripard wrote:
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/vc4/vc4_crtc.c | 272 +---
> drivers/gpu/drm/vc4/vc4_drv.h | 5 +-
> drivers/gpu/drm/vc4/vc4_hvs.c | 298 ++-
> 3 files changed, 309 insertions(+), 266 deletions(-)
> static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc)
> {
> - struct drm_device *dev = crtc->dev;
> - struct vc4_dev *vc4 = to_vc4_dev(dev);
> struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
> - struct drm_display_mode *mode = >state->adjusted_mode;
> - bool interlace = mode->flags & DRM_MODE_FLAG_INTERLACE;
> bool debug_dump_regs = false;
>
> if (debug_dump_regs) {
> @@ -418,42 +372,10 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc
> *crtc)
> drm_print_regset32(, _crtc->regset);
> }
>
> - if (vc4_crtc->data->hvs_output == 2) {
> - u32 dispctrl;
> - u32 dsp3_mux;
> -
> - /*
> -* SCALER_DISPCTRL_DSP3 = X, where X < 2 means 'connect DSP3
> to
> -* FIFO X'.
> -* SCALER_DISPCTRL_DSP3 = 3 means 'disable DSP 3'.
> -*
> -* DSP3 is connected to FIFO2 unless the transposer is
> -* enabled. In this case, FIFO 2 is directly accessed by the
> -* TXP IP, and we need to disable the FIFO2 -> pixelvalve1
> -* route.
> -*/
> - if (vc4_state->feed_txp)
> - dsp3_mux = VC4_SET_FIELD(3, SCALER_DISPCTRL_DSP3_MUX);
> - else
> - dsp3_mux = VC4_SET_FIELD(2, SCALER_DISPCTRL_DSP3_MUX);
> -
> - dispctrl = HVS_READ(SCALER_DISPCTRL) &
> - ~SCALER_DISPCTRL_DSP3_MUX_MASK;
> - HVS_WRITE(SCALER_DISPCTRL, dispctrl | dsp3_mux);
> - }
I just noticed, this block being moved looks like it should probably
have been removed as part of patch #33. Cleaning this up I think will
impact the following patches.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 040/105] drm/vc4: crtc: Turn pixelvalve reset into a function
On Wed, May 27, 2020 at 8:50 AM Maxime Ripard wrote: > > The driver resets the pixelvalve FIFO in a number of occurences without > always using the same sequence. > > Since this will be critical for BCM2711, let's move that sequence to a > function so that we are consistent. > > Signed-off-by: Maxime Ripard Patch 34-40 also r-b. Going to take a little break, this is a lot to try to process at once. Hopefully you can merge reviewed stuff to drm-misc and shorten the series. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 033/105] drm/vc4: crtc: Assign output to channel automatically
On Wed, May 27, 2020 at 8:50 AM Maxime Ripard wrote:
>
> The HVS found in the BCM2711 has 6 outputs and 3 FIFOs, with each output
> being connected to a pixelvalve, and some muxing between the FIFOs and
> outputs.
>
> Any output cannot feed from any FIFO though, and they all have a bunch of
> constraints.
>
> In order to support this, let's store the possible FIFOs each output can be
> assigned to in the vc4_crtc_data, and use that information at atomic_check
> time to iterate over all the CRTCs enabled and assign them FIFOs.
>
> The channel assigned is then set in the vc4_crtc_state so that the rest of
> the driver can use it.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/vc4/vc4_crtc.c | 37 +
> drivers/gpu/drm/vc4/vc4_drv.h | 7 +-
> drivers/gpu/drm/vc4/vc4_kms.c | 142 --
> drivers/gpu/drm/vc4/vc4_regs.h | 10 ++-
> 4 files changed, 172 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 580b37ad514d..a6c3f2f907bd 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -88,6 +88,7 @@ static bool vc4_crtc_get_scanout_position(struct drm_crtc
> *crtc,
> struct drm_device *dev = crtc->dev;
> struct vc4_dev *vc4 = to_vc4_dev(dev);
> struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> + struct vc4_crtc_state *vc4_crtc_state =
> to_vc4_crtc_state(crtc->state);
> unsigned int cob_size;
> u32 val;
> int fifo_lines;
> @@ -104,7 +105,7 @@ static bool vc4_crtc_get_scanout_position(struct drm_crtc
> *crtc,
> * Read vertical scanline which is currently composed for our
> * pixelvalve by the HVS, and also the scaler status.
> */
> - val = HVS_READ(SCALER_DISPSTATX(vc4_crtc->channel));
> + val = HVS_READ(SCALER_DISPSTATX(vc4_crtc_state->assigned_channel));
>
> /* Get optional system timestamp after query. */
> if (etime)
> @@ -124,7 +125,7 @@ static bool vc4_crtc_get_scanout_position(struct drm_crtc
> *crtc,
> *hpos += mode->crtc_htotal / 2;
> }
>
> - cob_size = vc4_crtc_get_cob_allocation(vc4, vc4_crtc->channel);
> + cob_size = vc4_crtc_get_cob_allocation(vc4,
> vc4_crtc_state->assigned_channel);
> /* This is the offset we need for translating hvs -> pv scanout pos.
> */
> fifo_lines = cob_size / mode->crtc_hdisplay;
>
> @@ -211,6 +212,7 @@ vc4_crtc_lut_load(struct drm_crtc *crtc)
> struct drm_device *dev = crtc->dev;
> struct vc4_dev *vc4 = to_vc4_dev(dev);
> struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> + struct vc4_crtc_state *vc4_crtc_state =
> to_vc4_crtc_state(crtc->state);
> u32 i;
>
> /* The LUT memory is laid out with each HVS channel in order,
> @@ -219,7 +221,7 @@ vc4_crtc_lut_load(struct drm_crtc *crtc)
> */
> HVS_WRITE(SCALER_GAMADDR,
> SCALER_GAMADDR_AUTOINC |
> - (vc4_crtc->channel * 3 * crtc->gamma_size));
> + (vc4_crtc_state->assigned_channel * 3 * crtc->gamma_size));
>
> for (i = 0; i < crtc->gamma_size; i++)
> HVS_WRITE(SCALER_GAMDATA, vc4_crtc->lut_r[i]);
> @@ -392,7 +394,7 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc)
> drm_print_regset32(, _crtc->regset);
> }
>
> - if (vc4_crtc->channel == 2) {
> + if (vc4_crtc->data->hvs_output == 2) {
> u32 dispctrl;
> u32 dsp3_mux;
Looks like this hunk is maybe supposed to be in the hvs_output rename patch?
> @@ -419,7 +421,7 @@ static void vc4_crtc_mode_set_nofb(struct drm_crtc *crtc)
> if (!vc4_state->feed_txp)
> vc4_crtc_config_pv(crtc);
>
> - HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel),
> + HVS_WRITE(SCALER_DISPBKGNDX(vc4_state->assigned_channel),
> SCALER_DISPBKGND_AUTOHS |
> SCALER_DISPBKGND_GAMMA |
> (interlace ? SCALER_DISPBKGND_INTERLACE : 0));
> @@ -451,7 +453,8 @@ static void vc4_crtc_atomic_disable(struct drm_crtc *crtc,
> struct drm_device *dev = crtc->dev;
> struct vc4_dev *vc4 = to_vc4_dev(dev);
> struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
> - u32 chan = vc4_crtc->channel;
> + struct vc4_crtc_state *vc4_crtc_state = to_vc4_crtc_state(old_state);
> + u32 chan = vc4_crtc_state->assigned_channel;
> int ret;
> require_hvs_enabled(dev);
>
> @@ -530,12 +533,12 @@ static void vc4_crtc_update_dlist(struct drm_crtc *crtc)
> crtc->state->event = NULL;
> }
>
> - HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel),
> + HVS_WRITE(SCALER_DISPLISTX(vc4_state->assigned_channel),
> vc4_state->mm.start);
>
> spin_unlock_irqrestore(>event_lock, flags);
>
Re: [PATCH v3 032/105] drm/vc4: crtc: Enable and disable the PV in atomic_enable / disable
On Wed, May 27, 2020 at 8:50 AM Maxime Ripard wrote: > > The VIDEN bit in the pixelvalve currently being used to enable or disable > the pixelvalve seems to not be enough in some situations, which whill end > up with the pixelvalve stalling. > > In such a case, even re-enabling VIDEN doesn't bring it back and we need to > clear the FIFO. This can only be done if the pixelvalve is disabled though. > > In order to overcome this, we can configure the pixelvalve during > mode_set_no_fb, but only enable it in atomic_enable and flush the FIFO > there, and in atomic_disable disable the pixelvalve again. What displays has this been tested with? Getting this sequencing right is so painful, and things like DSI are tricky to get to light up. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 020/105] drm/vc4: plane: Create overlays for any CRTC
On Wed, May 27, 2020 at 8:49 AM Maxime Ripard wrote:
>
> Now that we have everything in place, we can now register all the overlay
> planes that can be assigned to all the CRTCs.
>
> This has two side effects:
>
> - The number of overlay planes is reduced from 24 to 8. This is temporary
> and will be increased again in the next patch.
>
> - The ID of the various planes is changed again, and we will now have all
> the primary planes, then all the overlay planes and finally the cursor
> planes. This shouldn't cause any issue since the ordering between
> primary, overlay and cursor planes is preserved.
>
> Signed-off-by: Maxime Ripard
Honestly, I'd squash this with the previous two patches, the
individual refactors don't make much sense on their own or simplify
this patch I think. Either way, patch 17-29 r-b.
> ---
> drivers/gpu/drm/vc4/vc4_plane.c | 35 +-
> 1 file changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index 824c188980b0..5335123ae2a0 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -1378,26 +1378,27 @@ int vc4_plane_create_additional_planes(struct
> drm_device *drm)
> struct drm_crtc *crtc;
> unsigned int i;
>
> - drm_for_each_crtc(crtc, drm) {
> - /* Set up some arbitrary number of planes. We're not limited
> -* by a set number of physical registers, just the space in
> -* the HVS (16k) and how small an plane can be (28 bytes).
> -* However, each plane we set up takes up some memory, and
> -* increases the cost of looping over planes, which atomic
> -* modesetting does quite a bit. As a result, we pick a
> -* modest number of planes to expose, that should hopefully
> -* still cover any sane usecase.
> -*/
> - for (i = 0; i < 8; i++) {
> - struct drm_plane *plane =
> - vc4_plane_init(drm, DRM_PLANE_TYPE_OVERLAY);
> + /* Set up some arbitrary number of planes. We're not limited
> +* by a set number of physical registers, just the space in
> +* the HVS (16k) and how small an plane can be (28 bytes).
> +* However, each plane we set up takes up some memory, and
> +* increases the cost of looping over planes, which atomic
> +* modesetting does quite a bit. As a result, we pick a
> +* modest number of planes to expose, that should hopefully
> +* still cover any sane usecase.
> +*/
> + for (i = 0; i < 8; i++) {
> + struct drm_plane *plane =
> + vc4_plane_init(drm, DRM_PLANE_TYPE_OVERLAY);
>
> - if (IS_ERR(plane))
> - continue;
> + if (IS_ERR(plane))
> + continue;
>
> - plane->possible_crtcs = drm_crtc_mask(crtc);
> - }
> + plane->possible_crtcs =
> + GENMASK(drm->mode_config.num_crtc - 1, 0);
> + }
>
> + drm_for_each_crtc(crtc, drm) {
> /* Set up the legacy cursor after overlay initialization,
> * since we overlay planes on the CRTC in the order they were
> * initialized.
> --
> git-series 0.9.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 016/105] drm/vc4: plane: Improve LBM usage
On Wed, May 27, 2020 at 8:49 AM Maxime Ripard wrote:
>
> From: Dave Stevenson
>
> LBM allocations were always taking the worst case sizing of
> max(src_width, dst_width) * 16. This is significantly over
> the required sizing, and stops us rendering multiple 4k images
> to the screen.
>
> Add some of the additional constraints to more accurately
> describe the LBM requirements.
>
> Signed-off-by: Dave Stevenson
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/vc4/vc4_plane.c | 31 ---
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index 1575c05e3106..602927745f84 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -142,9 +142,10 @@ static const struct hvs_format *vc4_get_hvs_format(u32
> drm_format)
> return NULL;
> }
>
> -static enum vc4_scaling_mode vc4_get_scaling_mode(u32 src, u32 dst)
> +static enum vc4_scaling_mode vc4_get_scaling_mode(u32 src, u32 dst,
> + bool chroma_vrep)
> {
> - if (dst == src)
> + if (dst == src && !chroma_vrep)
> return VC4_SCALING_NONE;
> if (3 * dst >= 2 * src)
> return VC4_SCALING_PPF;
> @@ -369,9 +370,11 @@ static int vc4_plane_setup_clipping_and_scaling(struct
> drm_plane_state *state)
> return ret;
>
> vc4_state->x_scaling[0] = vc4_get_scaling_mode(vc4_state->src_w[0],
> - vc4_state->crtc_w);
> + vc4_state->crtc_w,
> + false);
> vc4_state->y_scaling[0] = vc4_get_scaling_mode(vc4_state->src_h[0],
> - vc4_state->crtc_h);
> + vc4_state->crtc_h,
> + false);
>
> vc4_state->is_unity = (vc4_state->x_scaling[0] == VC4_SCALING_NONE &&
>vc4_state->y_scaling[0] == VC4_SCALING_NONE);
> @@ -384,10 +387,12 @@ static int vc4_plane_setup_clipping_and_scaling(struct
> drm_plane_state *state)
>
> vc4_state->x_scaling[1] =
> vc4_get_scaling_mode(vc4_state->src_w[1],
> -vc4_state->crtc_w);
> +vc4_state->crtc_w,
> +v_subsample == 2);
> vc4_state->y_scaling[1] =
> vc4_get_scaling_mode(vc4_state->src_h[1],
> -vc4_state->crtc_h);
> +vc4_state->crtc_h,
> +v_subsample == 2);
>
> /* YUV conversion requires that horizontal scaling be enabled
> * on the UV plane even if vc4_get_scaling_mode() returned
The change above isn't mentioned in the commit message and I don't
understand what's going on. It should be split out with an
explanation.
> @@ -437,10 +442,7 @@ static void vc4_write_ppf(struct vc4_plane_state
> *vc4_state, u32 src, u32 dst)
> static u32 vc4_lbm_size(struct drm_plane_state *state)
> {
> struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
> - /* This is the worst case number. One of the two sizes will
> -* be used depending on the scaling configuration.
> -*/
> - u32 pix_per_line = max(vc4_state->src_w[0], (u32)vc4_state->crtc_w);
> + u32 pix_per_line;
> u32 lbm;
>
> /* LBM is not needed when there's no vertical scaling. */
> @@ -448,6 +450,11 @@ static u32 vc4_lbm_size(struct drm_plane_state *state)
> vc4_state->y_scaling[1] == VC4_SCALING_NONE)
> return 0;
>
> + if (vc4_state->x_scaling[0] == VC4_SCALING_TPZ)
> + pix_per_line = vc4_state->crtc_w;
> + else
> + pix_per_line = vc4_state->src_w[0];
Looks like it's also crtc_w for RGB or 4:4:4 and HPPF in (0.5,1.0].
Maybe drop a note in here that we're not covering that case, but src_w
> crtc_w so it's safe at least.
> +
> if (!vc4_state->is_yuv) {
> if (vc4_state->y_scaling[0] == VC4_SCALING_TPZ)
> lbm = pix_per_line * 8;
> @@ -583,7 +590,9 @@ static int vc4_plane_allocate_lbm(struct drm_plane_state
> *state)
> spin_lock_irqsave(>hvs->mm_lock, irqflags);
> ret = drm_mm_insert_node_generic(>hvs->lbm_mm,
> _state->lbm,
> -lbm_size, 32, 0, 0);
> +lbm_size,
> +vc4->hvs->hvs5 ? 64 : 32,
> +
Re: [PATCH v3 015/105] drm/vc4: hvs: Boost the core clock during modeset
On Wed, May 27, 2020 at 8:49 AM Maxime Ripard wrote: > > In order to prevent timeouts and stalls in the pipeline, the core clock > needs to be maxed at 500MHz during a modeset on the BCM2711. Like, the whole system's core clock? How is it reasonable for some device driver to crank the system's core clock up and back down to some fixed-in-the-driver frequency? Sounds like you need some sort of opp thing here. Patch 13,14 r-b. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 012/105] drm/vc4: drv: Support BCM2711
On Wed, May 27, 2020 at 8:49 AM Maxime Ripard wrote:
>
> The BCM2711 has a reworked display pipeline, and the load tracker needs
> some adjustement to operate properly. Let's add a compatible for BCM2711
> and disable the load tracker until properly supported.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/vc4/vc4_drv.c | 1 +-
> drivers/gpu/drm/vc4/vc4_drv.h | 3 ++-
> drivers/gpu/drm/vc4/vc4_kms.c | 42 +++---
> drivers/gpu/drm/vc4/vc4_plane.c | 5 -
> 4 files changed, 38 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index 76f93b662766..d7f554a6f0ed 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -364,6 +364,7 @@ static int vc4_platform_drm_remove(struct platform_device
> *pdev)
> }
>
> static const struct of_device_id vc4_of_match[] = {
> + { .compatible = "brcm,bcm2711-vc5", },
> { .compatible = "brcm,bcm2835-vc4", },
> { .compatible = "brcm,cygnus-vc4", },
> {},
Patch 6 Acked-by: Eric Anholt
Patch 7-11 Reviewed-by: Eric Anholt
This one to start probing needs to move later in the series once the
vc5 support is actually present in the driver.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 24/38] drm: v3d: fix common struct sg_table related issues
On Wed, May 13, 2020 at 6:33 AM Marek Szyprowski
wrote:
>
> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
> returns the number of the created entries in the DMA address space.
> However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
> dma_unmap_sg must be called with the original number of the entries
> passed to the dma_map_sg().
>
> struct sg_table is a common structure used for describing a non-contiguous
> memory buffer, used commonly in the DRM and graphics subsystems. It
> consists of a scatterlist with memory pages and DMA addresses (sgl entry),
> as well as the number of scatterlist entries: CPU pages (orig_nents entry)
> and DMA mapped pages (nents entry).
>
> It turned out that it was a common mistake to misuse nents and orig_nents
> entries, calling DMA-mapping functions with a wrong number of entries or
> ignoring the number of mapped entries returned by the dma_map_sg()
> function.
>
> To avoid such issues, lets use a common dma-mapping wrappers operating
> directly on the struct sg_table objects and use scatterlist page
> iterators where possible. This, almost always, hides references to the
> nents and orig_nents entries, making the code robust, easier to follow
> and copy/paste safe.
>
> Signed-off-by: Marek Szyprowski
Reviewed-by: Eric Anholt
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/msm: Check for powered down HW in the devfreq callbacks
On Fri, May 1, 2020 at 12:03 PM Jordan Crouse wrote:
>
> Writing to the devfreq sysfs nodes while the GPU is powered down can
> result in a system crash (on a5xx) or a nasty GMU error (on a6xx):
>
> $ /sys/class/devfreq/500.gpu# echo 5 > min_freq
> [ 104.841625] platform 506a000.gmu: [drm:a6xx_gmu_set_oob]
> *ERROR* Timeout waiting for GMU OOB set GPU_DCVS: 0x0
>
> Despite the fact that we carefully try to suspend the devfreq device when
> the hardware is powered down there are lots of holes in the governors that
> don't check for the suspend state and blindly call into the devfreq
> callbacks that end up triggering hardware reads in the GPU driver.
>
> Call pm_runtime_get_if_in_use() in the gpu_busy() and gpu_set_freq()
> callbacks to skip the hardware access if it isn't active.
>
> v2: Use pm_runtime_get_if_in_use() per Eric Anholt
>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Jordan Crouse
> ---
>
> drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 6 ++
> drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 8
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 7 +++
> 3 files changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> index 724024a2243a..4d7f269edfcc 100644
> --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
> @@ -1404,6 +1404,10 @@ static unsigned long a5xx_gpu_busy(struct msm_gpu *gpu)
> {
> u64 busy_cycles, busy_time;
>
> + /* Only read the gpu busy if the hardware is already active */
> + if (pm_runtime_get_if_in_use(>pdev->dev) <= 0)
> + return 0;
> +
RPM's APIs are a bit of a trap and will return a negative errno for
the get functions if runtime PM is disabled in kconfig, even though
usually that would mean that the power domain is not ever disabled by
RPM. I think in these checks you want "if (pm_runtime_get_if_in_use()
== 0)", and that seems to be a common pattern in other drivers. With
that,
Reviewed-by: Eric Anholt
(and tested, too)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/msm: Check for powered down HW in the devfreq callbacks
On Fri, May 1, 2020 at 11:26 AM Jordan Crouse wrote: > > Writing to the devfreq sysfs nodes while the GPU is powered down can > result in a system crash (on a5xx) or a nasty GMU error (on a6xx): > > $ /sys/class/devfreq/500.gpu# echo 5 > min_freq > [ 104.841625] platform 506a000.gmu: [drm:a6xx_gmu_set_oob] > *ERROR* Timeout waiting for GMU OOB set GPU_DCVS: 0x0 > > Despite the fact that we carefully try to suspend the devfreq device when > the hardware is powered down there are lots of holes in the governors that > don't check for the suspend state and blindly call into the devfreq > callbacks that end up triggering hardware reads in the GPU driver. > > Check the power state in the gpu_busy() and gpu_set_freq() callbacks for > a5xx and a6xx to make sure that the hardware is active before trying to > access it. Chatted on IRC -- while this avoids the instaboot on db820c when setting /sys/class/devfreq/devfreq1/min_freq, I think we should be using pm_runtime_get_if_in_use() to avoid the races while still avoiding bringing up the GPU. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: enable render nodes wherever buffer sharing is supported
On Thu, Apr 30, 2020 at 3:44 AM Daniel Vetter wrote: > > On Tue, Apr 28, 2020 at 2:46 PM Peter Collingbourne wrote: > > > > Render nodes are not just useful for devices supporting GPU hardware > > acceleration. Even on devices that only support dumb frame buffers, > > they are useful in situations where composition (using software > > rasterization) and KMS are done in different processes with buffer > > sharing being used to send frame buffers between them. This is the > > situation on Android, where surfaceflinger is the compositor and the > > composer HAL uses KMS to display the buffers. Thus it is beneficial > > to expose render nodes on all devices that support buffer sharing. > > > > Because all drivers that currently support render nodes also support > > buffer sharing, the DRIVER_RENDER flag is no longer necessary to mark > > devices as supporting render nodes, so remove it and just rely on > > the presence of a prime_handle_to_fd function pointer to determine > > whether buffer sharing is supported. > > The idea behind render nodes is that you can freely pass these to > unpriviledged users, and nothing bad happens. That's why we have gpu > reset code in drivers, proper pagetables, and also (in at least the > solid drivers) ban code so that repeat offenders from userspace who > constantly submit endless batch buffers and funny stuff like that > can't DOS the gpu. > > Ofc in practice there's hw issues and fun stuff like that sometimes, > and driver bugs, and all that. But that's the aspiration. > > Now many of these display-only drivers need contiguous buffers, and > there's not endless amounts of that around. So if you allow random > clients to allocate buffers, they can easily exhaust that, and not > just upset the render side of the gpu, but essentially make it > impossible for a compositor to allocate more framebuffers. I don't > think that's a good idea. > > I know there's hw like vc4 which needs contiguous buffers for > everything, but that's kinda the places where aspiration falls a bit > short. > > So from that pov I'm a rather worried with handing out render rights > to everyone for these display-only buffers. It's not entirely > harmless. This doesn't feel like a contiguous-mem-specific concern to me. We don't have resource limits on renderer GPU nodes today, you can allocate memory there to fill up and DOS the system, and unless something changed since last time I was looking, we don't even tell the OOM killer about our allocations so they can kill the right app! (my compositor always got picked, in my experience) And, keep in mind what we're fixing at a system level here: the current workaround is you either get your compositor to hand you a GPU fd, at which point you can DOS the system, or you open the master node yourself and try to drop master before the compositor comes up, then DOS the system. "But there are access controls for the compositor or the card node!" you say: yes, but there are for render nodes too. I know my distro doesn't default to open access to /dev/dri/render* ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: enable render nodes wherever buffer sharing is supported
On Mon, Apr 27, 2020 at 1:05 PM Peter Collingbourne wrote: > > Render nodes are not just useful for devices supporting GPU hardware > acceleration. Even on devices that only support dumb frame buffers, > they are useful in situations where composition (using software > rasterization) and KMS are done in different processes with buffer > sharing being used to send frame buffers between them. This is the > situation on Android, where surfaceflinger is the compositor and the > composer HAL uses KMS to display the buffers. Thus it is beneficial > to expose render nodes on all devices that support buffer sharing. > > Because all drivers that currently support render nodes also support > buffer sharing, the DRIVER_RENDER flag is no longer necessary to mark > devices as supporting render nodes, so remove it and just rely on > the presence of a prime_handle_to_fd function pointer to determine > whether buffer sharing is supported. I'm definitely interested in seeing a patch like this land, as I think the current state is an ugly historical artifact. We just have to be careful. Were there any instances of drivers with render engines exposing PRIME but not RENDER? We should be careful to make sure that we're not exposing new privileges for those through adding render nodes. There's a UAPI risk I see here. Right now, on a system with a single renderer GPU, we can just open /dev/dri/renderD128 and get the GPU for rendering, and various things are relying on that (such as libwaffle, used in piglit among other things) Adding render nodes for kms-only drivers could displace the actual GPU to 129, and the question is whether this will be disruptive. For Mesa, I think this works out, because kmsro should load on the kms device's node and then share buffers over to the real GPU that it digs around to find at init time. Just saying, I'm not sure I know all of the userspace well enough to say "this should be safe despite that" (And, maybe, if we decide that it's not safe enough, we could punt kms-only drivers to a higher starting number?) > @@ -260,12 +258,6 @@ static int vc4_drm_bind(struct device *dev) > if (!vc4) > return -ENOMEM; > > - /* If VC4 V3D is missing, don't advertise render nodes. */ > - node = of_find_matching_node_and_match(NULL, vc4_v3d_dt_match, NULL); > - if (!node || !of_device_is_available(node)) > - vc4_drm_driver.driver_features &= ~DRIVER_RENDER; > - of_node_put(node); > - > drm = drm_dev_alloc(_drm_driver, dev); > if (IS_ERR(drm)) > return PTR_ERR(drm); Looks like dropping this code from vc4 should be fine -- kmsro looks for a render node from each driver name it supports in turn, so even if v3d moves from renderD128 to renderD129, things should still work. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: pl111: enable render node
On Mon, Apr 27, 2020 at 7:45 AM Emil Velikov wrote: > > On Fri, 24 Apr 2020 at 19:54, Peter Collingbourne wrote: > > > > On Fri, Apr 24, 2020 at 4:11 AM Emil Velikov > > wrote: > > > > > > On Thu, 23 Apr 2020 at 23:51, Peter Collingbourne wrote: > > > > > > > > The render node is required by Android which does not support the legacy > > > > drmAuth authentication process. > > > > > > > > Signed-off-by: Peter Collingbourne > > > > --- > > > > drivers/gpu/drm/pl111/pl111_drv.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > The summary talks about drmAuth, yet exposes a render node. Even > > > through there's no rendering engine in the HW, as mentioned by Eric. > > > > > > AFAICT the only way drmAuth is relevant with pl111 is when you want to > > > export/import dma bufs. > > > Although that is handled in core and the artificial DRM_AUTH > > > restriction has been lifted with commit [1]. > > > > > > -Emil > > > > > > [1] > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.7-rc2=30a958526d2cc6df38347336a602479d048d92e7 > > > > Okay, most likely drmAuth is irrelevant here (I don't know much about > > it to be honest; I know that Android uses render nodes, so I figured > > that drmAuth must therefore be the thing that it doesn't use). Sorry > > for the confusion. Here is a better explanation of why I needed this > > change. > > > > Android has a composer process that opens the primary node and uses > > DRM_IOCTL_MODE_ATOMIC to switch between frame buffers, and a renderer > > process (surfaceflinger) that opens the render node, prepares frame > > buffers and sends them to the composer. One idea for adapting this > > architecture to devices without render nodes is to have the renderer > > process open the primary node instead. But this runs into a problem: > > suppose that the renderer process starts before the composer process. > > In this case, the kernel makes the renderer the DRM master, so the > > composer can't change the frame buffer. Render nodes don't have this > > problem because opening them doesn't affect the master. > > > > I considered fixing this by having the composer issue > > DRM_IOCTL_SET_MASTER, but this requires root privileges. If we require > > drivers to provide render nodes and control access to the primary node > > while opening up the render node, we can ensure that only authorized > > processes can become the master without requiring them to be root. > > > I think that the crux, is trying to open a _primary_ node for > _rendering_ purposes. > While that may work - as you outline above - it's usually a bad idea. > > To step back a bit, in practise we have three SoC combinations: > - complete lack of render device - then you default to software rendering [1] > - display and render device are one and the same - no change needed, > things should just work > - display and render devices are separate - surfaceflinger should > open the correct _rendering_ device node. > > [1] Mesa's libEGL (don't recall exact name on Android) can open VGEM > render node, to work with the kms-swrast_dri.so software rasteriser > module. > > While I did not try [1] with Android, it was working fine with CrOS. I > suggest giving it a try and reporting bugs. VGEM is not a solution, because it doesn't coordinate caching behavior with your display node and so you get corruption if you try to to share dma-bufs between them. In cros it's used only for some tests as far as I've heard, and it's been a source of pain. If we want to go the route of "kms-only nodes also provide a render node interface for doing swrast on", I think that would be a good idea, but we should do this at a core DRM level (probably "does this driver expose dma-buf? then also expose render nodes") rather than per kms driver. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: pl111: enable render node
On Thu, Apr 23, 2020 at 3:35 PM Peter Collingbourne wrote: > > The render node is required by Android which does not support the legacy > drmAuth authentication process. There's no rendering engine on pl111, so only the control node makes sense for this HW since all this driver does is display. Can you clarify what you're trying to do with pl111? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 40/44] drm/cirrus: Don't use drm_device->dev_private
On Fri, Apr 3, 2020 at 6:59 AM Daniel Vetter wrote: > > Upcasting using a container_of macro is more typesafe, faster and > easier for the compiler to optimize. > > Signed-off-by: Daniel Vetter > Cc: Dave Airlie > Cc: Gerd Hoffmann > Cc: Daniel Vetter > Cc: "Noralf Trønnes" > Cc: Sam Ravnborg Acked-by: Eric Anholt ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 11/44] drm/v3d: Don't set drm_device->dev_private
On Fri, Apr 3, 2020 at 6:58 AM Daniel Vetter wrote: > > And switch the helper over to container_of, which is a bunch faster > than chasing a pointer. Plus allows gcc to see through this maze. > > Signed-off-by: Daniel Vetter Acked-by: Eric Anholt ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 12/44] drm/v3d: Use devm_drm_dev_alloc
On Fri, Apr 3, 2020 at 6:58 AM Daniel Vetter wrote: > > Also allows us to simplify the unroll code since the drm_dev_put > disappears. > > Signed-off-by: Daniel Vetter Acked-by: Eric Anholt ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 24/44] drm/hx8357d: Use devm_drm_dev_alloc
On Fri, Apr 3, 2020 at 6:59 AM Daniel Vetter wrote: > > Already using devm_drm_dev_init, so very simple replacment. > > Signed-off-by: Daniel Vetter Acked-by: Eric Anholt ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 14/44] drm/v3d: Delete v3d_dev->pdev
On Fri, Apr 3, 2020 at 6:58 AM Daniel Vetter wrote: > > We already have it in v3d_dev->drm.dev with zero additional pointer > chasing. Personally I don't like duplicated pointers like this > because: > - reviewers need to check whether the pointer is for the same or > different objects if there's multiple > - compilers have an easier time too > > To avoid having to pull in some big headers I implemented the casting > function as a macro instead of a static inline. Typechecking thanks to > container_of still assured. > > But also a bit a bikeshed, so feel free to ignore. > > Signed-off-by: Daniel Vetter > Cc: Eric Anholt ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 13/44] drm/v3d: Delete v3d_dev->dev
On Fri, Apr 3, 2020 at 6:58 AM Daniel Vetter wrote: > > We already have it in v3d_dev->drm.dev with zero additional pointer > chasing. Personally I don't like duplicated pointers like this > because: > - reviewers need to check whether the pointer is for the same or > different objects if there's multiple > - compilers have an easier time too > > But also a bit a bikeshed, so feel free to ignore. > > Signed-off-by: Daniel Vetter > Cc: Eric Anholt a-b. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 18/22] drm/vc4: Use simple encoder
On Thu, Mar 5, 2020 at 8:00 AM Thomas Zimmermann wrote: > > The vc4 driver uses empty implementations for its encoders. Replace > the code with the generic simple encoder. Acked-by: Eric Anholt ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] gpu/drm/v3d: Add ARCH_BCM2835 to DRM_V3D Kconfig
On Wed, Dec 18, 2019 at 6:39 AM Nicolas Saenz Julienne wrote: > > Hi Peter, > > On Wed, 2019-12-18 at 08:43 +, Peter Robinson wrote: > > On arm64 the config ARCH_BCM doesn't exist so to be able to > > build for platforms such as the Raspberry Pi 4 we need to add > > ARCH_BCM2835 similar to what has been done on vc4. > > > > Signed-off-by: Peter Robinson > > --- > > v3d's upstream implementation doesn't support RPi4 for now. So I don't see how > we could benefit from this. All you need is a compatible string for making this driver work on pi4's v3d, so this seems like a good change to be making, to me. Peter, feel like defining the compatible string too? ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] GPU: DRM: VC4/V3D Replace wait_for macros in to remove use of msleep
On Thu, Feb 20, 2020 at 1:44 AM James Hughes wrote: > > On Wed, 19 Feb 2020 at 22:51, Eric Anholt wrote: > > > > On Mon, Feb 17, 2020 at 7:41 AM James Hughes > > wrote: > > > > > > The wait_for macro's for Broadcom VC4 and V3D drivers used msleep > > > which is inappropriate due to its inaccuracy at low values (minimum > > > wait time is about 30ms on the Raspberry Pi). > > > > > > This patch replaces the macro with the one from the Intel i915 version > > > which uses usleep_range to provide more accurate waits. > > > > > > Signed-off-by: James Hughes > > > > To apply this, we're going to want to split the patch up between v3d > > (with a fixes tag to the introduction of the driver, since it's a > > pretty critical fix) and vc4 (where it's used in KMS, and we're pretty > > sure we want it but changing timing like this in KMS paths is risky so > > we don't want to backport to stable). And adjust the commit messages > > to have consistent prefixes to the rest of the commits to those > > drivers. > > > > I've been fighting with the drm maintainer tools today to try to apply > > the patch, with no luck. I'll keep trying, and if I succeed, I'll > > push it. > > Hi Eric, > > unclear whether you want me to do the split or whether you are going > to (your last paragraph). Also I'm a bit unclear on the exact > requirements for the prefixes etc. I debugged what was going on with my maintainer tools and got the patches applied: https://cgit.freedesktop.org/drm/drm-misc/commit/?id=9daee6141cc9c75b09659b02b1cb9eeb2f5e16cc https://cgit.freedesktop.org/drm/drm-misc/commit/?id=7f2a09ecf2e8d86e22598dfb879db48e72c5a40e Apologies for the wait! I've fixed some mail filters I think so I should notice pings like this in the future. But also I hope Maxime can feel enabled to merge patches to vc4/v3d in the future -- I certainly don't want to be the limiting factor here, and it's under drm-misc so any drm-misc maintainer can apply stuff. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Mesa-dev] [Intel-gfx] gitlab.fd.o financial situation and impact on services
On Fri, Feb 28, 2020 at 12:48 AM Dave Airlie wrote: > > On Fri, 28 Feb 2020 at 18:18, Daniel Stone wrote: > > > > On Fri, 28 Feb 2020 at 03:38, Dave Airlie wrote: > > > b) we probably need to take a large step back here. > > > > > > Look at this from a sponsor POV, why would I give X.org/fd.o > > > sponsorship money that they are just giving straight to google to pay > > > for hosting credits? Google are profiting in some minor way from these > > > hosting credits being bought by us, and I assume we aren't getting any > > > sort of discounts here. Having google sponsor the credits costs google > > > substantially less than having any other company give us money to do > > > it. > > > > The last I looked, Google GCP / Amazon AWS / Azure were all pretty > > comparable in terms of what you get and what you pay for them. > > Obviously providers like Packet and Digital Ocean who offer bare-metal > > services are cheaper, but then you need to find someone who is going > > to properly administer the various machines, install decent > > monitoring, make sure that more storage is provisioned when we need > > more storage (which is basically all the time), make sure that the > > hardware is maintained in decent shape (pretty sure one of the fd.o > > machines has had a drive in imminent-failure state for the last few > > months), etc. > > > > Given the size of our service, that's a much better plan (IMO) than > > relying on someone who a) isn't an admin by trade, b) has a million > > other things to do, and c) hasn't wanted to do it for the past several > > years. But as long as that's the resources we have, then we're paying > > the cloud tradeoff, where we pay more money in exchange for fewer > > problems. > > Admin for gitlab and CI is a full time role anyways. The system is > definitely not self sustaining without time being put in by you and > anholt still. If we have $75k to burn on credits, and it was diverted > to just pay an admin to admin the real hw + gitlab/CI would that not > be a better use of the money? I didn't know if we can afford $75k for > an admin, but suddenly we can afford it for gitlab credits? As I think about the time that I've spent at google in less than a year on trying to keep the lights on for CI and optimize our infrastructure in the current cloud environment, that's more than the entire yearly budget you're talking about here. Saying "let's just pay for people to do more work instead of paying for full-service cloud" is not a cost optimization. > > Yes, we could federate everything back out so everyone runs their own > > builds and executes those. Tinderbox did something really similar to > > that IIRC; not sure if Buildbot does as well. Probably rules out > > pre-merge testing, mind. > > Why? does gitlab not support the model? having builds done in parallel > on runners closer to the test runners seems like it should be a thing. > I guess artifact transfer would cost less then as a result. Let's do some napkin math. The biggest artifacts cost we have in Mesa is probably meson-arm64/meson-arm (60MB zipped from meson-arm64, downloaded by 4 freedreno and 6ish lava, about 100 pipelines/day, makes ~1.8TB/month ($180 or so). We could build a local storage next to the lava dispatcher so that the artifacts didn't have to contain the rootfs that came from the container (~2/3 of the insides of the zip file), but that's another service to build and maintain. Building the drivers once locally and storing it would save downloading the other ~1/3 of the inside of the zip file, but that requires a big enough system to do builds in time. I'm planning on doing a local filestore for google's lava lab, since I need to be able to move our xml files off of the lava DUTs to get the xml results we've become accustomed to, but this would not bubble up to being a priority for my time if I wasn't doing it anyway. If it takes me a single day to set all this up (I estimate a couple of weeks), that costs my employer a lot more than sponsoring the costs of the inefficiencies of the system that has accumulated. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] GPU: DRM: VC4/V3D Replace wait_for macros in to remove use of msleep
On Mon, Feb 17, 2020 at 7:41 AM James Hughes wrote: > > The wait_for macro's for Broadcom VC4 and V3D drivers used msleep > which is inappropriate due to its inaccuracy at low values (minimum > wait time is about 30ms on the Raspberry Pi). > > This patch replaces the macro with the one from the Intel i915 version > which uses usleep_range to provide more accurate waits. > > Signed-off-by: James Hughes To apply this, we're going to want to split the patch up between v3d (with a fixes tag to the introduction of the driver, since it's a pretty critical fix) and vc4 (where it's used in KMS, and we're pretty sure we want it but changing timing like this in KMS paths is risky so we don't want to backport to stable). And adjust the commit messages to have consistent prefixes to the rest of the commits to those drivers. I've been fighting with the drm maintainer tools today to try to apply the patch, with no luck. I'll keep trying, and if I succeed, I'll push it. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/v3d: make v3d_debugfs_init return 0
On Tue, Feb 18, 2020 at 9:28 AM Wambui Karuga wrote:
>
> As drm_debugfs_create_files should return void, remove its use as the
> return value of v3d_debugfs_init and have the function return 0
> directly.
If we're going this route, then this function should be returning void, too.
>
> Signed-off-by: Wambui Karuga
> ---
> drivers/gpu/drm/v3d/v3d_debugfs.c | 7 ---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_debugfs.c
> b/drivers/gpu/drm/v3d/v3d_debugfs.c
> index 9e953ce64ef7..57dded6a3957 100644
> --- a/drivers/gpu/drm/v3d/v3d_debugfs.c
> +++ b/drivers/gpu/drm/v3d/v3d_debugfs.c
> @@ -261,7 +261,8 @@ static const struct drm_info_list v3d_debugfs_list[] = {
> int
> v3d_debugfs_init(struct drm_minor *minor)
> {
> - return drm_debugfs_create_files(v3d_debugfs_list,
> - ARRAY_SIZE(v3d_debugfs_list),
> - minor->debugfs_root, minor);
> + drm_debugfs_create_files(v3d_debugfs_list,
> +ARRAY_SIZE(v3d_debugfs_list),
> +minor->debugfs_root, minor);
> + return 0;
> }
> --
> 2.25.0
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 11/52] drm/v3d: Use drmm_add_final_kfree
On Wed, Feb 19, 2020 at 2:21 AM Daniel Vetter wrote: > > With this we can drop the final kfree from the release function. > > I also noticed that the unwind code is wrong, after drm_dev_init the > drm_device owns the v3d allocation, so the kfree(v3d) is a double-free. > Reorder the setup to fix this issue. > > After a bit more prep in drivers and drm core v3d should be able to > switch over to devm_drm_dev_init, which should clean this up further. > > Signed-off-by: Daniel Vetter > Cc: Eric Anholt Acked-by: Eric Anholt ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/msm/a5xx: Always set an OPP supported hardware value
On Fri, Feb 14, 2020 at 10:36 AM Jordan Crouse wrote: > > If the opp table specifies opp-supported-hw as a property but the driver > has not set a supported hardware value the OPP subsystem will reject > all the table entries. > > Set a "default" value that will match the default table entries but not > conflict with any possible real bin values. Also fix a small memory leak > and free the buffer allocated by nvmem_cell_read(). > > Signed-off-by: Jordan Crouse This does fix my warn at boot on db820c. Reviewed-by: Eric Anholt ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/4] drm/msm: Use dma_resv locking wrappers
On Fri, Dec 13, 2019 at 12:08 PM Daniel Vetter wrote:
>
> On Mon, Nov 25, 2019 at 10:43:55AM +0100, Daniel Vetter wrote:
> > I'll add more fancy logic to them soon, so everyone really has to use
> > them. Plus they already provide some nice additional debug
> > infrastructure on top of direct ww_mutex usage for the fences tracked
> > by dma_resv.
> >
> > Signed-off-by: Daniel Vetter
> > Cc: Rob Clark
> > Cc: Sean Paul
> > Cc: linux-arm-...@vger.kernel.org
> > Cc: freedr...@lists.freedesktop.org
>
> Ping for some review/acks.
>
> Thanks, Daniel
>
> > ---
> > drivers/gpu/drm/msm/msm_gem_submit.c | 10 +-
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> > b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index 7d04c47d0023..385d4965a8d0 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -157,7 +157,7 @@ static void submit_unlock_unpin_bo(struct
> > msm_gem_submit *submit,
> > msm_gem_unpin_iova(_obj->base, submit->aspace);
> >
> > if (submit->bos[i].flags & BO_LOCKED)
> > - ww_mutex_unlock(_obj->base.resv->lock);
> > + dma_resv_unlock(msm_obj->base.resv);
> >
> > if (backoff && !(submit->bos[i].flags & BO_VALID))
> > submit->bos[i].iova = 0;
> > @@ -180,8 +180,8 @@ static int submit_lock_objects(struct msm_gem_submit
> > *submit)
> > contended = i;
> >
> > if (!(submit->bos[i].flags & BO_LOCKED)) {
> > - ret =
> > ww_mutex_lock_interruptible(_obj->base.resv->lock,
> > - >ticket);
> > + ret = dma_resv_lock_interruptible(msm_obj->base.resv,
> > + >ticket);
> > if (ret)
> > goto fail;
> > submit->bos[i].flags |= BO_LOCKED;
> > @@ -202,8 +202,8 @@ static int submit_lock_objects(struct msm_gem_submit
> > *submit)
> > if (ret == -EDEADLK) {
> > struct msm_gem_object *msm_obj = submit->bos[contended].obj;
> > /* we lost out in a seqno race, lock and retry.. */
> > - ret =
> > ww_mutex_lock_slow_interruptible(_obj->base.resv->lock,
> > - >ticket);
> > + ret = dma_resv_lock_slow_interruptible(msm_obj->base.resv,
> > +>ticket);
> > if (!ret) {
> > submit->bos[contended].flags |= BO_LOCKED;
> > slow_locked = contended;
> > --
> > 2.24.0
> >
Reviewed-by: Eric Anholt
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/4] drm/vc4: Use dma_resv locking wrappers
On Fri, Dec 13, 2019 at 12:10 PM Daniel Vetter wrote:
>
> On Mon, Nov 25, 2019 at 10:43:56AM +0100, Daniel Vetter wrote:
> > I'll add more fancy logic to them soon, so everyone really has to use
> > them. Plus they already provide some nice additional debug
> > infrastructure on top of direct ww_mutex usage for the fences tracked
> > by dma_resv.
> >
> > Signed-off-by: Daniel Vetter
>
> Ping for some review/acks.
>
> Thanks, Daniel
>
> > ---
> > drivers/gpu/drm/vc4/vc4_gem.c | 11 +--
> > 1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
> > index 7a06cb6e31c5..e1cfc3ccd05a 100644
> > --- a/drivers/gpu/drm/vc4/vc4_gem.c
> > +++ b/drivers/gpu/drm/vc4/vc4_gem.c
> > @@ -568,7 +568,7 @@ vc4_unlock_bo_reservations(struct drm_device *dev,
> > for (i = 0; i < exec->bo_count; i++) {
> > struct drm_gem_object *bo = >bo[i]->base;
> >
> > - ww_mutex_unlock(>resv->lock);
> > + dma_resv_unlock(bo->resv);
> > }
> >
> > ww_acquire_fini(acquire_ctx);
> > @@ -595,8 +595,7 @@ vc4_lock_bo_reservations(struct drm_device *dev,
> > retry:
> > if (contended_lock != -1) {
> > bo = >bo[contended_lock]->base;
> > - ret = ww_mutex_lock_slow_interruptible(>resv->lock,
> > -acquire_ctx);
> > + ret = dma_resv_lock_slow_interruptible(bo->resv, acquire_ctx);
> > if (ret) {
> > ww_acquire_done(acquire_ctx);
> > return ret;
> > @@ -609,19 +608,19 @@ vc4_lock_bo_reservations(struct drm_device *dev,
> >
> > bo = >bo[i]->base;
> >
> > - ret = ww_mutex_lock_interruptible(>resv->lock,
> > acquire_ctx);
> > + ret = dma_resv_lock_interruptible(bo->resv, acquire_ctx);
> > if (ret) {
> > int j;
> >
> > for (j = 0; j < i; j++) {
> > bo = >bo[j]->base;
> > - ww_mutex_unlock(>resv->lock);
> > + dma_resv_unlock(bo->resv);
> > }
> >
> > if (contended_lock != -1 && contended_lock >= i) {
> > bo = >bo[contended_lock]->base;
> >
> > - ww_mutex_unlock(>resv->lock);
> > + dma_resv_unlock(bo->resv);
> > }
> >
> > if (ret == -EDEADLK) {
> > --
> > 2.24.0
> >
Assuming they're supposed to be exactly equivalent currently,
Acked-by: Eric Anholt
but we should really just be using drm_gem_lock_reservations()
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/msm/dpu: Add UBWC support for RGB8888 formats
Rob Clark writes:
> On Wed, Nov 6, 2019 at 3:26 PM Fritz Koenig wrote:
>>
>> Hardware only natively supports BGR UBWC.
>> UBWC support for RGB can be had by pretending
>> that the buffer is BGR.
>
> Just to expand, this aligns with how we handle RGB component order in
> mesa for tiled or tiled+ubwc. If uncompressed to linear the component
> order is RGB, but in tiled or tiled+ubwc, the component order is
> always the hw "native" order (BGR) regardless of what the outside
> world thinks. But that detail kinda doesn't matter, it's not like
> generic code is going to understand the tiled or tiled+ubwc format in
> the first place.. and code that does understand it, knows enough to
> know that tiled/tiled+ubwc is always in the native component order.
>
>> Signed-off-by: Fritz Koenig
>
> Reviewed-by: Rob Clark
Seems like a reasonable workaround to me, and permissible by our fourcc
modifier rules ("you just have to have one way to address the pixels
given a fourcc and a modifier").
Reviewed-by: Eric Anholt
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] drm/v3d: clean caches at the end of render jobs on request from user space
Iago Toral Quiroga writes: > Extends the user space ioctl for CL submissions so it can include a request > to flush the cache once the CL execution has completed. Fixes memory > write violation messages reported by the kernel in workloads involving > shader memory writes (SSBOs, shader images, scratch, etc) which sometimes > also lead to GPU resets during Piglit and CTS workloads. > > v2: if v3d_job_init() fails we need to kfree() the job instead of > v3d_job_put() it (Eric Anholt). > > v3 (Eric Anholt): > - Drop _FLAG suffix from the new flag name. > - Add a new param so userspace can tell whether cache flushing is > implemented in the kernel. Appled to drm-misc-next. Thanks! signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC][PATCH] libdrm: Convert to Android.mk to Android.bp
John Stultz writes: > This patch removes the deprecated Android.mk files and replaces > them with Android.bp files. > > This is needed in order to build libdrm/master against recent > Android releases and AOSP/master, as some of the Treble build > options required since Android O cannot be expressed in > Andorid.mk files. > > Patch originally by Dan Willemsen with fixes folded in by: > Jerry Zhang, Eliott Hughes and myself. > > With this change, the only patches carried by Android for libdrm > would be the gerrit OWNERS meta-data file. Acked-by: Eric Anholt signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/v3d: don't leak bin job if v3d_job_init fails.
Iago Toral Quiroga writes: > If the initialization of the job fails we need to kfree() it > before returning. > > Signed-off-by: Iago Toral Quiroga Applied to drm-misc-next with a fixes tag for the commit that introduced the bug. Thanks! signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/v3d: clean caches at the end of render jobs on request from user space
Iago Toral Quiroga writes:
> Extends the user space ioctl for CL submissions so it can include a request
> to flush the cache once the CL execution has completed. Fixes memory
> write violation messages reported by the kernel in workloads involving
> shader memory writes (SSBOs, shader images, scratch, etc) which sometimes
> also lead to GPU resets during Piglit and CTS workloads.
>
> v2: if v3d_job_init() fails we need to kfree() the job instead of
> v3d_job_put() it (Eric Anholt).
>
> Signed-off-by: Iago Toral Quiroga
> Reviewed-by: Eric Anholt
> Link:
> https://patchwork.freedesktop.org/patch/msgid/20190912083516.13797-1-ito...@igalia.com
> diff --git a/include/uapi/drm/v3d_drm.h b/include/uapi/drm/v3d_drm.h
> index 58fbe48c91e9..58d2040ea48c 100644
> --- a/include/uapi/drm/v3d_drm.h
> +++ b/include/uapi/drm/v3d_drm.h
> @@ -48,6 +48,8 @@ extern "C" {
> #define DRM_IOCTL_V3D_SUBMIT_TFU DRM_IOW(DRM_COMMAND_BASE +
> DRM_V3D_SUBMIT_TFU, struct drm_v3d_submit_tfu)
> #define DRM_IOCTL_V3D_SUBMIT_CSD DRM_IOW(DRM_COMMAND_BASE +
> DRM_V3D_SUBMIT_CSD, struct drm_v3d_submit_csd)
>
> +#define DRM_V3D_SUBMIT_CL_FLUSH_CACHE_FLAG0x01
> +
Hmm. vc4, msm, panfrost, etnaviv, i915 all name their submit flags
without "_FLAG" in the name, can we drop that?
Also, I just noticed: You don't have a new param to indicate the
availability of the new flag. You're going to need that (unless you've
got some other clever plan?) so that new Mesa can detect an old kernel
and not expose the GLES 3.1 features that require it.
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/v3d: clean caches at the end of render jobs on request from user space
Iago Toral writes:
> On Thu, 2019-09-12 at 10:25 -0700, Eric Anholt wrote:
>> Iago Toral Quiroga writes:
>>
>> > Extends the user space ioctl for CL submissions so it can include a
>> > request
>> > to flush the cache once the CL execution has completed. Fixes
>> > memory
>> > write violation messages reported by the kernel in workloads
>> > involving
>> > shader memory writes (SSBOs, shader images, scratch, etc) which
>> > sometimes
>> > also lead to GPU resets during Piglit and CTS workloads.
>>
>> Some context for any other reviewers: This patch is the interface
>> change
>> necessary to expose GLES 3.1 on V3D. It turns out the HW packets for
>> flushing the caches were broken in multiple ways.
>>
>> > Signed-off-by: Iago Toral Quiroga
>> > ---
>> > drivers/gpu/drm/v3d/v3d_gem.c | 51 +
>> > --
>> > include/uapi/drm/v3d_drm.h| 7 ++---
>> > 2 files changed, 47 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/v3d/v3d_gem.c
>> > b/drivers/gpu/drm/v3d/v3d_gem.c
>> > index 5d80507b539b..530fe9d9d5bd 100644
>> > --- a/drivers/gpu/drm/v3d/v3d_gem.c
>> > +++ b/drivers/gpu/drm/v3d/v3d_gem.c
>> > @@ -530,13 +530,16 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
>> > void *data,
>> >struct drm_v3d_submit_cl *args = data;
>> >struct v3d_bin_job *bin = NULL;
>> >struct v3d_render_job *render;
>> > + struct v3d_job *clean_job = NULL;
>> > + struct v3d_job *last_job;
>> >struct ww_acquire_ctx acquire_ctx;
>> >int ret = 0;
>> >
>> >trace_v3d_submit_cl_ioctl(>drm, args->rcl_start, args-
>> > >rcl_end);
>> >
>> > - if (args->pad != 0) {
>> > - DRM_INFO("pad must be zero: %d\n", args->pad);
>> > + if (args->flags != 0 &&
>> > + args->flags != DRM_V3D_SUBMIT_CL_FLUSH_CACHE_FLAG) {
>> > + DRM_INFO("invalid flags: %d\n", args->flags);
>> >return -EINVAL;
>> >}
>> >
>> > @@ -575,12 +578,28 @@ v3d_submit_cl_ioctl(struct drm_device *dev,
>> > void *data,
>> >bin->render = render;
>> >}
>> >
>> > - ret = v3d_lookup_bos(dev, file_priv, >base,
>> > + if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE_FLAG) {
>> > + clean_job = kcalloc(1, sizeof(*clean_job), GFP_KERNEL);
>> > + if (!clean_job) {
>> > + ret = -ENOMEM;
>> > + goto fail;
>> > + }
>> > +
>> > + ret = v3d_job_init(v3d, file_priv, clean_job,
>> > v3d_job_free, 0);
>> > + if (ret)
>> > + goto fail;
>>
>> Only issue I see: If v3d_job_init() fails, we need to not
>> v3d_job_put()
>> it. I'm fine with either kfree() it and NULL the ptr before jumping
>> to
>> fail, or open code the bin/render puts.
>
> It seems we also call v3d_job_put() for the bin job when v3d_job_init()
> fails, which also returns immediately in that case instead of jumping
> to fail to v3d_job_put the render job, so I guess we need the same
> treatment there. Shall I fix that in this patch too or would you rather
> see a different patch sent separately for that?
I think you might be looking at the put of the (already-inited) render
job when initing bin job fails?
Looks like we do leak bin in that case, though. Happy to see that as a
fixup patch.
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: RFC: IOCTL to label BO in DRM Core
Rohan Garg writes: > [ Unknown signature status ] > Hi > I'm investigating a way to label BO's in panfrost, similar to how VC4 does it > and realised that this could be something that's might be useful to all > drivers. > > With that in mind, would anyone be opposed to add a DRM_IOCTL_BO_SET_LABEL in > DRM core that can be utilised by all drivers to label BO's? I would love to see something shared. msm has an object labeling interface as well. vc4's had some overengineering due to wanting to keep a bucketed-by-name usage list so we could dump that when we ran out of memory or from debugfs. I think something much simpler would be better. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/v3d: clean caches at the end of render jobs on request from user space
Iago Toral Quiroga writes:
> Extends the user space ioctl for CL submissions so it can include a request
> to flush the cache once the CL execution has completed. Fixes memory
> write violation messages reported by the kernel in workloads involving
> shader memory writes (SSBOs, shader images, scratch, etc) which sometimes
> also lead to GPU resets during Piglit and CTS workloads.
Some context for any other reviewers: This patch is the interface change
necessary to expose GLES 3.1 on V3D. It turns out the HW packets for
flushing the caches were broken in multiple ways.
> Signed-off-by: Iago Toral Quiroga
> ---
> drivers/gpu/drm/v3d/v3d_gem.c | 51 +--
> include/uapi/drm/v3d_drm.h| 7 ++---
> 2 files changed, 47 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c
> index 5d80507b539b..530fe9d9d5bd 100644
> --- a/drivers/gpu/drm/v3d/v3d_gem.c
> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
> @@ -530,13 +530,16 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
> struct drm_v3d_submit_cl *args = data;
> struct v3d_bin_job *bin = NULL;
> struct v3d_render_job *render;
> + struct v3d_job *clean_job = NULL;
> + struct v3d_job *last_job;
> struct ww_acquire_ctx acquire_ctx;
> int ret = 0;
>
> trace_v3d_submit_cl_ioctl(>drm, args->rcl_start, args->rcl_end);
>
> - if (args->pad != 0) {
> - DRM_INFO("pad must be zero: %d\n", args->pad);
> + if (args->flags != 0 &&
> + args->flags != DRM_V3D_SUBMIT_CL_FLUSH_CACHE_FLAG) {
> + DRM_INFO("invalid flags: %d\n", args->flags);
> return -EINVAL;
> }
>
> @@ -575,12 +578,28 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
> bin->render = render;
> }
>
> - ret = v3d_lookup_bos(dev, file_priv, >base,
> + if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE_FLAG) {
> + clean_job = kcalloc(1, sizeof(*clean_job), GFP_KERNEL);
> + if (!clean_job) {
> + ret = -ENOMEM;
> + goto fail;
> + }
> +
> + ret = v3d_job_init(v3d, file_priv, clean_job, v3d_job_free, 0);
> + if (ret)
> + goto fail;
Only issue I see: If v3d_job_init() fails, we need to not v3d_job_put()
it. I'm fine with either kfree() it and NULL the ptr before jumping to
fail, or open code the bin/render puts.
With that,
Reviewed-by: Eric Anholt
> +
> + last_job = clean_job;
> + } else {
> + last_job = >base;
> + }
> +
> + ret = v3d_lookup_bos(dev, file_priv, last_job,
>args->bo_handles, args->bo_handle_count);
> if (ret)
> goto fail;
>
> - ret = v3d_lock_bo_reservations(>base, _ctx);
> + ret = v3d_lock_bo_reservations(last_job, _ctx);
> if (ret)
> goto fail;
>
> @@ -599,28 +618,44 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
> ret = v3d_push_job(v3d_priv, >base, V3D_RENDER);
> if (ret)
> goto fail_unreserve;
> +
> + if (clean_job) {
> + struct dma_fence *render_fence =
> + dma_fence_get(render->base.done_fence);
> + ret = drm_gem_fence_array_add(_job->deps, render_fence);
> + if (ret)
> + goto fail_unreserve;
> + ret = v3d_push_job(v3d_priv, clean_job, V3D_CACHE_CLEAN);
> + if (ret)
> + goto fail_unreserve;
> + }
> +
> mutex_unlock(>sched_lock);
>
> v3d_attach_fences_and_unlock_reservation(file_priv,
> - >base,
> + last_job,
>_ctx,
>args->out_sync,
> - render->base.done_fence);
> + last_job->done_fence);
>
> if (bin)
> v3d_job_put(>base);
> v3d_job_put(>base);
> + if (clean_job)
> + v3d_job_put(clean_job);
>
> return 0;
>
> fail_unreserve:
> mutex_unlock(>sched_lock);
> - drm_gem_unlock_reservations(render->base.bo,
> - render->base.bo_count, _ctx);
> + drm_gem_unlock_reservations(last_job->bo,
> + last_job->bo_count, _ctx);
> fail:
&
Re: [PATCH] Revert "ARM: bcm283x: Switch V3D over to using the PM driver instead of firmware."
Stefan Wahren writes:
> Since release of the new BCM2835 PM driver there has been several reports
> of V3D probing issues. This is caused by timeouts during powering-up the
> GRAFX PM domain:
>
> bcm2835-power: Timeout waiting for grafx power OK
>
> I was able to reproduce this reliable on my Raspberry Pi 3B+ after setting
> force_turbo=1 in the firmware configuration. Since there are no issues
> using the firmware PM driver with the same setup, there must be an issue
> in the BCM2835 PM driver.
>
> Unfortunately there hasn't been much progress in identifying the root cause
> since June (mostly in the lack of documentation), so i decided to switch
> back until the issue in the BCM2835 PM driver is fixed.
>
> Link: https://github.com/raspberrypi/linux/issues/3046
> Fixes: e1dc2b2e1bef (" ARM: bcm283x: Switch V3D over to using the PM driver
> instead of firmware.")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Stefan Wahren
Acked-by: Eric Anholt
I wish someone with firmware source had the time to look into why using
open source drivers to drive this hardware was failing, but I don't have
that time or code any more.
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/vc4/vc4_hdmi: fill in connector info
Hans Verkuil writes: > From: Dariusz Marcinkiewicz > > Fill in the connector info, allowing userspace to associate > the CEC device with the drm connector. Acked-by: Eric Anholt signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 2/9] drm/shmem: Put pages independent of a SG table being set
Rob Herring writes: > If a driver does its own management of pages, the shmem helper object's > pages array could be allocated when a SG table is not. There's not > really any good reason to tie putting pages with having a SG table when > freeing the object, so just put pages if the pages array is populated. > > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: Sean Paul > Cc: David Airlie > Cc: Daniel Vetter > Cc: Eric Anholt > Reviewed-by: Steven Price > Acked-by: Alyssa Rosenzweig > Signed-off-by: Rob Herring Patch 1, 2: Reviewed-by: Eric Anholt signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC 03/19] drm/vc4: Get rid of the dsi->bridge field
Boris Brezillon writes: > Now that we have an official way to request custom encoder/bridge > enable/disable sequences we can get rid of the extra ->bridge field > and use the encoder one. Patch 2, 3 are: Reviewed-by: Eric Anholt signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/6] drm/tinydrm: Move mipi_dbi
Noralf Trønnes writes: > This series ticks off the last tinydrm todo entry and moves out mipi_dbi > to be a core helper. > > It splits struct mipi_dbi into an interface part and a display pipeline > part (upload framebuffer over SPI). I also took the opportunity to > rename the ambiguous 'mipi' variable name to 'dbi'. This lines up with > the use of the 'dsi' variable name in the MIPI DSI helper. > > Note: > This depends on series: drm/tinydrm: Remove tinydrm.ko > > Series is also available here: > https://github.com/notro/linux/tree/move_mipi_dbi Congratulations on making it to this stage. This looks like a fine conclusion to tinydrm. Acked-by: Eric Anholt signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/3] drm/vgem: use normal cached mmap'ings
Rob Clark writes: > From: Rob Clark > > Since there is no real device associated with VGEM, it is impossible to > end up with appropriate dev->dma_ops, meaning that we have no way to > invalidate the shmem pages allocated by VGEM. So, at least on platforms > without drm_cflush_pages(), we end up with corruption when cache lines > from previous usage of VGEM bo pages get evicted to memory. > > The only sane option is to use cached mappings. This may be an improvement, but... pin/unpin is only on attaching/closing the dma-buf, right? So, great, you flushed the cached map once after exporting the vgem dma-buf to the actual GPU device, but from then on you still have no interface for getting coherent access through VGEM's mapping again, which still exists. I feel like this is papering over something that's really just broken, and we should stop providing VGEM just because someone wants to write dma-buf test code without driver-specific BO alloc ioctl code. signature.asc Description: PGP signature
Re: [Intel-gfx] [PATCH v3 1/3] drm/gem: don't force writecombine mmap'ing
Rob Clark writes: > From: Rob Clark > > The driver should be in control of this. > > Signed-off-by: Rob Clark > --- > It is possible that this was masking bugs (ie. not setting appropriate > pgprot) in drivers. I don't have a particularly good idea for tracking > those down (since I don't have the hw for most drivers). Unless someone > has a better idea, maybe land this and let driver maintainers fix any > potential fallout in their drivers? > > This is necessary for the last patch to fix VGEM brokenness on arm. This will break at least v3d and panfrost, and it looks like cirrus as well, since you're now promoting the mapping to cached by default and drm_gem_shmem_helper now produces cached mappings. That's all I could find that would break, but don't trust me on that. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/19] drm: drop use of drmp.h in drm-misc
Sam Ravnborg writes: > This patch set removes a far share of the remaining uses of drmP.h. > Common for all patches are that the respective files are maintained > in drm-misc. > All patches are independent. For v3d, vc4, pl111: Reviewed-by: Eric Anholt signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] gpu: drm: pl111: pl111_vexpress.c: Add of_node_put() before return
Nishka Dasgupta writes: > Each iteration of for_each_available_child_of_node puts the previous > node, but in the case of a break from the middle of the loop there is > no put, thus causing a memory leak. Hence add an of_node_put before the > break. > Issue found with Coccinelle. Pushed. Thanks! signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/5] clk: inherit clocks enabled by bootloader
Rob Clark writes: > From: Rob Clark > > The goal here is to support inheriting a display setup by bootloader, > although there may also be some non-display related use-cases. > > Rough idea is to add a flag for clks and power domains that might > already be enabled when kernel starts, and which should not be > disabled at late_initcall if the kernel thinks they are "unused". > > If bootloader is enabling display, and kernel is using efifb before > real display driver is loaded (potentially from kernel module after > userspace starts, in a typical distro kernel), we don't want to kill > the clocks and power domains that are used by the display before > userspace starts. > > Signed-off-by: Rob Clark Raspberry Pi is carrying downstream hacks to do similar stuff, and it would be great to see CCF finally support this. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/7] drm/fb-helper: use gem_bo.resv, not dma_buf.resv in prepare_fb
Daniel Vetter writes: > With > > commit 5f6ed9879a414636405a2bd77f122881695959e4 > Author: Daniel Vetter > Date: Fri Jun 14 22:35:57 2019 +0200 > > drm/prime: automatically set gem_obj->resv on import > > we consistently set drm_gem_bo.resv for imported buffers. Which means > we don't need to check to check the dma-buf in the prepare_fb helper, > but can generalize them so they're also useful for display+render > drivers which use gem_bo.resv to track their own rendering for their > own scanout buffers. 1-3 are: Reviewed-by: Eric Anholt signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 12/12] drm/vc4: hdmi: Set default state margin at reset
Maxime Ripard writes: > Now that the TV margins are properly parsed and filled into > drm_cmdline_mode, we just need to initialise the first state at reset to > get those values and start using them. > > Reviewed-by: Noralf Trønnes > Signed-off-by: Maxime Ripard Acked-by: Eric Anholt signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] vc4: no need to check return value of debugfs_create functions
Greg Kroah-Hartman writes: > When calling debugfs functions, there is no need to ever check the > return value. The function can work or not, but the code logic should > never do something different based on this. Applied to drm-misc-next. Thanks! signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/59] drm/todo: Improve drm_gem_object funcs todo
Daniel Vetter writes: > We're kinda going in the wrong direction. Spotted while typing better > gem/prime docs. > > Cc: Thomas Zimmermann > Cc: Gerd Hoffmann > Cc: Rob Herring > Cc: Noralf Trønnes > Signed-off-by: Daniel Vetter That's a big series, but a great cleanup. I took a look at a lot of it. Patch 1-2, 4-10, 41-47, 49-50, and all the gem_prime_import/export drop patches are: Reviewed-by: Eric Anholt I don't currently have a plan for reading the shuffle in patch 3. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/gem_shmem: Use a writecombine mapping for ->vaddr
Daniel Vetter writes: > On Fri, May 31, 2019 at 08:46:58AM -0700, Eric Anholt wrote: >> Boris Brezillon writes: >> >> > Right now, the BO is mapped as a cached region when ->vmap() is called >> > and the underlying object is not a dmabuf. >> > Doing that makes cache management a bit more complicated (you'd need >> > to call dma_map/unmap_sg() on the ->sgt field everytime the BO is about >> > to be passed to the GPU/CPU), so let's map the BO with writecombine >> > attributes instead (as done in most drivers). >> > >> > Signed-off-by: Boris Brezillon >> > --- >> > Found this issue while working on panfrost perfcnt where the GPU dumps >> > perf counter values in memory and the CPU reads them back in >> > kernel-space. This patch seems to solve the unpredictable behavior I >> > had. >> > >> > I can also go for the other option (call dma_map/unmap/_sg() when >> > needed) if you think that's more appropriate. >> >> writecombined was the intent, and this makes kernel vmap match the >> userspace mmap path. > > Since I missed that obviously: Where do the shmem helpers set write > combined mode for userspace mmap? That was the trick when I asked the question, too. drm_gem.c is what sets WC by default. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/gem_shmem: Use a writecombine mapping for ->vaddr
Boris Brezillon writes: > Right now, the BO is mapped as a cached region when ->vmap() is called > and the underlying object is not a dmabuf. > Doing that makes cache management a bit more complicated (you'd need > to call dma_map/unmap_sg() on the ->sgt field everytime the BO is about > to be passed to the GPU/CPU), so let's map the BO with writecombine > attributes instead (as done in most drivers). > > Signed-off-by: Boris Brezillon > --- > Found this issue while working on panfrost perfcnt where the GPU dumps > perf counter values in memory and the CPU reads them back in > kernel-space. This patch seems to solve the unpredictable behavior I > had. > > I can also go for the other option (call dma_map/unmap/_sg() when > needed) if you think that's more appropriate. writecombined was the intent, and this makes kernel vmap match the userspace mmap path. Reviewed-by: Eric Anholt signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2 v2] drm/doc: Allow new UAPI to be used once it's in drm-next/drm-misc-next.
Daniel Vetter writes: > On Wed, Apr 24, 2019 at 03:06:38PM -0700, Eric Anholt wrote: >> I was trying to figure out if it was permissible to merge the Mesa >> side of V3D's CSD support yet while it's in drm-misc-next but not >> drm-next, and developers on #dri-devel IRC had differing opinions of >> what the requirement was. >> >> v2: Restrict to just drm-next or drm-misc-next on airlied's request. > > Personally I think that's a bit too strict (if people want to screw up, > they will be able to anyway). But since I'm all for clearer rules where > possible, this has my support too. > > Reviewed-by: Daniel Vetter Pushed to drm-misc-next now. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Fix drm.h uapi header for GNU/kFreeBSD
Eric Anholt writes: > [ Unknown signature status ] > James Clarke writes: > >> Like GNU/Linux, GNU/kFreeBSD's sys/types.h does not define the uintX_t >> types, which differs from the BSDs' headers. Thus we should include >> stdint.h to ensure we have all the required integer types. >> >> Signed-off-by: James Clarke > > Reviewed-by: Eric Anholt And pushed to drm-misc-next now. signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v9 0/4] drm/vc4: Binner BO management improvements
Paul Kocialkowski writes: > Changes sinve v8: > * Added collected Reviewed-by; > * Fixed up another problematic case as discussed on v8. I think this is ready to go. Thanks! signature.asc Description: PGP signature
Re: [PATCH v8 4/4] drm/vc4: Allocate binner bo when starting to use the V3D
Paul Kocialkowski writes:
> The binner BO is not required until the V3D is in use, so avoid
> allocating it at probe and do it on the first non-dumb BO allocation.
>
> Keep track of which clients are using the V3D and liberate the buffer
> when there is none left, using a kref. Protect the logic with a
> mutex to avoid race conditions.
>
> The binner BO is created at the time of the first render ioctl and is
> destroyed when there is no client and no exec job using it left.
>
> The Out-Of-Memory (OOM) interrupt also gets some tweaking, to avoid
> enabling it before having allocated a binner bo.
>
> We also want to keep the BO alive during runtime suspend/resume to avoid
> failing to allocate it at resume. This happens when the CMA pool is
> full at that point and results in a hard crash.
>
> Signed-off-by: Paul Kocialkowski
> @@ -313,6 +321,49 @@ static int bin_bo_alloc(struct vc4_dev *vc4)
> return ret;
> }
>
> +int vc4_v3d_bin_bo_get(struct vc4_dev *vc4, bool *used)
> +{
> + int ret = 0;
> +
> + mutex_lock(>bin_bo_lock);
> +
> + if (used && *used)
> + goto complete;
> +
> + if (used)
> + *used = true;
> +
> + if (vc4->bin_bo) {
> + kref_get(>bin_bo_kref);
> + goto complete;
> + }
> +
> + ret = bin_bo_alloc(vc4);
I think this block wants to be:
if (vc4->bin_bo)
kref_get(>bin_bo_kref);
else
ret = bin_bo_alloc(vc4);
if (ret == 0 && used)
*used = true;
(so we don't flag used if bin_bo_alloc fails)
If you agree, then the series is:
Reviewed-by: Eric Anholt
> +
> +complete:
> + mutex_unlock(>bin_bo_lock);
> +
> + return ret;
> +}
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: Fix drm.h uapi header for GNU/kFreeBSD
James Clarke writes: > Like GNU/Linux, GNU/kFreeBSD's sys/types.h does not define the uintX_t > types, which differs from the BSDs' headers. Thus we should include > stdint.h to ensure we have all the required integer types. > > Signed-off-by: James Clarke Reviewed-by: Eric Anholt signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
