I tried to look but can't find any reason why here was needed GRAPHICS_VER(), probably was something that's not anymore. In any case as Jani wrote GRAPHICS_VER() need to be removed from display code and if something breaks from this I think it will come to my list of things to fix..
Reviewed-by: Juha-Pekka Heikkila <[email protected]> On Mon, Sep 8, 2025 at 1:29 PM Jani Nikula <[email protected]> wrote: > > On Sun, 07 Sep 2025, Juha-Pekka Heikkilä <[email protected]> wrote: > > On Thu, Sep 4, 2025 at 7:56 PM Matt Atwood <[email protected]> > > 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 <[email protected]> > >> > >--- > >> > > 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
