Re: [PATCH 00/14] drm: Some more vblank timestampi changes

[Mario Kleiner <mario.kleiner.de@gmail.com>]

On 29/10/13 19:06, ville.syrjala@linux.intel.com wrote:
 > So I took another look at the vblank timestamping code, and got a bit
 > excited. The result is this patchset.
 >
 > Summary of changes:
 > - kill crtc->hwmode dependency
 > - eliminate a bunch of 64bit math
 > - fix timestamps for stereo and interlaced modes (on i915 at least)
 > - move the "early vbl irq" hack into radeon code
 > - add a similar hack to i915, but make it as finely targeted
 >    as possibly to minimize the chance of accidentally
 >    applying it in the wrong place
 >
 > The s/clock/crtc_clock change could use some radeon people to verify
 > whether changing radeon_atom_get_tv_timings() is enough to make
 > crtc_clock always populated.
 >
 > This series applies on top of Mario's
 > "Vblank timestamping improvements/fixes for Linux drm." series.
 >
 > Ville Syrjälä (14):
 >        drm: Pass the display mode to drm_calc_timestamping_constants()
 >        drm: Pass the display mode to 
drm_calc_vbltimestamp_from_scanoutpos()
 >        drm/i915: Kill hwmode save/restore
 >        drm/i915: Call drm_calc_timestamping_constants() earlier
 >        drm: Improve drm_calc_timestamping_constants() documentation
 >        drm: Simplify the math in drm_calc_timestamping_constants()
 >        drm/radeon: Populate crtc_clock in radeon_atom_get_tv_timings()
 >        drm: Use crtc_clock in drm_calc_timestamping_constants()
 >        drm: Change {pixel,line,frame}dur_ns from s64 to int
 >        drm/i915: Fix scanoutpos calculations for interlaced modes
 >        drm: Fix vblank timestamping constants for interlaced modes
 >        drm: Pass 'flags' from the caller to .get_scanout_position()
 >        drm/radeon: Move the early vblank IRQ fixup to 
radeon_get_crtc_scanoutpos()
 >        drm/i915: Add a kludge for DSL incrementing too late and ISR 
not working
 >

Hi Ville,

sorry this took way longer than expected. I've reviewed all of your 
patches. Nice cleanups, nice improvements!

You can add a ...

Reviewed-by: mario.kleiner.de@gmail.com

... to all of them.

Patches 0 - 11 and 14 are fine as they are. Only tiny formatting/comment 
fixes needed so they apply cleanly against the current drm-next.

Patch 12 and 13 need some small fixes, after applying those i'm fine 
with them. I'll send separate e-mails for those.

As far as testing goes, i had more encounters with Murphy's law in the 
last weeks than ever before, hence the long delay. You can add

Tested-by: mario.kleiner.de@gmail.com

to the drm core and intel patches with the following restrictions:

I was able to "sort of" test the patchset on Intel GMA-950 (Gen-3 hw).

- I didn't test if your interlaced scanout patches 10 and 11 work as 
expected, because i was testing the patches first, then reviewing them, 
so i didn't realize at that point testing interlaced mode would be 
neccessary. The patches look correct to me though. I no longer have easy 
access to that machine.

- My photodiode test equipment, which i need for Intel testing 
malfunctioned. Not sure if my testing hardware is dying, or if it is a 
bug in the kernels usb or serial/tty stack, or some kernel 
misconfiguration wrt. low-latency, but there was so much timing noise in 
my equipment that i couldn't test with it.

- As a workaround I ran the kms-timestamping for regular non-interlaced 
mode against the original userspace implementation of the same code in 
my own toolkit Psychtoolbox, which itself was verified with testing 
equipment to do the right thing on that GMA-950 netbook earlier this 
year. Difference was less than 40 microseconds and more likely caused 
due to userspace noisyness and off-by-one errors in Psychtoolbox than 
your code, so i assume that your code is essentially correct at least 
for non-interlaced scanout, and that the DRM core changes are therefore 
also correct. If you or somebody would want to try this test yourself i 
can guide you through the steps. Psychtoolbox is easily apt-get'able for 
Debian and at least Ubuntu.

- The next limitation of my testing is wrt. to your "early vbl irq 
handling" improvements (patch 14). I currently only have Gen3 hardware 
which doesn't exercise those code path at all, so while the patch looks 
correct, it's not really tested by me.

As far as Radeon testing goes, i can't test it at all atm. After already 
not working very stable at all for the last half year, my last machine 
with an AMD card died during bootup for this test, but not without 
trying to corrupt the filesystem on my development drive as a little 
post-christmas gift to me. If somebody has a AMD card and wants to test 
this, it could be tested against the Psychtoolbox userspace reference 
implementation, which was verified with very precise external hardware 
last time a couple of months ago. However, patch 13 needs some fixes or 
it would crash. The now dead PC wasn't mine, but i still have the AMD card.

I will try to hunt for a new PC soon, and hopefully will get your 
patches better tested during the -rc phase if they get merged into 3.14.

Apart from a NVidia card, my 2010 MacBookPro also has an integrated 
Gen-4 Intel card, connected to the internal panel via hardware mux, but 
so far i wasn't successfull at all to make use of it under Ubuntu 13.10. 
I can power on the card, make it detected by vgaswitcheroo/kms and 
manually switch the mux via some hacks, but it never boots into a 
functional desktop :(. I haven't tried very hard though with more recent 
kernels.

-mario

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx