[PATCH 1/2] drm/msm: provide fb_dirty implemenation

[PATCH 1/2] drm/msm: provide fb_dirty implemenation

[Dmitry Baryshkov]

Since commit 93e81e38e197 ("drm/fb_helper: Minimize damage-helper
overhead") the drm_fb_helper_funcs::fb_dirty helper is required for
proper dirty/damage processing. The drm/msm driver requires that to
function to let CMD panels to work. Use simplified version of
drm_fbdev_generic_helper_fb_dirty() to fix support for CMD mode panels.

Reported-by: Degdag Mohamed <degdagmohamed@gmail.com>
Fixes: 93e81e38e197 ("drm/fb_helper: Minimize damage-helper overhead")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_fbdev.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
index fa9c1cbffae3..b933a85420f6 100644
--- a/drivers/gpu/drm/msm/msm_fbdev.c
+++ b/drivers/gpu/drm/msm/msm_fbdev.c
@@ -139,8 +139,28 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
 	return ret;
 }
 
+static int msm_fbdev_fb_dirty(struct drm_fb_helper *helper,
+			      struct drm_clip_rect *clip)
+{
+	struct drm_device *dev = helper->dev;
+	int ret;
+
+	/* Call damage handlers only if necessary */
+	if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
+		return 0;
+
+	if (helper->fb->funcs->dirty) {
+		ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
+		if (drm_WARN_ONCE(dev, ret, "Dirty helper failed: ret=%d\n", ret))
+			return ret;
+	}
+
+	return 0;
+}
+
 static const struct drm_fb_helper_funcs msm_fb_helper_funcs = {
 	.fb_probe = msm_fbdev_create,
+	.fb_dirty = msm_fbdev_fb_dirty,
 };
 
 /*
-- 
2.39.2

[PATCH 2/2] drm/msm/dsi: don't allow enabling 7nm VCO with unprogrammed rate

[Dmitry Baryshkov]

CCF can try enabling VCO before the rate has been programmed. This can
cause clock lockups and/or other boot issues. Program the VCO to the
minimal PLL rate if the read rate is 0 Hz.

Reported-by: Degdag Mohamed <degdagmohamed@gmail.com>
Fixes: 1ef7c99d145c ("drm/msm/dsi: add support for 7nm DSI PHY/PLL")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
index 3b1ed02f644d..6979d35eb7c3 100644
--- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
+++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
@@ -395,11 +395,16 @@ static void dsi_pll_phy_dig_reset(struct dsi_pll_7nm *pll)
 	wmb(); /* Ensure that the reset is deasserted */
 }
 
+static unsigned long dsi_pll_7nm_vco_recalc_rate(struct clk_hw *hw,
+						  unsigned long parent_rate);
 static int dsi_pll_7nm_vco_prepare(struct clk_hw *hw)
 {
 	struct dsi_pll_7nm *pll_7nm = to_pll_7nm(hw);
 	int rc;
 
+	if (dsi_pll_7nm_vco_recalc_rate(hw, VCO_REF_CLK_RATE) == 0)
+		dsi_pll_7nm_vco_set_rate(hw, pll_7nm->phy->cfg->min_pll_rate, VCO_REF_CLK_RATE);
+
 	dsi_pll_enable_pll_bias(pll_7nm);
 	if (pll_7nm->slave)
 		dsi_pll_enable_pll_bias(pll_7nm->slave);
-- 
2.39.2

Re: [PATCH 2/2] drm/msm/dsi: don't allow enabling 7nm VCO with unprogrammed rate

[Degdag Mohamed]

[Dmitry Baryshkov],

Thank you for sharing the patches. I have reviewed and tested all
three patches, and they seem to be working correctly.

Tested-by: Degdag Mohamed degdagmohamed@gmail.com

Please let me know if you need any further information or assistance.

Best regards,
Degdag Mohamed

On 6/12/23, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> CCF can try enabling VCO before the rate has been programmed. This can
> cause clock lockups and/or other boot issues. Program the VCO to the
> minimal PLL rate if the read rate is 0 Hz.
>
> Reported-by: Degdag Mohamed <degdagmohamed@gmail.com>
> Fixes: 1ef7c99d145c ("drm/msm/dsi: add support for 7nm DSI PHY/PLL")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> index 3b1ed02f644d..6979d35eb7c3 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> @@ -395,11 +395,16 @@ static void dsi_pll_phy_dig_reset(struct dsi_pll_7nm
> *pll)
>  	wmb(); /* Ensure that the reset is deasserted */
>  }
>
> +static unsigned long dsi_pll_7nm_vco_recalc_rate(struct clk_hw *hw,
> +						  unsigned long parent_rate);
>  static int dsi_pll_7nm_vco_prepare(struct clk_hw *hw)
>  {
>  	struct dsi_pll_7nm *pll_7nm = to_pll_7nm(hw);
>  	int rc;
>
> +	if (dsi_pll_7nm_vco_recalc_rate(hw, VCO_REF_CLK_RATE) == 0)
> +		dsi_pll_7nm_vco_set_rate(hw, pll_7nm->phy->cfg->min_pll_rate,
> VCO_REF_CLK_RATE);
> +
>  	dsi_pll_enable_pll_bias(pll_7nm);
>  	if (pll_7nm->slave)
>  		dsi_pll_enable_pll_bias(pll_7nm->slave);
> --
> 2.39.2
>
>

Re: [PATCH 1/2] drm/msm: provide fb_dirty implemenation

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2217 bytes --]

Hi

Am 12.06.23 um 05:16 schrieb Dmitry Baryshkov:
> Since commit 93e81e38e197 ("drm/fb_helper: Minimize damage-helper
> overhead") the drm_fb_helper_funcs::fb_dirty helper is required for
> proper dirty/damage processing. The drm/msm driver requires that to
> function to let CMD panels to work. Use simplified version of
> drm_fbdev_generic_helper_fb_dirty() to fix support for CMD mode panels.
> 
> Reported-by: Degdag Mohamed <degdagmohamed@gmail.com>
> Fixes: 93e81e38e197 ("drm/fb_helper: Minimize damage-helper overhead")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

To make mmap work correctly, you'll also need deferred I/O in the fbdev 
code. AFAICT the driver never supported that.

Best regards
Thomas

> ---
>   drivers/gpu/drm/msm/msm_fbdev.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
> index fa9c1cbffae3..b933a85420f6 100644
> --- a/drivers/gpu/drm/msm/msm_fbdev.c
> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> @@ -139,8 +139,28 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
>   	return ret;
>   }
>   
> +static int msm_fbdev_fb_dirty(struct drm_fb_helper *helper,
> +			      struct drm_clip_rect *clip)
> +{
> +	struct drm_device *dev = helper->dev;
> +	int ret;
> +
> +	/* Call damage handlers only if necessary */
> +	if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
> +		return 0;
> +
> +	if (helper->fb->funcs->dirty) {
> +		ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
> +		if (drm_WARN_ONCE(dev, ret, "Dirty helper failed: ret=%d\n", ret))
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>   static const struct drm_fb_helper_funcs msm_fb_helper_funcs = {
>   	.fb_probe = msm_fbdev_create,
> +	.fb_dirty = msm_fbdev_fb_dirty,
>   };
>   
>   /*

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 1/2] drm/msm: provide fb_dirty implemenation

[Marijn Suijten]

On 2023-06-12 06:16:15, Dmitry Baryshkov wrote:
> Since commit 93e81e38e197 ("drm/fb_helper: Minimize damage-helper
> overhead") the drm_fb_helper_funcs::fb_dirty helper is required for
> proper dirty/damage processing. The drm/msm driver requires that to
> function to let CMD panels to work. Use simplified version of
> drm_fbdev_generic_helper_fb_dirty() to fix support for CMD mode panels.
> 
> Reported-by: Degdag Mohamed <degdagmohamed@gmail.com>
> Fixes: 93e81e38e197 ("drm/fb_helper: Minimize damage-helper overhead")
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Thanks, this solves the following warning:

    msm_dpu ae01000.display-controller: drm_WARN_ON_ONCE(!helper->funcs->fb_dirty)
    WARNING: CPU: 0 PID: 9 at drivers/gpu/drm/drm_fb_helper.c:381 drm_fb_helper_damage_work+0x1c0/0x20c

Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>

Note that drm_fb_helper_funcs documents this as "This callback is
optional": is it no longer optional, or are we enabling a damage feature
that makes it not-optional?

- Marijn

> ---
>  drivers/gpu/drm/msm/msm_fbdev.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
> index fa9c1cbffae3..b933a85420f6 100644
> --- a/drivers/gpu/drm/msm/msm_fbdev.c
> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
> @@ -139,8 +139,28 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
>  	return ret;
>  }
>  
> +static int msm_fbdev_fb_dirty(struct drm_fb_helper *helper,
> +			      struct drm_clip_rect *clip)
> +{
> +	struct drm_device *dev = helper->dev;
> +	int ret;
> +
> +	/* Call damage handlers only if necessary */
> +	if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
> +		return 0;
> +
> +	if (helper->fb->funcs->dirty) {
> +		ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
> +		if (drm_WARN_ONCE(dev, ret, "Dirty helper failed: ret=%d\n", ret))
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct drm_fb_helper_funcs msm_fb_helper_funcs = {
>  	.fb_probe = msm_fbdev_create,
> +	.fb_dirty = msm_fbdev_fb_dirty,
>  };
>  
>  /*
> -- 
> 2.39.2
> 

Re: [PATCH 2/2] drm/msm/dsi: don't allow enabling 7nm VCO with unprogrammed rate

[Marijn Suijten]

On 2023-06-12 06:16:16, Dmitry Baryshkov wrote:
> CCF can try enabling VCO before the rate has been programmed. This can
> cause clock lockups and/or other boot issues. Program the VCO to the
> minimal PLL rate if the read rate is 0 Hz.
> 
> Reported-by: Degdag Mohamed <degdagmohamed@gmail.com>
> Fixes: 1ef7c99d145c ("drm/msm/dsi: add support for 7nm DSI PHY/PLL")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

This unfortunately regresses my Xperia 5 (sofef01 panel driver that's
on the lists) to now run at 30~33Hz instead of 60Hz.  I can provide
debugging and clk trees later, if needed.

- Marijn

> ---
>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> index 3b1ed02f644d..6979d35eb7c3 100644
> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> @@ -395,11 +395,16 @@ static void dsi_pll_phy_dig_reset(struct dsi_pll_7nm *pll)
>  	wmb(); /* Ensure that the reset is deasserted */
>  }
>  
> +static unsigned long dsi_pll_7nm_vco_recalc_rate(struct clk_hw *hw,
> +						  unsigned long parent_rate);
>  static int dsi_pll_7nm_vco_prepare(struct clk_hw *hw)
>  {
>  	struct dsi_pll_7nm *pll_7nm = to_pll_7nm(hw);
>  	int rc;
>  
> +	if (dsi_pll_7nm_vco_recalc_rate(hw, VCO_REF_CLK_RATE) == 0)
> +		dsi_pll_7nm_vco_set_rate(hw, pll_7nm->phy->cfg->min_pll_rate, VCO_REF_CLK_RATE);
> +
>  	dsi_pll_enable_pll_bias(pll_7nm);
>  	if (pll_7nm->slave)
>  		dsi_pll_enable_pll_bias(pll_7nm->slave);
> -- 
> 2.39.2
> 

Re: [PATCH 2/2] drm/msm/dsi: don't allow enabling 7nm VCO with unprogrammed rate

[Marijn Suijten]

On 2023-06-12 11:15:53, Marijn Suijten wrote:
> On 2023-06-12 06:16:16, Dmitry Baryshkov wrote:
> > CCF can try enabling VCO before the rate has been programmed. This can
> > cause clock lockups and/or other boot issues. Program the VCO to the
> > minimal PLL rate if the read rate is 0 Hz.
> > 
> > Reported-by: Degdag Mohamed <degdagmohamed@gmail.com>
> > Fixes: 1ef7c99d145c ("drm/msm/dsi: add support for 7nm DSI PHY/PLL")
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> This unfortunately regresses my Xperia 5 (sofef01 panel driver that's
> on the lists) to now run at 30~33Hz instead of 60Hz.  I can provide
> debugging and clk trees later, if needed.

I'll also retest the Xperia 1 with this, which has a 4k DSC panel.

Is this intended to get rid of:

    msm_dsi_phy ae94400.phy: [drm:dsi_pll_7nm_vco_prepare] *ERROR* DSI PLL(0) lock failed, status=0x00000000: -110
    PLL(0) lock failed

... at startup (and relevant rcg2 update_config failures, unbalanced
disables etc)?  It locks after a couple tries but it's still
unnecessarily spammy.

- Marijn

> 
> - Marijn
> 
> > ---
> >  drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> > index 3b1ed02f644d..6979d35eb7c3 100644
> > --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> > +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> > @@ -395,11 +395,16 @@ static void dsi_pll_phy_dig_reset(struct dsi_pll_7nm *pll)
> >  	wmb(); /* Ensure that the reset is deasserted */
> >  }
> >  
> > +static unsigned long dsi_pll_7nm_vco_recalc_rate(struct clk_hw *hw,
> > +						  unsigned long parent_rate);
> >  static int dsi_pll_7nm_vco_prepare(struct clk_hw *hw)
> >  {
> >  	struct dsi_pll_7nm *pll_7nm = to_pll_7nm(hw);
> >  	int rc;
> >  
> > +	if (dsi_pll_7nm_vco_recalc_rate(hw, VCO_REF_CLK_RATE) == 0)
> > +		dsi_pll_7nm_vco_set_rate(hw, pll_7nm->phy->cfg->min_pll_rate, VCO_REF_CLK_RATE);
> > +
> >  	dsi_pll_enable_pll_bias(pll_7nm);
> >  	if (pll_7nm->slave)
> >  		dsi_pll_enable_pll_bias(pll_7nm->slave);
> > -- 
> > 2.39.2
> > 

Re: [PATCH 1/2] drm/msm: provide fb_dirty implemenation

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2809 bytes --]

Hi

Am 12.06.23 um 11:14 schrieb Marijn Suijten:
> On 2023-06-12 06:16:15, Dmitry Baryshkov wrote:
>> Since commit 93e81e38e197 ("drm/fb_helper: Minimize damage-helper
>> overhead") the drm_fb_helper_funcs::fb_dirty helper is required for
>> proper dirty/damage processing. The drm/msm driver requires that to
>> function to let CMD panels to work. Use simplified version of
>> drm_fbdev_generic_helper_fb_dirty() to fix support for CMD mode panels.
>>
>> Reported-by: Degdag Mohamed <degdagmohamed@gmail.com>
>> Fixes: 93e81e38e197 ("drm/fb_helper: Minimize damage-helper overhead")
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Thanks, this solves the following warning:
> 
>      msm_dpu ae01000.display-controller: drm_WARN_ON_ONCE(!helper->funcs->fb_dirty)
>      WARNING: CPU: 0 PID: 9 at drivers/gpu/drm/drm_fb_helper.c:381 drm_fb_helper_damage_work+0x1c0/0x20c
> 
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> Note that drm_fb_helper_funcs documents this as "This callback is
> optional": is it no longer optional, or are we enabling a damage feature
> that makes it not-optional?

It is optional in the sense that most hardware and drivers don't require 
damage handling. Those that do, also require this callback.

Best regards
Thomas

> 
> - Marijn
> 
>> ---
>>   drivers/gpu/drm/msm/msm_fbdev.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c b/drivers/gpu/drm/msm/msm_fbdev.c
>> index fa9c1cbffae3..b933a85420f6 100644
>> --- a/drivers/gpu/drm/msm/msm_fbdev.c
>> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
>> @@ -139,8 +139,28 @@ static int msm_fbdev_create(struct drm_fb_helper *helper,
>>   	return ret;
>>   }
>>   
>> +static int msm_fbdev_fb_dirty(struct drm_fb_helper *helper,
>> +			      struct drm_clip_rect *clip)
>> +{
>> +	struct drm_device *dev = helper->dev;
>> +	int ret;
>> +
>> +	/* Call damage handlers only if necessary */
>> +	if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
>> +		return 0;
>> +
>> +	if (helper->fb->funcs->dirty) {
>> +		ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
>> +		if (drm_WARN_ONCE(dev, ret, "Dirty helper failed: ret=%d\n", ret))
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static const struct drm_fb_helper_funcs msm_fb_helper_funcs = {
>>   	.fb_probe = msm_fbdev_create,
>> +	.fb_dirty = msm_fbdev_fb_dirty,
>>   };
>>   
>>   /*
>> -- 
>> 2.39.2
>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/2] drm/msm/dsi: don't allow enabling 7nm VCO with unprogrammed rate

[Dmitry Baryshkov]

On 12/06/2023 12:21, Marijn Suijten wrote:
> On 2023-06-12 11:15:53, Marijn Suijten wrote:
>> On 2023-06-12 06:16:16, Dmitry Baryshkov wrote:
>>> CCF can try enabling VCO before the rate has been programmed. This can
>>> cause clock lockups and/or other boot issues. Program the VCO to the
>>> minimal PLL rate if the read rate is 0 Hz.
>>>
>>> Reported-by: Degdag Mohamed <degdagmohamed@gmail.com>
>>> Fixes: 1ef7c99d145c ("drm/msm/dsi: add support for 7nm DSI PHY/PLL")
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> This unfortunately regresses my Xperia 5 (sofef01 panel driver that's
>> on the lists) to now run at 30~33Hz instead of 60Hz.  I can provide
>> debugging and clk trees later, if needed.
> 
> I'll also retest the Xperia 1 with this, which has a 4k DSC panel.
> 
> Is this intended to get rid of:
> 
>      msm_dsi_phy ae94400.phy: [drm:dsi_pll_7nm_vco_prepare] *ERROR* DSI PLL(0) lock failed, status=0x00000000: -110
>      PLL(0) lock failed
> 
> ... at startup (and relevant rcg2 update_config failures, unbalanced
> disables etc)?  It locks after a couple tries but it's still
> unnecessarily spammy.

Yes.

> 
> - Marijn
> 
>>
>> - Marijn
>>
>>> ---
>>>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
>>> index 3b1ed02f644d..6979d35eb7c3 100644
>>> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
>>> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
>>> @@ -395,11 +395,16 @@ static void dsi_pll_phy_dig_reset(struct dsi_pll_7nm *pll)
>>>   	wmb(); /* Ensure that the reset is deasserted */
>>>   }
>>>   
>>> +static unsigned long dsi_pll_7nm_vco_recalc_rate(struct clk_hw *hw,
>>> +						  unsigned long parent_rate);
>>>   static int dsi_pll_7nm_vco_prepare(struct clk_hw *hw)
>>>   {
>>>   	struct dsi_pll_7nm *pll_7nm = to_pll_7nm(hw);
>>>   	int rc;
>>>   
>>> +	if (dsi_pll_7nm_vco_recalc_rate(hw, VCO_REF_CLK_RATE) == 0)
>>> +		dsi_pll_7nm_vco_set_rate(hw, pll_7nm->phy->cfg->min_pll_rate, VCO_REF_CLK_RATE);
>>> +
>>>   	dsi_pll_enable_pll_bias(pll_7nm);
>>>   	if (pll_7nm->slave)
>>>   		dsi_pll_enable_pll_bias(pll_7nm->slave);
>>> -- 
>>> 2.39.2
>>>

-- 
With best wishes
Dmitry

Re: [PATCH 1/2] drm/msm: provide fb_dirty implemenation

[Dmitry Baryshkov]

On 12/06/2023 13:48, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.06.23 um 11:14 schrieb Marijn Suijten:
>> On 2023-06-12 06:16:15, Dmitry Baryshkov wrote:
>>> Since commit 93e81e38e197 ("drm/fb_helper: Minimize damage-helper
>>> overhead") the drm_fb_helper_funcs::fb_dirty helper is required for
>>> proper dirty/damage processing. The drm/msm driver requires that to
>>> function to let CMD panels to work. Use simplified version of
>>> drm_fbdev_generic_helper_fb_dirty() to fix support for CMD mode panels.
>>>
>>> Reported-by: Degdag Mohamed <degdagmohamed@gmail.com>
>>> Fixes: 93e81e38e197 ("drm/fb_helper: Minimize damage-helper overhead")
>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> Thanks, this solves the following warning:
>>
>>      msm_dpu ae01000.display-controller: 
>> drm_WARN_ON_ONCE(!helper->funcs->fb_dirty)
>>      WARNING: CPU: 0 PID: 9 at drivers/gpu/drm/drm_fb_helper.c:381 
>> drm_fb_helper_damage_work+0x1c0/0x20c
>>
>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>
>> Note that drm_fb_helper_funcs documents this as "This callback is
>> optional": is it no longer optional, or are we enabling a damage feature
>> that makes it not-optional?
> 
> It is optional in the sense that most hardware and drivers don't require 
> damage handling. Those that do, also require this callback.

Can we please get this documented? I think it was really optional 
beforehand.

> 
> Best regards
> Thomas
> 
>>
>> - Marijn
>>
>>> ---
>>>   drivers/gpu/drm/msm/msm_fbdev.c | 20 ++++++++++++++++++++
>>>   1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c 
>>> b/drivers/gpu/drm/msm/msm_fbdev.c
>>> index fa9c1cbffae3..b933a85420f6 100644
>>> --- a/drivers/gpu/drm/msm/msm_fbdev.c
>>> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
>>> @@ -139,8 +139,28 @@ static int msm_fbdev_create(struct drm_fb_helper 
>>> *helper,
>>>       return ret;
>>>   }
>>> +static int msm_fbdev_fb_dirty(struct drm_fb_helper *helper,
>>> +                  struct drm_clip_rect *clip)
>>> +{
>>> +    struct drm_device *dev = helper->dev;
>>> +    int ret;
>>> +
>>> +    /* Call damage handlers only if necessary */
>>> +    if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
>>> +        return 0;
>>> +
>>> +    if (helper->fb->funcs->dirty) {
>>> +        ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 
>>> 1);
>>> +        if (drm_WARN_ONCE(dev, ret, "Dirty helper failed: ret=%d\n", 
>>> ret))
>>> +            return ret;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static const struct drm_fb_helper_funcs msm_fb_helper_funcs = {
>>>       .fb_probe = msm_fbdev_create,
>>> +    .fb_dirty = msm_fbdev_fb_dirty,
>>>   };
>>>   /*
>>> -- 
>>> 2.39.2
>>>
> 

-- 
With best wishes
Dmitry

Re: [PATCH 1/2] drm/msm: provide fb_dirty implemenation

[Marijn Suijten]

Title typo that I missed: implemenation -> implemen*T*ation :)

On 2023-06-12 11:14:41, Marijn Suijten wrote:
<snip>

Re: [PATCH 1/2] drm/msm: provide fb_dirty implemenation

[Dmitry Baryshkov]

On 12/06/2023 11:21, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.06.23 um 05:16 schrieb Dmitry Baryshkov:
>> Since commit 93e81e38e197 ("drm/fb_helper: Minimize damage-helper
>> overhead") the drm_fb_helper_funcs::fb_dirty helper is required for
>> proper dirty/damage processing. The drm/msm driver requires that to
>> function to let CMD panels to work. Use simplified version of
>> drm_fbdev_generic_helper_fb_dirty() to fix support for CMD mode panels.
>>
>> Reported-by: Degdag Mohamed <degdagmohamed@gmail.com>
>> Fixes: 93e81e38e197 ("drm/fb_helper: Minimize damage-helper overhead")
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> To make mmap work correctly, you'll also need deferred I/O in the fbdev 
> code. AFAICT the driver never supported that.

We do not use the deferred I/O. The damage/dirty tracking is used to 
check whether we should ping the CMD panel or not. See [1]

[1] https://lore.kernel.org/all/20220223191118.881321-1-robdclark@gmail.com/

> 
> Best regards
> Thomas
> 
>> ---
>>   drivers/gpu/drm/msm/msm_fbdev.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/msm/msm_fbdev.c 
>> b/drivers/gpu/drm/msm/msm_fbdev.c
>> index fa9c1cbffae3..b933a85420f6 100644
>> --- a/drivers/gpu/drm/msm/msm_fbdev.c
>> +++ b/drivers/gpu/drm/msm/msm_fbdev.c
>> @@ -139,8 +139,28 @@ static int msm_fbdev_create(struct drm_fb_helper 
>> *helper,
>>       return ret;
>>   }
>> +static int msm_fbdev_fb_dirty(struct drm_fb_helper *helper,
>> +                  struct drm_clip_rect *clip)
>> +{
>> +    struct drm_device *dev = helper->dev;
>> +    int ret;
>> +
>> +    /* Call damage handlers only if necessary */
>> +    if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
>> +        return 0;
>> +
>> +    if (helper->fb->funcs->dirty) {
>> +        ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
>> +        if (drm_WARN_ONCE(dev, ret, "Dirty helper failed: ret=%d\n", 
>> ret))
>> +            return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static const struct drm_fb_helper_funcs msm_fb_helper_funcs = {
>>       .fb_probe = msm_fbdev_create,
>> +    .fb_dirty = msm_fbdev_fb_dirty,
>>   };
>>   /*
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] drm/msm/dsi: don't allow enabling 7nm VCO with unprogrammed rate

[Marijn Suijten]

On 2023-06-12 21:30:05, Dmitry Baryshkov wrote:
> On 12/06/2023 12:21, Marijn Suijten wrote:
> > On 2023-06-12 11:15:53, Marijn Suijten wrote:
> >> On 2023-06-12 06:16:16, Dmitry Baryshkov wrote:
> >>> CCF can try enabling VCO before the rate has been programmed. This can
> >>> cause clock lockups and/or other boot issues. Program the VCO to the
> >>> minimal PLL rate if the read rate is 0 Hz.
> >>>
> >>> Reported-by: Degdag Mohamed <degdagmohamed@gmail.com>
> >>> Fixes: 1ef7c99d145c ("drm/msm/dsi: add support for 7nm DSI PHY/PLL")
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>
> >> This unfortunately regresses my Xperia 5 (sofef01 panel driver that's
> >> on the lists) to now run at 30~33Hz instead of 60Hz.  I can provide
> >> debugging and clk trees later, if needed.

The clktree before and after this patch remains identical (Xperia 5):

    bi_tcxo                          11       11        0    19200000          0     0  50000         Y
       dsi0vco_clk                    1        1        0  1132181982          0     0  50000         Y
          dsi0_pll_out_div_clk        1        1        0  1132181982          0     0  50000         Y
             dsi0_pll_post_out_div_clk       0        0        0   283045495          0     0  50000         Y
             dsi0_pll_bit_clk         2        2        0  1132181982          0     0  50000         Y
                dsi0_pclk_mux         1        1        0  1132181982          0     0  50000         Y
                   dsi0_phy_pll_out_dsiclk       1        1        0   188696997          0     0  50000         Y
                      disp_cc_mdss_pclk0_clk_src       1        1        0   188696997          0     0  50000         Y
                         disp_cc_mdss_pclk0_clk       1        1        0   188696997          0     0  50000         Y
                dsi0_pll_by_2_bit_clk       0        0        0   566090991          0     0  50000         Y
                dsi0_phy_pll_out_byteclk       1        1        0   141522747          0     0  50000         Y
                   disp_cc_mdss_byte0_clk_src       2        2        0   141522747          0     0  50000         Y
                      disp_cc_mdss_byte0_div_clk_src       1        1        0    70761374          0     0  50000         Y
                         disp_cc_mdss_byte0_intf_clk       1        1        0    70761374          0     0  50000         Y
                      disp_cc_mdss_byte0_clk       1        1        0   141522747          0     0  50000         Y

But the word-diff from `debugcc -p sm8150 -b disp -a` pre and post shows
that the clocks were at first ticking exactly how Linux expects them
to (accurate on kHz it seems), but significantly lower after this patch:

    diff --git a/bahamut_pre_debugcc b/bahamut_post_debugcc
    index be8269ef34ae5..395048a9f37fe 100644
    --- a/bahamut_pre_debugcc
    +++ b/bahamut_post_debugcc
    @@ -1,6 +1,6 @@
                                  disp_cc_mdss_ahb_clk: 19.199412MHz (19199412Hz)
                                disp_cc_mdss_byte0_clk: [-141.522568MHz (141522568Hz)-]{+124.998840MHz (124998840Hz)+}
                           disp_cc_mdss_byte0_intf_clk: [-70.761796MHz (70761796Hz)-]{+62.500812MHz (62500812Hz)+}
                                disp_cc_mdss_byte1_clk: off
                           disp_cc_mdss_byte1_intf_clk: off
                              disp_cc_mdss_dp_aux1_clk: off
    @@ -19,15 +19,15 @@
                             disp_cc_mdss_edp_link_clk: off
                        disp_cc_mdss_edp_link_intf_clk: off
                            disp_cc_mdss_edp_pixel_clk: off
                                 disp_cc_mdss_esc0_clk: [-19.200000MHz (19200000Hz)-]{+19.199412MHz (19199412Hz)+}
                                 disp_cc_mdss_esc1_clk: off
                                  disp_cc_mdss_mdp_clk: [-200.000692MHz (200000692Hz)-]{+200.001280MHz (200001280Hz)+}
                              disp_cc_mdss_mdp_lut_clk: [-200.000108MHz (200000108Hz)-]{+200.001280MHz (200001280Hz)+}
                         disp_cc_mdss_non_gdsc_ahb_clk: [-19.199412MHz (19199412Hz)-]{+19.199704MHz (19199704Hz)+}
                                disp_cc_mdss_pclk0_clk: [-188.697976MHz (188697976Hz)-]{+166.668196MHz (166668196Hz)+}
                                disp_cc_mdss_pclk1_clk: off
                                  disp_cc_mdss_rot_clk: 19.200000MHz (19200000Hz)
                             disp_cc_mdss_rscc_ahb_clk: 19.200000MHz (19200000Hz)
                           disp_cc_mdss_rscc_vsync_clk:[-19.200000MHz (19200000Hz)-]
    [-                            disp_cc_mdss_vsync_clk:-] 19.199704MHz (19199704Hz)
                                {+disp_cc_mdss_vsync_clk: 19.200292MHz (19200292Hz)+}
                                        disp_cc_xo_clk: [-19.199704MHz (19199704Hz)-]{+19.200000MHz (19200000Hz)+}

> > I'll also retest the Xperia 1 with this, which has a 4k DSC panel.

On the Xperia 1 modetest also reports 30 instead of 60Hz now (4k mode,
not the 2.5k mode, if you've kept up with my panel series) but it
remains black.
(Note that this panel uses DSC, so the byte and pixel clock for this
 1644x3840 panel are ticking slower than for a 1080x2520 one :) )

Same story for the clk tree here, it is the same before and after this
patch:

    bi_tcxo                          11       11        0    19200000          0     0  50000         Y
       dsi0vco_clk                    1        1        0  1736017822          0     0  50000         Y
          dsi0_pll_out_div_clk        1        1        0   868008911          0     0  50000         Y
             dsi0_pll_post_out_div_clk       0        0        0   217002227          0     0  50000         Y
             dsi0_pll_bit_clk         2        2        0   868008911          0     0  50000         Y
                dsi0_pclk_mux         1        1        0   868008911          0     0  50000         Y
                   dsi0_phy_pll_out_dsiclk       1        1        0   144668152          0     0  50000         Y
                      disp_cc_mdss_pclk0_clk_src       1        1        0   144668152          0     0  50000         Y
                         disp_cc_mdss_pclk0_clk       1        1        0   144668152          0     0  50000         Y
                dsi0_pll_by_2_bit_clk       0        0        0   434004455          0     0  50000         Y
                dsi0_phy_pll_out_byteclk       1        1        0   108501113          0     0  50000         Y
                   disp_cc_mdss_byte0_clk_src       2        2        0   108501113          0     0  50000         Y
                      disp_cc_mdss_byte0_div_clk_src       1        1        0    54250557          0     0  50000         Y
                         disp_cc_mdss_byte0_intf_clk       1        1        0    54250557          0     0  50000         Y
                      disp_cc_mdss_byte0_clk       1        1        0   108501113          0     0  50000         Y

The measured clocks are now about half of what they're supposed to be:

    diff --git a/griffin_pre_debugcc b/griffin_post_debugcc
    index a5987e1671d3b..8c8618e60ad1c 100644
    --- a/griffin_pre_debugcc
    +++ b/griffin_post_debugcc
    @@ -1,6 +1,6 @@
                                  disp_cc_mdss_ahb_clk: [-19.200000MHz (19200000Hz)-]{+19.199704MHz (19199704Hz)+}
                                disp_cc_mdss_byte0_clk: [-108.502356MHz (108502356Hz)-]{+62.499932MHz (62499932Hz)+}
                           disp_cc_mdss_byte0_intf_clk: [-54.250372MHz (54250372Hz)-]{+31.250040MHz (31250040Hz)+}
                                disp_cc_mdss_byte1_clk: off
                           disp_cc_mdss_byte1_intf_clk: off
                              disp_cc_mdss_dp_aux1_clk: off
    @@ -21,13 +21,13 @@
                            disp_cc_mdss_edp_pixel_clk: off
                                 disp_cc_mdss_esc0_clk: 19.200000MHz (19200000Hz)
                                 disp_cc_mdss_esc1_clk: off
                                  disp_cc_mdss_mdp_clk: [-460.001848MHz (460001848Hz)-]{+460.000384MHz (460000384Hz)+}
                              disp_cc_mdss_mdp_lut_clk: [-460.001556MHz (460001556Hz)-]{+459.996280MHz (459996280Hz)+}
                         disp_cc_mdss_non_gdsc_ahb_clk: 19.200000MHz (19200000Hz)
                                disp_cc_mdss_pclk0_clk: [-144.668004MHz (144668004Hz)-]{+83.333584MHz (83333584Hz)+}
                                disp_cc_mdss_pclk1_clk: off
                                  disp_cc_mdss_rot_clk: [-19.199704MHz (19199704Hz)-]{+19.200000MHz (19200000Hz)+}
                             disp_cc_mdss_rscc_ahb_clk: [-19.200292MHz (19200292Hz)-]{+19.200000MHz (19200000Hz)+}
                           disp_cc_mdss_rscc_vsync_clk:[-19.199412MHz (19199412Hz)-]
    [-                            disp_cc_mdss_vsync_clk:-] 19.200000MHz (19200000Hz)
                                {+disp_cc_mdss_vsync_clk: 19.199412MHz (19199412Hz)+}
                                        disp_cc_xo_clk: 19.199704MHz (19199704Hz)

> > Is this intended to get rid of:
> > 
> >      msm_dsi_phy ae94400.phy: [drm:dsi_pll_7nm_vco_prepare] *ERROR* DSI PLL(0) lock failed, status=0x00000000: -110
> >      PLL(0) lock failed
> > 
> > ... at startup (and relevant rcg2 update_config failures, unbalanced
> > disables etc)?  It locks after a couple tries but it's still
> > unnecessarily spammy.
> 
> Yes.

It is still occuring twice, with the unbalanced disable/unprepare
warnings for dsi0_phy_pll_out_{bytes,dsi}clk.

- Marijn

> 
> > 
> > - Marijn
> > 
> >>
> >> - Marijn
> >>
> >>> ---
> >>>   drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 5 +++++
> >>>   1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> >>> index 3b1ed02f644d..6979d35eb7c3 100644
> >>> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> >>> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> >>> @@ -395,11 +395,16 @@ static void dsi_pll_phy_dig_reset(struct dsi_pll_7nm *pll)
> >>>   	wmb(); /* Ensure that the reset is deasserted */
> >>>   }
> >>>   
> >>> +static unsigned long dsi_pll_7nm_vco_recalc_rate(struct clk_hw *hw,
> >>> +						  unsigned long parent_rate);
> >>>   static int dsi_pll_7nm_vco_prepare(struct clk_hw *hw)
> >>>   {
> >>>   	struct dsi_pll_7nm *pll_7nm = to_pll_7nm(hw);
> >>>   	int rc;
> >>>   
> >>> +	if (dsi_pll_7nm_vco_recalc_rate(hw, VCO_REF_CLK_RATE) == 0)
> >>> +		dsi_pll_7nm_vco_set_rate(hw, pll_7nm->phy->cfg->min_pll_rate, VCO_REF_CLK_RATE);
> >>> +
> >>>   	dsi_pll_enable_pll_bias(pll_7nm);
> >>>   	if (pll_7nm->slave)
> >>>   		dsi_pll_enable_pll_bias(pll_7nm->slave);
> >>> -- 
> >>> 2.39.2
> >>>
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [PATCH 1/2] drm/msm: provide fb_dirty implemenation

[Dmitry Baryshkov]

On Mon, 12 Jun 2023 06:16:15 +0300, Dmitry Baryshkov wrote:
> Since commit 93e81e38e197 ("drm/fb_helper: Minimize damage-helper
> overhead") the drm_fb_helper_funcs::fb_dirty helper is required for
> proper dirty/damage processing. The drm/msm driver requires that to
> function to let CMD panels to work. Use simplified version of
> drm_fbdev_generic_helper_fb_dirty() to fix support for CMD mode panels.
> 
> 
> [...]

Applied, thanks!

[1/2] drm/msm: provide fb_dirty implemenation
      https://gitlab.freedesktop.org/lumag/msm/-/commit/fda520976ef4

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>