Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation
On Thu, 2023-03-09 at 14:34 +0200, Ville Syrjälä wrote: > On Thu, Mar 09, 2023 at 12:09:55PM +0100, Maarten Lankhorst wrote: > > > > On 2023-03-09 12:04, Hogander, Jouni wrote: > > > On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote: > > > > On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst > > > > wrote: > > > > > Hey, > > > > > > > > > > On 2023-03-06 16:23, Souza, Jose wrote: > > > > > > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote: > > > > > > > As a fallback if we decide not to merge the frontbuffer > > > > > > > tracking, allow > > > > > > > i915 to keep its own implementation, and do the right > > > > > > > thing in > > > > > > > Xe. > > > > > > > > > > > > > > The frontbuffer tracking for Xe is still done per-fb, > > > > > > > while > > > > > > > i915 can > > > > > > > keep doing the weird intel_frontbuffer + i915_active > > > > > > > thing > > > > > > > without > > > > > > > blocking Xe. > > > > > > Please also disable PSR and FBC with this or at least add a > > > > > > way > > > > > > for users to disable those features. > > > > > > Without frontbuffer tracker those two features will break > > > > > > in some > > > > > > cases. > > > > > FBC and PSR work completely as expected. I don't remove > > > > > frontbuffer > > > > > tracking; I only remove the GEM parts. > > > > > > > > > > Explicit invalidation using pageflip or CPU rendering + > > > > > DirtyFB > > > > > continue > > > > > to work, as I validated on my laptop with FBC. > > > > Neither of which are relevant to the removal of the gem hooks. > > > > > > > > Like I already said ~10 times in the last meeting, we need a > > > > proper > > > > testcase. Here's a rough idea what it should do: > > > > > > > > prepare a batch with > > > > 1. spinner > > > > 2. something that clobbers the fb > > > > > > > > Then > > > > 1. grab reference crc > > > > 2. execbuffer > > > > 3. dirtyfb > > > > 4. wait long enough for fbc to recompress > > > > 5. terminate spinner > > > > 6. gem_sync > > > > 7. grab crc and compare with reference > > > > > > > > No idea what the current status of PSR+CRC is, so not sure > > > > whether we can actually test PSR or not. > > > > > > > CRC calculation doesn't work with PSR currently. PSR is disabled > > > if CRC > > > capture is requested. > > > > > > Are we supposed to support frontbuffer rendering using GPU? > > > > No other driver does that. > > Every driver does that when you run X w/o a compositor. Assuming > there is an actual GPU in there. > > > It's fine if DirtyFB hangs instead until the > > job it waits on completes. > > No one tried to make it just wait for the fence(s) w/o doing > a full blown atomic commit. It might work, but might also > still suck too much. I guess depends on how overloaded the GPU > is. > > What we could do is do a frontbuffer invalidate on dirtyfb > invocation, and then once the fence(s) signal we do a frontbuffer > flush. That would most closely match the gem hook behaviour, except > the invalidate comes in a bit later. The alternative would be to > skip the invalidate, which should still guarantee correctness in > the end, just with possibly jankier interactivity. > I have implemented this approach here: https://patchwork.freedesktop.org/series/116620/ Review/comments are welcome. BR, Jouni Högander
Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation
On Thu, Mar 09, 2023 at 12:09:55PM +0100, Maarten Lankhorst wrote: > > On 2023-03-09 12:04, Hogander, Jouni wrote: > > On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote: > >> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote: > >>> Hey, > >>> > >>> On 2023-03-06 16:23, Souza, Jose wrote: > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote: > > As a fallback if we decide not to merge the frontbuffer > > tracking, allow > > i915 to keep its own implementation, and do the right thing in > > Xe. > > > > The frontbuffer tracking for Xe is still done per-fb, while > > i915 can > > keep doing the weird intel_frontbuffer + i915_active thing > > without > > blocking Xe. > Please also disable PSR and FBC with this or at least add a way > for users to disable those features. > Without frontbuffer tracker those two features will break in some > cases. > >>> FBC and PSR work completely as expected. I don't remove frontbuffer > >>> tracking; I only remove the GEM parts. > >>> > >>> Explicit invalidation using pageflip or CPU rendering + DirtyFB > >>> continue > >>> to work, as I validated on my laptop with FBC. > >> Neither of which are relevant to the removal of the gem hooks. > >> > >> Like I already said ~10 times in the last meeting, we need a proper > >> testcase. Here's a rough idea what it should do: > >> > >> prepare a batch with > >> 1. spinner > >> 2. something that clobbers the fb > >> > >> Then > >> 1. grab reference crc > >> 2. execbuffer > >> 3. dirtyfb > >> 4. wait long enough for fbc to recompress > >> 5. terminate spinner > >> 6. gem_sync > >> 7. grab crc and compare with reference > >> > >> No idea what the current status of PSR+CRC is, so not sure > >> whether we can actually test PSR or not. > >> > > CRC calculation doesn't work with PSR currently. PSR is disabled if CRC > > capture is requested. > > > > Are we supposed to support frontbuffer rendering using GPU? > > No other driver does that. Every driver does that when you run X w/o a compositor. Assuming there is an actual GPU in there. > It's fine if DirtyFB hangs instead until the > job it waits on completes. No one tried to make it just wait for the fence(s) w/o doing a full blown atomic commit. It might work, but might also still suck too much. I guess depends on how overloaded the GPU is. What we could do is do a frontbuffer invalidate on dirtyfb invocation, and then once the fence(s) signal we do a frontbuffer flush. That would most closely match the gem hook behaviour, except the invalidate comes in a bit later. The alternative would be to skip the invalidate, which should still guarantee correctness in the end, just with possibly jankier interactivity. -- Ville Syrjälä Intel
Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation
On 2023-03-09 12:04, Hogander, Jouni wrote: On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote: On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote: Hey, On 2023-03-06 16:23, Souza, Jose wrote: On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote: As a fallback if we decide not to merge the frontbuffer tracking, allow i915 to keep its own implementation, and do the right thing in Xe. The frontbuffer tracking for Xe is still done per-fb, while i915 can keep doing the weird intel_frontbuffer + i915_active thing without blocking Xe. Please also disable PSR and FBC with this or at least add a way for users to disable those features. Without frontbuffer tracker those two features will break in some cases. FBC and PSR work completely as expected. I don't remove frontbuffer tracking; I only remove the GEM parts. Explicit invalidation using pageflip or CPU rendering + DirtyFB continue to work, as I validated on my laptop with FBC. Neither of which are relevant to the removal of the gem hooks. Like I already said ~10 times in the last meeting, we need a proper testcase. Here's a rough idea what it should do: prepare a batch with 1. spinner 2. something that clobbers the fb Then 1. grab reference crc 2. execbuffer 3. dirtyfb 4. wait long enough for fbc to recompress 5. terminate spinner 6. gem_sync 7. grab crc and compare with reference No idea what the current status of PSR+CRC is, so not sure whether we can actually test PSR or not. CRC calculation doesn't work with PSR currently. PSR is disabled if CRC capture is requested. Are we supposed to support frontbuffer rendering using GPU? No other driver does that. It's fine if DirtyFB hangs instead until the job it waits on completes. ~Maarten
Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation
On Mon, 2023-03-06 at 22:58 +0200, Ville Syrjälä wrote: > On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote: > > Hey, > > > > On 2023-03-06 16:23, Souza, Jose wrote: > > > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote: > > > > As a fallback if we decide not to merge the frontbuffer > > > > tracking, allow > > > > i915 to keep its own implementation, and do the right thing in > > > > Xe. > > > > > > > > The frontbuffer tracking for Xe is still done per-fb, while > > > > i915 can > > > > keep doing the weird intel_frontbuffer + i915_active thing > > > > without > > > > blocking Xe. > > > Please also disable PSR and FBC with this or at least add a way > > > for users to disable those features. > > > Without frontbuffer tracker those two features will break in some > > > cases. > > > > FBC and PSR work completely as expected. I don't remove frontbuffer > > tracking; I only remove the GEM parts. > > > > Explicit invalidation using pageflip or CPU rendering + DirtyFB > > continue > > to work, as I validated on my laptop with FBC. > > Neither of which are relevant to the removal of the gem hooks. > > Like I already said ~10 times in the last meeting, we need a proper > testcase. Here's a rough idea what it should do: > > prepare a batch with > 1. spinner > 2. something that clobbers the fb > > Then > 1. grab reference crc > 2. execbuffer > 3. dirtyfb > 4. wait long enough for fbc to recompress > 5. terminate spinner > 6. gem_sync > 7. grab crc and compare with reference > > No idea what the current status of PSR+CRC is, so not sure > whether we can actually test PSR or not. > CRC calculation doesn't work with PSR currently. PSR is disabled if CRC capture is requested. Are we supposed to support frontbuffer rendering using GPU? BR, Jouni Högander
Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation
On Wed, Mar 08, 2023 at 03:29:45PM +0100, Maarten Lankhorst wrote: > Hey, > > > On 2023-03-08 14:36, Ville Syrjälä wrote: > > On Wed, Mar 08, 2023 at 01:47:12PM +0100, Maarten Lankhorst wrote: > >> On 2023-03-06 21:58, Ville Syrjälä wrote: > >>> On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote: > Hey, > > On 2023-03-06 16:23, Souza, Jose wrote: > > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote: > >> As a fallback if we decide not to merge the frontbuffer tracking, allow > >> i915 to keep its own implementation, and do the right thing in Xe. > >> > >> The frontbuffer tracking for Xe is still done per-fb, while i915 can > >> keep doing the weird intel_frontbuffer + i915_active thing without > >> blocking Xe. > > Please also disable PSR and FBC with this or at least add a way for > > users to disable those features. > > Without frontbuffer tracker those two features will break in some cases. > FBC and PSR work completely as expected. I don't remove frontbuffer > tracking; I only remove the GEM parts. > > Explicit invalidation using pageflip or CPU rendering + DirtyFB continue > to work, as I validated on my laptop with FBC. > >>> Neither of which are relevant to the removal of the gem hooks. > >>> > >>> Like I already said ~10 times in the last meeting, we need a proper > >>> testcase. Here's a rough idea what it should do: > >>> > >>> prepare a batch with > >>> 1. spinner > >>> 2. something that clobbers the fb > >>> > >>> Then > >>> 1. grab reference crc > >>> 2. execbuffer > >>> 3. dirtyfb > >>> 4. wait long enough for fbc to recompress > >>> 5. terminate spinner > >>> 6. gem_sync > >>> 7. grab crc and compare with reference > >>> > >>> No idea what the current status of PSR+CRC is, so not sure > >>> whether we can actually test PSR or not. > >> This test doesn't make sense. DirtyFB should simply not return before > >> execbuffer finishes. > > Of course it should. It's not a blocking ioctl, and can't > > be because that will make X unusable. > > Except it actually is. > > DirtyFB blocks in its default implementation, and waits for the next vblank. > > drm_atomic_helper_dirtyfb() blocks by default as it's a synchronous > plane update. > > Considering every driver except i915 uses it, it works just fine. :) No it doesn't. We tries to use it, but that was an utter failure. -- Ville Syrjälä Intel
Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation
Hey, On 2023-03-08 14:36, Ville Syrjälä wrote: On Wed, Mar 08, 2023 at 01:47:12PM +0100, Maarten Lankhorst wrote: On 2023-03-06 21:58, Ville Syrjälä wrote: On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote: Hey, On 2023-03-06 16:23, Souza, Jose wrote: On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote: As a fallback if we decide not to merge the frontbuffer tracking, allow i915 to keep its own implementation, and do the right thing in Xe. The frontbuffer tracking for Xe is still done per-fb, while i915 can keep doing the weird intel_frontbuffer + i915_active thing without blocking Xe. Please also disable PSR and FBC with this or at least add a way for users to disable those features. Without frontbuffer tracker those two features will break in some cases. FBC and PSR work completely as expected. I don't remove frontbuffer tracking; I only remove the GEM parts. Explicit invalidation using pageflip or CPU rendering + DirtyFB continue to work, as I validated on my laptop with FBC. Neither of which are relevant to the removal of the gem hooks. Like I already said ~10 times in the last meeting, we need a proper testcase. Here's a rough idea what it should do: prepare a batch with 1. spinner 2. something that clobbers the fb Then 1. grab reference crc 2. execbuffer 3. dirtyfb 4. wait long enough for fbc to recompress 5. terminate spinner 6. gem_sync 7. grab crc and compare with reference No idea what the current status of PSR+CRC is, so not sure whether we can actually test PSR or not. This test doesn't make sense. DirtyFB should simply not return before execbuffer finishes. Of course it should. It's not a blocking ioctl, and can't be because that will make X unusable. Except it actually is. DirtyFB blocks in its default implementation, and waits for the next vblank. drm_atomic_helper_dirtyfb() blocks by default as it's a synchronous plane update. Considering every driver except i915 uses it, it works just fine. :) ~Maarten
Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation
On Wed, Mar 08, 2023 at 01:47:12PM +0100, Maarten Lankhorst wrote: > > On 2023-03-06 21:58, Ville Syrjälä wrote: > > On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote: > >> Hey, > >> > >> On 2023-03-06 16:23, Souza, Jose wrote: > >>> On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote: > As a fallback if we decide not to merge the frontbuffer tracking, allow > i915 to keep its own implementation, and do the right thing in Xe. > > The frontbuffer tracking for Xe is still done per-fb, while i915 can > keep doing the weird intel_frontbuffer + i915_active thing without > blocking Xe. > >>> Please also disable PSR and FBC with this or at least add a way for users > >>> to disable those features. > >>> Without frontbuffer tracker those two features will break in some cases. > >> FBC and PSR work completely as expected. I don't remove frontbuffer > >> tracking; I only remove the GEM parts. > >> > >> Explicit invalidation using pageflip or CPU rendering + DirtyFB continue > >> to work, as I validated on my laptop with FBC. > > Neither of which are relevant to the removal of the gem hooks. > > > > Like I already said ~10 times in the last meeting, we need a proper > > testcase. Here's a rough idea what it should do: > > > > prepare a batch with > > 1. spinner > > 2. something that clobbers the fb > > > > Then > > 1. grab reference crc > > 2. execbuffer > > 3. dirtyfb > > 4. wait long enough for fbc to recompress > > 5. terminate spinner > > 6. gem_sync > > 7. grab crc and compare with reference > > > > No idea what the current status of PSR+CRC is, so not sure > > whether we can actually test PSR or not. > > This test doesn't make sense. DirtyFB should simply not return before > execbuffer finishes. Of course it should. It's not a blocking ioctl, and can't be because that will make X unusable. -- Ville Syrjälä Intel
Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation
On 2023-03-06 21:58, Ville Syrjälä wrote: On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote: Hey, On 2023-03-06 16:23, Souza, Jose wrote: On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote: As a fallback if we decide not to merge the frontbuffer tracking, allow i915 to keep its own implementation, and do the right thing in Xe. The frontbuffer tracking for Xe is still done per-fb, while i915 can keep doing the weird intel_frontbuffer + i915_active thing without blocking Xe. Please also disable PSR and FBC with this or at least add a way for users to disable those features. Without frontbuffer tracker those two features will break in some cases. FBC and PSR work completely as expected. I don't remove frontbuffer tracking; I only remove the GEM parts. Explicit invalidation using pageflip or CPU rendering + DirtyFB continue to work, as I validated on my laptop with FBC. Neither of which are relevant to the removal of the gem hooks. Like I already said ~10 times in the last meeting, we need a proper testcase. Here's a rough idea what it should do: prepare a batch with 1. spinner 2. something that clobbers the fb Then 1. grab reference crc 2. execbuffer 3. dirtyfb 4. wait long enough for fbc to recompress 5. terminate spinner 6. gem_sync 7. grab crc and compare with reference No idea what the current status of PSR+CRC is, so not sure whether we can actually test PSR or not. This test doesn't make sense. DirtyFB should simply not return before execbuffer finishes. ~Maarten
Re: [Intel-gfx] [Intel-xe] [PATCH] drm/xe/display: Do not use i915 frontbuffer tracking implementation
On Mon, Mar 06, 2023 at 09:23:50PM +0100, Maarten Lankhorst wrote: > Hey, > > On 2023-03-06 16:23, Souza, Jose wrote: > > On Mon, 2023-03-06 at 15:16 +0100, Maarten Lankhorst wrote: > >> As a fallback if we decide not to merge the frontbuffer tracking, allow > >> i915 to keep its own implementation, and do the right thing in Xe. > >> > >> The frontbuffer tracking for Xe is still done per-fb, while i915 can > >> keep doing the weird intel_frontbuffer + i915_active thing without > >> blocking Xe. > > Please also disable PSR and FBC with this or at least add a way for users > > to disable those features. > > Without frontbuffer tracker those two features will break in some cases. > > FBC and PSR work completely as expected. I don't remove frontbuffer > tracking; I only remove the GEM parts. > > Explicit invalidation using pageflip or CPU rendering + DirtyFB continue > to work, as I validated on my laptop with FBC. Neither of which are relevant to the removal of the gem hooks. Like I already said ~10 times in the last meeting, we need a proper testcase. Here's a rough idea what it should do: prepare a batch with 1. spinner 2. something that clobbers the fb Then 1. grab reference crc 2. execbuffer 3. dirtyfb 4. wait long enough for fbc to recompress 5. terminate spinner 6. gem_sync 7. grab crc and compare with reference No idea what the current status of PSR+CRC is, so not sure whether we can actually test PSR or not. -- Ville Syrjälä Intel
