This series fixes a reset of state in duplicate state crtc funcs for VOP
driver, a possible crash and ensure crtc reset helper is called in VOP2
driver.
Patch 1 use kmemdup instead of kzalloc to duplicate the crtc state.
Patch 2 change to use crtc and plane cleanup helpers directly.
Patch 3 adds a crash guard for empty crtc state.
Patch 4 adds a missing call to crtc reset helper.
This is the next part of an ongoing effort to upstream HDMI 2.0 support
used in LibreELEC for the past 3+ years.
This series is also available at [1].
[1] https://github.com/Kwiboo/linux-rockchip/commits/next-20230619-duplicate-state
Jonas Karlman (4):
drm/rockchip: vop: Fix crtc duplicate state
drm/rockchip: vop: User cleanup helpers directly as destroy ops
drm/rockchip: vop2: Don't crash for invalid duplicate_state
drm/rockchip: vop2: Add missing call to crtc reset helper
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 19 +++--------
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 36 +++++++++-----------
2 files changed, 21 insertions(+), 34 deletions(-)
--
2.41.0
vop_plane_destroy and vop_crtc_destroy are plain wrappers around
drm_plane_cleanup and drm_crtc_cleanup. Use them directly as plane and
crtc funcs to closer match VOP2 driver.
Signed-off-by: Jonas Karlman <[email protected]>
---
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 60b23636a3fe..25c873d4ff53 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -766,11 +766,6 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
}
}
-static void vop_plane_destroy(struct drm_plane *plane)
-{
- drm_plane_cleanup(plane);
-}
-
static inline bool rockchip_afbc(u64 modifier)
{
return modifier == ROCKCHIP_AFBC_MOD;
@@ -1131,7 +1126,7 @@ static const struct drm_plane_helper_funcs plane_helper_funcs = {
static const struct drm_plane_funcs vop_plane_funcs = {
.update_plane = drm_atomic_helper_update_plane,
.disable_plane = drm_atomic_helper_disable_plane,
- .destroy = vop_plane_destroy,
+ .destroy = drm_plane_cleanup,
.reset = drm_atomic_helper_plane_reset,
.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
@@ -1602,11 +1597,6 @@ static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
.atomic_disable = vop_crtc_atomic_disable,
};
-static void vop_crtc_destroy(struct drm_crtc *crtc)
-{
- drm_crtc_cleanup(crtc);
-}
-
static struct drm_crtc_state *vop_crtc_duplicate_state(struct drm_crtc *crtc)
{
struct rockchip_crtc_state *rockchip_state;
@@ -1711,7 +1701,7 @@ vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
static const struct drm_crtc_funcs vop_crtc_funcs = {
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
- .destroy = vop_crtc_destroy,
+ .destroy = drm_crtc_cleanup,
.reset = vop_crtc_reset,
.atomic_duplicate_state = vop_crtc_duplicate_state,
.atomic_destroy_state = vop_crtc_destroy_state,
@@ -1962,7 +1952,7 @@ static void vop_destroy_crtc(struct vop *vop)
*/
list_for_each_entry_safe(plane, tmp, &drm_dev->mode_config.plane_list,
head)
- vop_plane_destroy(plane);
+ drm_plane_cleanup(plane);
/*
* Destroy CRTC after vop_plane_destroy() since vop_disable_plane()
--
2.41.0
It's possible for users to try to duplicate the CRTC state even when the
state doesn't exist. drm_atomic_helper_crtc_duplicate_state() (and other
users of __drm_atomic_helper_crtc_duplicate_state()) already guard this
with a WARN_ON() instead of crashing, so let's do that here too.
Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")
Signed-off-by: Jonas Karlman <[email protected]>
---
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index ca73b8ccc29f..f725487d02ef 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2099,11 +2099,13 @@ static void vop2_crtc_reset(struct drm_crtc *crtc)
static struct drm_crtc_state *vop2_crtc_duplicate_state(struct drm_crtc *crtc)
{
- struct rockchip_crtc_state *vcstate, *old_vcstate;
+ struct rockchip_crtc_state *vcstate;
- old_vcstate = to_rockchip_crtc_state(crtc->state);
+ if (WARN_ON(!crtc->state))
+ return NULL;
- vcstate = kmemdup(old_vcstate, sizeof(*old_vcstate), GFP_KERNEL);
+ vcstate = kmemdup(to_rockchip_crtc_state(crtc->state),
+ sizeof(*vcstate), GFP_KERNEL);
if (!vcstate)
return NULL;
--
2.41.0
struct rockchip_crtc_state members such as output_type, output_bpc and
enable_afbc is always reset to zero in the atomic_duplicate_state crtc
funcs.
Fix this by using kmemdup on the subclass rockchip_crtc_state struct.
Fixes: 4e257d9eee23 ("drm/rockchip: get rid of rockchip_drm_crtc_mode_config")
Signed-off-by: Jonas Karlman <[email protected]>
---
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index a530ecc4d207..60b23636a3fe 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -1614,7 +1614,8 @@ static struct drm_crtc_state *vop_crtc_duplicate_state(struct drm_crtc *crtc)
if (WARN_ON(!crtc->state))
return NULL;
- rockchip_state = kzalloc(sizeof(*rockchip_state), GFP_KERNEL);
+ rockchip_state = kmemdup(to_rockchip_crtc_state(crtc->state),
+ sizeof(*rockchip_state), GFP_KERNEL);
if (!rockchip_state)
return NULL;
--
2.41.0
Add missing call to crtc reset helper to properly vblank reset.
Also move vop2_crtc_reset and call vop2_crtc_destroy_state to simplify
and remove duplicated code.
Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")
Signed-off-by: Jonas Karlman <[email protected]>
---
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 28 ++++++++------------
1 file changed, 11 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index f725487d02ef..1be84fe0208f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2080,23 +2080,6 @@ static const struct drm_crtc_helper_funcs vop2_crtc_helper_funcs = {
.atomic_disable = vop2_crtc_atomic_disable,
};
-static void vop2_crtc_reset(struct drm_crtc *crtc)
-{
- struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(crtc->state);
-
- if (crtc->state) {
- __drm_atomic_helper_crtc_destroy_state(crtc->state);
- kfree(vcstate);
- }
-
- vcstate = kzalloc(sizeof(*vcstate), GFP_KERNEL);
- if (!vcstate)
- return;
-
- crtc->state = &vcstate->base;
- crtc->state->crtc = crtc;
-}
-
static struct drm_crtc_state *vop2_crtc_duplicate_state(struct drm_crtc *crtc)
{
struct rockchip_crtc_state *vcstate;
@@ -2123,6 +2106,17 @@ static void vop2_crtc_destroy_state(struct drm_crtc *crtc,
kfree(vcstate);
}
+static void vop2_crtc_reset(struct drm_crtc *crtc)
+{
+ struct rockchip_crtc_state *vcstate =
+ kzalloc(sizeof(*vcstate), GFP_KERNEL);
+
+ if (crtc->state)
+ vop2_crtc_destroy_state(crtc, crtc->state);
+
+ __drm_atomic_helper_crtc_reset(crtc, &vcstate->base);
+}
+
static const struct drm_crtc_funcs vop2_crtc_funcs = {
.set_config = drm_atomic_helper_set_config,
.page_flip = drm_atomic_helper_page_flip,
--
2.41.0
On Tue, Jun 20, 2023 at 06:47:37AM +0000, Jonas Karlman wrote:
> vop_plane_destroy and vop_crtc_destroy are plain wrappers around
> drm_plane_cleanup and drm_crtc_cleanup. Use them directly as plane and
> crtc funcs to closer match VOP2 driver.
>
> Signed-off-by: Jonas Karlman <[email protected]>
Reviewed-by: Sascha Hauer <[email protected]>
Sascha
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 16 +++-------------
> 1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 60b23636a3fe..25c873d4ff53 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -766,11 +766,6 @@ static void vop_crtc_atomic_disable(struct drm_crtc *crtc,
> }
> }
>
> -static void vop_plane_destroy(struct drm_plane *plane)
> -{
> - drm_plane_cleanup(plane);
> -}
> -
> static inline bool rockchip_afbc(u64 modifier)
> {
> return modifier == ROCKCHIP_AFBC_MOD;
> @@ -1131,7 +1126,7 @@ static const struct drm_plane_helper_funcs plane_helper_funcs = {
> static const struct drm_plane_funcs vop_plane_funcs = {
> .update_plane = drm_atomic_helper_update_plane,
> .disable_plane = drm_atomic_helper_disable_plane,
> - .destroy = vop_plane_destroy,
> + .destroy = drm_plane_cleanup,
> .reset = drm_atomic_helper_plane_reset,
> .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> @@ -1602,11 +1597,6 @@ static const struct drm_crtc_helper_funcs vop_crtc_helper_funcs = {
> .atomic_disable = vop_crtc_atomic_disable,
> };
>
> -static void vop_crtc_destroy(struct drm_crtc *crtc)
> -{
> - drm_crtc_cleanup(crtc);
> -}
> -
> static struct drm_crtc_state *vop_crtc_duplicate_state(struct drm_crtc *crtc)
> {
> struct rockchip_crtc_state *rockchip_state;
> @@ -1711,7 +1701,7 @@ vop_crtc_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> static const struct drm_crtc_funcs vop_crtc_funcs = {
> .set_config = drm_atomic_helper_set_config,
> .page_flip = drm_atomic_helper_page_flip,
> - .destroy = vop_crtc_destroy,
> + .destroy = drm_crtc_cleanup,
> .reset = vop_crtc_reset,
> .atomic_duplicate_state = vop_crtc_duplicate_state,
> .atomic_destroy_state = vop_crtc_destroy_state,
> @@ -1962,7 +1952,7 @@ static void vop_destroy_crtc(struct vop *vop)
> */
> list_for_each_entry_safe(plane, tmp, &drm_dev->mode_config.plane_list,
> head)
> - vop_plane_destroy(plane);
> + drm_plane_cleanup(plane);
>
> /*
> * Destroy CRTC after vop_plane_destroy() since vop_disable_plane()
> --
> 2.41.0
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, Jun 20, 2023 at 06:47:36AM +0000, Jonas Karlman wrote:
> struct rockchip_crtc_state members such as output_type, output_bpc and
> enable_afbc is always reset to zero in the atomic_duplicate_state crtc
> funcs.
>
> Fix this by using kmemdup on the subclass rockchip_crtc_state struct.
>
> Fixes: 4e257d9eee23 ("drm/rockchip: get rid of rockchip_drm_crtc_mode_config")
> Signed-off-by: Jonas Karlman <[email protected]>
Reviewed-by: Sascha Hauer <[email protected]>
Sascha
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index a530ecc4d207..60b23636a3fe 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -1614,7 +1614,8 @@ static struct drm_crtc_state *vop_crtc_duplicate_state(struct drm_crtc *crtc)
> if (WARN_ON(!crtc->state))
> return NULL;
>
> - rockchip_state = kzalloc(sizeof(*rockchip_state), GFP_KERNEL);
> + rockchip_state = kmemdup(to_rockchip_crtc_state(crtc->state),
> + sizeof(*rockchip_state), GFP_KERNEL);
> if (!rockchip_state)
> return NULL;
>
> --
> 2.41.0
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, Jun 20, 2023 at 06:47:38AM +0000, Jonas Karlman wrote:
> It's possible for users to try to duplicate the CRTC state even when the
> state doesn't exist. drm_atomic_helper_crtc_duplicate_state() (and other
> users of __drm_atomic_helper_crtc_duplicate_state()) already guard this
> with a WARN_ON() instead of crashing, so let's do that here too.
>
> Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")
> Signed-off-by: Jonas Karlman <[email protected]>
Reviewed-by: Sascha Hauer <[email protected]>
Sascha
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index ca73b8ccc29f..f725487d02ef 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -2099,11 +2099,13 @@ static void vop2_crtc_reset(struct drm_crtc *crtc)
>
> static struct drm_crtc_state *vop2_crtc_duplicate_state(struct drm_crtc *crtc)
> {
> - struct rockchip_crtc_state *vcstate, *old_vcstate;
> + struct rockchip_crtc_state *vcstate;
>
> - old_vcstate = to_rockchip_crtc_state(crtc->state);
> + if (WARN_ON(!crtc->state))
> + return NULL;
>
> - vcstate = kmemdup(old_vcstate, sizeof(*old_vcstate), GFP_KERNEL);
> + vcstate = kmemdup(to_rockchip_crtc_state(crtc->state),
> + sizeof(*vcstate), GFP_KERNEL);
> if (!vcstate)
> return NULL;
>
> --
> 2.41.0
>
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, Jun 20, 2023 at 06:47:39AM +0000, Jonas Karlman wrote:
> Add missing call to crtc reset helper to properly vblank reset.
>
> Also move vop2_crtc_reset and call vop2_crtc_destroy_state to simplify
> and remove duplicated code.
>
> Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")
> Signed-off-by: Jonas Karlman <[email protected]>
> ---
> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 28 ++++++++------------
> 1 file changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index f725487d02ef..1be84fe0208f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -2080,23 +2080,6 @@ static const struct drm_crtc_helper_funcs vop2_crtc_helper_funcs = {
> .atomic_disable = vop2_crtc_atomic_disable,
> };
>
> -static void vop2_crtc_reset(struct drm_crtc *crtc)
> -{
> - struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(crtc->state);
> -
> - if (crtc->state) {
> - __drm_atomic_helper_crtc_destroy_state(crtc->state);
> - kfree(vcstate);
> - }
> -
> - vcstate = kzalloc(sizeof(*vcstate), GFP_KERNEL);
> - if (!vcstate)
> - return;
> -
> - crtc->state = &vcstate->base;
> - crtc->state->crtc = crtc;
> -}
> -
> static struct drm_crtc_state *vop2_crtc_duplicate_state(struct drm_crtc *crtc)
> {
> struct rockchip_crtc_state *vcstate;
> @@ -2123,6 +2106,17 @@ static void vop2_crtc_destroy_state(struct drm_crtc *crtc,
> kfree(vcstate);
> }
>
> +static void vop2_crtc_reset(struct drm_crtc *crtc)
> +{
> + struct rockchip_crtc_state *vcstate =
> + kzalloc(sizeof(*vcstate), GFP_KERNEL);
> +
> + if (crtc->state)
> + vop2_crtc_destroy_state(crtc, crtc->state);
> +
> + __drm_atomic_helper_crtc_reset(crtc, &vcstate->base);
> +}
You missed to check for allocation failures before using vcstate.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 2023-06-21 10:11, Sascha Hauer wrote:
> On Tue, Jun 20, 2023 at 06:47:39AM +0000, Jonas Karlman wrote:
>> Add missing call to crtc reset helper to properly vblank reset.
>>
>> Also move vop2_crtc_reset and call vop2_crtc_destroy_state to simplify
>> and remove duplicated code.
>>
>> Fixes: 604be85547ce ("drm/rockchip: Add VOP2 driver")
>> Signed-off-by: Jonas Karlman <[email protected]>
>> ---
>> drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 28 ++++++++------------
>> 1 file changed, 11 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> index f725487d02ef..1be84fe0208f 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> @@ -2080,23 +2080,6 @@ static const struct drm_crtc_helper_funcs vop2_crtc_helper_funcs = {
>> .atomic_disable = vop2_crtc_atomic_disable,
>> };
>>
>> -static void vop2_crtc_reset(struct drm_crtc *crtc)
>> -{
>> - struct rockchip_crtc_state *vcstate = to_rockchip_crtc_state(crtc->state);
>> -
>> - if (crtc->state) {
>> - __drm_atomic_helper_crtc_destroy_state(crtc->state);
>> - kfree(vcstate);
>> - }
>> -
>> - vcstate = kzalloc(sizeof(*vcstate), GFP_KERNEL);
>> - if (!vcstate)
>> - return;
>> -
>> - crtc->state = &vcstate->base;
>> - crtc->state->crtc = crtc;
>> -}
>> -
>> static struct drm_crtc_state *vop2_crtc_duplicate_state(struct drm_crtc *crtc)
>> {
>> struct rockchip_crtc_state *vcstate;
>> @@ -2123,6 +2106,17 @@ static void vop2_crtc_destroy_state(struct drm_crtc *crtc,
>> kfree(vcstate);
>> }
>>
>> +static void vop2_crtc_reset(struct drm_crtc *crtc)
>> +{
>> + struct rockchip_crtc_state *vcstate =
>> + kzalloc(sizeof(*vcstate), GFP_KERNEL);
>> +
>> + if (crtc->state)
>> + vop2_crtc_destroy_state(crtc, crtc->state);
>> +
>> + __drm_atomic_helper_crtc_reset(crtc, &vcstate->base);
>> +}
>
> You missed to check for allocation failures before using vcstate.
Good catch, I will fix for both vop and vop2 driver in v2.
Regards,
Jonas
>
> Sascha
>