Re: [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state

From: Matt Roper <matthew.d.roper@intel.com>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org,
	"Ville Syrjälä" <ville.syrjala@intel.com>
Subject: Re: [RFC 1/3] drm/i915: Roll intel_crtc->atomic into intel_crtc_state
Date: Tue, 1 Sep 2015 08:30:05 -0700	[thread overview]
Message-ID: <20150901153005.GZ13406@intel.com> (raw)
In-Reply-To: <55E53683.8020005@linux.intel.com>

On Tue, Sep 01, 2015 at 07:24:19AM +0200, Maarten Lankhorst wrote:
> Op 29-08-15 om 01:57 schreef Matt Roper:
> > Way back at the beginning of i915's atomic conversion I added
> > intel_crtc->atomic as a temporary dumping ground for "stuff to do
> > outside vblank evasion" flags since CRTC states weren't properly wired
> > up and tracked at that time.  We've had proper CRTC state tracking for a
> > while now, so there's really no reason for this hack to continue to
> > exist.  Moving forward we want to store intermediate crtc/plane state
> > data for modesets in addition to the final state, so moving these fields
> > into the proper state object allows us to properly compute them for both
> > the intermediate and final state.
> >
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> Can I shoot this patch down? It's better to add a field 'wm_changed'
> to the crtc_state, which gets reset to false for each crtc_state
> duplication. It's needed for checking if a cs pageflip can be done for
> atomic. It would remove the duplication of some checks there.
> 
> The other atomic state members will die soon. I already have some
> patches to achieve that. :)
> 
> I'm not sure if an intermediate state is a good idea. Any code that
> disables a crtc should only be looking at the old state.
> pre_plane_update runs all stuff in preparation for disabling planes,
> while post_plane_update runs everything needed for enabling planes. So
> no need to split it up I think, maybe put in some intermediate
> watermarks in intel_atomic_state, but no need for a full crtc_state.

Well, the intermediate state stuff was requested by Ville in response to
my watermark series, so I posted these patches as an RFC to make sure I
was understanding what he was looking for properly.

Ville, can you comment?

Matt

> 
> After a modeset disable you should be able to put in any wm value in
> .crtc_enable because no plane will be active.

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx