Re: [PATCH v3 2/4] drm/i915: Wait for pending flips in intel_pipe_set_base()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 30, 2012 at 04:44:04PM +0100, Daniel Vetter wrote:
> On Fri, Nov 30, 2012 at 3:01 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > I had another look at this, and I think you're aiming for something other
> > than what this patch is trying to do.
> 
> Oh, I know that I'm trying to volunteer you to do a bit more ... it's
> why I'm the bastard maintainer ;-)
> 
> > The thing is that we are holding struct_mutex while waiting for pending
> > flips here, so we just need to get out asap to allow the reset work do
> > its thing.
> 
> Hm, I didn't notice that we're holding the struct_mutex there, so I
> guess we need to indeed bail out. Or rework the finsh_fb vs. gpu hang
> logic to not require the struct_mutex in finish_fb ...
> 
> > Completing pending page flips when a reset occurs is separate issue.
> > This code never did any of that, and I don't see why it should. We
> > would need to complete them anyway regardless of whether anyone is
> > currently waiting for them.
> 
> Yeah, that's my idea, but I haven't though through the details (see my
> ignorance with locking details above ...).
> 
> > Perhaps we can just call intel_finish_page_flip() from the reset
> > code and call it a day. I suppose doing that could end up unpinning
> > the current scanout buffer in case the display registers were never
> > written with the new values. One solution for that would be to always
> > do a set_base() after a reset. That would make sure we end up showing
> > the latest buffer at least, although the contents could be total
> > garbage.
> 
> Hm, I think first we should aim to no longer hang either the kernel or
> leave stuck userspace (since the drm_event never shows up) behind. Atm
> a gpu hang is allowed to corrupt pretty much everything. Later on,
> when we successfully avoid the hung kernel/userspace problems we can
> worry about making it look decent. Judging by how long it took to get
> basic reset working somewhat okish, it'll take a while. And we need
> some form of testcase to exercise the code.

At least the current patch would avoid having the process stuck in
D state. So I think it's better than nothing.

I do have other things on my plate, so I don't want to spend any more
time on this currently.

-- 
Ville Syrjälä
Intel OTC