[Intel-gfx] [PATCH v2 7/8] drm/i915/display/skl+: Drop frontbuffer rendering support
Gwan-gyeong Mun
gwan-gyeong.mun at intel.com
Mon Sep 6 09:44:39 UTC 2021
Looks good to me.
Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
On 9/4/21 3:26 AM, Souza, Jose wrote:
> On Fri, 2021-09-03 at 22:09 +0000, Souza, Jose wrote:
>> On Thu, 2021-09-02 at 21:42 +0300, Gwan-gyeong Mun wrote:
>>>
>>> On 8/25/21 3:58 AM, José Roberto de Souza wrote:
>>>> By now all the userspace applications should have migrated to atomic
>>>> or at least be calling DRM_IOCTL_MODE_DIRTYFB.
>>>>
>>>> With that we can kill frontbuffer rendering support in i915 for
>>>> modern platforms.
>>>>
>>>> So here converting legacy APIs into atomic commits so it can be
>>>> properly handled by driver i915.
>>>>
>>>> Several IGT tests will fail with this changes, because some tests
>>>> were stressing those frontbuffer rendering scenarios that no userspace
>>>> should be using by now, fixes to IGT should be sent soon.
>>>>
>>>> v2:
>>>> - return earlier to not set fb_tracking.busy/flip_bits
>>>> - added a warn on to make sure we are not setting the busy/flip_bits
>>>>
>>>> Cc: Daniel Vetter <daniel at ffwll.ch>
>>>> Cc: Gwan-gyeong Mun <gwan-gyeong.mun at intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>> Cc: Jani Nikula <jani.nikula at intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>>> Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
>>>> ---
>>>> drivers/gpu/drm/i915/display/intel_cursor.c | 6 ++----
>>>> drivers/gpu/drm/i915/display/intel_fb.c | 8 +++++++-
>>>> .../gpu/drm/i915/display/intel_frontbuffer.c | 18 ++++++++++++++++++
>>>> drivers/gpu/drm/i915/i915_drv.h | 2 ++
>>>> 4 files changed, 29 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
>>>> index c7618fef01439..5aa996c3b7980 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
>>>> @@ -617,6 +617,7 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>>>> u32 src_w, u32 src_h,
>>>> struct drm_modeset_acquire_ctx *ctx)
>>>> {
>>>> +struct drm_i915_private *i915 = to_i915(_crtc->dev);
>>>> struct intel_plane *plane = to_intel_plane(_plane);
>>>> struct intel_crtc *crtc = to_intel_crtc(_crtc);
>>>> struct intel_plane_state *old_plane_state =
>>>> @@ -633,12 +634,9 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>>>> * PSR2 selective fetch also requires the slow path as
>>>> * PSR2 plane and transcoder registers can only be updated during
>>>> * vblank.
>>>> - *
>>>> - * FIXME bigjoiner fastpath would be good
>>>> */
>>>> if (!crtc_state->hw.active || intel_crtc_needs_modeset(crtc_state) ||
>>>> - crtc_state->update_pipe || crtc_state->bigjoiner ||
>>>> - crtc_state->enable_psr2_sel_fetch)
>>>> + crtc_state->update_pipe || !HAS_FRONTBUFFER_RENDERING(i915))
>>>> goto slow;
>>>>
>>>> /*
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
>>>> index e4b8602ec0cd2..3eb60785c9f29 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_fb.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
>>>> @@ -3,6 +3,7 @@
>>>> * Copyright © 2021 Intel Corporation
>>>> */
>>>>
>>>> +#include <drm/drm_damage_helper.h>
>>>> #include <drm/drm_framebuffer.h>
>>>> #include <drm/drm_modeset_helper.h>
>>>>
>>>> @@ -1235,10 +1236,15 @@ static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
>>>> unsigned int num_clips)
>>>> {
>>>> struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>>> +struct drm_i915_private *i915 = to_i915(obj->base.dev);
>>>>
>>>> i915_gem_object_flush_if_display(obj);
>>>> -intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
>>>>
>>>> +if (!HAS_FRONTBUFFER_RENDERING(i915))
>>>> +return drm_atomic_helper_dirtyfb(fb, file, flags, color, clips,
>>>> + num_clips);
>>> Hi,
>>> Even if the userspace informs us of the dirty (damage) region of the
>>> front buffer being used, GEN9 to GEN12 still uses the HW Tracking
>>> function for PSR and FBC.
>>> And if you look at the description of the intel_psr_flush() function,
>>> you can see that there are the following restrictions.
>>>
>>> "Since the hardware frontbuffer tracking has gaps we need to integrate
>>> with the software frontbuffer tracking."
>>>
>>> If this restriction is still valid from GEN9 to GEN12, even if the
>>> existing frontbuffer tracking function is not used, when
>>> intel_user_framebuffer_dirty() is called, in the case of PSR,
>>> psr_force_hw_tracking_exit() is called or intel_psr_exit() and
>>> schedule_work(psr.work) seems to be required.
>>
>> As this will trigger calls to the functions that write the plane registers PSR HW tracking and FBC tracking will understand a page flip happened even
>> if going from and to the same surface.
>>
>> But will double check it.
>
> Yep, no issues with PSR or FBC. HW understand as a flip and it is properly handled.
>
>>
>>>
>>> In the case of FBC, it seems that calls to FBC deactive / FBC activate
>>> should be added.
>>>
>>> If GEN9 to GEN12 do not have the above restrictions, please ignore this
>>> comment.
>>>
>>> G.G.
>>>> +
>>>> +intel_frontbuffer_flush(to_intel_frontbuffer(fb), ORIGIN_DIRTYFB);
>>>> return 0;
>>>> }
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
>>>> index 0492446cd04ad..3860f87dac31c 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
>>>> @@ -112,6 +112,9 @@ static void frontbuffer_flush(struct drm_i915_private *i915,
>>>> void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915,
>>>> unsigned frontbuffer_bits)
>>>> {
>>>> +if (!HAS_FRONTBUFFER_RENDERING(i915))
>>>> +return;
>>>> +
>>>> spin_lock(&i915->fb_tracking.lock);
>>>> i915->fb_tracking.flip_bits |= frontbuffer_bits;
>>>> /* Remove stale busy bits due to the old buffer. */
>>>> @@ -132,6 +135,12 @@ void intel_frontbuffer_flip_prepare(struct drm_i915_private *i915,
>>>> void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
>>>> unsigned frontbuffer_bits)
>>>> {
>>>> +if (!HAS_FRONTBUFFER_RENDERING(i915)) {
>>>> +drm_WARN_ON_ONCE(&i915->drm, i915->fb_tracking.flip_bits |
>>>> + i915->fb_tracking.busy_bits);
>>>> +return;
>>>> +}
>>>> +
>>>> spin_lock(&i915->fb_tracking.lock);
>>>> /* Mask any cancelled flips. */
>>>> frontbuffer_bits &= i915->fb_tracking.flip_bits;
>>>> @@ -156,6 +165,9 @@ void intel_frontbuffer_flip_complete(struct drm_i915_private *i915,
>>>> void intel_frontbuffer_flip(struct drm_i915_private *i915,
>>>> unsigned frontbuffer_bits)
>>>> {
>>>> +if (!HAS_FRONTBUFFER_RENDERING(i915))
>>>> +return;
>>>> +
>>>> spin_lock(&i915->fb_tracking.lock);
>>>> /* Remove stale busy bits due to the old buffer. */
>>>> i915->fb_tracking.busy_bits &= ~frontbuffer_bits;
>>>> @@ -170,6 +182,9 @@ void __intel_fb_invalidate(struct intel_frontbuffer *front,
>>>> {
>>>> struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
>>>>
>>>> +if (!HAS_FRONTBUFFER_RENDERING(i915))
>>>> +return;
>>>> +
>>>> if (origin == ORIGIN_CS) {
>>>> spin_lock(&i915->fb_tracking.lock);
>>>> i915->fb_tracking.busy_bits |= frontbuffer_bits;
>>>> @@ -191,6 +206,9 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
>>>> {
>>>> struct drm_i915_private *i915 = to_i915(front->obj->base.dev);
>>>>
>>>> +if (!HAS_FRONTBUFFER_RENDERING(i915))
>>>> +return;
>>>> +
>>>> if (origin == ORIGIN_CS) {
>>>> spin_lock(&i915->fb_tracking.lock);
>>>> /* Filter out new bits since rendering started. */
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 91453f7dbd656..20c135a2bba33 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1721,6 +1721,8 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>>>>
>>>> #define HAS_ASYNC_FLIPS(i915)(DISPLAY_VER(i915) >= 5)
>>>>
>>>> +#define HAS_FRONTBUFFER_RENDERING(i915)(DISPLAY_VER(i915) < 9)
>>>> +
>>>> /* Only valid when HAS_DISPLAY() is true */
>>>> #define INTEL_DISPLAY_ENABLED(dev_priv) \
>>>> (drm_WARN_ON(&(dev_priv)->drm, !HAS_DISPLAY(dev_priv)), !(dev_priv)->params.disable_display)
>>>>
>>
>
More information about the Intel-gfx
mailing list