Re: [PATCH 04/12] drm: Nuke mode->vrefresh

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 25, 2020 at 05:45:06PM +0200, Ville Syrjälä wrote:
> On Tue, Feb 25, 2020 at 04:19:27PM +0100, Andrzej Hajda wrote:
> > On 25.02.2020 12:21, Ville Syrjälä wrote:
> > > On Mon, Feb 24, 2020 at 03:14:54PM +0100, Andrzej Hajda wrote:
> > >> On 19.02.2020 21:35, 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
> > >> ...
> > >>> diff --git a/drivers/gpu/drm/panel/panel-arm-versatile.c b/drivers/gpu/drm/panel/panel-arm-versatile.c
> > >>> index 41444a73c980..47b37fef7ee8 100644
> > >>> --- a/drivers/gpu/drm/panel/panel-arm-versatile.c
> > >>> +++ b/drivers/gpu/drm/panel/panel-arm-versatile.c
> > >>> @@ -143,7 +143,6 @@ static const struct versatile_panel_type versatile_panels[] = {
> > >>>  			.vsync_start = 240 + 5,
> > >>>  			.vsync_end = 240 + 5 + 6,
> > >>>  			.vtotal = 240 + 5 + 6 + 5,
> > >>> -			.vrefresh = 116,
> > >>
> > >> Are you sure vrefresh calculated (from totals and clock) is different
> > >> than this field? If not, we risk regressions.
> > >>
> > >> This case is OK, but there is plenty other cases.
> > > IIRC I did spot check a few of them. But which code exactly do you think
> > > is abusing vrefresh and thus could break?
> > 
> > 
> > I guess suspect/potential victim is every code which uses
> > drm_mode_vrefresh - after this patch the function can return different
> > value(if there are differences between provided and calculated vrefresh).
> > 
> > Quick examples where output of this function matters:
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c#L387
> 
> Already looks quite sketchy due to rounding.
> 
> > 
> > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c#L42
> 
> msleep() is in no way accurate so looks rather sketchy as well.
> 
> > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/tilcdc/tilcdc_crtc.c#L810
> 
> Another thing that suffers from rounding issues.
> 
> So to me these all look like code that someone should fix regardless.

OK, so I went ahead a wrote a bit of cocci [1] to find the bad apples.

Unfortunately it found a lot of strange stuff:
panel-sony-acx424akp.c:51/sony_acx424akp_vid_mode: 60 vs. 727 (.clock=330000 .htotal=480 + 15 + 0 + 15 .vtotal=864 + 14 + 1 + 11)
panel-sony-acx424akp.c:71/sony_acx424akp_cmd_mode: 60 vs. 711 (.clock=420160 .htotal=480 + 154 + 16 + 32 .vtotal=864 + 1 + 1 + 1)
panel-ilitek-ili9322.c:543/srgb_320x240_mode: 60 vs. 10168 (.clock=2453500 .htotal=320 + 359 + 1 + 241 .vtotal=262)
panel-ilitek-ili9322.c:587/yuv_640x320_mode: 60 vs. 7768 (.clock=2454000 .htotal=640 + 252 + 1 + 28 .vtotal=320 + 4 + 1 + 18)
panel-ilitek-ili9322.c:616/itu_r_bt_656_640_mode: 60 vs. 5358 (.clock=2454000 .htotal=640 + 3 + 1 + 272 .vtotal=500)
panel-ilitek-ili9322.c:557/srgb_360x240_mode: 60 vs. 16178 (.clock=2700000 .htotal=360 + 35 + 1 + 241 .vtotal=262)
panel-ilitek-ili9322.c:601/yuv_720x360_mode: 60 vs. 7071 (.clock=2700000 .htotal=720 + 252 + 1 + 24 .vtotal=360 + 4 + 1 + 18)
panel-ilitek-ili9322.c:631/itu_r_bt_656_720_mode: 60 vs. 5422 (.clock=2700000 .htotal=720 + 3 + 1 + 272 .vtotal=500)
panel-ilitek-ili9322.c:572/prgb_320x240_mode: 60 vs. 59725 (.clock=6400000 .htotal=320 + 38 + 1 + 50 .vtotal=262)
panel-lg-lg4573.c:200/default_mode: 60 vs. 57 (.clock=27000 .htotal=480 + 10 + 59 + 10 .vtotal=800 + 15 + 15 + 15)
panel-sitronix-st7789v.c:159/default_mode: 60 vs. 70 (.clock=7000 .htotal=240 + 38 + 10 + 10 .vtotal=320 + 8 + 4 + 4)
panel-arm-versatile.c:161/versatile_panels[]: 60 vs. 61 (.clock=25000 .htotal=640 + 24 + 96 + 24 .vtotal=480 + 11 + 2 + 32)
panel-arm-versatile.c:208/versatile_panels[]: 116 vs. 59 (.clock=5400 .htotal=240 + 10 + 10 + 20 .vtotal=320 + 2 + 2 + 2)
panel-arm-versatile.c:184/versatile_panels[]: 390 vs. 1523 (.clock=62500 .htotal=176 + 2 + 3 + 3 .vtotal=220 + 0 + 2 + 1)
panel-leadtek-ltk500hd1829.c:380/default_mode: 60 vs. 36 (.clock=41600 .htotal=720 + 50 + 50 + 50 .vtotal=1280 + 30 + 4 + 12)
panel-feixin-k101-im2ba02.c:394/k101_im2ba02_default_mode: 60 vs. 62 (.clock=70000 .htotal=800 + 20 + 20 + 20 .vtotal=1280 + 16 + 4 + 4)
panel-simple.c:1305/edt_etm043080dh6gp_mode: 60 vs. 67 (.clock=10870 .htotal=480 + 8 + 4 + 41 .vtotal=288 + 2 + 4 + 10)
panel-simple.c:3881/lg_acx467akm_7_mode: 60 vs. 72 (.clock=150000 .htotal=1080 + 2 + 2 + 2 .vtotal=1920 + 2 + 2 + 2)
panel-simple.c:2509/ortustech_com37h3m_mode: 60 vs. 67 (.clock=22153 .htotal=480 + 8 + 10 + 10 .vtotal=640 + 4 + 3 + 4)
panel-simple.c:2480/ontat_yx700wv03_mode: 60 vs. 59 (.clock=29500 .htotal=992 .vtotal=500)
panel-simple.c:1451/foxlink_fl500wvr00_a0t_mode: 60 vs. 55 (.clock=32260 .htotal=800 + 168 + 64 + 88 .vtotal=480 + 37 + 2 + 8)
panel-simple.c:2589/pda_91_00156_a0_mode: 60 vs. 68 (.clock=33300 .htotal=800 + 1 + 64 + 64 .vtotal=480 + 1 + 23 + 22)
panel-simple.c:2969/shelly_sca07010_bfn_lnn_mode: 60 vs. 68 (.clock=33300 .htotal=800 + 1 + 64 + 64 .vtotal=480 + 1 + 23 + 22)
panel-simple.c:1682/innolux_at070tn92_mode: 60 vs. 58 (.clock=33333 .htotal=800 + 210 + 20 + 46 .vtotal=480 + 22 + 23 + 10)
panel-simple.c:3260/vl050_8048nt_c01_mode: 60 vs. 58 (.clock=33333 .htotal=800 + 210 + 20 + 46 .vtotal=480 + 22 + 10 + 23)
panel-simple.c:1098/cdtech_s070wv95_ct16_mode: 60 vs. 72 (.clock=35000 .htotal=800 + 40 + 40 + 48 .vtotal=480 + 29 + 13 + 3)
panel-simple.c:788/auo_g104sn02_mode: 60 vs. 52 (.clock=40000 .htotal=800 + 40 + 216 + 128 .vtotal=600 + 10 + 35 + 2)
panel-simple.c:3201/tpk_f10a_0102_mode: 60 vs. 54 (.clock=45000 .htotal=1024 + 176 + 5 + 88 .vtotal=600 + 20 + 5 + 25)
panel-simple.c:2841/sharp_lq035q7db03_mode: 60 vs. 61 (.clock=5500 .htotal=240 + 16 + 7 + 5 .vtotal=320 + 9 + 1 + 7)
panel-simple.c:1279/edt_et035012dm6_mode: 60 vs. 61 (.clock=6500 .htotal=320 + 20 + 68 .vtotal=240 + 4 + 4 + 14)
panel-simple.c:2321/netron_dy_e231732_mode: 60 vs. 65 (.clock=66000 .htotal=1024 + 160 + 70 + 90 .vtotal=600 + 127 + 20 + 3)
panel-simple.c:1122/chunghwa_claa070wp03xg_mode: 60 vs. 57 (.clock=66770 .htotal=800 + 49 + 33 + 17 .vtotal=1280 + 1 + 7 + 15)
panel-simple.c:3824/lg_lh500wx1_sd03_mode: 60 vs. 61 (.clock=67000 .htotal=720 + 12 + 4 + 112 .vtotal=1280 + 8 + 4 + 12)
panel-simple.c:764/auo_g101evn010_mode: 60 vs. 58 (.clock=68930 .htotal=1280 + 82 + 2 + 84 .vtotal=800 + 8 + 2 + 6)
panel-simple.c:1169/chunghwa_claa101wb01_mode: 60 vs. 59 (.clock=69300 .htotal=1366 + 48 + 32 + 20 .vtotal=768 + 16 + 8 + 16)
panel-simple.c:2002/lemaker_bl035_rgb_002_mode: 60 vs. 65 (.clock=7000 .htotal=320 + 20 + 30 + 38 .vtotal=240 + 4 + 3 + 15)
panel-simple.c:2918/sharp_lq150x1lg11_mode: 60 vs. 65 (.clock=71100 .htotal=1024 + 168 + 64 + 88 .vtotal=768 + 37 + 2 + 8)
panel-simple.c:2214/logicpd_type_28_mode: 60 vs. 59 (.clock=9000 .htotal=480 + 3 + 42 + 2 .vtotal=272 + 2 + 11 + 3)
panel-simple.c:1525/giantplus_gpg482739qs5_mode: 60 vs. 59 (.clock=9000 .htotal=480 + 5 + 1 + 40 .vtotal=272 + 8 + 1 + 8)
panel-simple.c:1073/cdtech_s043wq26h_ct7_mode: 60 vs. 57 (.clock=9000 .htotal=480 + 5 + 5 + 40 .vtotal=272 + 8 + 8 + 8)
panel-simple.c:2613/qd43003c0_40_mode: 60 vs. 59 (.clock=9000 .htotal=480 + 8 + 4 + 39 .vtotal=272 + 4 + 10 + 2)
panel-simple.c:3124/ti_nspire_classic_lcd_mode[]: 60 vs. 123 (.clock=10000 .htotal=320 + 6 + 6 + 6 .vtotal=240 + 0 + 1 + 0)
panel-simple.c:3096/ti_nspire_cx_lcd_mode[]: 60 vs. 93 (.clock=10000 .htotal=320 + 50 + 6 + 38 .vtotal=240 + 3 + 1 + 17)

I presume a bunch of those are just lazyness, but there some
real oddballs in the mix for sure. CCing everyone...

[1]
@find_substruct@
identifier P, C;
@@
struct P {
...
	struct drm_display_mode C;
...
};

@submode@
identifier find_substruct.P, find_substruct.C, M;
expression CLK, HT, VT, VREF;
position POS;
@@
struct P M = {
...,
.C = {
	.clock = CLK@POS
	,...,
	.htotal = HT
	,...,
	.vtotal = VT
	,...,
	.vrefresh = VREF
}
,...
};

@submodes@
identifier find_substruct.P, find_substruct.C, A;
expression CLK, HT, VT, VREF;
position POS;
@@
struct P A[...] = {
...,
{
	.C = {
		.clock = CLK@POS
		,...,
		.htotal = HT
		,...,
		.vtotal = VT
		,...,
		.vrefresh = VREF
	}
}
,...
};

@mode@
identifier M;
expression CLK, HT, VT, VREF;
position POS;
@@
struct drm_display_mode M = {
	.clock = CLK@POS
	,...,
	.htotal = HT
	,...,
	.vtotal = VT
	,...,
	.vrefresh = VREF
};

@modes@
identifier A;
expression CLK, HT, VT, VREF;
position POS;
@@
struct drm_display_mode A[...] = {
	...,
	{
	...,
	.clock = CLK@POS
	,...,
	.htotal = HT
	,...,
	.vtotal = VT
	,...,
	.vrefresh = VREF
	,...
	}
	,...
};

@script:python@
ht << mode.HT;
vt << mode.VT;
clk << mode.CLK;
vref << mode.VREF;
m << mode.M;
pos << mode.POS;
@@
def mode_vrefresh(clk, ht, vt):
    return int(eval(clk)*1000.0/(eval(ht)*eval(vt))+0.5)
if mode_vrefresh(clk, ht, vt) != eval(vref):
   print "{}:{}/{}: {} vs. {} (.clock={} .htotal={} .vtotal={})".format(pos[0].file, pos[0].line, m, vref, mode_vrefresh(clk, ht, vt), clk, ht, vt)

@script:python@
ht << submode.HT;
vt << submode.VT;
clk << submode.CLK;
vref << submode.VREF;
m << submode.M;
pos << submode.POS;
@@
def mode_vrefresh(clk, ht, vt):
    return int(eval(clk)*1000.0/(eval(ht)*eval(vt))+0.5)
if mode_vrefresh(clk, ht, vt) != eval(vref):
   print "{}:{}/{}: {} vs. {} (.clock={} .htotal={} .vtotal={})".format(pos[0].file, pos[0].line, m, vref, mode_vrefresh(clk, ht, vt), clk, ht, vt)

@script:python@
ht << modes.HT;
vt << modes.VT;
clk << modes.CLK;
vref << modes.VREF;
m << modes.A;
pos << modes.POS;
@@
def mode_vrefresh(clk, ht, vt):
    return int(eval(clk)*1000.0/(eval(ht)*eval(vt))+0.5)
if mode_vrefresh(clk, ht, vt) != eval(vref):
   print "{}:{}/{}[]: {} vs. {} (.clock={} .htotal={} .vtotal={})".format(pos[0].file, pos[0].line, m, vref, mode_vrefresh(clk, ht, vt), clk, ht, vt)

@script:python@
ht << submodes.HT;
vt << submodes.VT;
clk << submodes.CLK;
vref << submodes.VREF;
m << submodes.A;
pos << submodes.POS;
@@
def mode_vrefresh(clk, ht, vt):
    return int(eval(clk)*1000.0/(eval(ht)*eval(vt))+0.5)
if mode_vrefresh(clk, ht, vt) != eval(vref):
   print "{}:{}/{}[]: {} vs. {} (.clock={} .htotal={} .vtotal={})".format(pos[0].file, pos[0].line, m, vref, mode_vrefresh(clk, ht, vt), clk, ht, vt)

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel