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

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 0/9] drm/i915: Check pixel clock when setting mode
Date: Mon, 6 Jul 2015 18:28:56 +0300	[thread overview]
Message-ID: <20150706152856.GP5176@intel.com> (raw)
In-Reply-To: <20150706152148.GT2156@phenom.ffwll.local>

On Mon, Jul 06, 2015 at 05:21:48PM +0200, Daniel Vetter wrote:
> 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 :(

I tried to look at sharing the checking code between .mode_valid() and
mdoeset a while back but it turned into a bit of a nightmare when I
started to think about stereo 3D. To do the checks properly for stereo
3D we'd basically need to feed the mode through
drm_mode_set_crtcinfo(CRTC_STEREO_DOUBLE) and then check the crtc_
timings instead of the normal ones. So that would mean changing all the
.mode_valid() callbacks, and when I started down that path I landed
somewhere in DVO land and couldn't even figure out what limitations
.mode_valid() functions were trying to check. At that point I gave up,
and I also suggested to Mika that he first just look at adding the
checks to the .mode_valid() callbacks and not worry too much about
stereo 3D. I think that's a good enough first step.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx