Re: [PATCH v2 03/17] drm: Nuke mode->vrefresh

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Ville,

Thank you for the patch.

On Fri, Apr 03, 2020 at 11:39:54PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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(&s)
> )
> 
> @@
> 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 <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: CK Hu <ck.hu@mediatek.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Ben Skeggs <bskeggs@redhat.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Jerry Han <hanxu5@huaqin.corp-partner.google.com>
> Cc: Icenowy Zheng <icenowy@aosc.io>
> Cc: Jagan Teki <jagan@amarulasolutions.com>
> Cc: Stefan Mavrodiev <stefan@olimex.com>
> Cc: Robert Chiras <robert.chiras@nxp.com>
> Cc: "Guido Günther" <agx@sigxcpu.org>
> Cc: Purism Kernel Team <kernel@puri.sm>
> Cc: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> Cc: Vincent Abriou <vincent.abriou@st.com>
> Cc: VMware Graphics <linux-graphics-maintainer@vmware.com>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: linux-amlogic@lists.infradead.org
> Cc: nouveau@lists.freedesktop.org
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  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 +-
>  .../drm/panel/panel-panasonic-vvx10f034n00.c  |   3 +-
>  .../drm/panel/panel-raspberrypi-touchscreen.c |   4 +-
>  drivers/gpu/drm/panel/panel-raydium-rm67191.c |   3 +-
>  drivers/gpu/drm/panel/panel-raydium-rm68200.c |   3 +-
>  .../drm/panel/panel-rocktech-jh057n00900.c    |   5 +-
>  drivers/gpu/drm/panel/panel-ronbo-rb070d30.c  |   1 -
>  drivers/gpu/drm/panel/panel-samsung-s6d16d0.c |   6 -
>  drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c |   4 +-
>  .../gpu/drm/panel/panel-samsung-s6e63j0x03.c  |   3 +-
>  drivers/gpu/drm/panel/panel-samsung-s6e63m0.c |   3 +-
>  .../panel/panel-samsung-s6e88a0-ams452ef01.c  |   1 -
>  drivers/gpu/drm/panel/panel-seiko-43wvf1g.c   |   3 +-
>  .../gpu/drm/panel/panel-sharp-lq101r1sx01.c   |   3 +-
>  .../gpu/drm/panel/panel-sharp-ls037v7dw01.c   |   1 -
>  .../gpu/drm/panel/panel-sharp-ls043t1le01.c   |   3 +-
>  drivers/gpu/drm/panel/panel-simple.c          |  87 +----
>  drivers/gpu/drm/panel/panel-sitronix-st7701.c |   2 +-
>  .../gpu/drm/panel/panel-sitronix-st7789v.c    |   3 +-
>  drivers/gpu/drm/panel/panel-sony-acx424akp.c  |   2 -
>  drivers/gpu/drm/panel/panel-sony-acx565akm.c  |   1 -
>  drivers/gpu/drm/panel/panel-tpo-td028ttec1.c  |   1 -
>  drivers/gpu/drm/panel/panel-tpo-td043mtea1.c  |   1 -
>  drivers/gpu/drm/panel/panel-tpo-tpg110.c      |   5 -
>  drivers/gpu/drm/panel/panel-truly-nt35597.c   |   1 -
>  .../gpu/drm/panel/panel-xinpeng-xpp055c272.c  |   3 +-
>  drivers/gpu/drm/sti/sti_hda.c                 |   1 -
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c           |   2 -
>  include/drm/drm_modes.h                       |  12 +-
>  66 files changed, 218 insertions(+), 417 deletions(-)

[snip]

> diff --git a/drivers/gpu/drm/mcde/mcde_dsi.c b/drivers/gpu/drm/mcde/mcde_dsi.c
> index 7af5ebb0c436..52031d826f2c 100644
> --- a/drivers/gpu/drm/mcde/mcde_dsi.c
> +++ b/drivers/gpu/drm/mcde/mcde_dsi.c
> @@ -538,7 +538,7 @@ static void mcde_dsi_setup_video_mode(struct mcde_dsi *d,
>  	 */
>  	/* (ps/s) / (pixels/s) = ps/pixels */
>  	pclk = DIV_ROUND_UP_ULL(1000000000000,
> -				(mode->vrefresh * mode->htotal * mode->vtotal));
> +				(drm_mode_vrefresh(mode) * mode->htotal * mode->vtotal));

Shouldn't you just use the pixel clock here ?

Update: There's a patch further in this series that handles this, great
:-)

>  	dev_dbg(d->dev, "picoseconds between two pixels: %llu\n",
>  		pclk);
>  

[snip]

> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index 9a9a7f5003d3..ac80b1ac459c 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -59,7 +59,6 @@ nouveau_conn_native_mode(struct drm_connector *connector)
>  	int high_w = 0, high_h = 0, high_v = 0;
>  
>  	list_for_each_entry(mode, &connector->probed_modes, head) {
> -		mode->vrefresh = drm_mode_vrefresh(mode);
>  		if (helper->mode_valid(connector, mode) != MODE_OK ||
>  		    (mode->flags & DRM_MODE_FLAG_INTERLACE))
>  			continue;
> @@ -80,12 +79,12 @@ nouveau_conn_native_mode(struct drm_connector *connector)
>  			continue;
>  
>  		if (mode->hdisplay == high_w && mode->vdisplay == high_h &&
> -		    mode->vrefresh < high_v)
> +		    drm_mode_vrefresh(mode) < high_v)

Here too, should the logic be modified to use the clock ?

This can be addressed in a separate patch, so for this,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  			continue;
>  
>  		high_w = mode->hdisplay;
>  		high_h = mode->vdisplay;
> -		high_v = mode->vrefresh;
> +		high_v = drm_mode_vrefresh(mode);
>  		largest = mode;
>  	}
> 

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic