From: Daniel Vetter <daniel@ffwll.ch>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 03/12] drm/i915: Reduce the time we hold struct mutex in sprite update_plane code
Date: Fri, 4 Oct 2013 14:41:27 +0200 [thread overview]
Message-ID: <20131004124126.GP31334@phenom.ffwll.local> (raw)
In-Reply-To: <20131004104008.GB9395@intel.com>
On Fri, Oct 04, 2013 at 01:40:08PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 04, 2013 at 11:21:20AM +0100, Chris Wilson wrote:
> > On Tue, Oct 01, 2013 at 06:02:12PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > We used to call the entire intel specific update_plane hook while
> > > holding struct_mutex. Actually we only need to hold struct_mutex while
> > > pinning/unpinning the obj. The plane state itself is protected by the
> > > kms locks, and as the object is pinned we can dig out the offset and
> > > tiling information from it without fearing that it would change
> > > underneath us.
> > >
> > > So now we don't need to drop and reacquire the lock around the
> > > wait_for_vblank. Also we will need another wait_for_vblank in the IVB
> > > specific update_plane hook, and this way we don't need to worry about
> > > struct_mutex there either.
> > >
> > > Also move the intel_plane->obj=NULL assignment outside strut_mutex in
> > > disable_plane to make it clear that it's not protected by struct_mutex.
> >
> > intel_update_fbc() needs to be taken out and shot. It needs the mode
> > lock, crtc lock and the struct_mutex.
> >
> > This patch looks fine, but anything touching fbc just makes me want to
> > curl up in a corner and whimper. Friends don't let friends enable fbc!
>
> Yeah, I have to fight the urge to beat that guy into shape every time I
> come across it. I fear it's going to be another rathole, which is why
> I'm trying to hold off until I've managed to clear my board of other
> tasks a bit.
Wrt cleaning up the fbc rathole (and it's one, same applies to psr btw):
We need to have some illusion of testcases first:
- Checks that screen updates work using the crc stuff.
- Functional checks that we actually enter psr/use compressed buffers. And
they _must_ fail if QA botches their setup somehow (not like the current
pc8+ test which is too polite and just skips).
- Integration into the pipe config and plane config stuff for atomic
modesets and nuclear pageflips and what not else.
This can easily cost us a few souls I think ;-)
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
next prev parent reply other threads:[~2013-10-04 12:41 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-10-01 15:02 [PATCH 00/12] drm/i915: Some cleanups and fixes to the sprite code v2 ville.syrjala
2013-10-01 15:02 ` [PATCH 01/12] drm/i915: Set primary_disabled in intel_{enable, disable}_plane ville.syrjala
2013-10-01 15:02 ` [PATCH v2 02/12] drm/i915: Allow sprites to be configured on a disabled pipe ville.syrjala
2013-10-01 15:02 ` [PATCH 03/12] drm/i915: Reduce the time we hold struct mutex in sprite update_plane code ville.syrjala
2013-10-04 10:21 ` Chris Wilson
2013-10-04 10:40 ` Ville Syrjälä
2013-10-04 12:41 ` Daniel Vetter [this message]
2013-10-01 15:02 ` [PATCH 04/12] drm/i915: Kill a goto from sprite disable code ville.syrjala
2013-10-01 15:02 ` [PATCH 05/12] drm/i915: Do a bit of cleanup in the sprite code ville.syrjala
2013-10-01 15:02 ` [PATCH 06/12] drm/i915: Save user requested plane coordinates only on success ville.syrjala
2013-10-01 15:02 ` [PATCH 07/12] drm/i915: Do the fbc vs. primary plane enable/disable in the right order ville.syrjala
2013-10-04 10:24 ` Chris Wilson
2013-10-01 15:02 ` [PATCH 08/12] drm/i915: Enable/disable IPS when primary is enabled/disabled ville.syrjala
2013-10-01 15:02 ` [PATCH 09/12] drm/i915: Rename intel_flush_display_plane to intel_flush_primary_plane ville.syrjala
2013-10-01 15:02 ` [PATCH 10/12] drm/i915: Rename intel_{enable, disable}_plane to intel_{enable, disable}_primary_plane ville.syrjala
2013-10-01 15:02 ` [PATCH 11/12] drm/i915: WARN if primary plane state doesn't match expectations ville.syrjala
2013-10-01 15:02 ` [PATCH 12/12] drm/i915: Flush primary plane changes in sprite code ville.syrjala
2013-10-04 10:28 ` [PATCH 00/12] drm/i915: Some cleanups and fixes to the sprite code v2 Chris Wilson
2013-10-07 9:04 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131004124126.GP31334@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=intel-gfx@lists.freedesktop.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.