[Intel-gfx] [PATCH] drm/i915: Support FP16 compressed formats on MTL

[Lobo, Melanie]

> -----Original Message-----
> From: Roper, Matthew D <matthew.d.roper at intel.com>
> Sent: Saturday, December 2, 2023 5:38 AM
> To: Lobo, Melanie <melanie.lobo at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Heikkila, Juha-pekka <juha-
> pekka.heikkila at 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 at intel.com
> >
> > Credits: Juha-Pekka <juha-pekka.heikkila at intel.com>
> > Cc: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> > Cc: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> > Cc: Swati Sharma <swati2.sharma at intel.com>
> > Reported-by: kernel test robot <lkp at intel.com>
> > Reported-by: Dan Carpenter <dan.carpenter at 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 at 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