On 21/11/2016 13:27, Ville Syrjälä wrote: > On Mon, Nov 21, 2016 at 08:42:13AM +0000, Tvrtko Ursulin wrote: >> >> Hi, >> >> On 18/11/2016 19:53, ville.syrjala at linux.intel.com wrote: >>> From: Ville Syrjälä <ville.syrjala at linux.intel.com> >>> >>> 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 at bwidawsk.net> >>> Cc: intel-gfx at lists.freedesktop.org >>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com> >>> --- >>> 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.
No idea on the status of this series. Just noticed new modifiers by accident and remembered the early discussions. > 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. I think in this case it might be much better. You just need one more feature which intersects with tiling and ccs to make the list not very manageable. Regards, Tvrtko