Re: [PATCH] drm/tidss: dispc: Rewrite naive plane positioning code

From: Jyri Sarha <jsarha@ti.com>
To: "Daniel Vetter" <daniel@ffwll.ch>,
	"Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: praneeth@ti.com, Daniel Vetter <daniel.vetter@ffwll.ch>,
	dri-devel@lists.freedesktop.org, peter.ujfalusi@ti.com,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	laurent.pinchart@ideasonboard.com, sam@ravnborg.org
Subject: Re: [PATCH] drm/tidss: dispc: Rewrite naive plane positioning code
Date: Wed, 12 Feb 2020 15:51:59 +0200	[thread overview]
Message-ID: <f53e11ea-0aa7-3c2a-f34b-f593f1a27394@ti.com> (raw)
In-Reply-To: <20200211154156.GH2363188@phenom.ffwll.local>

On 11/02/2020 17:41, Daniel Vetter wrote:
> On Tue, Feb 11, 2020 at 04:40:21PM +0100, Daniel Vetter wrote:
>> On Tue, Feb 11, 2020 at 03:00:30PM +0200, Ville Syrjälä wrote:
>>> On Tue, Feb 11, 2020 at 11:11:34AM +0200, Tomi Valkeinen wrote:
>>>> Hi Ville,
>>>>
>>>> On 10/02/2020 18:03, Ville Syrjälä wrote:
>>>>
>>>>> The usual approach we follow in i915 for things that affect more
>>>>> than one plane is is to collect that state into the crtc state.
>>>>> That way we get to remember it for the planes that are not part
>>>>> of the current commit.
>>>>>
>>>>> And when we have state that affects more than one crtc that again
>>>>> get collected up one level up in what we call global state
>>>>> (basically drm_private_obj with less heavy handed locking scheme).
>>>>
>>>> I'm confused. Don't we always have the full state available? Why do you need to store state into 
>>>> custom crtc-state?
>>>>
>>>> Here we are interested in the x, y and z positions of all the planes on a crtc. Creating a custom 
>>>> state object and duplicating that information there seems a bit silly, as surely that information is 
>>>> tracked by DRM?
>>>
>>> You can have it if you add all the planes to the state, which can be
>>> a bit expensive. Another option would to peek into the planes' states
>>> that aren't in the commit, but that's quite gross due to bypassing
>>> the normal locking rules and instead relying on the crtc mutex to
>>> sufficiently protect the plane states as well. And I suspect trying
>>> to do said peeking during the commit phase when the locks have
>>> already been dropped will end badly.
>>
>> Yup, don't peek outside of atomic_check.
>>
>> Also the peeking only works for planes associated to the crtc. Either
>> because that's how the hw works (i915 has fixed plane routing).
>>
>> Now if this is only about all the planes currently active on a crtc, then
>> you the helpers will already add all those plane states for you, and you
>> can just walk them in your commit function. Not exactly sure what you need
>> here.
> 
> See drm_atomic_add_affected_planes() in case you're rolling your own
> stuff.
Thanks,
This looks to be exactly what I needed.

BR,
Jyri
-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel