On Mon, Sep 08, 2025 at 04:26:12PM +0300, Juha-Pekka Heikkilä wrote: > 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]>
I'm not entirely sure how much the format modifier code is tested in CI. However the CI test failures, the few that there are, have been accounted for. Thanks for the review. MattA > > 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
