> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx> > Sent: Saturday, December 2, 2023 5:38 AM > To: Lobo, Melanie <melanie.lobo@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Heikkila, Juha-pekka <juha- > pekka.heikkila@xxxxxxxxx> > Subject: Re: [PATCH] drm/i915: Support FP16 compressed formats on > MTL > > On Fri, Dec 01, 2023 at 02:41:33PM +0530, Melanie Lobo wrote: > > Supports FP16 format which is a binary floating-point computer number > > format that occupies 16 bits in computer memory.Platform shall render > > compression in display engine to receive FP16 compressed formats. > > The explanation here is somewhat confusing and might also be missing a few > words. What you're actually adding support for is an RGB64 format where each > color component is a 16-bit floating point value, right? > > > > > This kernel change was tested with IGT patch, > > https://patchwork.freedesktop.org/patch/562014/ > > > > Test-with: 20231011095520.10768-1-melanie.lobo@xxxxxxxxx > > > > Credits: Juha-Pekka <juha-pekka.heikkila@xxxxxxxxx> > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@xxxxxxxxx> > > Cc: Bhanuprakash Modem <bhanuprakash.modem@xxxxxxxxx> > > Cc: Swati Sharma <swati2.sharma@xxxxxxxxx> > > Reported-by: kernel test robot <lkp@xxxxxxxxx> > > Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > > Closes: > > https://lore.kernel.org/r/202310150454.S9QF86bl-lkp@xxxxxxxxx/HEAD > > detached at 29b1181aa95a > > I think you misunderstood the robot's directions on your earlier patch. > It said that you should add these lines if you were fixing the mistakes "...in a > separate patch/commit (i.e. not just a new version of the same patch/commit)..." > Since you're just fixing this in the second version of the same patch you shouldn't > add these lines, otherwise it implies that the whole patch (i.e., the need to add > support for this new format) was something reported by the test robot. > > Instead you should pass the -v flat to git format-patch when creating a new > version of your patch (e.g., "-v2" for the second revision) and also include an > indication in the commit message about what has changed since the previous > versions. E.g., > > v2: > - Added foo > - Fixed bar > > Also, you seem to have some unintended text at the end of the Closes: > line that didn't belong there anyway. > > > Signed-off-by: Melanie Lobo <melanie.lobo@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/display/intel_fb.c | 10 ++++++++++ > > drivers/gpu/drm/i915/display/skl_universal_plane.c | 2 +- > > 2 files changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c > > b/drivers/gpu/drm/i915/display/intel_fb.c > > index e7678571b0d7..494baf20f76b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fb.c > > +++ b/drivers/gpu/drm/i915/display/intel_fb.c > > @@ -91,6 +91,8 @@ static const struct drm_format_info gen12_ccs_formats[] > = { > > { .format = DRM_FORMAT_P016, .num_planes = 4, > > .char_per_block = { 2, 4, 1, 1 }, .block_w = { 1, 1, 2, 2 }, .block_h = { 1, 1, > 1, 1 }, > > .hsub = 2, .vsub = 2, .is_yuv = true }, > > + { .format = DRM_FORMAT_XRGB16161616F, .depth = 64, .num_planes = > 2, > > + .char_per_block = { 8, 1}, .block_w = { 1, 4}, .block_h = { 1, 2}, > > +.hsub = 1, .vsub = 1 }, > > Should there be an ARGB form of this supported as well? > > > }; > > > > /* > > @@ -394,6 +396,9 @@ static bool plane_has_modifier(struct > drm_i915_private *i915, > > u8 plane_caps, > > const struct intel_modifier_desc *md) { > > + const struct drm_format_info *info = > > + lookup_format_info(md->formats, md->format_count, > > +DRM_FORMAT_XRGB16161616F); > > + > > if (!IS_DISPLAY_VER(i915, md->display_ver.from, md->display_ver.until)) > > return false; > > > > @@ -408,6 +413,11 @@ static bool plane_has_modifier(struct > drm_i915_private *i915, > > HAS_FLAT_CCS(i915) != !md->ccs.packed_aux_planes) > > return false; > > > > + /* Check if CSS modifier and FP16 format is supported on different > platforms */ > > + if (intel_fb_is_ccs_modifier(md->modifier) && info && > > + info->format == DRM_FORMAT_XRGB16161616F && > DISPLAY_VER(i915) < 14) > > + return false; > > When I look at bspec 49250 and 49251, it appears that this format can be > supported with compression on display versions 12 and 13 as well. Is there some > other reason (e.g., a hardware workaround) that requires us to restrict this to > version 14 and higher? > > > Matt > > > + > > return true; > > } > > > > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > index 245a64332cc7..f5e9be57a74b 100644 > > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > > @@ -2091,6 +2091,7 @@ static bool > gen12_plane_format_mod_supported(struct drm_plane *_plane, > > case DRM_FORMAT_XBGR8888: > > case DRM_FORMAT_ARGB8888: > > case DRM_FORMAT_ABGR8888: > > + case DRM_FORMAT_XRGB16161616F: > > if (intel_fb_is_ccs_modifier(modifier)) > > return true; > > fallthrough; > > @@ -2115,7 +2116,6 @@ static bool > gen12_plane_format_mod_supported(struct drm_plane *_plane, > > case DRM_FORMAT_C8: > > case DRM_FORMAT_XBGR16161616F: > > case DRM_FORMAT_ABGR16161616F: > > - case DRM_FORMAT_XRGB16161616F: > > case DRM_FORMAT_ARGB16161616F: > > case DRM_FORMAT_Y210: > > case DRM_FORMAT_Y212: > > -- > > 2.17.1 > > > > -- > Matt Roper > Graphics Software Engineer > Linux GPU Platform Enablement > Intel Corporation Thank you Matt. I have addressed your comments in the next revision. Ref: https://patchwork.freedesktop.org/patch/596756/?series=124957&rev=6 Regards, Melanie Lobo
