The rockchip_drm_fb.h header contains just a single function which is
not directly used by the VOP2 driver. Drop the unnecessary include.
Signed-off-by: Cristian Ciocaltea <[email protected]>
---
drivers/gpu/drm/rockchip/rockchip_drm_vop2.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
index 923985d4161b..74c533292bbb 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop2.c
@@ -35,7 +35,6 @@
#include "rockchip_drm_drv.h"
#include "rockchip_drm_gem.h"
-#include "rockchip_drm_fb.h"
#include "rockchip_drm_vop2.h"
#include "rockchip_rgb.h"
--
2.43.0
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 <[email protected]>
---
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;
--
2.43.0
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 <[email protected]>
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
Am Donnerstag, 4. Januar 2024, 15:39:49 CET schrieb Cristian Ciocaltea:
> The rockchip_drm_fb.h header contains just a single function which is
> not directly used by the VOP2 driver. Drop the unnecessary include.
>
> Signed-off-by: Cristian Ciocaltea <[email protected]>
applied to drm-misc-next-fixes as
commit 38709af26c33e398c3292e96837ccfde41fd9e6b
Regards,
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 <[email protected]>
>
> 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,
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 <[email protected]>
> >
> > 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/[email protected]/
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
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 <[email protected]>
>>
>> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
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 <[email protected]>
>
> 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/[email protected]/
>
>
> 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 <[email protected]>
[...]
>>> 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/[email protected]/
>
> 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/[email protected]/
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 <[email protected]>
[...]
>> 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