Re: [PATCH v3 7/8] drm: Connector helper function to release resources

From: "Lankhorst, Maarten" <maarten.lankhorst@intel.com>
To: "daniel@ffwll.ch" <daniel@ffwll.ch>,
	"Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
Cc: "daniel.vetter@ffwll.ch" <daniel.vetter@ffwll.ch>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"bskeggs@redhat.com" <bskeggs@redhat.com>,
	"alexander.deucher@amd.com" <alexander.deucher@amd.com>,
	"harry.wentland@amd.com" <harry.wentland@amd.com>
Subject: Re: [PATCH v3 7/8] drm: Connector helper function to release resources
Date: Mon, 27 Feb 2017 07:42:18 +0000	[thread overview]
Message-ID: <1488181337.26794.1.camel@intel.com> (raw)
In-Reply-To: <20170226200012.b5yn7vfjyhdd2pik@phenom.ffwll.local>

Daniel Vetter schreef op zo 26-02-2017 om 21:00 [+0100]:
> On Fri, Feb 24, 2017 at 12:52:53AM +0000, Pandiyan, Dhinakaran wrote:
> > 
> > On Thu, 2017-02-16 at 09:09 +0000, Lankhorst, Maarten wrote:
> > > 
> > > Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
> > > > 
> > > > On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> > > > <dhinakaran.pandiyan@intel.com> wrote:
> > > > > 
> > > > > On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
> > > > > > 
> > > > > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55
> > > > > > [+0000]:
> > > > > > > 
> > > > > > > > 
> > > > > > > > Could we deal with the VCPI state separately in
> > > > > > > > intel_modeset_checks,
> > > > > > > > like we do with dpll?
> > > > > > > 
> > > > > > > We'd want to release the VCPI slots before they are
> > > > > > > acquired in
> > > > > > > ->compute_config(). intel_modeset_checks() will be too
> > > > > > > late to
> > > > > > > release
> > > > > > > them. Are you suggesting both acquiring and releasing
> > > > > > > slots
> > > > > > > should be
> > > > > > > done in intel_modeset_checks()?
> > > > > > 
> > > > > > That makes things a bit more nasty. Maybe add a
> > > > > > conn_funcs->atomic_check that always gets called, something
> > > > > > like
> > > > > > I did
> > > > > > below?
> > > > > > 
> > > > > > I'd love to use it for some atomic connector properties
> > > > > > too.
> > > > > 
> > > > > 
> > > > > Adding and unconditionally calling conn_funcs->atomic_check()
> > > > > should be
> > > > > doable. It also follows the pattern we have for encoders and
> > > > > CRTCs.
> > > > > But
> > > > > I'll have to move the connector->state->crtc state checks
> > > > > inside
> > > > > the
> > > > > function.
> > > > 
> > > > Adding ->atomic_check that's unconditionally called sounds
> > > > troubling,
> > > > because all the other ->atomic_check functions are _only_
> > > > called when
> > > > enabling stuff. ->atomic_release sounds much better to me, and
> > > > from a
> > > > helper pov DK's patch above is the right place.
> > > 
> > > Having an atomic check would be nice for implementing connector
> > > properties. Some of them may need to be validated regardless of
> > > crtc.
> > > 
> > 
> > Can we add this later when we need state validation that is
> > appropriate
> > for an ->atomic_check()? 
> 
> +1 on not solving problems we don't have yet :-) If we'd write code
> for
> every eventuality that we can come up with, we'd get nothing done.
> And
> ime, such unused code will also be full of bugs.
> 
> Discussing issues like this is still good and useful, just to make
> sure we
> have a coherent plan for the eventual future, once it happens.

Near future, I'm working on converting i915 specific connector
properties to atomic, and it would be nice if I had a connector atomic
check function like this. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx