| Message ID | 20231201091133.23508-1-melanie.lobo@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Support FP16 compressed formats on MTL | expand |
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@intel.com > > Credits: Juha-Pekka <juha-pekka.heikkila@intel.com> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > Cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com> > Cc: Swati Sharma <swati2.sharma@intel.com> > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > Closes: https://lore.kernel.org/r/202310150454.S9QF86bl-lkp@intel.com/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@intel.com> > --- > 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 >
> -----Original Message----- > From: Roper, Matthew D <matthew.d.roper@intel.com> > Sent: Saturday, December 2, 2023 5:38 AM > To: Lobo, Melanie <melanie.lobo@intel.com> > Cc: intel-gfx@lists.freedesktop.org; Heikkila, Juha-pekka <juha- > pekka.heikkila@intel.com> > Subject: Re: [Intel-gfx] [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@intel.com > > > > Credits: Juha-Pekka <juha-pekka.heikkila@intel.com> > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> > > Cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com> > > Cc: Swati Sharma <swati2.sharma@intel.com> > > Reported-by: kernel test robot <lkp@intel.com> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > Closes: > > https://lore.kernel.org/r/202310150454.S9QF86bl-lkp@intel.com/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@intel.com> > > --- > > 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
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 }, }; /* @@ -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; + 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:
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. This kernel change was tested with IGT patch, https://patchwork.freedesktop.org/patch/562014/ Test-with: 20231011095520.10768-1-melanie.lobo@intel.com Credits: Juha-Pekka <juha-pekka.heikkila@intel.com> Cc: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> Cc: Bhanuprakash Modem <bhanuprakash.modem@intel.com> Cc: Swati Sharma <swati2.sharma@intel.com> Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Closes: https://lore.kernel.org/r/202310150454.S9QF86bl-lkp@intel.com/HEAD detached at 29b1181aa95a Signed-off-by: Melanie Lobo <melanie.lobo@intel.com> --- 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(-)