drm/rockchip: vop2: Drop unused if_dclk_rate variable

  Commit 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
introduced a variable which ended up being unused.  Remove it.

rockchip_drm_vop2.c:1688:23: warning: variable ‘if_dclk_rate’ set but not used [-Wunused-but-set-variable]

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2 --
 1 file changed, 2 deletions(-)
  

Hi Christian, Andy,

Am Donnerstag, 4. Januar 2024, 15:39:50 CET schrieb Cristian Ciocaltea:
> Commit 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
> introduced a variable which ended up being unused.  Remove it.
> 
> rockchip_drm_vop2.c:1688:23: warning: variable ‘if_dclk_rate’ set but not used [-Wunused-but-set-variable]
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

in general, please don't send non-series patches as replies to other patches.
It confuses tooling like b4 way too often, as this patch is not designated
as a 2/2 (similar to the first one not being 1/2).

> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> index 44508c2dd614..923985d4161b 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> @@ -1685,7 +1685,6 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
>  	unsigned long dclk_core_rate = v_pixclk >> 2;
>  	unsigned long dclk_rate = v_pixclk;
>  	unsigned long dclk_out_rate;
> -	unsigned long if_dclk_rate;
>  	unsigned long if_pixclk_rate;
>  	int K = 1;
>  
> @@ -1700,7 +1699,6 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
>  		}
>  
>  		if_pixclk_rate = (dclk_core_rate << 1) / K;
> -		if_dclk_rate = dclk_core_rate / K;
>  		/*
>  		 * *if_pixclk_div = dclk_rate / if_pixclk_rate;
>  		 * *if_dclk_div = dclk_rate / if_dclk_rate;
>  		 */
		*if_pixclk_div = 2;
		*if_dclk_div = 4;

with the code continuing with those static constants but the comment
showing a forumula, I do hope Andy can provide a bit of insight into
what is happening here.

I.e. I'd really like to understand if that really is just a remnant or
something different is needed.


Heiko
  

Hi Heiko,

On 1/4/24 17:58, Heiko Stübner wrote:
> Hi Christian, Andy,
> 
> Am Donnerstag, 4. Januar 2024, 15:39:50 CET schrieb Cristian Ciocaltea:
>> Commit 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
>> introduced a variable which ended up being unused.  Remove it.
>>
>> rockchip_drm_vop2.c:1688:23: warning: variable ‘if_dclk_rate’ set but not used [-Wunused-but-set-variable]
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> 
> in general, please don't send non-series patches as replies to other patches.
> It confuses tooling like b4 way too often, as this patch is not designated
> as a 2/2 (similar to the first one not being 1/2).

That was unintentional, sorry!  I wrongly assumed 'git send-email' is
able to correctly handle multiple patches which are not part of a
series.  I'm not sure if the '--no-thread' flag would have helped.

>> ---
>>  drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> index 44508c2dd614..923985d4161b 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> @@ -1685,7 +1685,6 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
>>  	unsigned long dclk_core_rate = v_pixclk >> 2;
>>  	unsigned long dclk_rate = v_pixclk;
>>  	unsigned long dclk_out_rate;
>> -	unsigned long if_dclk_rate;
>>  	unsigned long if_pixclk_rate;
>>  	int K = 1;
>>  
>> @@ -1700,7 +1699,6 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
>>  		}
>>  
>>  		if_pixclk_rate = (dclk_core_rate << 1) / K;
>> -		if_dclk_rate = dclk_core_rate / K;
>>  		/*
>>  		 * *if_pixclk_div = dclk_rate / if_pixclk_rate;
>>  		 * *if_dclk_div = dclk_rate / if_dclk_rate;
>>  		 */
> 		*if_pixclk_div = 2;
> 		*if_dclk_div = 4;
> 
> with the code continuing with those static constants but the comment
> showing a forumula, I do hope Andy can provide a bit of insight into
> what is happening here.
> 
> I.e. I'd really like to understand if that really is just a remnant or
> something different is needed.

The current implementation is not able to handle all display modes
supported by connected displays, e.g. in my testing environment I
encountered issues with 2560x1440-75.00Hz, 2048x1152-60.00Hz,
1024x768-60.00Hz.  Additionally, it doesn't seem to cope well with
non-integer refresh rates like 59.94, 29.97, 23.98, etc.

My temporary workaround relies on using the HDMI PHY PLL in conjunction
with a downstream-based hack to compute the clock rates.  I'm not sure
that would be an upstreamable solution, so I would let Andy shed some
light on the topic.

Thanks,
Cristian

> 
> Heiko
> 
>
  

Hi:

On 1/4/24 23:58, Heiko Stübner wrote:
> Hi Christian, Andy,
> 
> Am Donnerstag, 4. Januar 2024, 15:39:50 CET schrieb Cristian Ciocaltea:
>> Commit 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
>> introduced a variable which ended up being unused.  Remove it.
>>
>> rockchip_drm_vop2.c:1688:23: warning: variable ‘if_dclk_rate’ set but not used [-Wunused-but-set-variable]
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> 
> in general, please don't send non-series patches as replies to other patches.
> It confuses tooling like b4 way too often, as this patch is not designated
> as a 2/2 (similar to the first one not being 1/2).
> 
>> ---
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> index 44508c2dd614..923985d4161b 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>> @@ -1685,7 +1685,6 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
>>   	unsigned long dclk_core_rate = v_pixclk >> 2;
>>   	unsigned long dclk_rate = v_pixclk;
>>   	unsigned long dclk_out_rate;
>> -	unsigned long if_dclk_rate;
>>   	unsigned long if_pixclk_rate;
>>   	int K = 1;
>>   
>> @@ -1700,7 +1699,6 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
>>   		}
>>   
>>   		if_pixclk_rate = (dclk_core_rate << 1) / K;
>> -		if_dclk_rate = dclk_core_rate / K;
>>   		/*
>>   		 * *if_pixclk_div = dclk_rate / if_pixclk_rate;
>>   		 * *if_dclk_div = dclk_rate / if_dclk_rate;
>>   		 */
> 		*if_pixclk_div = 2;
> 		*if_dclk_div = 4;
> 
> with the code continuing with those static constants but the comment
> showing a forumula, I do hope Andy can provide a bit of insight into
> what is happening here.
> 
> I.e. I'd really like to understand if that really is just a remnant or
> something different is needed.

This is not a remnant, in my V1, I calculate all the div by formula, but Sascha prefer
more for a constants value[0], so I keep this formula as comments to indicate how these value come from.

[0]https://patchwork.kernel.org/project/linux-rockchip/patch/20231114112855.1771372-1-andyshrk@163.com/
> 
> 
> Heiko
> 
>
  

Hi Cristian:

On 1/5/24 03:12, Cristian Ciocaltea wrote:
> Hi Heiko,
> 
> On 1/4/24 17:58, Heiko Stübner wrote:
>> Hi Christian, Andy,
>>
>> Am Donnerstag, 4. Januar 2024, 15:39:50 CET schrieb Cristian Ciocaltea:
>>> Commit 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
>>> introduced a variable which ended up being unused.  Remove it.
>>>
>>> rockchip_drm_vop2.c:1688:23: warning: variable ‘if_dclk_rate’ set but not used [-Wunused-but-set-variable]
>>>
>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>>
>> in general, please don't send non-series patches as replies to other patches.
>> It confuses tooling like b4 way too often, as this patch is not designated
>> as a 2/2 (similar to the first one not being 1/2).
> 
> That was unintentional, sorry!  I wrongly assumed 'git send-email' is
> able to correctly handle multiple patches which are not part of a
> series.  I'm not sure if the '--no-thread' flag would have helped.
> 
>>> ---
>>>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>> index 44508c2dd614..923985d4161b 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
>>> @@ -1685,7 +1685,6 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
>>>   	unsigned long dclk_core_rate = v_pixclk >> 2;
>>>   	unsigned long dclk_rate = v_pixclk;
>>>   	unsigned long dclk_out_rate;
>>> -	unsigned long if_dclk_rate;
>>>   	unsigned long if_pixclk_rate;
>>>   	int K = 1;
>>>   
>>> @@ -1700,7 +1699,6 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
>>>   		}
>>>   
>>>   		if_pixclk_rate = (dclk_core_rate << 1) / K;
>>> -		if_dclk_rate = dclk_core_rate / K;
>>>   		/*
>>>   		 * *if_pixclk_div = dclk_rate / if_pixclk_rate;
>>>   		 * *if_dclk_div = dclk_rate / if_dclk_rate;
>>>   		 */
>> 		*if_pixclk_div = 2;
>> 		*if_dclk_div = 4;
>>
>> with the code continuing with those static constants but the comment
>> showing a forumula, I do hope Andy can provide a bit of insight into
>> what is happening here.
>>
>> I.e. I'd really like to understand if that really is just a remnant or
>> something different is needed.
> 
> The current implementation is not able to handle all display modes
> supported by connected displays, e.g. in my testing environment I
> encountered issues with 2560x1440-75.00Hz, 2048x1152-60.00Hz,
> 1024x768-60.00Hz.  Additionally, it doesn't seem to cope well with
> non-integer refresh rates like 59.94, 29.97, 23.98, etc.

I think this is because the thee PLL from cru can't divide accurate clock for these
non-integer refresh rates.
> 
> My temporary workaround relies on using the HDMI PHY PLL in conjunction
> with a downstream-based hack to compute the clock rates.  I'm not sure
> that would be an upstreamable solution, so I would let Andy shed some
> light on the topic.

Yes, use PLL from HDMI PHY can give more flexible clock rates to support more display mode.
We also use it in our bsp kernel, but one thing should  keep in mind that  use HDMI PHY pll
as dclk source can only work for HDMI 2.0 or bellow, if can't be used in hdmi 2.1 mode(such as 4K120),
so we need to switch the clock source by hdmi work mode.

The difficult thing is how to make this accepted by upstream.
> 
> Thanks,
> Cristian
> 
>>
>> Heiko
>>
>>
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
  

Hi,

Am Freitag, 5. Januar 2024, 10:04:55 CET schrieb Andy Yan:
> On 1/4/24 23:58, Heiko Stübner wrote:
> > Am Donnerstag, 4. Januar 2024, 15:39:50 CET schrieb Cristian Ciocaltea:
> >> Commit 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
> >> introduced a variable which ended up being unused.  Remove it.
> >>
> >> rockchip_drm_vop2.c:1688:23: warning: variable ‘if_dclk_rate’ set but not used [-Wunused-but-set-variable]
> >>
> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> > 
> > in general, please don't send non-series patches as replies to other patches.
> > It confuses tooling like b4 way too often, as this patch is not designated
> > as a 2/2 (similar to the first one not being 1/2).
> > 
> >> ---
> >>   drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 2 --
> >>   1 file changed, 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> >> index 44508c2dd614..923985d4161b 100644
> >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
> >> @@ -1685,7 +1685,6 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
> >>   	unsigned long dclk_core_rate = v_pixclk >> 2;
> >>   	unsigned long dclk_rate = v_pixclk;
> >>   	unsigned long dclk_out_rate;
> >> -	unsigned long if_dclk_rate;
> >>   	unsigned long if_pixclk_rate;
> >>   	int K = 1;
> >>   
> >> @@ -1700,7 +1699,6 @@ static unsigned long rk3588_calc_cru_cfg(struct vop2_video_port *vp, int id,
> >>   		}
> >>   
> >>   		if_pixclk_rate = (dclk_core_rate << 1) / K;
> >> -		if_dclk_rate = dclk_core_rate / K;
> >>   		/*
> >>   		 * *if_pixclk_div = dclk_rate / if_pixclk_rate;
> >>   		 * *if_dclk_div = dclk_rate / if_dclk_rate;
> >>   		 */
> > 		*if_pixclk_div = 2;
> > 		*if_dclk_div = 4;
> > 
> > with the code continuing with those static constants but the comment
> > showing a forumula, I do hope Andy can provide a bit of insight into
> > what is happening here.
> > 
> > I.e. I'd really like to understand if that really is just a remnant or
> > something different is needed.
> 
> This is not a remnant, in my V1, I calculate all the div by formula, but Sascha prefer
> more for a constants value[0], so I keep this formula as comments to indicate how these value come from.
> 
> [0]https://patchwork.kernel.org/project/linux-rockchip/patch/20231114112855.1771372-1-andyshrk@163.com/

thanks for referencing the source of the change.
Leaving the formula in there was the right choice I think

That still leaves the issue with the "unused" warning.

@Christan: in the hdmi block itself can you move the 
	if_dclk_rate = dclk_core_rate / K;
to the comment block please? And possibly reference the use
of the static values in the comment message.

The if_dclk_rate var declaration at the top of the function can of course
go away.

That way we still keep documenting how these values came to be:

  		/*
		 * if_dclk_rate = dclk_core_rate / K;
  		 * *if_pixclk_div = dclk_rate / if_pixclk_rate;
  		 * *if_dclk_div = dclk_rate / if_dclk_rate;
  		 */

Thanks
Heiko
  

On 1/5/24 11:57, Heiko Stübner wrote:
> Hi,
> 
> Am Freitag, 5. Januar 2024, 10:04:55 CET schrieb Andy Yan:
>> On 1/4/24 23:58, Heiko Stübner wrote:
>>> Am Donnerstag, 4. Januar 2024, 15:39:50 CET schrieb Cristian Ciocaltea:
>>>> Commit 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
>>>> introduced a variable which ended up being unused.  Remove it.
>>>>
>>>> rockchip_drm_vop2.c:1688:23: warning: variable ‘if_dclk_rate’ set but not used [-Wunused-but-set-variable]
>>>>
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

[...]

>>> with the code continuing with those static constants but the comment
>>> showing a forumula, I do hope Andy can provide a bit of insight into
>>> what is happening here.
>>>
>>> I.e. I'd really like to understand if that really is just a remnant or
>>> something different is needed.
>>
>> This is not a remnant, in my V1, I calculate all the div by formula, but Sascha prefer
>> more for a constants value[0], so I keep this formula as comments to indicate how these value come from.
>>
>> [0]https://patchwork.kernel.org/project/linux-rockchip/patch/20231114112855.1771372-1-andyshrk@163.com/
> 
> thanks for referencing the source of the change.
> Leaving the formula in there was the right choice I think
> 
> That still leaves the issue with the "unused" warning.
> 
> @Christan: in the hdmi block itself can you move the 
> 	if_dclk_rate = dclk_core_rate / K;
> to the comment block please? And possibly reference the use
> of the static values in the comment message.
> 
> The if_dclk_rate var declaration at the top of the function can of course
> go away.
> 
> That way we still keep documenting how these values came to be:

Requested changes submitted via v2:

https://lore.kernel.org/lkml/20240105174007.98054-1-cristian.ciocaltea@collabora.com/

Thanks,
Cristian
  

Hi Andy,

On 1/5/24 11:13, Andy Yan wrote:
> Hi Cristian:
> 
> On 1/5/24 03:12, Cristian Ciocaltea wrote:
>> Hi Heiko,
>>
>> On 1/4/24 17:58, Heiko Stübner wrote:
>>> Hi Christian, Andy,
>>>
>>> Am Donnerstag, 4. Januar 2024, 15:39:50 CET schrieb Cristian Ciocaltea:
>>>> Commit 5a028e8f062f ("drm/rockchip: vop2: Add support for rk3588")
>>>> introduced a variable which ended up being unused.  Remove it.
>>>>
>>>> rockchip_drm_vop2.c:1688:23: warning: variable ‘if_dclk_rate’ set
>>>> but not used [-Wunused-but-set-variable]
>>>>
>>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

[...]

>> The current implementation is not able to handle all display modes
>> supported by connected displays, e.g. in my testing environment I
>> encountered issues with 2560x1440-75.00Hz, 2048x1152-60.00Hz,
>> 1024x768-60.00Hz.  Additionally, it doesn't seem to cope well with
>> non-integer refresh rates like 59.94, 29.97, 23.98, etc.
> 
> I think this is because the thee PLL from cru can't divide accurate
> clock for these
> non-integer refresh rates.
>>
>> My temporary workaround relies on using the HDMI PHY PLL in conjunction
>> with a downstream-based hack to compute the clock rates.  I'm not sure
>> that would be an upstreamable solution, so I would let Andy shed some
>> light on the topic.
> 
> Yes, use PLL from HDMI PHY can give more flexible clock rates to support
> more display mode.
> We also use it in our bsp kernel, but one thing should  keep in mind
> that  use HDMI PHY pll
> as dclk source can only work for HDMI 2.0 or bellow, if can't be used in
> hdmi 2.1 mode(such as 4K120),
> so we need to switch the clock source by hdmi work mode.
> 
> The difficult thing is how to make this accepted by upstream.

Thanks for the heads-up!  I will try to have a look as soon as I
finalize the work on HDMI phy & bridge drivers (unfortunately I had to
put it temporarily on hold to focus on different tasks, but I expect to
resume soon).

Regards,
Cristian