Re: [Intel-gfx] [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode

From: Daniel Vetter <daniel@ffwll.ch>
To: Manasi Navare <manasi.d.navare@intel.com>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>,
	intel-gfx <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode
Date: Tue, 25 Oct 2016 08:40:05 +0200	[thread overview]
Message-ID: <20161025064005.kiymdqtitislhyia@phenom.ffwll.local> (raw)
In-Reply-To: <20161024220847.GA4039@intel.com>

On Mon, Oct 24, 2016 at 03:08:48PM -0700, Manasi Navare wrote:
> On Mon, Oct 24, 2016 at 09:12:35AM +0200, Daniel Vetter wrote:
> > On Mon, Oct 24, 2016 at 9:00 AM, Manasi Navare
> > <manasi.d.navare@intel.com> wrote:
> > >> I guess we just need to do some additional work on top to make sure the
> > >> vblank ioctl can't see invalid state. Which would then again make upfront
> > >> link trainig possible ...
> 
> Thanks for the explanation. So the atomic_commit code in the driver
> calls drm_crtc_send_vblank_event(crtc, crtc->state->event);, is this
> what needs to be filtered to be seen by Vblank IOCTL?
> 
> 
> > >
> > > Could you share some more details on what work would need to be done
> > > for vblank? Then I can investigate it a little bit more tomor during the day.
> > 
> > So the trouble is that drm_irq.c is still from the old pre-kms days.
> > Which means it deals in int pipe instead of struct drm_crtc *, among
> > other things. But we can't simply switch over because some old drivers
> > (pre-kms ones) still depend upon that old code. Hence we need:
> > 
> > 1. Duplicate most of the code in drm_irq.c into a new
> > drm_crtc_vblank.c, which is only used for kms drivers. And in the
> > ioctl code have a DRIVER_MODESET check and either call the new kms
> > version (in drm_crtc_vblank.c) or the old one (still in drm_irq.c).
> 
> What functions need to be duplicated?

Figuring out the exact list is way this is painful. Since we don't need
everything, but just enough to make the new drm_crtc_vblank.c work. And
then as a second step we probably should move all the functions starting
with drm_crtc_vblank_ which are exported to drivers over to that file too.

> Which IOCTL are we talking about here?  Do you mean in the atomic_commit_tail
> where we check for driver modeset, is where we should then call the new kms version
> of drm_crtc_send_vblank_event()?
> > 
> > 2. Convert the int pipe into a drm_crtc pointer in the new kms code.
> > For prettiness we could (try) to do that as much as possible.
> > 
> > 3. Grab the drm_crtc->mutex in the ioctl code to block these races.
> 
> Again which IOCTL needs to grab the drm_crtc->mutex?

The vblank ioctl in drm_irq.c, implemented by drm_wait_vblank.

> What is the end goal of thsi task, what race conditions are we trying to avoid
> in case of these modesets during link training failures?

Modeset code calls drm_crtc_vblank_on/off when doing a full modeset. That
changes the vblank status, which can be observed through the vblank ioctl
by userspace: In between the calls to _off/_on it will return -EINVAL,
instead of the last vblank timestamp (for a query) or doing the vblank
wait (otherwise), which is what it should do.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel