Re: [PATCH 0/9] drm/i915: Check pixel clock when setting mode

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jul 03, 2015 at 01:49:17PM +0100, Chris Wilson wrote:
> On Fri, Jul 03, 2015 at 02:35:48PM +0300, Mika Kahola wrote:
> > From EDID we can read and request higher pixel clock than
> > our HW can support. This set of patches add checks if
> > requested pixel clock is lower than the one supported by the HW.
> > The requested mode is discarded if we cannot support the requested
> > pixel clock. For example for Cherryview
> > 
> > 'cvt 2560 1600 60' gives
> > 
> > # 2560x1600 59.99 Hz (CVT 4.10MA) hsync: 99.46 kHz; pclk: 348.50 MHz
> > Modeline "2560x1600_60.00"  348.50  2560 2760 3032 3504  1600 1603 1609 1658 -hsync +vsync
> > 
> > where pixel clock 348.50 MHz is higher than the supported 304 MHz.
> > 
> > The checks are implemented for DisplayPort, HDMI, LVDS, DVO, SDVO, DSI,
> > CRT, TV, and DP-MST.
> 
> Why do I get the feeling that there was a lot of duplicated code?

The problem on top is that this only changes the mode_valid callback as
used by the probe helpers. Which means userspace can still do an addmode
of something not supported and try to trick over the code into accepting
something it can't. That code is the stuff around compute_config.

Which means we have some unpretty duplication going on, both between the
probe and compute_config paths and across all the different encoder types.
For the later an easy solution would be to add a device-global mode_valid
function and integrate that into the probe helpers. Should be a helper
library vfunc, i.e. separate from the main display vtable.

For the duplication between probe code and modeset code we should at least
try to cross-check the results (i.e. make sure that anything the modeset
code taks is also considered valid by the probe code, the other way round
only works for single-pipe and is a bit tricky due to other constraints
like plane limits). One idea I had for at least the encoder specific
checks (e.g. hdmi dotclock limits) would be to call the compute_config
function from mode_valid with a minimal pipe_config and hope for the best.
But I think that's way too tricky code, so probably the only thing we can
do without creating really hard to read&maintain code is to cross-check
the inevitable duplication :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx