On Tue, 23 Jan 2024, Juha-Pekka Heikkila <juhapekka.heikkila@xxxxxxxxx> wrote:
> On 23.1.2024 12.49, Jani Nikula wrote:
>> On Tue, 02 Jan 2024, Juha-Pekka Heikkila <juhapekka.heikkila@xxxxxxxxx> wrote:
>>> Aux ccs framebuffers don't work on Xe driver hence disable them
>>> from plane capabilities until they are fixed. Flat ccs framebuffers
>>> work and they are left enabled. Here is separated plane capabilities
>>> check on i915 so it can behave differencly depending on the driver.
>>
>> I still think there's too much going on in this one patch. It refactors
>> i915 and modifies xe behaviour in one go.
>>
>> It adds intel_plane_caps.[ch] in i915, but extracts them from skl+
>> specific functions and files. xe uses the .h but adds the code in
>> existing xe_plane_initial.c. There's also intel_plane_initial.c i915
>> side, but that's not where the functions get moved in i915 side.
>
> I was never against splitting it, I can do that.
>
>>
>> I'm trying to look at the actual functional change, and I'm wondering if
>> it isn't just this:
>>
>> index 511dc1544854..8bba6c2e5098 100644
>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>> @@ -2290,6 +2290,9 @@ static u8 skl_get_plane_caps(struct drm_i915_private *i915,
>> if (HAS_4TILE(i915))
>> caps |= INTEL_PLANE_CAP_TILING_4;
>>
>> + if (!IS_ENABLED(I915) && !HAS_FLAT_CCS(i915))
>> + return caps;
>> +
>> if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>> caps |= INTEL_PLANE_CAP_CCS_RC;
>> if (DISPLAY_VER(i915) >= 12)
>>
>> I'm not saying that's exactly pretty, either, but IIUC this is supposed
>> to be a temporary measure ("until they are fixed"), I'd rather take this
>> small thing instead.
>>
>
> Would that work when both i915 and Xe are being built?
IS_ENABLED(I915) will be false for xe build, true for i915 build. And
HAS_FLAT_CCS() is defined for both, in different ways.
It's essentially the same as #ifdef but much less of an eyesore.
BR,
Jani.
>
>>
>>
>>
>>>
>>> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/933
>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@xxxxxxxxx>
>>> ---
>>> drivers/gpu/drm/i915/Makefile | 1 +
>>> .../gpu/drm/i915/display/intel_plane_caps.c | 68 +++++++++++++++++++
>>> .../gpu/drm/i915/display/intel_plane_caps.h | 14 ++++
>>> .../drm/i915/display/skl_universal_plane.c | 61 +----------------
>>> drivers/gpu/drm/xe/display/xe_plane_initial.c | 23 +++++++
>>> 5 files changed, 107 insertions(+), 60 deletions(-)
>>> create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.c
>>> create mode 100644 drivers/gpu/drm/i915/display/intel_plane_caps.h
>>>
>>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>>> index e777686190ca..c5e3c2dd0a01 100644
>>> --- a/drivers/gpu/drm/i915/Makefile
>>> +++ b/drivers/gpu/drm/i915/Makefile
>>> @@ -302,6 +302,7 @@ i915-y += \
>>> display/intel_overlay.o \
>>> display/intel_pch_display.o \
>>> display/intel_pch_refclk.o \
>>> + display/intel_plane_caps.o \
>>> display/intel_plane_initial.o \
>>> display/intel_pmdemand.o \
>>> display/intel_psr.o \
>>> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.c b/drivers/gpu/drm/i915/display/intel_plane_caps.c
>>> new file mode 100644
>>> index 000000000000..6206ae11f296
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.c
>>> @@ -0,0 +1,68 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2024 Intel Corporation
>>> + */
>>> +
>>> +#include "i915_drv.h"
>>> +#include "intel_fb.h"
>>> +#include "intel_plane_caps.h"
>>> +
>>> +static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
>>> + enum pipe pipe, enum plane_id plane_id)
>>> +{
>>> + /* Wa_22011186057 */
>>> + if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>>> + return false;
>>> +
>>> + if (DISPLAY_VER(i915) >= 11)
>>> + return true;
>>> +
>>> + if (IS_GEMINILAKE(i915))
>>> + return pipe != PIPE_C;
>>> +
>>> + return pipe != PIPE_C &&
>>> + (plane_id == PLANE_PRIMARY ||
>>> + plane_id == PLANE_SPRITE0);
>>> +}
>>> +
>>> +static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
>>> + enum plane_id plane_id)
>>> +{
>>> + if (DISPLAY_VER(i915) < 12)
>>> + return false;
>>> +
>>> + /* Wa_14010477008 */
>>> + if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
>>> + (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
>>> + return false;
>>> +
>>> + /* Wa_22011186057 */
>>> + if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>>> + return false;
>>> +
>>> + return plane_id < PLANE_SPRITE4;
>>> +}
>>> +
>>> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>>> + enum pipe pipe, enum plane_id plane_id)
>>> +{
>>> + u8 caps = INTEL_PLANE_CAP_TILING_X;
>>> +
>>> + if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>>> + caps |= INTEL_PLANE_CAP_TILING_Y;
>>> + if (DISPLAY_VER(i915) < 12)
>>> + caps |= INTEL_PLANE_CAP_TILING_Yf;
>>> + if (HAS_4TILE(i915))
>>> + caps |= INTEL_PLANE_CAP_TILING_4;
>>> +
>>> + if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>>> + caps |= INTEL_PLANE_CAP_CCS_RC;
>>> + if (DISPLAY_VER(i915) >= 12)
>>> + caps |= INTEL_PLANE_CAP_CCS_RC_CC;
>>> + }
>>> +
>>> + if (gen12_plane_has_mc_ccs(i915, plane_id))
>>> + caps |= INTEL_PLANE_CAP_CCS_MC;
>>> +
>>> + return caps;
>>> +}
>>> diff --git a/drivers/gpu/drm/i915/display/intel_plane_caps.h b/drivers/gpu/drm/i915/display/intel_plane_caps.h
>>> new file mode 100644
>>> index 000000000000..60a941c76f23
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/display/intel_plane_caps.h
>>> @@ -0,0 +1,14 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2024 Intel Corporation
>>> + */
>>> +
>>> +#ifndef __INTEL_PLANE_CAPS_H__
>>> +#define __INTEL_PLANE_CAPS_H__
>>> +
>>> +#include "intel_display_types.h"
>>> +
>>> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>>> + enum pipe pipe, enum plane_id plane_id);
>>> +
>>> +#endif /* __INTEL_PLANE_CAPS_H__ */
>>> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>>> index 511dc1544854..f2fd3833c61d 100644
>>> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
>>> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
>>> @@ -17,6 +17,7 @@
>>> #include "intel_fb.h"
>>> #include "intel_fbc.h"
>>> #include "intel_frontbuffer.h"
>>> +#include "intel_plane_caps.h"
>>> #include "intel_psr.h"
>>> #include "intel_psr_regs.h"
>>> #include "skl_scaler.h"
>>> @@ -2242,66 +2243,6 @@ skl_plane_disable_flip_done(struct intel_plane *plane)
>>> spin_unlock_irq(&i915->irq_lock);
>>> }
>>>
>>> -static bool skl_plane_has_rc_ccs(struct drm_i915_private *i915,
>>> - enum pipe pipe, enum plane_id plane_id)
>>> -{
>>> - /* Wa_22011186057 */
>>> - if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>>> - return false;
>>> -
>>> - if (DISPLAY_VER(i915) >= 11)
>>> - return true;
>>> -
>>> - if (IS_GEMINILAKE(i915))
>>> - return pipe != PIPE_C;
>>> -
>>> - return pipe != PIPE_C &&
>>> - (plane_id == PLANE_PRIMARY ||
>>> - plane_id == PLANE_SPRITE0);
>>> -}
>>> -
>>> -static bool gen12_plane_has_mc_ccs(struct drm_i915_private *i915,
>>> - enum plane_id plane_id)
>>> -{
>>> - if (DISPLAY_VER(i915) < 12)
>>> - return false;
>>> -
>>> - /* Wa_14010477008 */
>>> - if (IS_DG1(i915) || IS_ROCKETLAKE(i915) ||
>>> - (IS_TIGERLAKE(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_D0)))
>>> - return false;
>>> -
>>> - /* Wa_22011186057 */
>>> - if (IS_ALDERLAKE_P(i915) && IS_DISPLAY_STEP(i915, STEP_A0, STEP_B0))
>>> - return false;
>>> -
>>> - return plane_id < PLANE_SPRITE4;
>>> -}
>>> -
>>> -static u8 skl_get_plane_caps(struct drm_i915_private *i915,
>>> - enum pipe pipe, enum plane_id plane_id)
>>> -{
>>> - u8 caps = INTEL_PLANE_CAP_TILING_X;
>>> -
>>> - if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>>> - caps |= INTEL_PLANE_CAP_TILING_Y;
>>> - if (DISPLAY_VER(i915) < 12)
>>> - caps |= INTEL_PLANE_CAP_TILING_Yf;
>>> - if (HAS_4TILE(i915))
>>> - caps |= INTEL_PLANE_CAP_TILING_4;
>>> -
>>> - if (skl_plane_has_rc_ccs(i915, pipe, plane_id)) {
>>> - caps |= INTEL_PLANE_CAP_CCS_RC;
>>> - if (DISPLAY_VER(i915) >= 12)
>>> - caps |= INTEL_PLANE_CAP_CCS_RC_CC;
>>> - }
>>> -
>>> - if (gen12_plane_has_mc_ccs(i915, plane_id))
>>> - caps |= INTEL_PLANE_CAP_CCS_MC;
>>> -
>>> - return caps;
>>> -}
>>> -
>>> struct intel_plane *
>>> skl_universal_plane_create(struct drm_i915_private *dev_priv,
>>> enum pipe pipe, enum plane_id plane_id)
>>> diff --git a/drivers/gpu/drm/xe/display/xe_plane_initial.c b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>>> index ccf83c12b545..425c6e6744a6 100644
>>> --- a/drivers/gpu/drm/xe/display/xe_plane_initial.c
>>> +++ b/drivers/gpu/drm/xe/display/xe_plane_initial.c
>>> @@ -15,6 +15,7 @@
>>> #include "intel_fb.h"
>>> #include "intel_fb_pin.h"
>>> #include "intel_frontbuffer.h"
>>> +#include "intel_plane_caps.h"
>>> #include "intel_plane_initial.h"
>>>
>>> static bool
>>> @@ -289,3 +290,25 @@ void intel_crtc_initial_plane_config(struct intel_crtc *crtc)
>>>
>>> plane_config_fini(&plane_config);
>>> }
>>> +
>>> +u8 skl_get_plane_caps(struct drm_i915_private *i915,
>>> + enum pipe pipe, enum plane_id plane_id)
>>> +{
>>> + u8 caps = INTEL_PLANE_CAP_TILING_X;
>>> +
>>> + if (DISPLAY_VER(i915) < 13 || IS_ALDERLAKE_P(i915))
>>> + caps |= INTEL_PLANE_CAP_TILING_Y;
>>> + if (DISPLAY_VER(i915) < 12)
>>> + caps |= INTEL_PLANE_CAP_TILING_Yf;
>>> + if (HAS_4TILE(i915))
>>> + caps |= INTEL_PLANE_CAP_TILING_4;
>>> +
>>> + if (HAS_FLAT_CCS(i915)) {
>>> + caps |= INTEL_PLANE_CAP_CCS_RC | INTEL_PLANE_CAP_CCS_RC_CC;
>>> +
>>> + if (plane_id < PLANE_SPRITE4)
>>> + caps |= INTEL_PLANE_CAP_CCS_MC;
>>> + }
>>> +
>>> + return caps;
>>> +}
>>
>
--
Jani Nikula, Intel
