Re: [Freedreno] [PATCH 1/3] drm/msm/dpu: Read previously-uninitialized SSPP scaler version from hw
On 2023-02-16 18:34:43, Dmitry Baryshkov wrote:
> On 16/02/2023 10:31, Marijn Suijten wrote:
> > On 2023-02-16 04:22:13, Dmitry Baryshkov wrote:
> >> On Thu, 16 Feb 2023 at 01:02, Marijn Suijten
> >> wrote:
> >>>
> >>> DPU's catalog never assigned dpu_scaler_blk::version leading to
> >>> initialization code in dpu_hw_setup_scaler3 to wander the wrong
> >>> codepaths. Instead of hardcoding the correct QSEED algorithm version,
> >>> read it back from a hardware register.
> >>>
> >>> Note that this register is only available starting with QSEED3, where
> >>> 0x1002 corresponds to QSEED3, 0x2004 to QSEED3LITE and 0x3000 to QSEED4.
> >>
> >> This is not purely accurate. 0x1003 (sdm845) also corresponds to QSEED3.
> >> I'd say instead that there are several variations of QSEED3 scalers,
> >> where starting from 0x2004 it is called QSEED3LITE and starting from
> >> 0x3000 it is called QSEED4.
> >
> > Good catch, I'll update that.
> >
> >>> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> >>> Signed-off-by: Marijn Suijten
> >>> ---
> >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
> >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c| 8 +++-
> >>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h| 3 +++
> >>> 3 files changed, 10 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >>> index ddab9caebb18..96ce1766f4a1 100644
> >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> >>> @@ -324,11 +324,9 @@ struct dpu_src_blk {
> >>> /**
> >>>* struct dpu_scaler_blk: Scaler information
> >>>* @info: HW register and features supported by this sub-blk
> >>> - * @version: qseed block revision
> >>>*/
> >>> struct dpu_scaler_blk {
> >>> DPU_HW_SUBBLK_INFO;
> >>> - u32 version;
> >>
> >> No. Please keep the version in the scaler subblk. It is a version of
> >> the QSEED (scaler block), not the SSPP's version.
> >
> > You are right that the new variable in the parent (SSPP) block is
> > nondescriptive and should have been named scaler_version.
> >
> > However.
> >
> > dpu_scaler_blk is only used as a const static struct in the catalog,
> > meaning we cannot (should not!) store a runtime-read register value
> > here. Instead I followed your IRC suggestion to read the register in
> > dpu_hw_sspp_init, but my original implementation called
> > dpu_hw_get_scaler3_ver in _dpu_hw_sspp_setup_scaler3 where we already
> > have access to the subblk_offset, allowing us to delete
> > _dpu_hw_sspp_get_scaler3_ver. Would you rather have that? We don't
> > need the register value anywhere else.
>
> After giving it another thought, let's follow the vendor's approach and
> store the predefined scaler_version in hw catalog (in dpu_scaler_blk, as
> it currently is). This way we can still drop all QSEED3/3LITE/4
> crazyness, while keeping the data sane.
You want to drop the descriptive #define's, and replace them with magic
0x1002/0x2004/0x3000 and whatever other values we know? That seems
impossible to port without reading back the register value, which we've
only done for a handful of SoCs. I hope I'm misunderstanding you?
After all the vendor approach (in a random 4.14 kernel I have open now)
is to read the register value at runtime but their catalog is also
dynamic and built at runtime based on version ranges and register reads,
which sometimes is more sensible. Ours is const.
> Then _dpu_hw_sspp_get_scaler3_ver() can also be dropped (or you can use
> it as a safety guard while doing dpu_hw_sspp init).
That (safety guard) is exactly what Abhinav requested against, since the
kernel (and our catalog) should be trustworthy. I'll let you two fight
this out and come to a consensus before sending v2.
> >> There is a block called DS (destination scaler), which can be used to
> >> scale the resulting image after the LM. This block also uses the
> >> QSEED3(,LITE,4) scaler block.
> >
> > Is this already supported in mainline, and is it the reason for
> > previously having qseed_type globally available? Is my understanding
> > correct that this scaler subblk in the SSPP is merely an interface to
> > it, allowing the same hardware to be used from the SSPP for intputs and
> > after the LM for outputs?
>
> No, I think qseed_type is a leftover from having the same thing
> implemented in three different ways. Maybe because of NIH syndrome?
Could be, downstream uses it to steer its catalog logic for example
(which happens before later reading the version register).
> DS is not supported, it was removed in the commit
> b033def8741aab3fb58e4bf6c1d5cd73b3beb357. I do not have a clear usecase
> for this block and of course we don't have uABI for it.
Is there no common DRM property to composite at a lower resolution and
upscale the final displayed image to match a CRTC/encoder? I wish I
understood
Re: [Freedreno] [PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()
On Thu, Feb 16, 2023 at 03:06:20PM +0100, Thomas Zimmermann wrote:
> Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the
> calling fbdev implementation. Avoids a possible stale mutex with
> generic fbdev code.
>
> As indicated by its name, drm_fb_helper_prepare() prepares struct
> drm_fb_helper before setting up the fbdev support with a call to
> drm_fb_helper_init(). In legacy fbdev emulation, this happens next
> to each other. If successful, drm_fb_helper_fini() later tear down
> the fbdev device and also unprepare via drm_fb_helper_unprepare().
>
> Generic fbdev emulation prepares struct drm_fb_helper immediately
> after allocating the instance. It only calls drm_fb_helper_init()
> as part of processing a hotplug event. If the hotplug-handling fails,
> it runs drm_fb_helper_fini(). This unprepares the fb-helper instance
> and the next hotplug event runs on stale data.
>
> Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini()
> into the fbdev implementations. Call it right before freeing the
> fb-helper instance.
>
> Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()")
> Cc: Thomas Zimmermann
> Cc: Javier Martinez Canillas
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: dri-de...@lists.freedesktop.org
>
> Signed-off-by: Thomas Zimmermann
This reminds me of an old patch I just recently stumbled over again:
https://lore.kernel.org/dri-devel/Y3St2VHJ7jEmcNFw@phenom.ffwll.local/
Should I resurrect that one maybe and send it out? I think that also ties
a bit into your story here.
> ---
> drivers/gpu/drm/armada/armada_fbdev.c | 3 +++
> drivers/gpu/drm/drm_fb_helper.c| 2 --
> drivers/gpu/drm/drm_fbdev_generic.c| 2 ++
> drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 3 ++-
> drivers/gpu/drm/gma500/framebuffer.c | 2 ++
> drivers/gpu/drm/i915/display/intel_fbdev.c | 1 +
> drivers/gpu/drm/msm/msm_fbdev.c| 2 ++
> drivers/gpu/drm/omapdrm/omap_fbdev.c | 2 ++
> drivers/gpu/drm/radeon/radeon_fb.c | 2 ++
> drivers/gpu/drm/tegra/fb.c | 1 +
> 10 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/armada/armada_fbdev.c
> b/drivers/gpu/drm/armada/armada_fbdev.c
> index 07e410c62b7a..0e44f53e9fa4 100644
> --- a/drivers/gpu/drm/armada/armada_fbdev.c
> +++ b/drivers/gpu/drm/armada/armada_fbdev.c
> @@ -147,6 +147,7 @@ int armada_fbdev_init(struct drm_device *dev)
> err_fb_setup:
> drm_fb_helper_fini(fbh);
> err_fb_helper:
> + drm_fb_helper_unprepare(fbh);
> priv->fbdev = NULL;
> return ret;
> }
> @@ -164,6 +165,8 @@ void armada_fbdev_fini(struct drm_device *dev)
> if (fbh->fb)
> fbh->fb->funcs->destroy(fbh->fb);
>
> + drm_fb_helper_unprepare(fbh);
> +
> priv->fbdev = NULL;
> }
> }
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 28c428e9c530..a39998047f8a 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -590,8 +590,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
I think it would be good to update the kerneldoc of _init() and _fini()
here to mention each another like we usually do with these pairs. Same
with prepare/unprepare() although the latter rerfences _prepare() already.
> }
> mutex_unlock(&kernel_fb_helper_lock);
>
> - drm_fb_helper_unprepare(fb_helper);
> -
> if (!fb_helper->client.funcs)
> drm_client_release(&fb_helper->client);
> }
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c
> b/drivers/gpu/drm/drm_fbdev_generic.c
> index 365f80717fa1..4d6325e91565 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -65,6 +65,8 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
>
> drm_client_framebuffer_delete(fb_helper->buffer);
> drm_client_release(&fb_helper->client);
> +
> + drm_fb_helper_unprepare(fb_helper);
> kfree(fb_helper);
> }
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index b89e33af8da8..4929ffe5a09a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -183,8 +183,8 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
>
> err_setup:
> drm_fb_helper_fini(helper);
> -
> err_init:
> + drm_fb_helper_unprepare(helper);
> private->fb_helper = NULL;
> kfree(fbdev);
>
> @@ -219,6 +219,7 @@ void exynos_drm_fbdev_fini(struct drm_device *dev)
> fbdev = to_exynos_fbdev(private->fb_helper);
>
> exynos_drm_fbdev_destroy(dev, private->fb_helper);
> + drm_fb_helper_unprepare(private->fb_helper);
> kfree(fbdev);
> private->fb_helper = NULL;
> }
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c
> b/drivers/gpu/drm/gma50
Re: [Freedreno] [PATCH] drm/msm: Fix potential invalid ptr free
On 2/16/23 02:50, Rob Clark wrote:
> From: Rob Clark
>
> The error path cleanup expects that chain and syncobj are either NULL or
> valid pointers. But post_deps was not allocated with __GFP_ZERO.
>
> Fixes: ab723b7a992a ("drm/msm: Add syncobj support.")
> Signed-off-by: Rob Clark
> ---
> drivers/gpu/drm/msm/msm_gem_submit.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
> b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 6503220e5a4b..e4d13540300e 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -640,8 +640,8 @@ static struct msm_submit_post_dep
> *msm_parse_post_deps(struct drm_device *dev,
> int ret = 0;
> uint32_t i, j;
>
> - post_deps = kmalloc_array(nr_syncobjs, sizeof(*post_deps),
> - GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
> + post_deps = kcalloc(nr_syncobjs, sizeof(*post_deps),
> + GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
> if (!post_deps)
> return ERR_PTR(-ENOMEM);
>
> @@ -656,7 +656,6 @@ static struct msm_submit_post_dep
> *msm_parse_post_deps(struct drm_device *dev,
> }
>
> post_deps[i].point = syncobj_desc.point;
> - post_deps[i].chain = NULL;
>
> if (syncobj_desc.flags) {
> ret = -EINVAL;
Good catch!
Reviewed-by: Dmitry Osipenko
--
Best regards,
Dmitry
Re: [Freedreno] [PATCH v3 20/27] drm/msm/dpu: add dpu_hw_pipe_cfg to dpu_plane_state
On 06/02/2023 21:07, Abhinav Kumar wrote: On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote: Now as all accesses to pipe_cfg and pstate have been cleaned, re-add struct dpu_hw_pipe_cfg back to dpu_plane_state, so that dpu_plane_atomic_check() and dpu_plane_atomic_update() do not have a chance to disagree about src/dst rectangles (currently dpu_plane_atomic_check() uses unclipped rectangles, while dpu_plane_atomic_update() uses clipped rectangles calculated by drm_atomic_helper_check_plane_state()). The title of the patch should now say "add dpu_hw_sspp_cfg" I have a question on the commit text, why does it say "re-add" and not "add". dpu_hw_pipe_cfg/dpu_hw_sspp_cfg was not a part of dpu_plane_state even before and I dont recall it was removed in this series and then added back. Ack, I'll fix both items in v4. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 64 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 2 + 2 files changed, 30 insertions(+), 36 deletions(-)-- With best wishes Dmitry
Re: [Freedreno] [PATCH] drm/msm: Fix potential invalid ptr free
On 16/02/2023 01:50, Rob Clark wrote:
From: Rob Clark
The error path cleanup expects that chain and syncobj are either NULL or
valid pointers. But post_deps was not allocated with __GFP_ZERO.
Fixes: ab723b7a992a ("drm/msm: Add syncobj support.")
Signed-off-by: Rob Clark
Reviewed-by: Dmitry Baryshkov
This makes me want to rewrite the ab723b7a992a ("drm/msm: Add syncobj
support.") in the usual explicit-error-path way. WDYT?
---
drivers/gpu/drm/msm/msm_gem_submit.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 6503220e5a4b..e4d13540300e 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -640,8 +640,8 @@ static struct msm_submit_post_dep
*msm_parse_post_deps(struct drm_device *dev,
int ret = 0;
uint32_t i, j;
- post_deps = kmalloc_array(nr_syncobjs, sizeof(*post_deps),
- GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
+ post_deps = kcalloc(nr_syncobjs, sizeof(*post_deps),
+ GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
if (!post_deps)
return ERR_PTR(-ENOMEM);
@@ -656,7 +656,6 @@ static struct msm_submit_post_dep *msm_parse_post_deps(struct drm_device *dev,
}
post_deps[i].point = syncobj_desc.point;
- post_deps[i].chain = NULL;
if (syncobj_desc.flags) {
ret = -EINVAL;
--
With best wishes
Dmitry
Re: [Freedreno] [PATCH 1/3] drm/msm/dpu: Read previously-uninitialized SSPP scaler version from hw
On 16/02/2023 10:31, Marijn Suijten wrote:
On 2023-02-16 04:22:13, Dmitry Baryshkov wrote:
On Thu, 16 Feb 2023 at 01:02, Marijn Suijten
wrote:
DPU's catalog never assigned dpu_scaler_blk::version leading to
initialization code in dpu_hw_setup_scaler3 to wander the wrong
codepaths. Instead of hardcoding the correct QSEED algorithm version,
read it back from a hardware register.
Note that this register is only available starting with QSEED3, where
0x1002 corresponds to QSEED3, 0x2004 to QSEED3LITE and 0x3000 to QSEED4.
This is not purely accurate. 0x1003 (sdm845) also corresponds to QSEED3.
I'd say instead that there are several variations of QSEED3 scalers,
where starting from 0x2004 it is called QSEED3LITE and starting from
0x3000 it is called QSEED4.
Good catch, I'll update that.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Marijn Suijten
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c| 8 +++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h| 3 +++
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index ddab9caebb18..96ce1766f4a1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -324,11 +324,9 @@ struct dpu_src_blk {
/**
* struct dpu_scaler_blk: Scaler information
* @info: HW register and features supported by this sub-blk
- * @version: qseed block revision
*/
struct dpu_scaler_blk {
DPU_HW_SUBBLK_INFO;
- u32 version;
No. Please keep the version in the scaler subblk. It is a version of
the QSEED (scaler block), not the SSPP's version.
You are right that the new variable in the parent (SSPP) block is
nondescriptive and should have been named scaler_version.
However.
dpu_scaler_blk is only used as a const static struct in the catalog,
meaning we cannot (should not!) store a runtime-read register value
here. Instead I followed your IRC suggestion to read the register in
dpu_hw_sspp_init, but my original implementation called
dpu_hw_get_scaler3_ver in _dpu_hw_sspp_setup_scaler3 where we already
have access to the subblk_offset, allowing us to delete
_dpu_hw_sspp_get_scaler3_ver. Would you rather have that? We don't
need the register value anywhere else.
After giving it another thought, let's follow the vendor's approach and
store the predefined scaler_version in hw catalog (in dpu_scaler_blk, as
it currently is). This way we can still drop all QSEED3/3LITE/4
crazyness, while keeping the data sane.
Then _dpu_hw_sspp_get_scaler3_ver() can also be dropped (or you can use
it as a safety guard while doing dpu_hw_sspp init).
There is a block called DS (destination scaler), which can be used to
scale the resulting image after the LM. This block also uses the
QSEED3(,LITE,4) scaler block.
Is this already supported in mainline, and is it the reason for
previously having qseed_type globally available? Is my understanding
correct that this scaler subblk in the SSPP is merely an interface to
it, allowing the same hardware to be used from the SSPP for intputs and
after the LM for outputs?
No, I think qseed_type is a leftover from having the same thing
implemented in three different ways. Maybe because of NIH syndrome?
DS is not supported, it was removed in the commit
b033def8741aab3fb58e4bf6c1d5cd73b3beb357. I do not have a clear usecase
for this block and of course we don't have uABI for it.
It would still be nice to keep it in the picture though. It was the main
reason for moving scaler code from dpu_hw_sspp to dpu_hw_util.
- Marijn
--
With best wishes
Dmitry
[Freedreno] [PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()
Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the
calling fbdev implementation. Avoids a possible stale mutex with
generic fbdev code.
As indicated by its name, drm_fb_helper_prepare() prepares struct
drm_fb_helper before setting up the fbdev support with a call to
drm_fb_helper_init(). In legacy fbdev emulation, this happens next
to each other. If successful, drm_fb_helper_fini() later tear down
the fbdev device and also unprepare via drm_fb_helper_unprepare().
Generic fbdev emulation prepares struct drm_fb_helper immediately
after allocating the instance. It only calls drm_fb_helper_init()
as part of processing a hotplug event. If the hotplug-handling fails,
it runs drm_fb_helper_fini(). This unprepares the fb-helper instance
and the next hotplug event runs on stale data.
Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini()
into the fbdev implementations. Call it right before freeing the
fb-helper instance.
Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()")
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: Maarten Lankhorst
Cc: Maxime Ripard
Cc: David Airlie
Cc: Daniel Vetter
Cc: dri-de...@lists.freedesktop.org
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/armada/armada_fbdev.c | 3 +++
drivers/gpu/drm/drm_fb_helper.c| 2 --
drivers/gpu/drm/drm_fbdev_generic.c| 2 ++
drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 3 ++-
drivers/gpu/drm/gma500/framebuffer.c | 2 ++
drivers/gpu/drm/i915/display/intel_fbdev.c | 1 +
drivers/gpu/drm/msm/msm_fbdev.c| 2 ++
drivers/gpu/drm/omapdrm/omap_fbdev.c | 2 ++
drivers/gpu/drm/radeon/radeon_fb.c | 2 ++
drivers/gpu/drm/tegra/fb.c | 1 +
10 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_fbdev.c
b/drivers/gpu/drm/armada/armada_fbdev.c
index 07e410c62b7a..0e44f53e9fa4 100644
--- a/drivers/gpu/drm/armada/armada_fbdev.c
+++ b/drivers/gpu/drm/armada/armada_fbdev.c
@@ -147,6 +147,7 @@ int armada_fbdev_init(struct drm_device *dev)
err_fb_setup:
drm_fb_helper_fini(fbh);
err_fb_helper:
+ drm_fb_helper_unprepare(fbh);
priv->fbdev = NULL;
return ret;
}
@@ -164,6 +165,8 @@ void armada_fbdev_fini(struct drm_device *dev)
if (fbh->fb)
fbh->fb->funcs->destroy(fbh->fb);
+ drm_fb_helper_unprepare(fbh);
+
priv->fbdev = NULL;
}
}
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 28c428e9c530..a39998047f8a 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -590,8 +590,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
}
mutex_unlock(&kernel_fb_helper_lock);
- drm_fb_helper_unprepare(fb_helper);
-
if (!fb_helper->client.funcs)
drm_client_release(&fb_helper->client);
}
diff --git a/drivers/gpu/drm/drm_fbdev_generic.c
b/drivers/gpu/drm/drm_fbdev_generic.c
index 365f80717fa1..4d6325e91565 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -65,6 +65,8 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
drm_client_framebuffer_delete(fb_helper->buffer);
drm_client_release(&fb_helper->client);
+
+ drm_fb_helper_unprepare(fb_helper);
kfree(fb_helper);
}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index b89e33af8da8..4929ffe5a09a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -183,8 +183,8 @@ int exynos_drm_fbdev_init(struct drm_device *dev)
err_setup:
drm_fb_helper_fini(helper);
-
err_init:
+ drm_fb_helper_unprepare(helper);
private->fb_helper = NULL;
kfree(fbdev);
@@ -219,6 +219,7 @@ void exynos_drm_fbdev_fini(struct drm_device *dev)
fbdev = to_exynos_fbdev(private->fb_helper);
exynos_drm_fbdev_destroy(dev, private->fb_helper);
+ drm_fb_helper_unprepare(private->fb_helper);
kfree(fbdev);
private->fb_helper = NULL;
}
diff --git a/drivers/gpu/drm/gma500/framebuffer.c
b/drivers/gpu/drm/gma500/framebuffer.c
index 1f04c07ee180..f471e0cb7298 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -427,6 +427,7 @@ int psb_fbdev_init(struct drm_device *dev)
fini:
drm_fb_helper_fini(fb_helper);
free:
+ drm_fb_helper_unprepare(fb_helper);
kfree(fb_helper);
return ret;
}
@@ -439,6 +440,7 @@ static void psb_fbdev_fini(struct drm_device *dev)
return;
psb_fbdev_destroy(dev, dev_priv->fb_helper);
+ drm_fb_helper_unprepare(dev_priv->fb_helper);
kfree(dev_priv->fb_helper);
dev_priv->fb_helper = NULL;
}
diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c
b/drivers/gpu/drm/i915/dis
[Freedreno] [PATCH] dt-bindings: display: msm: sm6115-mdss: Fix DSI compatible
Since the DSI autodetection is bound to work correctly on 6115 now, switch to using the correct per-SoC + generic fallback compatible combo. Signed-off-by: Konrad Dybcio --- Depends on (and should have been a part of): https://lore.kernel.org/linux-arm-msm/20230213121012.1768296-1-konrad.dyb...@linaro.org/ .../devicetree/bindings/display/msm/qcom,sm6115-mdss.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/msm/qcom,sm6115-mdss.yaml b/Documentation/devicetree/bindings/display/msm/qcom,sm6115-mdss.yaml index 2491cb100b33..146d3e36d1c9 100644 --- a/Documentation/devicetree/bindings/display/msm/qcom,sm6115-mdss.yaml +++ b/Documentation/devicetree/bindings/display/msm/qcom,sm6115-mdss.yaml @@ -40,7 +40,9 @@ patternProperties: type: object properties: compatible: -const: qcom,dsi-ctrl-6g-qcm2290 +items: + - const: qcom,sm6115-dsi-ctrl + - const: qcom,mdss-dsi-ctrl "^phy@[0-9a-f]+$": type: object -- 2.39.1
[Freedreno] [PATCH] drm/atomic-helpers: remove legacy_cursor_update hacks
The stuff never really worked, and leads to lots of fun because it
out-of-order frees atomic states. Which upsets KASAN, among other
things.
For async updates we now have a more solid solution with the
->atomic_async_check and ->atomic_async_commit hooks. Support for that
for msm and vc4 landed. nouveau and i915 have their own commit
routines, doing something similar.
For everyone else it's probably better to remove the use-after-free
bug, and encourage folks to use the async support instead. The
affected drivers which register a legacy cursor plane and don't either
use the new async stuff or their own commit routine are: amdgpu,
atmel, mediatek, qxl, rockchip, sti, sun4i, tegra, virtio, and vmwgfx.
Inspired by an amdgpu bug report.
v2: Drop RFC, I think with amdgpu converted over to use
atomic_async_check/commit done in
commit 674e78acae0dfb4beb56132e41cbae5b60f7d662
Author: Nicholas Kazlauskas
Date: Wed Dec 5 14:59:07 2018 -0500
drm/amd/display: Add fast path for cursor plane updates
we don't have any driver anymore where we have userspace expecting
solid legacy cursor support _and_ they are using the atomic helpers in
their fully glory. So we can retire this.
v3: Paper over msm and i915 regression. The complete_all is the only
thing missing afaict.
v4: Fixup i915 fixup ...
v5: Unallocate the crtc->event in msm to avoid hitting a WARN_ON in
dpu_crtc_atomic_flush(). This is a bit a hack, but simplest way to
untangle this all. Thanks to Abhinav Kumar for the debug help.
Cc: Abhinav Kumar
Cc: Thomas Zimmermann
Cc: Maxime Ripard
References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
References:
https://lore.kernel.org/all/20220221134155.125447-9-max...@cerno.tech/
References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
Cc: Maxime Ripard
Tested-by: Maxime Ripard
Cc: mikita.lip...@amd.com
Cc: Michel Dänzer
Cc: harry.wentl...@amd.com
Cc: Rob Clark
Cc: "Kazlauskas, Nicholas"
Cc: Dmitry Osipenko
Cc: Maarten Lankhorst
Cc: Dmitry Baryshkov
Cc: Sean Paul
Cc: Matthias Brugger
Cc: AngeloGioacchino Del Regno
Cc: "Ville Syrjälä"
Cc: Jani Nikula
Cc: Lucas De Marchi
Cc: Imre Deak
Cc: Manasi Navare
Cc: linux-arm-...@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-media...@lists.infradead.org
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_atomic_helper.c | 13 -
drivers/gpu/drm/i915/display/intel_display.c | 14 ++
drivers/gpu/drm/msm/msm_atomic.c | 15 +++
3 files changed, 29 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index d579fd8f7cb8..f6b4c3a00684 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1587,13 +1587,6 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device
*dev,
int i, ret;
unsigned int crtc_mask = 0;
-/*
- * Legacy cursor ioctls are completely unsynced, and userspace
- * relies on that (by doing tons of cursor updates).
- */
- if (old_state->legacy_cursor_update)
- return;
-
for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state,
new_crtc_state, i) {
if (!new_crtc_state->active)
continue;
@@ -2244,12 +2237,6 @@ int drm_atomic_helper_setup_commit(struct
drm_atomic_state *state,
continue;
}
- /* Legacy cursor updates are fully unsynced. */
- if (state->legacy_cursor_update) {
- complete_all(&commit->flip_done);
- continue;
- }
-
if (!new_crtc_state->event) {
commit->event = kzalloc(sizeof(*commit->event),
GFP_KERNEL);
diff --git a/drivers/gpu/drm/i915/display/intel_display.c
b/drivers/gpu/drm/i915/display/intel_display.c
index 3479125fbda6..2454451fcf95 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -7651,6 +7651,20 @@ static int intel_atomic_commit(struct drm_device *dev,
intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref);
return ret;
}
+
+ /*
+* FIXME: Cut over to (async) commit helpers instead of hand-rolling
+* everything.
+*/
+ if (state->base.legacy_cursor_update) {
+ struct intel_crtc_state *new_crtc_state;
+ struct intel_crtc *crtc;
+ int i;
+
+ for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
+ complete_all(&new_crtc_state->uapi.commit->flip_done);
+ }
+
intel_shared_dpll_swap_state(state);
intel_atomic_track_fbs(state);
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gp
Re: [Freedreno] [PATCH 0/3] drm/msm/dpu: Initialize SSPP scaler version (from register read)
On 2023-02-16 05:02:32, Dmitry Baryshkov wrote: > On Thu, 16 Feb 2023 at 01:02, Marijn Suijten > wrote: > > > > Random inspection of the SSPP code surfaced that the version field of > > dpu_scaler_blk was never assigned in the catalog, resulting in wrong > > codepaths to be taken within dpu_hw_setup_scaler3 based on a 0 version. > > Rectify this by reading an accurate value from a register (that is not > > equal to the values represented by DPU_SSPP_SCALER_QSEEDx enum > > variants) and deleting dead code around QSEED versioning. > > > > Future changes should likely get rid of the distinction between QSEED3 > > and up, as these are now purely determined from the register value. > > Furthermore implementations could look at the scaler subblk .id field > > rather than the SSPP feature bits, which currently hold redundant > > information. > > > > --- > > Marijn Suijten (3): > > drm/msm/dpu: Read previously-uninitialized SSPP scaler version from hw > > drm/msm/dpu: Drop unused get_scaler_ver callback from SSPP > > drm/msm/dpu: Drop unused qseed_type from catalog dpu_caps > > The cleanup looks good. However as you are on it, maybe you can also > add patch 4, dropping DPU_SSPP_SCALER_QSEED3LITE and > DPU_SSPP_SCALER_QSEED4 in favour of using QSEED3 for all these > scalers? I surely can! Do you mind if I rename it to QSEED3_AND_UP for clarity? How about the second question, dropping this redundant information from the SSPP feature flags and only looking at the scaler_blk.id? > As we are going to use scaler_version to distinguish between > them, it would be logical not to duplicate that bit of information > (not to mention all the possible troubles if scaler_version disagrees > with the sblk->scaler_blk.id). Note that we had a similar discussion for UBWC HW decoder version and it was decided to go the opposite route [1]. That may have been for technical reasons (unclocked register access), but it's an inconsistent approach to say the least. [1]: https://lore.kernel.org/linux-arm-msm/71f96910-e7b1-92f9-ae15-79bd1da40...@quicinc.com/ - Marijn
Re: [Freedreno] [PATCH 1/3] drm/msm/dpu: Read previously-uninitialized SSPP scaler version from hw
On 2023-02-16 04:22:13, Dmitry Baryshkov wrote:
> On Thu, 16 Feb 2023 at 01:02, Marijn Suijten
> wrote:
> >
> > DPU's catalog never assigned dpu_scaler_blk::version leading to
> > initialization code in dpu_hw_setup_scaler3 to wander the wrong
> > codepaths. Instead of hardcoding the correct QSEED algorithm version,
> > read it back from a hardware register.
> >
> > Note that this register is only available starting with QSEED3, where
> > 0x1002 corresponds to QSEED3, 0x2004 to QSEED3LITE and 0x3000 to QSEED4.
>
> This is not purely accurate. 0x1003 (sdm845) also corresponds to QSEED3.
> I'd say instead that there are several variations of QSEED3 scalers,
> where starting from 0x2004 it is called QSEED3LITE and starting from
> 0x3000 it is called QSEED4.
Good catch, I'll update that.
> > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> > Signed-off-by: Marijn Suijten
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 2 --
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.c| 8 +++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_hw_sspp.h| 3 +++
> > 3 files changed, 10 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > index ddab9caebb18..96ce1766f4a1 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> > @@ -324,11 +324,9 @@ struct dpu_src_blk {
> > /**
> > * struct dpu_scaler_blk: Scaler information
> > * @info: HW register and features supported by this sub-blk
> > - * @version: qseed block revision
> > */
> > struct dpu_scaler_blk {
> > DPU_HW_SUBBLK_INFO;
> > - u32 version;
>
> No. Please keep the version in the scaler subblk. It is a version of
> the QSEED (scaler block), not the SSPP's version.
You are right that the new variable in the parent (SSPP) block is
nondescriptive and should have been named scaler_version.
However.
dpu_scaler_blk is only used as a const static struct in the catalog,
meaning we cannot (should not!) store a runtime-read register value
here. Instead I followed your IRC suggestion to read the register in
dpu_hw_sspp_init, but my original implementation called
dpu_hw_get_scaler3_ver in _dpu_hw_sspp_setup_scaler3 where we already
have access to the subblk_offset, allowing us to delete
_dpu_hw_sspp_get_scaler3_ver. Would you rather have that? We don't
need the register value anywhere else.
> There is a block called DS (destination scaler), which can be used to
> scale the resulting image after the LM. This block also uses the
> QSEED3(,LITE,4) scaler block.
Is this already supported in mainline, and is it the reason for
previously having qseed_type globally available? Is my understanding
correct that this scaler subblk in the SSPP is merely an interface to
it, allowing the same hardware to be used from the SSPP for intputs and
after the LM for outputs?
- Marijn
