On Mon, Jul 06, 2015 at 06:28:56PM +0300, Ville Syrjälä wrote: > 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. Yeah makes sense as a first step. We should still look into how we could at least share most of this across encoders a bit. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
