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

[Ville Syrjälä]

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 at 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.

-- 
Ville Syrjälä
Intel