On Sun, 07 Sep 2025, Juha-Pekka Heikkilä <juhapekka.heikk...@gmail.com> wrote:
> On Thu, Sep 4, 2025 at 7:56 PM Matt Atwood <matthew.s.atw...@intel.com> wrote:
>>
>> On Thu, Sep 04, 2025 at 01:53:01PM -0300, Gustavo Sousa wrote:
>> > Quoting Matt Atwood (2025-09-03 14:08:21-03:00)
>> > >The checks in plane_has_modifier() should check against display version
>> > >instead of graphics version.
>> > >
>> > >Bspec: 67165, 70815
>> > >
>> > >Signed-off-by: Matt Atwood <matthew.s.atw...@intel.com>
>> > >---
>> > > drivers/gpu/drm/i915/display/intel_fb.c | 4 ++--
>> > > 1 file changed, 2 insertions(+), 2 deletions(-)
>> > >
>> > >diff --git a/drivers/gpu/drm/i915/display/intel_fb.c
>> > >b/drivers/gpu/drm/i915/display/intel_fb.c
>> > >index b210c3250501..1e4cee857d7b 100644
>> > >--- a/drivers/gpu/drm/i915/display/intel_fb.c
>> > >+++ b/drivers/gpu/drm/i915/display/intel_fb.c
>> > >@@ -563,11 +563,11 @@ static bool plane_has_modifier(struct intel_display
>> > >*display,
>> > > return false;
>> > >
>> > > if (md->modifier == I915_FORMAT_MOD_4_TILED_BMG_CCS &&
>> > >- (GRAPHICS_VER(i915) < 20 || !display->platform.dgfx))
>> > >+ (DISPLAY_VER(display) < 14 || !display->platform.dgfx))
>> > > return false;
>> > >
>> > > if (md->modifier == I915_FORMAT_MOD_4_TILED_LNL_CCS &&
>> > >- (GRAPHICS_VER(i915) < 20 || display->platform.dgfx))
>> > >+ (DISPLAY_VER(display) < 20 || display->platform.dgfx))
>> > > return false;
>> >
>> > Hm... Maybe using GRAPHICS_VER() here was intentional? The checks on
>> > display version are already covered by the entries in intel_modifiers.
>> >
>> > If we look at commit fca0abb23447 ("drm/i915/display: allow creation of
>> > Xe2 ccs framebuffers"), we'll see that the respective entries were added
>> > to intel_modifiers *and* the checks on GRAPHICS_VER() were added as
>> > well. Perhaps there are indeed restrictions on the graphics side to be
>> > able to use the format?
>> >
>> > --
>> > Gustavo Sousa
>> Honestly, I tried looking for reasons. I couldn't find anything in the
>> documentation to support. I decided to send upstream to see if it broke
>> stuff to not do the checks that way. CI seems very clean. Hoping Jani or
>> Juha-Pekka will chime in if it is indeed an issue.
>
> Using GRAPHICS_VER here was intentional. Jani didn't like it but these
> xe2 ccs don't follow display versioning but gt versioning.
>
> Proposed change look ok but I'll need to dig in to documentation
> before I can say for sure. I remember we had discussion about this
> with Jani but can't remember what convinced Jani I needed to use
> GRAPHICS_VER at that time.
I think I just took your word for it.
In the long run, we can't have GRAPHICS_VER() checks in display
code. Either this needs to be converted to DISPLAY_VER(), or, if that's
not right, we need to add a way to ask for the *feature* support from
i915/xe core. That means higher level semantics than just checking for
graphics version.
BR,
Jani.
>
> /Juha-Pekka
>
>> >
>> > >
>> > > return true;
>> > >--
>> > >2.50.1
>> > >
--
Jani Nikula, Intel
