Re: [Intel-gfx] [PATCH v2 03/17] drm: Nuke mode->vrefresh
Hi Jani
On 2020-04-06 01:32, Jani Nikula wrote:
On Fri, 03 Apr 2020, abhin...@codeaurora.org wrote:
On 2020-04-03 13:39, Ville Syrjala wrote:
diff --git a/drivers/gpu/drm/drm_modes.c
b/drivers/gpu/drm/drm_modes.c
index fec1c33b3045..e3d5f011f7bd 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -759,9 +759,7 @@ int drm_mode_vrefresh(const struct
drm_display_mode
*mode)
{
int refresh = 0;
- if (mode->vrefresh > 0)
- refresh = mode->vrefresh;
The mode->vrefresh has been replaced with calling this API in all its
usages.
However in this API, the above if statement was returning the vrefresh
if it was already
set. mode->clock is holding the pixel clock . So this will not cause
any
issues in non-compressed cases.
In case of compression like DSC, the pixel
clock will be different based on the compression ratio hence the
mode->clock will change but fps will not.
So we did have usages in our downstream driver where we would use this
API and the refresh rate
returned will be the mode->vrefresh which did not change but after
this
change for those cases it will end up returning the refresh rate
calculated using mode->clock which will result in a different value
now.
So is the recommendation that even in the case of compression
mode->clock should always hold
uncompressed pixel clock value because with this part of the change we
will now get a different value when we call this API.
Yes. The mode remains the same regardless of compression, and
compression is just an implementation detail of the transport.
You may need to maintain separate "physical port clock" and "logical
port clock" for DSC, where the latter is a function of the former and
the DSC parameters. And then you can see if your logical port clock
provides enough bandwidth for your mode. But this is up to your driver
and encoder implementation.
BR,
Jani.
Thanks for the information. We will make changes to our driver to
accommodate the changes in the
drm_mode_vrefresh API.
Thanks
Abhinav
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 03/17] drm: Nuke mode->vrefresh
On Fri, 03 Apr 2020, abhin...@codeaurora.org wrote:
> On 2020-04-03 13:39, Ville Syrjala wrote:
>> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
>> index fec1c33b3045..e3d5f011f7bd 100644
>> --- a/drivers/gpu/drm/drm_modes.c
>> +++ b/drivers/gpu/drm/drm_modes.c
>> @@ -759,9 +759,7 @@ int drm_mode_vrefresh(const struct drm_display_mode
>> *mode)
>> {
>> int refresh = 0;
>>
>> -if (mode->vrefresh > 0)
>> -refresh = mode->vrefresh;
>
> The mode->vrefresh has been replaced with calling this API in all its
> usages.
> However in this API, the above if statement was returning the vrefresh
> if it was already
> set. mode->clock is holding the pixel clock . So this will not cause any
> issues in non-compressed cases.
> In case of compression like DSC, the pixel
> clock will be different based on the compression ratio hence the
> mode->clock will change but fps will not.
> So we did have usages in our downstream driver where we would use this
> API and the refresh rate
> returned will be the mode->vrefresh which did not change but after this
> change for those cases it will end up returning the refresh rate
> calculated using mode->clock which will result in a different value now.
> So is the recommendation that even in the case of compression
> mode->clock should always hold
> uncompressed pixel clock value because with this part of the change we
> will now get a different value when we call this API.
Yes. The mode remains the same regardless of compression, and
compression is just an implementation detail of the transport.
You may need to maintain separate "physical port clock" and "logical
port clock" for DSC, where the latter is a function of the former and
the DSC parameters. And then you can see if your logical port clock
provides enough bandwidth for your mode. But this is up to your driver
and encoder implementation.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 03/17] drm: Nuke mode->vrefresh
Hi Ville,
Thank you for the patch.
On Fri, Apr 03, 2020 at 11:39:54PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> Get rid of mode->vrefresh and just calculate it on demand. Saves
> a bit of space and avoids the cached value getting out of sync
> with reality.
>
> Mostly done with cocci, with the following manual fixups:
> - Remove the now empty loop in drm_helper_probe_single_connector_modes()
> - Fix __MODE() macro in ch7006_mode.c
> - Fix DRM_MODE_ARG() macro in drm_modes.h
> - Remove leftover comment from samsung_s6d16d0_mode
> - Drop the TODO
>
> @@
> @@
> struct drm_display_mode {
> ...
> - int vrefresh;
> ...
> };
>
> @@
> identifier N;
> expression E;
> @@
> struct drm_display_mode N = {
> - .vrefresh = E
> };
>
> @@
> identifier N;
> expression E;
> @@
> struct drm_display_mode N[...] = {
> ...,
> {
> - .vrefresh = E
> }
> ,...
> };
>
> @@
> expression E;
> @@
> {
> DRM_MODE(...),
> - .vrefresh = E,
> }
>
> @@
> identifier M, R;
> @@
> int drm_mode_vrefresh(const struct drm_display_mode *M)
> {
> ...
> - if (M->vrefresh > 0)
> - R = M->vrefresh;
> - else
> if (...) {
> ...
> }
> ...
> }
>
> @@
> struct drm_display_mode *p;
> expression E;
> @@
> (
> - p->vrefresh = E;
> |
> - p->vrefresh
> + drm_mode_vrefresh(p)
> )
>
> @@
> struct drm_display_mode s;
> expression E;
> @@
> (
> - s.vrefresh = E;
> |
> - s.vrefresh
> + drm_mode_vrefresh()
> )
>
> @@
> expression E;
> @@
> - drm_mode_vrefresh(E) ? drm_mode_vrefresh(E) : drm_mode_vrefresh(E)
> + drm_mode_vrefresh(E)
>
> @find_substruct@
> identifier X;
> identifier S;
> @@
> struct X {
> ...
> struct drm_display_mode S;
> ...
> };
>
> @@
> identifier find_substruct.S;
> expression E;
> identifier I;
> @@
> {
> .S = {
> - .vrefresh = E
> }
> }
>
> @@
> identifier find_substruct.S;
> identifier find_substruct.X;
> expression E;
> identifier I;
> @@
> struct X I[...] = {
> ...,
> .S = {
> - .vrefresh = E
> }
> ,...
> };
>
> v2: Drop TODO
>
> Cc: Andrzej Hajda
> Cc: Neil Armstrong
> Cc: Laurent Pinchart
> Cc: Jonas Karlman
> Cc: Jernej Skrabec
> Cc: Inki Dae
> Cc: Joonyoung Shim
> Cc: Seung-Woo Kim
> Cc: Kyungmin Park
> Cc: Linus Walleij
> Cc: CK Hu
> Cc: Philipp Zabel
> Cc: Ben Skeggs
> Cc: Thierry Reding
> Cc: Sam Ravnborg
> Cc: Jerry Han
> Cc: Icenowy Zheng
> Cc: Jagan Teki
> Cc: Stefan Mavrodiev
> Cc: Robert Chiras
> Cc: "Guido Günther"
> Cc: Purism Kernel Team
> Cc: Benjamin Gaignard
> Cc: Vincent Abriou
> Cc: VMware Graphics
> Cc: Thomas Hellstrom
> Cc: linux-amlo...@lists.infradead.org
> Cc: nouv...@lists.freedesktop.org
> Reviewed-by: Emil Velikov
> Reviewed-by: Sam Ravnborg
> Acked-by: Linus Walleij
> Signed-off-by: Ville Syrjälä
> ---
> Documentation/gpu/todo.rst| 20 --
> drivers/gpu/drm/bridge/sii902x.c | 2 +-
> drivers/gpu/drm/drm_client_modeset.c | 2 +-
> drivers/gpu/drm/drm_edid.c| 328 +-
> drivers/gpu/drm/drm_modes.c | 9 +-
> drivers/gpu/drm/drm_probe_helper.c| 3 -
> drivers/gpu/drm/exynos/exynos_hdmi.c | 5 +-
> drivers/gpu/drm/exynos/exynos_mixer.c | 2 +-
> drivers/gpu/drm/i2c/ch7006_mode.c | 1 -
> drivers/gpu/drm/i915/display/intel_display.c | 1 -
> .../drm/i915/display/intel_display_debugfs.c | 4 +-
> drivers/gpu/drm/i915/display/intel_dp.c | 10 +-
> drivers/gpu/drm/i915/display/intel_tv.c | 3 -
> drivers/gpu/drm/mcde/mcde_dsi.c | 6 +-
> drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 4 +-
> drivers/gpu/drm/mediatek/mtk_hdmi.c | 2 +-
> drivers/gpu/drm/meson/meson_venc_cvbs.c | 2 -
> drivers/gpu/drm/nouveau/nouveau_connector.c | 5 +-
> drivers/gpu/drm/panel/panel-arm-versatile.c | 4 -
> drivers/gpu/drm/panel/panel-boe-himax8279d.c | 3 +-
> .../gpu/drm/panel/panel-boe-tv101wum-nl6.c| 6 +-
> drivers/gpu/drm/panel/panel-elida-kd35t133.c | 3 +-
> .../gpu/drm/panel/panel-feixin-k101-im2ba02.c | 3 +-
> .../drm/panel/panel-feiyang-fy07024di26a30d.c | 3 +-
> drivers/gpu/drm/panel/panel-ilitek-ili9322.c | 7 -
> drivers/gpu/drm/panel/panel-ilitek-ili9881c.c | 3 +-
> drivers/gpu/drm/panel/panel-innolux-p079zca.c | 4 +-
> .../gpu/drm/panel/panel-jdi-lt070me05000.c| 3 +-
> .../drm/panel/panel-kingdisplay-kd097d04.c| 3 +-
> .../drm/panel/panel-leadtek-ltk500hd1829.c| 3 +-
> drivers/gpu/drm/panel/panel-lg-lb035q02.c | 1 -
> drivers/gpu/drm/panel/panel-lg-lg4573.c | 3 +-
> drivers/gpu/drm/panel/panel-nec-nl8048hl11.c | 1 -
> drivers/gpu/drm/panel/panel-novatek-nt35510.c | 1 -
> drivers/gpu/drm/panel/panel-novatek-nt39016.c | 1 -
> .../drm/panel/panel-olimex-lcd-olinuxino.c| 1 -
> .../gpu/drm/panel/panel-orisetech-otm8009a.c | 3 +-
> .../drm/panel/panel-osd-osd101t2587-53ts.c| 3 +-
>
