On Mon, Dec 01, 2014 at 03:04:50PM -0200, Paulo Zanoni wrote: > 2014-12-01 14:36 GMT-02:00 Daniel Vetter <daniel@xxxxxxxx>: > > On Mon, Dec 01, 2014 at 11:41:42AM -0200, Paulo Zanoni wrote: > >> 2014-11-26 16:17 GMT-02:00 Daniel Vetter <daniel@xxxxxxxx>: > >> > On Wed, Nov 26, 2014 at 01:37:07PM -0200, Paulo Zanoni wrote: > >> >> 2014-11-24 14:02 GMT-02:00 Daniel Vetter <daniel.vetter@xxxxxxxx>: > >> >> > Apparently PCH fifo underruns are tricky, we have plenty reports that > >> >> > we see the occasional underrun (especially at boot-up). > >> >> > > >> >> > So for a change let's see what happens when we don't re-enable pch > >> >> > fifo underrun reporting when the pipe is disabled. > >> >> > >> >> Does that mean you don't really know if this patch is going to fix something? > >> >> > >> >> I see what this patch does, but I don't really see what is its > >> >> benefit, besides "we'll get less bug reports". Is there any reason why > >> >> the underruns are expected to happen at this time? > >> >> > >> >> Please explain a little more. > >> > > >> > No reason really beyond "less bug reports" and "no reduction in underrun > >> > reporting abilities when the pipe is actually enabled". Only a reduction > >> > in how quickly we'll notice an underrun, but since we mostly need cpu fifo > >> > underruns for debugging wm issues I don't think that has an impact for > >> > developers either. fifo underruns are useful for debugging some modeset > >> > issues, but as soon as you do modeset we'll spot the underrun. > >> > > >> >> > This means that the > >> >> > kernel can't catch pch fifo underruns when they happen (except when > >> >> > all pipes are on on the pch). But we'll still catch underruns when > >> >> > disabling the pipe again. > >> >> > >> >> Are you sure the sentences above are correct? > >> > > >> > We always re-enable underrun reporting in the crtc_enable hooks. That > >> > still doesn't enable the interrupts (when some other pch pipe is off), but > >> > it updates the sw tracking. > >> > > >> > When we again disable the fifo underrun reporting we do check the status > >> > bits, so if an underrun happened we won't get the interrupt right away. > >> > But when you shut down the pipe we'll notice that an interrupt happened. > >> > > >> > So yeah, the above claim should be correct. > >> > > >> >> > So not a terrible reduction in test > >> >> > coverage. > >> >> > >> >> Yeah, I agree, but please provide a nice reason for it :) > >> > > >> > See my reply to this patch, a bug reporter came around and tested this as > >> > "it works". I really do send out patches without testing them at all for > >> > bug team work ;-) > >> > >> But why does he say it works? Aren't we just delaying the DRM_ERROR message? > > > > Before we only disabled pch underruns while we disable the pch. But at the > > end of the ->crtc_disable hook pch underrun reporting is enabled. > > > > With my patch we keep pch underrun reporting disabled until ->crtc_enable. > > It seems like doing a modeset on the other pipe also gives us underruns on > > disabled pipes somehow. Or at least that's my (bad) theory. > > I guess you convinced me on IRC that this is better than reverting the > DRM_ERROR to DRM_DEBUG_KMS. > > Anyway, the patch does what it says and doesn't seem to add any > regressions, so Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>. Since this dmesg noise is a regression, also Cc: stable@xxxxxxxxxxxxxxx Jani, can you pls pick this patch up? Perhaps for the record it would be best to paste the entire discussion here into the commit log, too. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
