Make video port registers nonvolatile. As DSP_CTRL register is written
to twice due to gamma LUT enable bit which is set outside of the main
DSP_CTRL initialization within atomic_enable (for rk356x case it is also
necesarry to always disable gamma LUT before writing a new LUT) there is
a chance that DSP_CTRL value read-out in gamma LUT init/update code is
not the one which was written by the preceding DSP_CTRL initialization
code within atomic_enable. This might result in misconfigured DSP_CTRL
which leads to no visual output[1]. Since DSP_CTRL write takes effect
after VSYNC[1] the issue is not always present. When tested on Pinetab2
with kernel 6.14 it happenes only when DRM is compiled as a module[1].
In order to confirm that it is a timing issue I inserted 18ms udelay
before vop2_crtc_atomic_try_set_gamma in atomic enable and compiled DRM
as module - this has also fixed the issue.

[1] https://lore.kernel.org/linux-rockchip/562b38e5.a496.1975f09f983.Coremail.andyshrk@163.com/

Reported-by: Diederik de Haas <didi.debian@cknow.org>
Closes: https://lore.kernel.org/linux-rockchip/DAEVDSTMWI1E.J454VZN0R9MA@cknow.org/
Suggested-by: Andy Yan <andy.yan@rock-chips.com>
Signed-off-by: Piotr Zalewski <pZ010001011111@proton.me>
---

Notes:
    Changes in v2:
        - add spaces before and after '+'
    
    Link to v1: https://lore.kernel.org/linux-rockchip/20250628180914.1177177-2-pZ010001011111@proton.me/

 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index d0f5fea15e21..0931cb636493 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -2589,12 +2589,13 @@ static int vop2_win_init(struct vop2 *vop2)
 }
 
 /*
- * The window registers are only updated when config done is written.
- * Until that they read back the old value. As we read-modify-write
- * these registers mark them as non-volatile. This makes sure we read
- * the new values from the regmap register cache.
+ * The window and video port registers are only updated when config
+ * done is written. Until that they read back the old value. As we
+ * read-modify-write these registers mark them as non-volatile. This
+ * makes sure we read the new values from the regmap register cache.
  */
 static const struct regmap_range vop2_nonvolatile_range[] = {
+	regmap_reg_range(RK3568_VP0_CTRL_BASE, RK3588_VP3_CTRL_BASE + 255),
 	regmap_reg_range(0x1000, 0x23ff),
 };
 
-- 
2.50.0

On Sun, 06 Jul 2025 08:36:58 +0000, Piotr Zalewski wrote:
> Make video port registers nonvolatile. As DSP_CTRL register is written
> to twice due to gamma LUT enable bit which is set outside of the main
> DSP_CTRL initialization within atomic_enable (for rk356x case it is also
> necesarry to always disable gamma LUT before writing a new LUT) there is
> a chance that DSP_CTRL value read-out in gamma LUT init/update code is
> not the one which was written by the preceding DSP_CTRL initialization
> code within atomic_enable. This might result in misconfigured DSP_CTRL
> which leads to no visual output[1]. Since DSP_CTRL write takes effect
> after VSYNC[1] the issue is not always present. When tested on Pinetab2
> with kernel 6.14 it happenes only when DRM is compiled as a module[1].
> In order to confirm that it is a timing issue I inserted 18ms udelay
> before vop2_crtc_atomic_try_set_gamma in atomic enable and compiled DRM
> as module - this has also fixed the issue.
> 
> [...]

Applied, thanks!

[1/1] rockchip/drm: vop2: make vp registers nonvolatile
      commit: a52dffaa46c2c5ff0b311c4dc1288581f7b9109e

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>

Hi Piotr,

On Sun Jul 6, 2025 at 10:36 AM CEST, Piotr Zalewski wrote:
> Make video port registers nonvolatile. As DSP_CTRL register is written
> to twice due to gamma LUT enable bit which is set outside of the main
> DSP_CTRL initialization within atomic_enable (for rk356x case it is also
> necesarry to always disable gamma LUT before writing a new LUT) there is
> a chance that DSP_CTRL value read-out in gamma LUT init/update code is
> not the one which was written by the preceding DSP_CTRL initialization
> code within atomic_enable. This might result in misconfigured DSP_CTRL
> which leads to no visual output[1]. Since DSP_CTRL write takes effect
> after VSYNC[1] the issue is not always present. When tested on Pinetab2
> with kernel 6.14 it happenes only when DRM is compiled as a module[1].
> In order to confirm that it is a timing issue I inserted 18ms udelay
> before vop2_crtc_atomic_try_set_gamma in atomic enable and compiled DRM
> as module - this has also fixed the issue.
>
> [1] https://lore.kernel.org/linux-rockchip/562b38e5.a496.1975f09f983.Coremail.andyshrk@163.com/
>
> Reported-by: Diederik de Haas <didi.debian@cknow.org>
> Closes: https://lore.kernel.org/linux-rockchip/DAEVDSTMWI1E.J454VZN0R9MA@cknow.org/
> Suggested-by: Andy Yan <andy.yan@rock-chips.com>
> Signed-off-by: Piotr Zalewski <pZ010001011111@proton.me>

With a new version of a patch, you're supposed to add the tags you
received for previous versions, like my Tested-by tag [1].

(unless the new version has changed so much you feel they should not be
carried over; you then need to explicitly describe that and why you
dropped them)

Cheers,
  Diederik

[1] https://lore.kernel.org/linux-rockchip/DAZ4BALHEJ9M.10FO1U9IYP4WA@cknow.org/

> ---
>
> Notes:
>     Changes in v2:
>         - add spaces before and after '+'
>     
>     Link to v1: https://lore.kernel.org/linux-rockchip/20250628180914.1177177-2-pZ010001011111@proton.me/
>
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index d0f5fea15e21..0931cb636493 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -2589,12 +2589,13 @@ static int vop2_win_init(struct vop2 *vop2)
>  }
>  
>  /*
> - * The window registers are only updated when config done is written.
> - * Until that they read back the old value. As we read-modify-write
> - * these registers mark them as non-volatile. This makes sure we read
> - * the new values from the regmap register cache.
> + * The window and video port registers are only updated when config
> + * done is written. Until that they read back the old value. As we
> + * read-modify-write these registers mark them as non-volatile. This
> + * makes sure we read the new values from the regmap register cache.
>   */
>  static const struct regmap_range vop2_nonvolatile_range[] = {
> +	regmap_reg_range(RK3568_VP0_CTRL_BASE, RK3588_VP3_CTRL_BASE + 255),
>  	regmap_reg_range(0x1000, 0x23ff),
>  };
>  

> Hi Piotr,
Hi Diederik 

> With a new version of a patch, you're supposed to add the tags you
> received for previous versions, like my Tested-by tag [1].
> 
> (unless the new version has changed so much you feel they should not be
> carried over; you then need to explicitly describe that and why you
> dropped them)
 
Forgot... Should i send it as PATCH v2 RESEND?

Best regards, Piotr Zalewski

Hi Piotr,

On Sun Jul 6, 2025 at 12:20 PM CEST, Piotr Zalewski wrote:
>> With a new version of a patch, you're supposed to add the tags you
>> received for previous versions, like my Tested-by tag [1].
>> 
>> (unless the new version has changed so much you feel they should not be
>> carried over; you then need to explicitly describe that and why you
>> dropped them)
>  
> Forgot... Should i send it as PATCH v2 RESEND?

I don't think that's needed; the maintainer will let you know if that's
desirable or that they will add it (back) when committing.

Cheers,
  Diederik

Am Sonntag, 6. Juli 2025, 12:37:40 Mitteleuropäische Sommerzeit schrieb Diederik de Haas:
> Hi Piotr,
> 
> On Sun Jul 6, 2025 at 12:20 PM CEST, Piotr Zalewski wrote:
> >> With a new version of a patch, you're supposed to add the tags you
> >> received for previous versions, like my Tested-by tag [1].
> >> 
> >> (unless the new version has changed so much you feel they should not be
> >> carried over; you then need to explicitly describe that and why you
> >> dropped them)
> >  
> > Forgot... Should i send it as PATCH v2 RESEND?
> 
> I don't think that's needed; the maintainer will let you know if that's
> desirable or that they will add it (back) when committing.

The problem is then remembering to manually collect the tags from a
previous series.

For my reference, it was
Tested-by: Diederik de Haas <didi.debian@cknow.org>

So hopefully I'll remember now :-) and there is no need for a resend
at this time.


Heiko

Hi Heiko,

On Sun Jul 6, 2025 at 12:46 PM CEST, Heiko Stübner wrote:
> Am Sonntag, 6. Juli 2025, 12:37:40 Mitteleuropäische Sommerzeit schrieb Diederik de Haas:
>> On Sun Jul 6, 2025 at 12:20 PM CEST, Piotr Zalewski wrote:
>> >> With a new version of a patch, you're supposed to add the tags you
>> >> received for previous versions, like my Tested-by tag [1].
>> >> 
>> >> (unless the new version has changed so much you feel they should not be
>> >> carried over; you then need to explicitly describe that and why you
>> >> dropped them)
>> >  
>> > Forgot... Should i send it as PATCH v2 RESEND?
>> 
>> I don't think that's needed; the maintainer will let you know if that's
>> desirable or that they will add it (back) when committing.
>
> The problem is then remembering to manually collect the tags from a
> previous series.
>
> For my reference, it was
> Tested-by: Diederik de Haas <didi.debian@cknow.org>
>
> So hopefully I'll remember now :-) and there is no need for a resend
> at this time.

Is more needed to get this patch accepted? If so, can I help with that?

Cheers,
  Diederik