From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Ekstrand Subject: Re: [PATCH 1/6] drm/i915: Implement .get_format_info() hook for CCS Date: Wed, 2 Aug 2017 14:28:00 -0700 Message-ID: References: <20170801165817.7063-1-ben@bwidawsk.net> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1625249241==" Return-path: In-Reply-To: <20170801165817.7063-1-ben@bwidawsk.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Ben Widawsky Cc: Intel GFX , DRI Development List-Id: dri-devel@lists.freedesktop.org --===============1625249241== Content-Type: multipart/alternative; boundary="001a1141824c42f4900555cbf063" --001a1141824c42f4900555cbf063 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Drp... replied to an old patch. The userspace mesa patches to produce Y_TILED_CCS surfaces have been reviewed for a couple of weeks now. So long as kmscube counts as userspace, I think this should be good to land. Acked-by: Jason Ekstrand On Tue, Aug 1, 2017 at 9:58 AM, Ben Widawsky wrote: > From: Ville Syrj=C3=A4l=C3=A4 > > SKL+ display engine can scan out certain kinds of compressed surfaces > produced by the render engine. This involved telling the display engine > the location of the color control surfae (CCS) which describes which > parts of the main surface are compressed and which are not. The location > of CCS is provided by userspace as just another plane with its own offset= . > > By providing our own format information for the CCS formats, we should > be able to make framebuffer_check() do the right thing for the CCS > surface as well. > > Note that we'll return the same format info for both Y and Yf tiled > format as that's what happens with the non-CCS Y vs. Yf as well. If > desired, we could potentially return a unique pointer for each > pixel_format+tiling+ccs combination, in which case we immediately be > able to tell if any of that stuff changed by just comparing the > pointers. But that does sound a bit wasteful space wise. > > v2: Drop the 'dev' argument from the hook > v3: Include the description of the CCS surface layout > v4: Pretend CCS tiles are regular 128 byte wide Y tiles (Jason) > > Cc: Daniel Vetter > Cc: Ben Widawsky > Cc: Jason Ekstrand > Reviewed-by: Ben Widawsky (v3) > Signed-off-by: Ville Syrj=C3=A4 > Signed-off-by: Ben Widawsky > --- > drivers/gpu/drm/drm_fourcc.c | 2 +- > drivers/gpu/drm/i915/intel_display.c | 37 ++++++++++++++++++++++++++++++ > ++++++ > include/drm/drm_mode_config.h | 3 ++- > include/uapi/drm/drm_fourcc.h | 3 +++ > 4 files changed, 43 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > index 9c0152df45ad..50da6180c495 100644 > --- a/drivers/gpu/drm/drm_fourcc.c > +++ b/drivers/gpu/drm/drm_fourcc.c > @@ -222,7 +222,7 @@ drm_get_format_info(struct drm_device *dev, > const struct drm_format_info *info =3D NULL; > > if (dev->mode_config.funcs->get_format_info) > - info =3D dev->mode_config.funcs->get_format_info(mode_cmd= ); > + info =3D dev->mode_config.funcs->get_format_info(dev, > mode_cmd); > > if (!info) > info =3D drm_format_info(mode_cmd->pixel_format); > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index e92fd14c06c7..6b00689ef6e0 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2433,6 +2433,42 @@ static unsigned int intel_fb_modifier_to_tiling(ui= nt64_t > fb_modifier) > } > } > > +static const struct drm_format_info ccs_formats[] =3D { > + { .format =3D DRM_FORMAT_XRGB8888, .depth =3D 24, .num_planes =3D= 2, > .cpp =3D { 4, 1, }, .hsub =3D 16, .vsub =3D 8, }, > + { .format =3D DRM_FORMAT_XBGR8888, .depth =3D 24, .num_planes =3D= 2, > .cpp =3D { 4, 1, }, .hsub =3D 16, .vsub =3D 8, }, > + { .format =3D DRM_FORMAT_ARGB8888, .depth =3D 32, .num_planes =3D= 2, > .cpp =3D { 4, 1, }, .hsub =3D 16, .vsub =3D 8, }, > + { .format =3D DRM_FORMAT_ABGR8888, .depth =3D 32, .num_planes =3D= 2, > .cpp =3D { 4, 1, }, .hsub =3D 16, .vsub =3D 8, }, > +}; > + > +static const struct drm_format_info * > +lookup_format_info(const struct drm_format_info formats[], > + int num_formats, u32 format) > +{ > + int i; > + > + for (i =3D 0; i < num_formats; i++) { > + if (formats[i].format =3D=3D format) > + return &formats[i]; > + } > + > + return NULL; > +} > + > +static const struct drm_format_info * > +intel_get_format_info(struct drm_device *dev, > + const struct drm_mode_fb_cmd2 *cmd) > +{ > + switch (cmd->modifier[0]) { > + case I915_FORMAT_MOD_Y_TILED_CCS: > + case I915_FORMAT_MOD_Yf_TILED_CCS: > + return lookup_format_info(ccs_formats, > + ARRAY_SIZE(ccs_formats), > + cmd->pixel_format); > + default: > + return NULL; > + } > +} > + > static int > intel_fill_fb_info(struct drm_i915_private *dev_priv, > struct drm_framebuffer *fb) > @@ -14630,6 +14666,7 @@ static void intel_atomic_state_free(struct > drm_atomic_state *state) > > static const struct drm_mode_config_funcs intel_mode_funcs =3D { > .fb_create =3D intel_user_framebuffer_create, > + .get_format_info =3D intel_get_format_info, > .output_poll_changed =3D intel_fbdev_output_poll_changed, > .atomic_check =3D intel_atomic_check, > .atomic_commit =3D intel_atomic_commit, > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.= h > index 42981711189b..f0d3d3857ae2 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -81,7 +81,8 @@ struct drm_mode_config_funcs { > * The format information specific to the given fb metadata, or > * NULL if none is found. > */ > - const struct drm_format_info *(*get_format_info)(const struct > drm_mode_fb_cmd2 *mode_cmd); > + const struct drm_format_info *(*get_format_info)(struct drm_devic= e > *dev, > + const struct drm_mode_fb_cmd2 *mode_cmd); > > /** > * @output_poll_changed: > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.= h > index 7586c46f68bf..ee5910944fac 100644 > --- a/include/uapi/drm/drm_fourcc.h > +++ b/include/uapi/drm/drm_fourcc.h > @@ -252,6 +252,9 @@ extern "C" { > */ > #define I915_FORMAT_MOD_Yf_TILED fourcc_mod_code(INTEL, 3) > > +#define I915_FORMAT_MOD_Y_TILED_CCS fourcc_mod_code(INTEL, 4) > +#define I915_FORMAT_MOD_Yf_TILED_CCS fourcc_mod_code(INTEL, 5) > + > /* > * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblocks > * > -- > 2.13.3 > > --001a1141824c42f4900555cbf063 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Drp... replied to an old patch.

The userspace mesa = patches to produce Y_TILED_CCS surfaces have been reviewed for a couple of = weeks now.=C2=A0 So long as kmscube counts as userspace, I think this shoul= d be good to land.

Acked-by: Jason Ekstrand <jason@jlekstrand.net>

On Tue, Aug 1, 2017 at 9:58 AM, Be= n Widawsky <ben@bwidawsk.net> wrote:
From: Ville Syrj=C3=A4l=C3=A4 <ville.syrjala@linux.intel.com>

SKL+ display engine can scan out certain kinds of compressed surfaces
produced by the render engine. This involved telling the display engine
the location of the color control surfae (CCS) which describes which
parts of the main surface are compressed and which are not. The location of CCS is provided by userspace as just another plane with its own offset.<= br>
By providing our own format information for the CCS formats, we should
be able to make framebuffer_check() do the right thing for the CCS
surface as well.

Note that we'll return the same format info for both Y and Yf tiled
format as that's what happens with the non-CCS Y vs. Yf as well. If
desired, we could potentially return a unique pointer for each
pixel_format+tiling+ccs combination, in which case we immediately be
able to tell if any of that stuff changed by just comparing the
pointers. But that does sound a bit wasteful space wise.

v2: Drop the 'dev' argument from the hook
=C2=A0 =C2=A0 =C2=A0 =C2=A0 v3: Include the description of the CCS surface = layout
v4: Pretend CCS tiles are regular 128 byte wide Y tiles (Jason)

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Ben Widawsky <ben@bwidawsk.net>
Cc: Jason Ekstrand <jason@jlekst= rand.net>
Reviewed-by: Ben Widawsky <ben@bwida= wsk.net> (v3)
Signed-off-by: Ville Syrj=C3=A4 <ville.syrjala@linux.intel.com>
Signed-off-by: Ben Widawsky <ben@bwi= dawsk.net>
---
=C2=A0drivers/gpu/drm/drm_fourcc.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0= 2 +-
=C2=A0drivers/gpu/drm/i915/intel_display.c | 37 ++++++++++++++++++++++= ++++++++++++++
=C2=A0include/drm/drm_mode_config.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 3 ++= -
=C2=A0include/uapi/drm/drm_fourcc.h=C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 3 ++= +
=C2=A04 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c index 9c0152df45ad..50da6180c495 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -222,7 +222,7 @@ drm_get_format_info(struct drm_device *dev,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 const struct drm_format_info *info =3D NULL;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (dev->mode_config.funcs->get_form= at_info)
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0info =3D dev->mo= de_config.funcs->get_format_info(mode_cmd);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0info =3D dev->mo= de_config.funcs->get_format_info(dev, mode_cmd);

=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!info)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 info =3D drm_format= _info(mode_cmd->pixel_format);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i9= 15/intel_display.c
index e92fd14c06c7..6b00689ef6e0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2433,6 +2433,42 @@ static unsigned int intel_fb_modifier_to_tiling(uint64_t fb_modifier)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0}

+static const struct drm_format_info ccs_formats[] =3D {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0{ .format =3D DRM_FORMAT_XRGB8888, .depth =3D 2= 4, .num_planes =3D 2, .cpp =3D { 4, 1, }, .hsub =3D 16, .vsub =3D 8, },
+=C2=A0 =C2=A0 =C2=A0 =C2=A0{ .format =3D DRM_FORMAT_XBGR8888, .depth =3D 2= 4, .num_planes =3D 2, .cpp =3D { 4, 1, }, .hsub =3D 16, .vsub =3D 8, },
+=C2=A0 =C2=A0 =C2=A0 =C2=A0{ .format =3D DRM_FORMAT_ARGB8888, .depth =3D 3= 2, .num_planes =3D 2, .cpp =3D { 4, 1, }, .hsub =3D 16, .vsub =3D 8, },
+=C2=A0 =C2=A0 =C2=A0 =C2=A0{ .format =3D DRM_FORMAT_ABGR8888, .depth =3D 3= 2, .num_planes =3D 2, .cpp =3D { 4, 1, }, .hsub =3D 16, .vsub =3D 8, },
+};
+
+static const struct drm_format_info *
+lookup_format_info(const struct drm_format_info formats[],
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 int num_for= mats, u32 format)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0int i;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0for (i =3D 0; i < num_formats; i++) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (formats[i].form= at =3D=3D format)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0return &formats[i];
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return NULL;
+}
+
+static const struct drm_format_info *
+intel_get_format_info(struct drm_device *dev,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0const struct drm_mode_fb_cmd2 *cmd)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0switch (cmd->modifier[0]) {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0case I915_FORMAT_MOD_Y_TILED_CCS:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0case I915_FORMAT_MOD_Yf_TILED_CCS:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return lookup_forma= t_info(ccs_formats,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0AR= RAY_SIZE(ccs_formats),
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0cm= d->pixel_format);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0default:
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return NULL;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+}
+
=C2=A0static int
=C2=A0intel_fill_fb_info(struct drm_i915_private *dev_priv,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct= drm_framebuffer *fb)
@@ -14630,6 +14666,7 @@ static void intel_atomic_state_free(struct drm_atom= ic_state *state)

=C2=A0static const struct drm_mode_config_funcs intel_mode_funcs =3D {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 .fb_create =3D intel_user_framebuffer_create, +=C2=A0 =C2=A0 =C2=A0 =C2=A0.get_format_info =3D intel_get_format_info,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 .output_poll_changed =3D intel_fbdev_output_pol= l_changed,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 .atomic_check =3D intel_atomic_check,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 .atomic_commit =3D intel_atomic_commit,
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_conf= ig.h
index 42981711189b..f0d3d3857ae2 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -81,7 +81,8 @@ struct drm_mode_config_funcs {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* The format information specific to the = given fb metadata, or
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* NULL if none is found.
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
-=C2=A0 =C2=A0 =C2=A0 =C2=A0const struct drm_format_info *(*get_format_info= )(const struct drm_mode_fb_cmd2 *mode_cmd);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0const struct drm_format_info *(*get_format_info= )(struct drm_device *dev,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0const struct drm_mo= de_fb_cmd2 *mode_cmd);

=C2=A0 =C2=A0 =C2=A0 =C2=A0 /**
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* @output_poll_changed:
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_four= cc.h
index 7586c46f68bf..ee5910944fac 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -252,6 +252,9 @@ extern "C" {
=C2=A0 */
=C2=A0#define I915_FORMAT_MOD_Yf_TILED fourcc_mod_code(INTEL, 3)

+#define I915_FORMAT_MOD_Y_TILED_CCS=C2=A0 =C2=A0 fourcc_mod_code(INTEL, 4)=
+#define I915_FORMAT_MOD_Yf_TILED_CCS=C2=A0 =C2=A0fourcc_mod_code(INTEL, 5)=
+
=C2=A0/*
=C2=A0 * Tiled, NV12MT, grouped in 64 (pixels) x 32 (lines) -sized macroblo= cks
=C2=A0 *
--
2.13.3


--001a1141824c42f4900555cbf063-- --===============1625249241== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============1625249241==--