On Mon, Nov 21, 2016 at 08:42:13AM +0000, Tvrtko Ursulin wrote:
>
> Hi,
>
> On 18/11/2016 19:53, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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.
> >
> > Cc: Ben Widawsky <ben@xxxxxxxxxxxx>
> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 37 ++++++++++++++++++++++++++++++++++++
> > include/uapi/drm/drm_fourcc.h | 3 +++
> > 2 files changed, 40 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 7b7135be3b9e..de6909770c68 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2488,6 +2488,42 @@ static unsigned int intel_fb_modifier_to_tiling(uint64_t fb_modifier)
> > }
> > }
> >
> > +static const struct drm_format_info ccs_formats[] = {
> > + { .format = DRM_FORMAT_XRGB8888, .depth = 24, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 16, .vsub = 8, },
> > + { .format = DRM_FORMAT_XBGR8888, .depth = 24, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 16, .vsub = 8, },
> > + { .format = DRM_FORMAT_ARGB8888, .depth = 32, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 16, .vsub = 8, },
> > + { .format = DRM_FORMAT_ABGR8888, .depth = 32, .num_planes = 2, .cpp = { 4, 1, }, .hsub = 16, .vsub = 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 = 0; i < num_formats; i++) {
> > + if (formats[i].format == 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)
> > @@ -15922,6 +15958,7 @@ intel_user_framebuffer_create(struct drm_device *dev,
> >
> > static const struct drm_mode_config_funcs intel_mode_funcs = {
> > .fb_create = intel_user_framebuffer_create,
> > + .get_format_info = intel_get_format_info,
> > .output_poll_changed = intel_fbdev_output_poll_changed,
> > .atomic_check = intel_atomic_check,
> > .atomic_commit = intel_atomic_commit,
> > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
> > index a5890bf44c0a..2926d916f199 100644
> > --- a/include/uapi/drm/drm_fourcc.h
> > +++ b/include/uapi/drm/drm_fourcc.h
> > @@ -218,6 +218,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)
> > +
>
> I think when fb modifiers were started the idea was that we would later
> partition our vendor bit space for different classes of things and have
> helper functions to extract the tiling, etc, from them.
>
> For example have first 3-4 bits represent the tiling, then in this case
> one bit for CCS, etc.
>
> Have you considered that when adding these ones, and concluded this
> different scheme is better for some reason?
I haven't considered anything. And obviously this patch isn't meant
for inclusion as is. I just needed sometime to make it compile.
Generally I don't think adding magic meaning for individual bits for
things like this is a particularly good idea. Every time I've seen a
scheme like that it has eventually turned ugly on account of running
out of bits in one place or another.
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
